[WebAuthn] Obtain consent to create new credential when platform auth…

…enticator 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@apple.com> 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@apple.com> on 2021-10-18 Reviewed by Brent Fulgham. * TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm: (-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]): (TestWebKitAPI::TEST): Canonical link: https://commits.webkit.org/243182@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284413 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 19, 2021 · 3fe4e2ad09b65d01fc90bf420815e73f00b34c4f · 3fe4e2a
1 parent 9f9d8fc
commit 3fe4e2ad09b65d01fc90bf420815e73f00b34c4f
Showing 9 changed files with 91 additions and 5 deletions.
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local-silent.https.html b/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local-silent.https.html
@@ -50,7 +50,7 @@
         };
         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);
         });
@@ -84,7 +84,7 @@
         };
         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);
         });

diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html b/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html
@@ -48,7 +48,7 @@
         };
         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);
         });
@@ -81,7 +81,7 @@
         };
         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);
         });

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,27 @@
+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>
 
         Add a callback to know when the service worker loads via -[WKWebView _loadServiceWorker:]

diff --git a/Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h b/Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h
@@ -51,6 +51,7 @@ typedef NS_ENUM(NSInteger, _WKWebAuthenticationPanelUpdate) {
     _WKWebAuthenticationPanelUpdatePINInvalid,
     _WKWebAuthenticationPanelUpdateLAError,
     _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched,
+    _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent,
     _WKWebAuthenticationPanelUpdateLANoCredential,
 } WK_API_AVAILABLE(macos(10.15.4), ios(13.4));
 

diff --git a/Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm b/Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm
@@ -227,7 +227,18 @@ static inline bool emptyTransportsOrContain(const Vector<AuthenticatorTransport>
             ASSERT(rawId);
             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;
         }
     }

diff --git a/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm b/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
@@ -74,6 +74,8 @@ static _WKWebAuthenticationPanelUpdate wkWebAuthenticationPanelUpdate(WebAuthent
         return _WKWebAuthenticationPanelUpdateLAError;
     if (status == WebAuthenticationStatus::LAExcludeCredentialsMatched)
         return _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched;
+    if (status == WebAuthenticationStatus::LAExcludeCredentialsMatchedWithConsent)
+        return _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent;
     if (status == WebAuthenticationStatus::LANoCredential)
         return _WKWebAuthenticationPanelUpdateLANoCredential;
     ASSERT_NOT_REACHED();

diff --git a/Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h b/Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h
@@ -48,6 +48,7 @@ enum class WebAuthenticationStatus : uint8_t {
     PinInvalid,
     LAError,
     LAExcludeCredentialsMatched,
+    LAExcludeCredentialsMatchedWithConsent,
     LANoCredential,
 };
 

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,22 @@
+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>
 
         [ iOS15 ] TestWebKitAPI.WebKit.HTTPSProxy is a flaky, but frequent, timeout

diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm
@@ -70,6 +70,7 @@
 static bool webAuthenticationPanelUpdatePINInvalid = false;
 static bool webAuthenticationPanelUpdateLAError = false;
 static bool webAuthenticationPanelUpdateLAExcludeCredentialsMatched = false;
+static bool webAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent = false;
 static bool webAuthenticationPanelUpdateLANoCredential = false;
 static bool webAuthenticationPanelCancelImmediately = false;
 static _WKLocalAuthenticatorPolicy localAuthenticatorPolicy = _WKLocalAuthenticatorPolicyDisallow;
@@ -122,6 +123,10 @@ - (void)panel:(_WKWebAuthenticationPanel *)panel updateWebAuthenticationPanel:(_
         webAuthenticationPanelUpdateLAExcludeCredentialsMatched = true;
         return;
     }
+    if (update == _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent) {
+        webAuthenticationPanelUpdateLAExcludeCredentialsMatchedWithConsent = true;
+        return;
+    }
     if (update == _WKWebAuthenticationPanelUpdateLANoCredential) {
         webAuthenticationPanelUpdateLANoCredential = true;
         return;
@@ -1386,6 +1391,29 @@ HTTPServer server([parentFrame = String(parentFrame), subFrame = String(subFrame
     cleanUpKeychain("");
 }
 
+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)
 {
     reset();