Make "default password store" pref per-account

This CL removes prefs::kIsAccountStoreDefault, and instead adds an entry
in prefs::kAccountStoragePerAccountSettings.
Since the old pref was never launched, no migration code is necessary.

Bug: 1035407
Change-Id: I7d47fbaf663a7f07c2747d05633a0efb51d03587
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2014926Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734886}

components/autofill/core/common/password_form.h

@@ -291,13 +291,14 @@ struct PasswordForm {
   // as signal for password generation eligibility.
   bool is_new_password_reliable = false;
 
+  // Serialized to prefs, so don't change numeric values!
   enum class Store {
     // Default value.
-    kNotSet,
+    kNotSet = 0,
     // Credential came from the profile (i.e. local) storage.
-    kProfileStore,
+    kProfileStore = 1,
     // Credential came from the Gaia-account-scoped storage.
-    kAccountStore
+    kAccountStore = 2
   };
   Store in_store = Store::kNotSet;
 

components/password_manager/core/browser/password_feature_manager_impl.cc

@@ -60,17 +60,15 @@ void PasswordFeatureManagerImpl::SetAccountStorageOptIn(bool opt_in) {
 
 void PasswordFeatureManagerImpl::SetDefaultPasswordStore(
     const PasswordForm::Store& store) {
-  DCHECK(pref_service_);
-  pref_service_->SetBoolean(prefs::kIsAccountStoreDefault,
-                            store == PasswordForm::Store::kAccountStore);
+  password_manager_util::SetDefaultPasswordStore(pref_service_, sync_service_,
+                                                 store);
 }
 
 PasswordForm::Store PasswordFeatureManagerImpl::GetDefaultPasswordStore()
     const {
   DCHECK(pref_service_);
-  return pref_service_->GetBoolean(prefs::kIsAccountStoreDefault)
-             ? PasswordForm::Store::kAccountStore
-             : PasswordForm::Store::kProfileStore;
+  return password_manager_util::GetDefaultPasswordStore(pref_service_,
+                                                        sync_service_);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_manager.cc

@@ -201,7 +201,6 @@ void PasswordManager::RegisterProfilePrefs(
 
   registry->RegisterDictionaryPref(prefs::kAccountStoragePerAccountSettings);
 
-  registry->RegisterBooleanPref(prefs::kIsAccountStoreDefault, true);
 #if defined(OS_MACOSX)
   registry->RegisterIntegerPref(prefs::kKeychainMigrationStatus,
                                 4 /* MIGRATED_DELETED */);

components/password_manager/core/browser/password_manager_util.cc

@@ -53,6 +53,44 @@ bool IsBetterMatch(const PasswordForm* lhs, const PasswordForm* rhs) {
          std::make_pair(!rhs->is_public_suffix_match, rhs->date_last_used);
 }
 
+// Returns whether the account-scoped password storage can be enabled in
+// principle for the current profile. This is constant for a given profile
+// (until browser restart).
+bool CanAccountStorageBeEnabled(const syncer::SyncService* sync_service) {
+  if (!base::FeatureList::IsEnabled(
+          password_manager::features::kEnablePasswordsAccountStorage)) {
+    return false;
+  }
+
+  // |sync_service| is null in incognito mode, or if --disable-sync was
+  // specified on the command-line.
+  if (!sync_service)
+    return false;
+
+  return true;
+}
+
+// Whether the currently signed-in user (if any) is eligible for using the
+// account-scoped password storage. This is the case if:
+// - The account storage can be enabled in principle.
+// - Sync-the-transport is running (i.e. there's a signed-in user, Sync is not
+//   disabled by policy, etc).
+// - There is no custom passphrase (because Sync transport offers no way to
+//   enter the passphrase yet). Note that checking this requires the SyncEngine
+//   to be initialized.
+// - Sync-the-feature is NOT enabled (if it is, there's only a single combined
+//   storage).
+bool IsUserEligibleForAccountStorage(const syncer::SyncService* sync_service) {
+  return CanAccountStorageBeEnabled(sync_service) &&
+         sync_service->GetTransportState() !=
+             syncer::SyncService::TransportState::DISABLED &&
+         sync_service->IsEngineInitialized() &&
+         !sync_service->GetUserSettings()->IsUsingSecondaryPassphrase() &&
+         !sync_service->IsSyncFeatureEnabled();
+}
+
+// TODO(crbug.com/1035407): Don't use CoreAccountId as a persistent
+// identifier; see comment on CoreAccountId itself. Use the GAIA ID instead.
 std::string GetAccountHash(const CoreAccountId& account_id) {
   std::string account_hash;
   base::Base64Encode(crypto::SHA256HashString(account_id.ToString()),
@@ -60,7 +98,18 @@ std::string GetAccountHash(const CoreAccountId& account_id) {
   return account_hash;
 }
 
+PasswordForm::Store PasswordStoreFromInt(int value) {
+  switch (value) {
+    case static_cast<int>(PasswordForm::Store::kProfileStore):
+      return PasswordForm::Store::kProfileStore;
+    case static_cast<int>(PasswordForm::Store::kAccountStore):
+      return PasswordForm::Store::kAccountStore;
+  }
+  return PasswordForm::Store::kNotSet;
+}
+
 const char kAccountStorageOptedInKey[] = "opted_in";
+const char kAccountStorageDefaultStoreKey[] = "default_store";
 
 // Helper class for reading account storage settings for a given account.
 class AccountStorageSettingsReader {
@@ -82,6 +131,16 @@ class AccountStorageSettingsReader {
         .value_or(false);
   }
 
+  PasswordForm::Store GetDefaultStore() const {
+    if (!account_settings_)
+      return PasswordForm::Store::kNotSet;
+    base::Optional<int> value =
+        account_settings_->FindIntKey(kAccountStorageDefaultStoreKey);
+    if (!value)
+      return PasswordForm::Store::kNotSet;
+    return PasswordStoreFromInt(*value);
+  }
+
  private:
   // May be null, if no settings for this account were saved yet.
   const base::Value* account_settings_ = nullptr;
@@ -110,6 +169,11 @@ class ScopedAccountStorageSettingsUpdate {
     account_settings_->SetBoolKey(kAccountStorageOptedInKey, opt_in);
   }
 
+  void SetDefaultStore(PasswordForm::Store default_store) {
+    account_settings_->SetIntKey(kAccountStorageDefaultStoreKey,
+                                 static_cast<int>(default_store));
+  }
+
  private:
   DictionaryPrefUpdate update_;
   base::Value* account_settings_ = nullptr;
@@ -386,14 +450,7 @@ bool IsOptedInForAccountStorage(const PrefService* pref_service,
                                 const syncer::SyncService* sync_service) {
   DCHECK(pref_service);
 
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kEnablePasswordsAccountStorage)) {
-    return false;
-  }
-
-  // |sync_service| is null in incognito mode, or if --disable-sync was
-  // specified on the command-line.
-  if (!sync_service)
+  if (!CanAccountStorageBeEnabled(sync_service))
     return false;
 
   CoreAccountId account_id = sync_service->GetAuthenticatedAccountId();
@@ -407,29 +464,8 @@ bool ShouldShowAccountStorageOptIn(const PrefService* pref_service,
                                    const syncer::SyncService* sync_service) {
   DCHECK(pref_service);
 
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kEnablePasswordsAccountStorage)) {
-    return false;
-  }
-
-  // |sync_service| is null in incognito mode, or if --disable-sync was
-  // specified on the command-line.
-  if (!sync_service)
-    return false;
-
-  // Only show the opt-in if:
-  // - Sync transport is enabled (i.e. user is signed in, Sync is not disabled
-  //   by policy etc) - otherwise there's no point in asking.
-  // - There is no custom Sync passphrase (Sync transport offers no way to enter
-  //   the passphrase yet). Note that checking this requires the SyncEngine to
-  //   be initialized.
-  // - Sync feature is NOT enabled - Sync feature doesn't depend on this opt-in.
-  // - Not already opted in.
-  return sync_service->GetTransportState() !=
-             syncer::SyncService::TransportState::DISABLED &&
-         sync_service->IsEngineInitialized() &&
-         !sync_service->GetUserSettings()->IsUsingSecondaryPassphrase() &&
-         !sync_service->IsSyncFeatureEnabled() &&
+  // Show the opt-in if the user is eligible, but not yet opted in.
+  return IsUserEligibleForAccountStorage(sync_service) &&
          !IsOptedInForAccountStorage(pref_service, sync_service);
 }
 
@@ -451,4 +487,51 @@ void SetAccountStorageOptIn(PrefService* pref_service,
       .SetOptedIn(opt_in);
 }
 
+PasswordForm::Store GetDefaultPasswordStore(
+    const PrefService* pref_service,
+    const syncer::SyncService* sync_service) {
+  DCHECK(pref_service);
+
+  if (!IsUserEligibleForAccountStorage(sync_service))
+    return PasswordForm::Store::kProfileStore;
+
+  CoreAccountId account_id = sync_service->GetAuthenticatedAccountId();
+  if (account_id.empty())
+    return PasswordForm::Store::kProfileStore;
+
+  PasswordForm::Store default_store =
+      AccountStorageSettingsReader(pref_service, account_id).GetDefaultStore();
+  // If none of the early-outs above triggered, then we *can* save to the
+  // account store in principle (though the user might not have opted in to that
+  // yet). In this case, default to the account store.
+  if (default_store == PasswordForm::Store::kNotSet)
+    return PasswordForm::Store::kAccountStore;
+  return default_store;
+}
+
+void SetDefaultPasswordStore(PrefService* pref_service,
+                             const syncer::SyncService* sync_service,
+                             PasswordForm::Store default_store) {
+  DCHECK(pref_service);
+  DCHECK(sync_service);
+  DCHECK(base::FeatureList::IsEnabled(
+      password_manager::features::kEnablePasswordsAccountStorage));
+
+  CoreAccountId account_id = sync_service->GetAuthenticatedAccountId();
+  if (account_id.empty()) {
+    // Maybe the account went away since the UI was shown. This should be rare,
+    // but is ultimately harmless - just do nothing here.
+    return;
+  }
+  ScopedAccountStorageSettingsUpdate(pref_service, account_id)
+      .SetDefaultStore(default_store);
+  if (account_id.empty()) {
+    // Maybe the account went away since the UI was shown. This should be rare,
+    // but is ultimately harmless - just do nothing here.
+    return;
+  }
+  ScopedAccountStorageSettingsUpdate(pref_service, account_id)
+      .SetDefaultStore(default_store);
+}
+
 }  // namespace password_manager_util

components/password_manager/core/browser/password_manager_util.h

@@ -165,6 +165,19 @@ void SetAccountStorageOptIn(PrefService* pref_service,
                             const syncer::SyncService* sync_service,
                             bool opt_in);
 
+// Returns the default storage location for signed-in but non-syncing users
+// (i.e. will new passwords be saved to locally or to the account by default).
+// Always returns an actual value, never kNotSet.
+autofill::PasswordForm::Store GetDefaultPasswordStore(
+    const PrefService* pref_service,
+    const syncer::SyncService* sync_service);
+
+// Sets the default storage location for signed-in but non-syncing users (i.e.
+// will new passwords be saved to locally or to the account by default).
+void SetDefaultPasswordStore(PrefService* pref_service,
+                             const syncer::SyncService* sync_service,
+                             autofill::PasswordForm::Store default_store);
+
 }  // namespace password_manager_util
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_UTIL_H_

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -13,13 +13,16 @@
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/task_environment.h"
+#include "base/test/scoped_feature_list.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.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/identity_manager/account_info.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -364,4 +367,111 @@ TEST(PasswordManagerUtil, MakeNormalizedBlacklistedForm_Proxy) {
   EXPECT_EQ(GURL(kTestProxyOrigin), blacklisted_credential.origin);
 }
 
+TEST(PasswordManagerUtil, AccountStoragePerAccountSettings_FeatureDisabled) {
+  base::test::ScopedFeatureList features;
+  features.InitAndDisableFeature(
+      password_manager::features::kEnablePasswordsAccountStorage);
+
+  TestingPrefServiceSimple pref_service;
+  pref_service.registry()->RegisterDictionaryPref(
+      password_manager::prefs::kAccountStoragePerAccountSettings);
+
+  CoreAccountInfo account;
+  account.email = "first@account.com";
+  account.gaia = "first";
+  account.account_id = CoreAccountId::FromGaiaId(account.gaia);
+
+  // SyncService is running in transport mode with |account|.
+  syncer::TestSyncService sync_service;
+  sync_service.SetIsAuthenticatedAccountPrimary(false);
+  sync_service.SetAuthenticatedAccountInfo(account);
+  ASSERT_EQ(sync_service.GetTransportState(),
+            syncer::SyncService::TransportState::ACTIVE);
+  ASSERT_FALSE(sync_service.IsSyncFeatureEnabled());
+
+  // Since the account storage feature is disabled, the profile store should be
+  // the default.
+  EXPECT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_FALSE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kProfileStore);
+
+  // Same if the user is signed out.
+  sync_service.SetAuthenticatedAccountInfo(CoreAccountInfo());
+  sync_service.SetTransportState(syncer::SyncService::TransportState::DISABLED);
+  EXPECT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kProfileStore);
+}
+
+TEST(PasswordManagerUtil, AccountStoragePerAccountSettings) {
+  base::test::ScopedFeatureList features;
+  features.InitAndEnableFeature(
+      password_manager::features::kEnablePasswordsAccountStorage);
+
+  TestingPrefServiceSimple pref_service;
+  pref_service.registry()->RegisterDictionaryPref(
+      password_manager::prefs::kAccountStoragePerAccountSettings);
+
+  CoreAccountInfo first_account;
+  first_account.email = "first@account.com";
+  first_account.gaia = "first";
+  first_account.account_id = CoreAccountId::FromGaiaId(first_account.gaia);
+
+  CoreAccountInfo second_account;
+  second_account.email = "second@account.com";
+  second_account.gaia = "second";
+  second_account.account_id = CoreAccountId::FromGaiaId(second_account.gaia);
+
+  // SyncService is running in transport mode with |first_account|.
+  syncer::TestSyncService sync_service;
+  sync_service.SetIsAuthenticatedAccountPrimary(false);
+  sync_service.SetAuthenticatedAccountInfo(first_account);
+  ASSERT_EQ(sync_service.GetTransportState(),
+            syncer::SyncService::TransportState::ACTIVE);
+  ASSERT_FALSE(sync_service.IsSyncFeatureEnabled());
+
+  // By default, the user is not opted in. But since they're eligible for
+  // account storage, the default store should be the account one.
+  EXPECT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_TRUE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kAccountStore);
+
+  // Opt in!
+  SetAccountStorageOptIn(&pref_service, &sync_service, true);
+  EXPECT_TRUE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_FALSE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  // ...and change the default store to the profile one.
+  SetDefaultPasswordStore(&pref_service, &sync_service,
+                          autofill::PasswordForm::Store::kProfileStore);
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kProfileStore);
+
+  // Change to |second_account|. The opt-in for |first_account| should not
+  // apply, and similarly the default store should be back to "account".
+  sync_service.SetAuthenticatedAccountInfo(second_account);
+  EXPECT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_TRUE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kAccountStore);
+
+  // Change back to |first_account|. The previous opt-in and chosen default
+  // store should now apply again.
+  sync_service.SetAuthenticatedAccountInfo(first_account);
+  EXPECT_TRUE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_FALSE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kProfileStore);
+
+  // Sign out. Now the settings should have reasonable default values (not opted
+  // in, save to profile store).
+  sync_service.SetAuthenticatedAccountInfo(CoreAccountInfo());
+  sync_service.SetTransportState(syncer::SyncService::TransportState::DISABLED);
+  EXPECT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
+  EXPECT_FALSE(ShouldShowAccountStorageOptIn(&pref_service, &sync_service));
+  EXPECT_EQ(GetDefaultPasswordStore(&pref_service, &sync_service),
+            autofill::PasswordForm::Store::kProfileStore);
+}
+
 }  // namespace password_manager_util

components/password_manager/core/common/password_manager_pref_names.cc

@@ -38,8 +38,6 @@ const char kNumberSignInPasswordPromoShown[] =
 const char kSignInPasswordPromoRevive[] =
     "profile.sign_in_password_promo_revive";
 
-const char kIsAccountStoreDefault[] = "profile.is_account_store_default";
-
 const char kAccountStoragePerAccountSettings[] =
     "profile.password_account_storage_settings";
 

components/password_manager/core/common/password_manager_pref_names.h

@@ -63,10 +63,6 @@ extern const char kNumberSignInPasswordPromoShown[];
 // Safe to remove for M82.
 extern const char kSignInPasswordPromoRevive[];
 
-// Boolean that is true when the default password store is the Google account
-// store, and false when the profile store is the default store.
-extern const char kIsAccountStoreDefault[];
-
 // A dictionary of account-storage-related settings that exist per Gaia account
 // (e.g. whether that user has opted in). It maps from hash of Gaia ID to
 // dictionary of key-value pairs.