Update Credential Management API for WebAuthentication
<rdar://problem/36055239>
Created attachment 330026 [details] Patch
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?
(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.
Created attachment 330057 [details] Part 1/2
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?
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.
Committed r226245: <https://trac.webkit.org/changeset/226245>
Reopening to attach new patch.
Created attachment 330072 [details] Part 2/2
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!
Comment on attachment 330057 [details] Part 1/2 Clearing Dan's review flag, since this piece has already been landed.
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?
Created attachment 330078 [details] Part 2/2
Created attachment 330080 [details] Part 2/2
Created attachment 330084 [details] Part 2/2
EWS Mac bots don't allow me to move weakThis. Not sure what's wrong with them... Copy by value then.
Created attachment 330088 [details] Part 2/2
Created attachment 330093 [details] Part 2/2
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
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
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
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
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
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
Created attachment 330112 [details] Part 2/2
Created attachment 330224 [details] Part 2/2
(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!
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!
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.
Comment on attachment 330224 [details] Part 2/2 Clearing flags on attachment: 330224 Committed r226332: <https://trac.webkit.org/changeset/226332>
All reviewed patches have been landed. Closing bug.