Verify consistency between sync and sign-in account IDs

Local sync metadata belongs to one user account. Due to the distributed
nature of the locally persisted sync metadata, the cache GUID is used
as "epoch" to detect mismatched in edge cases like the browser crashing
during shutdown and before I/O gets flushed.

However, prior to this patch, the cache GUID itself has no safety
mechanism to detect it maps to the intended account ID. In this patch,
a new SyncPref is introduced to achieve that, in away that both prefs
(cache GUID and account ID) are stored atomically.

Because the pref is newly-introduced, migration logic is introduced to
populate it for the first time if initially empty.

Change-Id: I2cdd9f997077c4acd16e9283df8c025f51d40546
Bug: 1046237,1021527
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023528Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736848}

components/sync/base/pref_names.cc

@@ -77,6 +77,7 @@ const char kSyncEncryptionBootstrapToken[] = "sync.encryption_bootstrap_token";
 const char kSyncKeystoreEncryptionBootstrapToken[] =
     "sync.keystore_encryption_bootstrap_token";
 
+const char kSyncGaiaId[] = "sync.gaia_id";
 const char kSyncCacheGuid[] = "sync.cache_guid";
 const char kSyncBirthday[] = "sync.birthday";
 const char kSyncBagOfChips[] = "sync.bag_of_chips";

components/sync/base/pref_names.h

@@ -42,6 +42,7 @@ extern const char kSyncRequested[];
 extern const char kSyncEncryptionBootstrapToken[];
 extern const char kSyncKeystoreEncryptionBootstrapToken[];
 
+extern const char kSyncGaiaId[];
 extern const char kSyncCacheGuid[];
 extern const char kSyncBirthday[];
 extern const char kSyncBagOfChips[];

components/sync/base/sync_prefs.cc

@@ -298,6 +298,7 @@ void SyncPrefs::RegisterProfilePrefs(
                                std::string());
 
   // Internal or bookkeeping prefs.
+  registry->RegisterStringPref(prefs::kSyncGaiaId, std::string());
   registry->RegisterStringPref(prefs::kSyncCacheGuid, std::string());
   registry->RegisterStringPref(prefs::kSyncBirthday, std::string());
   registry->RegisterStringPref(prefs::kSyncBagOfChips, std::string());
@@ -366,6 +367,7 @@ void SyncPrefs::ClearLocalSyncTransportData() {
   pref_service_->ClearPref(prefs::kSyncPassphrasePrompted);
   pref_service_->ClearPref(prefs::kSyncInvalidationVersions);
   pref_service_->ClearPref(prefs::kSyncLastRunVersion);
+  pref_service_->ClearPref(prefs::kSyncGaiaId);
   pref_service_->ClearPref(prefs::kSyncCacheGuid);
   pref_service_->ClearPref(prefs::kSyncBirthday);
   pref_service_->ClearPref(prefs::kSyncBagOfChips);
@@ -628,6 +630,15 @@ void SyncPrefs::RegisterTypeSelectedPref(
   registry->RegisterBooleanPref(pref_name, false);
 }
 
+void SyncPrefs::SetGaiaId(const std::string& gaia_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  pref_service_->SetString(prefs::kSyncGaiaId, gaia_id);
+}
+
+std::string SyncPrefs::GetGaiaId() const {
+  return pref_service_->GetString(prefs::kSyncGaiaId);
+}
+
 void SyncPrefs::SetCacheGuid(const std::string& cache_guid) {
   pref_service_->SetString(prefs::kSyncCacheGuid, cache_guid);
 }

components/sync/base/sync_prefs.h

@@ -152,6 +152,8 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Maps |type| to its corresponding preference name.
   static const char* GetPrefNameForType(UserSelectableType type);
 
+  void SetGaiaId(const std::string& gaia_id);
+  std::string GetGaiaId() const;
   void SetCacheGuid(const std::string& cache_guid);
   std::string GetCacheGuid() const;
   void SetBirthday(const std::string& birthday);

components/sync/driver/profile_sync_service.cc

@@ -160,6 +160,38 @@ std::string GenerateCacheGUID() {
   return guid;
 }
 
+bool IsLocalSyncTransportDataValid(const SyncPrefs& sync_prefs,
+                                   const CoreAccountInfo& core_account_info) {
+  // If the cache GUID is empty, it most probably is because local sync data
+  // has been fully cleared via ClearLocalSyncTransportData() due to
+  // ShutdownReason::DISABLE_SYNC. Let's return false here anyway to make sure
+  // all prefs are cleared and a new random cache GUID generated.
+  if (sync_prefs.GetCacheGuid().empty()) {
+    return false;
+  }
+
+  // If cache GUID is initialized but the birthday isn't, it means the first
+  // sync cycle never completed (OnEngineInitialized()). This should be a rare
+  // case and theoretically harmless to resume, but as safety precaution, its
+  // simpler to regenerate the cache GUID and start from scratch, to avoid
+  // protocol violations (fetching updates requires that the request either has
+  // a birthday, or there should be no progress marker).
+  if (sync_prefs.GetBirthday().empty()) {
+    return false;
+  }
+
+  // Make sure the cached account information (gaia ID) is equal to the current
+  // one (otherwise the data may be corrupt). Note that, for local sync
+  // (IsLocalSyncEnabled()), the authenticated account is always empty.
+  if (sync_prefs.GetGaiaId() != core_account_info.gaia) {
+    DLOG(WARNING) << "Found mismatching gaia ID in sync preferences";
+    return false;
+  }
+
+  // All good: local sync data looks initialized and valid.
+  return true;
+}
+
 }  // namespace
 
 ProfileSyncService::InitParams::InitParams() = default;
@@ -502,6 +534,22 @@ void ProfileSyncService::InitializeBackendTaskRunnerIfNeeded() {
 void ProfileSyncService::StartUpSlowEngineComponents() {
   DCHECK(IsEngineAllowedToStart());
 
+  const CoreAccountInfo authenticated_account_info =
+      GetAuthenticatedAccountInfo();
+
+  if (IsLocalSyncEnabled()) {
+    // With local sync (roaming profiles) there is no identity manager and hence
+    // |authenticated_account_info| is empty. This is required for
+    // IsLocalSyncTransportDataValid() to work properly.
+    DCHECK(authenticated_account_info.gaia.empty());
+    DCHECK(authenticated_account_info.account_id.empty());
+  } else {
+    // Except for local sync (roaming profiles), the user must be signed in for
+    // sync to start.
+    DCHECK(!authenticated_account_info.gaia.empty());
+    DCHECK(!authenticated_account_info.account_id.empty());
+  }
+
   engine_ = sync_client_->GetSyncApiComponentFactory()->CreateSyncEngine(
       debug_identifier_, sync_client_->GetInvalidationService(),
       sync_prefs_.AsWeakPtr());
@@ -511,15 +559,22 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
     last_actionable_error_ = SyncProtocolError();
   }
 
-  // If either the cache GUID or the birthday are uninitialized, it means we
-  // haven't completed a first sync cycle (OnEngineInitialized()). Theoretically
-  // both or neither should be empty, but just in case, we regenerate the cache
-  // GUID if either of them is missing, to avoid protocol violations (fetching
-  // 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()) {
+  // The gaia ID in SyncPrefs was introduced with M81, so having an empty value
+  // is legitimate and should be populated as a one-off migration.
+  // TODO(mastiz): Clean up this migration code after a grace period (e.g. 1
+  // year).
+  if (sync_prefs_.GetGaiaId().empty()) {
+    sync_prefs_.SetGaiaId(authenticated_account_info.gaia);
+  }
+
+  if (!IsLocalSyncTransportDataValid(sync_prefs_, authenticated_account_info)) {
+    // Either the local data is uninitialized or corrupt, so let's throw
+    // everything away and start from scratch with a new cache GUID, which also
+    // cascades into datatypes throwing away their dangling sync metadata due to
+    // cache GUID mismatches.
     sync_prefs_.ClearLocalSyncTransportData();
     sync_prefs_.SetCacheGuid(GenerateCacheGUID());
+    sync_prefs_.SetGaiaId(authenticated_account_info.gaia);
   }
 
   InitializeBackendTaskRunnerIfNeeded();
@@ -539,8 +594,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   params.http_factory_getter = base::BindOnce(
       create_http_post_provider_factory_cb_, MakeUserAgentForSync(channel_),
       url_loader_factory_->Clone(), network_time_update_callback_);
-  params.authenticated_account_id = GetAuthenticatedAccountInfo().account_id;
-  DCHECK(!params.authenticated_account_id.empty() || IsLocalSyncEnabled());
+  params.authenticated_account_id = authenticated_account_info.account_id;
   if (!base::FeatureList::IsEnabled(switches::kSyncE2ELatencyMeasurement)) {
     invalidation::InvalidationService* invalidator =
         sync_client_->GetInvalidationService();

components/sync/driver/profile_sync_service_unittest.cc

@@ -229,7 +229,7 @@ class ProfileSyncServiceTest : public ::testing::Test {
     service_.reset();
   }
 
-  void InitializeForNthSync() {
+  void PopulatePrefsForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
     SyncPrefs sync_prefs(prefs());
     sync_prefs.SetCacheGuid(kTestCacheGuid);
@@ -241,6 +241,10 @@ class ProfileSyncServiceTest : public ::testing::Test {
         /*registered_types=*/UserSelectableTypeSet::All(),
         /*selected_types=*/UserSelectableTypeSet::All());
     sync_prefs.SetFirstSetupComplete();
+  }
+
+  void InitializeForNthSync() {
+    PopulatePrefsForNthSync();
     service_->Initialize();
   }
 
@@ -1564,5 +1568,53 @@ TEST_F(ProfileSyncServiceTestWithStopSyncInPausedState,
   EXPECT_FALSE(service()->GetDisableReasons().Empty());
 }
 
+TEST_F(ProfileSyncServiceTest, ShouldPopulateAccountIdCachedInPrefs) {
+  SyncPrefs sync_prefs(prefs());
+
+  SignIn();
+  CreateService(ProfileSyncService::AUTO_START);
+  InitializeForNthSync();
+  ASSERT_EQ(SyncService::TransportState::ACTIVE,
+            service()->GetTransportState());
+  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
+  EXPECT_EQ(signin::GetTestGaiaIdForEmail(kTestUser), sync_prefs.GetGaiaId());
+}
+
+TEST_F(ProfileSyncServiceTest,
+       ShouldNotPopulateAccountIdCachedInPrefsWithLocalSync) {
+  SyncPrefs sync_prefs(prefs());
+
+  SignIn();
+  CreateServiceWithLocalSyncBackend();
+  InitializeForNthSync();
+  ASSERT_EQ(SyncService::TransportState::ACTIVE,
+            service()->GetTransportState());
+  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
+  EXPECT_TRUE(sync_prefs.GetGaiaId().empty());
+}
+
+// Verifies that local sync transport data is thrown away if there is a mismatch
+// between the account ID cached in SyncPrefs and the actual one.
+TEST_F(ProfileSyncServiceTest,
+       ShouldClearLocalSyncTransportDataDueToAccountIdMismatch) {
+  SyncPrefs sync_prefs(prefs());
+
+  SignIn();
+  CreateService(ProfileSyncService::AUTO_START);
+  PopulatePrefsForNthSync();
+  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
+
+  // Manually override the authenticated account ID, which should be detected
+  // during initialization.
+  sync_prefs.SetGaiaId("corrupt_gaia_id");
+
+  service()->Initialize();
+
+  ASSERT_EQ(SyncService::TransportState::ACTIVE,
+            service()->GetTransportState());
+  EXPECT_NE(kTestCacheGuid, sync_prefs.GetCacheGuid());
+  EXPECT_EQ(signin::GetTestGaiaIdForEmail(kTestUser), sync_prefs.GetGaiaId());
+}
+
 }  // namespace
 }  // namespace syncer