Summary: | [WebAuthn] Implement SPI for the platform authenticator | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, jiewen_tan, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 181943 | ||||||||||||||||||||||
Attachments: |
|
Description
Jiewen Tan
2020-02-21 20:54:28 PST
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 |