Remove DirectoryCryptographer usages in USS tests

DirectoryCryptographer is subject to remove and its usages should be
replaced with CryptographerImpl or removed. This CL does it for all
remaining USS tests.

Bug: 1083924
Change-Id: Iedd4f6c039407cce0dfb502a4070ac42f4336c80
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226753
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774562}

components/sync/engine_impl/js_sync_encryption_handler_observer_unittest.cc

@@ -17,7 +17,7 @@
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/js/js_event_details.h"
 #include "components/sync/js/js_test_util.h"
-#include "components/sync/syncable/directory_cryptographer.h"
+#include "components/sync/nigori/cryptographer_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
@@ -134,9 +134,10 @@ TEST_F(JsSyncEncryptionHandlerObserverTest, OnCryptographerStateChanged) {
               HandleJsEvent("onCryptographerStateChanged",
                             HasDetailsAsDictionary(expected_details)));
 
-  DirectoryCryptographer cryptographer;
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::CreateEmpty();
   js_sync_encryption_handler_observer_.OnCryptographerStateChanged(
-      &cryptographer, /*has_pending_keys=*/false);
+      cryptographer.get(), /*has_pending_keys=*/false);
   PumpLoop();
 }
 

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -21,7 +21,9 @@
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/cycle/non_blocking_type_debug_info_emitter.h"
 #include "components/sync/engine_impl/cycle/status_controller.h"
-#include "components/sync/syncable/directory_cryptographer.h"
+#include "components/sync/nigori/cryptographer_impl.h"
+#include "components/sync/nigori/nigori.h"
+#include "components/sync/nigori/nigori_test_utils.h"
 #include "components/sync/test/engine/mock_model_type_processor.h"
 #include "components/sync/test/engine/mock_nudge_handler.h"
 #include "components/sync/test/engine/single_type_mock_server.h"
@@ -38,9 +40,6 @@ namespace syncer {
 
 namespace {
 
-// Special constant value taken from cryptographer.cc.
-const char kNigoriKeyName[] = "nigori-key";
-
 const char kTag1[] = "tag1";
 const char kTag2[] = "tag2";
 const char kTag3[] = "tag3";
@@ -77,15 +76,15 @@ std::string GetNigoriName(const Nigori& nigori) {
   return name;
 }
 
-// Returns a set of KeyParams for the cryptographer. Each input 'n' value
+// Returns a KeyParamsForTesting for the cryptographer. Each input 'n' value
 // results in a different set of parameters.
-KeyParams GetNthKeyParams(int n) {
+KeyParamsForTesting GetNthKeyParams(int n) {
   return {KeyDerivationParams::CreateForPbkdf2(),
           base::StringPrintf("pw%02d", n)};
 }
 
 sync_pb::EntitySpecifics EncryptPasswordSpecifics(
-    const KeyParams& key_params,
+    const KeyParamsForTesting& key_params,
     const sync_pb::PasswordSpecificsData& unencrypted_password) {
   std::unique_ptr<Nigori> nigori = Nigori::CreateByDerivation(
       key_params.derivation_params, key_params.password);
@@ -230,45 +229,18 @@ class ModelTypeWorkerTest : public ::testing::Test {
 
   void InitializeCryptographer() {
     if (!cryptographer_) {
-      cryptographer_ = std::make_unique<DirectoryCryptographer>();
+      cryptographer_ = CryptographerImpl::CreateEmpty();
     }
   }
 
-  // Introduce a new key that the local cryptographer can't decrypt.
+  // Mimic a Nigori update with a keybag that cannot be decrypted, which means
+  // the cryptographer becomes unusable (no default key until the issue gets
+  // resolved, via DecryptPendingKey()).
   void AddPendingKey() {
     InitializeCryptographer();
 
     foreign_encryption_key_index_++;
-
-    sync_pb::NigoriKeyBag bag;
-
-    for (int i = 0; i <= foreign_encryption_key_index_; ++i) {
-      KeyParams params = GetNthKeyParams(i);
-      std::unique_ptr<Nigori> nigori =
-          Nigori::CreateByDerivation(params.derivation_params, params.password);
-
-      sync_pb::NigoriKey* key = bag.add_key();
-
-      key->set_deprecated_name(GetNigoriName(*nigori));
-      nigori->ExportKeys(key->mutable_deprecated_user_key(),
-                         key->mutable_encryption_key(), key->mutable_mac_key());
-    }
-
-    // Re-create the last nigori from that loop.
-    KeyParams params = GetNthKeyParams(foreign_encryption_key_index_);
-    std::unique_ptr<Nigori> last_nigori =
-        Nigori::CreateByDerivation(params.derivation_params, params.password);
-
-    // Serialize and encrypt the bag with the last nigori.
-    std::string serialized_bag;
-    bag.SerializeToString(&serialized_bag);
-
-    sync_pb::EncryptedData encrypted;
-    encrypted.set_key_name(GetNigoriName(*last_nigori));
-    last_nigori->Encrypt(serialized_bag, encrypted.mutable_blob());
-
-    // Update the cryptographer with new pending keys.
-    cryptographer_->SetPendingKeys(encrypted);
+    cryptographer_->ClearDefaultEncryptionKey();
 
     // Update the worker with the latest cryptographer.
     if (worker()) {
@@ -278,11 +250,16 @@ class ModelTypeWorkerTest : public ::testing::Test {
 
   // Update the local cryptographer with all relevant keys.
   void DecryptPendingKey() {
+    DCHECK_NE(foreign_encryption_key_index_, 0);
     InitializeCryptographer();
 
-    KeyParams params = GetNthKeyParams(foreign_encryption_key_index_);
-    bool success = cryptographer_->DecryptPendingKeys(params);
-    DCHECK(success);
+    std::string last_key_name;
+    for (int i = 1; i <= foreign_encryption_key_index_; ++i) {
+      const KeyParamsForTesting key_params = GetNthKeyParams(i);
+      last_key_name = cryptographer_->EmplaceKey(key_params.password,
+                                                 key_params.derivation_params);
+    }
+    cryptographer_->SelectDefaultEncryptionKey(last_key_name);
 
     // Update the worker with the latest cryptographer.
     if (worker()) {
@@ -292,8 +269,9 @@ class ModelTypeWorkerTest : public ::testing::Test {
   }
 
   // Modifies the input/output parameter |specifics| by encrypting it with
-  // a Nigori initialized with the specified KeyParams.
-  void EncryptUpdate(const KeyParams& params, EntitySpecifics* specifics) {
+  // a Nigori initialized with the specified |params|.
+  void EncryptUpdate(const KeyParamsForTesting& params,
+                     EntitySpecifics* specifics) {
     std::unique_ptr<Nigori> nigori =
         Nigori::CreateByDerivation(params.derivation_params, params.password);
 
@@ -510,7 +488,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
   const ModelType model_type_;
 
   // The cryptographer itself. Null if we're not encrypting the type.
-  std::unique_ptr<DirectoryCryptographer> cryptographer_;
+  std::unique_ptr<CryptographerImpl> cryptographer_;
 
   // The number of the most recent foreign encryption key known to our
   // cryptographer. Note that not all of these will be decryptable.
@@ -1426,12 +1404,12 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   *entity.mutable_specifics() = GenerateSpecifics(kTag1, kValue1);
   UpdateResponseData response_data;
 
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
-  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+  EXPECT_EQ(
+      ModelTypeWorker::SUCCESS,
       ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, PREFERENCES, entity, &response_data));
+          /*cryptographer=*/nullptr, PREFERENCES, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_FALSE(data.id.empty());
   EXPECT_FALSE(data.parent_id.empty());
@@ -1459,12 +1437,10 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataForBookmarkTombstone) {
   // Add default value field for a Bookmark.
   entity.mutable_specifics()->mutable_bookmark();
 
-  DirectoryCryptographer cryptographer;
-
   UpdateResponseData response_data;
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
 
   const EntityData& data = response_data.entity;
   // A tombstone should remain a tombstone after populating the response data.
@@ -1483,12 +1459,11 @@ TEST_F(ModelTypeWorkerTest,
   *entity.mutable_specifics() = GenerateSpecifics(kTag1, kValue1);
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1511,12 +1486,11 @@ TEST_F(ModelTypeWorkerTest,
   *entity.mutable_specifics() = GenerateSpecifics(kTag1, kValue1);
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1539,12 +1513,11 @@ TEST_F(ModelTypeWorkerTest,
   *entity.mutable_specifics() = GenerateSpecifics(kTag1, kValue1);
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1568,12 +1541,11 @@ TEST_F(ModelTypeWorkerTest,
   *entity.mutable_specifics() = specifics;
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_FALSE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1592,12 +1564,12 @@ TEST_F(ModelTypeWorkerTest,
   *entity.mutable_specifics() = GenerateSpecifics(kTag1, kValue1);
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
   base::HistogramTester histogram_tester;
 
-  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+  EXPECT_EQ(
+      ModelTypeWorker::SUCCESS,
       ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, PREFERENCES, entity, &response_data));
+          /*cryptographer=*/nullptr, PREFERENCES, entity, &response_data));
   const EntityData& data = response_data.entity;
   EXPECT_FALSE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1629,11 +1601,10 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataForBookmarkWithGUID) {
       UniquePosition::InitialPosition(UniquePosition::RandomSuffix()).ToProto();
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
 
   const EntityData& data = response_data.entity;
 
@@ -1654,11 +1625,10 @@ TEST_F(ModelTypeWorkerTest,
       UniquePosition::InitialPosition(UniquePosition::RandomSuffix()).ToProto();
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
 
   const EntityData& data = response_data.entity;
 
@@ -1680,11 +1650,10 @@ TEST_F(ModelTypeWorkerTest,
       UniquePosition::InitialPosition(UniquePosition::RandomSuffix()).ToProto();
 
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, BOOKMARKS, entity, &response_data));
+                /*cryptographer=*/nullptr, BOOKMARKS, entity, &response_data));
 
   const EntityData& data = response_data.entity;
 
@@ -1696,7 +1665,6 @@ TEST_F(ModelTypeWorkerTest,
        PopulateUpdateResponseDataForWalletDataWithMissingClientTagHash) {
   NormalInitialize();
   UpdateResponseData response_data;
-  DirectoryCryptographer cryptographer;
 
   // Set up the entity with an arbitrary value for an arbitrary field in the
   // specifics (so that it _has_ autofill wallet specifics).
@@ -1706,7 +1674,8 @@ TEST_F(ModelTypeWorkerTest,
 
   ASSERT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, AUTOFILL_WALLET_DATA, entity, &response_data));
+                /*cryptographer=*/nullptr, AUTOFILL_WALLET_DATA, entity,
+                &response_data));
 
   // The client tag hash gets filled in by the worker.
   EXPECT_FALSE(response_data.entity.client_tag_hash.value().empty());

components/sync/engine_impl/non_blocking_type_commit_contribution_unittest.cc

@@ -15,7 +15,7 @@
 #include "components/sync/base/client_tag_hash.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/unique_position.h"
-#include "components/sync/syncable/directory_cryptographer.h"
+#include "components/sync/nigori/cryptographer_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
@@ -169,16 +169,17 @@ TEST(NonBlockingTypeCommitContributionTest,
   base::ObserverList<TypeDebugInfoObserver>::Unchecked observers;
   DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS, &observers);
 
-  DirectoryCryptographer cryptographer;
-  cryptographer.AddKey({KeyDerivationParams::CreateForPbkdf2(), "dummy"});
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting("dummy");
 
   CommitRequestDataList requests_data;
   requests_data.push_back(std::move(request_data));
   NonBlockingTypeCommitContribution contribution(
       PASSWORDS, sync_pb::DataTypeContext(), std::move(requests_data),
       /*on_commit_response_callback=*/base::NullCallback(),
-      /*on_full_commit_failure_callback=*/base::NullCallback(), &cryptographer,
-      PassphraseType::kImplicitPassphrase, &debug_info_emitter,
+      /*on_full_commit_failure_callback=*/base::NullCallback(),
+      cryptographer.get(), PassphraseType::kImplicitPassphrase,
+      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -235,16 +236,17 @@ TEST(NonBlockingTypeCommitContributionTest,
   base::ObserverList<TypeDebugInfoObserver>::Unchecked observers;
   DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS, &observers);
 
-  DirectoryCryptographer cryptographer;
-  cryptographer.AddKey({KeyDerivationParams::CreateForPbkdf2(), "dummy"});
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting("dummy");
 
   CommitRequestDataList requests_data;
   requests_data.push_back(std::move(request_data));
   NonBlockingTypeCommitContribution contribution(
       PASSWORDS, sync_pb::DataTypeContext(), std::move(requests_data),
       /*on_commit_response_callback=*/base::NullCallback(),
-      /*on_full_commit_failure_callback=*/base::NullCallback(), &cryptographer,
-      PassphraseType::kCustomPassphrase, &debug_info_emitter,
+      /*on_full_commit_failure_callback=*/base::NullCallback(),
+      cryptographer.get(), PassphraseType::kCustomPassphrase,
+      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -281,7 +283,8 @@ TEST(NonBlockingTypeCommitContributionTest,
   base::ObserverList<TypeDebugInfoObserver>::Unchecked observers;
   DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS, &observers);
 
-  DirectoryCryptographer cryptographer;
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::CreateEmpty();
 
   FailedCommitResponseDataList actual_error_response_list;
 
@@ -298,8 +301,9 @@ TEST(NonBlockingTypeCommitContributionTest,
   NonBlockingTypeCommitContribution contribution(
       PASSWORDS, sync_pb::DataTypeContext(), std::move(requests_data),
       std::move(on_commit_response_callback),
-      /*on_full_commit_failure_callback=*/base::NullCallback(), &cryptographer,
-      PassphraseType::kCustomPassphrase, &debug_info_emitter,
+      /*on_full_commit_failure_callback=*/base::NullCallback(),
+      cryptographer.get(), PassphraseType::kCustomPassphrase,
+      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -333,19 +337,18 @@ TEST(NonBlockingTypeCommitContributionTest,
 }
 
 TEST(NonBlockingTypeCommitContributionTest, ShouldPropagateFullCommitFailure) {
-  DirectoryCryptographer cryptographer;
   base::ObserverList<TypeDebugInfoObserver>::Unchecked observers;
-  DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS, &observers);
+  DataTypeDebugInfoEmitter debug_info_emitter(BOOKMARKS, &observers);
 
   base::MockOnceCallback<void(SyncCommitError commit_error)>
       on_commit_failure_callback;
   EXPECT_CALL(on_commit_failure_callback, Run(SyncCommitError::kNetworkError));
 
   NonBlockingTypeCommitContribution contribution(
-      PASSWORDS, sync_pb::DataTypeContext(), CommitRequestDataList(),
+      BOOKMARKS, sync_pb::DataTypeContext(), CommitRequestDataList(),
       /*on_commit_response_callback=*/base::NullCallback(),
-      on_commit_failure_callback.Get(), &cryptographer,
-      PassphraseType::kCustomPassphrase, &debug_info_emitter,
+      on_commit_failure_callback.Get(), /*cryptographer=*/nullptr,
+      PassphraseType::kKeystorePassphrase, &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   contribution.ProcessCommitFailure(SyncCommitError::kNetworkError);

components/sync/syncable/nigori_handler_proxy.cc

@@ -5,7 +5,7 @@
 #include "components/sync/syncable/nigori_handler_proxy.h"
 
 #include "components/sync/base/model_type.h"
-#include "components/sync/syncable/directory_cryptographer.h"
+#include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/syncable/syncable_base_transaction.h"
 #include "components/sync/syncable/user_share.h"
 #include "components/sync/syncable/write_transaction.h"
@@ -16,7 +16,7 @@ namespace syncable {
 
 NigoriHandlerProxy::NigoriHandlerProxy(UserShare* user_share)
     : user_share_(user_share),
-      cryptographer_(std::make_unique<DirectoryCryptographer>()),
+      cryptographer_(CryptographerImpl::CreateEmpty()),
       encrypted_types_(AlwaysEncryptedUserTypes()),
       passphrase_type_(SyncEncryptionHandler::kInitialPassphraseType) {
   DCHECK(user_share);