Persist account ID in sync metadata

This helps datatypes that are interested in the syncing account ID,
without having to wait until sync engine starting (which is usually
deferred for about 10 seconds).

This field is convenient for user consents, although leveraging this
data is not included in this patch.

A dedicated method is introduced in ModelTypeChangeProcessor although it
is somewhat redundant with IsTrackingMetadata(). The rationale is most
datatypes don't care so it seems overkill to update all tests with extra
boilerplate.

Bug: 830535
Change-Id: I623b87c693894b5c0845fc3d03772b2701292a00
Reviewed-on: https://chromium-review.googlesource.com/1138236
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593519}

components/sync/model/fake_model_type_change_processor.cc

@@ -56,6 +56,10 @@ bool FakeModelTypeChangeProcessor::IsTrackingMetadata() {
   return true;
 }
 
+std::string FakeModelTypeChangeProcessor::TrackedAccountId() {
+  return "";
+}
+
 void FakeModelTypeChangeProcessor::ReportError(const ModelError& error) {
   EXPECT_TRUE(expect_error_) << error.ToString();
   expect_error_ = false;

components/sync/model/fake_model_type_change_processor.h

@@ -40,6 +40,7 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
+  std::string TrackedAccountId() override;
   void ReportError(const ModelError& error) override;
   base::WeakPtr<ModelTypeControllerDelegate> GetControllerDelegate() override;
 

components/sync/model/mock_model_type_change_processor.cc

@@ -58,6 +58,8 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
 
   bool IsTrackingMetadata() override { return other_->IsTrackingMetadata(); }
 
+  std::string TrackedAccountId() override { return other_->TrackedAccountId(); }
+
   void ReportError(const ModelError& error) override {
     other_->ReportError(error);
   }
@@ -114,6 +116,9 @@ void MockModelTypeChangeProcessor::DelegateCallsByDefaultTo(
   ON_CALL(*this, IsTrackingMetadata())
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::IsTrackingMetadata));
+  ON_CALL(*this, TrackedAccountId())
+      .WillByDefault(
+          Invoke(delegate, &ModelTypeChangeProcessor::TrackedAccountId));
   ON_CALL(*this, ReportError(_))
       .WillByDefault(Invoke(delegate, &ModelTypeChangeProcessor::ReportError));
   ON_CALL(*this, GetControllerDelegate())

components/sync/model/mock_model_type_change_processor.h

@@ -36,6 +36,7 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   MOCK_METHOD1(ModelReadyToSync, void(std::unique_ptr<MetadataBatch> batch));
   MOCK_METHOD0(IsTrackingMetadata, bool());
+  MOCK_METHOD0(TrackedAccountId, std::string());
   MOCK_METHOD1(ReportError, void(const ModelError& error));
   MOCK_METHOD0(GetControllerDelegate,
                base::WeakPtr<ModelTypeControllerDelegate>());

components/sync/model/model_type_change_processor.h

@@ -83,6 +83,10 @@ class ModelTypeChangeProcessor {
   // will no-op and can be omitted by bridge.
   virtual bool IsTrackingMetadata() = 0;
 
+  // Returns the account ID for which metadata is being tracked, or empty if not
+  // tracking metadata.
+  virtual std::string TrackedAccountId() = 0;
+
   // Report an error in the model to sync. Should be called for any persistence
   // or consistency error the bridge encounters outside of a method that allows
   // returning a ModelError directly. Outstanding callbacks are not expected to

components/sync/model/recording_model_type_change_processor.cc

@@ -57,6 +57,10 @@ bool RecordingModelTypeChangeProcessor::IsTrackingMetadata() {
   return is_tracking_metadata_;
 }
 
+std::string RecordingModelTypeChangeProcessor::TrackedAccountId() {
+  return "";
+}
+
 void RecordingModelTypeChangeProcessor::SetIsTrackingMetadata(
     bool is_tracking) {
   is_tracking_metadata_ = is_tracking;

components/sync/model/recording_model_type_change_processor.h

@@ -35,6 +35,7 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
+  std::string TrackedAccountId() override;
 
   void SetIsTrackingMetadata(bool is_tracking);
 

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -171,7 +171,7 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
     model_type_state_.set_cache_guid(activation_request_.cache_guid);
   } else if (model_type_state_.cache_guid() != activation_request_.cache_guid) {
     // There is a mismatch between the cache guid stored in |model_type_state_|
-    // and the one received from sync and stored it |activation_request|. This
+    // and the one received from sync and stored it |activation_request_|. This
     // indicates that the stored metadata are invalid (e.g. has been
     // manipulated) and don't belong to the current syncing client.
     const ModelTypeSyncBridge::StopSyncResponse response =
@@ -197,6 +197,10 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
     bridge_->OnSyncStarting(activation_request_);
   }
 
+  // Cache GUID verification guarantees the user is the same.
+  model_type_state_.set_authenticated_account_id(
+      activation_request_.authenticated_account_id);
+
   auto activation_response = std::make_unique<DataTypeActivationResponse>();
   activation_response->model_type_state = model_type_state_;
   activation_response->type_processor =
@@ -281,6 +285,17 @@ bool ClientTagBasedModelTypeProcessor::IsTrackingMetadata() {
   return model_type_state_.initial_sync_done();
 }
 
+std::string ClientTagBasedModelTypeProcessor::TrackedAccountId() {
+  // Returning non-empty here despite !IsTrackingMetadata() has weird semantics,
+  // e.g. initial updates are being fetched but we haven't received the response
+  // (i.e. prior to exercising MergeSyncData()). Let's be cautious and hide the
+  // account ID.
+  if (!IsTrackingMetadata()) {
+    return "";
+  }
+  return model_type_state_.authenticated_account_id();
+}
+
 void ClientTagBasedModelTypeProcessor::ReportError(const ModelError& error) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -69,6 +69,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
+  std::string TrackedAccountId() override;
   void ReportError(const ModelError& error) override;
   base::WeakPtr<ModelTypeControllerDelegate> GetControllerDelegate() override;
 

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -225,13 +225,14 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   void OnCommitDataLoaded() { bridge()->OnCommitDataLoaded(); }
 
-  void OnSyncStarting() {
+  void OnSyncStarting(
+      const std::string& authenticated_account_id = "SomeAccountId") {
     DataTypeActivationRequest request;
     request.error_handler = base::BindRepeating(
         &ClientTagBasedModelTypeProcessorTest::ErrorReceived,
         base::Unretained(this));
     request.cache_guid = "TestCacheGuid";
-    request.authenticated_account_id = "SomeAccountId";
+    request.authenticated_account_id = authenticated_account_id;
     type_processor()->OnSyncStarting(
         request,
         base::BindOnce(&ClientTagBasedModelTypeProcessorTest::OnReadyToConnect,
@@ -367,6 +368,32 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
   bool expect_error_ = false;
 };
 
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldExposeNewlyTrackedAccountId) {
+  ModelReadyToSync();
+  ASSERT_EQ("", type_processor()->TrackedAccountId());
+  OnSyncStarting("SomeAccountId");
+  worker()->UpdateFromServer();
+  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
+}
+
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldExposePreviouslyTrackedAccountId) {
+  std::unique_ptr<MetadataBatch> metadata_batch = db().CreateMetadataBatch();
+  sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
+  model_type_state.set_initial_sync_done(true);
+  model_type_state.set_authenticated_account_id("PersistedAccountId");
+  metadata_batch->SetModelTypeState(model_type_state);
+  type_processor()->ModelReadyToSync(std::move(metadata_batch));
+
+  // Even prior to starting sync, the account ID should already be tracked.
+  EXPECT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
+
+  // If sync gets started, the account should still be tracked.
+  OnSyncStarting("PersistedAccountId");
+  EXPECT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
+}
+
 // Test that an initial sync handles local and remote items properly.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldMergeLocalAndRemoteChanges) {
   ModelReadyToSync();
@@ -2035,6 +2062,31 @@ class CommitOnlyClientTagBasedModelTypeProcessorTest
   bool IsCommitOnly() override { return true; }
 };
 
+TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
+       ShouldExposeNewlyTrackedAccountId) {
+  ModelReadyToSync();
+  ASSERT_EQ("", type_processor()->TrackedAccountId());
+  OnSyncStarting("SomeAccountId");
+  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
+}
+
+TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
+       ShouldExposePreviouslyTrackedAccountId) {
+  std::unique_ptr<MetadataBatch> metadata_batch = db().CreateMetadataBatch();
+  sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
+  model_type_state.set_initial_sync_done(true);
+  model_type_state.set_authenticated_account_id("PersistedAccountId");
+  metadata_batch->SetModelTypeState(model_type_state);
+  type_processor()->ModelReadyToSync(std::move(metadata_batch));
+
+  // Even prior to starting sync, the account ID should already be tracked.
+  EXPECT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
+
+  // If sync gets started, the account should still be tracked.
+  OnSyncStarting("PersistedAccountId");
+  EXPECT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
+}
+
 // Test that commit only types are deleted after commit response.
 TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
        ShouldCommitAndDeleteWhenAcked) {

components/sync/protocol/model_type_state.proto

@@ -37,4 +37,7 @@ message ModelTypeState {
   // persisted sync metadata, because cache_guid is reset when sync is disabled,
   // and disabling sync is supposed to clear sync metadata.
   optional string cache_guid = 5;
+
+  // Syncing account ID, representing the user.
+  optional string authenticated_account_id = 6;
 }