Unify clearing of sync prefs

SyncPrefs is a thin layer on top of preferences and hosts two groups
of preferences:
1) Actual user-facing settings, such as type-selection, exposed via
   SyncUserSettings.
2) Local "bookkeeping" sync metadata, such the last synced time or the
   client ID (cache GUID).

In addition, there are two cases that fall somewhere in the middle,
whose behavior is changed in this patch:
a) FirstSetupComplete: which roughly represents the user having
   consented to sync-the-feature (as opposed to transport-only upon
   sign-in without explicit user consent).
b) The encryption-bootstrap-token: which represent an explicit
   passphrase (usually custom passphrase) entered by the user, that
   allows decrypting the incoming sync changes and encrypt outgoing
   ones.

The last two preferences above fit group 1 better, so this patch stops
clearing them in SyncPrefs::ClearPreferences(), now renamed to
SyncPrefs::ClearLocalSyncTransportData().

With such cleanup, what used to be
ClearDirectoryConsistencyPreferences() is now merged into a single
clearing function, ClearLocalSyncTransportData(), and all calling sites
are unified by directly clearing prefs in ShutdownImpl(DISABLE_SYNC), as
opposed to individual calling sites.

This introduces -arguably desirable- behavioral changes because
codepaths like RESET_LOCAL_SYNC_DATA or STOP_SYNC_FOR_DISABLED_ACCOUNT
now actually clear all local metadata, which most notably includes
keystore keys.

Change-Id: I2c42f98c4e068c7e340580d0b78a5cd5b5c46171
Bug: 1046237
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023649
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735841}

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

@@ -54,7 +54,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldInitializePollPrefs) {
 // This test verifies that updates of the poll interval get persisted
 // That's important make sure clients with short live times will eventually poll
 // (e.g. Android).
-IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldUpdatePollPrefs) {
+IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
+                       PRE_ShouldUsePollIntervalFromPrefs) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   sync_pb::ClientCommand client_command;
@@ -76,17 +77,12 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldUpdatePollPrefs) {
 
 IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
                        ShouldUsePollIntervalFromPrefs) {
-  // Setup clients and provide new poll interval via prefs.
-  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
-  SyncPrefs sync_prefs(GetProfile(0)->GetPrefs());
-  sync_prefs.SetPollInterval(base::TimeDelta::FromSeconds(123));
-
   // Execute a sync cycle and verify this cycle used that interval.
   // This test assumes the SyncScheduler reads the actual interval from the
   // context. This is covered in the SyncSchedulerImpl's unittest.
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   EXPECT_THAT(GetClient(0)->GetLastCycleSnapshot().poll_interval().InSeconds(),
-              Eq(123));
+              Eq(67));
 }
 
 // This test simulates the poll interval expiring between restarts.

components/sync/base/sync_prefs.cc

@@ -293,6 +293,10 @@ void SyncPrefs::RegisterProfilePrefs(
   }
 #endif
 
+  // The encryption bootstrap token represents a user-entered passphrase.
+  registry->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
+                               std::string());
+
   // Internal or bookkeeping prefs.
   registry->RegisterStringPref(prefs::kSyncCacheGuid, std::string());
   registry->RegisterStringPref(prefs::kSyncBirthday, std::string());
@@ -301,8 +305,6 @@ void SyncPrefs::RegisterProfilePrefs(
   registry->RegisterInt64Pref(prefs::kSyncLastPollTime, 0);
   registry->RegisterInt64Pref(prefs::kSyncPollIntervalSeconds, 0);
   registry->RegisterBooleanPref(prefs::kSyncManaged, false);
-  registry->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
-                               std::string());
   registry->RegisterStringPref(prefs::kSyncKeystoreEncryptionBootstrapToken,
                                std::string());
   registry->RegisterBooleanPref(prefs::kSyncPassphrasePrompted, false);
@@ -347,7 +349,7 @@ void SyncPrefs::RemoveSyncPrefObserver(SyncPrefObserver* sync_pref_observer) {
   sync_pref_observers_.RemoveObserver(sync_pref_observer);
 }
 
-void SyncPrefs::ClearPreferences() {
+void SyncPrefs::ClearLocalSyncTransportData() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Clear user's birth year and gender.
@@ -357,31 +359,19 @@ void SyncPrefs::ClearPreferences() {
   // even reveal their true birth year.
   pref_service_->ClearPref(prefs::kSyncDemographics);
 
-  ClearDirectoryConsistencyPreferences();
-
   pref_service_->ClearPref(prefs::kSyncLastSyncedTime);
   pref_service_->ClearPref(prefs::kSyncLastPollTime);
   pref_service_->ClearPref(prefs::kSyncPollIntervalSeconds);
-  pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
   pref_service_->ClearPref(prefs::kSyncKeystoreEncryptionBootstrapToken);
   pref_service_->ClearPref(prefs::kSyncPassphrasePrompted);
   pref_service_->ClearPref(prefs::kSyncInvalidationVersions);
   pref_service_->ClearPref(prefs::kSyncLastRunVersion);
-  // No need to clear kManaged, kEnableLocalSyncBackend or kLocalSyncBackendDir,
-  // since they're never actually set as user preferences.
-
-  // Note: We do *not* clear prefs which are directly user-controlled such as
-  // the set of selected types here, so that if the user ever chooses to enable
-  // Sync again, they start off with their previous settings by default.
-  // We do however require going through first-time setup again.
-  pref_service_->ClearPref(prefs::kSyncFirstSetupComplete);
-}
-
-void SyncPrefs::ClearDirectoryConsistencyPreferences() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   pref_service_->ClearPref(prefs::kSyncCacheGuid);
   pref_service_->ClearPref(prefs::kSyncBirthday);
   pref_service_->ClearPref(prefs::kSyncBagOfChips);
+
+  // No need to clear kManaged, kEnableLocalSyncBackend or kLocalSyncBackendDir,
+  // since they're never actually set as user preferences.
 }
 
 bool SyncPrefs::IsFirstSetupComplete() const {
@@ -394,6 +384,11 @@ void SyncPrefs::SetFirstSetupComplete() {
   pref_service_->SetBoolean(prefs::kSyncFirstSetupComplete, true);
 }
 
+void SyncPrefs::ClearFirstSetupComplete() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  pref_service_->ClearPref(prefs::kSyncFirstSetupComplete);
+}
+
 bool SyncPrefs::IsSyncRequested() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return pref_service_->GetBoolean(prefs::kSyncRequested);
@@ -556,6 +551,11 @@ void SyncPrefs::SetEncryptionBootstrapToken(const std::string& token) {
   pref_service_->SetString(prefs::kSyncEncryptionBootstrapToken, token);
 }
 
+void SyncPrefs::ClearEncryptionBootstrapToken() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
+}
+
 std::string SyncPrefs::GetKeystoreEncryptionBootstrapToken() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return pref_service_->GetString(prefs::kSyncKeystoreEncryptionBootstrapToken);

components/sync/base/sync_prefs.h

@@ -77,19 +77,15 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Clears "bookkeeping" sync preferences, such as the last synced time,
   // whether the last shutdown was clean, etc. Does *not* clear sync preferences
   // which are directly user-controlled, such as the set of selected types.
-  void ClearPreferences();
-
-  // Clears only the subset of preferences that are redundant with the sync
-  // directory and used only for verifying consistency with prefs.
-  // TODO(crbug.com/923285): Remove this function and instead rely solely on
-  // ClearPreferences() once investigations are finalized are we understand the
-  // source of discrepancies for UMA Sync.DirectoryVsPrefsConsistency.
-  void ClearDirectoryConsistencyPreferences();
+  void ClearLocalSyncTransportData();
 
   // Getters and setters for global sync prefs.
 
+  // First-Setup-Complete is conceptually similar to the user's consent to
+  // enable sync-the-feature.
   bool IsFirstSetupComplete() const;
   void SetFirstSetupComplete();
+  void ClearFirstSetupComplete();
 
   bool IsSyncRequested() const;
   void SetSyncRequested(bool is_requested);
@@ -139,9 +135,14 @@ class SyncPrefs : public CryptoSyncPrefs,
   // policy, is handled directly in ProfileSyncService.
   bool IsManaged() const;
 
-  // Use this encryption bootstrap token if we're using an explicit passphrase.
+  // The encryption bootstrap token is used for explicit passphrase users
+  // (usually custom passphrase) and represents a user-entered passphrase.
+  // Hence, it gets treated as user-controlled similarly to sync datatype
+  // selection settings (i.e. doesn't get cleared in
+  // ClearLocalSyncTransportData()).
   std::string GetEncryptionBootstrapToken() const override;
   void SetEncryptionBootstrapToken(const std::string& token) override;
+  void ClearEncryptionBootstrapToken();
 
   // Use this keystore bootstrap token if we're not using an explicit
   // passphrase.
@@ -151,10 +152,6 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Maps |type| to its corresponding preference name.
   static const char* GetPrefNameForType(UserSelectableType type);
 
-  // Copy of various fields historically owned and persisted by the Directory.
-  // This is a future-proof approach to ultimately replace the Directory once
-  // most users have populated prefs and the Directory is about to be removed.
-  // TODO(crbug.com/923287): Figure out if this is an appropriate place.
   void SetCacheGuid(const std::string& cache_guid);
   std::string GetCacheGuid() const;
   void SetBirthday(const std::string& birthday);

components/sync/base/sync_prefs_unittest.cc

@@ -122,9 +122,7 @@ TEST_F(SyncPrefsTest, ObservedPrefs) {
 
   sync_prefs_->SetFirstSetupComplete();
   EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
-  // There's no direct way to clear the first-setup-complete bit, so just reset
-  // all prefs instead.
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearFirstSetupComplete();
   EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
 
   sync_prefs_->SetSyncRequested(true);
@@ -148,24 +146,32 @@ TEST_F(SyncPrefsTest, SetSelectedOsTypesTriggersPreferredDataTypesPrefChange) {
 }
 #endif
 
-TEST_F(SyncPrefsTest, ClearPreferences) {
-  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
-  EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
+TEST_F(SyncPrefsTest, ClearLocalSyncTransportData) {
+  ASSERT_FALSE(sync_prefs_->IsFirstSetupComplete());
+  ASSERT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
+  ASSERT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
 
   sync_prefs_->SetFirstSetupComplete();
   sync_prefs_->SetLastSyncedTime(base::Time::Now());
-  sync_prefs_->SetEncryptionBootstrapToken("token");
+  sync_prefs_->SetEncryptionBootstrapToken("explicit_passphrase_token");
+  sync_prefs_->SetKeystoreEncryptionBootstrapToken("keystore_token");
 
-  EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
-  EXPECT_NE(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_EQ("token", sync_prefs_->GetEncryptionBootstrapToken());
+  ASSERT_TRUE(sync_prefs_->IsFirstSetupComplete());
+  ASSERT_NE(base::Time(), sync_prefs_->GetLastSyncedTime());
+  ASSERT_EQ("explicit_passphrase_token",
+            sync_prefs_->GetEncryptionBootstrapToken());
+  ASSERT_EQ("keystore_token",
+            sync_prefs_->GetKeystoreEncryptionBootstrapToken());
 
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearLocalSyncTransportData();
 
-  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
   EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
+  EXPECT_TRUE(sync_prefs_->GetKeystoreEncryptionBootstrapToken().empty());
+
+  // User-entered field should not have been cleared.
+  EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
+  EXPECT_EQ("explicit_passphrase_token",
+            sync_prefs_->GetEncryptionBootstrapToken());
 }
 
 TEST_F(SyncPrefsTest, ReadDemographicsWithRandomOffset) {
@@ -221,7 +227,7 @@ TEST_F(SyncPrefsTest, ReadAndClearUserDemographicPreferences) {
     ASSERT_TRUE(demographics_result.IsSuccess());
   }
 
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearLocalSyncTransportData();
 
   // Verify that demographics are not provided and kSyncDemographics is cleared.
   // Note that we retain kSyncDemographicsBirthYearOffset. If the user resumes

components/sync/driver/profile_sync_service.cc

@@ -518,9 +518,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   // updates requires that the request either has a birthday, or there should be
   // no progress marker).
   if (sync_prefs_.GetCacheGuid().empty() || sync_prefs_.GetBirthday().empty()) {
-    // TODO(crbug.com/923285): This doesn't seem to belong here, or if it does,
-    // all preferences should be cleared via SyncPrefs::ClearPreferences().
-    sync_prefs_.ClearDirectoryConsistencyPreferences();
+    sync_prefs_.ClearLocalSyncTransportData();
     sync_prefs_.SetCacheGuid(GenerateCacheGUID());
   }
 
@@ -627,6 +625,7 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
             base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
                            sync_client_->GetSyncDataPath()));
       }
+      sync_prefs_.ClearLocalSyncTransportData();
     }
     return;
   }
@@ -681,6 +680,10 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
     auth_manager_->ConnectionClosed();
   }
 
+  if (reason == ShutdownReason::DISABLE_SYNC) {
+    sync_prefs_.ClearLocalSyncTransportData();
+  }
+
   NotifyObservers();
 }
 
@@ -692,11 +695,15 @@ void ProfileSyncService::StopImpl(SyncStopDataFate data_fate) {
     case CLEAR_DATA:
       ClearUnrecoverableError();
       ShutdownImpl(DISABLE_SYNC);
-      // Clear prefs (including SyncSetupHasCompleted) before shutting down so
-      // PSS clients don't think we're set up while we're shutting down.
-      // Note: We do this after shutting down, so that notifications about the
-      // changed pref values don't mess up our state.
-      sync_prefs_.ClearPreferences();
+      // Note: ShutdownImpl(DISABLE_SYNC) does *not* clear prefs which are
+      // directly user-controlled such as the set of selected types here, so
+      // that if the user ever chooses to enable Sync again, they start off
+      // with their previous settings by default. We do however require going
+      // through first-time setup again.
+      sync_prefs_.ClearFirstSetupComplete();
+      // For explicit passphrase users, clear the encryption key, such that they
+      // will need to reenter it if sync gets re-enabled.
+      sync_prefs_.ClearEncryptionBootstrapToken();
       break;
   }
 }
@@ -856,15 +863,6 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
 
   // Shut all data types down.
   ShutdownImpl(DISABLE_SYNC);
-
-  // This is the equivalent for Directory::DeleteDirectoryFiles(), guaranteed
-  // to be called, either directly in ShutdownImpl(), or later in
-  // SyncEngineBackend::DoShutdown().
-  // TODO(crbug.com/923285): This doesn't seem to belong here, or if it does,
-  // all preferences should be cleared via SyncPrefs::ClearPreferences(),
-  // which is done by some of the callers (but not all). Care must be taken
-  // however for scenarios like custom passphrase being set.
-  sync_prefs_.ClearDirectoryConsistencyPreferences();
 }
 
 void ProfileSyncService::DataTypePreconditionChanged(ModelType type) {
@@ -1062,27 +1060,9 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
       // restart.
       sync_disabled_by_admin_ = true;
       ShutdownImpl(DISABLE_SYNC);
-      // This is the equivalent for Directory::DeleteDirectoryFiles(),
-      // guaranteed to be called, either directly in ShutdownImpl(), or later in
-      // SyncEngineBackend::DoShutdown().
-      // TODO(crbug.com/923285): This doesn't seem to belong here, or if it
-      // does, all preferences should be cleared via
-      // SyncPrefs::ClearPreferences(), which is done by some of the callers
-      // (but not all). Care must be taken however for scenarios like custom
-      // passphrase being set.
-      sync_prefs_.ClearDirectoryConsistencyPreferences();
       break;
     case RESET_LOCAL_SYNC_DATA:
       ShutdownImpl(DISABLE_SYNC);
-      // This is the equivalent for Directory::DeleteDirectoryFiles(),
-      // guaranteed to be called, either directly in ShutdownImpl(), or later in
-      // SyncEngineBackend::DoShutdown().
-      // TODO(crbug.com/923285): This doesn't seem to belong here, or if it
-      // does, all preferences should be cleared via
-      // SyncPrefs::ClearPreferences(), which is done by some of the callers
-      // (but not all). Care must be taken however for scenarios like custom
-      // passphrase being set.
-      sync_prefs_.ClearDirectoryConsistencyPreferences();
       startup_controller_->TryStart(/*force_immediate=*/true);
       break;
     case UNKNOWN_ACTION:

components/sync/driver/profile_sync_service_unittest.cc

@@ -54,6 +54,7 @@ namespace {
 constexpr int kOldEnoughForDemographicsUserBirthYear = 1983;
 
 constexpr char kTestUser[] = "test_user@gmail.com";
+constexpr char kTestCacheGuid[] = "test_cache_guid";
 
 // Now time in string format.
 constexpr char kNowTimeInStringFormat[] = "23 Mar 2019 16:00:00 UDT";
@@ -231,6 +232,8 @@ class ProfileSyncServiceTest : public ::testing::Test {
   void InitializeForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
     SyncPrefs sync_prefs(prefs());
+    sync_prefs.SetCacheGuid(kTestCacheGuid);
+    sync_prefs.SetBirthday(FakeSyncEngine::kTestBirthday);
     sync_prefs.SetLastSyncedTime(base::Time::Now());
     sync_prefs.SetSyncRequested(true);
     sync_prefs.SetSelectedTypes(
@@ -387,13 +390,18 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   CreateService(ProfileSyncService::MANUAL_START);
 
   SyncPrefs sync_prefs(prefs());
-  base::Time now = base::Time::Now();
-  sync_prefs.SetLastSyncedTime(now);
   sync_prefs.SetSyncRequested(true);
   sync_prefs.SetSelectedTypes(
       /*keep_everything_synced=*/true,
       /*registered_types=*/UserSelectableTypeSet::All(),
       /*selected_types=*/UserSelectableTypeSet::All());
+
+  // Mimic a sync cycle (transport-only) having completed earlier.
+  const base::Time kLastSyncedTime = base::Time::Now();
+  sync_prefs.SetLastSyncedTime(kLastSyncedTime);
+  sync_prefs.SetCacheGuid(kTestCacheGuid);
+  sync_prefs.SetBirthday(FakeSyncEngine::kTestBirthday);
+
   service()->Initialize();
 
   EXPECT_EQ(SyncService::DisableReasonSet(), service()->GetDisableReasons());
@@ -404,10 +412,9 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   EXPECT_FALSE(service()->IsSyncFeatureActive());
   EXPECT_FALSE(service()->IsSyncFeatureEnabled());
 
-  // The last sync time shouldn't be cleared.
-  // TODO(zea): figure out a way to check that the directory itself wasn't
-  // cleared.
-  EXPECT_EQ(now, sync_prefs.GetLastSyncedTime());
+  // The local sync data shouldn't be cleared.
+  EXPECT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
+  EXPECT_EQ(kLastSyncedTime, sync_prefs.GetLastSyncedTime());
 }
 
 // Verify that the SetSetupInProgress function call updates state