[password manager] Plumb removing passkeys

Plumb removing passkeys from the front-end to the PasskeyModel.
Rename `passwordsPrivate.removeSavedPassword` to `removeCredential`.

Tapping "Delete" is enough to trigger removing a passkey. A follow-up
will add a modal dialog confirmation.

For now, SavedPasswordsPresenter::RemoveCredential triggers a login
changed event. This will be replaced by an observer interface.

This feature is guarded by the WebAuthenticationManageGPMPasskeys flag.

Design doc:
https://docs.google.com/document/d/1Ox_VBGNO-Ff9bcP97FUMcN15Xtfn_sz0_WuDe3ujbwA

Low-Coverage-Reason: defaulted dtor that is never called.
Bug: 1432717
Change-Id: I735b4c7a916bbb34fb70716da65dd0367b7b65a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4568555
Auto-Submit: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153880}

chrome/browser/extensions/api/passwords_private/passwords_private_api.cc

@@ -72,17 +72,17 @@ ResponseAction PasswordsPrivateChangeSavedPasswordFunction::Run() {
       "id."));
 }
 
-// PasswordsPrivateRemoveSavedPasswordFunction
-ResponseAction PasswordsPrivateRemoveSavedPasswordFunction::Run() {
+// PasswordsPrivateRemoveCredentialFunction
+ResponseAction PasswordsPrivateRemoveCredentialFunction::Run() {
   if (!GetDelegate(browser_context())) {
     return RespondNow(Error(kNoDelegateError));
   }
 
   auto parameters =
-      api::passwords_private::RemoveSavedPassword::Params::Create(args());
+      api::passwords_private::RemoveCredential::Params::Create(args());
   EXTENSION_FUNCTION_VALIDATE(parameters);
   GetDelegate(browser_context())
-      ->RemoveSavedPassword(parameters->id, parameters->from_stores);
+      ->RemoveCredential(parameters->id, parameters->from_stores);
   return RespondNow(NoArguments());
 }
 

chrome/browser/extensions/api/passwords_private/passwords_private_api.h

@@ -41,13 +41,13 @@ class PasswordsPrivateChangeSavedPasswordFunction : public ExtensionFunction {
   ResponseAction Run() override;
 };
 
-class PasswordsPrivateRemoveSavedPasswordFunction : public ExtensionFunction {
+class PasswordsPrivateRemoveCredentialFunction : public ExtensionFunction {
  public:
-  DECLARE_EXTENSION_FUNCTION("passwordsPrivate.removeSavedPassword",
-                             PASSWORDSPRIVATE_REMOVESAVEDPASSWORD)
+  DECLARE_EXTENSION_FUNCTION("passwordsPrivate.removeCredential",
+                             PASSWORDSPRIVATE_REMOVECREDENTIAL)
 
  protected:
-  ~PasswordsPrivateRemoveSavedPasswordFunction() override = default;
+  ~PasswordsPrivateRemoveCredentialFunction() override = default;
 
   // ExtensionFunction overrides.
   ResponseAction Run() override;

chrome/browser/extensions/api/passwords_private/passwords_private_apitest.cc

@@ -221,6 +221,10 @@ IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest,
       << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest, RemovePasskey) {
+  EXPECT_TRUE(RunPasswordsSubtest("removePasskey")) << message_;
+}
+
 IN_PROC_BROWSER_TEST_F(PasswordsPrivateApiTest, RequestPlaintextPassword) {
   EXPECT_TRUE(RunPasswordsSubtest("requestPlaintextPassword")) << message_;
 }

chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h

@@ -96,9 +96,9 @@ class PasswordsPrivateDelegate
       int id,
       const api::passwords_private::ChangeSavedPasswordParams& params) = 0;
 
-  // Removes the saved password entry corresponding to the |id| in the
-  // specified |from_stores|. Any invalid id will be ignored.
-  virtual void RemoveSavedPassword(
+  // Removes the credential entry corresponding to the |id| in the specified
+  // |from_stores|. Any invalid id will be ignored.
+  virtual void RemoveCredential(
       int id,
       api::passwords_private::PasswordStoreSet from_stores) = 0;
 

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -446,7 +446,7 @@ absl::optional<int> PasswordsPrivateDelegateImpl::ChangeSavedPassword(
   return credential_id_generator_.GenerateId(std::move(updated_credential));
 }
 
-void PasswordsPrivateDelegateImpl::RemoveSavedPassword(
+void PasswordsPrivateDelegateImpl::RemoveCredential(
     int id,
     api::passwords_private::PasswordStoreSet from_stores) {
   ExecuteFunction(
@@ -470,6 +470,9 @@ void PasswordsPrivateDelegateImpl::RemoveEntryInternal(
   if (entry->blocked_by_user) {
     base::RecordAction(
         base::UserMetricsAction("PasswordManager_RemovePasswordException"));
+  } else if (entry->is_passkey) {
+    base::RecordAction(
+        base::UserMetricsAction("PasswordManager_RemovePasskey"));
   } else {
     base::RecordAction(
         base::UserMetricsAction("PasswordManager_RemoveSavedPassword"));

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h

@@ -74,7 +74,7 @@ class PasswordsPrivateDelegateImpl
   absl::optional<int> ChangeSavedPassword(
       int id,
       const api::passwords_private::ChangeSavedPasswordParams& params) override;
-  void RemoveSavedPassword(
+  void RemoveCredential(
       int id,
       api::passwords_private::PasswordStoreSet from_stores) override;
   void RemovePasswordException(int id) override;

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/test/gmock_callback_support.h"
 #include "base/test/gmock_move_support.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/metrics/user_action_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h"
@@ -301,6 +302,17 @@ password_manager::PasswordForm CreateSampleForm(
   return form;
 }
 
+sync_pb::WebauthnCredentialSpecifics CreatePasskey() {
+  sync_pb::WebauthnCredentialSpecifics passkey;
+  passkey.set_sync_id(base::RandBytesAsString(16));
+  passkey.set_credential_id(base::RandBytesAsString(16));
+  passkey.set_rp_id("abc1.com");
+  passkey.set_user_id({1, 2, 3, 4});
+  passkey.set_user_name("passkey_username");
+  passkey.set_user_display_name("passkey_display_name");
+  return passkey;
+}
+
 MATCHER_P(PasswordUiEntryDataEquals, expected, "") {
   return testing::Value(expected.get().is_passkey, arg.is_passkey) &&
          testing::Value(expected.get().urls.link, arg.urls.link) &&
@@ -1485,14 +1497,8 @@ TEST_F(PasswordsPrivateDelegateImplTest, GetPasskeyInGroups) {
   PasskeyModel* passkey_model = PasskeyModelFactory::GetForProfile(profile());
   ASSERT_EQ(passkey_model, PasskeyModelFactory::GetForProfile(profile()));
   ASSERT_TRUE(passkey_model);
-  sync_pb::WebauthnCredentialSpecifics passkey;
-  passkey.set_sync_id(base::RandBytesAsString(16));
-  passkey.set_credential_id(base::RandBytesAsString(16));
-  passkey.set_rp_id("abc1.com");
-  passkey.set_user_id({1, 2, 3, 4});
-  passkey.set_user_name("passkey_username");
-  passkey.set_user_display_name("passkey_display_name");
-  passkey_model->AddNewPasskeyForTesting(std::move(passkey));
+  sync_pb::WebauthnCredentialSpecifics passkey = CreatePasskey();
+  passkey_model->AddNewPasskeyForTesting(passkey);
 
   password_manager::PasswordForm password = CreateSampleForm(
       password_manager::PasswordForm::Store::kProfileStore, u"username1");
@@ -1511,8 +1517,8 @@ TEST_F(PasswordsPrivateDelegateImplTest, GetPasskeyInGroups) {
   api::passwords_private::PasswordUiEntry expected_entry2;
   expected_entry2.is_passkey = true;
   expected_entry2.urls.link = "https://abc1.com/";
-  expected_entry2.username = "passkey_username";
-  expected_entry2.display_name = "passkey_display_name";
+  expected_entry2.username = passkey.user_name();
+  expected_entry2.display_name = passkey.user_display_name();
   expected_entry2.stored_in =
       api::passwords_private::PASSWORD_STORE_SET_ACCOUNT;
   EXPECT_THAT(groups[0].entries,
@@ -1521,4 +1527,43 @@ TEST_F(PasswordsPrivateDelegateImplTest, GetPasskeyInGroups) {
                   PasswordUiEntryDataEquals(testing::ByRef(expected_entry2))));
 }
 
+TEST_F(PasswordsPrivateDelegateImplTest, RemovePasskey) {
+  base::UserActionTester user_action_tester;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {password_manager::features::kPasswordsGrouping,
+       password_manager::features::kPasswordManagerPasskeys,
+       syncer::kSyncWebauthnCredentials},
+      /*disabled_features=*/{});
+
+  auto delegate = CreateDelegate();
+
+  PasskeyModel* passkey_model = PasskeyModelFactory::GetForProfile(profile());
+  ASSERT_EQ(passkey_model, PasskeyModelFactory::GetForProfile(profile()));
+  ASSERT_TRUE(passkey_model);
+  sync_pb::WebauthnCredentialSpecifics passkey = CreatePasskey();
+  passkey_model->AddNewPasskeyForTesting(std::move(passkey));
+  SetUpPasswordStores({});
+
+  auto groups = delegate->GetCredentialGroups();
+  api::passwords_private::PasswordUiEntry& passkey_entry =
+      groups.at(0).entries.at(0);
+  ASSERT_TRUE(passkey_entry.is_passkey);
+  EXPECT_EQ(user_action_tester.GetActionCount("PasswordManager_RemovePasskey"),
+            0);
+
+  delegate->RemoveCredential(passkey_entry.id, passkey_entry.stored_in);
+  groups = delegate->GetCredentialGroups();
+  EXPECT_TRUE(groups.empty());
+  EXPECT_EQ(user_action_tester.GetActionCount("PasswordManager_RemovePasskey"),
+            1);
+
+  // Attempt removing a non existent entry.
+  delegate->RemoveCredential(
+      /*id=*/42,
+      api::passwords_private::PasswordStoreSet::PASSWORD_STORE_SET_ACCOUNT);
+  EXPECT_EQ(user_action_tester.GetActionCount("PasswordManager_RemovePasskey"),
+            1);
+}
+
 }  // namespace extensions

chrome/browser/extensions/api/passwords_private/test_passwords_private_delegate.cc

@@ -52,6 +52,10 @@ TestPasswordsPrivateDelegate::TestPasswordsPrivateDelegate()
     current_entries_.push_back(CreateEntry(i));
     current_exceptions_.push_back(CreateException(i));
   }
+  api::passwords_private::PasswordUiEntry passkey = CreateEntry(kNumMocks);
+  passkey.is_passkey = true;
+  passkey.display_name = "displayName";
+  current_entries_.push_back(std::move(passkey));
 }
 TestPasswordsPrivateDelegate::~TestPasswordsPrivateDelegate() = default;
 
@@ -65,8 +69,8 @@ TestPasswordsPrivateDelegate::GetCredentialGroups() {
   std::vector<api::passwords_private::CredentialGroup> groups;
   api::passwords_private::CredentialGroup group_api;
   group_api.name = "test.com";
-  for (size_t i = 0; i < kNumMocks; i++) {
-    group_api.entries.push_back(CreateEntry(i));
+  for (const auto& entry : current_entries_) {
+    group_api.entries.push_back(entry.Clone());
   }
   groups.push_back(std::move(group_api));
   return groups;
@@ -114,18 +118,15 @@ absl::optional<int> TestPasswordsPrivateDelegate::ChangeSavedPassword(
   return id;
 }
 
-void TestPasswordsPrivateDelegate::RemoveSavedPassword(
+void TestPasswordsPrivateDelegate::RemoveCredential(
     int id,
     api::passwords_private::PasswordStoreSet from_stores) {
-  if (current_entries_.empty())
-    return;
-
-  // Since this is just mock data, remove the first element regardless of the
-  // data contained. One case where this logic is especially false is when the
-  // password is stored in both stores and |store| only specifies one of them
-  // (in that case the number of entries shouldn't change).
-  last_deleted_entry_ = std::move(current_entries_[0]);
-  current_entries_.erase(current_entries_.begin());
+  const auto [removed, _] = std::ranges::remove_if(
+      current_entries_, [id](const auto& entry) { return entry.id == id; });
+  if (removed != current_entries_.end()) {
+    last_deleted_entry_ = std::move(*removed);
+    current_entries_.erase(removed);
+  }
   SendSavedPasswordsList();
 }
 

chrome/browser/extensions/api/passwords_private/test_passwords_private_delegate.h

@@ -14,7 +14,7 @@
 namespace extensions {
 // A test PasswordsPrivateDelegate implementation which uses mock data.
 // TestDelegate starts out with kNumMocks mocks of each type (saved password
-// and password exception) and removes one mock each time RemoveSavedPassword()
+// and password exception) and removes one mock each time RemoveCredential()
 // or RemovePasswordException() is called.
 class TestPasswordsPrivateDelegate : public PasswordsPrivateDelegate {
  public:
@@ -44,7 +44,7 @@ class TestPasswordsPrivateDelegate : public PasswordsPrivateDelegate {
   absl::optional<int> ChangeSavedPassword(
       const int id,
       const api::passwords_private::ChangeSavedPasswordParams& params) override;
-  void RemoveSavedPassword(
+  void RemoveCredential(
       int id,
       api::passwords_private::PasswordStoreSet from_store) override;
   void RemovePasswordException(int id) override;

chrome/browser/resources/password_manager/checkup_list_item.ts

@@ -175,7 +175,7 @@ export class CheckupListItemElement extends CheckupListItemElementBase {
   }
 
   private onDeletePasswordClick_() {
-    PasswordManagerImpl.getInstance().removeSavedPassword(
+    PasswordManagerImpl.getInstance().removeCredential(
         this.item.id, this.item.storedIn);
     this.dispatchEvent(new CustomEvent('password-removed', {
       bubbles: true,

chrome/browser/resources/password_manager/credential_details/passkey_details_card.html

@@ -5,7 +5,9 @@
   <div class="credential-container">
     <!-- TODO(crbug.com/1432717): i18n title when UI approved -->
     <h4 class="card-title"></h4>
-    <div class="cr-secondary-text label">[[getPasskeyUsageInfoString_()]]</div>
+    <div class="cr-secondary-text label">
+      [[getPasskeyUsageInfoString_(passkey)]]
+    </div>
     <div class="row-container">
       <div class="column-container">
         <!-- TODO(crbug.com/1432717): i18n labels for 'copy display name' and

chrome/browser/resources/password_manager/credential_details/passkey_details_card.ts

@@ -14,7 +14,7 @@ import {CrButtonElement} from 'chrome://resources/cr_elements/cr_button/cr_butto
 import {I18nMixin} from 'chrome://resources/cr_elements/i18n_mixin.js';
 import {PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
-import {PasswordViewPageInteractions} from '../password_manager_proxy.js';
+import {PasswordManagerImpl, PasswordViewPageInteractions} from '../password_manager_proxy.js';
 
 import {CredentialFieldElement} from './credential_field.js';
 import {CredentialNoteElement} from './credential_note.js';
@@ -82,7 +82,11 @@ export class PasskeyDetailsCardElement extends PasskeyDetailsCardElementBase {
   }
 
   private onDeleteClick_() {
-    // TODO(crbug.com/1432717): fill this in.
+    // TODO(crbug.com/1432717): show a modal dialog instead.
+    PasswordManagerImpl.getInstance().recordPasswordViewInteraction(
+        PasswordViewPageInteractions.PASSKEY_DELETE_BUTTON_CLICKED);
+    PasswordManagerImpl.getInstance().removeCredential(
+        this.passkey.id, this.passkey.storedIn);
   }
 
   private onEditClicked_() {

chrome/browser/resources/password_manager/credential_details/password_details_card.ts

@@ -137,7 +137,7 @@ export class PasswordDetailsCardElement extends PasswordDetailsCardElementBase {
       this.showDeletePasswordDialog_ = true;
       return;
     }
-    PasswordManagerImpl.getInstance().removeSavedPassword(
+    PasswordManagerImpl.getInstance().removeCredential(
         this.password.id, this.password.storedIn);
     this.dispatchEvent(new CustomEvent('password-removed', {
       bubbles: true,

chrome/browser/resources/password_manager/dialogs/multi_store_delete_password_dialog.ts

@@ -95,7 +95,7 @@ export class MultiStoreDeletePasswordDialogElement extends
     } else {
       assert(this.removeFromDeviceChecked_);
     }
-    PasswordManagerImpl.getInstance().removeSavedPassword(
+    PasswordManagerImpl.getInstance().removeCredential(
         this.duplicatedPassword.id, fromStores);
     this.dispatchEvent(new CustomEvent('password-removed', {
       bubbles: true,

chrome/browser/resources/password_manager/password_manager_proxy.ts

@@ -65,8 +65,9 @@ export enum PasswordViewPageInteractions {
   TIMED_OUT_IN_VIEW_PAGE = 10,
   CREDENTIAL_REQUESTED_BY_URL = 11,
   PASSKEY_DISPLAY_NAME_COPY_BUTTON_CLICKED = 12,
+  PASSKEY_DELETE_BUTTON_CLICKED = 13,
   // Must be last.
-  COUNT = 13,
+  COUNT = 14,
 }
 
 /**
@@ -209,12 +210,12 @@ export interface PasswordManagerProxy {
       Promise<number>;
 
   /**
-   * Should remove the saved password and notify that the list has changed.
-   * @param id The id for the password entry being removed. No-op if |id| is not
-   *     in the list.
+   * Should remove the credential and notify that the list has changed.
+   * @param id The id for the credential being removed. No-op if |id| is not in
+   *     the list.
    * @param fromStores The store from which credential should be removed.
    */
-  removeSavedPassword(
+  removeCredential(
       id: number, fromStores: chrome.passwordsPrivate.PasswordStoreSet): void;
 
   /**
@@ -475,9 +476,9 @@ export class PasswordManagerImpl implements PasswordManagerProxy {
     return chrome.passwordsPrivate.changeSavedPassword(id, params);
   }
 
-  removeSavedPassword(
+  removeCredential(
       id: number, fromStores: chrome.passwordsPrivate.PasswordStoreSet) {
-    chrome.passwordsPrivate.removeSavedPassword(id, fromStores);
+    chrome.passwordsPrivate.removeCredential(id, fromStores);
   }
 
   removeBlockedSite(id: number) {

chrome/browser/resources/settings/autofill_page/password_manager_proxy.ts

@@ -95,12 +95,12 @@ export interface PasswordManagerProxy {
       Promise<number>;
 
   /**
-   * Should remove the saved password and notify that the list has changed.
-   * @param id The id for the password entry being removed. No-op if |id| is not
-   *     in the list.
+   * Should remove the credential and notify that the list has changed.
+   * @param id The id for the credential being removed. No-op if |id| is not in
+   *     the list.
    * @param fromStores The store from which credential should be removed.
    */
-  removeSavedPassword(
+  removeCredential(
       id: number, fromStores: chrome.passwordsPrivate.PasswordStoreSet): void;
 
   /**
@@ -413,9 +413,9 @@ export class PasswordManagerImpl implements PasswordManagerProxy {
     return chrome.passwordsPrivate.changeSavedPassword(id, params);
   }
 
-  removeSavedPassword(
+  removeCredential(
       id: number, fromStores: chrome.passwordsPrivate.PasswordStoreSet) {
-    chrome.passwordsPrivate.removeSavedPassword(id, fromStores);
+    chrome.passwordsPrivate.removeCredential(id, fromStores);
   }
 
   movePasswordsToAccount(ids: number[]) {

chrome/browser/resources/settings/autofill_page/password_removal_mixin.ts

@@ -37,7 +37,7 @@ export const PasswordRemovalMixin = dedupingMixin(
             return false;
           }
 
-          PasswordManagerImpl.getInstance().removeSavedPassword(
+          PasswordManagerImpl.getInstance().removeCredential(
               password.id, password.storedIn);
           return true;
         }