[ios/uno] Stop migrating most global SelectedTypes prefs

Behavior changes are behind the existing
kReplaceSyncPromosWithSignInPromos flag.
Before this CL, the current value of the global prefs was taken into
account for the new per-account prefs introduced by UNO. The motivation
was that sync happens to honor the global prefs even for signed-in
non-syncing users today. "So we'd better not start syncing types
that were previously disabled".

Well, this behavior is in fact a bug [1]. Pre-UNO, signed-in
non-syncing users cannot configure most such prefs via any UI.
Meaning some data types could be disabled without any apparent reason.
We can even achieve this broken state by:
- Enabling sync.
- Disabling some types.
- Disabling sync (keeps data type prefs).
- Signing in without syncing.

In this CL the migration logic stops reading the global prefs for most
types, i.e. leaves the new account-scoped prefs in their default
states. There is one exception: passwords, which *does* currently
expose a toggle reading/writing the global pref. So that one needs
to stay. We also use the occasion to fix a tangentially related bug:
only the user-configurable pref should be read, not the one from
enterprise policies.

The main point of this CL is making it easier (in follow-ups) to
use the account-scoped prefs also on other platforms.

[1] Honoring the managed version of the pref is still important to
respect the SyncTypesListDisabled policy. That's still guaranteed by
IsTypeManagedByPolicy().

Bug: 1485015
Change-Id: I159165040d8b1f047586fbe72c58ebd0e6b99f60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4894708
Reviewed-by: Marc Treib <treib@chromium.org>
Auto-Submit: Victor Vianna <victorvianna@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#1226232}

chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc

@@ -551,7 +551,10 @@ IN_PROC_BROWSER_TEST_F(
   ASSERT_FALSE(GetSyncService(0)->GetUserSettings()->GetSelectedTypes().Has(
       syncer::UserSelectableType::kPreferences));
 
-  // The user opts out of Passwords.
+  // The global passwords pref is set to false. This can happen either because
+  // a) the user was syncing before and disabled the toggle, or b) a bug
+  // happened. This is a broken state where kEnablePasswordsAccountStorage
+  // doesn't work but the user has no idea why.
   syncer::UserSelectableTypeSet selected_types =
       GetSyncService(0)->GetUserSettings()->GetRegisteredSelectableTypes();
   selected_types.Remove(syncer::UserSelectableType::kPasswords);
@@ -579,14 +582,15 @@ IN_PROC_BROWSER_TEST_F(
   ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
             GetSyncService(0)->GetTransportState());
 
-  // Bookmarks and Passwords should still be enabled and disabled, respectively.
+  // Bookmarks should be enabled as before. Passwords too - the broken state was
+  // fixed!
   // Note that GetSelectedTypes() now reads from the account-scoped prefs!
   // TODO(crbug.com/1494120): Re-enable
   // `syncer::kEnableBookmarksAccountStorage` when possible and reflect
   // expectations below.
   // EXPECT_TRUE(GetSyncService(0)->GetUserSettings()->GetSelectedTypes().Has(
   //    syncer::UserSelectableType::kBookmarks));
-  EXPECT_FALSE(GetSyncService(0)->GetUserSettings()->GetSelectedTypes().Has(
+  EXPECT_TRUE(GetSyncService(0)->GetUserSettings()->GetSelectedTypes().Has(
       syncer::UserSelectableType::kPasswords));
   // Preferences should've been disabled by the migration.
   EXPECT_FALSE(GetSyncService(0)->GetUserSettings()->GetSelectedTypes().Has(

components/sync/service/sync_prefs.cc

@@ -668,7 +668,7 @@ void SyncPrefs::ClearPassphrasePromptMutedProductVersion() {
 
 bool SyncPrefs::MaybeMigratePrefsForSyncToSigninPart1(
     SyncAccountState account_state,
-    signin::GaiaIdHash gaia_id_hash) {
+    const signin::GaiaIdHash& gaia_id_hash) {
   if (!base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos)) {
     // Ensure that the migration runs again when the feature gets enabled.
     pref_service_->ClearPref(kSyncToSigninMigrationState);
@@ -710,53 +710,42 @@ bool SyncPrefs::MaybeMigratePrefsForSyncToSigninPart1(
       base::Value::Dict* account_settings =
           update_selected_types_dict->EnsureDict(gaia_id_hash.ToBase64());
 
-      // Mostly, the values of the "global" data type prefs get copied to the
-      // account-specific ones. But some data types get special treatment.
-      for (UserSelectableType type : UserSelectableTypeSet::All()) {
-        const char* pref_name = GetPrefNameForType(type);
-        CHECK(pref_name);
-
-        // Initial default value: From the global datatype pref (compare to
-        // GetSelectedTypes()).
-        // TODO(crbug.com/1455963): Find a better solution than manually
-        // overriding the prefs' default values.
-        bool enabled =
-            pref_service_->GetBoolean(pref_name) ||
-            pref_service_->FindPreference(pref_name)->IsDefaultValue();
-
-        // History and open tabs do *not* get migrated; they always start out
-        // "off".
-        if (type == UserSelectableType::kHistory ||
-            type == UserSelectableType::kTabs) {
-          enabled = false;
-        }
-
-        // Settings aka preferences always starts out "off".
-        if (type == UserSelectableType::kPreferences) {
-          enabled = false;
-        }
-
 #if BUILDFLAG(IS_IOS)
-        // Bookmarks and reading list remain enabled only if the user previously
-        // explicitly opted in.
-        if ((type == UserSelectableType::kBookmarks ||
-             type == UserSelectableType::kReadingList) &&
-            !pref_service_->GetBoolean(
-                prefs::internal::kBookmarksAndReadingListAccountStorageOptIn)) {
-          enabled = false;
-        }
+      // Before kReplaceSyncPromosWithSignInPromos was enabled, iOS users had a
+      // toggle that wrote to the global passwords pref. If that toggle was
+      // previously disabled, the new per-account setting must be too.
+      // Read from the user pref store, and not from PrefService::GetBoolean(),
+      // the latter might be affected by enterprise policies.
+      // An unset pref (!old_pref_value) is considered as enabled here, like in
+      // GetSelectedTypes().
+      const base::Value* old_pref_value = pref_service_->GetUserPrefValue(
+          GetPrefNameForType(UserSelectableType::kPasswords));
+      account_settings->Set(GetPrefNameForType(UserSelectableType::kPasswords),
+                            !old_pref_value || old_pref_value->GetBool());
 #endif  // BUILDFLAG(IS_IOS)
 
-        account_settings->Set(pref_name, enabled);
-      }
+      // Settings aka preferences always starts out "off" for existing
+      // signed-in non-syncing users.
+      account_settings->Set(
+          GetPrefNameForType(UserSelectableType::kPreferences), false);
 
+#if BUILDFLAG(IS_IOS)
+      // Bookmarks and reading list remain enabled only if the user previously
+      // explicitly opted in.
+      const bool was_opted_in = pref_service_->GetBoolean(
+          prefs::internal::kBookmarksAndReadingListAccountStorageOptIn);
+      account_settings->Set(GetPrefNameForType(UserSelectableType::kBookmarks),
+                            was_opted_in);
+      account_settings->Set(
+          GetPrefNameForType(UserSelectableType::kReadingList), was_opted_in);
+#endif  // BUILDFLAG(IS_IOS)
       return true;
     }
   }
 }
 
 bool SyncPrefs::MaybeMigratePrefsForSyncToSigninPart2(
-    signin::GaiaIdHash gaia_id_hash,
+    const signin::GaiaIdHash& gaia_id_hash,
     bool is_using_explicit_passphrase) {
   // The migration pref shouldn't be set if the feature is disabled, but if it
   // somehow happened, do *not* run the migration, and clear the pref so that

components/sync/service/sync_prefs.h

@@ -223,16 +223,18 @@ class SyncPrefs {
   // more precisely, new profiles).
   // This should be called early during browser startup.
   // Returns whether the migration ran, i.e. whether any user settings were set.
-  bool MaybeMigratePrefsForSyncToSigninPart1(SyncAccountState account_state,
-                                             signin::GaiaIdHash gaia_id_hash);
+  bool MaybeMigratePrefsForSyncToSigninPart1(
+      SyncAccountState account_state,
+      const signin::GaiaIdHash& gaia_id_hash);
 
   // Second part of the above migration, which depends on the user's passphrase
   // type, which isn't known yet during browser startup. This should be called
   // as soon as the passphrase type is known, and will only do any migration if
   // the above method has flagged that it's necessary.
   // Returns whether the migration ran, i.e. whether any user settings were set.
-  bool MaybeMigratePrefsForSyncToSigninPart2(signin::GaiaIdHash gaia_id_hash,
-                                             bool is_using_explicit_passphrase);
+  bool MaybeMigratePrefsForSyncToSigninPart2(
+      const signin::GaiaIdHash& gaia_id_hash,
+      bool is_using_explicit_passphrase);
 
   // Should be called when Sync gets disabled / the user signs out. Clears any
   // temporary state from the above migration.

components/sync/service/sync_prefs_unittest.cc

@@ -762,19 +762,6 @@ class SyncPrefsMigrationTest : public testing::Test {
     return pref_value->GetBool() ? PREF_TRUE : PREF_FALSE;
   }
 
-  bool BooleanUserPrefMatches(const char* pref_name,
-                              BooleanPrefState state) const {
-    const base::Value* pref_value = pref_service_.GetUserPrefValue(pref_name);
-    switch (state) {
-      case PREF_FALSE:
-        return pref_value && !pref_value->GetBool();
-      case PREF_TRUE:
-        return pref_value && pref_value->GetBool();
-      case PREF_UNSET:
-        return !pref_value;
-    }
-  }
-
   // Global prefs for syncing users, affecting all accounts.
   const char* kGlobalBookmarksPref =
       SyncPrefs::GetPrefNameForTypeForTesting(UserSelectableType::kBookmarks);
@@ -955,9 +942,9 @@ TEST_F(SyncPrefsMigrationTest, SyncToSignin_GlobalPrefsAreUnchanged) {
       kReplaceSyncPromosWithSignInPromos);
 
   for (UserSelectableType type : UserSelectableTypeSet::All()) {
-    ASSERT_TRUE(
-        BooleanUserPrefMatches(SyncPrefs::GetPrefNameForTypeForTesting(type),
-                               BooleanPrefState::PREF_UNSET));
+    ASSERT_EQ(
+        GetBooleanUserPrefValue(SyncPrefs::GetPrefNameForTypeForTesting(type)),
+        BooleanPrefState::PREF_UNSET);
   }
 
   SyncPrefs prefs(&pref_service_);
@@ -969,9 +956,9 @@ TEST_F(SyncPrefsMigrationTest, SyncToSignin_GlobalPrefsAreUnchanged) {
       /*is_using_explicit_passphrase=*/true));
 
   for (UserSelectableType type : UserSelectableTypeSet::All()) {
-    EXPECT_TRUE(
-        BooleanUserPrefMatches(SyncPrefs::GetPrefNameForTypeForTesting(type),
-                               BooleanPrefState::PREF_UNSET));
+    EXPECT_EQ(
+        GetBooleanUserPrefValue(SyncPrefs::GetPrefNameForTypeForTesting(type)),
+        BooleanPrefState::PREF_UNSET);
   }
 }
 
@@ -1213,6 +1200,83 @@ TEST_F(SyncPrefsMigrationTest, SyncToSignin_Part2RunsOnSecondAttempt) {
   }
 }
 
+#if BUILDFLAG(IS_IOS)
+TEST_F(SyncPrefsMigrationTest,
+       SyncToSignin_DisablesPasswordsIfUserDisabledGlobalPref) {
+  {
+    // One day, before kReplaceSyncPromosWithSignInPromos was enabled and the
+    // per-account prefs were used, the user disabled the temporary passwords
+    // toggle, writing to the global pref.
+    base::test::ScopedFeatureList disable_sync_to_signin;
+    disable_sync_to_signin.InitAndDisableFeature(
+        kReplaceSyncPromosWithSignInPromos);
+
+    SetBooleanUserPrefValue(kGlobalPasswordsPref, BooleanPrefState::PREF_FALSE);
+
+    ASSERT_FALSE(
+        SyncPrefs(&pref_service_)
+            .GetSelectedTypes(SyncPrefs::SyncAccountState::kSignedInNotSyncing)
+            .Has(UserSelectableType::kPasswords));
+  }
+
+  {
+    // After kReplaceSyncPromosWithSignInPromos is enabled and
+    // GetSelectedTypesForAccount() starts being used, passwords should still be
+    // disabled.
+    base::test::ScopedFeatureList enable_sync_to_signin(
+        kReplaceSyncPromosWithSignInPromos);
+
+    SyncPrefs prefs(&pref_service_);
+    prefs.MaybeMigratePrefsForSyncToSigninPart1(
+        SyncPrefs::SyncAccountState::kSignedInNotSyncing, gaia_id_hash_);
+
+    EXPECT_FALSE(prefs.GetSelectedTypesForAccount(gaia_id_hash_)
+                     .Has(UserSelectableType::kPasswords));
+  }
+}
+
+TEST_F(SyncPrefsMigrationTest,
+       SyncToSignin_LeavesPasswordsAloneIfDisabledByPolicy) {
+  {
+    // One day, before kReplaceSyncPromosWithSignInPromos was enabled and the
+    // per-account prefs were used, passwords were disabled by a policy.
+    base::test::ScopedFeatureList disable_sync_to_signin;
+    disable_sync_to_signin.InitAndDisableFeature(
+        kReplaceSyncPromosWithSignInPromos);
+
+    pref_service_.SetManagedPref(kGlobalPasswordsPref, base::Value(false));
+
+    ASSERT_FALSE(
+        SyncPrefs(&pref_service_)
+            .GetSelectedTypes(SyncPrefs::SyncAccountState::kSignedInNotSyncing)
+            .Has(UserSelectableType::kPasswords));
+  }
+
+  {
+    // kReplaceSyncPromosWithSignInPromos is enabled and
+    // GetSelectedTypesForAccount() starts being used.
+    base::test::ScopedFeatureList enable_sync_to_signin(
+        kReplaceSyncPromosWithSignInPromos);
+
+    SyncPrefs prefs(&pref_service_);
+    prefs.MaybeMigratePrefsForSyncToSigninPart1(
+        SyncPrefs::SyncAccountState::kSignedInNotSyncing, gaia_id_hash_);
+
+    // The policy is still in place, so passwords is still disabled for the
+    // moment.
+    ASSERT_FALSE(prefs.GetSelectedTypesForAccount(gaia_id_hash_)
+                     .Has(UserSelectableType::kPasswords));
+
+    // The policy is lifted.
+    pref_service_.RemoveManagedPref(kGlobalPasswordsPref);
+
+    // Passwords should now be enabled.
+    EXPECT_TRUE(prefs.GetSelectedTypesForAccount(gaia_id_hash_)
+                    .Has(UserSelectableType::kPasswords));
+  }
+}
+#endif  // BUILDFLAG(IS_IOS)
+
 }  // namespace
 
 }  // namespace syncer