Handle credential edit & delete when grouping enabled.

- If a credential has more than one website, when we edit it or when we
delete it, it should update the corresponding password forms.
- Fix index calculation in password_details_coordinator.mm

Bug: 1406290
Change-Id: Id81c0fcdc6550aaad5bd8f7d76a5e27cb97a8e77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4151472
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Viktor Semeniuk <vsemeniuk@google.com>
Commit-Queue: Veronique Nguyen <veronguyen@google.com>
Cr-Commit-Position: refs/heads/main@{#1091940}

components/password_manager/core/browser/password_list_sorter.cc

@@ -89,22 +89,27 @@ std::string CreateSortKey(const CredentialUIEntry& credential) {
 }
 
 std::string CreateUsernamePasswordSortKey(const PasswordForm& form) {
+  return CreateUsernamePasswordSortKey(CredentialUIEntry(form));
+}
+
+std::string CreateUsernamePasswordSortKey(const CredentialUIEntry& credential) {
   std::string key;
   // The origin isn't taken into account for normal credentials since we want to
   // group them together.
-  if (!form.blocked_by_user) {
-    key += base::UTF16ToUTF8(form.username_value) + kSortKeyPartsSeparator +
-           base::UTF16ToUTF8(form.password_value);
+  if (!credential.blocked_by_user) {
+    key += base::UTF16ToUTF8(credential.username) + kSortKeyPartsSeparator +
+           base::UTF16ToUTF8(credential.password);
 
     key += kSortKeyPartsSeparator;
-    if (!form.federation_origin.opaque())
-      key += form.federation_origin.host();
-    else
+    if (!credential.federation_origin.opaque()) {
+      key += credential.federation_origin.host();
+    } else {
       key += kSortKeyNoFederationSymbol;
+    }
   } else {
     // Key for blocked by user credential since it does not store username and
     // password. These credentials are not grouped together.
-    key = GetShownOrigin(CredentialUIEntry(form));
+    key = GetShownOrigin(credential);
   }
   return key;
 }

components/password_manager/core/browser/password_list_sorter.h

@@ -37,6 +37,8 @@ std::string CreateSortKey(const CredentialUIEntry& credential);
 // Creates a key to map passwords within an affiliated group with the same
 // username and password.
 std::string CreateUsernamePasswordSortKey(const PasswordForm& form);
+// Same as |CreateUsernamePasswordSortKey| for |PasswordForm|.
+std::string CreateUsernamePasswordSortKey(const CredentialUIEntry& credential);
 
 // Sort entries of |list| based on sort key. The key is the concatenation of
 // origin, entry type (non-Android credential, Android w/ affiliated web realm

components/password_manager/core/browser/ui/password_grouping_util.cc

@@ -43,6 +43,29 @@ PasswordGroupingInfo& PasswordGroupingInfo::operator=(
 PasswordGroupingInfo& PasswordGroupingInfo::operator=(
     PasswordGroupingInfo&& other) = default;
 
+std::vector<PasswordForm> PasswordGroupingInfo::GetPasswordFormsVector(
+    const CredentialUIEntry& credential) const {
+  std::vector<PasswordForm> forms;
+  auto group_id_iterator = map_signon_realm_to_group_id.find(
+      SignonRealm(credential.GetFirstSignonRealm()));
+  if (group_id_iterator == map_signon_realm_to_group_id.end()) {
+    return forms;
+  }
+  GroupId group_id = group_id_iterator->second;
+  auto group_iterator = map_group_id_to_forms.find(group_id);
+  if (group_iterator == map_group_id_to_forms.end()) {
+    return forms;
+  }
+  std::map<UsernamePasswordKey, std::vector<PasswordForm>> map =
+      group_iterator->second;
+  auto forms_iterator =
+      map.find(UsernamePasswordKey(CreateUsernamePasswordSortKey(credential)));
+  if (forms_iterator != map.end()) {
+    forms = forms_iterator->second;
+  }
+  return forms;
+}
+
 FacetBrandingInfo CreateBrandingInfoFromFacetURI(
     const CredentialUIEntry& credential) {
   FacetBrandingInfo branding_info;

components/password_manager/core/browser/ui/password_grouping_util.h

@@ -41,6 +41,19 @@ struct PasswordGroupingInfo {
   // Structure to keep track of the blocked sites by user. They are not grouped
   // into affiliated groups.
   std::vector<password_manager::CredentialUIEntry> blocked_sites;
+
+  // Call clear method on all the variables in this struct.
+  void clear() {
+    map_signon_realm_to_group_id.clear();
+    map_group_id_to_branding_info.clear();
+    map_group_id_to_forms.clear();
+    blocked_sites.clear();
+  }
+
+  // Returns the vector of PasswordForm corresponding to the CredentialUIEntry
+  // object.
+  std::vector<PasswordForm> GetPasswordFormsVector(
+      const CredentialUIEntry& credential) const;
 };
 
 // Apply grouping algorithm to credentials. The grouping algorithm group

components/password_manager/core/browser/ui/saved_passwords_presenter.cc

@@ -129,6 +129,7 @@ SavedPasswordsPresenter::~SavedPasswordsPresenter() {
 void SavedPasswordsPresenter::Init() {
   // Clear old cache.
   sort_key_to_password_forms_.clear();
+  password_grouping_info_.clear();
 
   profile_store_->AddObserver(this);
   if (account_store_)
@@ -155,24 +156,21 @@ void SavedPasswordsPresenter::RemoveObservers() {
 
 bool SavedPasswordsPresenter::RemoveCredential(
     const CredentialUIEntry& credential) {
-  const auto range =
-      sort_key_to_password_forms_.equal_range(CreateSortKey(credential));
-  bool removed = false;
+  std::vector<PasswordForm> forms_to_delete =
+      GetCorrespondingPasswordForms(credential);
   undo_helper_->StartGroupingActions();
-  std::for_each(range.first, range.second, [&](const auto& pair) {
-    const auto& current_form = pair.second;
+  for (const auto& current_form : forms_to_delete) {
     // Make sure |credential| and |current_form| share the same store.
     if (credential.stored_in.contains(current_form.in_store)) {
       // |current_form| is unchanged result obtained from
-      // 'OnGetPasswordStoreResultsFrom'. So it can be present only in one store
-      // at a time..
+      // 'OnGetPasswordStoreResultsFrom'. So it can be present only in one
+      // store at a time.
       GetStoreFor(current_form).RemoveLogin(current_form);
       undo_helper_->PasswordRemoved(current_form);
-      removed = true;
     }
-  });
+  }
   undo_helper_->EndGroupingActions();
-  return removed;
+  return !forms_to_delete.empty();
 }
 
 void SavedPasswordsPresenter::UndoLastRemoval() {
@@ -463,11 +461,17 @@ std::vector<CredentialUIEntry> SavedPasswordsPresenter::GetBlockedSites() {
 std::vector<PasswordForm>
 SavedPasswordsPresenter::GetCorrespondingPasswordForms(
     const CredentialUIEntry& credential) const {
-  const auto range =
-      sort_key_to_password_forms_.equal_range(CreateSortKey(credential));
   std::vector<PasswordForm> forms;
-  base::ranges::transform(range.first, range.second, std::back_inserter(forms),
-                          [](const auto& pair) { return pair.second; });
+  if (base::FeatureList::IsEnabled(
+          password_manager::features::kPasswordsGrouping)) {
+    forms = password_grouping_info_.GetPasswordFormsVector(credential);
+  } else {
+    const auto range =
+        sort_key_to_password_forms_.equal_range(CreateSortKey(credential));
+    base::ranges::transform(range.first, range.second,
+                            std::back_inserter(forms),
+                            [](const auto& pair) { return pair.second; });
+  }
   return forms;
 }
 

components/password_manager/core/browser/ui/saved_passwords_presenter_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/scoped_observation.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
+#include "base/test/gmock_callback_support.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/scoped_feature_list.h"
@@ -1513,6 +1514,98 @@ TEST_F(SavedPasswordsPresenterWithTwoStoresTest, EditUsername) {
                                ElementsAre(profile_store_form))));
 }
 
+// Tests whether editing passwords in a credential group modify them properly.
+TEST_F(SavedPasswordsPresenterTest, EditPasswordsInCredentialGroup) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      password_manager::features::kPasswordsGrouping);
+
+  PasswordForm form =
+      CreateTestPasswordForm(PasswordForm::Store::kProfileStore);
+  PasswordForm form2 =
+      CreateTestPasswordForm(PasswordForm::Store::kProfileStore);
+  form2.url = GURL("https://m.test0.com");
+  form2.signon_realm = form2.url.spec();
+
+  store().AddLogin(form);
+  store().AddLogin(form2);
+
+  std::vector<password_manager::GroupedFacets> grouped_facets(1);
+  Facet facet;
+  facet.uri = FacetURI::FromPotentiallyInvalidSpec(form.signon_realm);
+  grouped_facets[0].facets.push_back(std::move(facet));
+  Facet facet2;
+  facet2.uri = FacetURI::FromPotentiallyInvalidSpec(form2.signon_realm);
+  grouped_facets[0].facets.push_back(std::move(facet2));
+  EXPECT_CALL(affiliation_service(), GetAllGroups)
+      .WillRepeatedly(base::test::RunOnceCallback<0>(grouped_facets));
+
+  RunUntilIdle();
+
+  std::vector<PasswordForm> original_forms = {form, form2};
+  CredentialUIEntry original_credential(original_forms);
+
+  // Prepare updated credential.
+  const std::u16string new_password = u"new_password";
+  CredentialUIEntry updated_credential(original_forms);
+  updated_credential.password = new_password;
+
+  // Expect successful passwords editing.
+  EXPECT_EQ(SavedPasswordsPresenter::EditResult::kSuccess,
+            presenter().EditSavedCredentials(CredentialUIEntry({form, form2}),
+                                             updated_credential));
+  base::Time date_password_modified = base::Time::Now();
+  RunUntilIdle();
+
+  // Prepare expected updated forms.
+  PasswordForm updated1 = form;
+  updated1.password_value = new_password;
+  updated1.date_password_modified = date_password_modified;
+  PasswordForm updated2 = form2;
+  updated2.password_value = new_password;
+  updated2.date_password_modified = date_password_modified;
+
+  EXPECT_THAT(store().stored_passwords(),
+              UnorderedElementsAre(
+                  Pair(form.signon_realm, UnorderedElementsAre(updated1)),
+                  Pair(form2.signon_realm, ElementsAre(updated2))));
+}
+
+// Tests whether deleting passwords in a credential group works properly.
+TEST_F(SavedPasswordsPresenterTest, DeletePasswordsInCredentialGroup) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      password_manager::features::kPasswordsGrouping);
+
+  PasswordForm form =
+      CreateTestPasswordForm(PasswordForm::Store::kProfileStore);
+  PasswordForm form2 =
+      CreateTestPasswordForm(PasswordForm::Store::kProfileStore);
+  form2.url = GURL("https://m.test0.com");
+  form2.signon_realm = form2.url.spec();
+
+  store().AddLogin(form);
+  store().AddLogin(form2);
+
+  std::vector<password_manager::GroupedFacets> grouped_facets(1);
+  Facet facet;
+  facet.uri = FacetURI::FromPotentiallyInvalidSpec(form.signon_realm);
+  grouped_facets[0].facets.push_back(std::move(facet));
+  Facet facet2;
+  facet2.uri = FacetURI::FromPotentiallyInvalidSpec(form2.signon_realm);
+  grouped_facets[0].facets.push_back(std::move(facet2));
+  EXPECT_CALL(affiliation_service(), GetAllGroups)
+      .WillRepeatedly(base::test::RunOnceCallback<0>(grouped_facets));
+
+  RunUntilIdle();
+
+  // Delete credential with multiple facets.
+  presenter().RemoveCredential(CredentialUIEntry({form, form2}));
+  RunUntilIdle();
+
+  EXPECT_TRUE(store().IsEmpty());
+}
+
 // Tests that duplicates of credentials are removed only from the store that
 // the initial credential belonged to.
 TEST_F(SavedPasswordsPresenterWithTwoStoresTest, DeleteCredentialProfileStore) {

ios/chrome/browser/ui/settings/password/password_details/password_details_coordinator.mm

@@ -255,7 +255,7 @@ - (void)showPasswordDeleteDialogWithPasswordDetails:(PasswordDetails*)password
                 isEqualToString:base::SysUTF16ToNSString(credential.password)];
       });
   if (it != self.mediator.credentials.end()) {
-    int index = it - self.mediator.credentials.begin();
+    int index = std::distance(self.mediator.credentials.begin(), it);
     DCHECK((unsigned long)index < self.mediator.credentials.size());
     [self showPasswordDeleteDialogWithOrigin:password.origin
                          compromisedPassword:password.isCompromised