Avoid accessing PrefService from PasswordReuseDetector

Previous implementation queries PrefService in
PasswordReuseDetector::CheckNonGaiaEnterprisePasswordReuse(..). However,
CheckNonGaiaEnterprisePasswordReuse(..) is called in a backgroud sequence,
which violates the valid sequence check in PrefService. In other words,
PrefService can NOT be called on background thread. Otherwise, it
will causes crashing.
This CL changes the implementation by propagating enterprise URLs to
PasswordReuseDetector when PasswordStore is initialized to avoid accessing
PrefService from PasswordReuseDetector.

Bug: 852138
Change-Id: I8556f259e6c0e4a7eee077e70db142739336ed67
Reviewed-on: https://chromium-review.googlesource.com/1107247
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569358}

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -190,6 +190,16 @@ ChromePasswordProtectionService::ChromePasswordProtectionService(
       base::BindRepeating(
           &ChromePasswordProtectionService::OnWarningTriggerChanged,
           base::Unretained(this)));
+  pref_change_registrar_->Add(
+      prefs::kPasswordProtectionLoginURLs,
+      base::BindRepeating(
+          &ChromePasswordProtectionService::OnEnterprisePasswordUrlChanged,
+          base::Unretained(this)));
+  pref_change_registrar_->Add(
+      prefs::kPasswordProtectionChangePasswordURL,
+      base::BindRepeating(
+          &ChromePasswordProtectionService::OnEnterprisePasswordUrlChanged,
+          base::Unretained(this)));
   password_manager::HashPasswordManager hash_password_manager;
   hash_password_manager.set_prefs(profile->GetPrefs());
   base::Optional<password_manager::PasswordHashData> sync_hash_data =
@@ -1190,4 +1200,10 @@ void ChromePasswordProtectionService::OnWarningTriggerChanged() {
       ->SchedulePasswordHashUpdate(/*should_log_metrics*/ false);
 }
 
+void ChromePasswordProtectionService::OnEnterprisePasswordUrlChanged() {
+  PasswordStoreFactory::GetForProfile(profile_,
+                                      ServiceAccessType::EXPLICIT_ACCESS)
+      ->ScheduleEnterprisePasswordURLUpdate();
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_password_protection_service.h

@@ -339,6 +339,10 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   // enterprise password hashes.
   void OnWarningTriggerChanged();
 
+  // Informs PasswordReuseDetector that enterprise password URLs (login URL or
+  // change password URL) have been changed.
+  void OnEnterprisePasswordUrlChanged();
+
   scoped_refptr<SafeBrowsingUIManager> ui_manager_;
   TriggerManager* trigger_manager_;
   // Profile associated with this instance.

components/password_manager/core/browser/BUILD.gn

@@ -213,18 +213,12 @@ static_library("browser") {
     "//url",
   ]
 
-  if (password_reuse_detection_support) {
-    deps += [
-      "//components/safe_browsing:features",
-      "//components/safe_browsing/common:safe_browsing_prefs",
-    ]
-  }
-
   if (!is_ios) {
     sources += [
       "hsts_query.cc",
       "hsts_query.h",
     ]
+    deps += [ "//components/safe_browsing/common:safe_browsing_prefs" ]
   }
 
   if (is_posix && !is_mac && !is_ios) {

components/password_manager/core/browser/password_reuse_detector.cc

@@ -68,7 +68,7 @@ bool ReverseStringLess::operator()(const base::string16& lhs,
                                       rhs.rend());
 }
 
-PasswordReuseDetector::PasswordReuseDetector() : prefs_(nullptr) {}
+PasswordReuseDetector::PasswordReuseDetector() {}
 
 PasswordReuseDetector::~PasswordReuseDetector() {}
 
@@ -160,10 +160,9 @@ PasswordReuseDetector::CheckNonGaiaEnterprisePasswordReuse(
   // Skips password reuse check if |domain| matches enterprise login URL or
   // enterprise change password URL.
   GURL page_url(domain);
-  if (!prefs_ ||
-      safe_browsing::MatchesPasswordProtectionLoginURL(page_url, *prefs_) ||
-      safe_browsing::MatchesPasswordProtectionChangePasswordURL(page_url,
-                                                                *prefs_)) {
+  if (enterprise_password_urls_.has_value() &&
+      safe_browsing::MatchesURLList(page_url,
+                                    enterprise_password_urls_.value())) {
     return base::nullopt;
   }
 
@@ -228,6 +227,20 @@ void PasswordReuseDetector::UseNonGaiaEnterprisePasswordHash(
   enterprise_password_hash_data_list_ = std::move(password_hash_data_list);
 }
 
+void PasswordReuseDetector::UseEnterprisePasswordURLs(
+    base::Optional<std::vector<GURL>> enterprise_login_urls,
+    base::Optional<GURL> enterprise_change_password_url) {
+  enterprise_password_urls_ = std::move(enterprise_login_urls);
+  if (!enterprise_change_password_url.has_value() ||
+      !enterprise_change_password_url->is_valid()) {
+    return;
+  }
+
+  if (!enterprise_password_urls_)
+    enterprise_password_urls_ = base::make_optional<std::vector<GURL>>();
+  enterprise_password_urls_->push_back(enterprise_change_password_url.value());
+}
+
 void PasswordReuseDetector::ClearGaiaPasswordHash(const std::string& username) {
   if (!gaia_password_hash_data_list_)
     return;

components/password_manager/core/browser/password_reuse_detector.h

@@ -65,14 +65,18 @@ class PasswordReuseDetector : public PasswordStoreConsumer {
   void UseNonGaiaEnterprisePasswordHash(
       base::Optional<std::vector<PasswordHashData>> password_hash_data_list);
 
+  // Stores enterprise login URLs and change password URL.
+  // These URLs should be skipped in enterprise password reuse checking.
+  void UseEnterprisePasswordURLs(
+      base::Optional<std::vector<GURL>> enterprise_login_urls,
+      base::Optional<GURL> enterprise_change_password_url);
+
   void ClearGaiaPasswordHash(const std::string& username);
 
   void ClearAllGaiaPasswordHash();
 
   void ClearAllEnterprisePasswordHash();
 
-  void SetPrefs(PrefService* prefs) { prefs_ = prefs; }
-
  private:
   using passwords_iterator = std::map<base::string16,
                                       std::set<std::string>,
@@ -127,7 +131,7 @@ class PasswordReuseDetector : public PasswordStoreConsumer {
   base::Optional<std::vector<PasswordHashData>>
       enterprise_password_hash_data_list_;
 
-  PrefService* prefs_;
+  base::Optional<std::vector<GURL>> enterprise_password_urls_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordReuseDetector);
 };

components/password_manager/core/browser/password_reuse_detector_unittest.cc

@@ -14,10 +14,6 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/hash_password_manager.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/testing_pref_service.h"
-#include "components/safe_browsing/common/safe_browsing_prefs.h"
-#include "components/safe_browsing/features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -104,15 +100,13 @@ std::vector<PasswordHashData> PrepareEnterprisePasswordData(
   return result;
 }
 
-void ConfigureEnterprisePasswordProtection(TestingPrefServiceSimple* prefs) {
-  prefs->registry()->RegisterStringPref(
-      prefs::kPasswordProtectionChangePasswordURL, "");
-  prefs->registry()->RegisterListPref(prefs::kPasswordProtectionLoginURLs);
-  prefs->SetString(prefs::kPasswordProtectionChangePasswordURL,
-                   "https://changepassword.example.com/");
-  base::ListValue login_urls;
-  login_urls.AppendString("https://login.example.com");
-  prefs->Set(prefs::kPasswordProtectionLoginURLs, login_urls);
+void ConfigureEnterprisePasswordProtection(
+    PasswordReuseDetector* reuse_detector) {
+  base::Optional<std::vector<GURL>> login_urls =
+      base::make_optional<std::vector<GURL>>();
+  login_urls->push_back(GURL("https://login.example.com"));
+  reuse_detector->UseEnterprisePasswordURLs(
+      login_urls, GURL("https://changepassword.example.com/"));
 }
 
 TEST(PasswordReuseDetectorTest, TypingPasswordOnDifferentSite) {
@@ -296,14 +290,8 @@ TEST(PasswordReuseDetectorTest, GaiaPasswordReuseFound) {
 }
 
 TEST(PasswordReuseDetectorTest, EnterprisePasswordNoReuse) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      safe_browsing::kEnterprisePasswordProtectionV1);
-  TestingPrefServiceSimple prefs;
-  ConfigureEnterprisePasswordProtection(&prefs);
-
   PasswordReuseDetector reuse_detector;
-  reuse_detector.SetPrefs(&prefs);
+  ConfigureEnterprisePasswordProtection(&reuse_detector);
   reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords()));
   MockPasswordReuseDetectorConsumer mockConsumer;
 
@@ -330,14 +318,8 @@ TEST(PasswordReuseDetectorTest, EnterprisePasswordNoReuse) {
 }
 
 TEST(PasswordReuseDetectorTest, EnterprisePasswordReuseFound) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      safe_browsing::kEnterprisePasswordProtectionV1);
-  TestingPrefServiceSimple prefs;
-  ConfigureEnterprisePasswordProtection(&prefs);
-
   PasswordReuseDetector reuse_detector;
-  reuse_detector.SetPrefs(&prefs);
+  ConfigureEnterprisePasswordProtection(&reuse_detector);
   reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords()));
   MockPasswordReuseDetectorConsumer mockConsumer;
 
@@ -406,17 +388,11 @@ TEST(PasswordReuseDetectorTest, MatchSavedPasswordButNotGaiaPassword) {
 }
 
 TEST(PasswordReuseDetectorTest, MatchEnterpriseAndMultipleSavedPasswords) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      safe_browsing::kEnterprisePasswordProtectionV1);
-  TestingPrefServiceSimple prefs;
-  ConfigureEnterprisePasswordProtection(&prefs);
-
   const std::vector<std::pair<std::string, std::string>> domain_passwords = {
       {"https://a.com", "34567890"}, {"https://b.com", "01234567890"},
   };
   PasswordReuseDetector reuse_detector;
-  reuse_detector.SetPrefs(&prefs);
+  ConfigureEnterprisePasswordProtection(&reuse_detector);
   reuse_detector.OnGetPasswordStoreResults(GetForms(domain_passwords));
 
   std::string enterprise_password = "1234567890";
@@ -449,14 +425,8 @@ TEST(PasswordReuseDetectorTest, MatchEnterpriseAndMultipleSavedPasswords) {
 }
 
 TEST(PasswordReuseDetectorTest, MatchSavedPasswordButNotEnterprisePassword) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      safe_browsing::kEnterprisePasswordProtectionV1);
-  TestingPrefServiceSimple prefs;
-  ConfigureEnterprisePasswordProtection(&prefs);
-
   PasswordReuseDetector reuse_detector;
-  reuse_detector.SetPrefs(&prefs);
+  ConfigureEnterprisePasswordProtection(&reuse_detector);
   reuse_detector.OnGetPasswordStoreResults(GetForms(GetTestDomainsPasswords()));
   MockPasswordReuseDetectorConsumer mockConsumer;
 
@@ -472,17 +442,11 @@ TEST(PasswordReuseDetectorTest, MatchSavedPasswordButNotEnterprisePassword) {
 }
 
 TEST(PasswordReuseDetectorTest, MatchGaiaEnterpriseAndSavedPassword) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      safe_browsing::kEnterprisePasswordProtectionV1);
-  TestingPrefServiceSimple prefs;
-  ConfigureEnterprisePasswordProtection(&prefs);
-
   const std::vector<std::pair<std::string, std::string>> domain_passwords = {
       {"https://a.com", "34567890"}, {"https://b.com", "01234567890"},
   };
   PasswordReuseDetector reuse_detector;
-  reuse_detector.SetPrefs(&prefs);
+  ConfigureEnterprisePasswordProtection(&reuse_detector);
   reuse_detector.OnGetPasswordStoreResults(GetForms(domain_passwords));
 
   std::string gaia_password = "123456789";

components/password_manager/core/browser/password_store.cc

@@ -29,6 +29,7 @@
 
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
 #include "components/password_manager/core/browser/password_store_signin_notifier.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #endif
 
 using autofill::PasswordForm;
@@ -366,6 +367,7 @@ void PasswordStore::PrepareSyncPasswordHashData(
   // the migration.
   hash_password_manager_.MaybeMigrateExistingSyncPasswordHash(sync_username);
   SchedulePasswordHashUpdate(/*should_log_metrics=*/true);
+  ScheduleEnterprisePasswordURLUpdate();
 }
 
 void PasswordStore::SaveGaiaPasswordHash(
@@ -433,6 +435,18 @@ void PasswordStore::SchedulePasswordHashUpdate(bool should_log_metrics) {
       should_log_metrics));
 }
 
+void PasswordStore::ScheduleEnterprisePasswordURLUpdate() {
+  std::vector<GURL> enterprise_login_urls;
+  safe_browsing::GetPasswordProtectionLoginURLsPref(*prefs_,
+                                                    &enterprise_login_urls);
+  GURL enterprise_change_password_url =
+      safe_browsing::GetPasswordProtectionChangePasswordURLPref(*prefs_);
+
+  ScheduleTask(base::BindRepeating(&PasswordStore::SaveEnterprisePasswordURLs,
+                                   this, base::Passed(&enterprise_login_urls),
+                                   enterprise_change_password_url));
+}
+
 #endif
 
 PasswordStore::~PasswordStore() {
@@ -454,7 +468,6 @@ void PasswordStore::InitOnBackgroundSequence(
 // TODO(crbug.com/706392): Fix password reuse detection for Android.
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
   reuse_detector_ = new PasswordReuseDetector;
-  reuse_detector_->SetPrefs(prefs_);
   GetAutofillableLoginsImpl(
       std::make_unique<GetLoginsRequest>(reuse_detector_));
 #endif
@@ -554,6 +567,15 @@ void PasswordStore::SaveProtectedPasswordHashImpl(
       std::move(enterprise_password_hash_list));
 }
 
+void PasswordStore::SaveEnterprisePasswordURLs(
+    const std::vector<GURL>& enterprise_login_urls,
+    const GURL& enterprise_change_password_url) {
+  if (!reuse_detector_)
+    return;
+  reuse_detector_->UseEnterprisePasswordURLs(std::move(enterprise_login_urls),
+                                             enterprise_change_password_url);
+}
+
 void PasswordStore::ClearProtectedPasswordHashImpl() {
   // TODO(crbug.com/844134): Find a way to clear corresponding Gaia password
   // hash when user logs out individual Gaia account in content area.

components/password_manager/core/browser/password_store.h

@@ -293,9 +293,13 @@ class PasswordStore : protected PasswordStoreSync,
   void SetPasswordStoreSigninNotifier(
       std::unique_ptr<PasswordStoreSigninNotifier> notifier);
 
-  // Schedule the update of password hashes used by reuse detector.
+  // Schedules the update of password hashes used by reuse detector.
   void SchedulePasswordHashUpdate(bool should_log_metrics);
 
+  // Schedules the update of enterprise login and change password URLs.
+  // These URLs are used in enterprise password reuse detection.
+  void ScheduleEnterprisePasswordURLUpdate();
+
 #endif
 
  protected:
@@ -483,6 +487,12 @@ class PasswordStore : protected PasswordStoreSync,
       PasswordHashDataList protected_password_data_list,
       bool should_log_metrics);
 
+  // Propagates enterprise login urls and change password url to
+  // |reuse_detector_|.
+  void SaveEnterprisePasswordURLs(
+      const std::vector<GURL>& enterprise_login_urls,
+      const GURL& enterprise_change_password_url);
+
   // Synchronous implementation of ClearProtectedPasswordHash().
   void ClearProtectedPasswordHashImpl();
 #endif

components/safe_browsing/common/safe_browsing_prefs.cc

@@ -483,12 +483,15 @@ bool MatchesPasswordProtectionLoginURL(const GURL& url,
 
   std::vector<GURL> login_urls;
   GetPasswordProtectionLoginURLsPref(prefs, &login_urls);
-  if (login_urls.empty())
-    return false;
+  return MatchesURLList(url, login_urls);
+}
 
-  GURL simple_url = GetSimplifiedURL(url);
-  for (const GURL& login_url : login_urls) {
-    if (GetSimplifiedURL(login_url) == simple_url) {
+bool MatchesURLList(const GURL& target_url, const std::vector<GURL> url_list) {
+  if (url_list.empty() || !target_url.is_valid())
+    return false;
+  GURL simple_target_url = GetSimplifiedURL(target_url);
+  for (const GURL& url : url_list) {
+    if (GetSimplifiedURL(url) == simple_target_url) {
       return true;
     }
   }

components/safe_browsing/common/safe_browsing_prefs.h

@@ -247,6 +247,9 @@ GURL GetPasswordProtectionChangePasswordURLPref(const PrefService& prefs);
 bool MatchesPasswordProtectionChangePasswordURL(const GURL& url,
                                                 const PrefService& prefs);
 
+// Helper function to match a |target_url| against |url_list|.
+bool MatchesURLList(const GURL& target_url, const std::vector<GURL> url_list);
+
 }  // namespace safe_browsing
 
 #endif  // COMPONENTS_SAFE_BROWSING_COMMON_SAFE_BROWSING_PREFS_H_