[Passwords] Implement PasswordScriptsFetching flag

This CL also renames |has_script| to |has_startable_script| as the flag
is true only if the username is not empty and password sync is on.

Bug: 1132230
Change-Id: I27493c04629a61af788e20f4a6c81ddd542e590a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436339
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812050}

chrome/browser/password_check/android/internal/java/src/org/chromium/chrome/browser/password_check/PasswordCheckBridge.java

@@ -64,7 +64,7 @@ class PasswordCheckBridge {
     // TODO(crbug.com/1102025): Add call from native.
     void onCompromisedCredentialFound(String signonRealm, GURL origin, String username,
             String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
-            String associatedApp, long creationTime, boolean hasScript,
+            String associatedApp, long creationTime, boolean hasStartableScript,
             boolean hasAutoChangeButton) {
         assert signonRealm != null;
         assert displayOrigin != null;
@@ -72,7 +72,7 @@ class PasswordCheckBridge {
         assert password != null;
         mPasswordCheckObserver.onCompromisedCredentialFound(new CompromisedCredential(signonRealm,
                 origin, username, displayOrigin, displayUsername, password, passwordChangeUrl,
-                associatedApp, creationTime, true, false, hasScript, hasAutoChangeButton));
+                associatedApp, creationTime, true, false, hasStartableScript, hasAutoChangeButton));
     }
 
     @CalledByNative
@@ -99,11 +99,11 @@ class PasswordCheckBridge {
     private static void insertCredential(CompromisedCredential[] credentials, int index,
             String signonRealm, GURL origin, String username, String displayOrigin,
             String displayUsername, String password, String passwordChangeUrl, String associatedApp,
-            long creationTime, boolean leaked, boolean phished, boolean hasScript,
+            long creationTime, boolean leaked, boolean phished, boolean hasStartableScript,
             boolean hasAutoChangeButton) {
         credentials[index] = new CompromisedCredential(signonRealm, origin, username, displayOrigin,
                 displayUsername, password, passwordChangeUrl, associatedApp, creationTime, leaked,
-                phished, hasScript, hasAutoChangeButton);
+                phished, hasStartableScript, hasAutoChangeButton);
     }
 
     /**

chrome/browser/password_check/android/java/src/org/chromium/chrome/browser/password_check/CompromisedCredential.java

@@ -35,12 +35,12 @@ public class CompromisedCredential implements Parcelable {
                     in.readBooleanArray(boolArguments);
                     final boolean leaked = boolArguments[0];
                     final boolean phished = boolArguments[1];
-                    final boolean hasScript = boolArguments[2];
+                    final boolean hasStartableScript = boolArguments[2];
                     final boolean hasAutoChangeButton = boolArguments[3];
 
                     return new CompromisedCredential(signonRealm, origin, username, displayOrigin,
                             displayUsername, password, passwordChangeUrl, associatedApp,
-                            creationTime, leaked, phished, hasScript, hasAutoChangeButton);
+                            creationTime, leaked, phished, hasStartableScript, hasAutoChangeButton);
                 }
 
                 @Override
@@ -60,7 +60,7 @@ public class CompromisedCredential implements Parcelable {
     private final long mCreationTime;
     private final boolean mLeaked;
     private final boolean mPhished;
-    private final boolean mHasScript;
+    private final boolean mHasStartableScript;
     private final boolean mHasAutoChangeButton;
 
     /**
@@ -77,14 +77,15 @@ public class CompromisedCredential implements Parcelable {
      *        time at which the compromised credential was first found to be compromised during
      *        a check.
      * @param phished True iff the credential was entered on an unsafe site.
-     * @param hasScript True iff there is a script to automatically fix the credential.
+     * @param hasStartableScript True iff there is a script to automatically fix the credential and
+     *         it can be started (username is not empty, password sync is on, etc.)
      * @param hasAutoChangeButton True iff the button to automatically change the credential should
      *         be shown.
      */
     public CompromisedCredential(String signonRealm, GURL origin, String username,
             String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
             String associatedApp, long creationTime, boolean leaked, boolean phished,
-            boolean hasScript, boolean hasAutoChangeButton) {
+            boolean hasStartableScript, boolean hasAutoChangeButton) {
         assert origin != null : "Credential origin is null! Pass an empty one instead.";
         assert signonRealm != null;
         assert passwordChangeUrl != null : "Change URL may be empty but not null!";
@@ -93,8 +94,9 @@ public class CompromisedCredential implements Parcelable {
                 || !associatedApp.isEmpty()
             : "Change URL and app name may not be empty at the same time!";
         assert leaked || phished : "A compromised credential must be leaked or phished!";
-        assert hasScript
-                || !hasAutoChangeButton : "Auto change button cannot be shown without a script!";
+        assert hasStartableScript
+                || !hasAutoChangeButton
+            : "Auto change button cannot be shown without a script that can start!";
         mSignonRealm = signonRealm;
         mOrigin = origin;
         mUsername = username;
@@ -106,7 +108,7 @@ public class CompromisedCredential implements Parcelable {
         mCreationTime = creationTime;
         mLeaked = leaked;
         mPhished = phished;
-        mHasScript = hasScript;
+        mHasStartableScript = hasStartableScript;
         mHasAutoChangeButton = hasAutoChangeButton;
     }
 
@@ -147,8 +149,8 @@ public class CompromisedCredential implements Parcelable {
     public boolean isPhished() {
         return mPhished;
     }
-    public boolean hasScript() {
-        return mHasScript;
+    public boolean hasStartableScript() {
+        return mHasStartableScript;
     }
     public boolean hasAutoChangeButton() {
         return mHasAutoChangeButton;
@@ -166,7 +168,7 @@ public class CompromisedCredential implements Parcelable {
                 && mPasswordChangeUrl.equals(that.mPasswordChangeUrl)
                 && mAssociatedApp.equals(that.mAssociatedApp) && mCreationTime == that.mCreationTime
                 && mLeaked == that.mLeaked && mPhished == that.mPhished
-                && mHasScript == that.mHasScript
+                && mHasStartableScript == that.mHasStartableScript
                 && mHasAutoChangeButton == that.mHasAutoChangeButton;
     }
 
@@ -178,7 +180,7 @@ public class CompromisedCredential implements Parcelable {
                 + ", displayUsername='" + mDisplayUsername + '\'' + ", password='" + mPassword
                 + '\'' + ", passwordChangeUrl='" + mPasswordChangeUrl + '\'' + ", associatedApp='"
                 + mAssociatedApp + '\'' + ", creationTime=" + mCreationTime + ", leaked=" + mLeaked
-                + ", phished=" + mPhished + ", hasScript=" + mHasScript
+                + ", phished=" + mPhished + ", hasStartableScript=" + mHasStartableScript
                 + ", hasAutoChangeButton=" + mHasAutoChangeButton + '}';
     }
 
@@ -186,7 +188,7 @@ public class CompromisedCredential implements Parcelable {
     public int hashCode() {
         return Objects.hash(mSignonRealm, mOrigin.getPossiblyInvalidSpec(), mUsername,
                 mDisplayOrigin, mDisplayUsername, mPassword, mPasswordChangeUrl, mAssociatedApp,
-                mCreationTime, mLeaked, mPhished, mHasScript, mHasAutoChangeButton);
+                mCreationTime, mLeaked, mPhished, mHasStartableScript, mHasAutoChangeButton);
     }
 
     @Override
@@ -201,7 +203,7 @@ public class CompromisedCredential implements Parcelable {
         parcel.writeString(mAssociatedApp);
         parcel.writeLong(mCreationTime);
         parcel.writeBooleanArray(
-                new boolean[] {mLeaked, mPhished, mHasScript, mHasAutoChangeButton});
+                new boolean[] {mLeaked, mPhished, mHasStartableScript, mHasAutoChangeButton});
     }
 
     @Override

chrome/browser/password_check/android/java/src/org/chromium/chrome/browser/password_check/PasswordCheckMetricsRecorder.java

@@ -25,7 +25,7 @@ public final class PasswordCheckMetricsRecorder {
 
     public static void recordCheckResolutionAction(
             @PasswordCheckResolutionAction int action, CompromisedCredential credential) {
-        if (credential.hasScript()) {
+        if (credential.hasStartableScript()) {
             RecordHistogram.recordEnumeratedHistogram(
                     "PasswordManager.AutomaticChange.ForSitesWithScripts", action,
                     PasswordCheckResolutionAction.COUNT);

chrome/browser/password_check/android/password_check_bridge.cc

@@ -90,7 +90,7 @@ void PasswordCheckBridge::GetCompromisedCredentials(
          password_manager::InsecureCredentialTypeFlags::kCredentialLeaked),
         (credential.insecure_type ==
          password_manager::InsecureCredentialTypeFlags::kCredentialPhished),
-        credential.has_script, credential.has_auto_change_button);
+        credential.has_startable_script, credential.has_auto_change_button);
   }
 }
 

chrome/browser/password_check/android/password_check_manager.cc

@@ -72,7 +72,8 @@ PasswordCheckManager::PasswordCheckManager(Profile* profile, Observer* observer)
   // empty list.
   saved_passwords_presenter_.Init();
   insecure_credentials_manager_.Init();
-  if (!ShouldOfferAutomaticPasswordChange()) {
+
+  if (!ShouldFetchPasswordScripts()) {
     // Ensure that scripts are treated as initialized if they are unnecessary.
     FulfillPrecondition(kScriptsCachePrewarmed);
   }
@@ -224,13 +225,14 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential(
       credential.signon_realm);
 
   ui_credential.display_username = GetDisplayUsername(credential.username);
-  ui_credential.has_script =
-      !credential.username.empty() && ShouldOfferAutomaticPasswordChange() &&
+  ui_credential.has_startable_script =
+      !credential.username.empty() && ShouldFetchPasswordScripts() &&
       password_script_fetcher_->IsScriptAvailable(
           url::Origin::Create(credential.url.GetOrigin()));
-  // TODO(crbug.com/1132230): Implement separate logic for scripts fetching and
-  // auto buttons. For now, has_auto_change_button <=> has_script.
-  ui_credential.has_auto_change_button = ui_credential.has_script;
+  ui_credential.has_auto_change_button =
+      ui_credential.has_startable_script &&
+      base::FeatureList::IsEnabled(
+          password_manager::features::kPasswordChangeInSettings);
 
   if (facet.IsValidAndroidFacetURI()) {
     const PasswordForm& android_form =
@@ -305,7 +307,7 @@ bool PasswordCheckManager::AreScriptsRefreshed() const {
 }
 
 void PasswordCheckManager::RefreshScripts() {
-  if (!ShouldOfferAutomaticPasswordChange()) {
+  if (!ShouldFetchPasswordScripts()) {
     FulfillPrecondition(kScriptsCachePrewarmed);
     return;
   }
@@ -327,7 +329,7 @@ void PasswordCheckManager::OnScriptsFetched() {
   }
 }
 
-bool PasswordCheckManager::ShouldOfferAutomaticPasswordChange() const {
+bool PasswordCheckManager::ShouldFetchPasswordScripts() const {
   SyncState sync_state = password_manager_util::GetPasswordSyncState(
       ProfileSyncServiceFactory::GetForProfile(profile_));
 
@@ -338,7 +340,7 @@ bool PasswordCheckManager::ShouldOfferAutomaticPasswordChange() const {
   }
 
   return base::FeatureList::IsEnabled(
-      password_manager::features::kPasswordChangeInSettings);
+      password_manager::features::kPasswordScriptsFetching);
 }
 
 bool PasswordCheckManager::IsPreconditionFulfilled(

chrome/browser/password_check/android/password_check_manager.h

@@ -51,7 +51,7 @@ class PasswordCheckManager
     base::string16 display_origin;
     std::string package_name;
     std::string change_password_url;
-    bool has_script = false;
+    bool has_startable_script = false;
     bool has_auto_change_button = false;
   };
 
@@ -183,10 +183,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;
+  // Returns true if the password scripts fetching (kPasswordScriptsFetching) is
+  // enabled. To have precise metrics about user actions on credentials with
+  // scripts, scripts are fetched only for the users who can start a script,
+  // i.e. sync users.
+  bool ShouldFetchPasswordScripts() const;
 
   // Callback when PasswordScriptsFetcher's cache has been warmed up.
   void OnScriptsFetched();

chrome/browser/password_check/android/password_check_manager_unittest.cc

@@ -175,6 +175,7 @@ auto ExpectCompromisedCredentialForUI(
     const base::Optional<std::string>& package_name,
     const base::Optional<std::string>& change_password_url,
     InsecureCredentialTypeFlags insecure_type,
+    bool has_startable_script,
     bool has_auto_change_button) {
   auto package_name_field_matcher =
       package_name.has_value()
@@ -191,6 +192,8 @@ auto ExpectCompromisedCredentialForUI(
       Field(&CompromisedCredentialForUI::display_origin, display_origin),
       package_name_field_matcher, change_password_url_field_matcher,
       Field(&CompromisedCredentialForUI::insecure_type, insecure_type),
+      Field(&CompromisedCredentialForUI::has_startable_script,
+            has_startable_script),
       Field(&CompromisedCredentialForUI::has_auto_change_button,
             has_auto_change_button));
 }
@@ -291,8 +294,10 @@ TEST_F(PasswordCheckManagerTest,
        RunCheckAfterLastInitializationAutomaticChangeOn) {
   // Enable password sync
   sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
-  feature_list().InitAndEnableFeature(
-      password_manager::features::kPasswordChangeInSettings);
+  feature_list().InitWithFeatures(
+      {password_manager::features::kPasswordScriptsFetching,
+       password_manager::features::kPasswordChangeInSettings},
+      {});
   EXPECT_CALL(mock_observer(), OnPasswordCheckStatusChanged).Times(AtLeast(1));
   EXPECT_CALL(mock_observer(), OnSavedPasswordsFetched(1));
   store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
@@ -331,6 +336,7 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForSiteCredential) {
           base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
           base::nullopt, "https://example.com/",
           InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/false,
           /*has_auto_change_button=*/false)));
 }
 
@@ -355,11 +361,13 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) {
               base::ASCIIToUTF16(kUsername1),
               base::ASCIIToUTF16("App (com.example.app)"), "com.example.app",
               base::nullopt, InsecureCredentialTypeFlags::kCredentialLeaked,
+              /*has_startable_script=*/false,
               /*has_auto_change_button=*/false),
           ExpectCompromisedCredentialForUI(
               base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("Example App"),
               "com.example.app", base::nullopt,
               InsecureCredentialTypeFlags::kCredentialLeaked,
+              /*has_startable_script=*/false,
               /*has_auto_change_button=*/false)));
 }
 
@@ -394,22 +402,28 @@ TEST_F(PasswordCheckManagerTest,
   InitializeManager();
   // Disable password sync
   sync_service().SetActiveDataTypes(syncer::ModelTypeSet());
-  feature_list().InitAndEnableFeature(
-      password_manager::features::kPasswordChangeInSettings);
+  feature_list().InitWithFeatures(
+      {password_manager::features::kPasswordScriptsFetching,
+       password_manager::features::kPasswordChangeInSettings},
+      {});
   store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
   store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
 
   RunUntilIdle();
+  // To have precise metrics, scripts are not requested for users who cannot
+  // start a script, i.e. non-sync users.
   EXPECT_CALL(fetcher(), RefreshScriptsIfNecessary).Times(0);
 
   manager().RefreshScripts();
 
+  EXPECT_CALL(fetcher(), IsScriptAvailable).Times(0);
   EXPECT_THAT(
       manager().GetCompromisedCredentials(),
       ElementsAre(ExpectCompromisedCredentialForUI(
           base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
           base::nullopt, "https://example.com/",
           InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/false,
           /*has_auto_change_button=*/false)));
 }
 
@@ -418,8 +432,10 @@ TEST_F(PasswordCheckManagerTest,
   InitializeManager();
   // Enable password sync
   sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
-  feature_list().InitAndEnableFeature(
-      password_manager::features::kPasswordChangeInSettings);
+  feature_list().InitWithFeatures(
+      {password_manager::features::kPasswordScriptsFetching,
+       password_manager::features::kPasswordChangeInSettings},
+      {});
   store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
   store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
 
@@ -430,13 +446,14 @@ TEST_F(PasswordCheckManagerTest,
 
   manager().RefreshScripts();
 
-  EXPECT_CALL(fetcher(), IsScriptAvailable).WillRepeatedly(Return(true));
+  EXPECT_CALL(fetcher(), IsScriptAvailable).WillOnce(Return(true));
   EXPECT_THAT(
       manager().GetCompromisedCredentials(),
       ElementsAre(ExpectCompromisedCredentialForUI(
           base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
           base::nullopt, "https://example.com/",
           InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/true,
           /*has_auto_change_button=*/true)));
 }
 
@@ -445,8 +462,10 @@ TEST_F(PasswordCheckManagerTest,
   InitializeManager();
   // Enable password sync
   sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
-  feature_list().InitAndEnableFeature(
-      password_manager::features::kPasswordChangeInSettings);
+  feature_list().InitWithFeatures(
+      {password_manager::features::kPasswordScriptsFetching,
+       password_manager::features::kPasswordChangeInSettings},
+      {});
   store().AddLogin(MakeSavedPassword(kExampleCom, ""));
   store().AddCompromisedCredentials(MakeCompromised(kExampleCom, ""));
 
@@ -457,13 +476,80 @@ TEST_F(PasswordCheckManagerTest,
 
   manager().RefreshScripts();
 
-  // Scripts are not offered if username is empty.
+  // Particular script availability is not requested as a script cannot be
+  // started with an empty username.
+  EXPECT_CALL(fetcher(), IsScriptAvailable).Times(0);
   EXPECT_THAT(
       manager().GetCompromisedCredentials(),
       ElementsAre(ExpectCompromisedCredentialForUI(
           base::ASCIIToUTF16("No username"), base::ASCIIToUTF16("example.com"),
           base::nullopt, "https://example.com/",
           InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/false,
+          /*has_auto_change_button=*/false)));
+}
+
+TEST_F(PasswordCheckManagerTest,
+       CorrectlyCreatesUIStructWithScriptsFetchingButAutomaticChangeOff) {
+  InitializeManager();
+  // Enable password sync
+  sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
+  feature_list().InitWithFeatures(
+      /*enabled_features=*/{password_manager::features::
+                                kPasswordScriptsFetching},
+      /*disabled_features=*/{
+          password_manager::features::kPasswordChangeInSettings});
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
+  store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
+
+  RunUntilIdle();
+  EXPECT_CALL(fetcher(), RefreshScriptsIfNecessary)
+      .WillOnce(Invoke(
+          [](base::OnceClosure callback) { std::move(callback).Run(); }));
+
+  manager().RefreshScripts();
+
+  // A script is available but an auto change button is not shown because
+  // |kPasswordChangeInSettings| is disabled.
+  EXPECT_CALL(fetcher(), IsScriptAvailable).WillOnce(Return(true));
+  EXPECT_THAT(
+      manager().GetCompromisedCredentials(),
+      ElementsAre(ExpectCompromisedCredentialForUI(
+          base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
+          base::nullopt, "https://example.com/",
+          InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/true,
+          /*has_auto_change_button=*/false)));
+}
+
+TEST_F(PasswordCheckManagerTest,
+       CorrectlyCreatesUIStructWithScriptsFetchingButNoAvailableScript) {
+  InitializeManager();
+  // Enable password sync
+  sync_service().SetActiveDataTypes(syncer::ModelTypeSet(syncer::PASSWORDS));
+  feature_list().InitWithFeatures(
+      {password_manager::features::kPasswordScriptsFetching,
+       password_manager::features::kPasswordChangeInSettings},
+      {});
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
+  store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
+
+  RunUntilIdle();
+  EXPECT_CALL(fetcher(), RefreshScriptsIfNecessary)
+      .WillOnce(Invoke(
+          [](base::OnceClosure callback) { std::move(callback).Run(); }));
+
+  manager().RefreshScripts();
+
+  // A script is not available and therefore no auto change button is shown.
+  EXPECT_CALL(fetcher(), IsScriptAvailable).WillOnce(Return(false));
+  EXPECT_THAT(
+      manager().GetCompromisedCredentials(),
+      ElementsAre(ExpectCompromisedCredentialForUI(
+          base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
+          base::nullopt, "https://example.com/",
+          InsecureCredentialTypeFlags::kCredentialLeaked,
+          /*has_startable_script=*/false,
           /*has_auto_change_button=*/false)));
 }