[Passwords] Check the account store in PasswordReuseDetectionManager

This CL queries both the profile and account store for password use,
and makes sure the PasswordProtectionService is invoked only after both
stores respond.

Bug: 1119286
Change-Id: Iedab99826f7822c14da6cbf7c876bd9de8609738
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410486
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807553}

components/password_manager/core/browser/password_reuse_detection_manager.cc

@@ -84,10 +84,7 @@ void PasswordReuseDetectionManager::OnKeyPressed(const base::string16& text,
   const base::string16 text_to_check =
       is_committed ? input_characters_ : input_characters_ + text;
 
-  PasswordStore* store = client_->GetProfilePasswordStore();
-  if (!store)
-    return;
-  store->CheckReuse(text_to_check, main_frame_url_.GetOrigin().spec(), this);
+  CheckStoresForReuse(text_to_check);
 }
 
 void PasswordReuseDetectionManager::OnPaste(const base::string16 text) {
@@ -97,10 +94,8 @@ void PasswordReuseDetectionManager::OnPaste(const base::string16 text) {
   base::string16 input = std::move(text);
   if (input.size() > kMaxNumberOfCharactersToStore)
     input = input.substr(input.size() - kMaxNumberOfCharactersToStore);
-  PasswordStore* store = client_->GetProfilePasswordStore();
-  if (!store)
-    return;
-  store->CheckReuse(input, main_frame_url_.GetOrigin().spec(), this);
+
+  CheckStoresForReuse(input);
 }
 
 void PasswordReuseDetectionManager::OnReuseCheckDone(
@@ -109,18 +104,30 @@ void PasswordReuseDetectionManager::OnReuseCheckDone(
     base::Optional<PasswordHashData> reused_protected_password_hash,
     const std::vector<MatchingReusedCredential>& matching_reused_credentials,
     int saved_passwords) {
-  if (!is_reuse_found)
+  // Cache the results.
+  all_matching_reused_credentials_.insert(matching_reused_credentials.begin(),
+                                          matching_reused_credentials.end());
+  reuse_on_this_page_was_found_ |= is_reuse_found;
+
+  // If we're still waiting for more results, nothing to be done yet.
+  if (--wait_counter_ > 0)
     return;
-  reuse_on_this_page_was_found_ = true;
+
+  // If no reuse was found, we're done.
+  if (!reuse_on_this_page_was_found_) {
+    all_matching_reused_credentials_.clear();
+    return;
+  }
+
   metrics_util::PasswordType reused_password_type = GetReusedPasswordType(
-      reused_protected_password_hash, matching_reused_credentials.size());
+      reused_protected_password_hash, all_matching_reused_credentials_.size());
 
   if (password_manager_util::IsLoggingActive(client_)) {
     BrowserSavePasswordProgressLogger logger(client_->GetLogManager());
     std::vector<std::string> domains_to_log;
-    domains_to_log.reserve(matching_reused_credentials.size());
+    domains_to_log.reserve(all_matching_reused_credentials_.size());
     for (const MatchingReusedCredential& credential :
-         matching_reused_credentials) {
+         all_matching_reused_credentials_) {
       domains_to_log.push_back(credential.signon_realm);
     }
     switch (reused_password_type) {
@@ -153,7 +160,7 @@ void PasswordReuseDetectionManager::OnReuseCheckDone(
           : false;
 
   metrics_util::LogPasswordReuse(password_length, saved_passwords,
-                                 matching_reused_credentials.size(),
+                                 all_matching_reused_credentials_.size(),
                                  password_field_detected, reused_password_type);
 #if defined(PASSWORD_REUSE_WARNING_ENABLED)
   if (reused_password_type ==
@@ -166,10 +173,12 @@ void PasswordReuseDetectionManager::OnReuseCheckDone(
                              ? reused_protected_password_hash->username
                              : "";
 
-  client_->CheckProtectedPasswordEntry(reused_password_type, username,
-                                       matching_reused_credentials,
-                                       password_field_detected);
+  client_->CheckProtectedPasswordEntry(
+      reused_password_type, username,
+      std::move(all_matching_reused_credentials_).extract(),
+      password_field_detected);
 #endif
+  all_matching_reused_credentials_.clear();
 }
 
 void PasswordReuseDetectionManager::SetClockForTesting(base::Clock* clock) {
@@ -194,4 +203,19 @@ metrics_util::PasswordType PasswordReuseDetectionManager::GetReusedPasswordType(
   }
 }
 
+void PasswordReuseDetectionManager::CheckStoresForReuse(
+    const base::string16& input) {
+  PasswordStore* profile_store = client_->GetProfilePasswordStore();
+  if (profile_store) {
+    ++wait_counter_;
+    profile_store->CheckReuse(input, main_frame_url_.GetOrigin().spec(), this);
+  }
+
+  PasswordStore* account_store = client_->GetAccountPasswordStore();
+  if (account_store) {
+    ++wait_counter_;
+    account_store->CheckReuse(input, main_frame_url_.GetOrigin().spec(), this);
+  }
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/password_reuse_detection_manager.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_REUSE_DETECTION_MANAGER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_REUSE_DETECTION_MANAGER_H_
 
+#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
@@ -52,6 +53,9 @@ class PasswordReuseDetectionManager : public PasswordReuseDetectorConsumer {
   metrics_util::PasswordType GetReusedPasswordType(
       base::Optional<PasswordHashData> reused_protected_password_hash,
       size_t match_domain_count);
+
+  void CheckStoresForReuse(const base::string16& input);
+
   PasswordManagerClient* client_;
   base::string16 input_characters_;
   GURL main_frame_url_;
@@ -60,6 +64,11 @@ class PasswordReuseDetectionManager : public PasswordReuseDetectorConsumer {
   base::Clock* clock_;
   bool reuse_on_this_page_was_found_ = false;
 
+  // Caches the results returned from each store until all stores
+  // respond. All credentials are then forwarded to the `client_`.
+  base::flat_set<MatchingReusedCredential> all_matching_reused_credentials_;
+  int wait_counter_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(PasswordReuseDetectionManager);
 };
 

components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc

@@ -33,6 +33,13 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
   ~MockPasswordManagerClient() override = default;
 
   MOCK_CONST_METHOD0(GetProfilePasswordStore, PasswordStore*());
+  MOCK_CONST_METHOD0(GetAccountPasswordStore, PasswordStore*());
+
+  MOCK_METHOD4(CheckProtectedPasswordEntry,
+               void(metrics_util::PasswordType,
+                    const std::string&,
+                    const std::vector<MatchingReusedCredential>&,
+                    bool));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockPasswordManagerClient);
@@ -199,6 +206,42 @@ TEST_F(PasswordReuseDetectionManagerTest, CheckReuseCalledOnPaste) {
   }
 }
 
+TEST_F(PasswordReuseDetectionManagerTest,
+       CheckReuseCalledOnPasteTwiceProduceNoDuplicates) {
+  const GURL kURL("https://www.example.com");
+  const base::string16 kInput =
+      base::ASCIIToUTF16("1234567890abcdefghijklmnopqrstuvxyz");
+
+  EXPECT_CALL(client_, GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store_.get()));
+  PasswordReuseDetectionManager manager(&client_);
+
+  manager.DidNavigateMainFrame(kURL);
+  EXPECT_CALL(*store_, CheckReuse(kInput, kURL.GetOrigin().spec(), &manager))
+      .Times(2);
+  // The user paste the text twice before the store gets to respond.
+  manager.OnPaste(kInput);
+  manager.OnPaste(kInput);
+  testing::Mock::VerifyAndClearExpectations(store_.get());
+
+  std::vector<MatchingReusedCredential> reused_credentials = {
+      {.signon_realm = "www.example2.com",
+       .username = base::ASCIIToUTF16("username1"),
+       .in_store = autofill::PasswordForm::Store::kProfileStore}};
+
+  // CheckProtectedPasswordEntry should get called once, and the reused
+  // credentials get used reported once in this call.
+  EXPECT_CALL(client_,
+              CheckProtectedPasswordEntry(_, _, reused_credentials, _));
+  // Simulate 2 responses from the store with the same reused credentials.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           reused_credentials, /*saved_passwords=*/1);
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           reused_credentials, /*saved_passwords=*/1);
+}
+
 #if defined(OS_ANDROID)
 TEST_F(PasswordReuseDetectionManagerTest,
        CheckReusedCalledWithUncommittedText) {
@@ -225,6 +268,152 @@ TEST_F(PasswordReuseDetectionManagerTest,
 }
 #endif
 
+class PasswordReuseDetectionManagerWithTwoStoresTest
+    : public PasswordReuseDetectionManagerTest {
+ public:
+  PasswordReuseDetectionManagerWithTwoStoresTest() = default;
+  void SetUp() override {
+    PasswordReuseDetectionManagerTest::SetUp();
+    account_store_ = new testing::StrictMock<MockPasswordStore>;
+    CHECK(account_store_->Init(nullptr));
+  }
+  void TearDown() override {
+    account_store_->ShutdownOnUIThread();
+    account_store_ = nullptr;
+    PasswordReuseDetectionManagerTest::TearDown();
+  }
+
+ protected:
+  scoped_refptr<MockPasswordStore> account_store_;
+};
+
+TEST_F(PasswordReuseDetectionManagerWithTwoStoresTest,
+       CheckReuseCalledOnPasteReuseExistsInBothStores) {
+  const GURL kURL("https://www.example.com");
+  const base::string16 kInput =
+      base::ASCIIToUTF16("1234567890abcdefghijklmnopqrstuvxyz");
+
+  EXPECT_CALL(client_, GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store_.get()));
+  EXPECT_CALL(client_, GetAccountPasswordStore())
+      .WillRepeatedly(testing::Return(account_store_.get()));
+
+  PasswordReuseDetectionManager manager(&client_);
+
+  manager.DidNavigateMainFrame(kURL);
+  EXPECT_CALL(*store_, CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  EXPECT_CALL(*account_store_,
+              CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  manager.OnPaste(kInput);
+  testing::Mock::VerifyAndClearExpectations(store_.get());
+  testing::Mock::VerifyAndClearExpectations(account_store_.get());
+
+  std::vector<MatchingReusedCredential> profile_reused_credentials = {
+      {.signon_realm = "www.example2.com",
+       .username = base::ASCIIToUTF16("username1"),
+       .in_store = autofill::PasswordForm::Store::kProfileStore}};
+  // Simulate response from the profile store.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           profile_reused_credentials, /*saved_passwords=*/1);
+
+  std::vector<MatchingReusedCredential> account_reused_credentials{
+      {.signon_realm = "www.example2.com",
+       .username = base::ASCIIToUTF16("username2"),
+       .in_store = autofill::PasswordForm::Store::kAccountStore}};
+
+  // The callback is run only after both stores respond.
+  EXPECT_CALL(client_,
+              CheckProtectedPasswordEntry(
+                  _, _,
+                  testing::UnorderedElementsAre(profile_reused_credentials[0],
+                                                account_reused_credentials[0]),
+                  _));
+  // Simulate response from the account store.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           account_reused_credentials, /*saved_passwords=*/1);
+}
+
+TEST_F(PasswordReuseDetectionManagerWithTwoStoresTest,
+       CheckReuseCalledOnPasteReuseExistsInFirstStoreResponse) {
+  const GURL kURL("https://www.example.com");
+  const base::string16 kInput =
+      base::ASCIIToUTF16("1234567890abcdefghijklmnopqrstuvxyz");
+
+  EXPECT_CALL(client_, GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store_.get()));
+  EXPECT_CALL(client_, GetAccountPasswordStore())
+      .WillRepeatedly(testing::Return(account_store_.get()));
+
+  PasswordReuseDetectionManager manager(&client_);
+
+  manager.DidNavigateMainFrame(kURL);
+  EXPECT_CALL(*store_, CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  EXPECT_CALL(*account_store_,
+              CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  manager.OnPaste(kInput);
+  testing::Mock::VerifyAndClearExpectations(store_.get());
+  testing::Mock::VerifyAndClearExpectations(account_store_.get());
+
+  std::vector<MatchingReusedCredential> profile_reused_credentials = {
+      {.signon_realm = "www.example2.com",
+       .username = base::ASCIIToUTF16("username1"),
+       .in_store = autofill::PasswordForm::Store::kProfileStore}};
+  // Simulate response from the profile store.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           profile_reused_credentials, /*saved_passwords=*/1);
+
+  // The callback is run only after both stores respond.
+  EXPECT_CALL(client_,
+              CheckProtectedPasswordEntry(_, _, profile_reused_credentials, _));
+  // Simulate response from the account store with no reuse found.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/false, /*password_length=*/0,
+                           /*reused_protected_password_hash=*/base::nullopt, {},
+                           /*saved_passwords=*/0);
+}
+
+TEST_F(PasswordReuseDetectionManagerWithTwoStoresTest,
+       CheckReuseCalledOnPasteReuseExistsInSecondStoreResponse) {
+  const GURL kURL("https://www.example.com");
+  const base::string16 kInput =
+      base::ASCIIToUTF16("1234567890abcdefghijklmnopqrstuvxyz");
+
+  EXPECT_CALL(client_, GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store_.get()));
+  EXPECT_CALL(client_, GetAccountPasswordStore())
+      .WillRepeatedly(testing::Return(account_store_.get()));
+
+  PasswordReuseDetectionManager manager(&client_);
+
+  manager.DidNavigateMainFrame(kURL);
+  EXPECT_CALL(*store_, CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  EXPECT_CALL(*account_store_,
+              CheckReuse(kInput, kURL.GetOrigin().spec(), &manager));
+  manager.OnPaste(kInput);
+  testing::Mock::VerifyAndClearExpectations(store_.get());
+  testing::Mock::VerifyAndClearExpectations(account_store_.get());
+
+  // Simulate response from the account store with no reuse found.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/false, /*password_length=*/0,
+                           /*reused_protected_password_hash=*/base::nullopt, {},
+                           /*saved_passwords=*/0);
+
+  std::vector<MatchingReusedCredential> profile_reused_credentials = {
+      {.signon_realm = "www.example2.com",
+       .username = base::ASCIIToUTF16("username1"),
+       .in_store = autofill::PasswordForm::Store::kProfileStore}};
+
+  // The callback is run only after both stores respond.
+  EXPECT_CALL(client_,
+              CheckProtectedPasswordEntry(_, _, profile_reused_credentials, _));
+  // Simulate response from the profile store.
+  manager.OnReuseCheckDone(/*is_reuse_found=*/true, /*password_length=*/10,
+                           /*reused_protected_password_hash=*/base::nullopt,
+                           profile_reused_credentials, /*saved_passwords=*/1);
+}
+
 }  // namespace
 
 }  // namespace password_manager