Bug 181082 – Update Credential Management API for WebAuthentication

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Update Credential Management API for WebAuthentication

[Jiewen Tan]

<rdar://problem/36055239>

[Jiewen Tan]

Created attachment 330026 [details]
Patch

[Brent Fulgham]

Comment on attachment 330026 [details]
Patch

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

r- because the patch doesn't apply to the current tree (so no ews results), and because it is very large. Please break it into two steps as I suggest in the review notes below.

> Source/WebCore/ChangeLog:15
> +        5. Set runtime flag of Credential Management API in order to enable testing.

I think this would be easier to review if we did it in steps.

Can you have a "patch 1" that just moves the folder from credentials -> credentialsmanagement, and removes the useless dummy implementations.
Then, have a "patch 2" that adds new IDLs for WebAuthN, add the new dummy functions, and add the runtime flag?

[Jiewen Tan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 330026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330026&action=review
> 
> r- because the patch doesn't apply to the current tree (so no ews results),
> and because it is very large. Please break it into two steps as I suggest in
> the review notes below.
> 
> > Source/WebCore/ChangeLog:15
> > +        5. Set runtime flag of Credential Management API in order to enable testing.
> 
> I think this would be easier to review if we did it in steps.
> 
> Can you have a "patch 1" that just moves the folder from credentials ->
> credentialsmanagement, and removes the useless dummy implementations.
> Then, have a "patch 2" that adds new IDLs for WebAuthN, add the new dummy
> functions, and add the runtime flag?

NP.

[Jiewen Tan]

Created attachment 330057 [details]
Part 1/2

[Daniel Bates]

Comment on attachment 330057 [details]
Part 1/2

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

> Source/WebCore/ChangeLog:3
> +        [Part 1/2] Update Credential Management API for WebAuthentication

Typically we put the  "Part 1/2" above the "Reviewed by" line and have this line match the title of the bug verbatim.

> LayoutTests/ChangeLog:9
> +        * credentials/idlharness-expected.txt:

Should we rename the directory from credentials to credentialmanagement?

> LayoutTests/credentials/idlharness-expected.txt:37
> +FAIL PasswordCredential interface: existence and properties of interface object assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface object length assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface object name assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface: existence and properties of interface prototype object assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface: existence and properties of interface prototype object's "constructor" property assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface: attribute password assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> +FAIL PasswordCredential interface: attribute name assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing

Would it be bad to remove all the password credential and password credential-related tests from this file instead of landing expected failure results?

[Jiewen Tan]

Comment on attachment 330057 [details]
Part 1/2

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

Thanks Dan for r+ my patch.

>> Source/WebCore/ChangeLog:3
>> +        [Part 1/2] Update Credential Management API for WebAuthentication
> 
> Typically we put the  "Part 1/2" above the "Reviewed by" line and have this line match the title of the bug verbatim.

I assume you mean below?

>> LayoutTests/ChangeLog:9
>> +        * credentials/idlharness-expected.txt:
> 
> Should we rename the directory from credentials to credentialmanagement?

No need. My second patch will move the whole tests into wpt tests.

>> LayoutTests/credentials/idlharness-expected.txt:37
>> +FAIL PasswordCredential interface: attribute name assert_own_property: self does not have own property "PasswordCredential" expected property "PasswordCredential" missing
> 
> Would it be bad to remove all the password credential and password credential-related tests from this file instead of landing expected failure results?

Ditto.

[Jiewen Tan]

Committed r226245: <https://trac.webkit.org/changeset/226245>

[Jiewen Tan]

Reopening to attach new patch.

[Jiewen Tan]

Created attachment 330072 [details]
Part 2/2

[Brent Fulgham]

Comment on attachment 330072 [details]
Part 2/2

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

I think this looks good, but I had some wording changes and a method name change I'd like you to make. Also, can you please explain the std::optional change I asked about in the notes below?

> Source/WebCore/ChangeLog:12
> +        which is required by WebAuthN. Besides, it also sets the CredentialManagement runtime flag to enable testing.

You don't need 'Besides' here.

> Source/WebCore/ChangeLog:13
> +        Noted, it introduces a dummy PublicKeyCredential interface for testing functionalities of the Credential interface,

'Noted, it introduces' should be 'Note that it introduces'

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:48
> +bool CredentialsContainer::isSameOriginWithItsAncestors()

I think this should be called 'doesHaveSameOriginAsItsAncestors()'

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
> +    auto task = [promiseIndex, weakThis, isSameOriginWithItsAncestors = isSameOriginWithItsAncestors(), operation = WTFMove(operation)] () {

Should we be using WTFMove() on the weakThis?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:85
> +    // FIXME: Optional options are passed with no contents. It should be std::optional.

I don't understand this comment. You seem to be changing this code to pass CredentialRequestOptions directly, from a std::optional argument.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:-41
> -    void get(std::optional<CredentialRequestOptions>, DOMPromiseDeferred<IDLInterface<BasicCredential>>&&);

I don't get why you are making this change, if you want to go back to std::optional. Can you clarify?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:58
> +    bool isSameOriginWithItsAncestors();

I think this should be called 'doesHaveSameOriginAsItsAncestors()'

> Source/WebCore/Modules/credentialmanagement/NavigatorCredentials.cpp:46
> +        m_credentialsContainer = CredentialsContainer::create(frame);

Can we have a CredentialsContainer with a nullptr frame? Is returning a null-constructed CredentialsContainer appropriate?

> Source/WebCore/page/RuntimeEnabledFeatures.h:251
> +    bool m_isCredentialManagementEnabled { true };

I don't think we want to turn this on yet. Test cases should be able to flip this switch on in the test case.

> LayoutTests/imported/w3c/web-platform-tests/credential-management/idl.https-expected.txt:19
> +PASS CredentialsContainer interface: navigator.credentials must inherit property "preventSilentAccess()" with the proper type 

Yay!

[Brent Fulgham]

Comment on attachment 330057 [details]
Part 1/2

Clearing Dan's review flag, since this piece has already been landed.

[Jiewen Tan]

Comment on attachment 330072 [details]
Part 2/2

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

Thanks Brent for reviewing the patch.

>> Source/WebCore/ChangeLog:12
>> +        which is required by WebAuthN. Besides, it also sets the CredentialManagement runtime flag to enable testing.
> 
> You don't need 'Besides' here.

Fixed.

>> Source/WebCore/ChangeLog:13
>> +        Noted, it introduces a dummy PublicKeyCredential interface for testing functionalities of the Credential interface,
> 
> 'Noted, it introduces' should be 'Note that it introduces'

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:48
>> +bool CredentialsContainer::isSameOriginWithItsAncestors()
> 
> I think this should be called 'doesHaveSameOriginAsItsAncestors()'

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>> +    auto task = [promiseIndex, weakThis, isSameOriginWithItsAncestors = isSameOriginWithItsAncestors(), operation = WTFMove(operation)] () {
> 
> Should we be using WTFMove() on the weakThis?

Yes!

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:85
>> +    // FIXME: Optional options are passed with no contents. It should be std::optional.
> 
> I don't understand this comment. You seem to be changing this code to pass CredentialRequestOptions directly, from a std::optional argument.

There seems a bug in our code biding generator such that it generates an empty CredentialRequestOptions object instead of std::nullopt when there is no options passed from JS. Hence, if we use std::optional here, it will always be initialized with that empty object. It makes no point to check options then. To reveal this problem, I explicitly choose to use CredentialRequestOptions&& instead of std::optional<CredentialRequestOptions>. This will help us to confirm a bug fix in the code biding generator later on.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:-41
>> -    void get(std::optional<CredentialRequestOptions>, DOMPromiseDeferred<IDLInterface<BasicCredential>>&&);
> 
> I don't get why you are making this change, if you want to go back to std::optional. Can you clarify?

Ditto.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:58
>> +    bool isSameOriginWithItsAncestors();
> 
> I think this should be called 'doesHaveSameOriginAsItsAncestors()'

Fixed.

>> Source/WebCore/Modules/credentialmanagement/NavigatorCredentials.cpp:46
>> +        m_credentialsContainer = CredentialsContainer::create(frame);
> 
> Can we have a CredentialsContainer with a nullptr frame? Is returning a null-constructed CredentialsContainer appropriate?

I think it might be more appropriate to store a weak pointer of a Document then as the API requires SecureContext and is only exposed in Window. Thanks for pointing it out.

>> Source/WebCore/page/RuntimeEnabledFeatures.h:251
>> +    bool m_isCredentialManagementEnabled { true };
> 
> I don't think we want to turn this on yet. Test cases should be able to flip this switch on in the test case.

Fixed. Could we override it for WebKitTestRunner and DumpRenderTree to make testing easier?

[Jiewen Tan]

Created attachment 330078 [details]
Part 2/2

[Jiewen Tan]

Created attachment 330080 [details]
Part 2/2

[Jiewen Tan]

Created attachment 330084 [details]
Part 2/2

[Jiewen Tan]

EWS Mac bots don't allow me to move weakThis. Not sure what's wrong with them...
Copy by value then.

[Jiewen Tan]

Created attachment 330088 [details]
Part 2/2

[Jiewen Tan]

Created attachment 330093 [details]
Part 2/2

[EWS Watchlist]

Comment on attachment 330093 [details]
Part 2/2

Attachment 330093 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5797277

New failing tests:
fast/dom/navigator-detached-no-crash.html

[EWS Watchlist]

Created attachment 330098 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330093 [details]
Part 2/2

Attachment 330093 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5797287

New failing tests:
fast/dom/navigator-detached-no-crash.html

[EWS Watchlist]

Created attachment 330099 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330093 [details]
Part 2/2

Attachment 330093 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5797377

New failing tests:
fast/dom/navigator-detached-no-crash.html

[EWS Watchlist]

Created attachment 330102 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jiewen Tan]

Created attachment 330112 [details]
Part 2/2

[Jiewen Tan]

Created attachment 330224 [details]
Part 2/2

[Brent Fulgham]

(In reply to Jiewen Tan from comment #17)
> EWS Mac bots don't allow me to move weakThis. Not sure what's wrong with
> them...
> Copy by value then.

Interesting. We should show Chris, JF, or somebody with strong C++ knowledge to see what's going on!

[Brent Fulgham]

Comment on attachment 330224 [details]
Part 2/2

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

Thanks for your work on this, Jiewen! r=me.

> Source/WebCore/Modules/credentialmanagement/CredentialCreationOptions.idl:29
> +    boolean publicKey; // fake for now

Is this just a one-bit payload that may change to a larger data type in the future? I'm not sure what 'fake for now' means here.

> LayoutTests/imported/w3c/web-platform-tests/credential-management/idl.https-expected.txt:19
> +PASS CredentialsContainer interface: navigator.credentials must inherit property "preventSilentAccess()" with the proper type 

Yay!

[Jiewen Tan]

Comment on attachment 330224 [details]
Part 2/2

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

Thanks Brent for r+ my patch.

>> Source/WebCore/Modules/credentialmanagement/CredentialCreationOptions.idl:29
>> +    boolean publicKey; // fake for now
> 
> Is this just a one-bit payload that may change to a larger data type in the future? I'm not sure what 'fake for now' means here.

The actual type for the publicKey member is quite complicated. I am just faking the type for this patch to avoid extra complexity.

[WebKit Commit Bot]

Comment on attachment 330224 [details]
Part 2/2

Clearing flags on attachment: 330224

Committed r226332: <https://trac.webkit.org/changeset/226332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.