Adopt new commit infrastructure for setting custom passphrase

The newly introduced mechanism around PendingLocalNigoriCommit allows
implementing NigoriSyncBridgeImpl::SetEncryptionPassphrase(),
responsible for setting up a custom passphrase, in a way that:
a) Success is only reported when the sync server acks the commit.
b) Conflict-resolution becomes simple.
c) Logic is factored out from NigoriSyncBridgeImpl.

There is a risk that the round trip to the sync server introduces
noticeable UX latency when setting up a custom passphrase. However,
manual tests suggests that it's actually not noticeable.

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

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

@@ -1048,17 +1048,23 @@ bool ServerBookmarksEqualityChecker::IsExitConditionSatisfied() {
   // Make a copy so we can remove bookmarks that were found.
   std::vector<ExpectedBookmark> expected = expected_bookmarks_;
   for (const sync_pb::SyncEntity& entity : entities) {
-    // If the cryptographer was provided, we expect the specifics to have
-    // encrypted data.
-    EXPECT_EQ(entity.specifics().has_encrypted(), cryptographer_ != nullptr);
-
     sync_pb::BookmarkSpecifics actual_specifics;
     if (entity.specifics().has_encrypted()) {
+      // If no cryptographer was provided, we expect the specifics to have
+      // unencrypted data.
+      if (!cryptographer_) {
+        return false;
+      }
       sync_pb::EntitySpecifics entity_specifics;
       EXPECT_TRUE(cryptographer_->Decrypt(entity.specifics().encrypted(),
                                           &entity_specifics));
       actual_specifics = entity_specifics.bookmark();
     } else {
+      // If the cryptographer was provided, we expect the specifics to have
+      // encrypted data.
+      if (cryptographer_) {
+        return false;
+      }
       actual_specifics = entity.specifics().bookmark();
     }
 

components/sync/BUILD.gn

@@ -331,6 +331,7 @@ jumbo_static_library("rest_of_sync") {
     "nigori/nigori_sync_bridge.h",
     "nigori/nigori_sync_bridge_impl.cc",
     "nigori/nigori_sync_bridge_impl.h",
+    "nigori/pending_local_nigori_commit.cc",
     "nigori/pending_local_nigori_commit.h",
     "syncable/base_node.cc",
     "syncable/base_node.h",

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/base64.h"
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/sync/base/encryptor.h"
@@ -54,36 +55,6 @@ NigoriSpecifics MakeDefaultKeystoreNigori(
   return state.ToSpecificsProto();
 }
 
-// Returns the key derivation method to be used when a user sets a new
-// custom passphrase.
-KeyDerivationMethod GetDefaultKeyDerivationMethodForCustomPassphrase() {
-  if (base::FeatureList::IsEnabled(
-          switches::kSyncUseScryptForNewCustomPassphrases) &&
-      !base::FeatureList::IsEnabled(
-          switches::kSyncForceDisableScryptForCustomPassphrase)) {
-    return KeyDerivationMethod::SCRYPT_8192_8_11;
-  }
-
-  return KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003;
-}
-
-KeyDerivationParams CreateKeyDerivationParamsForCustomPassphrase(
-    const base::RepeatingCallback<std::string()>& random_salt_generator) {
-  KeyDerivationMethod method =
-      GetDefaultKeyDerivationMethodForCustomPassphrase();
-  switch (method) {
-    case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
-      return KeyDerivationParams::CreateForPbkdf2();
-    case KeyDerivationMethod::SCRYPT_8192_8_11:
-      return KeyDerivationParams::CreateForScrypt(random_salt_generator.Run());
-    case KeyDerivationMethod::UNSUPPORTED:
-      break;
-  }
-
-  NOTREACHED();
-  return KeyDerivationParams::CreateWithUnsupportedMethod();
-}
-
 KeyDerivationMethod GetKeyDerivationMethodFromSpecifics(
     const sync_pb::NigoriSpecifics& specifics) {
   KeyDerivationMethod key_derivation_method = ProtoKeyDerivationMethodToEnum(
@@ -525,68 +496,9 @@ bool NigoriSyncBridgeImpl::Init() {
 void NigoriSyncBridgeImpl::SetEncryptionPassphrase(
     const std::string& passphrase) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // TODO(crbug.com/922900): Adopt QueuePendingLocalCommit().
-  switch (state_.passphrase_type) {
-    case NigoriSpecifics::UNKNOWN:
-      NOTREACHED();
-      return;
-    case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
-    case NigoriSpecifics::CUSTOM_PASSPHRASE:
-      // Attempt to set the explicit passphrase when one was already set. It's
-      // possible if we received new NigoriSpecifics during the passphrase
-      // setup.
-      DVLOG(1) << "Attempt to set explicit passphrase failed, because one was "
-                  "already set.";
-      // TODO(crbug.com/922900): investigate whether we need to call
-      // OnPassphraseRequired() to prompt for decryption passphrase.
-      return;
-    case NigoriSpecifics::IMPLICIT_PASSPHRASE:
-    case NigoriSpecifics::KEYSTORE_PASSPHRASE:
-    case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
-      if (state_.pending_keys.has_value()) {
-        // TODO(crbug.com/922900): investigate whether we need to call
-        // MaybeNotifyOfPendingKeys() to prompt for decryption passphrase.
-        DVLOG(1) << "Attempt to set explicit passphrase failed, because there "
-                 << "are pending keys.";
-        return;
-      }
-      break;
-  }
-  DCHECK(!state_.pending_keys.has_value());
-  DCHECK(state_.cryptographer->CanEncrypt());
-
-  const KeyDerivationParams custom_passphrase_key_derivation_params =
-      CreateKeyDerivationParamsForCustomPassphrase(random_salt_generator_);
-
-  const std::string default_key_name = state_.cryptographer->EmplaceKey(
-      passphrase, custom_passphrase_key_derivation_params);
-  if (default_key_name.empty()) {
-    DLOG(ERROR) << "Failed to set encryption passphrase";
-    return;
-  }
-
-  state_.cryptographer->SelectDefaultEncryptionKey(default_key_name);
-  state_.pending_keystore_decryptor_token.reset();
-  state_.passphrase_type = NigoriSpecifics::CUSTOM_PASSPHRASE;
-  state_.custom_passphrase_key_derivation_params =
-      custom_passphrase_key_derivation_params;
-
-  state_.encrypt_everything = true;
-  state_.custom_passphrase_time = base::Time::Now();
-  processor_->Put(GetData());
-  storage_->StoreData(SerializeAsNigoriLocalData());
-
-  broadcasting_observer_->OnPassphraseAccepted();
-  broadcasting_observer_->OnPassphraseTypeChanged(
-      PassphraseType::kCustomPassphrase, state_.custom_passphrase_time);
-  broadcasting_observer_->OnCryptographerStateChanged(
-      state_.cryptographer.get(), state_.pending_keys.has_value());
-  broadcasting_observer_->OnEncryptedTypesChanged(EncryptableUserTypes(),
-                                                  state_.encrypt_everything);
 
-  UMA_HISTOGRAM_BOOLEAN("Sync.CustomEncryption", true);
-  // OnLocalSetPassphraseEncryption() is intentionally not called here, because
-  // it's needed only for the Directory implementation unit tests.
+  QueuePendingLocalCommit(PendingLocalNigoriCommit::ForSetCustomPassphrase(
+      passphrase, random_salt_generator_));
 }
 
 void NigoriSyncBridgeImpl::SetDecryptionPassphrase(

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -878,6 +878,8 @@ INSTANTIATE_TEST_SUITE_P(Scrypt,
 // passphrase.
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldPutAndNotifyObserversWhenSetEncryptionPassphrase) {
+  const std::string kCustomPassphrase = "passphrase";
+
   EntityData default_entity_data;
   *default_entity_data.specifics.mutable_nigori() =
       sync_pb::NigoriSpecifics::default_instance();
@@ -886,7 +888,13 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(base::nullopt));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
 
-  const std::string passphrase = "passphrase";
+  // Calling SetEncryptionPassphrase() triggers a commit cycle but doesn't
+  // immediately expose the new state, until the commit completes.
+  EXPECT_CALL(*processor(), Put(HasCustomPassphraseNigori()));
+  bridge()->SetEncryptionPassphrase(kCustomPassphrase);
+  EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
+
+  // Mimic commit completion.
   EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
                                /*encrypted_types=*/EncryptableUserTypes(),
@@ -898,8 +906,7 @@ TEST_F(NigoriSyncBridgeImplTest,
                                       /*passphrase_time=*/Not(NullTime())));
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(Ne(std::string()),
                                                    PASSPHRASE_BOOTSTRAP_TOKEN));
-  EXPECT_CALL(*processor(), Put(HasCustomPassphraseNigori()));
-  bridge()->SetEncryptionPassphrase(passphrase);
+  EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));
   EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
 
   // TODO(crbug.com/922900): find a good way to get key derivation method and
@@ -1179,6 +1186,13 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
 
+  // Calling SetEncryptionPassphrase() triggers a commit cycle but doesn't
+  // immediately expose the new state, until the commit completes.
+  EXPECT_CALL(*processor(), Put(HasCustomPassphraseNigori()));
+  bridge()->SetEncryptionPassphrase(kCustomPassphrase);
+  EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
+
+  // Mimic commit completion.
   EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
                                /*encrypted_types=*/EncryptableUserTypes(),
@@ -1190,8 +1204,7 @@ TEST_F(NigoriSyncBridgeImplTest,
                                       /*passphrase_time=*/Not(NullTime())));
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(Ne(std::string()),
                                                    PASSPHRASE_BOOTSTRAP_TOKEN));
-  EXPECT_CALL(*processor(), Put(HasCustomPassphraseNigori()));
-  bridge()->SetEncryptionPassphrase(kCustomPassphrase);
+  EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));
   EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
 }
 

components/sync/nigori/pending_local_nigori_commit.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/nigori/pending_local_nigori_commit.h"
+
+#include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/time/time.h"
+#include "components/sync/base/sync_base_switches.h"
+#include "components/sync/engine/sync_engine_switches.h"
+#include "components/sync/nigori/cryptographer_impl.h"
+#include "components/sync/nigori/nigori_state.h"
+
+namespace syncer {
+
+namespace {
+
+using sync_pb::NigoriSpecifics;
+
+// Returns the key derivation method to be used when a user sets a new
+// custom passphrase.
+KeyDerivationMethod GetDefaultKeyDerivationMethodForCustomPassphrase() {
+  if (base::FeatureList::IsEnabled(
+          switches::kSyncUseScryptForNewCustomPassphrases) &&
+      !base::FeatureList::IsEnabled(
+          switches::kSyncForceDisableScryptForCustomPassphrase)) {
+    return KeyDerivationMethod::SCRYPT_8192_8_11;
+  }
+
+  return KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003;
+}
+
+KeyDerivationParams CreateKeyDerivationParamsForCustomPassphrase(
+    const base::RepeatingCallback<std::string()>& random_salt_generator) {
+  KeyDerivationMethod method =
+      GetDefaultKeyDerivationMethodForCustomPassphrase();
+  switch (method) {
+    case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
+      return KeyDerivationParams::CreateForPbkdf2();
+    case KeyDerivationMethod::SCRYPT_8192_8_11:
+      return KeyDerivationParams::CreateForScrypt(random_salt_generator.Run());
+    case KeyDerivationMethod::UNSUPPORTED:
+      break;
+  }
+
+  NOTREACHED();
+  return KeyDerivationParams::CreateWithUnsupportedMethod();
+}
+
+class CustomPassphraseSetter : public PendingLocalNigoriCommit {
+ public:
+  CustomPassphraseSetter(
+      const std::string& passphrase,
+      const base::RepeatingCallback<std::string()>& random_salt_generator)
+      : passphrase_(passphrase),
+        key_derivation_params_(CreateKeyDerivationParamsForCustomPassphrase(
+            random_salt_generator)) {}
+
+  ~CustomPassphraseSetter() override = default;
+
+  bool TryApply(NigoriState* state) const override {
+    if (state->pending_keys.has_value()) {
+      return false;
+    }
+
+    switch (state->passphrase_type) {
+      case NigoriSpecifics::UNKNOWN:
+        return false;
+      case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
+      case NigoriSpecifics::CUSTOM_PASSPHRASE:
+        // Attempt to set the explicit passphrase when one was already set. It's
+        // possible if we received new NigoriSpecifics during the passphrase
+        // setup.
+        DVLOG(1)
+            << "Attempt to set explicit passphrase failed, because one was "
+               "already set.";
+        return false;
+      case NigoriSpecifics::IMPLICIT_PASSPHRASE:
+      case NigoriSpecifics::KEYSTORE_PASSPHRASE:
+      case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
+        break;
+    }
+
+    const std::string default_key_name =
+        state->cryptographer->EmplaceKey(passphrase_, key_derivation_params_);
+    if (default_key_name.empty()) {
+      DLOG(ERROR) << "Failed to set encryption passphrase";
+      return false;
+    }
+
+    state->cryptographer->SelectDefaultEncryptionKey(default_key_name);
+    state->pending_keystore_decryptor_token.reset();
+    state->passphrase_type = NigoriSpecifics::CUSTOM_PASSPHRASE;
+    state->custom_passphrase_key_derivation_params = key_derivation_params_;
+    state->encrypt_everything = true;
+    state->custom_passphrase_time = base::Time::Now();
+
+    return true;
+  }
+
+  void OnSuccess(const NigoriState& state,
+                 SyncEncryptionHandler::Observer* observer) override {
+    DCHECK(!state.pending_keys.has_value());
+
+    observer->OnPassphraseAccepted();
+    observer->OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
+                                      state.custom_passphrase_time);
+    observer->OnCryptographerStateChanged(state.cryptographer.get(),
+                                          /*has_pending_keys=*/false);
+    observer->OnEncryptedTypesChanged(EncryptableUserTypes(),
+                                      /*encrypt_everything=*/true);
+
+    UMA_HISTOGRAM_BOOLEAN("Sync.CustomEncryption", true);
+
+    // OnLocalSetPassphraseEncryption() is intentionally not called here,
+    // because it's needed only for the Directory implementation unit tests.
+  }
+
+  void OnFailure(SyncEncryptionHandler::Observer* observer) override {
+    // TODO(crbug.com/922900): investigate whether we need to call
+    // OnPassphraseRequired() to prompt for decryption passphrase.
+  }
+
+ private:
+  const std::string passphrase_;
+  const KeyDerivationParams key_derivation_params_;
+
+  DISALLOW_COPY_AND_ASSIGN(CustomPassphraseSetter);
+};
+
+}  // namespace
+
+// static
+std::unique_ptr<PendingLocalNigoriCommit>
+PendingLocalNigoriCommit::ForSetCustomPassphrase(
+    const std::string& passphrase,
+    const base::RepeatingCallback<std::string()>& random_salt_generator) {
+  return std::make_unique<CustomPassphraseSetter>(passphrase,
+                                                  random_salt_generator);
+}
+
+}  // namespace syncer

components/sync/nigori/pending_local_nigori_commit.h

@@ -5,8 +5,10 @@
 #ifndef COMPONENTS_SYNC_NIGORI_PENDING_LOCAL_NIGORI_COMMIT_H_
 #define COMPONENTS_SYNC_NIGORI_PENDING_LOCAL_NIGORI_COMMIT_H_
 
+#include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 
@@ -18,6 +20,10 @@ struct NigoriState;
 // is pending a commit to the sync server.
 class PendingLocalNigoriCommit {
  public:
+  static std::unique_ptr<PendingLocalNigoriCommit> ForSetCustomPassphrase(
+      const std::string& passphrase,
+      const base::RepeatingCallback<std::string()>& random_salt_generator);
+
   PendingLocalNigoriCommit() = default;
   virtual ~PendingLocalNigoriCommit() = default;