Remove cleaning username/password on the blacklisted credentials

The code is executed once per profile. As it was introduced 5 months ago,
and acording to metric reported users don't need it anymore.
It's a manual revert of https://crrev.com/c/951607/

Bug: 817754
Change-Id: Iab2c5dfdb9effb4e2347f174faebeb67f9dd6a3e
Reviewed-on: https://chromium-review.googlesource.com/1179151Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Narcis Gemene <gemene@google.com>
Cr-Commit-Position: refs/heads/master@{#584675}

chrome/browser/password_manager/password_store_factory.cc

@@ -288,7 +288,7 @@ PasswordStoreFactory::BuildServiceInstanceFor(
   ps->PreparePasswordHashData(GetSyncUsername(profile));
 #endif
 
-  password_manager_util::CleanBlacklistedCredentials(ps.get(),
+  password_manager_util::DeleteBlacklistedDuplicates(ps.get(),
                                                      profile->GetPrefs(), 60);
   base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,

components/password_manager/core/browser/password_manager.cc

@@ -291,7 +291,6 @@ void PasswordManager::RegisterProfilePrefs(
   registry->RegisterBooleanPref(
       prefs::kWasAutoSignInFirstRunExperienceShown, false,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF);
-  registry->RegisterBooleanPref(prefs::kBlacklistedCredentialsStripped, false);
   registry->RegisterBooleanPref(prefs::kDuplicatedBlacklistedCredentialsRemoved,
                                 false);
 #if defined(OS_MACOSX)

components/password_manager/core/browser/password_manager_util.cc

@@ -32,57 +32,20 @@ using autofill::PasswordForm;
 namespace password_manager_util {
 namespace {
 
-// Clears username/password on the blacklisted credentials and delete
-// blacklisted duplicates.
-class BlacklistedCredentialsCleaner
+// This class is responsible for deleting blacklisted duplicates.
+class BlacklistedDuplicatesCleaner
     : public password_manager::PasswordStoreConsumer {
  public:
-  BlacklistedCredentialsCleaner(password_manager::PasswordStore* store,
-                                PrefService* prefs)
+  BlacklistedDuplicatesCleaner(password_manager::PasswordStore* store,
+                               PrefService* prefs)
       : store_(store), prefs_(prefs) {
     store_->GetBlacklistLogins(this);
   }
-  ~BlacklistedCredentialsCleaner() override = default;
+  ~BlacklistedDuplicatesCleaner() override = default;
 
+  // PasswordStoreConsumer:
   void OnGetPasswordStoreResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results) override {
-    if (!prefs_->GetBoolean(
-            password_manager::prefs::kBlacklistedCredentialsStripped))
-      RemoveUsernameAndPassword(results);
-
-    if (!prefs_->GetBoolean(
-            password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved))
-      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<PasswordForm>>& results) {
-    bool cleaned_something = false;
-    for (const auto& form : results) {
-      DCHECK(form->blacklisted_by_user);
-      if (!form->username_value.empty() || !form->password_value.empty()) {
-        cleaned_something = true;
-        store_->RemoveLogin(*form);
-        form->username_value.clear();
-        form->password_value.clear();
-        store_->AddLogin(*form);
-      }
-    }
-
-    // Update the pref if no forms were handled. The password store is async,
-    // therefore, one can't be sure that the changes applied cleanly.
-    if (!cleaned_something) {
-      prefs_->SetBoolean(
-          password_manager::prefs::kBlacklistedCredentialsStripped, true);
-    }
-  }
-
-  void RemoveDuplicates(
-      const std::vector<std::unique_ptr<PasswordForm>>& results) {
     std::set<std::string> signon_realms;
     for (const auto& form : results) {
       DCHECK(form->blacklisted_by_user);
@@ -97,19 +60,21 @@ class BlacklistedCredentialsCleaner
           password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved,
           true);
     }
+    delete this;
   }
 
+ private:
   password_manager::PasswordStore* store_;
   PrefService* prefs_;
 
-  DISALLOW_COPY_AND_ASSIGN(BlacklistedCredentialsCleaner);
+  DISALLOW_COPY_AND_ASSIGN(BlacklistedDuplicatesCleaner);
 };
 
-void StartCleaningBlacklisted(
+void StartDeletingBlacklistedDuplicates(
     const scoped_refptr<password_manager::PasswordStore>& store,
     PrefService* prefs) {
   // The object will delete itself once the credentials are retrieved.
-  new BlacklistedCredentialsCleaner(store.get(), prefs);
+  new BlacklistedDuplicatesCleaner(store.get(), prefs);
 }
 
 // Return true if
@@ -383,23 +348,19 @@ void UserTriggeredManualGenerationFromContextMenu(
       autofill::password_generation::PASSWORD_GENERATION_CONTEXT_MENU_PRESSED);
 }
 
-void CleanBlacklistedCredentials(password_manager::PasswordStore* store,
+void DeleteBlacklistedDuplicates(password_manager::PasswordStore* store,
                                  PrefService* prefs,
                                  int delay_in_seconds) {
-  const bool need_to_stripe_username = !prefs->GetBoolean(
-      password_manager::prefs::kBlacklistedCredentialsStripped);
-  base::UmaHistogramBoolean("PasswordManager.BlacklistedSites.NeedToBeCleaned",
-                            need_to_stripe_username);
   const bool need_to_remove_blacklisted_duplicates = !prefs->GetBoolean(
       password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved);
   base::UmaHistogramBoolean(
       "PasswordManager.BlacklistedSites.NeedRemoveBlacklistDuplicates",
       need_to_remove_blacklisted_duplicates);
-  if (need_to_stripe_username || need_to_remove_blacklisted_duplicates)
+  if (need_to_remove_blacklisted_duplicates)
     base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
-        base::BindOnce(&StartCleaningBlacklisted, base::WrapRefCounted(store),
-                       prefs),
+        base::BindOnce(&StartDeletingBlacklistedDuplicates,
+                       base::WrapRefCounted(store), prefs),
         base::TimeDelta::FromSeconds(delay_in_seconds));
 }
 

components/password_manager/core/browser/password_manager_util.h

@@ -89,10 +89,9 @@ void UserTriggeredShowAllSavedPasswordsFromContextMenu(
 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) and delete
-// blacklisted duplicates.
-void CleanBlacklistedCredentials(password_manager::PasswordStore* store,
+// Two blacklisted forms are considered equal if they have the same
+// signon_realm.
+void DeleteBlacklistedDuplicates(password_manager::PasswordStore* store,
                                  PrefService* prefs,
                                  int delay_in_seconds);
 

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -31,7 +31,6 @@ 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";
@@ -85,57 +84,6 @@ TEST(PasswordManagerUtil, TrimUsernameOnlyCredentials) {
   EXPECT_THAT(forms, UnorderedPasswordFormElementsAre(&expected_forms));
 }
 
-TEST(PasswordManagerUtil, CleanBlacklistedUsernamePassword) {
-  autofill::PasswordForm blacklisted;
-  blacklisted.blacklisted_by_user = true;
-  blacklisted.signon_realm = kTestURL;
-  blacklisted.origin = GURL(kTestURL);
-
-  autofill::PasswordForm blacklisted_with_username = blacklisted;
-  blacklisted_with_username.username_value = base::ASCIIToUTF16(kTestUsername);
-
-  autofill::PasswordForm blacklisted_with_password = blacklisted;
-  blacklisted_with_password.password_value = base::ASCIIToUTF16(kTestPassword);
-
-  base::test::ScopedTaskEnvironment scoped_task_environment;
-  TestingPrefServiceSimple prefs;
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kBlacklistedCredentialsStripped, false);
-  // Prevent cleaning of duplicated blacklist entries.
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved, true);
-  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_with_username),
-                      AppendForm(blacklisted_with_password), Return(true)));
-  // Wrong credentials are to be cleaned.
-  EXPECT_CALL(*password_store, RemoveLogin(blacklisted_with_username));
-  EXPECT_CALL(*password_store, RemoveLogin(blacklisted_with_password));
-  EXPECT_CALL(*password_store, AddLogin(blacklisted)).Times(2);
-
-  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
-  scoped_task_environment.RunUntilIdle();
-
-  EXPECT_FALSE(prefs.GetBoolean(
-      password_manager::prefs::kBlacklistedCredentialsStripped));
-
-  // Clean up with no credentials to be updated.
-  EXPECT_CALL(*password_store, FillBlacklistLogins(_))
-      .WillOnce(DoAll(AppendForm(blacklisted), Return(true)));
-  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
-  scoped_task_environment.RunUntilIdle();
-
-  EXPECT_TRUE(prefs.GetBoolean(
-      password_manager::prefs::kBlacklistedCredentialsStripped));
-
-  password_store->ShutdownOnUIThread();
-}
-
 TEST(PasswordManagerUtil, RemoveBlacklistedDuplicates) {
   autofill::PasswordForm blacklisted;
   blacklisted.blacklisted_by_user = true;
@@ -151,12 +99,6 @@ TEST(PasswordManagerUtil, RemoveBlacklistedDuplicates) {
   base::test::ScopedTaskEnvironment scoped_task_environment;
   TestingPrefServiceSimple prefs;
 
-  // In this test we are explicitly only testing the clean up of duplicated
-  // credentials and setting this true will prevent making other unrelated
-  // clean-up.
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kBlacklistedCredentialsStripped, true);
-
   prefs.registry()->RegisterBooleanPref(
       password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved, false);
 
@@ -173,56 +115,11 @@ TEST(PasswordManagerUtil, RemoveBlacklistedDuplicates) {
 
   // Duplicated credentials are to be deleted.
   EXPECT_CALL(*password_store, RemoveLogin(blacklisted_first_example));
-  CleanBlacklistedCredentials(password_store.get(), &prefs, 0);
+  DeleteBlacklistedDuplicates(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;
-
-  // Here we test the behavior when the Password Store contains blacklisted
-  // entries with password or username values that have to be cleared and
-  // blacklisted duplicates, too.
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kBlacklistedCredentialsStripped, false);
-
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved, 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();
-}
-
 #if !defined(OS_IOS)
 TEST(PasswordManagerUtil, ReportHttpMigrationMetrics) {
   for (bool is_hsts_enabled : {false, true}) {

components/password_manager/core/common/password_manager_pref_names.cc

@@ -42,9 +42,6 @@ const char kSyncPasswordHash[] = "profile.sync_password_hash";
 const char kSyncPasswordLengthAndHashSalt[] =
     "profile.sync_password_length_and_hash_salt";
 
-const char kBlacklistedCredentialsStripped[] =
-    "profile.blacklisted_credentials_stripped";
-
 const char kDuplicatedBlacklistedCredentialsRemoved[] =
     "profile.duplicated_blacklisted_credentials_removed";
 

components/password_manager/core/common/password_manager_pref_names.h

@@ -70,9 +70,6 @@ extern const char kSyncPasswordHash[];
 // int>.<16 char salt>".
 extern const char kSyncPasswordLengthAndHashSalt[];
 
-// Whether Chrome cleaned up username/password in the blacklisted credentials.
-extern const char kBlacklistedCredentialsStripped[];
-
 // Whether Chrome deleted blacklisted credentials that were duplicated.
 extern const char kDuplicatedBlacklistedCredentialsRemoved[];
 

tools/metrics/histograms/histograms.xml

@@ -70417,6 +70417,9 @@ uploading your change for review.
 
 <histogram name="PasswordManager.BlacklistedSites.NeedToBeCleaned"
     enum="BooleanNeedsClearing">
+  <obsolete>
+    Deprecated August 2018.
+  </obsolete>
   <owner>vasilii@chromium.org</owner>
   <summary>
     Records once on startup whether the blacklisted sites in the password store