[webkit-reviews] review granted: [Bug 208087] [WebAuthn] Implement SPI for the platform authenticator : [Attachment 391497] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 208087: [WebAuthn] Implement SPI for the platform authenticator
https://bugs.webkit.org/show_bug.cgi?id=208087

Attachment 391497: Patch

https://bugs.webkit.org/attachment.cgi?id=391497&action=review




--- Comment #10 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 391497
  --> https://bugs.webkit.org/attachment.cgi?id=391497
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.