[Passwords] Respond to passwords changes in CompromisedCredentialsProvider

This change modifies CompromisedCredentialsProvider to also implement
the SavedPasswordsProvider::Observer interface. This allows the provider
to also react to changes in the list of saved passwords.

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

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

@@ -44,6 +44,40 @@ struct CredentialLess {
   using is_transparent = void;
 };
 
+// This function takes two lists of compromised credentials and saved passwords
+// and joins them, producing a new list that contains an entry for each element
+// of |saved_passwords| whose signon_realm and username are also present in
+// |compromised_credentials|.
+std::vector<CredentialWithPassword>
+JoinCompromisedCredentialsWithSavedPasswords(
+    base::span<const CompromisedCredentials> compromised_credentials,
+    SavedPasswordsPresenter::SavedPasswordsView saved_passwords) {
+  std::vector<CredentialWithPassword> compromised_credentials_with_passwords;
+  compromised_credentials_with_passwords.reserve(
+      compromised_credentials.size());
+
+  // Since a single (signon_realm, username) pair might have multiple
+  // corresponding entries in saved_passwords, we are using a multiset and doing
+  // look-up via equal_range. In most cases the resulting |range| should have a
+  // size of 1, however.
+  std::multiset<CredentialView, CredentialLess> credentials(
+      saved_passwords.begin(), saved_passwords.end());
+  for (const auto& compromised_credential : compromised_credentials) {
+    auto range = credentials.equal_range(compromised_credential);
+    // Make use of a set to only filter out repeated passwords, if any.
+    base::flat_set<base::string16> passwords;
+    std::for_each(range.first, range.second, [&](const CredentialView& view) {
+      if (passwords.insert(view.password).second) {
+        compromised_credentials_with_passwords.emplace_back(
+            compromised_credential);
+        compromised_credentials_with_passwords.back().password = view.password;
+      }
+    });
+  }
+
+  return compromised_credentials_with_passwords;
+}
+
 }  // namespace
 
 bool operator==(const CredentialWithPassword& lhs,
@@ -67,11 +101,12 @@ CompromisedCredentialsProvider::CompromisedCredentialsProvider(
     scoped_refptr<PasswordStore> store,
     SavedPasswordsPresenter* presenter)
     : store_(std::move(store)), presenter_(presenter) {
-  DCHECK(store_);
   store_->AddDatabaseCompromisedCredentialsObserver(this);
+  presenter_->AddObserver(this);
 }
 
 CompromisedCredentialsProvider::~CompromisedCredentialsProvider() {
+  presenter_->RemoveObserver(this);
   store_->RemoveDatabaseCompromisedCredentialsObserver(this);
 }
 
@@ -81,7 +116,7 @@ void CompromisedCredentialsProvider::Init() {
 
 CompromisedCredentialsProvider::CredentialsView
 CompromisedCredentialsProvider::GetCompromisedCredentials() const {
-  return compromised_credentials_;
+  return compromised_credentials_with_passwords_;
 }
 
 void CompromisedCredentialsProvider::AddObserver(Observer* observer) {
@@ -98,41 +133,32 @@ void CompromisedCredentialsProvider::OnCompromisedCredentialsChanged() {
   store_->GetAllCompromisedCredentials(this);
 }
 
-// This function takes two lists of compromised credentials and saved passwords
-// and joins them, producing a new list that contains an entry for each element
-// of |saved_passwords| whose signon_realm and username are also present in
-// |compromised_credentials|.
+// Re-computes the list of compromised credentials with passwords after
+// obtaining a new list of compromised credentials.
 void CompromisedCredentialsProvider::OnGetCompromisedCredentials(
     std::vector<CompromisedCredentials> compromised_credentials) {
-  compromised_credentials_.clear();
-  compromised_credentials_.reserve(compromised_credentials.size());
-
-  SavedPasswordsPresenter::SavedPasswordsView saved_passwords =
-      presenter_->GetSavedPasswords();
-  // Since a single (signon_realm, username) pair might have multiple
-  // corresponding entries in saved_passwords, we are using a multiset and doing
-  // look-up via equal_range. In most cases the resulting |range| should have a
-  // size of 1, however.
-  std::multiset<CredentialView, CredentialLess> credentials(
-      saved_passwords.begin(), saved_passwords.end());
-  for (const auto& compromised_credential : compromised_credentials) {
-    auto range = credentials.equal_range(compromised_credential);
-    // Make use of a set to only filter out repeated passwords, if any.
-    base::flat_set<base::string16> passwords;
-    std::for_each(range.first, range.second, [&](const CredentialView& view) {
-      if (passwords.insert(view.password).second) {
-        compromised_credentials_.emplace_back(compromised_credential);
-        compromised_credentials_.back().password = view.password;
-      }
-    });
-  }
+  compromised_credentials_ = std::move(compromised_credentials);
+  compromised_credentials_with_passwords_ =
+      JoinCompromisedCredentialsWithSavedPasswords(
+          compromised_credentials_, presenter_->GetSavedPasswords());
+  NotifyCompromisedCredentialsChanged();
+}
 
+// Re-computes the list of compromised credentials with passwords after
+// obtaining a new list of saved passwords.
+void CompromisedCredentialsProvider::OnSavedPasswordsChanged(
+    SavedPasswordsPresenter::SavedPasswordsView saved_passwords) {
+  compromised_credentials_with_passwords_ =
+      JoinCompromisedCredentialsWithSavedPasswords(compromised_credentials_,
+                                                   saved_passwords);
   NotifyCompromisedCredentialsChanged();
 }
 
 void CompromisedCredentialsProvider::NotifyCompromisedCredentialsChanged() {
-  for (auto& observer : observers_)
-    observer.OnCompromisedCredentialsChanged(compromised_credentials_);
+  for (auto& observer : observers_) {
+    observer.OnCompromisedCredentialsChanged(
+        compromised_credentials_with_passwords_);
+  }
 }
 
 }  // namespace password_manager

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

@@ -13,12 +13,11 @@
 #include "components/password_manager/core/browser/compromised_credentials_consumer.h"
 #include "components/password_manager/core/browser/compromised_credentials_table.h"
 #include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 #include "url/gurl.h"
 
 namespace password_manager {
 
-class SavedPasswordsPresenter;
-
 // Simple struct that augments the CompromisedCredentials with a password.
 struct CredentialWithPassword : CompromisedCredentials {
   // Enable explicit construction from the parent struct. This will leave
@@ -40,7 +39,8 @@ std::ostream& operator<<(std::ostream& out,
 // notified about changes to the list.
 class CompromisedCredentialsProvider
     : public PasswordStore::DatabaseCompromisedCredentialsObserver,
-      public CompromisedCredentialsConsumer {
+      public CompromisedCredentialsConsumer,
+      public SavedPasswordsPresenter::Observer {
  public:
 
   using CredentialsView = base::span<const CredentialWithPassword>;
@@ -76,7 +76,12 @@ class CompromisedCredentialsProvider
   void OnGetCompromisedCredentials(
       std::vector<CompromisedCredentials> compromised_credentials) override;
 
-  // Notify observers about changes to |compromised_credentials_|.
+  // SavedPasswordsPresenter::Observer:
+  void OnSavedPasswordsChanged(
+      SavedPasswordsPresenter::SavedPasswordsView passwords) override;
+
+  // Notify observers about changes to
+  // |compromised_credentials_with_passwords_|.
   void NotifyCompromisedCredentialsChanged();
 
   // The password store containing the compromised credentials.
@@ -87,7 +92,10 @@ class CompromisedCredentialsProvider
   SavedPasswordsPresenter* presenter_ = nullptr;
 
   // Cache of the most recently obtained compromised credentials.
-  std::vector<CredentialWithPassword> compromised_credentials_;
+  std::vector<CompromisedCredentials> compromised_credentials_;
+
+  // Cache of the most recently obtained compromised credentials with passwords.
+  std::vector<CredentialWithPassword> compromised_credentials_with_passwords_;
 
   base::ObserverList<Observer, /*check_empty=*/true> observers_;
 };

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

@@ -93,14 +93,15 @@ class CompromisedCredentialsProviderTest : public ::testing::Test {
 }  // namespace
 
 // Tests whether adding and removing an observer works as expected.
-TEST_F(CompromisedCredentialsProviderTest, NotifyObservers) {
+TEST_F(CompromisedCredentialsProviderTest,
+       NotifyObserversAboutCompromisedCredentialChanges) {
   std::vector<CompromisedCredentials> credentials = {
       MakeCompromised(kExampleCom, kUsername1)};
 
   StrictMockCompromisedCredentialsProviderObserver observer;
   provider().AddObserver(&observer);
 
-  // Adding a credential should notify observers.
+  // Adding a compromised credential should notify observers.
   EXPECT_CALL(observer, OnCompromisedCredentialsChanged);
   store().AddCompromisedCredentials(credentials[0]);
   RunUntilIdle();
@@ -135,6 +136,38 @@ TEST_F(CompromisedCredentialsProviderTest, NotifyObservers) {
   EXPECT_THAT(store().compromised_credentials(), ElementsAreArray(credentials));
 }
 
+// Tests whether adding and removing an observer works as expected.
+TEST_F(CompromisedCredentialsProviderTest,
+       NotifyObserversAboutSavedPasswordsChanges) {
+  StrictMockCompromisedCredentialsProviderObserver observer;
+  provider().AddObserver(&observer);
+
+  PasswordForm saved_password =
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1);
+
+  // Adding a saved password should notify observers.
+  EXPECT_CALL(observer, OnCompromisedCredentialsChanged);
+  store().AddLogin(saved_password);
+  RunUntilIdle();
+
+  // Updating a saved password should notify observers.
+  saved_password.password_value = base::ASCIIToUTF16(kPassword2);
+  EXPECT_CALL(observer, OnCompromisedCredentialsChanged);
+  store().UpdateLogin(saved_password);
+  RunUntilIdle();
+
+  // Removing a saved password should notify observers.
+  EXPECT_CALL(observer, OnCompromisedCredentialsChanged);
+  store().RemoveLogin(saved_password);
+  RunUntilIdle();
+
+  // After an observer is removed it should no longer receive notifications.
+  provider().RemoveObserver(&observer);
+  EXPECT_CALL(observer, OnCompromisedCredentialsChanged).Times(0);
+  store().AddLogin(saved_password);
+  RunUntilIdle();
+}
+
 // Tests that the provider is able to join a single password with a compromised
 // credential.
 TEST_F(CompromisedCredentialsProviderTest, JoinSingleCredentials) {
@@ -174,6 +207,48 @@ TEST_F(CompromisedCredentialsProviderTest, JoinPhishedAndLeaked) {
               ElementsAre(expected1, expected2));
 }
 
+// Tests that the provider reacts whenever the saved passwords or the
+// compromised credentials change.
+TEST_F(CompromisedCredentialsProviderTest, ReactToChangesInBothTables) {
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1),
+      MakeSavedPassword(kExampleCom, kUsername2, kPassword2)};
+
+  std::vector<CompromisedCredentials> credentials = {
+      MakeCompromised(kExampleCom, kUsername1),
+      MakeCompromised(kExampleCom, kUsername2)};
+
+  std::vector<CredentialWithPassword> expected(credentials.begin(),
+                                               credentials.end());
+  expected[0].password = passwords[0].password_value;
+  expected[1].password = passwords[1].password_value;
+
+  store().AddLogin(passwords[0]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(), IsEmpty());
+
+  store().AddCompromisedCredentials(credentials[0]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(), ElementsAre(expected[0]));
+
+  store().AddLogin(passwords[1]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(), ElementsAre(expected[0]));
+
+  store().AddCompromisedCredentials(credentials[1]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(),
+              ElementsAreArray(expected));
+
+  store().RemoveLogin(passwords[0]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(), ElementsAre(expected[1]));
+
+  store().RemoveLogin(passwords[1]);
+  RunUntilIdle();
+  EXPECT_THAT(provider().GetCompromisedCredentials(), IsEmpty());
+}
+
 // Tests that the provider is able to join multiple passwords with compromised
 // credentials.
 TEST_F(CompromisedCredentialsProviderTest, JoinMultipleCredentials) {