[webauthn] Filter out platform devices for U2F requests.

This requirement is derived from clause 5.1.3.19.1 in the spec:

  "If options.authenticatorSelection.authenticatorAttachment is present
   and its value is not equal to authenticator’s attachment modality,
   continue",

and the fact that there are no U2F platform devices.

This CL also fixes the error message when the all U2F devices are
filtered out by the resident key and user verification criteria. While
the CL (crrev.com/c/949453) introducing these options correctly
documented the intended behavior in a comment in u2f_sign, it did not
actually change the DOMException itself.

Finally, the CL moves related webauthn tests from
credentialmanager_browsertests to webauth_browsertests.

Bug: 803832
Change-Id: Ie8f124af3783c2aaec85abd3baac050cfbb926b7
Reviewed-on: https://chromium-review.googlesource.com/989413
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547664}

chrome/browser/password_manager/credential_manager_browsertest.cc

@@ -31,9 +31,7 @@ namespace {
 
 class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
  public:
-  CredentialManagerBrowserTest() {
-    scoped_feature_list_.InitAndEnableFeature(features::kWebAuth);
-  }
+  CredentialManagerBrowserTest() {}
 
   void SetUpOnMainThread() override {
     PasswordManagerBrowserTestBase::SetUpOnMainThread();
@@ -48,10 +46,6 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     PasswordManagerBrowserTestBase::SetUpCommandLine(command_line);
-
-    // To permit using webauthentication features.
-    command_line->AppendSwitch(
-        switches::kEnableExperimentalWebPlatformFeatures);
   }
 
   // Similarly to PasswordManagerBrowserTestBase::NavigateToFile this is a
@@ -82,56 +76,6 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
     ASSERT_EQ(expect_has_results, result);
   }
 
-  // Attempt to create a publicKeyCredential with an unsupported algorithm type.
-  void CreatePublicKeyCredentialWithUnsupportedAlgorithmAndExpectNotSupported(
-      content::WebContents* web_contents) {
-    std::string result;
-    std::string script =
-        "navigator.credentials.create({ publicKey: {"
-        "  challenge: new TextEncoder().encode('climb a mountain'),"
-        "  rp: { id: 'example.com', name: 'Acme' },"
-        "  user: { "
-        "    id: new TextEncoder().encode('1098237235409872'),"
-        "    name: 'avery.a.jones@example.com',"
-        "    displayName: 'Avery A. Jones', "
-        "    icon: 'https://pics.acme.com/00/p/aBjjjpqPb.png'},"
-        "  pubKeyCredParams: [{ type: 'public-key', alg: '123'}],"
-        "  timeout: 60000,"
-        "  excludeCredentials: [] }"
-        "}).catch(c => window.domAutomationController.send(c.toString()));";
-    ASSERT_TRUE(
-        content::ExecuteScriptAndExtractString(web_contents, script, &result));
-    ASSERT_EQ(
-        "NotSupportedError: Parameters for this operation are not supported.",
-        result);
-  }
-
-
-  // Attempt to create a publicKeyCredential with an invalid relying party.
-  void CreatePublicKeyCredentialWithUnsupportedRpIdAndExpectInvalidRpId(
-      content::WebContents* web_contents) {
-    std::string result;
-    std::string script =
-        "navigator.credentials.create({ publicKey: {"
-        "  challenge: new TextEncoder().encode('climb a mountain'),"
-        "  rp: { id: 'localhost', name: 'Acme' },"
-        "  user: { "
-        "    id: new TextEncoder().encode('1098237235409872'),"
-        "    name: 'avery.a.jones@example.com',"
-        "    displayName: 'Avery A. Jones', "
-        "    icon: 'https://pics.acme.com/00/p/aBjjjpqPb.png'},"
-        "  pubKeyCredParams: [{ type: 'public-key', alg: '-7'}],"
-        "  timeout: 60000,"
-        "  excludeCredentials: [] }"
-        "}).catch(c => window.domAutomationController.send(c.toString()));";
-    ASSERT_TRUE(
-        content::ExecuteScriptAndExtractString(web_contents, script, &result));
-    ASSERT_EQ(
-        "SecurityError: The relying party ID 'localhost' is not a registrable "
-        "domain suffix of, nor equal to 'https://www.example.com",
-        result.substr(0, 124));
-  }
-
   // Schedules a call to be made to navigator.credentials.store() in the
   // `unload` handler to save a credential with |username| and |password|.
   void ScheduleNavigatorStoreCredentialAtUnload(
@@ -1038,30 +982,4 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest, CredentialsAutofilled) {
   WaitForElementValue("password_field", "12345");
 }
 
-// Tests that when navigator.credentials.create() is called with an unsupported
-// algorithm, we get a NotSupportedError.
-IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
-                       CreatePublicKeyCredentialAlgorithmNotSupported) {
-  const GURL a_url1 =
-      https_test_server().GetURL("www.example.com", "/title1.html");
-  // Navigate to a mostly empty page.
-  ui_test_utils::NavigateToURL(browser(), a_url1);
-
-  ASSERT_NO_FATAL_FAILURE(
-      CreatePublicKeyCredentialWithUnsupportedAlgorithmAndExpectNotSupported(
-          WebContents()));
-}
-
-// Tests that when navigator.credentials.create() is called with an invalid
-// relying party id, we get a SecurityError
-IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
-                       CreatePublicKeyCredentialInvalidRp) {
-  const GURL a_url1 =
-      https_test_server().GetURL("www.example.com", "/title1.html");
-  ui_test_utils::NavigateToURL(browser(), a_url1);
-  ASSERT_NO_FATAL_FAILURE(
-      CreatePublicKeyCredentialWithUnsupportedRpIdAndExpectInvalidRpId(
-          WebContents()));
-}
-
 }  // namespace

content/browser/webauth/authenticator_impl.cc

@@ -174,11 +174,11 @@ bool AreOptionsSupportedByU2fAuthenticators(
   if (options->authenticator_selection) {
     if (options->authenticator_selection->user_verification ==
             webauth::mojom::UserVerificationRequirement::REQUIRED ||
-        options->authenticator_selection->require_resident_key)
+        options->authenticator_selection->require_resident_key ||
+        options->authenticator_selection->authenticator_attachment ==
+            webauth::mojom::AuthenticatorAttachment::PLATFORM)
       return false;
   }
-  if (!IsAlgorithmSupportedByU2fAuthenticators(options->public_key_parameters))
-    return false;
   return true;
 }
 
@@ -402,8 +402,17 @@ void AuthenticatorImpl::MakeCredential(
 
   // Verify that the request doesn't contain parameters that U2F authenticators
   // cannot fulfill.
-  // TODO(crbug.com/819256): Improve messages for "Not Supported" errors.
+  // TODO(crbug.com/819256): Improve messages for "Not Allowed" errors.
   if (!AreOptionsSupportedByU2fAuthenticators(options)) {
+    InvokeCallbackAndCleanup(
+        std::move(callback),
+        webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
+    return;
+  }
+
+  // TODO(crbug.com/819256): Improve messages for "Not Supported" errors.
+  if (!IsAlgorithmSupportedByU2fAuthenticators(
+          options->public_key_parameters)) {
     InvokeCallbackAndCleanup(
         std::move(callback),
         webauth::mojom::AuthenticatorStatus::NOT_SUPPORTED_ERROR, nullptr);
@@ -490,7 +499,7 @@ void AuthenticatorImpl::GetAssertion(
       webauth::mojom::UserVerificationRequirement::REQUIRED) {
     InvokeCallbackAndCleanup(
         std::move(callback),
-        webauth::mojom::AuthenticatorStatus::NOT_SUPPORTED_ERROR, nullptr);
+        webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
     return;
   }
 

content/browser/webauth/authenticator_impl_unittest.cc

@@ -352,7 +352,7 @@ TEST_F(AuthenticatorImplTest, MakeCredentialNoSupportedAlgorithm) {
   EXPECT_EQ(AuthenticatorStatus::NOT_SUPPORTED_ERROR, cb.status());
 }
 
-// Test that service returns NOT_SUPPORTED_ERROR if user verification is
+// Test that service returns NOT_ALLOWED_ERROR if user verification is
 // REQUIRED for get().
 TEST_F(AuthenticatorImplTest, GetAssertionUserVerification) {
   SimulateNavigation(GURL(kTestOrigin1));
@@ -365,10 +365,10 @@ TEST_F(AuthenticatorImplTest, GetAssertionUserVerification) {
   TestGetAssertionCallback cb;
   authenticator->GetAssertion(std::move(options), cb.callback());
   cb.WaitForCallback();
-  EXPECT_EQ(AuthenticatorStatus::NOT_SUPPORTED_ERROR, cb.status());
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, cb.status());
 }
 
-// Test that service returns NOT_SUPPORTED_ERROR if user verification is
+// Test that service returns NOT_ALLOWED_ERROR if user verification is
 // REQUIRED for create().
 TEST_F(AuthenticatorImplTest, MakeCredentialUserVerification) {
   SimulateNavigation(GURL(kTestOrigin1));
@@ -382,10 +382,10 @@ TEST_F(AuthenticatorImplTest, MakeCredentialUserVerification) {
   TestMakeCredentialCallback cb;
   authenticator->MakeCredential(std::move(options), cb.callback());
   cb.WaitForCallback();
-  EXPECT_EQ(AuthenticatorStatus::NOT_SUPPORTED_ERROR, cb.status());
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, cb.status());
 }
 
-// Test that service returns NOT_SUPPORTED_ERROR if resident key is
+// Test that service returns NOT_ALLOWED_ERROR if resident key is
 // requested for create().
 TEST_F(AuthenticatorImplTest, MakeCredentialResidentKey) {
   SimulateNavigation(GURL(kTestOrigin1));
@@ -398,7 +398,24 @@ TEST_F(AuthenticatorImplTest, MakeCredentialResidentKey) {
   TestMakeCredentialCallback cb;
   authenticator->MakeCredential(std::move(options), cb.callback());
   cb.WaitForCallback();
-  EXPECT_EQ(AuthenticatorStatus::NOT_SUPPORTED_ERROR, cb.status());
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, cb.status());
+}
+
+// Test that service returns NOT_ALLOWED_ERROR if a platform authenticator is
+// requested for U2F.
+TEST_F(AuthenticatorImplTest, MakeCredentialPlatformAuthenticator) {
+  SimulateNavigation(GURL(kTestOrigin1));
+  AuthenticatorPtr authenticator = ConnectToAuthenticator();
+
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+  options->authenticator_selection->authenticator_attachment =
+      webauth::mojom::AuthenticatorAttachment::PLATFORM;
+
+  TestMakeCredentialCallback cb;
+  authenticator->MakeCredential(std::move(options), cb.callback());
+  cb.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, cb.status());
 }
 
 // Parses its arguments as JSON and expects that all the keys in the first are

content/browser/webauth/webauth_browsertest.cc

@@ -55,28 +55,64 @@ using TestGetCallbackReceiver = ::device::test::StatusAndValueCallbackReceiver<
     AuthenticatorStatus,
     GetAssertionAuthenticatorResponsePtr>;
 
+constexpr char kNotAllowedErrorMessage[] =
+    "NotAllowedError: The operation either timed out or was not allowed. See: "
+    "https://w3c.github.io/webauthn/#sec-assertion-privacy.";
+
+constexpr char kRelyingPartySecurityErrorMessage[] =
+    "SecurityError: The relying party ID 'localhost' is not a registrable "
+    "domain suffix of, nor equal to 'https://www.example.com";
+
 constexpr char kNotSupportedErrorMessage[] =
     "NotSupportedError: Parameters for this operation are not supported.";
 
 // Templates to be used with base::ReplaceStringPlaceholders. Can be
-// modified to include up to 9 replacements.
+// modified to include up to 9 replacements. The default values for
+// any additional replacements added should also be added to the
+// CreateParameters struct.
 constexpr char kCreatePublicKeyTemplate[] =
     "navigator.credentials.create({ publicKey: {"
     "  challenge: new TextEncoder().encode('climb a mountain'),"
-    "  rp: { id: 'example.com', name: 'Acme' },"
+    "  rp: { id: '$3', name: 'Acme' },"
     "  user: { "
     "    id: new TextEncoder().encode('1098237235409872'),"
     "    name: 'avery.a.jones@example.com',"
     "    displayName: 'Avery A. Jones', "
     "    icon: 'https://pics.acme.com/00/p/aBjjjpqPb.png'},"
-    "  pubKeyCredParams: [{ type: 'public-key', alg: '-7'}],"
+    "  pubKeyCredParams: [{ type: 'public-key', alg: '$4'}],"
     "  timeout: 60000,"
     "  excludeCredentials: [],"
-    "  authenticatorSelection : { "
-    "     requireResidentKey: $1, "
-    "     userVerification: '$2' }}"
+    "  authenticatorSelection : {"
+    "     requireResidentKey: $1,"
+    "     userVerification: '$2',"
+    "     authenticatorAttachment: '$5' }}"
     "}).catch(c => window.domAutomationController.send(c.toString()));";
 
+constexpr char kPlatform[] = "platform";
+constexpr char kCrossPlatform[] = "cross-platform";
+constexpr char kPreferredVerification[] = "preferred";
+constexpr char kRequiredVerification[] = "required";
+
+// Default values for kCreatePublicKeyTemplate.
+struct CreateParameters {
+  const char* rp_id = "example.com";
+  bool require_resident_key = false;
+  const char* user_verification = kPreferredVerification;
+  const char* authenticator_attachment = kCrossPlatform;
+  const char* algorithm_identifier = "-7";
+};
+
+std::string BuildCreateCallWithParameters(const CreateParameters& parameters) {
+  std::vector<std::string> substititions;
+  substititions.push_back(parameters.require_resident_key ? "true" : "false");
+  substititions.push_back(parameters.user_verification);
+  substititions.push_back(parameters.rp_id);
+  substititions.push_back(parameters.algorithm_identifier);
+  substititions.push_back(parameters.authenticator_attachment);
+  return base::ReplaceStringPlaceholders(kCreatePublicKeyTemplate,
+                                         substititions, nullptr);
+}
+
 constexpr char kGetPublicKeyTemplate[] =
     "navigator.credentials.get({ publicKey: {"
     "  challenge: new TextEncoder().encode('climb a mountain'),"
@@ -460,33 +496,34 @@ class WebAuthJavascriptClientBrowserTest : public WebAuthBrowserTestBase {
 };
 
 // Tests that when navigator.credentials.create() is called with user
-// verification required we get a NotSupportedError.
+// verification required we get a NotAllowedError.
 IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
                        CreatePublicKeyCredentialWithUserVerification) {
-  const std::string kScript = base::ReplaceStringPlaceholders(
-      kCreatePublicKeyTemplate, {"false", "required"}, nullptr);
-
+  CreateParameters parameters;
+  parameters.user_verification = kRequiredVerification;
   std::string result;
   ASSERT_TRUE(content::ExecuteScriptAndExtractString(
-      shell()->web_contents()->GetMainFrame(), kScript, &result));
-  ASSERT_EQ(kNotSupportedErrorMessage, result);
+      shell()->web_contents()->GetMainFrame(),
+      BuildCreateCallWithParameters(parameters), &result));
+  ASSERT_EQ(kNotAllowedErrorMessage, result);
 }
 
 // Tests that when navigator.credentials.create() is called with resident key
-// required, we get a NotSupportedError.
+// required, we get a NotAllowedError.
 IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
                        CreatePublicKeyCredentialWithResidentKeyRequired) {
-  const std::string kScript = base::ReplaceStringPlaceholders(
-      kCreatePublicKeyTemplate, {"true", "preferred"}, nullptr);
-
+  CreateParameters parameters;
+  parameters.require_resident_key = true;
   std::string result;
   ASSERT_TRUE(content::ExecuteScriptAndExtractString(
-      shell()->web_contents()->GetMainFrame(), kScript, &result));
-  ASSERT_EQ(kNotSupportedErrorMessage, result);
+      shell()->web_contents()->GetMainFrame(),
+      BuildCreateCallWithParameters(parameters), &result));
+
+  ASSERT_EQ(kNotAllowedErrorMessage, result);
 }
 
 // Tests that when navigator.credentials.get() is called with user verification
-// required, we get a NotSupportedError.
+// required, we get a NotAllowedError.
 IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
                        GetPublicKeyCredentialUserVerification) {
   const std::string kScript = base::ReplaceStringPlaceholders(
@@ -495,10 +532,54 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
   std::string result;
   ASSERT_TRUE(content::ExecuteScriptAndExtractString(
       shell()->web_contents()->GetMainFrame(), kScript, &result));
+  ASSERT_EQ(kNotAllowedErrorMessage, result);
+}
+
+// Tests that when navigator.credentials.create() is called with an invalid
+// relying party id, we get a SecurityError.
+IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
+                       CreatePublicKeyCredentialInvalidRp) {
+  CreateParameters parameters;
+  parameters.rp_id = "localhost";
+  std::string result;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+      shell()->web_contents()->GetMainFrame(),
+      BuildCreateCallWithParameters(parameters), &result));
+
+  ASSERT_EQ(kRelyingPartySecurityErrorMessage,
+            result.substr(0, strlen(kRelyingPartySecurityErrorMessage)));
+}
+
+// Tests that when navigator.credentials.create() is called with an
+// unsupported algorithm, we get a NotSupportedError.
+IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
+                       CreatePublicKeyCredentialAlgorithmNotSupported) {
+  CreateParameters parameters;
+  parameters.algorithm_identifier = "123";
+  std::string result;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+      shell()->web_contents()->GetMainFrame(),
+      BuildCreateCallWithParameters(parameters), &result));
+
   ASSERT_EQ(kNotSupportedErrorMessage, result);
 }
 
-// WebAuthBrowserBleDisabledTest ----------------------------------------------
+// Tests that when navigator.credentials.create() is called with a
+// platform authenticator requested, we get a NotAllowedError.
+IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
+                       CreatePublicKeyCredentialPlatformAuthenticator) {
+  CreateParameters parameters;
+  parameters.authenticator_attachment = kPlatform;
+  std::string result;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+      shell()->web_contents()->GetMainFrame(),
+      BuildCreateCallWithParameters(parameters), &result));
+
+  ASSERT_EQ(kNotAllowedErrorMessage, result);
+}
+
+// WebAuthBrowserBleDisabledTest
+// ----------------------------------------------
 
 // A test fixture that does not enable BLE discovery.
 class WebAuthBrowserBleDisabledTest : public WebAuthLocalClientBrowserTest {