Bug 208087

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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2020-02-21 20:54:28 PST
Implement SPI for the platform authenticator.
Comment 1 Jiewen Tan 2020-02-21 20:54:52 PST
<rdar://problem/59369305>.
Comment 2 Jiewen Tan 2020-02-21 21:15:10 PST
Created attachment 391449 [details]
Patch
Comment 3 Jiewen Tan 2020-02-21 21:21:51 PST
Created attachment 391450 [details]
Patch
Comment 4 Jiewen Tan 2020-02-22 21:27:47 PST
Created attachment 391475 [details]
Patch
Comment 5 Jiewen Tan 2020-02-22 21:44:46 PST
Created attachment 391476 [details]
Patch
Comment 6 Jiewen Tan 2020-02-22 21:59:12 PST
Created attachment 391478 [details]
Patch
Comment 7 Jiewen Tan 2020-02-23 00:09:25 PST
Created attachment 391482 [details]
Patch
Comment 8 Jiewen Tan 2020-02-23 11:51:18 PST
Created attachment 391496 [details]
Patch
Comment 9 Jiewen Tan 2020-02-23 12:23:21 PST
Created attachment 391497 [details]
Patch
Comment 10 Brent Fulgham 2020-02-24 10:17:06 PST
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 11 Jiewen Tan 2020-02-24 14:13:38 PST
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.
Comment 12 Jiewen Tan 2020-02-24 15:05:42 PST
Created attachment 391586 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2020-02-24 15:51:22 PST
Comment on attachment 391586 [details]
Patch for landing

Clearing flags on attachment: 391586

Committed r257269: <https://trac.webkit.org/changeset/257269>
Comment 14 Jiewen Tan 2020-02-25 13:41:34 PST Comment hidden (obsolete)
Comment 15 Jiewen Tan 2020-02-25 13:45:09 PST
Here is the actual build fix:
https://trac.webkit.org/changeset/257371/webkit