Remove blacklisted duplicates

Blacklisted credentials are special PasswordForm instances saved in PasswordStore, which record the user's choice never to store credentials for a given site.
When PasswordFormManager asks for all stored forms for a given PasswordStore::FormDigest(scheme, realm and origin), it suppresses all saving functions as long as there is at least one entry marked as blacklisted.

In particular, saving more than one blacklisted entry for a given scheme/realm/origin combination has no additional effect compared to just one. Nevertheless, duplicates could have been created by older versions of Chrome, and their frequency is measured by the PasswordManager.BlacklistedDuplicates histogram.

To avoid potentially confusing the user by copies of the blacklisted entries and also to avoid potential bugs when deleting just one blacklisted entry would keep the site blacklisted, this CL makes the PasswordStore prune duplicated blacklist entries on start-up.

Bug: 862930
Change-Id: I48d8919139c50c267053244573b70b246e9e99b2
Reviewed-on: https://chromium-review.googlesource.com/1140158
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578318}

chrome/browser/password_manager/password_store_factory.cc

@@ -275,10 +275,8 @@ PasswordStoreFactory::BuildServiceInstanceFor(
   ps->PreparePasswordHashData(GetSyncUsername(profile));
 #endif
 
-  // TODO(https://crbug.com/817754): remove the code once majority of the users
-  // executed it.
-  password_manager_util::CleanUserDataInBlacklistedCredentials(
-      ps.get(), profile->GetPrefs(), 60);
+  password_manager_util::CleanBlacklistedCredentials(ps.get(),
+                                                     profile->GetPrefs(), 60);
 
 #if defined(OS_WIN) || defined(OS_MACOSX) || \
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))

components/password_manager/core/browser/password_manager_util.cc

@@ -5,6 +5,8 @@
 #include "components/password_manager/core/browser/password_manager_util.h"
 
 #include <algorithm>
+#include <set>
+#include <string>
 
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ -20,13 +22,13 @@
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync/driver/sync_service.h"
-
 using autofill::PasswordForm;
 
 namespace password_manager_util {
 namespace {
 
-// Clears username/password on the blacklisted credentials.
+// Clears username/password on the blacklisted credentials and delete
+// blacklisted duplicates.
 class BlacklistedCredentialsCleaner
     : public password_manager::PasswordStoreConsumer {
  public:
@@ -39,6 +41,21 @@ class BlacklistedCredentialsCleaner
 
   void OnGetPasswordStoreResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results) override {
+    bool need_to_clean = !prefs_->GetBoolean(
+        password_manager::prefs::kBlacklistedCredentialsStripped);
+    UMA_HISTOGRAM_BOOLEAN("PasswordManager.BlacklistedSites.NeedToBeCleaned",
+                          need_to_clean);
+    if (need_to_clean)
+      RemoveUsernameAndPassword(results);
+    RemoveDuplicates(results);
+    delete this;
+  }
+
+ private:
+  // TODO(https://crbug.com/817754): remove the code once majority of the users
+  // executed it.
+  void RemoveUsernameAndPassword(
+      const std::vector<std::unique_ptr<autofill::PasswordForm>>& results) {
     bool cleaned_something = false;
     for (const auto& form : results) {
       DCHECK(form->blacklisted_by_user);
@@ -57,10 +74,20 @@ class BlacklistedCredentialsCleaner
       prefs_->SetBoolean(
           password_manager::prefs::kBlacklistedCredentialsStripped, true);
     }
-    delete this;
   }
 
- private:
+  void RemoveDuplicates(
+      const std::vector<std::unique_ptr<autofill::PasswordForm>>& results) {
+    std::set<std::string> signon_realms;
+    for (const auto& form : results) {
+      DCHECK(form->blacklisted_by_user);
+      if (!signon_realms.insert(form->signon_realm).second) {
+        // |results| already contain a form with the same signon_realm.
+        store_->RemoveLogin(*form);
+      }
+    }
+  }
+
   password_manager::PasswordStore* store_;
   PrefService* prefs_;
 
@@ -207,21 +234,14 @@ void UserTriggeredManualGenerationFromContextMenu(
       autofill::password_generation::PASSWORD_GENERATION_CONTEXT_MENU_PRESSED);
 }
 
-void CleanUserDataInBlacklistedCredentials(
-    password_manager::PasswordStore* store,
-    PrefService* prefs,
-    int delay_in_seconds) {
-  bool need_to_clean = !prefs->GetBoolean(
-      password_manager::prefs::kBlacklistedCredentialsStripped);
-  UMA_HISTOGRAM_BOOLEAN("PasswordManager.BlacklistedSites.NeedToBeCleaned",
-                        need_to_clean);
-  if (need_to_clean) {
-    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&StartCleaningBlacklisted, base::WrapRefCounted(store),
-                       prefs),
-        base::TimeDelta::FromSeconds(delay_in_seconds));
-  }
+void CleanBlacklistedCredentials(password_manager::PasswordStore* store,
+                                 PrefService* prefs,
+                                 int delay_in_seconds) {
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&StartCleaningBlacklisted, base::WrapRefCounted(store),
+                     prefs),
+      base::TimeDelta::FromSeconds(delay_in_seconds));
 }
 
 void FindBestMatches(

components/password_manager/core/browser/password_manager_util.h

@@ -82,11 +82,11 @@ void UserTriggeredManualGenerationFromContextMenu(
     password_manager::PasswordManagerClient* password_manager_client);
 
 // Clean up the blacklisted entries in the password store. Those shouldn't
-// contain username/password pair. https://crbug.com/817754
-void CleanUserDataInBlacklistedCredentials(
-    password_manager::PasswordStore* store,
-    PrefService* prefs,
-    int delay_in_seconds);
+// contain username/password pair (https://crbug.com/817754) and delete
+// blacklisted duplicates.
+void CleanBlacklistedCredentials(password_manager::PasswordStore* store,
+                                 PrefService* prefs,
+                                 int delay_in_seconds);
 
 // Given all non-blacklisted |matches|, finds and populates
 // |best_matches_|, |preferred_match_| and |non_best_matches_| accordingly.

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/password_manager/core/browser/password_manager_util.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
@@ -12,6 +13,7 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/test_password_store.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
@@ -26,6 +28,7 @@ namespace {
 constexpr char kTestAndroidRealm[] = "android://hash@com.example.beta.android";
 constexpr char kTestFederationURL[] = "https://google.com/";
 constexpr char kTestURL[] = "https://example.com/";
+constexpr char kTestUsernameElement[] = "username_element";
 constexpr char kTestUsername[] = "Username";
 constexpr char kTestUsername2[] = "Username2";
 constexpr char kTestPassword[] = "12345";
@@ -108,7 +111,12 @@ TEST(PasswordManagerUtil, CleanBlacklistedUsernamePassword) {
   EXPECT_CALL(*password_store, RemoveLogin(blacklisted_with_username));
   EXPECT_CALL(*password_store, RemoveLogin(blacklisted_with_password));
   EXPECT_CALL(*password_store, AddLogin(blacklisted)).Times(2);
-  CleanUserDataInBlacklistedCredentials(password_store.get(), &prefs, 0);
+
+  // These deletions are not related to clean-up user credentials.
+  // They are performed by RemoveDuplicated().
+  EXPECT_CALL(*password_store, RemoveLogin(blacklisted)).Times(2);
+
+  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
   scoped_task_environment.RunUntilIdle();
 
   EXPECT_FALSE(prefs.GetBoolean(
@@ -117,18 +125,92 @@ TEST(PasswordManagerUtil, CleanBlacklistedUsernamePassword) {
   // Clean up with no credentials to be updated.
   EXPECT_CALL(*password_store, FillBlacklistLogins(_))
       .WillOnce(DoAll(AppendForm(blacklisted), Return(true)));
-  CleanUserDataInBlacklistedCredentials(password_store.get(), &prefs, 0);
+  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
   scoped_task_environment.RunUntilIdle();
 
   EXPECT_TRUE(prefs.GetBoolean(
       password_manager::prefs::kBlacklistedCredentialsStripped));
 
-  // Clean up again. Nothing should happen.
-  EXPECT_CALL(*password_store, FillBlacklistLogins(_)).Times(0);
-  CleanUserDataInBlacklistedCredentials(password_store.get(), &prefs, 0);
+  password_store->ShutdownOnUIThread();
+}
+
+TEST(PasswordManagerUtil, RemoveBlacklistedDuplicates) {
+  autofill::PasswordForm blacklisted;
+  blacklisted.blacklisted_by_user = true;
+  blacklisted.signon_realm = kTestURL;
+  blacklisted.origin = GURL(kTestURL);
+
+  autofill::PasswordForm blacklisted_first_example = blacklisted;
+  blacklisted_first_example.signon_realm += "first_example";
+
+  autofill::PasswordForm blacklisted_second_example = blacklisted;
+  blacklisted_second_example.signon_realm += "second_example";
+
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+  TestingPrefServiceSimple prefs;
+
+  // The following preference is used inside the blacklist cleaning utility
+  // for an unrelated clean-up
+  prefs.registry()->RegisterBooleanPref(
+      password_manager::prefs::kBlacklistedCredentialsStripped, false);
+
+  auto password_store = base::MakeRefCounted<
+      testing::StrictMock<password_manager::MockPasswordStore>>();
+  ASSERT_TRUE(
+      password_store->Init(syncer::SyncableService::StartSyncFlare(), nullptr));
+
+  EXPECT_CALL(*password_store, FillBlacklistLogins(_))
+      .WillOnce(DoAll(AppendForm(blacklisted),
+                      AppendForm(blacklisted_first_example),
+                      AppendForm(blacklisted_second_example),
+                      AppendForm(blacklisted_first_example), Return(true)));
+
+  // Duplicated credentials are to be deleted.
+  EXPECT_CALL(*password_store, RemoveLogin(blacklisted_first_example));
+  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
+  scoped_task_environment.RunUntilIdle();
+  password_store->ShutdownOnUIThread();
+}
+
+TEST(PasswordManagerUtil, RemoveBlacklistedDuplicatesWithCredentials) {
+  autofill::PasswordForm blacklisted;
+  blacklisted.blacklisted_by_user = true;
+  blacklisted.signon_realm = kTestURL;
+  blacklisted.origin = GURL(kTestURL);
+
+  autofill::PasswordForm blacklisted_with_username_and_password = blacklisted;
+  blacklisted_with_username_and_password.username_element =
+      base::ASCIIToUTF16(kTestUsernameElement);
+  blacklisted_with_username_and_password.username_value =
+      base::ASCIIToUTF16(kTestUsername);
+  blacklisted_with_username_and_password.password_value =
+      base::ASCIIToUTF16(kTestPassword);
+
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+  TestingPrefServiceSimple prefs;
+
+  // The following preference is used inside the blacklist cleaning utility
+  // for an unrelated clean-up
+  prefs.registry()->RegisterBooleanPref(
+      password_manager::prefs::kBlacklistedCredentialsStripped, false);
+
+  auto password_store =
+      base::MakeRefCounted<password_manager::TestPasswordStore>();
+  ASSERT_TRUE(
+      password_store->Init(syncer::SyncableService::StartSyncFlare(), nullptr));
+
+  password_store->AddLogin(blacklisted);
+  password_store->AddLogin(blacklisted_with_username_and_password);
   scoped_task_environment.RunUntilIdle();
 
+  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
+  scoped_task_environment.RunUntilIdle();
+
+  EXPECT_EQ(password_store->stored_passwords(),
+            password_manager::TestPasswordStore::PasswordMap(
+                {{blacklisted.signon_realm, {{blacklisted}}}}));
   password_store->ShutdownOnUIThread();
+  scoped_task_environment.RunUntilIdle();
 }
 
 TEST(PasswordManagerUtil, FindBestMatches) {