Bug 191535

<rdar://problem/47102027>

Created attachment 358560 [details] Patch

Comment on attachment 358560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358560&action=review > Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidAuthenticator.cpp:1 > +/* No changes to the file. Just renaming. > Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidAuthenticator.h:1 > +/* No changes to the file. Just renaming. > Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidDriver.cpp:1 > +/* No changes to the file. Just renaming. > Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidDriver.h:1 > +/* No changes to the file. Just renaming.

Created attachment 358561 [details] Patch

Created attachment 358578 [details] Patch

It looks like that our svn system doesn't like case sensitive renaming for folders, i.e. fido => Fido. I will revert the renaming change then.

Created attachment 358633 [details] Patch

Comment on attachment 358633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358633&action=review Looks good, but you may need to land it manually (iOS project file seems unhappy). > Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:120 > // FIXME(191535): Support U2F authenticators. Do you still need this comment? If you do, I don't think the Bugzilla # is right! :-) > Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:138 > + if (m_requestMessage->cmd() == FidoHidDeviceCommand::kCbor) { Are there other request messages we are supporting (besides kCbor)? If not, consider early return here. > Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:219 > + message = FidoHidMessage::create(m_currentChannel, FidoHidDeviceCommand::kError, { static_cast<uint8_t>(CtapDeviceResponseCode::kCtap1ErrInvalidCommand) }); Does this cause a console message a developer can see? We might need good logging here to explain that we don't support older device protocols. > Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:52 > + // FIXME(191520): We need a way to convert std::unique_ptr to UniqueRef. You should talk to David Kilzer about that one...

Comment on attachment 358633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358633&action=review Thanks Brent for r+ this patch. I will figure out what's going on with iOS bots before landing this patch. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:120 >> // FIXME(191535): Support U2F authenticators. > > Do you still need this comment? If you do, I don't think the Bugzilla # is right! :-) Yup, we don't need that anymore. >> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:138 >> + if (m_requestMessage->cmd() == FidoHidDeviceCommand::kCbor) { > > Are there other request messages we are supporting (besides kCbor)? If not, consider early return here. We anticipate other request messages as well. kMsg for U2F, and kCbor for CTAP. Therefore, we can't add early return. >> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:219 >> + message = FidoHidMessage::create(m_currentChannel, FidoHidDeviceCommand::kError, { static_cast<uint8_t>(CtapDeviceResponseCode::kCtap1ErrInvalidCommand) }); > > Does this cause a console message a developer can see? We might need good logging here to explain that we don't support older device protocols. Oh no, this is mocking a U2F authenticator's response to a kCbor(CTAP) msg command. After this patch, we will support both U2F and CTAP authenticators, and therefore we don't need to tell developers we don't support old protocols anymore. :) >> Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:52 >> + // FIXME(191520): We need a way to convert std::unique_ptr to UniqueRef. > > You should talk to David Kilzer about that one... Will be glad to.

Created attachment 358644 [details] Patch for landing

Comment on attachment 358644 [details] Patch for landing Clearing flags on attachment: 358644 Committed r239752: <https://trac.webkit.org/changeset/239752>

The new test added in https://trac.webkit.org/changeset/239752/webkit http/wpt/webauthn/public-key-credential-get-success-u2f.https.html is a flakey timeout and failure. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-u2f.https.html Diff: --- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-u2f.https-expected.txt +++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-u2f.https-actual.txt @@ -1,5 +1,5 @@ PASS PublicKeyCredential's [[get]] with minimum options in a mock hid authenticator. -PASS PublicKeyCredential's [[get]] with more allow credentials in a mock hid authenticator. +FAIL PublicKeyCredential's [[get]] with more allow credentials in a mock hid authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out." PASS PublicKeyCredential's [[get]] with test of user presence in a mock hid authenticator.

Additionally: http/wpt/webauthn/public-key-credential-get-failure-u2f-silent.https.html Is a flaky timeout: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-u2f-silent.https.html http/wpt/webauthn/public-key-credential-create-success-u2f.https.html is a flaky failure: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-u2f.https.html

(In reply to Truitt Savell from comment #13) > Additionally: > > http/wpt/webauthn/public-key-credential-get-failure-u2f-silent.https.html Is > a flaky timeout: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential- > get-failure-u2f-silent.https.html > > > > http/wpt/webauthn/public-key-credential-create-success-u2f.https.html is a > flaky failure: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential- > create-success-u2f.https.html Don't worry. They will be covered by Bug 192061.