[Sync::USS] Add cache_guid to model_type_state.proto

In preparation for detecting a mismatch between the running sync client
cache guid, and the one persisted in the metadata, a new field is
introduced in model_type_state.proto such that the cache_guid is
persisted togethter with other model type state fields.

Mismatch detection will be used to prevent leak of sync data between
different accounts syncing on the same device.

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

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -147,17 +147,25 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
 
   if (model_error_) {
     activation_request_.error_handler.Run(model_error_.value());
-  } else {
-    auto activation_response = std::make_unique<DataTypeActivationResponse>();
-    activation_response->model_type_state = model_type_state_;
-    activation_response->type_processor =
-        std::make_unique<ModelTypeProcessorProxy>(
-            weak_ptr_factory_for_worker_.GetWeakPtr(),
-            base::ThreadTaskRunnerHandle::Get());
-    std::move(start_callback_).Run(std::move(activation_response));
+    start_callback_.Reset();
+    return;
   }
 
-  start_callback_.Reset();
+  if (!model_type_state_.has_cache_guid()) {
+    model_type_state_.set_cache_guid(activation_request_.cache_guid);
+  }
+  // TODO(crbug.com/820049): Check if there is a mismatch between the retrieved
+  //   cache guid and stored in |model_type_state_| and the one received from
+  //   sync and stored it |activation_request|. Note that many clients will have
+  //   an empty proto field, since the field was introduced recently.
+
+  auto activation_response = std::make_unique<DataTypeActivationResponse>();
+  activation_response->model_type_state = model_type_state_;
+  activation_response->type_processor =
+      std::make_unique<ModelTypeProcessorProxy>(
+          weak_ptr_factory_for_worker_.GetWeakPtr(),
+          base::ThreadTaskRunnerHandle::Get());
+  std::move(start_callback_).Run(std::move(activation_response));
 }
 
 bool ClientTagBasedModelTypeProcessor::IsConnected() const {

components/sync/protocol/model_type_state.proto

@@ -30,4 +30,11 @@ message ModelTypeState {
   // ModelTypeProcessor should not attempt to commit any items until this
   // flag is set.
   optional bool initial_sync_done = 4;
+
+  // A GUID that identifies the committing sync client. It's persisted within
+  // the sync metadata and should be used to check the integrity of the
+  // metadata. Mismatches with the guid of the running client indicates invalid
+  // 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;
 }

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -14,6 +14,7 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/engine/commit_queue.h"
 #include "components/sync/engine/model_type_processor_proxy.h"
+#include "components/sync/model/data_type_activation_request.h"
 #include "components/sync/protocol/bookmark_model_metadata.pb.h"
 #include "components/sync_bookmarks/bookmark_model_observer_impl.h"
 #include "components/undo/bookmark_undo_utils.h"
@@ -195,11 +196,11 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
 
   if (!bookmark_tracker_) {
     // TODO(crbug.com/516866): Implement the merge logic.
-    auto model_type_state = std::make_unique<sync_pb::ModelTypeState>();
-    model_type_state->set_initial_sync_done(true);
+    auto state = std::make_unique<sync_pb::ModelTypeState>(model_type_state);
+    state->set_cache_guid(cache_guid_);
     std::vector<NodeMetadataPair> nodes_metadata;
     bookmark_tracker_ = std::make_unique<SyncedBookmarkTracker>(
-        std::move(nodes_metadata), std::move(model_type_state));
+        std::move(nodes_metadata), std::move(state));
   }
   // TODO(crbug.com/516866): Set the model type state.
 
@@ -540,7 +541,9 @@ void BookmarkModelTypeProcessor::OnSyncStarting(
   DCHECK(start_callback);
   DVLOG(1) << "Sync is starting for Bookmarks";
 
+  cache_guid_ = request.cache_guid;
   start_callback_ = std::move(start_callback);
+  DCHECK(!cache_guid_.empty());
   ConnectIfReady();
 }
 
@@ -554,14 +557,29 @@ void BookmarkModelTypeProcessor::ConnectIfReady() {
     return;
   }
 
+  DCHECK(!cache_guid_.empty());
+
+  if (bookmark_tracker_ &&
+      bookmark_tracker_->model_type_state().cache_guid() != cache_guid_) {
+    // TODO(crbug.com/820049): Properly handle a mismatch between the loaded
+    // cache guid stored in |bookmark_tracker_.model_type_state_| at
+    // DecodeSyncMetadata() and the one received from sync at OnSyncStarting()
+    // stored in |cache_guid_|.
+    return;
+  }
+
   auto activation_context =
       std::make_unique<syncer::DataTypeActivationResponse>();
-  // TODO(crbug.com/516866): Read the model type state from persisted sync
-  // metadata instead of feeding an empty one.
-  sync_pb::ModelTypeState model_type_state;
-  model_type_state.mutable_progress_marker()->set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(syncer::BOOKMARKS));
-  activation_context->model_type_state = model_type_state;
+  if (bookmark_tracker_) {
+    activation_context->model_type_state =
+        bookmark_tracker_->model_type_state();
+  } else {
+    sync_pb::ModelTypeState model_type_state;
+    model_type_state.mutable_progress_marker()->set_data_type_id(
+        GetSpecificsFieldNumberFromModelType(syncer::BOOKMARKS));
+    model_type_state.set_cache_guid(cache_guid_);
+    activation_context->model_type_state = model_type_state;
+  }
   activation_context->type_processor =
       std::make_unique<syncer::ModelTypeProcessorProxy>(
           weak_ptr_factory_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get());
@@ -571,6 +589,7 @@ void BookmarkModelTypeProcessor::ConnectIfReady() {
 void BookmarkModelTypeProcessor::OnSyncStopping(
     syncer::SyncStopMetadataFate metadata_fate) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  cache_guid_.clear();
   NOTIMPLEMENTED();
 }
 

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -166,6 +166,10 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // called during startup, as part of the bookmark-loading process.
   std::unique_ptr<SyncedBookmarkTracker> bookmark_tracker_;
 
+  // GUID string that identifies the sync client and is received from the sync
+  // engine.
+  std::string cache_guid_;
+
   std::unique_ptr<BookmarkModelObserverImpl> bookmark_model_observer_;
 
   base::WeakPtrFactory<BookmarkModelTypeProcessor> weak_ptr_factory_;

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -70,6 +70,7 @@ class SyncedBookmarkTracker {
     DISALLOW_COPY_AND_ASSIGN(Entity);
   };
 
+  // |model_type_state| must not be null.
   SyncedBookmarkTracker(
       std::vector<NodeMetadataPair> nodes_metadata,
       std::unique_ptr<sync_pb::ModelTypeState> model_type_state);
@@ -107,6 +108,10 @@ class SyncedBookmarkTracker {
   // Returns true if there are any local entities to be committed.
   bool HasLocalChanges() const;
 
+  const sync_pb::ModelTypeState& model_type_state() const {
+    return *model_type_state_;
+  }
+
   // Returns number of tracked entities. Used only in test.
   std::size_t TrackedEntitiesCountForTest() const;