Changeset 284413 in webkit

Timestamp:

Oct 18, 2021 5:07:29 PM (9 months ago)

Author:

commit-queue@webkit.org

Message:

[WebAuthn] Obtain consent to create new credential when platform authenticator in excludedCredentials
​https://bugs.webkit.org/show_bug.cgi?id=219813
<rdar://problem/72484635>

Currently, whenever the platform authenticator is within excludedCredentials during makeCredential we
always return NotAllowedError and merely flash a consent screen. This does not match the spec per Step 3.1
of makeCredential (​https://w3c.github.io/webauthn/#sctn-op-make-cred). Instead, we should always obtain consent
and return a different error depending on consent was obtained.

Source/WebKit:

A fixme to add this was inadvertently removed in ​https://bugs.webkit.org/attachment.cgi?id=393180&action=prettypatch

Patch by John Pascoe <J Pascoe> on 2021-10-18
Reviewed by Brent Fulgham.

Added api test TestWebKitAPI.WebAuthenticationPanel.LADuplicateCredentialWithConsent

• UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:
• UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:

(WebKit::LocalAuthenticator::makeCredential):

• UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:

(WebKit::wkWebAuthenticationPanelUpdate):

• UIProcess/WebAuthentication/WebAuthenticationFlags.h:

Tools:

This adds a test to confirm a different path is taken whenever consent is obtained.

Patch by John Pascoe <J Pascoe> on 2021-10-18
Reviewed by Brent Fulgham.

• TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:

(-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]):
(TestWebKitAPI::TEST):

Location:

trunk

Files:

• LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local-silent.https.html (modified) (2 diffs)
• LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h (modified) (1 diff)
• Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm (modified) (1 diff)
• Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm (modified) (1 diff)
• Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm (modified) (3 diffs)

trunk/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local-silent.https.html

@@ -51 +51 @@
         if (window.testRunner)
             testRunner.addTestKeyToKeychain(privateKeyBase64, testRpId, testUserEntityBundleBase64);
-        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(options), "Operation timed out.").then(() => {
+        return promiseRejects(t, "InvalidStateError", navigator.credentials.create(options), "Operation timed out.").then(() => {
             if (window.testRunner)
                 testRunner.cleanUpKeychain(testRpId, credentialIDBase64);
@@ -85 +85 @@
         if (window.testRunner)
             testRunner.addTestKeyToKeychain(privateKeyBase64, testRpId, testUserEntityBundleBase64);
-        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(options), "Operation timed out.").then(() => {
+        return promiseRejects(t, "InvalidStateError", navigator.credentials.create(options), "Operation timed out.").then(() => {
             if (window.testRunner)
                 testRunner.cleanUpKeychain(testRpId, credentialIDBase64);

trunk/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html

@@ -49 +49 @@
         if (window.testRunner)
             testRunner.addTestKeyToKeychain(privateKeyBase64, testRpId, testUserEntityBundleBase64);
-        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(options), "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator.").then(() => {
+        return promiseRejects(t, "InvalidStateError", navigator.credentials.create(options), "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator.").then(() => {
             if (window.testRunner)
                 testRunner.cleanUpKeychain(testRpId, credentialIDBase64);
@@ -82 +82 @@
         if (window.testRunner)
             testRunner.addTestKeyToKeychain(privateKeyBase64, testRpId, testUserEntityBundleBase64);
-        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(options), "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator.").then(() => {
+        return promiseRejects(t, "InvalidStateError", navigator.credentials.create(options), "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator.").then(() => {
             if (window.testRunner)
                 testRunner.cleanUpKeychain(testRpId, credentialIDBase64);

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-10-18  John Pascoe  <j_pascoe@apple.com>
+
+        [WebAuthn] Obtain consent to create new credential when platform authenticator in excludedCredentials
+        https://bugs.webkit.org/show_bug.cgi?id=219813
+        <rdar://problem/72484635>
+
+        Currently, whenever the platform authenticator is within excludedCredentials during makeCredential we
+        always return NotAllowedError and merely flash a consent screen. This does not match the spec per Step 3.1
+        of makeCredential (https://w3c.github.io/webauthn/#sctn-op-make-cred). Instead, we should always obtain consent
+        and return a different error depending on consent was obtained.
+
+        A fixme to add this was inadvertently removed in  https://bugs.webkit.org/attachment.cgi?id=393180&action=prettypatch
+
+        Reviewed by Brent Fulgham.
+
+        Added api test TestWebKitAPI.WebAuthenticationPanel.LADuplicateCredentialWithConsent 
+
+        * UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:
+        * UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:
+        (WebKit::LocalAuthenticator::makeCredential):
+        * UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:
+        (WebKit::wkWebAuthenticationPanelUpdate):
+        * UIProcess/WebAuthentication/WebAuthenticationFlags.h:
+
 2021-10-18  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h

@@ -52 +52 @@
     _WKWebAuthenticationPanelUpdateLAError,
     _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched,
+    _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent,
     _WKWebAuthenticationPanelUpdateLANoCredential,
 } WK_API_AVAILABLE(macos(10.15.4), ios(13.4));

trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm

@@ -228 +228 @@
             return excludeCredentialIds.contains(base64EncodeToString(rawId->data(), rawId->byteLength()));
         })) {
-            receiveException({ NotAllowedError, "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator."_s }, WebAuthenticationStatus::LAExcludeCredentialsMatched);
+            // Obtain consent per Step 3.1
+            auto callback = [weakThis = WeakPtr { *this }] (LocalAuthenticatorPolicy policy) {
+                ASSERT(RunLoop::isMain());
+                if (!weakThis)
+                    return;
+
+                if (policy == LocalAuthenticatorPolicy::Allow)
+                    weakThis->receiveException({ InvalidStateError, "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator."_s }, WebAuthenticationStatus::LAExcludeCredentialsMatchedWithConsent);
+                else
+                    weakThis->receiveException({ NotAllowedError, "At least one credential matches an entry of the excludeCredentials list in the platform attached authenticator."_s }, WebAuthenticationStatus::LAExcludeCredentialsMatched);
+            };
+            observer()->decidePolicyForLocalAuthenticator(WTFMove(callback));
             return;
         }

trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm

@@ -75 +75 @@
     if (status == WebAuthenticationStatus::LAExcludeCredentialsMatched)
         return _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched;
+    if (status == WebAuthenticationStatus::LAExcludeCredentialsMatchedWithConsent)
+        return _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent;
     if (status == WebAuthenticationStatus::LANoCredential)
         return _WKWebAuthenticationPanelUpdateLANoCredential;

trunk/Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h

@@ -49 +49 @@
     LAError,
     LAExcludeCredentialsMatched,
+    LAExcludeCredentialsMatchedWithConsent,
     LANoCredential,
 };

trunk/Tools/ChangeLog

@@ +1 @@
+2021-10-18  John Pascoe  <j_pascoe@apple.com>
+
+        [WebAuthn] Obtain consent to create new credential when platform authenticator in excludedCredentials
+        https://bugs.webkit.org/show_bug.cgi?id=219813
+        <rdar://problem/72484635>
+
+        Currently, whenever the platform authenticator is within excludedCredentials during makeCredential we
+        always return NotAllowedError and merely flash a consent screen. This does not match the spec per Step 3.1
+        of makeCredential (https://w3c.github.io/webauthn/#sctn-op-make-cred). Instead, we should always obtain consent
+        and return a different error depending on consent was obtained.
+
+        This adds a test to confirm a different path is taken whenever consent is obtained.
+
+        Reviewed by Brent Fulgham.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
+        (-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]):
+        (TestWebKitAPI::TEST):
+
 2021-10-18  Alex Christensen  <achristensen@webkit.org>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm

@@ -71 +71 @@
 static bool webAuthenticationPanelUpdateLAError = false;
 static bool webAuthenticationPanelUpdateLAExcludeCredentialsMatched = false;
+static bool webAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent = false;
 static bool webAuthenticationPanelUpdateLANoCredential = false;
 static bool webAuthenticationPanelCancelImmediately = false;
@@ -123 +124 @@
         return;
     }
+    if (update == _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent) {
+        webAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent = true;
+        return;
+    }
     if (update == _WKWebAuthenticationPanelUpdateLANoCredential) {
         webAuthenticationPanelUpdateLANoCredential = true;
@@ -1387 +1392 @@
 }
 
+TEST(WebAuthenticationPanel, LADuplicateCredentialWithConsent)
+{
+    reset();
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-make-credential-la-duplicate-credential" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
+    [[configuration preferences] _setEnabled:NO forExperimentalFeature:webAuthenticationModernExperimentalFeature()];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
+    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
+    [webView setUIDelegate:delegate.get()];
+    [webView focus];
+
+    ASSERT_TRUE(addKeyToKeychain(testES256PrivateKeyBase64, "", testUserEntityBundleBase64));
+
+    localAuthenticatorPolicy = _WKLocalAuthenticatorPolicyAllow;
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&webAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent);
+    cleanUpKeychain("");
+}
+
 TEST(WebAuthenticationPanel, LANoCredential)
 {