[Passwords] PasswordProtectionService picks the correct store when ...

... persisting and removing compromised credentials records.

Bug: 1119286
Change-Id: I1899c6802e2fff955bfc0f1cdb6942a787784e70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410492
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Bettina Dea <bdea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807664}

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -22,6 +22,7 @@
 #include "chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router.h"
 #include "chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/password_manager/account_password_store_factory.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/advanced_protection_status_manager.h"
@@ -1727,14 +1728,14 @@ void ChromePasswordProtectionService::PersistPhishedSavedPasswordCredential(
         matching_reused_credentials) {
   if (!profile_)
     return;
-  scoped_refptr<password_manager::PasswordStore> password_store =
-      GetProfilePasswordStore();
 
-  // Password store can be null in tests.
-  if (!password_store) {
-    return;
-  }
   for (const auto& credential : matching_reused_credentials) {
+    password_manager::PasswordStore* password_store =
+        GetStoreForReusedCredential(credential);
+    // Password store can be null in tests.
+    if (!password_store) {
+      continue;
+    }
     password_store->AddCompromisedCredentials(
         {credential.signon_realm, credential.username, base::Time::Now(),
          password_manager::CompromiseType::kPhished});
@@ -1746,14 +1747,14 @@ void ChromePasswordProtectionService::RemovePhishedSavedPasswordCredential(
         matching_reused_credentials) {
   if (!profile_)
     return;
-  scoped_refptr<password_manager::PasswordStore> password_store =
-      GetProfilePasswordStore();
 
-  // Password store can be null in tests.
-  if (!password_store) {
-    return;
-  }
   for (const auto& credential : matching_reused_credentials) {
+    password_manager::PasswordStore* password_store =
+        GetStoreForReusedCredential(credential);
+    // Password store can be null in tests.
+    if (!password_store) {
+      continue;
+    }
     password_store->RemoveCompromisedCredentials(
         credential.signon_realm, credential.username,
         password_manager::RemoveCompromisedCredentialsReason::
@@ -1770,6 +1771,15 @@ ChromePasswordProtectionService::GetProfilePasswordStore() const {
       .get();
 }
 
+password_manager::PasswordStore*
+ChromePasswordProtectionService::GetAccountPasswordStore() const {
+  // Always use EXPLICIT_ACCESS as the password manager checks IsIncognito
+  // itself when it shouldn't access the PasswordStore.
+  return AccountPasswordStoreFactory::GetForProfile(
+             profile_, ServiceAccessType::EXPLICIT_ACCESS)
+      .get();
+}
+
 void ChromePasswordProtectionService::SanitizeReferrerChain(
     ReferrerChain* referrer_chain) {
   SafeBrowsingNavigationObserverManager::SanitizeReferrerChain(referrer_chain);
@@ -1829,4 +1839,15 @@ gfx::Size ChromePasswordProtectionService::GetCurrentContentAreaSize() const {
 }
 #endif  // FULL_SAFE_BROWSING
 
+password_manager::PasswordStore*
+ChromePasswordProtectionService::GetStoreForReusedCredential(
+    const password_manager::MatchingReusedCredential& reused_credential) {
+  if (!profile_)
+    return nullptr;
+  return reused_credential.in_store ==
+                 autofill::PasswordForm::Store::kAccountStore
+             ? GetAccountPasswordStore()
+             : GetProfilePasswordStore();
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_password_protection_service.h

@@ -246,6 +246,11 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   // Returns the profile PasswordStore associated with this instance.
   password_manager::PasswordStore* GetProfilePasswordStore() const;
 
+  // Returns the account PasswordStore associated with this instance. The
+  // account password store contains passwords stored in the account and is
+  // accessible only when the user is signed in.
+  password_manager::PasswordStore* GetAccountPasswordStore() const;
+
   // Gets the type of sync account associated with current profile or
   // |NOT_SIGNED_IN|.
   LoginReputationClientRequest::PasswordReuseEvent::SyncAccountType
@@ -558,6 +563,9 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   // Code shared by both ctors.
   void Init();
 
+  password_manager::PasswordStore* GetStoreForReusedCredential(
+      const password_manager::MatchingReusedCredential& reused_credential);
+
   scoped_refptr<SafeBrowsingUIManager> ui_manager_;
   TriggerManager* trigger_manager_;
   // Profile associated with this instance.

chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc

@@ -254,9 +254,8 @@ class ChromePasswordProtectionServiceTest
         PasswordProtectionTrigger::PHISHING_REUSE);
     HostContentSettingsMap::RegisterProfilePrefs(test_pref_service_.registry());
     content_setting_map_ = new HostContentSettingsMap(
-        &test_pref_service_, false /* is_off_the_record */,
-        false /* store_last_modified */,
-        false /* restore_session*/);
+        &test_pref_service_, /*is_off_the_record=*/false,
+        /*store_last_modified=*/false, /*restore_session=*/false);
 
     cache_manager_ = std::make_unique<VerdictCacheManager>(
         nullptr, content_setting_map_.get());
@@ -1621,4 +1620,54 @@ TEST_F(ChromePasswordProtectionServiceTest, VerifyGetPingNotSentReason) {
   }
 }
 
+namespace {
+
+class ChromePasswordProtectionServiceWithAccountPasswordStoreTest
+    : public ChromePasswordProtectionServiceTest {
+ public:
+  ChromePasswordProtectionServiceWithAccountPasswordStoreTest() {
+    feature_list_.InitAndEnableFeature(
+        password_manager::features::kEnablePasswordsAccountStorage);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(ChromePasswordProtectionServiceWithAccountPasswordStoreTest,
+       VerifyPersistPhishedSavedPasswordCredential) {
+  service_->ConfigService(/*is_incognito=*/false,
+                          /*is_extended_reporting=*/true);
+  std::vector<password_manager::MatchingReusedCredential> credentials = {
+      {.signon_realm = "http://example.test",
+       .in_store = autofill::PasswordForm::Store::kAccountStore},
+      {.signon_realm = "http://2.example.test",
+       .in_store = autofill::PasswordForm::Store::kAccountStore}};
+
+  EXPECT_CALL(*account_password_store_, AddCompromisedCredentialsImpl(_))
+      .Times(2);
+  service_->PersistPhishedSavedPasswordCredential(credentials);
+}
+
+TEST_F(ChromePasswordProtectionServiceWithAccountPasswordStoreTest,
+       VerifyRemovePhishedSavedPasswordCredential) {
+  service_->ConfigService(/*is_incognito=*/false,
+                          /*is_extended_reporting=*/true);
+  std::vector<password_manager::MatchingReusedCredential> credentials = {
+      {"http://example.test", base::ASCIIToUTF16("username1"),
+       autofill::PasswordForm::Store::kAccountStore},
+      {"http://2.example.test", base::ASCIIToUTF16("username2"),
+       autofill::PasswordForm::Store::kAccountStore}};
+
+  EXPECT_CALL(*account_password_store_,
+              RemoveCompromisedCredentialsImpl(
+                  _, _,
+                  password_manager::RemoveCompromisedCredentialsReason::
+                      kMarkSiteAsLegitimate))
+      .Times(2);
+  service_->RemovePhishedSavedPasswordCredential(credentials);
+}
+
+}  // namespace
+
 }  // namespace safe_browsing