webauthn: various behaviour changes for iCloud Keychain

Bug: 1439987
Change-Id: I2074b791e11b38137894f45f7e3328eb79815d1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4785929
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185316}

chrome/browser/ui/webauthn/sheet_models.cc

@@ -1496,6 +1496,8 @@ AuthenticatorMultiSourcePickerSheetModel::
   vector_illustrations_.emplace(kPasskeyHeaderIcon, kPasskeyHeaderDarkIcon);
 
   using CredentialMech = AuthenticatorRequestDialogModel::Mechanism::Credential;
+  using ICloudKeychainMech =
+      AuthenticatorRequestDialogModel::Mechanism::ICloudKeychain;
   bool has_local_passkeys =
       std::ranges::any_of(dialog_model->mechanisms(), [](const auto& mech) {
         return absl::holds_alternative<CredentialMech>(mech.type) &&
@@ -1508,9 +1510,14 @@ AuthenticatorMultiSourcePickerSheetModel::
     for (size_t i = 0; i < dialog_model->mechanisms().size(); ++i) {
       const AuthenticatorRequestDialogModel::Mechanism& mech =
           dialog_model->mechanisms()[i];
-      if (absl::holds_alternative<CredentialMech>(mech.type) &&
-          absl::get<CredentialMech>(mech.type).value() !=
-              device::AuthenticatorType::kPhone) {
+      if ((absl::holds_alternative<CredentialMech>(mech.type) &&
+           absl::get<CredentialMech>(mech.type).value() !=
+               device::AuthenticatorType::kPhone) ||
+          // iCloud Keychain appears in the primary list if present. This
+          // happens when Chrome does not have permission to enumerate
+          // credentials from iCloud Keychain. Thus this generic option is the
+          // only way for the user to trigger it.
+          absl::holds_alternative<ICloudKeychainMech>(mech.type)) {
         primary_passkey_indices_.push_back(i);
       } else {
         secondary_passkey_indices_.push_back(i);

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -1015,6 +1015,16 @@ std::vector<std::string> AuthenticatorRequestDialogModel::paired_phone_names()
   return names;
 }
 
+void AuthenticatorRequestDialogModel::set_allow_icloud_keychain(
+    bool is_allowed) {
+  allow_icloud_keychain_ = is_allowed;
+}
+
+void AuthenticatorRequestDialogModel::set_should_create_in_icloud_keychain(
+    bool is_enabled) {
+  should_create_in_icloud_keychain_ = is_enabled;
+}
+
 #if BUILDFLAG(IS_MAC)
 
 // This enum is used in a histogram. Never change assigned values and only add
@@ -1445,6 +1455,10 @@ void AuthenticatorRequestDialogModel::PopulateMechanisms() {
           !list_phone_passkeys) {
         continue;
       }
+      if (cred.source == device::AuthenticatorType::kICloudKeychain &&
+          !allow_icloud_keychain_) {
+        continue;
+      }
       if (cred.source == device::AuthenticatorType::kPhone) {
         specific_phones_listed = true;
       }
@@ -1513,11 +1527,17 @@ void AuthenticatorRequestDialogModel::PopulateMechanisms() {
     }
   }
 
-  if (transport_availability_.has_icloud_keychain) {
+  if (transport_availability_.has_icloud_keychain && allow_icloud_keychain_ &&
+      // The mechanism for iCloud Keychain only appears for create(), or if
+      // Chrome doesn't have permission to enumerate credentials and thus the
+      // user needs a generic mechanism to trigger it.
+      (!is_new_get_assertion_ui ||
+       transport_availability_.has_icloud_keychain_credential ==
+           device::FidoRequestHandlerBase::RecognizedCredential::kUnknown)) {
     const std::u16string name =
         l10n_util::GetStringUTF16(IDS_WEBAUTHN_TRANSPORT_ICLOUD_KEYCHAIN);
     mechanisms_.emplace_back(
-        Mechanism::ICloudKeychain(), name, name, kSmartphoneIcon,
+        Mechanism::ICloudKeychain(), name, name, vector_icons::kPasskeyIcon,
         base::BindRepeating(
             &AuthenticatorRequestDialogModel::StartICloudKeychain,
             base::Unretained(this)));
@@ -1626,7 +1646,9 @@ void AuthenticatorRequestDialogModel::PopulateMechanisms() {
 
 absl::optional<size_t>
 AuthenticatorRequestDialogModel::IndexOfPriorityMechanism() {
-  if (base::FeatureList::IsEnabled(device::kWebAuthnNewPasskeyUI)) {
+  if (base::FeatureList::IsEnabled(device::kWebAuthnNewPasskeyUI) &&
+      transport_availability_.request_type ==
+          device::FidoRequestType::kGetAssertion) {
     // If there is a single mechanism, go to that.
     if (mechanisms_.size() == 1) {
       return 0;
@@ -1670,8 +1692,9 @@ AuthenticatorRequestDialogModel::IndexOfPriorityMechanism() {
       if (!windows_handles_hybrid) {
         // If there's a match on the platform authenticator, jump to that.
         if (transport_availability_.has_icloud_keychain_credential ==
-            device::FidoRequestHandlerBase::RecognizedCredential::
-                kHasRecognizedCredential) {
+                device::FidoRequestHandlerBase::RecognizedCredential::
+                    kHasRecognizedCredential &&
+            allow_icloud_keychain_) {
           priority_list.emplace_back(Mechanism::ICloudKeychain());
         }
         if (transport_availability_.has_platform_authenticator_credential ==
@@ -1736,6 +1759,22 @@ AuthenticatorRequestDialogModel::IndexOfPriorityMechanism() {
       priority_list.emplace_back(Mechanism::WindowsAPI());
     }
 
+#if BUILDFLAG(IS_MAC)
+    if (*transport_availability_.make_credential_attachment ==
+        device::AuthenticatorAttachment::kPlatform) {
+      // For platform attachments, either we have iCloud Keychain available
+      // or not. If not, then there's only a single active mechanism (the
+      // profile authenticator) and we would have picked it above. Thus here we
+      // must be picking between the profile authenticator and iCloud Keychain.
+      if (should_create_in_icloud_keychain_) {
+        priority_list.emplace_back(Mechanism::ICloudKeychain());
+      } else {
+        priority_list.emplace_back(
+            Mechanism::Transport(AuthenticatorTransport::kInternal));
+      }
+    }
+#endif
+
     const bool is_passkey_request =
         resident_key_requirement() !=
         device::ResidentKeyRequirement::kDiscouraged;

chrome/browser/webauthn/authenticator_request_dialog_model.h

@@ -627,6 +627,9 @@ class AuthenticatorRequestDialogModel {
 
   bool offer_try_again_in_ui() const { return offer_try_again_in_ui_; }
 
+  void set_allow_icloud_keychain(bool);
+  void set_should_create_in_icloud_keychain(bool);
+
 #if BUILDFLAG(IS_MAC)
   void RecordMacOsStartedHistogram();
   void RecordMacOsSuccessHistogram(device::FidoRequestType,
@@ -848,6 +851,16 @@ class AuthenticatorRequestDialogModel {
   // with the request.
   device::PublicKeyCredentialUserEntity user_entity_;
 
+  // allow_icloud_keychain_ is true if iCloud Keychain can be used for this
+  // request. It is disabled for Secure Payment Confirmation and other non-
+  // WebAuthn cases, for example.
+  bool allow_icloud_keychain_ = false;
+
+  // should_create_in_icloud_keychain is true if creation requests with
+  // attachment=platform should default to iCloud Keychain rather than the
+  // profile authenticator.
+  bool should_create_in_icloud_keychain_ = false;
+
 #if BUILDFLAG(IS_MAC)
   // did_record_macos_start_histogram_ is set to true if a histogram record of
   // starting the current request was made. Any later successful completion will

chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc

@@ -140,6 +140,8 @@ enum class TransportAvailabilityParam {
   kAttachmentCrossPlatform,
   kBleDisabled,
   kBleAccessDenied,
+  kHasICloudKeychain,
+  kCreateInICloudKeychain,
 };
 
 base::StringPiece TransportAvailabilityParamToString(
@@ -181,6 +183,10 @@ base::StringPiece TransportAvailabilityParamToString(
       return "kBleDisabled";
     case TransportAvailabilityParam::kBleAccessDenied:
       return "kBleAccessDenied";
+    case TransportAvailabilityParam::kHasICloudKeychain:
+      return "kHasICloudKeychain";
+    case TransportAvailabilityParam::kCreateInICloudKeychain:
+      return "kCreateInICloudKeychain";
   }
 }
 
@@ -274,11 +280,17 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
   const auto att_xplat = TransportAvailabilityParam::kAttachmentCrossPlatform;
   const auto ble_off = TransportAvailabilityParam::kBleDisabled;
   const auto ble_denied = TransportAvailabilityParam::kBleAccessDenied;
+  [[maybe_unused]] const auto has_ickc =
+      TransportAvailabilityParam::kHasICloudKeychain;
+  [[maybe_unused]] const auto create_ickc =
+      TransportAvailabilityParam::kCreateInICloudKeychain;
   using c = AuthenticatorRequestDialogModel::Mechanism::Credential;
   using t = AuthenticatorRequestDialogModel::Mechanism::Transport;
   using p = AuthenticatorRequestDialogModel::Mechanism::Phone;
   const auto winapi = AuthenticatorRequestDialogModel::Mechanism::WindowsAPI();
   const auto add = AuthenticatorRequestDialogModel::Mechanism::AddPhone();
+  [[maybe_unused]] const auto ickc =
+      AuthenticatorRequestDialogModel::Mechanism::ICloudKeychain();
   const auto usb_ui = Step::kUsbInsertAndActivate;
   const auto mss = Step::kMechanismSelection;
   const auto plat_ui = Step::kNotStarted;
@@ -434,12 +446,18 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
 
       // QR code first: Make credential should jump to the QR code with
       // RK=true.
-      {L, mc, {usb, internal, cable}, {rk}, {}, {add, t(internal), t(usb)}, qr},
+      {L,
+       mc,
+       {usb, internal, cable},
+       {rk, att_xplat},
+       {},
+       {add, t(internal), t(usb)},
+       qr},
       // Unless there is a phone paired already.
       {L,
        mc,
        {usb, internal, cable},
-       {rk},
+       {rk, att_xplat},
        {pqr("a")},
        {p("a"), add, t(internal), t(usb)},
        mss},
@@ -451,14 +469,6 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
        {},
        {add, t(internal), t(usb)},
        mss},
-      // But not for any attachment, like platform
-      {L,
-       mc,
-       {usb, internal, cable},
-       {rk, att_xplat},
-       {},
-       {add, t(internal), t(usb)},
-       qr},
       // If RK=false, go to the default for the platform instead.
       {
           L,
@@ -648,6 +658,29 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
        {psync("a")},
        {c(phone), add},
        hero},
+
+#if BUILDFLAG(IS_MAC)
+      // Even with iCloud Keychain present, we shouldn't jump to it without
+      // additional flags set.
+      {L, mc, {internal}, {rk, has_ickc}, {}, {ickc, t(internal)}, create_pk},
+      // iCloud Keychain should be the default if the request delegate
+      // configured that.
+      {L,
+       mc,
+       {internal},
+       {rk, has_ickc, create_ickc},
+       {},
+       {ickc, t(internal)},
+       plat_ui},
+      // ... and only for attachment=platform
+      {L,
+       mc,
+       {internal},
+       {rk, att_any, has_ickc, create_ickc},
+       {},
+       {ickc, t(internal)},
+       mss},
+#endif
   };
 
   Test kListSyncedPasskeysTests_Windows_NoWinHybrid[]{
@@ -769,6 +802,8 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
         test.params, TransportAvailabilityParam::kOnlyHybridOrInternal);
     transports_info.request_is_internal_only =
         base::Contains(test.params, TransportAvailabilityParam::kOnlyInternal);
+    transports_info.has_icloud_keychain = base::Contains(
+        test.params, TransportAvailabilityParam::kHasICloudKeychain);
 
     if (base::Contains(
             test.params,
@@ -815,6 +850,12 @@ TEST_F(AuthenticatorRequestDialogModelTest, Mechanisms) {
       has_v2_cable_extension = true;
     }
 
+    model.set_allow_icloud_keychain(transports_info.has_icloud_keychain);
+    if (base::Contains(test.params,
+                       TransportAvailabilityParam::kCreateInICloudKeychain)) {
+      model.set_should_create_in_icloud_keychain(true);
+    }
+
     if (has_v2_cable_extension.has_value() || !test.phones.empty() ||
         base::Contains(test.transports,
                        device::FidoTransportProtocol::kHybrid)) {
@@ -1047,6 +1088,10 @@ TEST_F(AuthenticatorRequestDialogModelTest, Cable2ndFactorFlows) {
     transports_info.is_ble_powered = test.ble_power == BLEPower::ON;
     transports_info.can_power_on_ble_adapter = true;
     transports_info.request_type = test.request_type;
+    if (transports_info.request_type == RequestType::kMakeCredential) {
+      transports_info.make_credential_attachment =
+          device::AuthenticatorAttachment::kAny;
+    }
     transports_info.available_transports = {AuthenticatorTransport::kHybrid};
     transports_info.is_off_the_record_context =
         test.profile == Profile::INCOGNITO;