[Sync:USS] Handle new encryption requirements for bookmarks updates

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

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

@@ -1987,6 +1987,38 @@ IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
   ASSERT_TRUE(BookmarksMatchChecker().Wait());
 }
 
+// Enable enccryption and then trigger the server side creation of Synced
+// Bookmarks. Ensure both clients remain syncing afterwards. Add bookmarks to
+// the synced bookmarks folder and ensure both clients receive the bookmark.
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
+                       CreateSyncedBookmarksWithSingleClientEnableEncryption) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  // Enable the encryption on Client 0.
+  ASSERT_TRUE(EnableEncryption(0));
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+
+  fake_server_->InjectEntity(syncer::PersistentPermanentEntity::CreateNew(
+      syncer::BOOKMARKS, "synced_bookmarks", "Synced Bookmarks",
+      "google_chrome_bookmarks"));
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+
+  // Add a bookmark on Client 0 and ensure it syncs over. This will also trigger
+  // both clients downloading the new Synced Bookmarks folder.
+  ASSERT_NE(nullptr, AddURL(0, "Google", GURL("http://www.google.com")));
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+
+  // Now add a bookmark within the Synced Bookmarks folder and ensure it syncs
+  // over.
+  const BookmarkNode* synced_bookmarks = GetSyncedBookmarksNode(0);
+  ASSERT_TRUE(synced_bookmarks);
+  ASSERT_NE(nullptr, AddURL(0, synced_bookmarks, 0, "Google2",
+                            GURL("http://www.google2.com")));
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+}
+
 IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
                        BookmarkAllNodesRemovedEvent) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -179,7 +179,10 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
       bookmark_model_, bookmark_undo_service_, bookmark_model_observer_.get());
   BookmarkRemoteUpdatesHandler updates_handler(
       bookmark_model_, favicon_service_, bookmark_tracker_.get());
-  updates_handler.Process(updates);
+  const bool got_new_encryption_requirements =
+      bookmark_tracker_->model_type_state().encryption_key_name() !=
+      model_type_state.encryption_key_name();
+  updates_handler.Process(updates, got_new_encryption_requirements);
   bookmark_tracker_->set_model_type_state(
       std::make_unique<sync_pb::ModelTypeState>(model_type_state));
   // Schedule save just in case one is needed.

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -7,6 +7,7 @@
 #include <set>
 #include <string>
 #include <unordered_map>
+#include <unordered_set>
 #include <utility>
 
 #include "base/metrics/histogram_macros.h"
@@ -131,7 +132,13 @@ BookmarkRemoteUpdatesHandler::BookmarkRemoteUpdatesHandler(
 }
 
 void BookmarkRemoteUpdatesHandler::Process(
-    const syncer::UpdateResponseDataList& updates) {
+    const syncer::UpdateResponseDataList& updates,
+    bool got_new_encryption_requirements) {
+  // If new encryption requirements come from the server, the entities that are
+  // in |updates| will be recorded here so they can be ignored during the
+  // re-encryption phase at the end.
+  std::unordered_set<std::string> entities_with_up_to_date_encryption;
+
   for (const syncer::UpdateResponseData* update : ReorderUpdates(&updates)) {
     const syncer::EntityData& update_entity = update->entity.value();
     // Only non deletions and non premanent node should have valid specifics and
@@ -182,26 +189,64 @@ void BookmarkRemoteUpdatesHandler::Process(
       tracked_entity = bookmark_tracker_->GetEntityForSyncId(update_entity.id);
     }
 
-    // TODO(crbug.com/516866): Handle the case of conflict as a result of
-    // re-encryption request.
     if (tracked_entity && tracked_entity->IsUnsynced()) {
       ProcessConflict(*update, tracked_entity);
-      continue;
-    }
-    if (update_entity.is_deleted()) {
+    } else if (update_entity.is_deleted()) {
       ProcessDelete(update_entity, tracked_entity);
+      // If the local entity has been deleted, no need to check for out of date
+      // encryption. Therefore, we can go ahead and process the next update.
       continue;
-    }
-    if (!tracked_entity) {
+    } else if (!tracked_entity) {
       ProcessCreate(*update);
-      continue;
+      // Because the Synced Bookmarks node can be created server side, it's
+      // possible it'll arrive at the client as a creation. No need to check
+      // encryption for permanent folders.
+      if (update_entity.server_defined_unique_tag == kMobileBookmarksTag) {
+        continue;
+      }
+    } else {
+      // Ignore changes to the permanent nodes (e.g. bookmarks bar). We only
+      // care about their children.
+      if (bookmark_model_->is_permanent_node(tracked_entity->bookmark_node())) {
+        continue;
+      }
+      ProcessUpdate(*update, tracked_entity);
     }
-    // Ignore changes to the permanent nodes (e.g. bookmarks bar). We only care
-    // about their children.
-    if (bookmark_model_->is_permanent_node(tracked_entity->bookmark_node())) {
-      continue;
+    // If the received entity has out of date encryption, we schedule another
+    // commit to fix it.
+    if (bookmark_tracker_->model_type_state().encryption_key_name() !=
+        update->encryption_key_name) {
+      DVLOG(2) << "Bookmarks: Requesting re-encrypt commit "
+               << update->encryption_key_name << " -> "
+               << bookmark_tracker_->model_type_state().encryption_key_name();
+      bookmark_tracker_->IncrementSequenceNumber(update_entity.id);
+    }
+
+    if (got_new_encryption_requirements) {
+      entities_with_up_to_date_encryption.insert(update_entity.id);
+    }
+  }
+
+  // Recommit entities with out of date encryption.
+  if (got_new_encryption_requirements) {
+    std::vector<const SyncedBookmarkTracker::Entity*> all_entities =
+        bookmark_tracker_->GetAllEntities();
+    for (const SyncedBookmarkTracker::Entity* entity : all_entities) {
+      // No need to recommit tombstones and permanent nodes.
+      if (entity->metadata()->is_deleted()) {
+        continue;
+      }
+      DCHECK(entity->bookmark_node());
+      if (entity->bookmark_node()->is_permanent_node()) {
+        continue;
+      }
+      if (entities_with_up_to_date_encryption.count(
+              entity->metadata()->server_id()) != 0) {
+        continue;
+      }
+      bookmark_tracker_->IncrementSequenceNumber(
+          entity->metadata()->server_id());
     }
-    ProcessUpdate(*update, tracked_entity);
   }
 }
 
@@ -421,6 +466,9 @@ void BookmarkRemoteUpdatesHandler::ProcessConflict(
     const syncer::UpdateResponseData& update,
     const SyncedBookmarkTracker::Entity* tracked_entity) {
   const syncer::EntityData& update_entity = update.entity.value();
+  // TODO(crbug.com/516866): Handle the case of conflict as a result of
+  // re-encryption request.
+
   // TODO(crbug.com/516866): Add basic unit test for this function.
 
   // Can only conflict with existing nodes.
@@ -481,7 +529,7 @@ void BookmarkRemoteUpdatesHandler::ProcessConflict(
   const bookmarks::BookmarkNode* new_parent =
       new_parent_entity->bookmark_node();
   // |new_parent| would be null if the parent has been deleted locally and not
-  // committed yet. Deletions are excuted recusively, so a parent deletions
+  // committed yet. Deletions are executed recursively, so a parent deletions
   // entails child deletion, and if this child has been updated on another
   // client, this would cause conflict.
   if (!new_parent) {

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -33,8 +33,12 @@ class BookmarkRemoteUpdatesHandler {
                                favicon::FaviconService* favicon_service,
                                SyncedBookmarkTracker* bookmark_tracker);
   // Processes the updates received from the sync server in |updates| and
-  // updates the |bookmark_model_| and |bookmark_tracker_| accordingly.
-  void Process(const syncer::UpdateResponseDataList& updates);
+  // updates the |bookmark_model_| and |bookmark_tracker_| accordingly. If
+  // |got_new_encryption_requirements| is true, it recommits all tracked
+  // entities except those in |updates| which should use the latest encryption
+  // key and hence don't need recommitting.
+  void Process(const syncer::UpdateResponseDataList& updates,
+               bool got_new_encryption_requirements);
 
   // Public for testing.
   static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest(

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -218,7 +218,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
       CreatePermanentFolderUpdateData(kMobileBookmarksId, kMobileBookmarksTag));
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // Both the bookmark bar and mobile bookmarks nodes should be tracked.
   EXPECT_THAT(tracker.TrackedEntitiesCountForTest(), Eq(2U));
@@ -265,7 +265,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
 
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // All nodes should be tracked including the bookmark_bar.
   EXPECT_THAT(tracker.TrackedEntitiesCountForTest(), Eq(4U));
@@ -338,7 +338,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // |tracker| should be empty now.
   EXPECT_THAT(tracker.TrackedEntitiesCountForTest(), Eq(0U));
@@ -391,7 +391,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
 
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // All nodes should have been added to the model in the correct order.
   const bookmarks::BookmarkNode* bookmark_bar_node =
@@ -463,7 +463,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // Model should have been updated.
   ASSERT_THAT(bookmark_bar_node->child_count(), Eq(5));
@@ -530,7 +530,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // Model should have been updated.
   ASSERT_THAT(bookmark_bar_node->child_count(), Eq(5));
@@ -597,7 +597,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // Model should have been updated.
   ASSERT_THAT(bookmark_bar_node->child_count(), Eq(4));
@@ -655,7 +655,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   EXPECT_CALL(favicon_service,
               AddPageNoVisitForBookmark(kUrl, base::UTF8ToUTF16(kTitle)));
   EXPECT_CALL(favicon_service, MergeFavicon(kUrl, kIconUrl, _, _, _));
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 }
 
 TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
@@ -704,7 +704,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   EXPECT_CALL(favicon_service,
               DeleteFaviconMappings(ElementsAre(kUrl),
                                     favicon_base::IconType::kFavicon));
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 }
 
 // This tests the case when a local creation is successfully committed to the
@@ -764,7 +764,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
                                                &favicon_service, &tracker);
-  updates_handler.Process(updates);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
 
   // The sync id in the tracker should have been updated.
   EXPECT_THAT(tracker.GetEntityForSyncId(kOriginatorClientItemId), IsNull());
@@ -775,6 +775,112 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
 }
 
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
+     ShouldRecommitWhenEncryptionKeyNameMistmatch) {
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  auto model_type_state = std::make_unique<sync_pb::ModelTypeState>();
+  model_type_state->set_encryption_key_name("encryption_key_name");
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::move(model_type_state));
+
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
+  testing::NiceMock<favicon::MockFaviconService> favicon_service;
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(),
+                      &favicon_service, &tracker)
+      .Merge();
+
+  const std::string kId0 = "id0";
+  syncer::UpdateResponseDataList updates;
+  syncer::UpdateResponseData response_data =
+      CreateUpdateResponseData(/*server_id=*/kId0,
+                               /*parent_id=*/kBookmarkBarId,
+                               /*is_deletion=*/false);
+  response_data.encryption_key_name = "another_encryption_key_name";
+  updates.push_back(response_data);
+
+  BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
+                                               &favicon_service, &tracker);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  ASSERT_THAT(tracker.GetEntityForSyncId(kId0), NotNull());
+  EXPECT_THAT(tracker.GetEntityForSyncId(kId0)->IsUnsynced(), Eq(true));
+}
+
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
+     ShouldRecommitWhenGotNewEncryptionRequirements) {
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
+  testing::NiceMock<favicon::MockFaviconService> favicon_service;
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(),
+                      &favicon_service, &tracker)
+      .Merge();
+
+  const std::string kId0 = "id0";
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateResponseData(/*server_id=*/kId0,
+                                             /*parent_id=*/kBookmarkBarId,
+                                             /*is_deletion=*/false));
+  BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
+                                               &favicon_service, &tracker);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  ASSERT_THAT(tracker.GetEntityForSyncId(kId0), NotNull());
+  EXPECT_THAT(tracker.GetEntityForSyncId(kId0)->IsUnsynced(), Eq(false));
+
+  updates_handler.Process({}, /*got_new_encryption_requirements=*/true);
+  EXPECT_THAT(tracker.GetEntityForSyncId(kId0)->IsUnsynced(), Eq(true));
+  // Permanent nodes shouldn't be committed. They are only created on the server
+  // and synced down.
+  EXPECT_THAT(tracker.GetEntityForSyncId(kBookmarkBarId)->IsUnsynced(),
+              Eq(false));
+}
+
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
+     ShouldNotRecommitUptoDateEntitiesWhenGotNewEncryptionRequirements) {
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
+  testing::NiceMock<favicon::MockFaviconService> favicon_service;
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(),
+                      &favicon_service, &tracker)
+      .Merge();
+
+  const std::string kId0 = "id0";
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateResponseData(/*server_id=*/kId0,
+                                             /*parent_id=*/kBookmarkBarId,
+                                             /*is_deletion=*/false));
+  BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
+                                               &favicon_service, &tracker);
+  updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  ASSERT_THAT(tracker.GetEntityForSyncId(kId0), NotNull());
+  EXPECT_THAT(tracker.GetEntityForSyncId(kId0)->IsUnsynced(), Eq(false));
+
+  // Push another update to for the same entity.
+  syncer::UpdateResponseData response_data =
+      CreateUpdateResponseData(/*server_id=*/kId0,
+                               /*parent_id=*/kBookmarkBarId,
+                               /*is_deletion=*/false);
+
+  // Increment the server version to make sure the update isn't discarded as
+  // reflection.
+  response_data.response_version++;
+  updates_handler.Process({response_data},
+                          /*got_new_encryption_requirements=*/true);
+  EXPECT_THAT(tracker.GetEntityForSyncId(kId0)->IsUnsynced(), Eq(false));
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -297,6 +297,16 @@ bool SyncedBookmarkTracker::HasLocalChanges() const {
   return false;
 }
 
+std::vector<const SyncedBookmarkTracker::Entity*>
+SyncedBookmarkTracker::GetAllEntities() const {
+  std::vector<const SyncedBookmarkTracker::Entity*> entities;
+  for (const std::pair<const std::string, std::unique_ptr<Entity>>& pair :
+       sync_id_to_entities_map_) {
+    entities.push_back(pair.second.get());
+  }
+  return entities;
+}
+
 std::vector<const SyncedBookmarkTracker::Entity*>
 SyncedBookmarkTracker::GetEntitiesWithLocalChanges(size_t max_entries) const {
   std::vector<const SyncedBookmarkTracker::Entity*> entities_with_local_changes;

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -157,6 +157,8 @@ class SyncedBookmarkTracker {
     model_type_state_ = std::move(model_type_state);
   }
 
+  std::vector<const Entity*> GetAllEntities() const;
+
   std::vector<const Entity*> GetEntitiesWithLocalChanges(
       size_t max_entries) const;