Implement SPI for the platform authenticator.
<rdar://problem/59369305>.
Created attachment 391449 [details] Patch
Created attachment 391450 [details] Patch
Created attachment 391475 [details] Patch
Created attachment 391476 [details] Patch
Created attachment 391478 [details] Patch
Created attachment 391482 [details] Patch
Created attachment 391496 [details] Patch
Created attachment 391497 [details] Patch
Comment on attachment 391497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391497&action=review I have a few suggestions about the code, but I think this looks good. > Source/WebKit/ChangeLog:29 > + 1) _WKWebAuthenticationPanelUpdate: three error are added to help clients to inform users. Three ERRORS are added to help clients present meaningful error messages to users. > Source/WebKit/ChangeLog:34 > + returned during makeCredential and before verifyUserWithAccessControl delegate. Does the WebAuthn spec give much guidance on what should happen if a user attempts to create a new credential on a site where they already have one? Could the user be trying to replace a credential for some reason? > Source/WebKit/ChangeLog:50 > + wait until verifyUserWithAccessControl to show the combined UI. If any of the LAError is received before If any of the LAError STATES ARE received before > Source/WebKit/ChangeLog:51 > + verifyUserWithAccessControl, clients can then only show the external authenticator UI. Also, platform authenticator and I think this should be: "... clients SHOULD THEN show the external authenticator UI." > Source/WebKit/ChangeLog:52 > + external authenticators are being discovered at the same time, which means, a user can plug in a security key at anytime. Extra comma: "same time, which means a user can plug in a security key at any time." > Source/WebKit/ChangeLog:55 > + Besides introducing the SPI, and all the plumbings to make it happen. This patch also: and all the NECESSARY PLUMBING, this patch also: > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:56 > +void AuthenticatorAssertionResponse::setAuthenticatorData(const Vector<uint8_t>& authenticatorData) I think this should be a Vector<uint8_t>&&. > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:58 > + m_authenticatorData = ArrayBuffer::create(authenticatorData.data(), authenticatorData.size()); Do we have a 'create' that reuses the passed memory? It would be a nice improvement. > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:51 > + WEBCORE_EXPORT void setAuthenticatorData(const Vector<uint8_t>&); Propose Vector<uint8_t>&& > Source/WebKit/Platform/spi/Cocoa/LocalAuthenticationSPI.h:37 > + LAOptionNotInteractive, It's a little funny to have an enum with a single value! :-) > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:300 > + RetainPtr<SecAccessControlRef> accessControl = accessControlRef; Minor nit: If we always take ownership when we enter this function, should we just pass a RetainPtr<SecAccessControlRef>&&? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:496 > + response->setAuthenticatorData(authData); Seems like we should WTFMove() the authData, since we create it in the method, and never use it outside of the response object.
Comment on attachment 391497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391497&action=review Thanks Brent for r+ this patch. >> Source/WebKit/ChangeLog:29 >> + 1) _WKWebAuthenticationPanelUpdate: three error are added to help clients to inform users. > > Three ERRORS are added to help clients present meaningful error messages to users. Thanks. I don't know from where I got the impression that error is plural. >> Source/WebKit/ChangeLog:34 >> + returned during makeCredential and before verifyUserWithAccessControl delegate. > > Does the WebAuthn spec give much guidance on what should happen if a user attempts to create a new credential on a site where they already have one? Could the user be trying to replace a credential for some reason? The default behavior is to overwrite the existing one with the new one. Therefore, the excludeList is provided as a way to prevent the overwriting. >> Source/WebKit/ChangeLog:50 >> + wait until verifyUserWithAccessControl to show the combined UI. If any of the LAError is received before > > If any of the LAError STATES ARE received before Fixed. >> Source/WebKit/ChangeLog:51 >> + verifyUserWithAccessControl, clients can then only show the external authenticator UI. Also, platform authenticator and > > I think this should be: "... clients SHOULD THEN show the external authenticator UI." Fixed. >> Source/WebKit/ChangeLog:52 >> + external authenticators are being discovered at the same time, which means, a user can plug in a security key at anytime. > > Extra comma: "same time, which means a user can plug in a security key at any time." Fixed. >> Source/WebKit/ChangeLog:55 >> + Besides introducing the SPI, and all the plumbings to make it happen. This patch also: > > and all the NECESSARY PLUMBING, this patch also: Fixed. >> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:56 >> +void AuthenticatorAssertionResponse::setAuthenticatorData(const Vector<uint8_t>& authenticatorData) > > I think this should be a Vector<uint8_t>&&. Fixed. >> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:58 >> + m_authenticatorData = ArrayBuffer::create(authenticatorData.data(), authenticatorData.size()); > > Do we have a 'create' that reuses the passed memory? It would be a nice improvement. So unfortunate that there isn't one. >> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.h:51 >> + WEBCORE_EXPORT void setAuthenticatorData(const Vector<uint8_t>&); > > Propose Vector<uint8_t>&& Fixed. >> Source/WebKit/Platform/spi/Cocoa/LocalAuthenticationSPI.h:37 >> + LAOptionNotInteractive, > > It's a little funny to have an enum with a single value! :-) Right given I don't want to expose others... >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:300 >> + RetainPtr<SecAccessControlRef> accessControl = accessControlRef; > > Minor nit: If we always take ownership when we enter this function, should we just pass a RetainPtr<SecAccessControlRef>&&? It is a copy instead of a transfer. I think passing a SecAccessControlRef is better than a const RetainPtr<SecAccessControlRef>&. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:496 >> + response->setAuthenticatorData(authData); > > Seems like we should WTFMove() the authData, since we create it in the method, and never use it outside of the response object. Fixed.
Created attachment 391586 [details] Patch for landing
Comment on attachment 391586 [details] Patch for landing Clearing flags on attachment: 391586 Committed r257269: <https://trac.webkit.org/changeset/257269>
A build fix: https://trac.webkit.org/changeset/257269/webkit
Here is the actual build fix: https://trac.webkit.org/changeset/257371/webkit