[Passwords] Guard checking a password after edit on prefs

This change implements guarding the automatic checking of an edited
password on the leak detection and safe browsing prefs.

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

chrome/browser/extensions/api/passwords_private/password_check_delegate.cc

@@ -142,7 +142,8 @@ PasswordCheckDelegate::PasswordCheckDelegate(Profile* profile)
                                         &saved_passwords_presenter_),
       bulk_leak_check_service_adapter_(
           &saved_passwords_presenter_,
-          BulkLeakCheckServiceFactory::GetForProfile(profile_)) {
+          BulkLeakCheckServiceFactory::GetForProfile(profile_),
+          profile_->GetPrefs()) {
   observed_saved_passwords_presenter_.Add(&saved_passwords_presenter_);
   observed_compromised_credentials_provider_.Add(
       &compromised_credentials_provider_);

components/password_manager/core/browser/leak_detection_delegate.cc

@@ -28,8 +28,9 @@ namespace {
 
 using Logger = autofill::SavePasswordProgressLogger;
 
-void LogString(PasswordManagerClient* client, Logger::StringID string_id) {
-  if (password_manager_util::IsLoggingActive(client)) {
+void LogString(const PasswordManagerClient* client,
+               Logger::StringID string_id) {
+  if (client && password_manager_util::IsLoggingActive(client)) {
     BrowserSavePasswordProgressLogger logger(client->GetLogManager());
     logger.LogMessage(string_id);
   }
@@ -47,37 +48,8 @@ void LeakDetectionDelegate::StartLeakCheck(const autofill::PasswordForm& form) {
   if (client_->IsIncognito())
     return;
 
-  bool is_leak_protection_on = client_->GetPrefs()->GetBoolean(
-      password_manager::prefs::kPasswordLeakDetectionEnabled);
-
-  // Leak detection can only start if:
-  // 1. The user has not opted out and Safe Browsing is turned on, or
-  // 2. The user is an enhanced protection user
-  // Safe Browsing is only available on non-IOS.
-#if defined(OS_IOS)
-  if (!is_leak_protection_on) {
-    LogString(client_, Logger::STRING_LEAK_DETECTION_DISABLED_FEATURE);
+  if (!CanStartLeakCheck(*client_->GetPrefs(), client_))
     return;
-  }
-#else
-  safe_browsing::SafeBrowsingState sb_state =
-      safe_browsing::GetSafeBrowsingState(*client_->GetPrefs());
-  switch (sb_state) {
-    case safe_browsing::NO_SAFE_BROWSING:
-      LogString(client_, Logger::STRING_LEAK_DETECTION_DISABLED_SAFE_BROWSING);
-      return;
-    case safe_browsing::STANDARD_PROTECTION:
-      if (!is_leak_protection_on) {
-        LogString(client_, Logger::STRING_LEAK_DETECTION_DISABLED_FEATURE);
-        return;
-      }
-      // feature is on.
-      break;
-    case safe_browsing::ENHANCED_PROTECTION:
-      // feature is on.
-      break;
-  }
-#endif
 
   if (form.username_value.empty())
     return;
@@ -156,4 +128,37 @@ void LeakDetectionDelegate::OnError(LeakDetectionError error) {
   }
 }
 
+bool CanStartLeakCheck(const PrefService& prefs,
+                       const PasswordManagerClient* client) {
+  const bool is_leak_protection_on =
+      prefs.GetBoolean(password_manager::prefs::kPasswordLeakDetectionEnabled);
+
+  // Leak detection can only start if:
+  // 1. The user has not opted out and Safe Browsing is turned on, or
+  // 2. The user is an enhanced protection user
+  // Safe Browsing is only available on non-IOS.
+#if defined(OS_IOS)
+  if (!is_leak_protection_on)
+    LogString(client, Logger::STRING_LEAK_DETECTION_DISABLED_FEATURE);
+  return is_leak_protection_on;
+#else
+  safe_browsing::SafeBrowsingState sb_state =
+      safe_browsing::GetSafeBrowsingState(prefs);
+  switch (sb_state) {
+    case safe_browsing::NO_SAFE_BROWSING:
+      LogString(client, Logger::STRING_LEAK_DETECTION_DISABLED_SAFE_BROWSING);
+      return false;
+    case safe_browsing::STANDARD_PROTECTION:
+      if (!is_leak_protection_on)
+        LogString(client, Logger::STRING_LEAK_DETECTION_DISABLED_FEATURE);
+      return is_leak_protection_on;
+    case safe_browsing::ENHANCED_PROTECTION:
+      // feature is on.
+      break;
+  }
+
+  return true;
+#endif
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/leak_detection_delegate.h

@@ -17,6 +17,8 @@ namespace autofill {
 struct PasswordForm;
 }  // namespace autofill
 
+class PrefService;
+
 namespace password_manager {
 
 class LeakDetectionCheck;
@@ -77,6 +79,11 @@ class LeakDetectionDelegate : public LeakDetectionDelegateInterface {
   std::unique_ptr<LeakDetectionDelegateHelper> helper_;
 };
 
+// Determines whether the leak check can be started depending on |prefs|. Will
+// use |client| for logging if non-null.
+bool CanStartLeakCheck(const PrefService& prefs,
+                       const PasswordManagerClient* client = nullptr);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_LEAK_DETECTION_DELEGATE_H_

components/password_manager/core/browser/leak_detection_delegate_unittest.cc

@@ -207,6 +207,7 @@ TEST_F(LeakDetectionDelegateTest, StartCheckWithStandardProtection) {
   delegate().StartLeakCheck(form);
 
   EXPECT_TRUE(delegate().leak_check());
+  EXPECT_TRUE(CanStartLeakCheck(*pref_service()));
 }
 
 TEST_F(LeakDetectionDelegateTest, StartCheckWithEnhancedProtection) {
@@ -225,6 +226,7 @@ TEST_F(LeakDetectionDelegateTest, StartCheckWithEnhancedProtection) {
   delegate().StartLeakCheck(form);
 
   EXPECT_TRUE(delegate().leak_check());
+  EXPECT_TRUE(CanStartLeakCheck(*pref_service()));
 }
 
 TEST_F(LeakDetectionDelegateTest, DoNotStartCheckWithoutSafeBrowsing) {
@@ -237,6 +239,7 @@ TEST_F(LeakDetectionDelegateTest, DoNotStartCheckWithoutSafeBrowsing) {
   delegate().StartLeakCheck(form);
 
   EXPECT_FALSE(delegate().leak_check());
+  EXPECT_FALSE(CanStartLeakCheck(*pref_service()));
 }
 
 TEST_F(LeakDetectionDelegateTest, DoNotStartLeakCheckIfLeakCheckIsOff) {
@@ -249,6 +252,7 @@ TEST_F(LeakDetectionDelegateTest, DoNotStartLeakCheckIfLeakCheckIsOff) {
   delegate().StartLeakCheck(form);
 
   EXPECT_FALSE(delegate().leak_check());
+  EXPECT_FALSE(CanStartLeakCheck(*pref_service()));
 }
 #endif
 

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

@@ -12,7 +12,9 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/leak_detection/bulk_leak_check.h"
 #include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
+#include "components/password_manager/core/browser/leak_detection_delegate.h"
 #include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
+#include "components/prefs/pref_service.h"
 
 namespace password_manager {
 
@@ -41,10 +43,12 @@ bool operator<(const CanonicalizedCredential& lhs,
 
 BulkLeakCheckServiceAdapter::BulkLeakCheckServiceAdapter(
     SavedPasswordsPresenter* presenter,
-    BulkLeakCheckService* service)
-    : presenter_(presenter), service_(service) {
+    BulkLeakCheckService* service,
+    PrefService* prefs)
+    : presenter_(presenter), service_(service), prefs_(prefs) {
   DCHECK(presenter_);
   DCHECK(service_);
+  DCHECK(prefs_);
   presenter_->AddObserver(this);
 }
 
@@ -91,11 +95,13 @@ size_t BulkLeakCheckServiceAdapter::GetPendingChecksCount() const {
 }
 
 void BulkLeakCheckServiceAdapter::OnEdited(const PasswordForm& form) {
-  // Here no extra canonicalization is needed, as there are no other forms we
-  // could de-dupe before we pass it on to the service.
-  std::vector<LeakCheckCredential> credentials;
-  credentials.emplace_back(form.username_value, form.password_value);
-  service_->CheckUsernamePasswordPairs(std::move(credentials));
+  if (CanStartLeakCheck(*prefs_)) {
+    // Here no extra canonicalization is needed, as there are no other forms we
+    // could de-dupe before we pass it on to the service.
+    std::vector<LeakCheckCredential> credentials;
+    credentials.emplace_back(form.username_value, form.password_value);
+    service_->CheckUsernamePasswordPairs(std::move(credentials));
+  }
 }
 
 }  // namespace password_manager

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

@@ -13,6 +13,8 @@ namespace autofill {
 struct PasswordForm;
 }
 
+class PrefService;
+
 namespace password_manager {
 
 // This class serves as an apdater for the BulkLeakCheckService and exposes an
@@ -20,7 +22,8 @@ namespace password_manager {
 class BulkLeakCheckServiceAdapter : public SavedPasswordsPresenter::Observer {
  public:
   BulkLeakCheckServiceAdapter(SavedPasswordsPresenter* presenter,
-                              BulkLeakCheckService* service);
+                              BulkLeakCheckService* service,
+                              PrefService* prefs);
   ~BulkLeakCheckServiceAdapter() override;
 
   // Instructs the adapter to start a check. This is a no-op in case a check is
@@ -47,6 +50,8 @@ class BulkLeakCheckServiceAdapter : public SavedPasswordsPresenter::Observer {
   // null and must outlive the adapter.
   SavedPasswordsPresenter* presenter_ = nullptr;
   BulkLeakCheckService* service_ = nullptr;
+
+  PrefService* prefs_ = nullptr;
 };
 
 }  // namespace password_manager

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

@@ -20,6 +20,10 @@
 #include "components/password_manager/core/browser/leak_detection/mock_leak_detection_check_factory.h"
 #include "components/password_manager/core/browser/test_password_store.h"
 #include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/sync/model/syncable_service.h"
 #include "services/network/test/test_shared_url_loader_factory.h"
@@ -95,6 +99,11 @@ class BulkLeakCheckServiceAdapterTest : public ::testing::Test {
     service_.set_leak_factory(std::move(factory));
     store_->Init(syncer::SyncableService::StartSyncFlare(),
                  /*prefs=*/nullptr);
+    prefs_.registry()->RegisterBooleanPref(prefs::kPasswordLeakDetectionEnabled,
+                                           true);
+    prefs_.registry()->RegisterBooleanPref(::prefs::kSafeBrowsingEnabled, true);
+    prefs_.registry()->RegisterBooleanPref(::prefs::kSafeBrowsingEnhanced,
+                                           false);
   }
 
   ~BulkLeakCheckServiceAdapterTest() override {
@@ -105,6 +114,7 @@ class BulkLeakCheckServiceAdapterTest : public ::testing::Test {
   TestPasswordStore& store() { return *store_; }
   SavedPasswordsPresenter& presenter() { return presenter_; }
   MockLeakDetectionCheckFactory& factory() { return *factory_; }
+  PrefService& prefs() { return prefs_; }
   BulkLeakCheckServiceAdapter& adapter() { return adapter_; }
 
   void RunUntilIdle() { task_env_.RunUntilIdle(); }
@@ -119,7 +129,8 @@ class BulkLeakCheckServiceAdapterTest : public ::testing::Test {
       identity_test_env_.identity_manager(),
       base::MakeRefCounted<network::TestSharedURLLoaderFactory>()};
   MockLeakDetectionCheckFactory* factory_ = nullptr;
-  BulkLeakCheckServiceAdapter adapter_{&presenter_, &service_};
+  TestingPrefServiceSimple prefs_;
+  BulkLeakCheckServiceAdapter adapter_{&presenter_, &service_, &prefs_};
 };
 
 }  // namespace
@@ -216,9 +227,26 @@ TEST_F(BulkLeakCheckServiceAdapterTest, StopBulkLeakCheck) {
             adapter().GetBulkLeakCheckState());
 }
 
+// Tests that editing a password through the presenter does not result in
+// another call to CheckCredentials with a corresponding change to the checked
+// password if the corresponding prefs are not set.
+TEST_F(BulkLeakCheckServiceAdapterTest, OnEditedNoPrefs) {
+  prefs().SetBoolean(prefs::kPasswordLeakDetectionEnabled, false);
+  prefs().SetBoolean(::prefs::kSafeBrowsingEnabled, false);
+
+  PasswordForm password =
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1);
+  store().AddLogin(password);
+  RunUntilIdle();
+
+  EXPECT_CALL(factory(), TryCreateBulkLeakCheck).Times(0);
+  presenter().EditPassword(password, base::ASCIIToUTF16(kPassword2));
+}
+
 // Tests that editing a password through the presenter will result in another
-// call to CheckCredentials with a corresponding change to the checked password.
-TEST_F(BulkLeakCheckServiceAdapterTest, OnEdited) {
+// call to CheckCredentials with a corresponding change to the checked password
+// if the corresponding prefs are set.
+TEST_F(BulkLeakCheckServiceAdapterTest, OnEditedWithPrefs) {
   PasswordForm password =
       MakeSavedPassword(kExampleCom, kUsername1, kPassword1);
   store().AddLogin(password);