[Password change] Offer automatic password change only to sync users

Bug: 1086114, 1092444
Change-Id: I94968b0566d303c42c20a75eb04bdcfd453af16f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362905
Commit-Queue: Milica Selakovic <selakovic@google.com>
Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800041}

chrome/browser/password_check/android/password_check_manager.cc

@@ -55,8 +55,8 @@ void OrderCompromisedCredentials(
       });
 
   // By construction the phished credentials precede the leaked credentials in
-  // |credentials|. Now sort both groups by their creation date so that most recent
-  // compromises appear first in both lists.
+  // |credentials|. Now sort both groups by their creation date so that most
+  // recent compromises appear first in both lists.
   auto create_time_cmp = [](const auto& lhs, const auto& rhs) {
     return lhs.create_time > rhs.create_time;
   };
@@ -227,8 +227,7 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential(
 
   ui_credential.display_username = GetDisplayUsername(credential.username);
   ui_credential.has_script =
-      base::FeatureList::IsEnabled(
-          password_manager::features::kPasswordChangeInSettings) &&
+      ShouldOfferAutomaticPasswordChange() &&
       password_script_fetcher_->IsScriptAvailable(
           url::Origin::Create(credential.url.GetOrigin()));
 
@@ -300,16 +299,11 @@ bool PasswordCheckManager::CanUseAccountCheck() const {
 }
 
 bool PasswordCheckManager::AreScriptsRefreshed() const {
-  // Don't fetch scripts if password change is not enabled.
-  return are_scripts_refreshed_ ||
-         !base::FeatureList::IsEnabled(
-             password_manager::features::kPasswordChangeInSettings);
+  return are_scripts_refreshed_ || !ShouldOfferAutomaticPasswordChange();
 }
 
 void PasswordCheckManager::RefreshScripts() {
-  // Don't fetch scripts if password change is not enabled.
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kPasswordChangeInSettings)) {
+  if (!ShouldOfferAutomaticPasswordChange()) {
     return;
   }
 
@@ -333,3 +327,17 @@ void PasswordCheckManager::OnScriptsFetched() {
   if (was_start_requested_)
     StartCheck();
 }
+
+bool PasswordCheckManager::ShouldOfferAutomaticPasswordChange() const {
+  SyncState sync_state = password_manager_util::GetPasswordSyncState(
+      ProfileSyncServiceFactory::GetForProfile(profile_));
+
+  // Password change scripts are using password generation, so automatic
+  // password change should not be offered to non sync users.
+  if (sync_state == password_manager::NOT_SYNCING) {
+    return false;
+  }
+
+  return base::FeatureList::IsEnabled(
+      password_manager::features::kPasswordChangeInSettings);
+}

chrome/browser/password_check/android/password_check_manager.h

@@ -131,6 +131,11 @@ class PasswordCheckManager
   // in the account if the quota limit was reached.
   bool CanUseAccountCheck() const;
 
+  // Returns true if the automatic password change should be offered.
+  // It should be offered only to sync users and who have
+  // kPasswordChangeInSettings enabled.
+  bool ShouldOfferAutomaticPasswordChange() const;
+
   // Callback when PasswordScriptsFetcher's cache has been warmed up.
   void OnScriptsFetched();
 

chrome/browser/password_check/android/password_check_manager_unittest.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/password_manager/bulk_leak_check_service_factory.h"
 #include "chrome/browser/password_manager/password_scripts_fetcher_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/password_manager/core/browser/bulk_leak_check_service.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
@@ -27,6 +28,7 @@
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/test/browser_task_environment.h"
@@ -126,6 +128,14 @@ MockPasswordScriptsFetcher* CreateAndUseMockPasswordScriptsFetcher(
           }));
 }
 
+syncer::TestSyncService* CreateAndUseSyncService(Profile* profile) {
+  return ProfileSyncServiceFactory::GetInstance()
+      ->SetTestingSubclassFactoryAndUse(
+          profile, base::BindLambdaForTesting([](content::BrowserContext*) {
+            return std::make_unique<syncer::TestSyncService>();
+          }));
+}
+
 PasswordForm MakeSavedPassword(base::StringPiece signon_realm,
                                base::StringPiece username,
                                base::StringPiece password = kPassword1,
@@ -211,6 +221,7 @@ class PasswordCheckManagerTest : public testing::Test {
   MockPasswordScriptsFetcher& fetcher() { return *fetcher_; }
   PasswordCheckManager& manager() { return *manager_; }
   base::test::ScopedFeatureList& feature_list() { return feature_list_; }
+  syncer::TestSyncService& sync_service() { return *sync_service_; }
 
  private:
   content::BrowserTaskEnvironment task_env_;
@@ -221,6 +232,7 @@ class PasswordCheckManagerTest : public testing::Test {
                                        &profile_);
   scoped_refptr<TestPasswordStore> store_ =
       CreateAndUseTestPasswordStore(&profile_);
+  syncer::TestSyncService* sync_service_ = CreateAndUseSyncService(&profile_);
   NiceMock<MockPasswordCheckManagerObserver> mock_observer_;
   MockPasswordScriptsFetcher* fetcher_ =
       CreateAndUseMockPasswordScriptsFetcher(&profile_);
@@ -331,8 +343,34 @@ TEST_F(PasswordCheckManagerTest, DoesntRecordTimestampOfUnsuccessfulCheck) {
   EXPECT_EQ(0.0, manager().GetLastCheckTimestamp().ToDoubleT());
 }
 
-TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructWithPasswordScripts) {
+TEST_F(PasswordCheckManagerTest,
+       CorrectlyCreatesUIStructWithPasswordScriptsSyncOff) {
+  InitializeManager();
+  // Disable password sync
+  sync_service().SetActiveDataTypes(syncer::ModelTypeSet());
+  feature_list().InitAndEnableFeature(
+      password_manager::features::kPasswordChangeInSettings);
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
+  store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
+
+  RunUntilIdle();
+  EXPECT_CALL(fetcher(), RefreshScriptsIfNecessary).Times(0);
+
+  manager().RefreshScripts();
+
+  EXPECT_THAT(
+      manager().GetCompromisedCredentials(),
+      ElementsAre(ExpectCompromisedCredentialForUI(
+          base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
+          base::nullopt, "https://example.com/",
+          CompromiseTypeFlags::kCredentialLeaked, /*has_script=*/false)));
+}
+
+TEST_F(PasswordCheckManagerTest,
+       CorrectlyCreatesUIStructWithPasswordScriptsSyncOn) {
   InitializeManager();
+  // Enable password sync
+  sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
   feature_list().InitAndEnableFeature(
       password_manager::features::kPasswordChangeInSettings);
   store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));