[UPM] Fetch password settings when new password forms are seen.

This CL allows to fetch settings for android users enrolled into the Unified Password Manager experiment when password forms are loaded on the page. This allows to avoid applying outdated settings, if the user has changed them before in GMS. Bug: 1324229 Change-Id: I4e87f40c1454b5f3cbe88ca8d42601aa7c30b43f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640565 Reviewed-by: Ioana Pandele <ioanap@chromium.org> Commit-Queue: Maria Kazinova <kazinova@google.com> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/main@{#1002541}
chromium · May 12, 2022 · 7b54118 · 7b54118
1 parent 73baaa8
commit 7b54118
Show file tree

Hide file tree

Showing 15 changed files with 146 additions and 24 deletions.
diff --git a/chrome/browser/password_manager/android/password_manager_settings_service_android_impl.cc b/chrome/browser/password_manager/android/password_manager_settings_service_android_impl.cc
@@ -145,10 +145,13 @@ bool PasswordManagerSettingsServiceAndroidImpl::IsSettingEnabled(
   return android_pref->GetValue()->GetBool();
 }
 
-void PasswordManagerSettingsServiceAndroidImpl::OnChromeForegrounded() {
-  if (!IsPasswordSyncEnabled(sync_service_))
-    return;
+void PasswordManagerSettingsServiceAndroidImpl::RequestSettingsFromBackend() {
+  // Backend has settings data only if passwords are synced.
+  if (bridge_ && IsPasswordSyncEnabled(sync_service_))
+    FetchSettings();
+}
 
+void PasswordManagerSettingsServiceAndroidImpl::OnChromeForegrounded() {
   RequestSettingsFromBackend();
 }
 
@@ -233,16 +236,7 @@ void PasswordManagerSettingsServiceAndroidImpl::OnStateChanged(
   fetch_after_sync_status_change_in_progress_ = true;
   for (PasswordManagerSetting setting : kAllPasswordSettings)
     awaited_settings_.insert(setting);
-  RequestSettingsFromBackend();
-}
-
-void PasswordManagerSettingsServiceAndroidImpl::RequestSettingsFromBackend() {
-  for (PasswordManagerSetting setting : kAllPasswordSettings) {
-    bridge_->GetPasswordSettingValue(
-        PasswordSettingsUpdaterAndroidBridge::SyncingAccount(
-            pref_service_->GetString(::prefs::kGoogleServicesLastUsername)),
-        setting);
-  }
+  FetchSettings();
 }
 
 void PasswordManagerSettingsServiceAndroidImpl::UpdateSettingFetchState(
@@ -255,6 +249,16 @@ void PasswordManagerSettingsServiceAndroidImpl::UpdateSettingFetchState(
     fetch_after_sync_status_change_in_progress_ = false;
 }
 
+void PasswordManagerSettingsServiceAndroidImpl::FetchSettings() {
+  DCHECK(bridge_);
+  for (PasswordManagerSetting setting : kAllPasswordSettings) {
+    bridge_->GetPasswordSettingValue(
+        PasswordSettingsUpdaterAndroidBridge::SyncingAccount(
+            pref_service_->GetString(::prefs::kGoogleServicesLastUsername)),
+        setting);
+  }
+}
+
 void PasswordManagerSettingsServiceAndroidImpl::DumpChromePrefsIntoGMSPrefs() {
   for (PasswordManagerSetting setting : kAllPasswordSettings) {
     const PrefService::Preference* regular_pref =

diff --git a/chrome/browser/password_manager/android/password_manager_settings_service_android_impl.h b/chrome/browser/password_manager/android/password_manager_settings_service_android_impl.h
@@ -47,8 +47,10 @@ class PasswordManagerSettingsServiceAndroidImpl
 
   ~PasswordManagerSettingsServiceAndroidImpl() override;
 
+  // PasswordManagerSettingsService implementation
   bool IsSettingEnabled(
       password_manager::PasswordManagerSetting setting) override;
+  void RequestSettingsFromBackend() override;
 
  private:
   void OnChromeForegrounded();
@@ -71,14 +73,14 @@ class PasswordManagerSettingsServiceAndroidImpl
   // syncer::SyncServiceObserver implementation
   void OnStateChanged(syncer::SyncService* sync) override;
 
-  // Asynchronously fetch password settings.
-  void RequestSettingsFromBackend();
-
   // Updates information about the current setting fetch after receiving
   // a reply from the backend.
   void UpdateSettingFetchState(
       password_manager::PasswordManagerSetting received_setting);
 
+  // Asynchronously fetches settings from backend regardless of sync status.
+  void FetchSettings();
+
   // Copies the values of chrome prefs that have user-set values into the
   // GMS prefs.
   void DumpChromePrefsIntoGMSPrefs();

diff --git a/...owser/password_manager/android/password_manager_settings_service_android_impl_unittest.cc b/...owser/password_manager/android/password_manager_settings_service_android_impl_unittest.cc
@@ -65,6 +65,7 @@ class PasswordManagerSettingsServiceAndroidImplTest : public testing::Test {
   void SetSettingsSync(bool enabled);
 
   void AssertInitialMigrationDidntChangePrefs();
+  void ExpectSettingsRetrievalFromBackend(size_t times);
 
   void ExpectSettingsRetrievalFromBackend();
 
@@ -187,17 +188,19 @@ void PasswordManagerSettingsServiceAndroidImplTest::
 }
 
 void PasswordManagerSettingsServiceAndroidImplTest::
-    ExpectSettingsRetrievalFromBackend() {
+    ExpectSettingsRetrievalFromBackend(size_t times) {
   EXPECT_CALL(*bridge(),
               GetPasswordSettingValue(
                   Eq(PasswordSettingsUpdaterAndroidBridge::SyncingAccount(
                       kTestAccount)),
-                  Eq(PasswordManagerSetting::kOfferToSavePasswords)));
+                  Eq(PasswordManagerSetting::kOfferToSavePasswords)))
+      .Times(times);
   EXPECT_CALL(*bridge(),
               GetPasswordSettingValue(
                   Eq(PasswordSettingsUpdaterAndroidBridge::SyncingAccount(
                       kTestAccount)),
-                  Eq(PasswordManagerSetting::kAutoSignIn)));
+                  Eq(PasswordManagerSetting::kAutoSignIn)))
+      .Times(times);
 }
 
 void PasswordManagerSettingsServiceAndroidImplTest::RegisterPrefs() {
@@ -561,7 +564,7 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
       password_manager::prefs::kAutoSignInEnabledGMS));
 
   // Settings should be requested from GMS Core on sync state change.
-  ExpectSettingsRetrievalFromBackend();
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
   SetPasswordsSync(/*enabled=*/true);
   sync_service()->FireStateChanged();
 
@@ -587,7 +590,7 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
       password_manager::prefs::kAutoSignInEnabledGMS));
 
   // Settings should be requested from GMS Core on sync state change.
-  ExpectSettingsRetrievalFromBackend();
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
   SetPasswordsSync(/*enabled=*/true);
   sync_service()->FireStateChanged();
 
@@ -617,7 +620,7 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
       password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
 
   // Settings should be requested from GMS Core on sync state change.
-  ExpectSettingsRetrievalFromBackend();
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
   SetPasswordsSync(/*enabled=*/true);
   sync_service()->FireStateChanged();
 
@@ -646,7 +649,7 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
       password_manager::prefs::kAutoSignInEnabledGMS));
 
   // Settings should be requested from GMS Core on sync state change.
-  ExpectSettingsRetrievalFromBackend();
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
   SetPasswordsSync(/*enabled=*/false);
   sync_service()->FireStateChanged();
 
@@ -673,7 +676,7 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
       password_manager::prefs::kOfferToSavePasswordsEnabledGMS));
 
   // Settings should be requested from GMS Core on sync state change.
-  ExpectSettingsRetrievalFromBackend();
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
   SetPasswordsSync(/*enabled=*/false);
   sync_service()->FireStateChanged();
 
@@ -798,3 +801,19 @@ TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
   EXPECT_FALSE(settings_service()->IsSettingEnabled(
       PasswordManagerSetting::kAutoSignIn));
 }
+
+TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
+       SettingsAreRequestedFromBackendWhenPasswordSyncEnabled) {
+  InitializeSettingsService(/*password_sync_enabled=*/true,
+                            /*setting_sync_enabled=*/true);
+  ExpectSettingsRetrievalFromBackend(/*times=*/1);
+  settings_service()->RequestSettingsFromBackend();
+}
+
+TEST_F(PasswordManagerSettingsServiceAndroidImplTest,
+       SettingsAreNotRequestedFromBackendWhenPasswordSyncDisabled) {
+  InitializeSettingsService(/*password_sync_enabled=*/false,
+                            /*setting_sync_enabled=*/true);
+  ExpectSettingsRetrievalFromBackend(/*times=*/0);
+  settings_service()->RequestSettingsFromBackend();
+}
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -1031,6 +1031,18 @@ version_info::Channel ChromePasswordManagerClient::GetChannel() const {
   return chrome::GetChannel();
 }
 
+void ChromePasswordManagerClient::RefreshPasswordManagerSettingsIfNeeded()
+    const {
+#if BUILDFLAG(IS_ANDROID)
+  // Settings need to be requested for android clients enrolled into the unified
+  // password manager experiment.
+  if (!password_manager::features::UsesUnifiedPasswordManagerUi())
+    return;
+  PasswordManagerSettingsServiceFactory::GetForProfile(profile_)
+      ->RequestSettingsFromBackend();
+#endif
+}
+
 void ChromePasswordManagerClient::AutomaticGenerationAvailable(
     const autofill::password_generation::PasswordGenerationUIData& ui_data) {
   content::RenderFrameHost* rfh =

diff --git a/chrome/browser/password_manager/chrome_password_manager_client.h b/chrome/browser/password_manager/chrome_password_manager_client.h
@@ -264,6 +264,7 @@ class ChromePasswordManagerClient
   password_manager::WebAuthnCredentialsDelegate*
   GetWebAuthnCredentialsDelegate() override;
   version_info::Channel GetChannel() const override;
+  void RefreshPasswordManagerSettingsIfNeeded() const override;
 
   // autofill::mojom::PasswordGenerationDriver overrides.
   void AutomaticGenerationAvailable(

diff --git a/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc b/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
@@ -47,6 +47,7 @@
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/common/credential_manager_types.h"
+#include "components/password_manager/core/common/password_manager_feature_variations_android.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/safe_browsing/buildflags.h"
@@ -890,6 +891,21 @@ TEST_F(ChromePasswordManagerClientTest, MissingUIDelegate) {
   client->HideManualFallbackForSaving();
 }
 
+TEST_F(ChromePasswordManagerClientTest,
+       RefreshPasswordManagerSettingsIfNeededUPMDisabled) {
+#if BUILDFLAG(IS_ANDROID)
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(
+      password_manager::features::kUnifiedPasswordManagerAndroid);
+#endif
+
+  MockPasswordManagerSettingsService* settings_service =
+      static_cast<MockPasswordManagerSettingsService*>(
+          PasswordManagerSettingsServiceFactory::GetForProfile(profile()));
+  EXPECT_CALL(*settings_service, RequestSettingsFromBackend).Times(0);
+  GetClient()->RefreshPasswordManagerSettingsIfNeeded();
+}
+
 #if BUILDFLAG(IS_ANDROID)
 class ChromePasswordManagerClientAndroidTest
     : public ChromePasswordManagerClientTest {
@@ -1118,4 +1134,21 @@ TEST_F(ChromePasswordManagerClientAndroidTest,
   uma_recorder.ExpectTotalCount(
       "PasswordManager.TouchToFill.TimeToSuccessfulLogin", 1);
 }
+
+TEST_F(ChromePasswordManagerClientAndroidTest,
+       RefreshPasswordManagerSettingsIfNeededUPMFeatureEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  const std::map<std::string, std::string> params = {
+      {"stage", base::NumberToString(static_cast<int>(
+                    password_manager::features::UpmExperimentVariation::
+                        kEnableForSyncingUsers))}};
+  feature_list.InitAndEnableFeatureWithParameters(
+      password_manager::features::kUnifiedPasswordManagerAndroid, params);
+
+  MockPasswordManagerSettingsService* settings_service =
+      static_cast<MockPasswordManagerSettingsService*>(
+          PasswordManagerSettingsServiceFactory::GetForProfile(profile()));
+  EXPECT_CALL(*settings_service, RequestSettingsFromBackend);
+  GetClient()->RefreshPasswordManagerSettingsIfNeeded();
+}
 #endif  //  BUILDFLAG(IS_ANDROID)
diff --git a/chrome/browser/password_manager/password_manager_settings_service_impl.cc b/chrome/browser/password_manager/password_manager_settings_service_impl.cc
@@ -24,3 +24,8 @@ bool PasswordManagerSettingsServiceImpl::IsSettingEnabled(
           password_manager::prefs::kCredentialsEnableAutosignin);
   }
 }
+
+void PasswordManagerSettingsServiceImpl::RequestSettingsFromBackend() {
+  // This method is invoked only on android when UPM is enabled.
+  NOTREACHED();
+}
diff --git a/chrome/browser/password_manager/password_manager_settings_service_impl.h b/chrome/browser/password_manager/password_manager_settings_service_impl.h
@@ -29,6 +29,8 @@ class PasswordManagerSettingsServiceImpl
   bool IsSettingEnabled(
       password_manager::PasswordManagerSetting setting) override;
 
+  void RequestSettingsFromBackend() override;
+
  private:
   raw_ptr<PrefService> pref_service_ = nullptr;
 };

diff --git a/components/password_manager/core/browser/mock_password_manager_settings_service.h b/components/password_manager/core/browser/mock_password_manager_settings_service.h
@@ -19,6 +19,7 @@ class MockPasswordManagerSettingsService
               IsSettingEnabled,
               (password_manager::PasswordManagerSetting),
               (override));
+  MOCK_METHOD(void, RequestSettingsFromBackend, (), (override));
 };
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_MOCK_PASSWORD_MANAGER_SETTINGS_SERVICE_H_
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
@@ -564,6 +564,8 @@ void PasswordManager::HideManualFallbackForSaving() {
 void PasswordManager::OnPasswordFormsParsed(
     PasswordManagerDriver* driver,
     const std::vector<FormData>& form_data) {
+  if (NewFormsParsed(driver, form_data))
+    client_->RefreshPasswordManagerSettingsIfNeeded();
   CreatePendingLoginManagers(driver, form_data);
 
   PasswordGenerationFrameHelper* password_generation_manager =
@@ -1359,6 +1361,13 @@ void PasswordManager::ShowManualFallbackForSaving(
   }
 }
 
+bool PasswordManager::NewFormsParsed(PasswordManagerDriver* driver,
+                                     const std::vector<FormData>& form_data) {
+  return base::ranges::any_of(form_data, [driver, this](const FormData& form) {
+    return !GetMatchedManager(driver, form.unique_renderer_id);
+  });
+}
+
 void PasswordManager::ResetPendingCredentials() {
   for (auto& form_manager : form_managers_)
     form_manager->ResetState();

diff --git a/components/password_manager/core/browser/password_manager.h b/components/password_manager/core/browser/password_manager.h
@@ -328,6 +328,11 @@ class PasswordManager : public PasswordManagerInterface {
   void ShowManualFallbackForSaving(PasswordFormManager* form_manager,
                                    const autofill::FormData& form_data);
 
+  // Returns true if |form_data| contains forms that are parsed for the first
+  // time and have no dedicated PasswordFormsManagers yet.
+  bool NewFormsParsed(PasswordManagerDriver* driver,
+                      const std::vector<autofill::FormData>& form_data);
+
   // Returns the timeout for the disabling Password Manager's prompts.
   base::TimeDelta GetTimeoutForDisablingPrompts();
 

diff --git a/components/password_manager/core/browser/password_manager_client.cc b/components/password_manager/core/browser/password_manager_client.cc
@@ -167,4 +167,8 @@ version_info::Channel PasswordManagerClient::GetChannel() const {
   return version_info::Channel::UNKNOWN;
 }
 
+void PasswordManagerClient::RefreshPasswordManagerSettingsIfNeeded() const {
+  // For most implementations settings do not need to be refreshed.
+}
+
 }  // namespace password_manager
diff --git a/components/password_manager/core/browser/password_manager_client.h b/components/password_manager/core/browser/password_manager_client.h
@@ -477,6 +477,9 @@ class PasswordManagerClient {
 
   // Returns the Chrome channel for the installation.
   virtual version_info::Channel GetChannel() const;
+
+  // Refreshes password manager settings stored in prefs.
+  virtual void RefreshPasswordManagerSettingsIfNeeded() const;
 };
 
 }  // namespace password_manager

diff --git a/components/password_manager/core/browser/password_manager_settings_service.h b/components/password_manager/core/browser/password_manager_settings_service.h
@@ -15,6 +15,9 @@ class PasswordManagerSettingsService : public KeyedService {
   virtual bool IsSettingEnabled(
       password_manager::PasswordManagerSetting setting) = 0;
 
+  // Asynchronously fetch password settings from backend.
+  virtual void RequestSettingsFromBackend() = 0;
+
  protected:
   ~PasswordManagerSettingsService() override = default;
 };

diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -221,6 +221,10 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
               (),
               (const, override));
   MOCK_METHOD(version_info::Channel, GetChannel, (), (const override));
+  MOCK_METHOD(void,
+              RefreshPasswordManagerSettingsIfNeeded,
+              (),
+              (const, override));
   MOCK_METHOD(WebAuthnCredentialsDelegate*,
               GetWebAuthnCredentialsDelegate,
               (),
@@ -4456,6 +4460,21 @@ TEST_P(PasswordManagerTest, StartLeakCheckWhenForUsernameNotMuted) {
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 }
 
+TEST_P(PasswordManagerTest, ParsingNewFormsTriggersSettingFetch) {
+  // Check that seeing the form for the first time triggers fetching settings.
+  std::vector<FormData> observed;
+  observed.emplace_back(MakeSignUpFormData());
+  EXPECT_CALL(client_, RefreshPasswordManagerSettingsIfNeeded);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+
+  // Check that settings are not refetched if the already seen form dynamically
+  // changes and is parsed again.
+  FormFieldData new_field;
+  observed[0].fields.push_back(new_field);
+  EXPECT_CALL(client_, RefreshPasswordManagerSettingsIfNeeded).Times(0);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+}
+
 INSTANTIATE_TEST_SUITE_P(, PasswordManagerTest, testing::Bool());
 
 }  // namespace password_manager