Bug 198176 – [WebAuthn] Use WebPreferences instead of RuntimeEnabledFeatures in UIProcess

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Move the check for LocalAuthenticator runtime flag from LocalService to WebCore, as the method is only useful in web processes not UI processes. Somehow, it serves my purpose now.

[Radar WebKit Bug Importer]

<rdar://problem/55285709>

[Jiewen Tan]

Created attachment 378611 [details]
Patch

[youenn fablet]

Comment on attachment 378611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378611&action=review

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:142
> +        initTimeOutTimer(options.timeout);

This is done in both cases, can we have this line out of the switch.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:219
> +        if (transport == AuthenticatorTransport::Internal && !m_pendingRequest.preferences->store().getBoolValueForKey(WebPreferencesKey::webAuthenticationLocalAuthenticatorEnabledKey()))

If webAuthenticationLocalAuthenticatorEnabledKey value is false, should we even call startDiscovery?
Could we just return early somewhere higher in the stack, like in handleRequest?
Also, what happens to m_pendingCompletionHandler if we are not starting the discovery, is it called?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:113
> +    HashSet<String> excludeCredentialIds = produceHashSet(WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).excludeCredentials);

auto

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:354
> +    HashSet<String> allowCredentialIds = produceHashSet(WTF::get<PublicKeyCredentialRequestOptions>(requestData().options).allowCredentials);

auto

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:41
> +    RefPtr<WebPreferences> preferences;

Do we really need to have a ref of the preferences?
Can we just pass them along the request data or use it in the right place?

[Jiewen Tan]

Comment on attachment 378611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378611&action=review

Thanks Youenn for taking a look.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:142
>> +        initTimeOutTimer(options.timeout);
> 
> This is done in both cases, can we have this line out of the switch.

I guess we have no way to know the type of the options before the switchOn? Then we don't really have a way to access the options.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:219
>> +        if (transport == AuthenticatorTransport::Internal && !m_pendingRequest.preferences->store().getBoolValueForKey(WebPreferencesKey::webAuthenticationLocalAuthenticatorEnabledKey()))
> 
> If webAuthenticationLocalAuthenticatorEnabledKey value is false, should we even call startDiscovery?
> Could we just return early somewhere higher in the stack, like in handleRequest?
> Also, what happens to m_pendingCompletionHandler if we are not starting the discovery, is it called?

WebAuthn defines multiple transports to communicate with different type of authenticators. webAuthenticationLocalAuthenticatorEnabledKey is only for the Internal transport type which is our platform authenticators, i.e., TouchID/FaceID. We allow USB/NFC by default and therefore this method should continue.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:113
>> +    HashSet<String> excludeCredentialIds = produceHashSet(WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).excludeCredentials);
> 
> auto

Right.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:354
>> +    HashSet<String> allowCredentialIds = produceHashSet(WTF::get<PublicKeyCredentialRequestOptions>(requestData().options).allowCredentials);
> 
> auto

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:41
>> +    RefPtr<WebPreferences> preferences;
> 
> Do we really need to have a ref of the preferences?
> Can we just pass them along the request data or use it in the right place?

Given the request handling happens in async mostly. It is really tedious to capture it in every lambda and pass it along the way. Also, we might need different runtime flag for different features. For example, we might have a flag to turn on and off the UI or part of the UI when we have one.

[youenn fablet]

Comment on attachment 378611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378611&action=review

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:101
> +    for (auto& pubKeyCredParam : WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).pubKeyCredParams) {

Can we refactor the code to pass directly a const PublicKeyCredentialCreationOptions& to makeCredential.
That will allow to move the WTF::get closer to the WTF::hold and remove the question of whether we are always using the right variant.
Can we do the same for getAssertion()?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:176
> +        (id)kSecAttrLabel: WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).rp.id,

We could put WTF::get<PublicKeyCredentialCreationOptions>(requestData().options in a temporary auto& to improve code readability since this is called several times.

[Jiewen Tan]

Comment on attachment 378611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378611&action=review

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:101
>> +    for (auto& pubKeyCredParam : WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).pubKeyCredParams) {
> 
> Can we refactor the code to pass directly a const PublicKeyCredentialCreationOptions& to makeCredential.
> That will allow to move the WTF::get closer to the WTF::hold and remove the question of whether we are always using the right variant.
> Can we do the same for getAssertion()?

The reason why I make WebAuthenticationRequestData is that makeCredential not only requires PublicKeyCredentialCreationOptions but also hash. Instead of passing two parameters, I prefer pass one struct that contains those two.
The reason why I make WebAuthenticationRequestData a private member instead of parameters is that makeCredential is an async operation and consists more than one step: makeCredential => continueMakeCredentialAfterUserConsented => continueMakeCredentialAfterAttested, capturing them in lambdas and passing them along all steps is just too tedious.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:176
>> +        (id)kSecAttrLabel: WTF::get<PublicKeyCredentialCreationOptions>(requestData().options).rp.id,
> 
> We could put WTF::get<PublicKeyCredentialCreationOptions>(requestData().options in a temporary auto& to improve code readability since this is called several times.

Sounds good. Fixed.

[Jiewen Tan]

Created attachment 378914 [details]
Patch

[youenn fablet]

Comment on attachment 378914 [details]
Patch

OK, let's go.
I would continue to reduce the use of WTF::get.
Methods like continueGetAssertionAfterUserConsented could be given the correctly typed values so that only makeCredential/getAssertion and their lambdas would do the WTF::get

[Jiewen Tan]

(In reply to youenn fablet from comment #8)
> Comment on attachment 378914 [details]
> Patch
> 
> OK, let's go.
> I would continue to reduce the use of WTF::get.
> Methods like continueGetAssertionAfterUserConsented could be given the
> correctly typed values so that only makeCredential/getAssertion and their
> lambdas would do the WTF::get

Thanks Youenn for r+ the patch. Maybe I shouldn't use a variant. Anyway, both the old and the new way are not perfect.

[Jiewen Tan]

Comment on attachment 378914 [details]
Patch

The wincairo and mac-debug-wk1 failures don't seem to be related. cq+.

[WebKit Commit Bot]

Comment on attachment 378914 [details]
Patch

Clearing flags on attachment: 378914

Committed r249949: <https://trac.webkit.org/changeset/249949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.