Bug 198176 - [WebAuthn] Use WebPreferences instead of RuntimeEnabledFeatures in UIProcess
Summary: [WebAuthn] Use WebPreferences instead of RuntimeEnabledFeatures in UIProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2019-05-23 00:43 PDT by Jiewen Tan
Modified: 2019-09-17 01:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (36.73 KB, patch)
2019-09-11 18:41 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (35.43 KB, patch)
2019-09-16 16:56 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-05-23 00:43:29 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-09-11 18:28:23 PDT
<rdar://problem/55285709>
Comment 2 Jiewen Tan 2019-09-11 18:41:53 PDT
Created attachment 378611 [details]
Patch
Comment 3 youenn fablet 2019-09-12 03:07:50 PDT
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?
Comment 4 Jiewen Tan 2019-09-12 11:59:51 PDT
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.
Comment 5 youenn fablet 2019-09-13 05:46:05 PDT
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.
Comment 6 Jiewen Tan 2019-09-16 16:40:22 PDT
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.
Comment 7 Jiewen Tan 2019-09-16 16:56:18 PDT
Created attachment 378914 [details]
Patch
Comment 8 youenn fablet 2019-09-16 17:08:44 PDT
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
Comment 9 Jiewen Tan 2019-09-16 17:28:18 PDT
(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.
Comment 10 Jiewen Tan 2019-09-17 00:33:21 PDT
Comment on attachment 378914 [details]
Patch

The wincairo and mac-debug-wk1 failures don't seem to be related. cq+.
Comment 11 WebKit Commit Bot 2019-09-17 01:17:24 PDT
Comment on attachment 378914 [details]
Patch

Clearing flags on attachment: 378914

Committed r249949: <https://trac.webkit.org/changeset/249949>
Comment 12 WebKit Commit Bot 2019-09-17 01:17:25 PDT
All reviewed patches have been landed.  Closing bug.