Address duplicates of BuildTrustedVaultNigoriSpecifics()

BuildTrustedVaultNigoriSpecifics() had its own implementation for each
of single_client_nigori_sync_test, nigori_sync_bridge_impl_unittests
and fake_server_helper_android (was inlined into other function).

This CL factors out the implementation and addresses these duplicates.
There is small difference between old implementations: keybag_is_frozen
wasn't set in some of them, but this field isn't consumed by modern
clients.

Bug: 1046663
Change-Id: Ia173ac1f1942b988d972adc3e47c3b3c803a3d39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2168881
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763460}

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

@@ -43,6 +43,7 @@ namespace {
 using fake_server::GetServerNigori;
 using fake_server::SetNigoriInFakeServer;
 using syncer::BuildKeystoreNigoriSpecifics;
+using syncer::BuildTrustedVaultNigoriSpecifics;
 using syncer::KeyParamsForTesting;
 using syncer::Pbkdf2KeyParamsForTesting;
 using testing::NotNull;
@@ -104,27 +105,6 @@ std::string ComputeKeyName(const KeyParamsForTesting& key_params) {
   return key_name;
 }
 
-sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
-    const std::vector<std::vector<uint8_t>>& trusted_vault_keys) {
-  sync_pb::NigoriSpecifics specifics;
-  specifics.set_passphrase_type(
-      sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE);
-  specifics.set_keybag_is_frozen(true);
-
-  std::unique_ptr<syncer::CryptographerImpl> cryptographer =
-      syncer::CryptographerImpl::CreateEmpty();
-  for (const std::vector<uint8_t>& trusted_vault_key : trusted_vault_keys) {
-    const std::string key_name = cryptographer->EmplaceKey(
-        base::Base64Encode(trusted_vault_key),
-        syncer::KeyDerivationParams::CreateForPbkdf2());
-    cryptographer->SelectDefaultEncryptionKey(key_name);
-  }
-
-  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
-                                     specifics.mutable_encryption_keybag()));
-  return specifics;
-}
-
 // Used to wait until a tab closes.
 class TabClosedChecker : public StatusChangeChecker,
                          public content::WebContentsObserver {

components/sync/BUILD.gn

@@ -685,6 +685,7 @@ source_set("unit_tests") {
     ":sync",
     ":test_support_engine",
     ":test_support_model",
+    ":test_support_nigori",
     "//base",
     "//base/test:test_support",
     "//components/invalidation/impl",

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -17,6 +17,7 @@
 #include "components/sync/nigori/keystore_keys_cryptographer.h"
 #include "components/sync/nigori/nigori_state.h"
 #include "components/sync/nigori/nigori_storage.h"
+#include "components/sync/nigori/nigori_test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -222,24 +223,6 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
   return specifics;
 }
 
-sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
-    const std::vector<KeyParams>& trusted_vault_key_params) {
-  std::unique_ptr<CryptographerImpl> cryptographer =
-      CryptographerImpl::CreateEmpty();
-  for (const KeyParams& key_params : trusted_vault_key_params) {
-    const std::string key_name = cryptographer->EmplaceKey(
-        key_params.password, key_params.derivation_params);
-    cryptographer->SelectDefaultEncryptionKey(key_name);
-  }
-
-  sync_pb::NigoriSpecifics specifics;
-  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
-                                     specifics.mutable_encryption_keybag()));
-  specifics.set_passphrase_type(
-      sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE);
-  return specifics;
-}
-
 // Builds NigoriSpecifics with following fields:
 // 1. encryption_keybag contains keys derived from |passphrase_key_params|
 // and |*old_key_params| (if |old_key_params| isn't nullopt). Encrypted with
@@ -1283,8 +1266,8 @@ TEST(NigoriSyncBridgeImplPersistenceTest,
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldRequireUserActionIfInitiallyUsingTrustedVault) {
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
 
@@ -1337,8 +1320,7 @@ TEST_F(NigoriSyncBridgeImplTest,
 
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics(
-          {TrustedVaultKeyParams(kTrustedVaultKey)});
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
@@ -1373,8 +1355,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
 
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1389,8 +1371,7 @@ TEST_F(NigoriSyncBridgeImplTest,
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() =
       BuildTrustedVaultNigoriSpecifics(
-          {TrustedVaultKeyParams(kTrustedVaultKey),
-           TrustedVaultKeyParams(kRotatedTrustedVaultKey)});
+          {kTrustedVaultKey, kRotatedTrustedVaultKey});
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnPassphraseTypeChanged(_, _)).Times(0);
@@ -1416,8 +1397,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   const std::string kCustomPassphrase = "custom_passphrase";
 
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1456,11 +1437,9 @@ TEST_F(NigoriSyncBridgeImplTest,
 // vault to keystore passphrase.
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldProcessRemoteTransitionFromTrustedVaultToKeystore) {
-  const KeyParams kTrustedVaultKeyParams =
-      TrustedVaultKeyParams(kTrustedVaultKey);
   EntityData entity_data;
   *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKeyParams});
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1473,6 +1452,8 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
 
+  const KeyParams kTrustedVaultKeyParams =
+      TrustedVaultKeyParams(kTrustedVaultKey);
   const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
@@ -1506,11 +1487,9 @@ TEST_F(NigoriSyncBridgeImplTest,
 // vault to custom passphrase.
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldProcessRemoteTransitionFromTrustedVaultToCustomPassphrase) {
-  const KeyParams kTrustedVaultKeyParams =
-      TrustedVaultKeyParams(kTrustedVaultKey);
   EntityData entity_data;
   *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKeyParams});
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1523,6 +1502,8 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
 
+  const KeyParams kTrustedVaultKeyParams =
+      TrustedVaultKeyParams(kTrustedVaultKey);
   const KeyParams kCustomPassphraseKeyParams =
       Pbkdf2KeyParams("custom_passphrase");
   EntityData new_entity_data;
@@ -1563,8 +1544,8 @@ TEST_F(NigoriSyncBridgeImplTest,
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldFailOnInvalidRemoteTransitionFromTrustedVaultToKeystore) {
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1595,8 +1576,8 @@ TEST_F(NigoriSyncBridgeImplTest,
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldFailOnInvalidRemoteTransitionFromTrustedVaultToCustomPassphrase) {
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1662,8 +1643,8 @@ TEST(NigoriSyncBridgeImplPersistenceTest,
   // Perform initial sync with trusted vault passphrase.
   const std::vector<uint8_t> kTrustedVaultKey = {2, 3, 4, 5, 6};
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey});
 
   const std::vector<uint8_t> kRawKeystoreKey = {0, 1, 2, 3, 4};
   ASSERT_TRUE(bridge1->SetKeystoreKeys({kRawKeystoreKey}));
@@ -1715,8 +1696,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   const std::vector<uint8_t> kTrustedVaultKey1{kTrustedVaultKey};
   const std::vector<uint8_t> kTrustedVaultKey2 = {3, 4, 5, 6};
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
-      {TrustedVaultKeyParams(kTrustedVaultKey1)});
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKey1});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),

components/sync/nigori/nigori_test_utils.cc

@@ -56,6 +56,27 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
   return specifics;
 }
 
+sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
+    const std::vector<std::vector<uint8_t>>& trusted_vault_keys) {
+  sync_pb::NigoriSpecifics specifics;
+  specifics.set_passphrase_type(
+      sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE);
+  specifics.set_keybag_is_frozen(true);
+
+  std::unique_ptr<syncer::CryptographerImpl> cryptographer =
+      syncer::CryptographerImpl::CreateEmpty();
+  for (const std::vector<uint8_t>& trusted_vault_key : trusted_vault_keys) {
+    const std::string key_name = cryptographer->EmplaceKey(
+        base::Base64Encode(trusted_vault_key),
+        syncer::KeyDerivationParams::CreateForPbkdf2());
+    cryptographer->SelectDefaultEncryptionKey(key_name);
+  }
+
+  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
+                                     specifics.mutable_encryption_keybag()));
+  return specifics;
+}
+
 sync_pb::NigoriSpecifics CreateCustomPassphraseNigori(
     const KeyParamsForTesting& passphrase_key_params,
     const base::Optional<KeyParamsForTesting>& old_key_params) {

components/sync/nigori/nigori_test_utils.h

@@ -47,6 +47,14 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
     const KeyParamsForTesting& keystore_decryptor_params,
     const KeyParamsForTesting& keystore_key_params);
 
+// Builds NigoriSpecifics with following fields:
+// 1. encryption_keybag contains keys derived from |trusted_vault_keys| and
+// encrypted with key derived from last of them.
+// 2. passphrase_type is TRUSTED_VAULT_PASSPHRASE.
+// 3. keybag_is_frozen set to true.
+sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
+    const std::vector<std::vector<uint8_t>>& trusted_vault_keys);
+
 // Creates a NigoriSpecifics that describes encryption using a custom
 // passphrase with the given |passphrase_key_params|. If |old_key_params| is
 // presented, |encryption_keybag| will also contain keys derived from it.

components/sync/test/fake_server/BUILD.gn

@@ -61,6 +61,7 @@ if (is_android) {
       ":fake_server_jni",
       "//base",
       "//components/sync",
+      "//components/sync:test_support_nigori",
       "//testing/gtest",
       "//url:url",
     ]

components/sync/test/fake_server/android/fake_server_helper_android.cc

@@ -11,57 +11,25 @@
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
-#include "base/base64.h"
 #include "base/logging.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
 #include "components/sync/driver/profile_sync_service.h"
-#include "components/sync/nigori/cryptographer_impl.h"
+#include "components/sync/nigori/nigori_test_utils.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
 #include "components/sync/test/fake_server/fake_server.h"
 #include "components/sync/test/fake_server/fake_server_jni/FakeServerHelper_jni.h"
 #include "components/sync/test/fake_server/fake_server_network_resources.h"
+#include "components/sync/test/fake_server/fake_server_nigori_helper.h"
 #include "components/sync/test/fake_server/fake_server_verifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 using base::android::JavaParamRef;
 
-namespace {
-
-// TODO(crbug.com/1046663): avoid duplicates with BuildTrustedVaultNigori() in
-// single_client_nigori_sync_test.cc (it likely means to move part of
-// encryption_helper.h/cc to components/sync/test).
-void SetTrustedVaultNigoriInFakeServer(
-    fake_server::FakeServer* fake_server,
-    const std::vector<uint8_t>& trusted_vault_key) {
-  sync_pb::NigoriSpecifics nigori;
-  nigori.set_passphrase_type(
-      sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE);
-  nigori.set_keybag_is_frozen(true);
-
-  std::unique_ptr<syncer::CryptographerImpl> cryptographer =
-      syncer::CryptographerImpl::FromSingleKeyForTesting(
-          base::Base64Encode(trusted_vault_key));
-
-  bool encrypt_result = cryptographer->Encrypt(
-      cryptographer->ToProto().key_bag(), nigori.mutable_encryption_keybag());
-  DCHECK(encrypt_result);
-
-  std::string nigori_entity_id =
-      fake_server->GetTopLevelPermanentItemId(syncer::NIGORI);
-  DCHECK_NE(nigori_entity_id, "");
-
-  sync_pb::EntitySpecifics entity_specifics;
-  *entity_specifics.mutable_nigori() = nigori;
-  fake_server->ModifyEntitySpecifics(nigori_entity_id, entity_specifics);
-}
-
-}  // namespace
-
 FakeServerHelperAndroid::FakeServerHelperAndroid(JNIEnv* env, jobject obj) {}
 
 FakeServerHelperAndroid::~FakeServerHelperAndroid() {}
@@ -380,9 +348,9 @@ void FakeServerHelperAndroid::SetTrustedVaultNigori(
   std::vector<uint8_t> native_trusted_vault_key;
   base::android::JavaByteArrayToByteVector(env, trusted_vault_key,
                                            &native_trusted_vault_key);
-  SetTrustedVaultNigoriInFakeServer(
-      reinterpret_cast<fake_server::FakeServer*>(fake_server),
-      native_trusted_vault_key);
+  SetNigoriInFakeServer(
+      syncer::BuildTrustedVaultNigoriSpecifics({native_trusted_vault_key}),
+      reinterpret_cast<fake_server::FakeServer*>(fake_server));
 }
 
 void FakeServerHelperAndroid::ClearServerData(JNIEnv* env,