Revert "Add encryption support to pseudo-USS"

This reverts commit 66a392fa.

Reason for revert: Causing component_unittest failures:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Cast%20Audio%20Linux/23527

failing tests:
SyncableServiceBasedBridgeTest.ShouldDecryptPreviousDirectoryDataAfterRestart
SyncableServiceBasedBridgeTest.ShouldDecryptInitialRemoteData
SyncableServiceBasedBridgeTest.ShouldDecryptForRemoteDeletion

Original change's description:
> Add encryption support to pseudo-USS
> 
> This patch extends SyncableServiceBasedBridge (core to pseudo-USS)
> with support for encryption, via a newly introduced interface
> (ModelCryptographer), which needs to be usable in the model thread
> (where the bridge lives).
> 
> This should unblock the migration of PASSWORDS to pseudo-USS, because
> password data must be encrypted at all times. This is a first step of
> a proposal described in a dedicated section in the Design Doc:
> https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot
> 
> Actual usage of this functionality will be introduced in follow-up
> patches.
> 
> Bug: 870624
> Change-Id: I227d429dc952bfe1a3a4fb05cd1ab71cac9ba1c6
> Reviewed-on: https://chromium-review.googlesource.com/c/1288594
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601254}

TBR=treib@chromium.org,mastiz@chromium.org,mamir@chromium.org

Change-Id: Iab6463f9541ca242facdc8c5217f1cf4c114ba49
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 870624
Reviewed-on: https://chromium-review.googlesource.com/c/1292512Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601270}

components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc

@@ -24,11 +24,8 @@ class LazyBridgeBuilder {
       NonUiSyncableServiceBasedModelTypeController::SyncableServiceProvider
           syncable_service_provider,
       const base::RepeatingClosure& dump_stack,
-      scoped_refptr<base::SequencedTaskRunner> task_runner,
-      scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
-          cryptographer)
+      scoped_refptr<base::SequencedTaskRunner> task_runner)
       : type_(type),
-        cryptographer_(std::move(cryptographer)),
         store_factory_(std::move(store_factory)),
         syncable_service_provider_(std::move(syncable_service_provider)),
         dump_stack_(dump_stack),
@@ -47,15 +44,13 @@ class LazyBridgeBuilder {
           type_, std::move(store_factory_),
           std::make_unique<ClientTagBasedModelTypeProcessor>(type_,
                                                              dump_stack_),
-          syncable_service.get(), cryptographer_));
+          syncable_service.get()));
     }
     return bridge_->change_processor()->GetControllerDelegate();
   }
 
  private:
   const ModelType type_;
-  const scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
-      cryptographer_;
   OnceModelTypeStoreFactory store_factory_;
   NonUiSyncableServiceBasedModelTypeController::SyncableServiceProvider
       syncable_service_provider_;
@@ -73,9 +68,7 @@ NonUiSyncableServiceBasedModelTypeController::
         OnceModelTypeStoreFactory store_factory,
         SyncableServiceProvider syncable_service_provider,
         const base::RepeatingClosure& dump_stack,
-        scoped_refptr<base::SequencedTaskRunner> task_runner,
-        scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
-            cryptographer)
+        scoped_refptr<base::SequencedTaskRunner> task_runner)
     : ModelTypeController(
           type,
           std::make_unique<ProxyModelTypeControllerDelegate>(
@@ -86,8 +79,7 @@ NonUiSyncableServiceBasedModelTypeController::
                                       std::move(store_factory),
                                       std::move(syncable_service_provider),
                                       dump_stack,
-                                      task_runner,
-                                      std::move(cryptographer))))) {}
+                                      task_runner)))) {}
 
 NonUiSyncableServiceBasedModelTypeController::
     ~NonUiSyncableServiceBasedModelTypeController() {}

components/sync/driver/non_ui_syncable_service_based_model_type_controller.h

@@ -15,7 +15,6 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/model_type_controller.h"
 #include "components/sync/model/model_type_store.h"
-#include "components/sync/model_impl/syncable_service_based_bridge.h"
 
 namespace syncer {
 
@@ -40,9 +39,7 @@ class NonUiSyncableServiceBasedModelTypeController
       OnceModelTypeStoreFactory store_factory,
       SyncableServiceProvider syncable_service_provider,
       const base::RepeatingClosure& dump_stack,
-      scoped_refptr<base::SequencedTaskRunner> task_runner,
-      scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
-          cryptographer = nullptr);
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   ~NonUiSyncableServiceBasedModelTypeController() override;
 
  private:

components/sync/model/sync_data.cc

@@ -6,7 +6,6 @@
 
 #include <algorithm>
 #include <ostream>
-#include <utility>
 
 #include "base/json/json_writer.h"
 #include "base/strings/string_number_conversions.h"
@@ -76,13 +75,14 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
 }
 
 // Static.
-SyncData SyncData::CreateRemoteData(int64_t id,
-                                    sync_pb::EntitySpecifics specifics,
-                                    base::Time modification_time,
-                                    std::string client_tag_hash) {
+SyncData SyncData::CreateRemoteData(
+    int64_t id,
+    const sync_pb::EntitySpecifics& specifics,
+    const base::Time& modification_time,
+    const std::string& client_tag_hash) {
   sync_pb::SyncEntity entity;
-  *entity.mutable_specifics() = std::move(specifics);
-  entity.set_client_defined_unique_tag(std::move(client_tag_hash));
+  entity.mutable_specifics()->CopyFrom(specifics);
+  entity.set_client_defined_unique_tag(client_tag_hash);
   return SyncData(/*is_local=*/false, id, &entity, modification_time);
 }
 

components/sync/model/sync_data.h

@@ -8,7 +8,6 @@
 #include <stdint.h>
 
 #include <iosfwd>
-#include <memory>
 #include <string>
 #include <vector>
 
@@ -59,10 +58,11 @@ class SyncData {
                                   const sync_pb::EntitySpecifics& specifics);
 
   // Helper method for creating SyncData objects originating from the syncer.
-  static SyncData CreateRemoteData(int64_t id,
-                                   sync_pb::EntitySpecifics specifics,
-                                   base::Time last_modified_time,
-                                   std::string client_tag_hash = std::string());
+  static SyncData CreateRemoteData(
+      int64_t id,
+      const sync_pb::EntitySpecifics& specifics,
+      const base::Time& last_modified_time,
+      const std::string& client_tag_hash = std::string());
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
   // without valid data is to use the default constructor.

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -372,13 +372,7 @@ void ClientTagBasedModelTypeProcessor::Put(
   DCHECK(data);
   DCHECK(!data->is_deleted());
   DCHECK(!data->non_unique_name.empty());
-
-  // Only the pseudo-USS bridge for PASSWORDS populates encrypted data.
-  // TODO(crbug.com/856941): Remove when PASSWORDS are migrated to USS and
-  // replace instead with a DCHECK that verifies the input is not encrypted.
-  if (!data->specifics.has_encrypted()) {
-    DCHECK_EQ(type_, GetModelTypeFromSpecifics(data->specifics));
-  }
+  DCHECK_EQ(type_, GetModelTypeFromSpecifics(data->specifics));
 
   if (!model_type_state_.initial_sync_done()) {
     // Ignore changes before the initial sync is done.

components/sync/model_impl/syncable_service_based_bridge.h

@@ -11,7 +11,6 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "components/sync/model/model_error.h"
@@ -19,10 +18,6 @@
 #include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/model/sync_change_processor.h"
 
-namespace sync_pb {
-class EntitySpecifics;
-}  // namespace sync_pb
-
 namespace syncer {
 
 class MetadataBatch;
@@ -36,35 +31,12 @@ class SyncableService;
 // considered an implementation detail.
 class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
  public:
-  // Used for passwords only.
-  // TODO(crbug.com/856941): Remove when PASSWORDS are migrated to USS, which
-  // will likely make this API unnecessary.
-  class ModelCryptographer
-      : public base::RefCountedThreadSafe<ModelCryptographer> {
-   public:
-    ModelCryptographer();
-
-    virtual base::Optional<ModelError> Decrypt(
-        sync_pb::EntitySpecifics* specifics) = 0;
-    virtual base::Optional<ModelError> Encrypt(
-        sync_pb::EntitySpecifics* specifics) = 0;
-
-   protected:
-    virtual ~ModelCryptographer();
-
-   private:
-    friend class base::RefCountedThreadSafe<ModelCryptographer>;
-
-    DISALLOW_COPY_AND_ASSIGN(ModelCryptographer);
-  };
-
   // Pointers must not be null and |syncable_service| must outlive this object.
   SyncableServiceBasedBridge(
       ModelType type,
       OnceModelTypeStoreFactory store_factory,
       std::unique_ptr<ModelTypeChangeProcessor> change_processor,
-      SyncableService* syncable_service,
-      scoped_refptr<ModelCryptographer> cryptographer = nullptr);
+      SyncableService* syncable_service);
   ~SyncableServiceBasedBridge() override;
 
   // ModelTypeSyncBridge implementation.
@@ -98,10 +70,9 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
   void OnReadAllMetadataForInit(const base::Optional<ModelError>& error,
                                 std::unique_ptr<MetadataBatch> metadata_batch);
   void MaybeStartSyncableService();
-  base::Optional<ModelError> StoreAndConvertRemoteChanges(
+  SyncChangeList StoreAndConvertRemoteChanges(
       std::unique_ptr<MetadataChangeList> metadata_change_list,
-      EntityChangeList input_entity_change_list,
-      SyncChangeList* output_sync_change_list);
+      EntityChangeList entity_change_list);
   void OnReadDataForProcessor(
       DataCallback callback,
       const base::Optional<ModelError>& error,
@@ -115,7 +86,6 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
 
   const ModelType type_;
   SyncableService* const syncable_service_;
-  const scoped_refptr<ModelCryptographer> cryptographer_;
   OnceModelTypeStoreFactory store_factory_;
 
   std::unique_ptr<ModelTypeStore> store_;

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -38,8 +38,6 @@ using testing::Return;
 using testing::SaveArg;
 using testing::_;
 
-using ModelCryptographer = SyncableServiceBasedBridge::ModelCryptographer;
-
 const ModelType kModelType = PREFERENCES;
 
 sync_pb::EntitySpecifics GetTestSpecifics(const std::string& name = "name") {
@@ -64,10 +62,6 @@ MATCHER_P(HasName, name, "") {
   return arg && arg->specifics.preference().name() == name;
 }
 
-MATCHER(IsEncrypted, "") {
-  return arg && arg->specifics.encrypted().has_blob();
-}
-
 class MockSyncableService : public SyncableService {
  public:
   MOCK_METHOD4(
@@ -83,44 +77,6 @@ class MockSyncableService : public SyncableService {
   MOCK_CONST_METHOD1(GetAllSyncData, SyncDataList(ModelType type));
 };
 
-// Simple ModelCryptographer implementation that serializes the input proto to a
-// string and puts it in proto field EntitySpecifics.encrypted.blob.
-class TestModelCryptographer : public ModelCryptographer {
- public:
-  TestModelCryptographer() {}
-
-  base::Optional<ModelError> Decrypt(
-      sync_pb::EntitySpecifics* specifics) override {
-    if (!specifics->has_encrypted()) {
-      return ModelError(FROM_HERE, "Not encrypted");
-    }
-    sync_pb::EntitySpecifics unencrypted_specifics;
-    if (!unencrypted_specifics.ParseFromString(specifics->encrypted().blob())) {
-      return ModelError(FROM_HERE, "Cannot parse blob");
-    }
-    *specifics = unencrypted_specifics;
-    return base::nullopt;
-  }
-
-  base::Optional<ModelError> Encrypt(
-      sync_pb::EntitySpecifics* specifics) override {
-    if (specifics->has_encrypted()) {
-      return ModelError(FROM_HERE, "Already encrypted");
-    }
-    sync_pb::EntitySpecifics encrypted_specifics;
-    encrypted_specifics.mutable_encrypted()->set_blob(
-        specifics->SerializeAsString());
-    *specifics = encrypted_specifics;
-    return base::nullopt;
-  }
-
- protected:
-  ~TestModelCryptographer() override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestModelCryptographer);
-};
-
 class SyncableServiceBasedBridgeTest : public ::testing::Test {
  protected:
   SyncableServiceBasedBridgeTest()
@@ -137,8 +93,7 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
 
   ~SyncableServiceBasedBridgeTest() override {}
 
-  void InitializeBridge(
-      scoped_refptr<ModelCryptographer> cryptographer = nullptr) {
+  void InitializeBridge() {
     real_processor_ =
         std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
             kModelType, /*dump_stack=*/base::DoNothing(),
@@ -147,8 +102,7 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
     bridge_ = std::make_unique<SyncableServiceBasedBridge>(
         kModelType,
         ModelTypeStoreTestUtil::FactoryForForwardingStore(store_.get()),
-        mock_processor_.CreateForwardingProcessor(), &syncable_service_,
-        cryptographer);
+        mock_processor_.CreateForwardingProcessor(), &syncable_service_);
   }
 
   void ShutdownBridge() {
@@ -496,111 +450,5 @@ TEST_F(SyncableServiceBasedBridgeTest, ShouldPropagateRemoteDeletion) {
   EXPECT_THAT(GetAllData(), IsEmpty());
 }
 
-TEST_F(SyncableServiceBasedBridgeTest, ShouldDecryptInitialRemoteData) {
-  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
-  InitializeBridge(cryptographer);
-  StartSyncing();
-
-  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
-  // Once the initial encrypted data is fetched from the server,
-  // MergeDataAndStartSyncing() should be exercised, which should receive
-  // decrypted specifics.
-  EXPECT_CALL(syncable_service_,
-              MergeDataAndStartSyncing(
-                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
-                  NotNull(), NotNull()));
-  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
-  DCHECK(!cryptographer->Encrypt(&specifics));
-  worker_->UpdateFromServer(kClientTagHash, specifics);
-  // The bridge's store should contain encrypted content.
-  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
-}
-
-TEST_F(SyncableServiceBasedBridgeTest,
-       ShouldHandleDecryptionErrorForInitialRemoteData) {
-  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
-  InitializeBridge(cryptographer);
-  StartSyncing();
-
-  EXPECT_CALL(mock_error_handler_, Run(_));
-  EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _)).Times(0);
-
-  sync_pb::EntitySpecifics specifics;
-  specifics.mutable_encrypted()->set_blob("corrupt-encrypted-blob");
-  DCHECK(cryptographer->Decrypt(&specifics).has_value());
-  worker_->UpdateFromServer(kClientTagHash, specifics);
-  EXPECT_THAT(GetAllData(), IsEmpty());
-}
-
-TEST_F(SyncableServiceBasedBridgeTest, ShouldDecryptForRemoteDeletion) {
-  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
-  InitializeBridge(cryptographer);
-  StartSyncing();
-
-  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
-  DCHECK(!cryptographer->Encrypt(&specifics));
-  worker_->UpdateFromServer(kClientTagHash, specifics);
-  ASSERT_THAT(start_syncing_sync_processor_, NotNull());
-  ASSERT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
-
-  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
-  EXPECT_CALL(syncable_service_,
-              ProcessSyncChanges(_, ElementsAre(SyncChangeMatches(
-                                        SyncChange::ACTION_DELETE, "name1"))));
-  worker_->TombstoneFromServer(kClientTagHash);
-  EXPECT_THAT(GetAllData(), IsEmpty());
-}
-
-TEST_F(SyncableServiceBasedBridgeTest,
-       ShouldDecryptPreviousDirectoryDataAfterRestart) {
-  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
-  InitializeBridge(cryptographer);
-  StartSyncing();
-
-  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
-  DCHECK(!cryptographer->Encrypt(&specifics));
-  worker_->UpdateFromServer(kClientTagHash, specifics);
-  // The bridge's store should contain encrypted content.
-  ASSERT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
-
-  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
-  // Mimic restart, which shouldn't start syncing until OnSyncStarting() is
-  // received (exercised in StartSyncing()).
-  ShutdownBridge();
-  InitializeBridge(cryptographer);
-
-  EXPECT_CALL(syncable_service_,
-              MergeDataAndStartSyncing(
-                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
-                  NotNull(), NotNull()));
-  StartSyncing();
-
-  // The bridge's store should still contain encrypted content.
-  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
-}
-
-TEST_F(SyncableServiceBasedBridgeTest, ShouldEncryptLocalCreation) {
-  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
-  InitializeBridge(cryptographer);
-  StartSyncing();
-  worker_->UpdateFromServer();
-
-  ASSERT_THAT(start_syncing_sync_processor_, NotNull());
-  ASSERT_THAT(GetAllData(), IsEmpty());
-  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
-
-  // The processor should receive encrypted data.
-  EXPECT_CALL(mock_processor_, Put(kClientTagHash, IsEncrypted(), NotNull()));
-
-  SyncChangeList change_list;
-  change_list.emplace_back(
-      FROM_HERE, SyncChange::ACTION_ADD,
-      SyncData::CreateLocalData(kClientTag, "title", GetTestSpecifics()));
-  const SyncError error =
-      start_syncing_sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
-  EXPECT_FALSE(error.IsSet());
-  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
-}
-
 }  // namespace
 }  // namespace syncer

components/sync/protocol/password_specifics.proto

@@ -120,8 +120,7 @@ message PasswordSpecifics {
   // message.
   optional EncryptedData encrypted = 1;
   // An unsynced field for use internally on the client. This field should
-  // never be set in any network-based communications because it contains
-  // unencrypted material.
+  // never be set in any network-based communications.
   optional PasswordSpecificsData client_only_encrypted_data = 2;
   // Password related metadata, which is sent to the server side. The field
   // should never be set for full encryption users. If encryption is enabled,

components/sync/test/engine/mock_model_type_worker.cc

@@ -160,9 +160,7 @@ UpdateResponseData MockModelTypeWorker::GenerateUpdateData(
   data.creation_time = base::Time::UnixEpoch() + base::TimeDelta::FromDays(1);
   data.modification_time =
       data.creation_time + base::TimeDelta::FromSeconds(version);
-  data.non_unique_name = data.specifics.has_encrypted()
-                             ? "encrypted"
-                             : data.specifics.preference().name();
+  data.non_unique_name = data.specifics.preference().name();
 
   UpdateResponseData response_data;
   response_data.entity = data.PassToPtr();