Move BlacklistedDuplicatesCleaner into a separate class

This CL move the class that removes blacklisted credentials that are
duplicates in separate files and create stronger unittests for that class.

Bug: 862930
Change-Id: Ia07004779bc66d01a3268f7fc4d2779beabb89a2
Reviewed-on: https://chromium-review.googlesource.com/1199385
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@{#588385}

components/password_manager/core/browser/BUILD.gn

@@ -37,6 +37,8 @@ jumbo_static_library("browser") {
     "android_affiliation/lookup_affiliation_response_parser.cc",
     "android_affiliation/lookup_affiliation_response_parser.h",
     "android_affiliation/test_affiliation_fetcher_factory.h",
+    "blacklisted_duplicates_cleaner.cc",
+    "blacklisted_duplicates_cleaner.h",
     "browser_save_password_progress_logger.cc",
     "browser_save_password_progress_logger.h",
     "credential_manager_impl.cc",
@@ -366,6 +368,7 @@ source_set("unit_tests") {
     "android_affiliation/affiliation_service_unittest.cc",
     "android_affiliation/affiliation_utils_unittest.cc",
     "android_affiliation/facet_manager_unittest.cc",
+    "blacklisted_duplicates_cleaner_unittest.cc",
     "browser_save_password_progress_logger_unittest.cc",
     "credential_manager_impl_unittest.cc",
     "credential_manager_logger_unittest.cc",

components/password_manager/core/browser/blacklisted_duplicates_cleaner.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/blacklisted_duplicates_cleaner.h"
+
+#include <set>
+#include <string>
+#include <utility>
+
+#include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_service.h"
+
+namespace password_manager {
+
+BlacklistedDuplicatesCleaner::BlacklistedDuplicatesCleaner(
+    scoped_refptr<PasswordStore> store,
+    PrefService* prefs)
+    : store_(std::move(store)), prefs_(prefs) {
+  store_->GetBlacklistLogins(this);
+}
+
+BlacklistedDuplicatesCleaner::~BlacklistedDuplicatesCleaner() = default;
+
+void BlacklistedDuplicatesCleaner::OnGetPasswordStoreResults(
+    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);
+    }
+  }
+  prefs_->SetBoolean(prefs::kDuplicatedBlacklistedCredentialsRemoved,
+                     results.size() == signon_realms.size());
+  delete this;
+}
+
+}  // namespace password_manager
\ No newline at end of file

components/password_manager/core/browser/blacklisted_duplicates_cleaner.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_BLACKLISTED_DUPLICATES_CLEANER_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_BLACKLISTED_DUPLICATES_CLEANER_H_
+
+#include <memory>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
+
+namespace autofill {
+struct PasswordForm;
+}  // namespace autofill
+
+class PrefService;
+
+namespace password_manager {
+
+class PasswordStore;
+
+// This class is responsible for deleting blacklisted duplicates. Two
+// blacklisted forms are considered equal if they have the same signon_realm.
+// Important note: The object will delete itself once the credentials are
+// cleared.
+// TODO(https://crbug.com/866794): Remove the code once majority of the users
+// executed it.
+class BlacklistedDuplicatesCleaner : public PasswordStoreConsumer {
+ public:
+  BlacklistedDuplicatesCleaner(scoped_refptr<PasswordStore> store,
+                               PrefService* prefs);
+  ~BlacklistedDuplicatesCleaner() override;
+
+  // PasswordStoreConsumer:
+  void OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<autofill::PasswordForm>> results) override;
+
+ private:
+  scoped_refptr<PasswordStore> store_;
+  PrefService* prefs_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlacklistedDuplicatesCleaner);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_BLACKLISTED_DUPLICATES_CLEANER_H_
\ No newline at end of file

components/password_manager/core/browser/blacklisted_duplicates_cleaner_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/blacklisted_duplicates_cleaner.h"
+
+#include "base/stl_util.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_task_environment.h"
+#include "components/password_manager/core/browser/password_manager_util.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"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace password_manager {
+
+class BlacklistedDuplicatesCleanerCleanerTest : public ::testing::Test {
+ public:
+  BlacklistedDuplicatesCleanerCleanerTest() = default;
+
+  ~BlacklistedDuplicatesCleanerCleanerTest() override = default;
+
+ protected:
+  TestPasswordStore* store() { return store_.get(); }
+
+  TestingPrefServiceSimple* prefs() { return &prefs_; }
+
+  bool StoreContains(const autofill::PasswordForm& form) {
+    const auto it = store_->stored_passwords().find(form.signon_realm);
+    return it != store_->stored_passwords().end() &&
+           base::ContainsValue(it->second, form);
+  }
+
+ private:
+  scoped_refptr<TestPasswordStore> store_ =
+      base::MakeRefCounted<TestPasswordStore>();
+  TestingPrefServiceSimple prefs_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlacklistedDuplicatesCleanerCleanerTest);
+};
+
+TEST_F(BlacklistedDuplicatesCleanerCleanerTest, RemoveBlacklistedDuplicates) {
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+  ASSERT_TRUE(
+      store()->Init(syncer::SyncableService::StartSyncFlare(), nullptr));
+
+  autofill::PasswordForm blacklisted;
+  blacklisted.blacklisted_by_user = true;
+  blacklisted.origin = GURL("https://example.com/");
+  blacklisted.signon_realm = "https://example.com/";
+  store()->AddLogin(blacklisted);
+
+  autofill::PasswordForm blacklisted_different;
+  blacklisted_different.blacklisted_by_user = true;
+  blacklisted_different.origin = GURL("https://host.com/");
+  blacklisted_different.signon_realm = "https://host.com/";
+  store()->AddLogin(blacklisted_different);
+
+  autofill::PasswordForm blacklisted_duplicated;
+  blacklisted_duplicated.blacklisted_by_user = true;
+  blacklisted_duplicated.origin = GURL("https://example.com/duplicated/");
+  blacklisted_duplicated.signon_realm = "https://example.com/";
+  store()->AddLogin(blacklisted_duplicated);
+
+  autofill::PasswordForm not_blacklisted;
+  not_blacklisted.blacklisted_by_user = false;
+  not_blacklisted.origin = GURL("https://google.com/");
+  not_blacklisted.signon_realm = "https://google.com/";
+  store()->AddLogin(not_blacklisted);
+
+  scoped_task_environment.RunUntilIdle();
+
+  // Check that all credentials were successfully added.
+  ASSERT_TRUE(StoreContains(blacklisted));
+  ASSERT_TRUE(StoreContains(blacklisted_different));
+  ASSERT_TRUE(StoreContains(blacklisted_duplicated));
+  ASSERT_TRUE(StoreContains(not_blacklisted));
+
+  prefs()->registry()->RegisterBooleanPref(
+      prefs::kDuplicatedBlacklistedCredentialsRemoved, false);
+
+  password_manager_util::DeleteBlacklistedDuplicates(store(), prefs(), 0);
+  scoped_task_environment.RunUntilIdle();
+
+  // Check that one of the next two forms was removed.
+  EXPECT_NE(StoreContains(blacklisted_duplicated), StoreContains(blacklisted));
+
+  EXPECT_TRUE(StoreContains(blacklisted_different));
+  EXPECT_TRUE(StoreContains(not_blacklisted));
+
+  EXPECT_FALSE(
+      prefs()->GetBoolean(prefs::kDuplicatedBlacklistedCredentialsRemoved));
+
+  password_manager_util::DeleteBlacklistedDuplicates(store(), prefs(), 0);
+  scoped_task_environment.RunUntilIdle();
+  EXPECT_TRUE(
+      prefs()->GetBoolean(prefs::kDuplicatedBlacklistedCredentialsRemoved));
+
+  store()->ShutdownOnUIThread();
+  scoped_task_environment.RunUntilIdle();
+}
+
+}  // namespace password_manager
\ No newline at end of file

components/password_manager/core/browser/password_manager_util.cc

@@ -16,6 +16,7 @@
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_generation_util.h"
+#include "components/password_manager/core/browser/blacklisted_duplicates_cleaner.h"
 #include "components/password_manager/core/browser/hsts_query.h"
 #include "components/password_manager/core/browser/log_manager.h"
 #include "components/password_manager/core/browser/password_generation_manager.h"
@@ -33,49 +34,11 @@ using autofill::PasswordForm;
 namespace password_manager_util {
 namespace {
 
-// This class is responsible for deleting blacklisted duplicates.
-class BlacklistedDuplicatesCleaner
-    : public password_manager::PasswordStoreConsumer {
- public:
-  BlacklistedDuplicatesCleaner(password_manager::PasswordStore* store,
-                               PrefService* prefs)
-      : store_(store), prefs_(prefs) {
-    store_->GetBlacklistLogins(this);
-  }
-  ~BlacklistedDuplicatesCleaner() override = default;
-
-  // PasswordStoreConsumer:
-  void OnGetPasswordStoreResults(
-      std::vector<std::unique_ptr<autofill::PasswordForm>> results) override {
-    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);
-      }
-    }
-    const size_t duplicates = results.size() - signon_realms.size();
-    if (duplicates == 0) {
-      prefs_->SetBoolean(
-          password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved,
-          true);
-    }
-    delete this;
-  }
-
- private:
-  password_manager::PasswordStore* store_;
-  PrefService* prefs_;
-
-  DISALLOW_COPY_AND_ASSIGN(BlacklistedDuplicatesCleaner);
-};
-
 void StartDeletingBlacklistedDuplicates(
-    const scoped_refptr<password_manager::PasswordStore>& store,
+    scoped_refptr<password_manager::PasswordStore> store,
     PrefService* prefs) {
   // The object will delete itself once the credentials are retrieved.
-  new BlacklistedDuplicatesCleaner(store.get(), prefs);
+  new password_manager::BlacklistedDuplicatesCleaner(std::move(store), prefs);
 }
 
 // Return true if
@@ -370,9 +333,10 @@ void UserTriggeredManualGenerationFromContextMenu(
       autofill::password_generation::PASSWORD_GENERATION_CONTEXT_MENU_PRESSED);
 }
 
-void DeleteBlacklistedDuplicates(password_manager::PasswordStore* store,
-                                 PrefService* prefs,
-                                 int delay_in_seconds) {
+void DeleteBlacklistedDuplicates(
+    scoped_refptr<password_manager::PasswordStore> store,
+    PrefService* prefs,
+    int delay_in_seconds) {
   const bool need_to_remove_blacklisted_duplicates = !prefs->GetBoolean(
       password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved);
   base::UmaHistogramBoolean(
@@ -381,8 +345,8 @@ void DeleteBlacklistedDuplicates(password_manager::PasswordStore* store,
   if (need_to_remove_blacklisted_duplicates)
     base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
-        base::BindOnce(&StartDeletingBlacklistedDuplicates,
-                       base::WrapRefCounted(store), prefs),
+        base::BindOnce(&StartDeletingBlacklistedDuplicates, std::move(store),
+                       prefs),
         base::TimeDelta::FromSeconds(delay_in_seconds));
 }
 

components/password_manager/core/browser/password_manager_util.h

@@ -93,9 +93,10 @@ void UserTriggeredManualGenerationFromContextMenu(
 
 // 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);
+void DeleteBlacklistedDuplicates(
+    scoped_refptr<password_manager::PasswordStore> store,
+    PrefService* prefs,
+    int delay_in_seconds);
 
 // Report metrics about HTTP to HTTPS migration process. This function cannot be
 // used on iOS platform because the HSTS query is not supported.

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -20,8 +20,6 @@
 #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"
 #include "net/url_request/url_request_test_util.h"
 #include "services/network/network_context.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -34,7 +32,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 kTestUsername[] = "Username";
 constexpr char kTestUsername2[] = "Username2";
 constexpr char kTestPassword[] = "12345";
@@ -88,42 +85,6 @@ TEST(PasswordManagerUtil, TrimUsernameOnlyCredentials) {
   EXPECT_THAT(forms, UnorderedPasswordFormElementsAre(&expected_forms));
 }
 
-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;
-
-  prefs.registry()->RegisterBooleanPref(
-      password_manager::prefs::kDuplicatedBlacklistedCredentialsRemoved, 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));
-  DeleteBlacklistedDuplicates(password_store.get(), &prefs, 0);
-  scoped_task_environment.RunUntilIdle();
-  password_store->ShutdownOnUIThread();
-}
-
 #if !defined(OS_IOS)
 TEST(PasswordManagerUtil, ReportHttpMigrationMetrics) {
   enum class HttpCredentialType { kNoMatching, kEquivalent, kConflicting };