Reland "Add test for keystore key rotation"

This is a reland of b9c8d354

CL was reverted because of test failures on many builders. The reason
for this failures is ModifyEntitySpecifics() being wrapped in DCHECK(),
which makes new test extremely flaky. Solution: unwrap it.

Original change's description:
> Add test for keystore key rotation
>
> New test mimics server-side behavior for triggering keystore key
> rotation: generation of new keystore key with touching the Nigori
> node. After that client should receive updated keystore keys and the
> same Nigori node as it already has (modulo metadata update). The client
> should commit the new Nigori node with rotated keystore key and this
> verified by checking |encryption_keybag| encryption key name.
>
> Bug: 922900
> Change-Id: I91b86bbb0f68a55fdcc10049fa90afa70dc32155
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1887611
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
> Auto-Submit: Maksim Moskvitin <mmoskvitin@google.com>
> Cr-Commit-Position: refs/heads/master@{#712502}

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

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

@@ -161,6 +161,28 @@ bool ServerNigoriChecker::IsExitConditionSatisfied(std::ostream* os) {
              expected_passphrase_type_;
 }
 
+ServerNigoriKeyNameChecker::ServerNigoriKeyNameChecker(
+    const std::string& expected_key_name,
+    syncer::ProfileSyncService* service,
+    fake_server::FakeServer* fake_server)
+    : SingleClientStatusChangeChecker(service),
+      fake_server_(fake_server),
+      expected_key_name_(expected_key_name) {}
+
+bool ServerNigoriKeyNameChecker::IsExitConditionSatisfied(std::ostream* os) {
+  std::vector<sync_pb::SyncEntity> nigori_entities =
+      fake_server_->GetPermanentSyncEntitiesByModelType(syncer::NIGORI);
+  DCHECK_EQ(nigori_entities.size(), 1U);
+
+  const std::string given_key_name =
+      nigori_entities[0].specifics().nigori().encryption_keybag().key_name();
+
+  *os << "Waiting for a Nigori node with proper key bag encryption key name ("
+      << expected_key_name_ << ") to become available on the server."
+      << "The server key bag encryption key name is " << given_key_name << ".";
+  return given_key_name == expected_key_name_;
+}
+
 PassphraseRequiredStateChecker::PassphraseRequiredStateChecker(
     syncer::ProfileSyncService* service,
     bool desired_state)

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

@@ -64,6 +64,21 @@ class ServerNigoriChecker : public SingleClientStatusChangeChecker {
   const syncer::PassphraseType expected_passphrase_type_;
 };
 
+// Checker used to block until a Nigori with a given keybag encryption key name
+// is available on the server.
+class ServerNigoriKeyNameChecker : public SingleClientStatusChangeChecker {
+ public:
+  ServerNigoriKeyNameChecker(const std::string& expected_key_name,
+                             syncer::ProfileSyncService* service,
+                             fake_server::FakeServer* fake_server);
+
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+
+ private:
+  fake_server::FakeServer* const fake_server_;
+  const std::string expected_key_name_;
+};
+
 // Checker used to block until Sync requires or stops requiring a passphrase.
 class PassphraseRequiredStateChecker : public SingleClientStatusChangeChecker {
  public:

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

@@ -69,6 +69,14 @@ KeyParams KeystoreKeyParams(const std::string& key) {
           std::move(encoded_key)};
 }
 
+std::string ComputeKeyName(const KeyParams& key_params) {
+  std::string key_name;
+  syncer::Nigori::CreateByDerivation(key_params.derivation_params,
+                                     key_params.password)
+      ->Permute(syncer::Nigori::Password, syncer::kNigoriKeyName, &key_name);
+  return key_name;
+}
+
 // Builds NigoriSpecifics with following fields:
 // 1. encryption_keybag contains all keys derived from |keybag_keys_params|
 // and encrypted with a key derived from |keybag_decryptor_params|.
@@ -328,6 +336,22 @@ IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
   EXPECT_TRUE(WaitForPasswordForms({password_form}));
 }
 
+IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
+                       ShouldRotateKeystoreKey) {
+  ASSERT_TRUE(SetupSync());
+
+  GetFakeServer()->TriggerKeystoreKeyRotation();
+  const std::vector<std::string>& keystore_keys =
+      GetFakeServer()->GetKeystoreKeys();
+  ASSERT_THAT(keystore_keys, SizeIs(2));
+  const KeyParams new_keystore_key_params = KeystoreKeyParams(keystore_keys[1]);
+  const std::string expected_key_bag_key_name =
+      ComputeKeyName(new_keystore_key_params);
+  EXPECT_TRUE(ServerNigoriKeyNameChecker(expected_key_bag_key_name,
+                                         GetSyncService(0), GetFakeServer())
+                  .Wait());
+}
+
 IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
                        ShouldExposeExperimentalAuthenticationKey) {
   const std::vector<std::string>& keystore_keys =

components/sync/engine_impl/loopback_server/loopback_server.cc

@@ -374,6 +374,10 @@ void LoopbackServer::EnableStrongConsistencyWithConflictDetectionModel() {
   strong_consistency_model_enabled_ = true;
 }
 
+void LoopbackServer::AddNewKeystoreKeyForTesting() {
+  keystore_keys_.push_back(GenerateNewKeystoreKey());
+}
+
 bool LoopbackServer::HandleGetUpdatesRequest(
     const sync_pb::GetUpdatesMessage& get_updates,
     const std::string& store_birthday,

components/sync/engine_impl/loopback_server/loopback_server.h

@@ -78,6 +78,8 @@ class LoopbackServer {
     return keystore_keys_;
   }
 
+  void AddNewKeystoreKeyForTesting();
+
  private:
   // Allow the FakeServer decorator to inspect the internals of this class.
   friend class fake_server::FakeServer;

components/sync/test/fake_server/fake_server.cc

@@ -364,6 +364,20 @@ const std::vector<std::string>& FakeServer::GetKeystoreKeys() const {
   return loopback_server_->GetKeystoreKeysForTesting();
 }
 
+void FakeServer::TriggerKeystoreKeyRotation() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  loopback_server_->AddNewKeystoreKeyForTesting();
+
+  std::vector<sync_pb::SyncEntity> nigori_entities =
+      loopback_server_->GetPermanentSyncEntitiesByModelType(syncer::NIGORI);
+
+  DCHECK_EQ(nigori_entities.size(), 1U);
+  bool success = ModifyEntitySpecifics(
+      loopback_server_->GetTopLevelPermanentItemId(syncer::NIGORI),
+      nigori_entities[0].specifics());
+  DCHECK(success);
+}
+
 void FakeServer::InjectEntity(std::unique_ptr<LoopbackServerEntity> entity) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(entity->GetModelType() != syncer::AUTOFILL_WALLET_DATA)

components/sync/test/fake_server/fake_server.h

@@ -100,6 +100,10 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // Returns all keystore keys from the server.
   const std::vector<std::string>& GetKeystoreKeys() const;
 
+  // Triggers the keystore key rotation events on the server side: generating
+  // new keystore key and touching the Nigori node.
+  void TriggerKeystoreKeyRotation();
+
   // Adds |entity| to the server's collection of entities. This method makes no
   // guarantees that the added entity will result in successful server
   // operations.