Cleanup around password sync test helpers

A bunch of passwords/encryption-related helpers existed in
passwords_helper/encryption_helper but also had almost-identical copies
in single_client_passwords_sync_test.

This CL moves all the helpers into passwords/encryption_helper and
de-dupes the almost-copies -> less duplication, and they can now be
used in additional tests (like PasswordManagerSyncTest).

Bug: 1058339
Change-Id: I5bae176053ca6c2a063d09fa3a80efb116df4003
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2101000
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#750579}

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

@@ -10,6 +10,7 @@
 #include "chrome/browser/sync/test/integration/encryption_helper.h"
 #include "components/sync/base/passphrase_enums.h"
 #include "components/sync/base/sync_base_switches.h"
+#include "components/sync/base/time.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/sync/driver/sync_client.h"
 #include "components/sync/engine/sync_engine_switches.h"
@@ -19,6 +20,40 @@
 
 namespace encryption_helper {
 
+namespace {
+
+sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
+    const std::string& passphrase) {
+  syncer::KeyDerivationParams key_derivation_params =
+      syncer::KeyDerivationParams::CreateForPbkdf2();
+
+  std::unique_ptr<syncer::CryptographerImpl> cryptographer =
+      syncer::CryptographerImpl::FromSingleKeyForTesting(passphrase,
+                                                         key_derivation_params);
+  cryptographer->EmplaceKey(passphrase, key_derivation_params);
+
+  sync_pb::NigoriSpecifics specifics;
+  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
+                                     specifics.mutable_encryption_keybag()));
+
+  std::string serialized_keystore_decryptor =
+      cryptographer->ExportDefaultKey().SerializeAsString();
+
+  std::unique_ptr<syncer::CryptographerImpl> keystore_cryptographer =
+      syncer::CryptographerImpl::FromSingleKeyForTesting(passphrase,
+                                                         key_derivation_params);
+  EXPECT_TRUE(keystore_cryptographer->EncryptString(
+      serialized_keystore_decryptor,
+      specifics.mutable_keystore_decryptor_token()));
+
+  specifics.set_passphrase_type(sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE);
+  specifics.set_keystore_migration_time(
+      syncer::TimeToProtoTime(base::Time::Now()));
+  return specifics;
+}
+
+}  // namespace
+
 bool GetServerNigori(fake_server::FakeServer* fake_server,
                      sync_pb::NigoriSpecifics* nigori) {
   std::vector<sync_pb::SyncEntity> entity_list =
@@ -152,6 +187,14 @@ void SetNigoriInFakeServer(fake_server::FakeServer* fake_server,
   fake_server->ModifyEntitySpecifics(nigori_entity_id, nigori_entity_specifics);
 }
 
+void SetKeystoreNigoriInFakeServer(fake_server::FakeServer* fake_server) {
+  const std::vector<std::vector<uint8_t>>& keystore_keys =
+      fake_server->GetKeystoreKeys();
+  ASSERT_EQ(keystore_keys.size(), 1u);
+  const std::string passphrase = base::Base64Encode(keystore_keys.back());
+  SetNigoriInFakeServer(fake_server, BuildKeystoreNigoriSpecifics(passphrase));
+}
+
 }  // namespace encryption_helper
 
 ServerNigoriChecker::ServerNigoriChecker(

chrome/browser/sync/test/integration/encryption_helper.h

@@ -37,6 +37,10 @@ bool GetServerNigori(fake_server::FakeServer* fake_server,
 void SetNigoriInFakeServer(fake_server::FakeServer* fake_server,
                            const sync_pb::NigoriSpecifics& nigori);
 
+// Given a |fake_server|, sets the Nigori instance stored in it to a standard
+// Keystore Nigori.
+void SetKeystoreNigoriInFakeServer(fake_server::FakeServer* fake_server);
+
 // Given a |nigori| with CUSTOM_PASSPHRASE passphrase type, initializes the
 // given |cryptographer| with the key described in it. Since the key inside the
 // Nigori is encrypted (by design), the provided |passphrase| will be used to

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

@@ -48,18 +48,6 @@ void GetNewTab(Browser* browser, content::WebContents** web_contents) {
 }
 #endif  // !defined(OS_CHROMEOS)
 
-class PasswordSyncActiveChecker : public SingleClientStatusChangeChecker {
- public:
-  explicit PasswordSyncActiveChecker(syncer::ProfileSyncService* service)
-      : SingleClientStatusChangeChecker(service) {}
-  ~PasswordSyncActiveChecker() override = default;
-
-  // SingleClientStatusChangeChecker.
-  bool IsExitConditionSatisfied(std::ostream* os) override {
-    return service()->GetActiveDataTypes().Has(syncer::PASSWORDS);
-  }
-};
-
 // This test fixture is similar to SingleClientPasswordsSyncTest, but it also
 // sets up all the necessary test hooks etc for PasswordManager code (like
 // PasswordManagerBrowserTestBase), to allow for integration tests covering

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

@@ -7,6 +7,7 @@
 #include <sstream>
 #include <utility>
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
@@ -78,49 +79,44 @@ void ClearSyncDateField(std::vector<std::unique_ptr<PasswordForm>>* forms) {
   }
 }
 
-sync_pb::PasswordSpecifics SpecificsFromPassword(
+sync_pb::PasswordSpecificsData SpecificsDataFromPasswordForm(
     const autofill::PasswordForm& password_form) {
-  sync_pb::PasswordSpecifics specifics;
-  sync_pb::PasswordSpecificsData* password_data =
-      specifics.mutable_client_only_encrypted_data();
-  password_data->set_scheme(static_cast<int>(password_form.scheme));
-  password_data->set_signon_realm(password_form.signon_realm);
-  password_data->set_origin(password_form.origin.spec());
-  password_data->set_action(password_form.action.spec());
-  password_data->set_username_element(
+  sync_pb::PasswordSpecificsData password_data;
+  password_data.set_scheme(static_cast<int>(password_form.scheme));
+  password_data.set_signon_realm(password_form.signon_realm);
+  password_data.set_origin(password_form.origin.spec());
+  password_data.set_action(password_form.action.spec());
+  password_data.set_username_element(
       base::UTF16ToUTF8(password_form.username_element));
-  password_data->set_password_element(
+  password_data.set_password_element(
       base::UTF16ToUTF8(password_form.password_element));
-  password_data->set_username_value(
+  password_data.set_username_value(
       base::UTF16ToUTF8(password_form.username_value));
-  password_data->set_password_value(
+  password_data.set_password_value(
       base::UTF16ToUTF8(password_form.password_value));
-  password_data->set_date_last_used(
+  password_data.set_date_last_used(
       password_form.date_last_used.ToDeltaSinceWindowsEpoch().InMicroseconds());
-  password_data->set_date_created(
+  password_data.set_date_created(
       password_form.date_created.ToDeltaSinceWindowsEpoch().InMicroseconds());
-  password_data->set_blacklisted(password_form.blacklisted_by_user);
-  password_data->set_type(static_cast<int>(password_form.type));
-  password_data->set_times_used(password_form.times_used);
-  password_data->set_display_name(
-      base::UTF16ToUTF8(password_form.display_name));
-  password_data->set_avatar_url(password_form.icon_url.spec());
-  password_data->set_federation_url(
+  password_data.set_blacklisted(password_form.blacklisted_by_user);
+  password_data.set_type(static_cast<int>(password_form.type));
+  password_data.set_times_used(password_form.times_used);
+  password_data.set_display_name(base::UTF16ToUTF8(password_form.display_name));
+  password_data.set_avatar_url(password_form.icon_url.spec());
+  password_data.set_federation_url(
       password_form.federation_origin.opaque()
           ? std::string()
           : password_form.federation_origin.Serialize());
-  return specifics;
+  return password_data;
 }
 
 sync_pb::EntitySpecifics EncryptPasswordSpecifics(
-    const sync_pb::PasswordSpecifics& unencrypted_specifics,
+    const sync_pb::PasswordSpecificsData& password_data,
     const std::string& passphrase,
     const syncer::KeyDerivationParams& key_derivation_params) {
   std::unique_ptr<syncer::CryptographerImpl> cryptographer =
       syncer::CryptographerImpl::FromSingleKeyForTesting(passphrase,
                                                          key_derivation_params);
-  const sync_pb::PasswordSpecificsData& password_data =
-      unencrypted_specifics.client_only_encrypted_data();
   sync_pb::EntitySpecifics encrypted_specifics;
   encrypted_specifics.mutable_password()
       ->mutable_unencrypted_metadata()
@@ -130,7 +126,6 @@ sync_pb::EntitySpecifics EncryptPasswordSpecifics(
       encrypted_specifics.mutable_password()->mutable_encrypted());
   DCHECK(result);
   return encrypted_specifics;
-  return sync_pb::EntitySpecifics();
 }
 
 std::string GetClientTag(const sync_pb::PasswordSpecificsData& password_data) {
@@ -317,21 +312,45 @@ void InjectEncryptedServerPassword(
     const std::string& encryption_passphrase,
     const syncer::KeyDerivationParams& key_derivation_params,
     fake_server::FakeServer* fake_server) {
+  InjectEncryptedServerPassword(SpecificsDataFromPasswordForm(form),
+                                encryption_passphrase, key_derivation_params,
+                                fake_server);
+}
+
+void InjectEncryptedServerPassword(
+    const sync_pb::PasswordSpecificsData& password_data,
+    const std::string& encryption_passphrase,
+    const syncer::KeyDerivationParams& key_derivation_params,
+    fake_server::FakeServer* fake_server) {
   DCHECK(fake_server);
-  const sync_pb::PasswordSpecifics unencrypted_specifics =
-      SpecificsFromPassword(form);
   const sync_pb::EntitySpecifics encrypted_specifics = EncryptPasswordSpecifics(
-      unencrypted_specifics, encryption_passphrase, key_derivation_params);
+      password_data, encryption_passphrase, key_derivation_params);
   fake_server->InjectEntity(
       syncer::PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
-          /*non_unique_name=*/"encrypted",
-          GetClientTag(unencrypted_specifics.client_only_encrypted_data()),
+          /*non_unique_name=*/"encrypted", GetClientTag(password_data),
           encrypted_specifics,
           /*creation_time=*/0, /*last_modified_time=*/0));
 }
 
+void InjectKeystoreEncryptedServerPassword(
+    const sync_pb::PasswordSpecificsData& password_data,
+    fake_server::FakeServer* fake_server) {
+  InjectEncryptedServerPassword(
+      password_data, base::Base64Encode(fake_server->GetKeystoreKeys().back()),
+      syncer::KeyDerivationParams::CreateForPbkdf2(), fake_server);
+}
+
 }  // namespace passwords_helper
 
+PasswordSyncActiveChecker::PasswordSyncActiveChecker(
+    syncer::ProfileSyncService* service)
+    : SingleClientStatusChangeChecker(service) {}
+PasswordSyncActiveChecker::~PasswordSyncActiveChecker() = default;
+
+bool PasswordSyncActiveChecker::IsExitConditionSatisfied(std::ostream* os) {
+  return service()->GetActiveDataTypes().Has(syncer::PASSWORDS);
+}
+
 SamePasswordFormsChecker::SamePasswordFormsChecker()
     : MultiClientStatusChangeChecker(
           sync_datatype_helper::test()->GetSyncServices()),

chrome/browser/sync/test/integration/passwords_helper.h

@@ -100,15 +100,37 @@ int GetVerifierPasswordCount();
 autofill::PasswordForm CreateTestPasswordForm(int index);
 
 // Injects the password entity based on given |form| and encrypted with key
-// derived from |key_params| into |fake_server|.
+// derived from |key_derivation_params| into |fake_server|.
+// For Keystore encryption, the |encryption_passphrase| is the base64 encoding
+// of FakeServer::GetKeystoreKeys().back().
 void InjectEncryptedServerPassword(
     const autofill::PasswordForm& form,
     const std::string& encryption_passphrase,
     const syncer::KeyDerivationParams& key_derivation_params,
     fake_server::FakeServer* fake_server);
+// As above, but takes a PasswordSpecificsData instead of a PasswordForm.
+void InjectEncryptedServerPassword(
+    const sync_pb::PasswordSpecificsData& password_data,
+    const std::string& encryption_passphrase,
+    const syncer::KeyDerivationParams& key_derivation_params,
+    fake_server::FakeServer* fake_server);
+// As above, but using standard Keystore encryption.
+void InjectKeystoreEncryptedServerPassword(
+    const sync_pb::PasswordSpecificsData& password_data,
+    fake_server::FakeServer* fake_server);
 
 }  // namespace passwords_helper
 
+// Checker to wait until the PASSWORDS datatype becomes active.
+class PasswordSyncActiveChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit PasswordSyncActiveChecker(syncer::ProfileSyncService* service);
+  ~PasswordSyncActiveChecker() override;
+
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+};
+
 // TODO(crbug.com/1010490): avoid re-entrance protection in checkers below or
 // factor it out to not duplicate in every checker.
 // Checker to block until all profiles contain the same password forms.

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

@@ -200,22 +200,6 @@ class PageTitleChecker : public StatusChangeChecker,
   DISALLOW_COPY_AND_ASSIGN(PageTitleChecker);
 };
 
-class PasswordsDataTypeActiveChecker : public SingleClientStatusChangeChecker {
- public:
-  explicit PasswordsDataTypeActiveChecker(syncer::ProfileSyncService* service)
-      : SingleClientStatusChangeChecker(service) {}
-  ~PasswordsDataTypeActiveChecker() override {}
-
-  // StatusChangeChecker implementation.
-  bool IsExitConditionSatisfied(std::ostream* os) override {
-    *os << "Waiting for PASSWORDS to become active";
-    return service()->GetActiveDataTypes().Has(syncer::PASSWORDS);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(PasswordsDataTypeActiveChecker);
-};
-
 class SingleClientNigoriSyncTestWithUssTests
     : public SyncTest,
       public testing::WithParamInterface<bool> {
@@ -603,7 +587,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
       GetBrowser(0)->tab_strip_model()->GetActiveWebContents());
   EXPECT_TRUE(title_checker.Wait());
 
-  EXPECT_TRUE(PasswordsDataTypeActiveChecker(GetSyncService(0)).Wait());
+  EXPECT_TRUE(PasswordSyncActiveChecker(GetSyncService(0)).Wait());
   EXPECT_FALSE(GetSyncService(0)
                    ->GetUserSettings()
                    ->IsTrustedVaultKeyRequiredForPreferredDataTypes());

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

@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/base64.h"
 #include "base/macros.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
@@ -17,9 +16,7 @@
 #include "components/password_manager/core/browser/sync/password_sync_bridge.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
-#include "components/sync/base/time.h"
 #include "components/sync/driver/profile_sync_service.h"
-#include "components/sync/syncable/directory_cryptographer.h"
 #include "content/public/test/test_launcher.h"
 
 namespace {
@@ -40,42 +37,10 @@ using testing::IsEmpty;
 const syncer::SyncFirstSetupCompleteSource kSetSourceFromTest =
     syncer::SyncFirstSetupCompleteSource::BASIC_FLOW;
 
-syncer::KeyParams KeystoreKeyParams(const std::vector<uint8_t>& key) {
-  return {syncer::KeyDerivationParams::CreateForPbkdf2(),
-          base::Base64Encode(key)};
-}
-
-sync_pb::PasswordSpecifics EncryptPasswordSpecifics(
-    const syncer::KeyParams& key_params,
-    const sync_pb::PasswordSpecificsData& password_data) {
-  syncer::DirectoryCryptographer cryptographer;
-  cryptographer.AddKey(key_params);
-
-  sync_pb::PasswordSpecifics encrypted_specifics;
-  encrypted_specifics.mutable_unencrypted_metadata()->set_url(
-      password_data.signon_realm());
-  bool result = cryptographer.Encrypt(password_data,
-                                      encrypted_specifics.mutable_encrypted());
-  DCHECK(result);
-  return encrypted_specifics;
-}
-
-class PasswordSyncActiveChecker : public SingleClientStatusChangeChecker {
- public:
-  explicit PasswordSyncActiveChecker(syncer::ProfileSyncService* service)
-      : SingleClientStatusChangeChecker(service) {}
-  ~PasswordSyncActiveChecker() override = default;
-
-  // SingleClientStatusChangeChecker.
-  bool IsExitConditionSatisfied(std::ostream* os) override {
-    return service()->GetActiveDataTypes().Has(syncer::PASSWORDS);
-  }
-};
-
 class SingleClientPasswordsSyncTest : public SyncTest {
  public:
   SingleClientPasswordsSyncTest() : SyncTest(SINGLE_CLIENT) {}
-  ~SingleClientPasswordsSyncTest() override {}
+  ~SingleClientPasswordsSyncTest() override = default;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SingleClientPasswordsSyncTest);
@@ -251,7 +216,7 @@ class SingleClientPasswordsWithAccountStorageSyncTest : public SyncTest {
                                   kEnablePasswordsAccountStorage},
         /*disabled_features=*/{});
   }
-  ~SingleClientPasswordsWithAccountStorageSyncTest() override {}
+  ~SingleClientPasswordsWithAccountStorageSyncTest() override = default;
 
   void SetUpInProcessBrowserTestFixture() override {
     test_signin_client_factory_ =
@@ -264,34 +229,7 @@ class SingleClientPasswordsWithAccountStorageSyncTest : public SyncTest {
 #endif  // defined(OS_CHROMEOS)
     SyncTest::SetUpOnMainThread();
 
-    AddKeystoreNigoriToFakeServer();
-  }
-
-  void AddKeystoreNigoriToFakeServer() {
-    const std::vector<std::vector<uint8_t>>& keystore_keys =
-        GetFakeServer()->GetKeystoreKeys();
-    ASSERT_TRUE(keystore_keys.size() == 1);
-    const syncer::KeyParams key_params =
-        KeystoreKeyParams(keystore_keys.back());
-
-    sync_pb::NigoriSpecifics nigori;
-    nigori.set_keybag_is_frozen(true);
-    nigori.set_keystore_migration_time(1U);
-    nigori.set_encrypt_everything(false);
-    nigori.set_passphrase_type(sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE);
-
-    syncer::DirectoryCryptographer cryptographer;
-    ASSERT_TRUE(cryptographer.AddKey(key_params));
-    ASSERT_TRUE(cryptographer.AddNonDefaultKey(key_params));
-    ASSERT_TRUE(cryptographer.GetKeys(nigori.mutable_encryption_keybag()));
-
-    syncer::DirectoryCryptographer keystore_cryptographer;
-    ASSERT_TRUE(keystore_cryptographer.AddKey(key_params));
-    ASSERT_TRUE(keystore_cryptographer.EncryptString(
-        cryptographer.GetDefaultNigoriKeyData(),
-        nigori.mutable_keystore_decryptor_token()));
-
-    encryption_helper::SetNigoriInFakeServer(GetFakeServer(), nigori);
+    encryption_helper::SetKeystoreNigoriInFakeServer(GetFakeServer());
   }
 
   void AddTestPasswordToFakeServer() {
@@ -305,29 +243,11 @@ class SingleClientPasswordsWithAccountStorageSyncTest : public SyncTest {
     // Other data.
     password_data.set_password_value("password_value");
 
-    fake_server_->InjectEntity(
-        syncer::PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
-            /*non_unique_name=*/"encrypted", /*client_tag=*/
-            password_manager::PasswordSyncBridge::ComputeClientTagForTesting(
-                password_data),
-            CreateSpecifics(password_data),
-            /*creation_time=*/syncer::TimeToProtoTime(base::Time::Now()),
-            /*last_modified_time=*/syncer::TimeToProtoTime(base::Time::Now())));
+    passwords_helper::InjectKeystoreEncryptedServerPassword(password_data,
+                                                            GetFakeServer());
   }
 
  private:
-  sync_pb::EntitySpecifics CreateSpecifics(
-      const sync_pb::PasswordSpecificsData& password_data) const {
-    const syncer::KeyParams key_params =
-        KeystoreKeyParams(GetFakeServer()->GetKeystoreKeys().back());
-
-    sync_pb::EntitySpecifics specifics;
-    *specifics.mutable_password() =
-        EncryptPasswordSpecifics(key_params, password_data);
-
-    return specifics;
-  }
-
   base::test::ScopedFeatureList feature_list_;
 
   secondary_account_helper::ScopedSigninClientFactory