webauthn: update account picker trigger.

The logic around this has changed from CTAP 2.0 to the current draft of
2.1 and is likely to change further[1]. There's a good argument to be
made based on CTAP 2.0 (but not the 2.1 draft) that it's ok to send
numberOfCredentials even for a non-empty allow list case so we shouldn't
depend on that.

Instead, have AuthenticatorCommon just remember whether the allow list
was empty or not. Then, in the empty case, show a picker if we can. If
there are multiple options then we have to show a picker. If there's
only a single option but we have identifying information, show a picker
to let the user confirm which account they wish to use.

In the future, if there's an explicit userSelected signal in CTAP2 then
we'll suppress the picker when that's asserted.

[1] https://github.com/fido-alliance/fido-2-specs/pull/717

Change-Id: I1060c94258428adfdc14f6999b1e07bde93456fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762942
Commit-Queue: Adam Langley <agl@chromium.org>
Auto-Submit: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#689300}

content/browser/webauth/authenticator_common.cc

@@ -926,13 +926,15 @@ void AuthenticatorCommon::GetAssertion(
         std::move(options->challenge));
   }
 
-  if (options->allow_credentials.empty() &&
-      (!base::FeatureList::IsEnabled(device::kWebAuthResidentKeys) ||
-       !request_delegate_->SupportsResidentKeys())) {
-    InvokeCallbackAndCleanup(
-        std::move(callback),
-        blink::mojom::AuthenticatorStatus::RESIDENT_CREDENTIALS_UNSUPPORTED);
-    return;
+  if (options->allow_credentials.empty()) {
+    if (!base::FeatureList::IsEnabled(device::kWebAuthResidentKeys) ||
+        !request_delegate_->SupportsResidentKeys()) {
+      InvokeCallbackAndCleanup(
+          std::move(callback),
+          blink::mojom::AuthenticatorStatus::RESIDENT_CREDENTIALS_UNSUPPORTED);
+      return;
+    }
+    empty_allow_list_ = true;
   }
 
   if (options->appid) {
@@ -1298,13 +1300,15 @@ void AuthenticatorCommon::OnSignResponse(
             *authenticator->AuthenticatorTransport());
       }
 
-      // Show an account picker for requests with empty allow lists (where
-      // num_credentials() is set in the response). Authenticators may omit the
-      // user entity if only one credential matches, or if they have account
-      // selection UI built-in. In that case, consider that credential
-      // pre-selected.
-      if (response_data->at(0).num_credentials() &&
-          (response_data->size() > 1 || response_data->at(0).user_entity())) {
+      // Show an account picker for requests with empty allow lists.
+      // Authenticators may omit the identifying information in the user entity
+      // if only one credential matches, or if they have account selection UI
+      // built-in. In that case, consider that credential pre-selected.
+      if (empty_allow_list_ &&
+          (response_data->size() > 1 ||
+           (response_data->at(0).user_entity() &&
+            (response_data->at(0).user_entity()->name ||
+             response_data->at(0).user_entity()->display_name)))) {
         request_delegate_->SelectAccount(
             std::move(*response_data),
             base::BindOnce(&AuthenticatorCommon::OnAccountSelected,
@@ -1465,6 +1469,7 @@ void AuthenticatorCommon::Cleanup() {
   caller_origin_ = url::Origin();
   relying_party_id_.clear();
   attestation_requested_ = false;
+  empty_allow_list_ = false;
   error_awaiting_user_acknowledgement_ =
       blink::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
 }

content/browser/webauth/authenticator_common.h

@@ -197,6 +197,9 @@ class CONTENT_EXPORT AuthenticatorCommon {
       get_assertion_response_callback_;
   std::string client_data_json_;
   bool attestation_requested_;
+  // empty_allow_list_ is true iff a GetAssertion is currently pending and the
+  // request did not list any credential IDs in the allow list.
+  bool empty_allow_list_ = false;
   url::Origin caller_origin_;
   std::string relying_party_id_;
   std::unique_ptr<base::OneShotTimer> timer_;

content/browser/webauth/authenticator_impl_unittest.cc

@@ -3726,16 +3726,18 @@ TEST_F(ResidentKeyAuthenticatorImplTest, StorageFull) {
             test_client_.failure_reason);
 }
 
-TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionSingle) {
+TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionSingleNoPII) {
   ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectResidentKey(
       /*credential_id=*/{{4, 3, 2, 1}}, kTestRelyingPartyId,
-      /*user_id=*/{{1, 2, 3, 4}}, "test@example.com", "Test User"));
+      /*user_id=*/{{1, 2, 3, 4}}, base::nullopt, base::nullopt));
 
   TestServiceManagerContext smc;
   mojo::Remote<blink::mojom::Authenticator> authenticator =
       ConnectToAuthenticator();
   TestGetAssertionCallback callback_receiver;
-  // |SelectAccount| should not be called when there's only a single response.
+  // |SelectAccount| should not be called when there's only a single response
+  // with no identifying user info because the UI is bad in that case: we can
+  // only display the single choice of "Unknown user".
   test_client_.expected_accounts = "<invalid>";
   authenticator->GetAssertion(get_credential_options(),
                               callback_receiver.callback());
@@ -3744,6 +3746,25 @@ TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionSingle) {
   EXPECT_TRUE(HasUV(callback_receiver));
 }
 
+TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionSingleWithPII) {
+  ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectResidentKey(
+      /*credential_id=*/{{4, 3, 2, 1}}, kTestRelyingPartyId,
+      /*user_id=*/{{1, 2, 3, 4}}, base::nullopt, "Test User"));
+
+  TestServiceManagerContext smc;
+  mojo::Remote<blink::mojom::Authenticator> authenticator =
+      ConnectToAuthenticator();
+  TestGetAssertionCallback callback_receiver;
+  // |SelectAccount| should be called when PII is available.
+  test_client_.expected_accounts = "01020304::Test User";
+  test_client_.selected_user_id = {1, 2, 3, 4};
+  authenticator->GetAssertion(get_credential_options(),
+                              callback_receiver.callback());
+  callback_receiver.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+  EXPECT_TRUE(HasUV(callback_receiver));
+}
+
 TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionMulti) {
   ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectResidentKey(
       /*credential_id=*/{{4, 3, 2, 1}}, kTestRelyingPartyId,
@@ -3777,13 +3798,14 @@ TEST_F(ResidentKeyAuthenticatorImplTest, GetAssertionUVDiscouraged) {
 
   ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectResidentKey(
       /*credential_id=*/{{4, 3, 2, 1}}, kTestRelyingPartyId,
-      /*user_id=*/{{1, 2, 3, 4}}, "test@example.com", "Test User"));
+      /*user_id=*/{{1, 2, 3, 4}}, base::nullopt, base::nullopt));
 
   TestServiceManagerContext smc;
   mojo::Remote<blink::mojom::Authenticator> authenticator =
       ConnectToAuthenticator();
   TestGetAssertionCallback callback_receiver;
-  // |SelectAccount| should not be called when there's only a single response.
+  // |SelectAccount| should not be called when there's only a single response
+  // without identifying information.
   test_client_.expected_accounts = "<invalid>";
   PublicKeyCredentialRequestOptionsPtr options(get_credential_options());
   options->user_verification =
@@ -3993,13 +4015,14 @@ TEST_F(ResidentKeyAuthenticatorImplTest, WithAppIDExtension) {
   virtual_device_factory_->SetCtap2Config(config);
   ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectResidentKey(
       /*credential_id=*/{{4, 3, 2, 1}}, kTestRelyingPartyId,
-      /*user_id=*/{{1, 2, 3, 4}}, "test@example.com", "Test User"));
+      /*user_id=*/{{1, 2, 3, 4}}, base::nullopt, base::nullopt));
 
   TestServiceManagerContext smc;
   mojo::Remote<blink::mojom::Authenticator> authenticator =
       ConnectToAuthenticator();
   TestGetAssertionCallback callback_receiver;
-  // |SelectAccount| should not be called when there's only a single response.
+  // |SelectAccount| should not be called when there's only a single response
+  // without identifying information.
   test_client_.expected_accounts = "<invalid>";
 
   PublicKeyCredentialRequestOptionsPtr options = get_credential_options();

device/fido/virtual_fido_device.cc

@@ -125,12 +125,13 @@ bool VirtualFidoDevice::State::InjectResidentKey(
     base::span<const uint8_t> credential_id,
     const std::string& relying_party_id,
     base::span<const uint8_t> user_id,
-    const std::string& user_name,
-    const std::string& user_display_name) {
+    base::Optional<std::string> user_name,
+    base::Optional<std::string> user_display_name) {
   return InjectResidentKey(
       credential_id, PublicKeyCredentialRpEntity(std::move(relying_party_id)),
       PublicKeyCredentialUserEntity(fido_parsing_utils::Materialize(user_id),
-                                    user_name, user_display_name,
+                                    std::move(user_name),
+                                    std::move(user_display_name),
                                     /*icon_url=*/base::nullopt));
 }
 

device/fido/virtual_fido_device.h

@@ -181,8 +181,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) VirtualFidoDevice : public FidoDevice {
     bool InjectResidentKey(base::span<const uint8_t> credential_id,
                            const std::string& relying_party_id,
                            base::span<const uint8_t> user_id,
-                           const std::string& user_name,
-                           const std::string& user_display_name);
+                           base::Optional<std::string> user_name,
+                           base::Optional<std::string> user_display_name);
 
    private:
     friend class base::RefCounted<State>;