[Passwords] Join Saved Passwords with Compromised Credentials

This change modifies CompromisedCredentialProvider to make use of the
SavedPasswordsPresenter to be able to merge compromised credentials
with passwords present in the password store.

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

components/password_manager/core/browser/compromised_credentials_table.h

@@ -40,7 +40,7 @@ struct CompromisedCredentials {
   // The date when the record was created.
   base::Time create_time;
   // The type of the credentials that was compromised.
-  CompromiseType compromise_type;
+  CompromiseType compromise_type = CompromiseType::kLeaked;
 };
 
 bool operator==(const CompromisedCredentials& lhs,

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

@@ -3,15 +3,70 @@
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/ui/compromised_credentials_provider.h"
+
+#include <algorithm>
 #include <iterator>
+#include <set>
 
+#include "base/containers/flat_set.h"
 #include "base/logging.h"
+#include "base/strings/string16.h"
+#include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/compromised_credentials_table.h"
+#include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 
 namespace password_manager {
 
+namespace {
+
+// A view over a PasswordForm that only stores the signon_realm, username and
+// password. An implicit constructor is provided for convenience.
+struct CredentialView {
+  CredentialView(const autofill::PasswordForm& form)
+      : signon_realm(form.signon_realm),
+        username(form.username_value),
+        password(form.password_value) {}
+
+  std::string signon_realm;
+  base::string16 username;
+  base::string16 password;
+};
+
+// Transparent comparator that can compare various types like CredentialView or
+// CompromisedCredentials.
+struct CredentialLess {
+  template <typename T, typename U>
+  bool operator()(const T& lhs, const U& rhs) const {
+    return std::tie(lhs.signon_realm, lhs.username) <
+           std::tie(rhs.signon_realm, rhs.username);
+  }
+
+  using is_transparent = void;
+};
+
+}  // namespace
+
+bool operator==(const CredentialWithPassword& lhs,
+                const CredentialWithPassword& rhs) {
+  // Upcast `lhs` to trigger the CompromisedCredentials operator==.
+  return static_cast<const CompromisedCredentials&>(lhs) == rhs &&
+         lhs.password == rhs.password;
+}
+
+std::ostream& operator<<(std::ostream& out,
+                         const CredentialWithPassword& credential) {
+  return out << "{ signon_realm: " << credential.signon_realm
+             << ", username: " << credential.username
+             << ", create_time: " << credential.create_time
+             << ", compromise_type: "
+             << static_cast<int>(credential.compromise_type)
+             << ", password: " << credential.password << " }";
+}
+
 CompromisedCredentialsProvider::CompromisedCredentialsProvider(
-    scoped_refptr<PasswordStore> store)
-    : store_(std::move(store)) {
+    scoped_refptr<PasswordStore> store,
+    SavedPasswordsPresenter* presenter)
+    : store_(std::move(store)), presenter_(presenter) {
   DCHECK(store_);
   store_->AddDatabaseCompromisedCredentialsObserver(this);
 }
@@ -43,13 +98,35 @@ 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|.
 void CompromisedCredentialsProvider::OnGetCompromisedCredentials(
     std::vector<CompromisedCredentials> compromised_credentials) {
-  // TODO(crbug.com/1047726): Implement setting the password for each
-  // credential.
-  compromised_credentials_.assign(
-      std::make_move_iterator(compromised_credentials.begin()),
-      std::make_move_iterator(compromised_credentials.end()));
+  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;
+      }
+    });
+  }
+
   NotifyCompromisedCredentialsChanged();
 }
 

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

@@ -17,6 +17,24 @@
 
 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
+  // |password| empty.
+  explicit CredentialWithPassword(CompromisedCredentials credential)
+      : CompromisedCredentials(std::move(credential)) {}
+
+  base::string16 password;
+};
+
+bool operator==(const CredentialWithPassword& lhs,
+                const CredentialWithPassword& rhs);
+
+std::ostream& operator<<(std::ostream& out,
+                         const CredentialWithPassword& credential);
+
 // This class provides a read-only view over saved compromised credentials. It
 // supports an observer interface, and clients can register themselves to get
 // notified about changes to the list.
@@ -24,21 +42,6 @@ class CompromisedCredentialsProvider
     : public PasswordStore::DatabaseCompromisedCredentialsObserver,
       public CompromisedCredentialsConsumer {
  public:
-  // Simple struct that augments the CompromisedCredentials with a password.
-  struct CredentialWithPassword : CompromisedCredentials {
-    // Enable explicit construction and assignment from the parent struct. These
-    // will leave |password| empty.
-    explicit CredentialWithPassword(CompromisedCredentials credential)
-        : CompromisedCredentials(std::move(credential)) {}
-
-    CredentialWithPassword& operator=(CompromisedCredentials credential) {
-      CompromisedCredentials::operator=(std::move(credential));
-      password.clear();
-      return *this;
-    }
-
-    base::string16 password;
-  };
 
   using CredentialsView = base::span<const CredentialWithPassword>;
 
@@ -52,7 +55,8 @@ class CompromisedCredentialsProvider
         CredentialsView credentials) = 0;
   };
 
-  explicit CompromisedCredentialsProvider(scoped_refptr<PasswordStore> store);
+  explicit CompromisedCredentialsProvider(scoped_refptr<PasswordStore> store,
+                                          SavedPasswordsPresenter* presenter);
   ~CompromisedCredentialsProvider() override;
 
   void Init();
@@ -78,6 +82,10 @@ class CompromisedCredentialsProvider
   // The password store containing the compromised credentials.
   scoped_refptr<PasswordStore> store_;
 
+  // A weak handle to the presenter used to join the list of compromised
+  // credentials with saved passwords. Needs to outlive this instance.
+  SavedPasswordsPresenter* presenter_ = nullptr;
+
   // Cache of the most recently obtained compromised credentials.
   std::vector<CredentialWithPassword> compromised_credentials_;
 

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

@@ -5,9 +5,13 @@
 #include "components/password_manager/core/browser/ui/compromised_credentials_provider.h"
 
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/string_piece_forward.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/task_environment.h"
+#include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/compromised_credentials_table.h"
 #include "components/password_manager/core/browser/test_password_store.h"
+#include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 #include "components/sync/model/syncable_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -16,6 +20,17 @@ namespace password_manager {
 
 namespace {
 
+constexpr char kExampleCom[] = "https://example.com";
+constexpr char kExampleOrg[] = "https://example.org";
+
+constexpr char kUsername1[] = "alice";
+constexpr char kUsername2[] = "bob";
+
+constexpr char kPassword1[] = "f00b4r";
+constexpr char kPassword2[] = "s3cr3t";
+
+using autofill::PasswordForm;
+using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
 
@@ -30,6 +45,27 @@ struct MockCompromisedCredentialsProviderObserver
 using StrictMockCompromisedCredentialsProviderObserver =
     ::testing::StrictMock<MockCompromisedCredentialsProviderObserver>;
 
+CompromisedCredentials MakeCompromised(
+    base::StringPiece signon_realm,
+    base::StringPiece username,
+    CompromiseType type = CompromiseType::kLeaked) {
+  return {.signon_realm = std::string(signon_realm),
+          .username = base::ASCIIToUTF16(username),
+          .compromise_type = type};
+}
+
+PasswordForm MakeSavedPassword(base::StringPiece signon_realm,
+                               base::StringPiece username,
+                               base::StringPiece password,
+                               base::StringPiece username_element = "") {
+  PasswordForm form;
+  form.signon_realm = std::string(signon_realm);
+  form.username_value = base::ASCIIToUTF16(username);
+  form.password_value = base::ASCIIToUTF16(password);
+  form.username_element = base::ASCIIToUTF16(username_element);
+  return form;
+}
+
 class CompromisedCredentialsProviderTest : public ::testing::Test {
  protected:
   CompromisedCredentialsProviderTest() {
@@ -50,22 +86,22 @@ class CompromisedCredentialsProviderTest : public ::testing::Test {
   base::test::SingleThreadTaskEnvironment task_env_;
   scoped_refptr<TestPasswordStore> store_ =
       base::MakeRefCounted<TestPasswordStore>();
-  CompromisedCredentialsProvider provider_{store_};
+  SavedPasswordsPresenter presenter_{store_};
+  CompromisedCredentialsProvider provider_{store_, &presenter_};
 };
 
 }  // namespace
 
 // Tests whether adding and removing an observer works as expected.
 TEST_F(CompromisedCredentialsProviderTest, NotifyObservers) {
-  std::vector<CompromisedCredentials> credentials = {CompromisedCredentials()};
+  std::vector<CompromisedCredentials> credentials = {
+      MakeCompromised(kExampleCom, kUsername1)};
 
   StrictMockCompromisedCredentialsProviderObserver observer;
   provider().AddObserver(&observer);
 
-  // Adding a credential should notify observers. Furthermore, the credential
-  // should be present of the list that is passed along.
-  EXPECT_CALL(observer,
-              OnCompromisedCredentialsChanged(ElementsAreArray(credentials)));
+  // Adding a credential should notify observers.
+  EXPECT_CALL(observer, OnCompromisedCredentialsChanged);
   store().AddCompromisedCredentials(credentials[0]);
   RunUntilIdle();
   EXPECT_THAT(store().compromised_credentials(), ElementsAreArray(credentials));
@@ -99,4 +135,149 @@ TEST_F(CompromisedCredentialsProviderTest, NotifyObservers) {
   EXPECT_THAT(store().compromised_credentials(), ElementsAreArray(credentials));
 }
 
+// Tests that the provider is able to join a single password with a compromised
+// credential.
+TEST_F(CompromisedCredentialsProviderTest, JoinSingleCredentials) {
+  PasswordForm password =
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1);
+  CompromisedCredentials credential = MakeCompromised(kExampleCom, kUsername1);
+
+  store().AddLogin(password);
+  store().AddCompromisedCredentials(credential);
+  RunUntilIdle();
+
+  CredentialWithPassword expected(credential);
+  expected.password = password.password_value;
+  EXPECT_THAT(provider().GetCompromisedCredentials(), ElementsAre(expected));
+}
+
+// Tests that the provider is able to join a password with a credential that was
+// compromised in multiple ways.
+TEST_F(CompromisedCredentialsProviderTest, JoinPhishedAndLeaked) {
+  PasswordForm password =
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1);
+  CompromisedCredentials leaked =
+      MakeCompromised(kExampleCom, kUsername1, CompromiseType::kLeaked);
+  CompromisedCredentials phished =
+      MakeCompromised(kExampleCom, kUsername1, CompromiseType::kPhished);
+
+  store().AddLogin(password);
+  store().AddCompromisedCredentials(leaked);
+  store().AddCompromisedCredentials(phished);
+  RunUntilIdle();
+
+  CredentialWithPassword expected1(leaked);
+  expected1.password = password.password_value;
+  CredentialWithPassword expected2(phished);
+  expected2.password = password.password_value;
+  EXPECT_THAT(provider().GetCompromisedCredentials(),
+              ElementsAre(expected1, expected2));
+}
+
+// Tests that the provider is able to join multiple passwords with compromised
+// credentials.
+TEST_F(CompromisedCredentialsProviderTest, JoinMultipleCredentials) {
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1),
+      MakeSavedPassword(kExampleCom, kUsername2, kPassword2)};
+
+  std::vector<CompromisedCredentials> credentials = {
+      MakeCompromised(kExampleCom, kUsername1),
+      MakeCompromised(kExampleCom, kUsername2)};
+
+  store().AddLogin(passwords[0]);
+  store().AddLogin(passwords[1]);
+  store().AddCompromisedCredentials(credentials[0]);
+  store().AddCompromisedCredentials(credentials[1]);
+  RunUntilIdle();
+
+  CredentialWithPassword expected1(credentials[0]);
+  expected1.password = passwords[0].password_value;
+
+  CredentialWithPassword expected2(credentials[1]);
+  expected2.password = passwords[1].password_value;
+
+  EXPECT_THAT(provider().GetCompromisedCredentials(),
+              ElementsAre(expected1, expected2));
+}
+
+// Tests that joining a compromised credential with saved passwords with a
+// different username results in an empty list.
+TEST_F(CompromisedCredentialsProviderTest, JoinWitDifferentUsername) {
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername2, kPassword1),
+      MakeSavedPassword(kExampleCom, kUsername2, kPassword2)};
+
+  CompromisedCredentials credential = MakeCompromised(kExampleCom, kUsername1);
+
+  store().AddLogin(passwords[0]);
+  store().AddLogin(passwords[1]);
+  store().AddCompromisedCredentials(credential);
+  RunUntilIdle();
+
+  EXPECT_THAT(provider().GetCompromisedCredentials(), IsEmpty());
+}
+
+// Tests that joining a compromised credential with saved passwords with a
+// matching username but different signon_realm results in an empty list.
+TEST_F(CompromisedCredentialsProviderTest, JoinWitDifferentSignonRealm) {
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleOrg, kUsername1, kPassword1),
+      MakeSavedPassword(kExampleOrg, kUsername1, kPassword2)};
+
+  CompromisedCredentials credential = MakeCompromised(kExampleCom, kUsername1);
+
+  store().AddLogin(passwords[0]);
+  store().AddLogin(passwords[1]);
+  store().AddCompromisedCredentials(credential);
+  RunUntilIdle();
+
+  EXPECT_THAT(provider().GetCompromisedCredentials(), IsEmpty());
+}
+
+// Tests that joining a compromised credential with multiple saved passwords for
+// the same signon_realm and username combination results in multiple entries
+// when the passwords are distinct.
+TEST_F(CompromisedCredentialsProviderTest, JoinWithMultipleDistinctPasswords) {
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1, "element_1"),
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword2, "element_2")};
+
+  CompromisedCredentials credential = MakeCompromised(kExampleCom, kUsername1);
+
+  store().AddLogin(passwords[0]);
+  store().AddLogin(passwords[1]);
+  store().AddCompromisedCredentials(credential);
+  RunUntilIdle();
+
+  CredentialWithPassword expected1(credential);
+  expected1.password = passwords[0].password_value;
+
+  CredentialWithPassword expected2(credential);
+  expected2.password = passwords[1].password_value;
+
+  EXPECT_THAT(provider().GetCompromisedCredentials(),
+              ElementsAre(expected1, expected2));
+}
+
+// Tests that joining a compromised credential with multiple saved passwords for
+// the same signon_realm and username combination results in a single entry
+// when the passwords are the same.
+TEST_F(CompromisedCredentialsProviderTest, JoinWithMultipleRepeatedPasswords) {
+  CompromisedCredentials credential = MakeCompromised(kExampleCom, kUsername1);
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1, "element_1"),
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1, "element_2")};
+
+  store().AddLogin(passwords[0]);
+  store().AddLogin(passwords[1]);
+  store().AddCompromisedCredentials(credential);
+  RunUntilIdle();
+
+  CredentialWithPassword expected(credential);
+  expected.password = passwords[0].password_value;
+
+  EXPECT_THAT(provider().GetCompromisedCredentials(), ElementsAre(expected));
+}
+
 }  // namespace password_manager