From 7e7925c11daa299d8e4a08ecf5fd62821e0c090f Mon Sep 17 00:00:00 2001 From: Ioana Pandele Date: Fri, 8 Apr 2022 10:24:06 +0000 Subject: [PATCH] [PwdSettings] Add new prefs for password manager settings Bug: 1289700 Change-Id: If64d22f7b5fa13da950e1401927d1b51da5b1f0f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3562944 Reviewed-by: Vasilii Sukhanov Reviewed-by: John Wu Commit-Queue: Ioana Pandele Cr-Commit-Position: refs/heads/main@{#990328} --- .../chrome_password_manager_client.cc | 19 +- .../chrome_password_manager_client.h | 7 +- .../ui/autofill/chrome_autofill_client.cc | 5 +- .../core/browser/credential_manager_impl.cc | 9 +- .../core/browser/credential_manager_impl.h | 3 - .../core/browser/password_manager.cc | 2 + .../core/browser/password_manager_client.h | 4 + .../browser/password_manager_client_helper.cc | 5 +- .../core/browser/password_manager_util.cc | 55 ++++++ .../core/browser/password_manager_util.h | 12 ++ .../browser/password_manager_util_unittest.cc | 166 ++++++++++++++++++ .../browser/stub_password_manager_client.cc | 4 + .../browser/stub_password_manager_client.h | 2 + .../common/password_manager_pref_names.cc | 4 + .../core/common/password_manager_pref_names.h | 28 ++- .../ios_chrome_password_manager_client.h | 2 + .../ios_chrome_password_manager_client.mm | 19 +- .../web_view_password_manager_client.h | 2 +- .../web_view_password_manager_client.mm | 9 +- 19 files changed, 321 insertions(+), 36 deletions(-) diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc index 6250fe1e66f6de..db7892ae93f0e5 100644 --- a/chrome/browser/password_manager/chrome_password_manager_client.cc +++ b/chrome/browser/password_manager/chrome_password_manager_client.cc @@ -64,6 +64,7 @@ #include "components/password_manager/core/browser/hsts_query.h" #include "components/password_manager/core/browser/http_auth_manager.h" #include "components/password_manager/core/browser/http_auth_manager_impl.h" +#include "components/password_manager/core/browser/password_bubble_experiment.h" #include "components/password_manager/core/browser/password_change_success_tracker.h" #include "components/password_manager/core/browser/password_form.h" #include "components/password_manager/core/browser/password_form_manager_for_ui.h" @@ -187,7 +188,7 @@ namespace { static const char kPasswordBreachEntryTrigger[] = "PASSWORD_ENTRY"; #endif -const syncer::SyncService* GetSyncService(Profile* profile) { +const syncer::SyncService* GetSyncServiceForProfile(Profile* profile) { if (SyncServiceFactory::HasSyncService(profile)) return SyncServiceFactory::GetForProfile(profile); return nullptr; @@ -262,7 +263,9 @@ bool ChromePasswordManagerClient::IsSavingAndFillingEnabled( // page, and there is no API to access (or dismiss) UI bubbles/infobars. return false; } - return *saving_passwords_enabled_ && !IsIncognito() && IsFillingEnabled(url); + return password_manager_util::IsSavingPasswordsEnabled(GetPrefs(), + GetSyncService()) && + !IsIncognito() && IsFillingEnabled(url); } bool ChromePasswordManagerClient::IsFillingEnabled(const GURL& url) const { @@ -617,6 +620,10 @@ PrefService* ChromePasswordManagerClient::GetPrefs() const { return profile_->GetPrefs(); } +const syncer::SyncService* ChromePasswordManagerClient::GetSyncService() const { + return GetSyncServiceForProfile(profile_); +} + password_manager::PasswordStoreInterface* ChromePasswordManagerClient::GetProfilePasswordStore() const { // Always use EXPLICIT_ACCESS as the password manager checks IsIncognito @@ -1232,7 +1239,7 @@ ChromePasswordManagerClient::ChromePasswordManagerClient( #if BUILDFLAG(ENABLE_DICE_SUPPORT) credentials_filter_( this, - base::BindRepeating(&GetSyncService, profile_), + base::BindRepeating(&GetSyncServiceForProfile, profile_), DiceWebSigninInterceptorFactory::GetForProfile(profile_)), account_storage_auth_helper_( IdentityManagerFactory::GetForProfile(profile_), @@ -1245,7 +1252,9 @@ ChromePasswordManagerClient::ChromePasswordManagerClient( }, web_contents)), #else - credentials_filter_(this, base::BindRepeating(&GetSyncService, profile_)), + credentials_filter_( + this, + base::BindRepeating(&GetSyncServiceForProfile, profile_)), #endif helper_(this) { ContentPasswordManagerDriverFactory::CreateForWebContents(web_contents, this, @@ -1259,8 +1268,6 @@ ChromePasswordManagerClient::ChromePasswordManagerClient( &ContentPasswordManagerDriverFactory::RequestSendLoggingAvailability, base::Unretained(driver_factory_))); - saving_passwords_enabled_.Init( - password_manager::prefs::kCredentialsEnableService, GetPrefs()); driver_factory_->RequestSendLoggingAvailability(); auto* autofill_assistant_manager = diff --git a/chrome/browser/password_manager/chrome_password_manager_client.h b/chrome/browser/password_manager/chrome_password_manager_client.h index 68aa1648c376ca..cb1b2172eb6bae 100644 --- a/chrome/browser/password_manager/chrome_password_manager_client.h +++ b/chrome/browser/password_manager/chrome_password_manager_client.h @@ -36,6 +36,7 @@ #include "components/prefs/pref_member.h" #include "components/safe_browsing/buildflags.h" #include "components/signin/public/base/signin_buildflags.h" +#include "components/sync/driver/sync_service.h" #include "content/public/browser/render_frame_host_receiver_set.h" #include "content/public/browser/render_widget_host.h" #include "content/public/browser/web_contents_observer.h" @@ -175,6 +176,7 @@ class ChromePasswordManagerClient base::OnceCallback reauth_callback) override; void TriggerSignIn(signin_metrics::AccessPoint access_point) override; PrefService* GetPrefs() const override; + const syncer::SyncService* GetSyncService() const override; password_manager::PasswordStoreInterface* GetProfilePasswordStore() const override; password_manager::PasswordStoreInterface* GetAccountPasswordStore() @@ -406,11 +408,6 @@ class ChromePasswordManagerClient // Controls the popup base::WeakPtr popup_controller_; - // Set to false to disable password saving (will no longer ask if you - // want to save passwords). There is no pref for disabling filling at this - // point. - BooleanPrefMember saving_passwords_enabled_; - #if BUILDFLAG(ENABLE_DICE_SUPPORT) // MultiProfileCredentialsFilter requires DICE support. const MultiProfileCredentialsFilter credentials_filter_; diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.cc b/chrome/browser/ui/autofill/chrome_autofill_client.cc index a4b37aac5213a9..35cb3fac4e43d7 100644 --- a/chrome/browser/ui/autofill/chrome_autofill_client.cc +++ b/chrome/browser/ui/autofill/chrome_autofill_client.cc @@ -61,6 +61,7 @@ #include "components/autofill_assistant/browser/public/runtime_manager.h" #include "components/password_manager/content/browser/content_password_manager_driver.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h" +#include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_requirements_service.h" #include "components/password_manager/core/common/password_manager_pref_names.h" #include "components/prefs/pref_service.h" @@ -867,8 +868,8 @@ bool ChromeAutofillClient::IsAutocompleteEnabled() { } bool ChromeAutofillClient::IsPasswordManagerEnabled() { - return GetPrefs()->GetBoolean( - password_manager::prefs::kCredentialsEnableService); + return password_manager_util::IsSavingPasswordsEnabled(GetPrefs(), + GetSyncService()); } void ChromeAutofillClient::PropagateAutofillPredictions( diff --git a/components/password_manager/core/browser/credential_manager_impl.cc b/components/password_manager/core/browser/credential_manager_impl.cc index 7f8f10f71a5153..3edcffb238ba58 100644 --- a/components/password_manager/core/browser/credential_manager_impl.cc +++ b/components/password_manager/core/browser/credential_manager_impl.cc @@ -28,10 +28,7 @@ void RunGetCallback(GetCallback callback, const CredentialInfo& info) { } // namespace CredentialManagerImpl::CredentialManagerImpl(PasswordManagerClient* client) - : client_(client), leak_delegate_(client) { - auto_signin_enabled_.Init(prefs::kCredentialsEnableAutosignin, - client_->GetPrefs()); -} + : client_(client), leak_delegate_(client) {} CredentialManagerImpl::~CredentialManagerImpl() = default; @@ -166,7 +163,9 @@ void CredentialManagerImpl::Get(CredentialMediationRequirement mediation, } bool CredentialManagerImpl::IsZeroClickAllowed() const { - return *auto_signin_enabled_ && !client_->IsIncognito(); + return password_manager_util::IsAutoSignInEnabled( + client_->GetPrefs(), client_->GetSyncService()) && + !client_->IsIncognito(); } PasswordFormDigest CredentialManagerImpl::GetSynthesizedFormForOrigin() const { diff --git a/components/password_manager/core/browser/credential_manager_impl.h b/components/password_manager/core/browser/credential_manager_impl.h index 70ea6a70086b8e..cf6bfe689741f2 100644 --- a/components/password_manager/core/browser/credential_manager_impl.h +++ b/components/password_manager/core/browser/credential_manager_impl.h @@ -82,9 +82,6 @@ class CredentialManagerImpl raw_ptr client_; - // Set to false to disable automatic signing in. - BooleanPrefMember auto_signin_enabled_; - // Used to store or update a credential. Calls OnProvisionalSaveComplete // on this delegate. std::unique_ptr form_manager_; diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc index 5ba0c0b4fe3cdc..56da4a65e92994 100644 --- a/components/password_manager/core/browser/password_manager.cc +++ b/components/password_manager/core/browser/password_manager.cc @@ -253,6 +253,8 @@ void PasswordManager::RegisterProfilePrefs( prefs::kPasswordDismissCompromisedAlertEnabled, true, user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); #if BUILDFLAG(IS_ANDROID) + registry->RegisterBooleanPref(prefs::kOfferToSavePasswordsEnabledGMS, true); + registry->RegisterBooleanPref(prefs::kAutoSignInEnabledGMS, true); registry->RegisterIntegerPref( prefs::kCurrentMigrationVersionToGoogleMobileServices, 0); registry->RegisterDoublePref(prefs::kTimeOfLastMigrationAttempt, 0.0); diff --git a/components/password_manager/core/browser/password_manager_client.h b/components/password_manager/core/browser/password_manager_client.h index 26ec1566983fbc..24d3f18e0ea600 100644 --- a/components/password_manager/core/browser/password_manager_client.h +++ b/components/password_manager/core/browser/password_manager_client.h @@ -28,6 +28,7 @@ #include "components/password_manager/core/browser/webauthn_credentials_delegate.h" #include "components/profile_metrics/browser_profile_type.h" #include "components/safe_browsing/buildflags.h" +#include "components/sync/driver/sync_service.h" #include "net/cert/cert_status_flags.h" #include "services/metrics/public/cpp/ukm_recorder.h" @@ -276,6 +277,9 @@ class PasswordManagerClient { // Gets prefs associated with this embedder. virtual PrefService* GetPrefs() const = 0; + // Gets the sync service associated with this client. + virtual const syncer::SyncService* GetSyncService() const = 0; + // Returns the profile PasswordStore associated with this instance. virtual PasswordStoreInterface* GetProfilePasswordStore() const = 0; diff --git a/components/password_manager/core/browser/password_manager_client_helper.cc b/components/password_manager/core/browser/password_manager_client_helper.cc index d06396765dbddf..9a1b804df35269 100644 --- a/components/password_manager/core/browser/password_manager_client_helper.cc +++ b/components/password_manager/core/browser/password_manager_client_helper.cc @@ -9,6 +9,7 @@ #include "components/password_manager/core/browser/password_feature_manager.h" #include "components/password_manager/core/browser/password_form_manager_for_ui.h" #include "components/password_manager/core/browser/password_manager.h" +#include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_sync_util.h" #include "components/password_manager/core/common/password_manager_pref_names.h" #include "components/prefs/pref_service.h" @@ -98,8 +99,8 @@ bool PasswordManagerClientHelper::ShouldPromptToEnableAutoSignIn() const { return password_bubble_experiment:: ShouldShowAutoSignInPromptFirstRunExperience( delegate_->GetPrefs()) && - delegate_->GetPrefs()->GetBoolean( - password_manager::prefs::kCredentialsEnableAutosignin) && + password_manager_util::IsAutoSignInEnabled( + delegate_->GetPrefs(), delegate_->GetSyncService()) && !delegate_->IsIncognito(); } diff --git a/components/password_manager/core/browser/password_manager_util.cc b/components/password_manager/core/browser/password_manager_util.cc index 3660d23f320129..19017bfb6bcd92 100644 --- a/components/password_manager/core/browser/password_manager_util.cc +++ b/components/password_manager/core/browser/password_manager_util.cc @@ -28,6 +28,7 @@ #include "components/password_manager/core/browser/credentials_cleaner_runner.h" #include "components/password_manager/core/browser/http_credentials_cleaner.h" #include "components/password_manager/core/browser/old_google_credentials_cleaner.h" +#include "components/password_manager/core/browser/password_bubble_experiment.h" #include "components/password_manager/core/browser/password_feature_manager.h" #include "components/password_manager/core/browser/password_form.h" #include "components/password_manager/core/browser/password_generation_frame_helper.h" @@ -69,6 +70,60 @@ bool IsBetterMatch(const PasswordForm* lhs, const PasswordForm* rhs) { } // namespace +bool IsSavingPasswordsEnabled(const PrefService* pref_service, + const syncer::SyncService* sync_service) { + DCHECK(pref_service); + const PrefService::Preference* save_passwords_pref = + pref_service->FindPreference( + password_manager::prefs::kCredentialsEnableService); + DCHECK(save_passwords_pref); +#if BUILDFLAG(IS_ANDROID) + if (!password_bubble_experiment::HasChosenToSyncPasswords(sync_service)) { + return save_passwords_pref->GetValue()->GetBool(); + } + + if (!password_manager::features::UsesUnifiedPasswordManagerUi()) { + return save_passwords_pref->GetValue()->GetBool(); + } + + if (save_passwords_pref->IsManaged()) { + return save_passwords_pref->GetValue()->GetBool(); + } + + return pref_service->GetBoolean( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS); +#else + return save_passwords_pref->GetValue()->GetBool(); +#endif +} + +bool IsAutoSignInEnabled(const PrefService* pref_service, + const syncer::SyncService* sync_service) { + DCHECK(pref_service); + const PrefService::Preference* auto_sign_in_pref = + pref_service->FindPreference( + password_manager::prefs::kCredentialsEnableAutosignin); + DCHECK(auto_sign_in_pref); +#if BUILDFLAG(IS_ANDROID) + if (!password_bubble_experiment::HasChosenToSyncPasswords(sync_service)) { + return auto_sign_in_pref->GetValue()->GetBool(); + } + + if (!password_manager::features::UsesUnifiedPasswordManagerUi()) { + return auto_sign_in_pref->GetValue()->GetBool(); + } + + if (auto_sign_in_pref->IsManaged()) { + return auto_sign_in_pref->GetValue()->GetBool(); + } + + return pref_service->GetBoolean( + password_manager::prefs::kAutoSignInEnabledGMS); +#else + return auto_sign_in_pref->GetValue()->GetBool(); +#endif +} + // Update |credential| to reflect usage. void UpdateMetadataForUsage(PasswordForm* credential) { ++credential->times_used; diff --git a/components/password_manager/core/browser/password_manager_util.h b/components/password_manager/core/browser/password_manager_util.h index 05325c32443c80..e0f46a4d339929 100644 --- a/components/password_manager/core/browser/password_manager_util.h +++ b/components/password_manager/core/browser/password_manager_util.h @@ -53,6 +53,18 @@ enum class GetLoginMatchType { kPSL, }; +// Checks if saving passwords is enabled. On Android, it ensures that the +// correct pref is checked on Android, which depends on the unified password +// manager status. +bool IsSavingPasswordsEnabled(const PrefService* pref_service, + const syncer::SyncService* sync_service); + +// Checks if auto sign in is enabled. On Android, it ensures that the +// correct pref is checked on Android, which depends on the unified password +// manager status. +bool IsAutoSignInEnabled(const PrefService* pref_service, + const syncer::SyncService* sync_service); + // Update |credential| to reflect usage. void UpdateMetadataForUsage(password_manager::PasswordForm* credential); diff --git a/components/password_manager/core/browser/password_manager_util_unittest.cc b/components/password_manager/core/browser/password_manager_util_unittest.cc index fab480349bc708..1c1b007f564380 100644 --- a/components/password_manager/core/browser/password_manager_util_unittest.cc +++ b/components/password_manager/core/browser/password_manager_util_unittest.cc @@ -11,6 +11,7 @@ #include "base/callback_helpers.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/scoped_feature_list.h" #include "base/time/time.h" #include "base/values.h" #include "build/build_config.h" @@ -24,7 +25,12 @@ #include "components/password_manager/core/browser/stub_password_manager_client.h" #include "components/password_manager/core/browser/test_password_store.h" #include "components/password_manager/core/common/password_manager_features.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/signin/public/base/signin_metrics.h" +#include "components/sync/base/user_selectable_type.h" +#include "components/sync/driver/test_sync_service.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -287,6 +293,166 @@ using testing::_; using testing::DoAll; using testing::Return; +class PasswordManagerUtilTest : public testing::Test { + public: + PasswordManagerUtilTest() { + pref_service_.registry()->RegisterBooleanPref( + password_manager::prefs::kCredentialsEnableService, true); + pref_service_.registry()->RegisterBooleanPref( + password_manager::prefs::kCredentialsEnableAutosignin, true); +#if BUILDFLAG(IS_ANDROID) + pref_service_.registry()->RegisterBooleanPref( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS, true); + pref_service_.registry()->RegisterBooleanPref( + password_manager::prefs::kAutoSignInEnabledGMS, true); +#endif + } + + protected: + TestingPrefServiceSimple pref_service_; + syncer::TestSyncService sync_service_; +}; + +#if BUILDFLAG(IS_ANDROID) +TEST_F(PasswordManagerUtilTest, SavePasswordsSettingNoUPM) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndDisableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + + pref_service_.SetUserPref(password_manager::prefs::kCredentialsEnableService, + base::Value(true)); + pref_service_.SetUserPref( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS, + base::Value(false)); + EXPECT_TRUE(password_manager_util::IsSavingPasswordsEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, SavePasswordsSettingNotSyncing) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes(false, {}); + pref_service_.SetUserPref(password_manager::prefs::kCredentialsEnableService, + base::Value(true)); + pref_service_.SetUserPref( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS, + base::Value(false)); + EXPECT_TRUE(password_manager_util::IsSavingPasswordsEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, SavePasswordsSettingSyncingUPMManaged) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + pref_service_.SetManagedPref( + password_manager::prefs::kCredentialsEnableService, base::Value(false)); + pref_service_.SetUserPref( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS, + base::Value(true)); + EXPECT_FALSE(password_manager_util::IsSavingPasswordsEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, SavePasswordsSettingSyncingUPMNotManaged) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + pref_service_.SetUserPref(password_manager::prefs::kCredentialsEnableService, + base::Value(true)); + pref_service_.SetUserPref( + password_manager::prefs::kOfferToSavePasswordsEnabledGMS, + base::Value(false)); + EXPECT_FALSE(password_manager_util::IsSavingPasswordsEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, AutoSignInSettingNoUPM) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndDisableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + + pref_service_.SetUserPref( + password_manager::prefs::kCredentialsEnableAutosignin, base::Value(true)); + pref_service_.SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS, + base::Value(false)); + EXPECT_TRUE(password_manager_util::IsAutoSignInEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, AutoSignInSettingNotSyncing) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes(false, {}); + pref_service_.SetUserPref( + password_manager::prefs::kCredentialsEnableAutosignin, base::Value(true)); + pref_service_.SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS, + base::Value(false)); + EXPECT_TRUE(password_manager_util::IsAutoSignInEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, AutoSignInSettingSyncingUPMManaged) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + pref_service_.SetManagedPref( + password_manager::prefs::kCredentialsEnableAutosignin, + base::Value(false)); + pref_service_.SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS, + base::Value(true)); + EXPECT_FALSE(password_manager_util::IsAutoSignInEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, AutoSignInSettingSyncingUPMNotManaged) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kUnifiedPasswordManagerAndroid); + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + pref_service_.SetUserPref( + password_manager::prefs::kCredentialsEnableAutosignin, base::Value(true)); + pref_service_.SetUserPref(password_manager::prefs::kAutoSignInEnabledGMS, + base::Value(false)); + EXPECT_FALSE(password_manager_util::IsAutoSignInEnabled(&pref_service_, + &sync_service_)); +} +#else // !BUILDFLAG(IS_ANDROID) +TEST_F(PasswordManagerUtilTest, SavePasswordsSettingNotAndroid) { + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + + pref_service_.SetUserPref(password_manager::prefs::kCredentialsEnableService, + base::Value(false)); + EXPECT_FALSE(password_manager_util::IsSavingPasswordsEnabled(&pref_service_, + &sync_service_)); +} + +TEST_F(PasswordManagerUtilTest, AutoSignInSettingNotAndroid) { + sync_service_.GetUserSettings()->SetSelectedTypes( + false, {syncer::UserSelectableType::kPasswords}); + + pref_service_.SetUserPref( + password_manager::prefs::kCredentialsEnableAutosignin, + base::Value(false)); + EXPECT_FALSE(password_manager_util::IsAutoSignInEnabled(&pref_service_, + &sync_service_)); +} +#endif // BUILDFLAG(IS_ANDROID) + TEST(PasswordManagerUtil, TrimUsernameOnlyCredentials) { std::vector> forms; std::vector> expected_forms; diff --git a/components/password_manager/core/browser/stub_password_manager_client.cc b/components/password_manager/core/browser/stub_password_manager_client.cc index af829d6a13f9fa..7c51cf7238d3ee 100644 --- a/components/password_manager/core/browser/stub_password_manager_client.cc +++ b/components/password_manager/core/browser/stub_password_manager_client.cc @@ -66,6 +66,10 @@ PrefService* StubPasswordManagerClient::GetPrefs() const { return nullptr; } +const syncer::SyncService* StubPasswordManagerClient::GetSyncService() const { + return nullptr; +} + PasswordStoreInterface* StubPasswordManagerClient::GetProfilePasswordStore() const { return nullptr; diff --git a/components/password_manager/core/browser/stub_password_manager_client.h b/components/password_manager/core/browser/stub_password_manager_client.h index 1290fc7cbc8f6f..469677124f5f58 100644 --- a/components/password_manager/core/browser/stub_password_manager_client.h +++ b/components/password_manager/core/browser/stub_password_manager_client.h @@ -13,6 +13,7 @@ #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_reuse_detector.h" #include "components/password_manager/core/browser/stub_credentials_filter.h" +#include "components/sync/driver/sync_service.h" #include "testing/gmock/include/gmock/gmock.h" #include "third_party/abseil-cpp/absl/types/optional.h" @@ -60,6 +61,7 @@ class StubPasswordManagerClient : public PasswordManagerClient { void AutomaticPasswordSave( std::unique_ptr saved_manager) override; PrefService* GetPrefs() const override; + const syncer::SyncService* GetSyncService() const override; PasswordStoreInterface* GetProfilePasswordStore() const override; PasswordStoreInterface* GetAccountPasswordStore() const override; PasswordReuseManager* GetPasswordReuseManager() const override; diff --git a/components/password_manager/core/common/password_manager_pref_names.cc b/components/password_manager/core/common/password_manager_pref_names.cc index be87af9a907f3b..c46351fa1f3cb6 100644 --- a/components/password_manager/core/common/password_manager_pref_names.cc +++ b/components/password_manager/core/common/password_manager_pref_names.cc @@ -13,6 +13,10 @@ const char kCredentialsEnableAutosignin[] = "credentials_enable_autosignin"; const char kCredentialsEnableService[] = "credentials_enable_service"; #if BUILDFLAG(IS_ANDROID) +const char kAutoSignInEnabledGMS[] = "profile.auto_sign_in_enabled_gms"; +const char kOfferToSavePasswordsEnabledGMS[] = + "profile.save_passwords_enabed_gms"; + const char kCurrentMigrationVersionToGoogleMobileServices[] = "current_migration_version_to_google_mobile_services"; diff --git a/components/password_manager/core/common/password_manager_pref_names.h b/components/password_manager/core/common/password_manager_pref_names.h index 78c9641b09b806..eca338d1c87944 100644 --- a/components/password_manager/core/common/password_manager_pref_names.h +++ b/components/password_manager/core/common/password_manager_pref_names.h @@ -15,16 +15,40 @@ namespace prefs { // Boolean controlling whether the password manager allows automatic signing in // through Credential Management API. +// +// IMPORTANT: This pref is neither querried nor updated on Android if the +// unified password manager is enabled. +// Use `password_manager_util::IsAutoSignInEnabled` to check +// the value of this setting instead. extern const char kCredentialsEnableAutosignin[]; // The value of this preference controls whether the Password Manager will save // credentials. When it is false, it doesn't ask if you want to save passwords // but will continue to fill passwords. -// TODO(melandory): Preference should also control autofill behavior for the -// passwords. +// +// IMPORTANT: This pref is neither querried nor updated on Android if the +// unified password manager is enabled. +// Use `password_manager_util::IsSavingPasswordsEnabled` to check the value of +// this setting instead. extern const char kCredentialsEnableService[]; #if BUILDFLAG(IS_ANDROID) +// Boolean controlling whether the password manager allows automatic signing in +// through Credential Management API. This pref is not synced. Its value is set +// by fetching the latest value from Google Mobile Services. Except for +// migration steps, it should not be modified in Chrome. +extern const char kAutoSignInEnabledGMS[]; + +// Boolean controlling whether the password manager offers to save passwords. +// If false, the password manager will not save credentials, but it will still +// fill previously saved ones. This pref is not synced. Its value is set +// by fetching the latest value from Google Mobile Services. Except for +// migration steps, it should not be modified in Chrome. +// +// This pref doesn't have a policy mapped to it directly, instead, the policy +// mapped to `kCredentialEnableService` will be applied. +extern const char kOfferToSavePasswordsEnabledGMS[]; + // Integer value which indicates the version used to migrate passwords from // built in storage to Google Mobile Services. extern const char kCurrentMigrationVersionToGoogleMobileServices[]; diff --git a/ios/chrome/browser/passwords/ios_chrome_password_manager_client.h b/ios/chrome/browser/passwords/ios_chrome_password_manager_client.h index 7b1861796ff573..f2e2efbbe3d3ca 100644 --- a/ios/chrome/browser/passwords/ios_chrome_password_manager_client.h +++ b/ios/chrome/browser/passwords/ios_chrome_password_manager_client.h @@ -22,6 +22,7 @@ #include "components/password_manager/core/browser/sync_credentials_filter.h" #include "components/password_manager/ios/password_manager_client_bridge.h" #include "components/prefs/pref_member.h" +#include "components/sync/driver/sync_service.h" #import "ios/chrome/browser/safe_browsing/input_event_observer.h" #import "ios/chrome/browser/safe_browsing/password_protection_java_script_feature.h" #import "ios/web/public/web_state.h" @@ -106,6 +107,7 @@ class IOSChromePasswordManagerClient const password_manager::PasswordFeatureManager* GetPasswordFeatureManager() const override; PrefService* GetPrefs() const override; + const syncer::SyncService* GetSyncService() const override; password_manager::PasswordStoreInterface* GetProfilePasswordStore() const override; password_manager::PasswordStoreInterface* GetAccountPasswordStore() diff --git a/ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm b/ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm index 259a5a527303e2..c4bc459a5dcb54 100644 --- a/ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm +++ b/ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm @@ -61,7 +61,8 @@ namespace { -const syncer::SyncService* GetSyncService(ChromeBrowserState* browser_state) { +const syncer::SyncService* GetSyncServiceForBrowserState( + ChromeBrowserState* browser_state) { return SyncServiceFactory::GetForBrowserStateIfExists(browser_state); } @@ -70,12 +71,13 @@ IOSChromePasswordManagerClient::IOSChromePasswordManagerClient( id bridge) : bridge_(bridge), - password_feature_manager_(GetPrefs(), - GetSyncService(bridge_.browserState)), + password_feature_manager_( + GetPrefs(), + GetSyncServiceForBrowserState(bridge_.browserState)), password_reuse_detection_manager_(this), - credentials_filter_( - this, - base::BindRepeating(&GetSyncService, bridge_.browserState)), + credentials_filter_(this, + base::BindRepeating(&GetSyncServiceForBrowserState, + bridge_.browserState)), helper_(this) { saving_passwords_enabled_.Init( password_manager::prefs::kCredentialsEnableService, GetPrefs()); @@ -179,6 +181,11 @@ return (bridge_.browserState)->GetPrefs(); } +const syncer::SyncService* IOSChromePasswordManagerClient::GetSyncService() + const { + return GetSyncServiceForBrowserState(bridge_.browserState); +} + PasswordStoreInterface* IOSChromePasswordManagerClient::GetProfilePasswordStore() const { return IOSChromePasswordStoreFactory::GetForBrowserState( diff --git a/ios/web_view/internal/passwords/web_view_password_manager_client.h b/ios/web_view/internal/passwords/web_view_password_manager_client.h index bdb9b7b25d4792..0a1b2344666498 100644 --- a/ios/web_view/internal/passwords/web_view_password_manager_client.h +++ b/ios/web_view/internal/passwords/web_view_password_manager_client.h @@ -88,6 +88,7 @@ class WebViewPasswordManagerClient const password_manager::PasswordFeatureManager* GetPasswordFeatureManager() const override; PrefService* GetPrefs() const override; + const syncer::SyncService* GetSyncService() const override; password_manager::PasswordStoreInterface* GetProfilePasswordStore() const override; password_manager::PasswordStoreInterface* GetAccountPasswordStore() @@ -132,7 +133,6 @@ class WebViewPasswordManagerClient bool IsAutofillAssistantUIVisible() const override; void set_bridge(id bridge) { bridge_ = bridge; } - const syncer::SyncService* GetSyncService(); safe_browsing::PasswordProtectionService* GetPasswordProtectionService() const override; diff --git a/ios/web_view/internal/passwords/web_view_password_manager_client.mm b/ios/web_view/internal/passwords/web_view_password_manager_client.mm index 74e8dab07e2427..cc11ad99e522fe 100644 --- a/ios/web_view/internal/passwords/web_view_password_manager_client.mm +++ b/ios/web_view/internal/passwords/web_view_password_manager_client.mm @@ -188,6 +188,11 @@ return pref_service_; } +const syncer::SyncService* WebViewPasswordManagerClient::GetSyncService() + const { + return sync_service_; +} + PasswordStoreInterface* WebViewPasswordManagerClient::GetProfilePasswordStore() const { return profile_store_; @@ -321,10 +326,6 @@ return false; } -const syncer::SyncService* WebViewPasswordManagerClient::GetSyncService() { - return sync_service_; -} - safe_browsing::PasswordProtectionService* WebViewPasswordManagerClient::GetPasswordProtectionService() const { // TODO(crbug.com/1148229): Enable PhishGuard in web_view.