Bug 208087 – [WebAuthn] Implement SPI for the platform authenticator

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Implement SPI for the platform authenticator.

[Jiewen Tan]

<rdar://problem/59369305>.

[Jiewen Tan]

Created attachment 391449 [details]
Patch

[Jiewen Tan]

Created attachment 391450 [details]
Patch

[Jiewen Tan]

Created attachment 391475 [details]
Patch

[Jiewen Tan]

Created attachment 391476 [details]
Patch

[Jiewen Tan]

Created attachment 391478 [details]
Patch

[Jiewen Tan]

Created attachment 391482 [details]
Patch

[Jiewen Tan]

Created attachment 391496 [details]
Patch

[Jiewen Tan]

Created attachment 391497 [details]
Patch

[Brent Fulgham]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 391586 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391586 [details]
Patch for landing

Clearing flags on attachment: 391586

Committed r257269: <https://trac.webkit.org/changeset/257269>

[Jiewen Tan]

A build fix:
https://trac.webkit.org/changeset/257269/webkit

[Jiewen Tan]

Here is the actual build fix:
https://trac.webkit.org/changeset/257371/webkit