Make sure that CompromisedCredentialsObserver removes the credentials

synchronously.

Before this patch CompromisedCredentialsObserver operated on the UI thread.
- [UI] UpdateLogins
- [Background] change the password, notify the observers
- [UI] CompromisedCredentialsObserver processes the update
- [Background] compromised credential removed, observers are notified.

This is not ideal because removal of the compromised credential is
visible on the UI thread much later at unspecified time. Now:
- [UI] UpdateLogins
- [Background] change the password, notify the observers, compromised
password is removed, the observers are notified.
The UI thread sees the compromised credential gone as soon as the new
password value was updated.

Bug: 1049200
Change-Id: I9665bf77a216842925d913479c3d192597f3b606
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2162767Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761942}

components/password_manager/core/browser/compromised_credentials_observer.cc

@@ -13,22 +13,8 @@
 
 namespace password_manager {
 
-CompromisedCredentialsObserver::CompromisedCredentialsObserver(
-    PasswordStore* store)
-    : store_(store) {
-  DCHECK(store_);
-}
-
-void CompromisedCredentialsObserver::Initialize() {
-  store_->AddObserver(this);
-}
-
-CompromisedCredentialsObserver::~CompromisedCredentialsObserver() {
-  store_->RemoveObserver(this);
-}
-
-void CompromisedCredentialsObserver::OnLoginsChanged(
-    const PasswordStoreChangeList& changes) {
+void ProcessLoginsChanged(const PasswordStoreChangeList& changes,
+                          const RemoveCompromisedCallback& remove_callback) {
   bool password_protection_show_domains_for_saved_password_is_on =
       base::FeatureList::IsEnabled(
           safe_browsing::kPasswordProtectionShowDomainsForSavedPasswords);
@@ -51,8 +37,8 @@ void CompromisedCredentialsObserver::OnLoginsChanged(
         })) {
       reason = RemoveCompromisedCredentialsReason::kRemove;
     }
-    store_->RemoveCompromisedCredentials(change.form().signon_realm,
-                                         change.form().username_value, reason);
+    remove_callback.Run(change.form().signon_realm,
+                        change.form().username_value, reason);
     UMA_HISTOGRAM_ENUMERATION(
         "PasswordManager.RemoveCompromisedCredentials",
         reason == RemoveCompromisedCredentialsReason::kUpdate

components/password_manager/core/browser/compromised_credentials_observer.h

@@ -5,36 +5,27 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_COMPROMISED_CREDENTIALS_OBSERVER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_COMPROMISED_CREDENTIALS_OBSERVER_H_
 
-#include "components/password_manager/core/browser/password_store.h"
+#include "base/callback.h"
+#include "components/password_manager/core/browser/compromised_credentials_table.h"
+#include "components/password_manager/core/browser/password_store_change.h"
 
 namespace password_manager {
 
-// Observes changes in Password Store logins and updates the
-// CompromisedCredentialsTable accordingly.
-class CompromisedCredentialsObserver : public PasswordStore::Observer {
- public:
-  // Fordbid copying and assigning.
-  CompromisedCredentialsObserver(const CompromisedCredentialsObserver&) =
-      delete;
-  CompromisedCredentialsObserver& operator=(
-      const CompromisedCredentialsObserver&) = delete;
-
-  explicit CompromisedCredentialsObserver(PasswordStore* store);
-  ~CompromisedCredentialsObserver() override;
-
-  // Adds this instance as an observer in |store_|.
-  void Initialize();
-
- private:
-  // Called when the contents of the password store change.
-  // Removes rows from the compromised credentials database if the login
-  // was removed or the password was updated.  If row is not in the database,
-  // the call is ignored.
-  void OnLoginsChanged(const PasswordStoreChangeList& changes) override;
-
-  PasswordStore* const store_;
-};
+// The callback to use to synchronously remove a compromised
+// credentials from the password store. The parameters match those in
+// RemoveCompromisedCredentials.
+using RemoveCompromisedCallback =
+    base::RepeatingCallback<void(const std::string&,
+                                 const base::string16&,
+                                 RemoveCompromisedCredentialsReason)>;
+
+// Called when the content of the password store changes.
+// Removes rows from the compromised credentials database if the login
+// was removed or the password was updated. If row is not in the database,
+// the call is ignored.
+void ProcessLoginsChanged(const PasswordStoreChangeList& changes,
+                          const RemoveCompromisedCallback& remove_callback);
 
 }  // namespace password_manager
 
-#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_COMPROMISED_CREDENTIALS_OBSERVER_H_
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_COMPROMISED_CREDENTIALS_OBSERVER_H_
\ No newline at end of file

components/password_manager/core/browser/compromised_credentials_observer_unittest.cc

@@ -4,13 +4,11 @@
 
 #include "components/password_manager/core/browser/compromised_credentials_observer.h"
 
-#include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/mock_callback.h"
 #include "base/test/scoped_feature_list.h"
-#include "base/test/task_environment.h"
-#include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -38,81 +36,72 @@ class CompromisedCredentialsObserverTest : public testing::Test {
  public:
   CompromisedCredentialsObserverTest() {
     feature_list_.InitAndEnableFeature(features::kPasswordCheck);
-    mock_store_->Init(nullptr);
-    observer_.Initialize();
   }
 
-  ~CompromisedCredentialsObserverTest() override {
-    mock_store_->ShutdownOnUIThread();
-  }
+  ~CompromisedCredentialsObserverTest() override = default;
 
-  void WaitForPasswordStore() { task_environment_.RunUntilIdle(); }
-  MockPasswordStore& store() { return *mock_store_; }
   base::HistogramTester& histogram_tester() { return histogram_tester_; }
-  PasswordStore::Observer& observer() { return observer_; }
+  base::MockCallback<RemoveCompromisedCallback>& remove_callback() {
+    return remove_callback_;
+  }
 
  private:
-  base::test::SingleThreadTaskEnvironment task_environment_;
   base::test::ScopedFeatureList feature_list_;
-  scoped_refptr<MockPasswordStore> mock_store_ =
-      base::MakeRefCounted<testing::StrictMock<MockPasswordStore>>();
   base::HistogramTester histogram_tester_;
-  CompromisedCredentialsObserver observer_{mock_store_.get()};
+  base::MockCallback<RemoveCompromisedCallback> remove_callback_;
 };
 
 TEST_F(CompromisedCredentialsObserverTest, DeletePassword) {
   const autofill::PasswordForm form = TestForm(kUsername);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           form.signon_realm, form.username_value,
-                           RemoveCompromisedCredentialsReason::kRemove));
-  observer().OnLoginsChanged(
-      {PasswordStoreChange(PasswordStoreChange::REMOVE, form)});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(),
+              Run(form.signon_realm, form.username_value,
+                  RemoveCompromisedCredentialsReason::kRemove));
+  ProcessLoginsChanged({PasswordStoreChange(PasswordStoreChange::REMOVE, form)},
+                       remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::REMOVE, 1);
 }
 
 TEST_F(CompromisedCredentialsObserverTest, UpdateFormNoPasswordChange) {
   const autofill::PasswordForm form = TestForm(kUsername);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl).Times(0);
-  observer().OnLoginsChanged(
-      {PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1000, false)});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(), Run).Times(0);
+  ProcessLoginsChanged(
+      {PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1000, false)},
+      remove_callback().Get());
   histogram_tester().ExpectTotalCount(kHistogramName, 0);
 }
 
 TEST_F(CompromisedCredentialsObserverTest, UpdatePassword) {
   const autofill::PasswordForm form = TestForm(kUsername);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           form.signon_realm, form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  observer().OnLoginsChanged(
-      {PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1000, true)});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(),
+              Run(form.signon_realm, form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  ProcessLoginsChanged(
+      {PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1000, true)},
+      remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::UPDATE, 1);
 }
 
 TEST_F(CompromisedCredentialsObserverTest, UpdateTwice) {
   const autofill::PasswordForm form = TestForm(kUsername);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           form.signon_realm, form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  observer().OnLoginsChanged(
+  EXPECT_CALL(remove_callback(),
+              Run(form.signon_realm, form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  ProcessLoginsChanged(
       {PasswordStoreChange(PasswordStoreChange::UPDATE, TestForm(kUsernameNew),
                            1000, false),
-       PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1001, true)});
-  WaitForPasswordStore();
+       PasswordStoreChange(PasswordStoreChange::UPDATE, form, 1001, true)},
+      remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::UPDATE, 1);
 }
 
 TEST_F(CompromisedCredentialsObserverTest, AddPassword) {
   const autofill::PasswordForm form = TestForm(kUsername);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl).Times(0);
-  observer().OnLoginsChanged(
-      {PasswordStoreChange(PasswordStoreChange::ADD, form)});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(), Run).Times(0);
+  ProcessLoginsChanged({PasswordStoreChange(PasswordStoreChange::ADD, form)},
+                       remove_callback().Get());
   histogram_tester().ExpectTotalCount(kHistogramName, 0);
 }
 
@@ -121,11 +110,10 @@ TEST_F(CompromisedCredentialsObserverTest, AddReplacePassword) {
   PasswordStoreChange remove(PasswordStoreChange::REMOVE, form);
   form.password_value = base::ASCIIToUTF16("new_password_12345");
   PasswordStoreChange add(PasswordStoreChange::ADD, form);
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           form.signon_realm, form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  observer().OnLoginsChanged({remove, add});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(),
+              Run(form.signon_realm, form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  ProcessLoginsChanged({remove, add}, remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::UPDATE, 1);
 }
@@ -134,11 +122,10 @@ TEST_F(CompromisedCredentialsObserverTest, UpdateWithPrimaryKey) {
   const autofill::PasswordForm old_form = TestForm(kUsername);
   PasswordStoreChange remove(PasswordStoreChange::REMOVE, old_form);
   PasswordStoreChange add(PasswordStoreChange::ADD, TestForm(kUsernameNew));
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           old_form.signon_realm, old_form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  observer().OnLoginsChanged({remove, add});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(),
+              Run(old_form.signon_realm, old_form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  ProcessLoginsChanged({remove, add}, remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::UPDATE, 1);
 }
@@ -150,15 +137,15 @@ TEST_F(CompromisedCredentialsObserverTest, UpdateWithPrimaryKey_RemoveTwice) {
   PasswordStoreChange remove_conflicting(PasswordStoreChange::REMOVE,
                                          conflicting_new_form);
   PasswordStoreChange add(PasswordStoreChange::ADD, TestForm(kUsernameNew));
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           old_form.signon_realm, old_form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  EXPECT_CALL(store(), RemoveCompromisedCredentialsImpl(
-                           conflicting_new_form.signon_realm,
-                           conflicting_new_form.username_value,
-                           RemoveCompromisedCredentialsReason::kUpdate));
-  observer().OnLoginsChanged({remove_old, remove_conflicting, add});
-  WaitForPasswordStore();
+  EXPECT_CALL(remove_callback(),
+              Run(old_form.signon_realm, old_form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  EXPECT_CALL(remove_callback(),
+              Run(conflicting_new_form.signon_realm,
+                  conflicting_new_form.username_value,
+                  RemoveCompromisedCredentialsReason::kUpdate));
+  ProcessLoginsChanged({remove_old, remove_conflicting, add},
+                       remove_callback().Get());
   histogram_tester().ExpectUniqueSample(kHistogramName,
                                         PasswordStoreChange::UPDATE, 2);
 }

components/password_manager/core/browser/password_store.cc

@@ -161,10 +161,6 @@ bool PasswordStore::Init(PrefService* prefs,
         base::BindOnce(&PasswordStore::OnInitCompleted, this));
   }
 
-  compromised_credentials_observer_ =
-      std::make_unique<CompromisedCredentialsObserver>(this);
-  compromised_credentials_observer_->Initialize();
-
   return true;
 }
 
@@ -629,7 +625,6 @@ void PasswordStore::ScheduleEnterprisePasswordURLUpdate() {
 
 PasswordStore::~PasswordStore() {
   DCHECK(shutdown_called_);
-  compromised_credentials_observer_.reset(nullptr);
 }
 
 scoped_refptr<base::SequencedTaskRunner>
@@ -703,6 +698,19 @@ void PasswordStore::NotifyLoginsChanged(
     if (reuse_detector_)
       reuse_detector_->OnLoginsChanged(changes);
 #endif
+    ProcessLoginsChanged(
+        changes,
+        base::BindRepeating(
+            [](scoped_refptr<PasswordStore> store,
+               const std::string& signon_realm, const base::string16& username,
+               RemoveCompromisedCredentialsReason reason) {
+              auto callback = base::BindOnce(
+                  &PasswordStore::RemoveCompromisedCredentialsImpl, store,
+                  signon_realm, username, reason);
+              store->InvokeAndNotifyAboutCompromisedPasswordsChange(
+                  std::move(callback));
+            },
+            scoped_refptr<PasswordStore>(this)));
   }
 }
 

components/password_manager/core/browser/password_store.h

@@ -54,7 +54,6 @@ using metrics_util::GaiaPasswordHashChange;
 #endif
 
 class AffiliatedMatchHelper;
-class CompromisedCredentialsObserver;
 class PasswordStoreConsumer;
 class CompromisedCredentialsConsumer;
 class PasswordStoreSigninNotifier;
@@ -814,9 +813,6 @@ class PasswordStore : protected PasswordStoreSync,
 
   std::unique_ptr<AffiliatedMatchHelper> affiliated_match_helper_;
 
-  std::unique_ptr<CompromisedCredentialsObserver>
-      compromised_credentials_observer_;
-
   PrefService* prefs_ = nullptr;
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
   // PasswordReuseDetector can be only destroyed on the background sequence. It

components/password_manager/core/browser/password_store_unittest.cc

@@ -1537,6 +1537,83 @@ TEST_F(PasswordStoreTest, RemoveCompromisedCredentialsCreatedBetween) {
   store->ShutdownOnUIThread();
 }
 
+// Test that updating a password in the store deletes the corresponding
+// compromised record synchronously.
+TEST_F(PasswordStoreTest, RemoveCompromisedCredentialsSyncOnUpdate) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(password_manager::features::kPasswordCheck);
+
+  scoped_refptr<PasswordStoreDefault> store = CreatePasswordStore();
+  store->Init(nullptr);
+
+  CompromisedCredentials compromised_credentials = {
+      kTestWebRealm1, base::ASCIIToUTF16("username1"),
+      base::Time::FromTimeT(100), CompromiseType::kLeaked};
+  constexpr PasswordFormData kTestCredential = {PasswordForm::Scheme::kHtml,
+                                                kTestWebRealm1,
+                                                kTestWebOrigin1,
+                                                "",
+                                                L"",
+                                                L"username_element_1",
+                                                L"password_element_1",
+                                                L"username1",
+                                                L"12345",
+                                                10,
+                                                5};
+  std::unique_ptr<PasswordForm> form(FillPasswordFormWithData(kTestCredential));
+  store->AddCompromisedCredentials(compromised_credentials);
+  store->AddLogin(*form);
+  WaitForPasswordStore();
+
+  // Update the password value and immediately get the compromised passwords.
+  form->password_value = base::ASCIIToUTF16("new_password");
+  store->UpdateLogin(*form);
+  MockCompromisedCredentialsConsumer consumer;
+  store->GetAllCompromisedCredentials(&consumer);
+  EXPECT_CALL(consumer, OnGetCompromisedCredentials(IsEmpty()));
+  WaitForPasswordStore();
+
+  store->ShutdownOnUIThread();
+}
+
+// Test that deleting a password in the store deletes the corresponding
+// compromised record synchronously.
+TEST_F(PasswordStoreTest, RemoveCompromisedCredentialsSyncOnDelete) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(password_manager::features::kPasswordCheck);
+
+  scoped_refptr<PasswordStoreDefault> store = CreatePasswordStore();
+  store->Init(nullptr);
+
+  CompromisedCredentials compromised_credentials = {
+      kTestWebRealm1, base::ASCIIToUTF16("username1"),
+      base::Time::FromTimeT(100), CompromiseType::kLeaked};
+  constexpr PasswordFormData kTestCredential = {PasswordForm::Scheme::kHtml,
+                                                kTestWebRealm1,
+                                                kTestWebOrigin1,
+                                                "",
+                                                L"",
+                                                L"username_element_1",
+                                                L"password_element_1",
+                                                L"username1",
+                                                L"12345",
+                                                10,
+                                                5};
+  std::unique_ptr<PasswordForm> form(FillPasswordFormWithData(kTestCredential));
+  store->AddCompromisedCredentials(compromised_credentials);
+  store->AddLogin(*form);
+  WaitForPasswordStore();
+
+  // Delete the password and immediately get the compromised passwords.
+  store->RemoveLogin(*form);
+  MockCompromisedCredentialsConsumer consumer;
+  store->GetAllCompromisedCredentials(&consumer);
+  EXPECT_CALL(consumer, OnGetCompromisedCredentials(IsEmpty()));
+  WaitForPasswordStore();
+
+  store->ShutdownOnUIThread();
+}
+
 #if !defined(OS_ANDROID)
 // TODO(https://crbug.com/1051914): Enable on Android after making local
 // heuristics reliable.