Refactor SyncServiceCrypto to avoid PassphraseRequiredReason

No behavioral changes: the API and implementation of SyncServiceCrypto
are decoupled from the enum that is designed for SyncEncryptionHandler,
a lower-level interface.

Bug: 1010189
Change-Id: Icd8d3124a93a928535894f877b64ade53bcd1e53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834226
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701963}

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

@@ -6,6 +6,7 @@
 
 #include <cstddef>
 #include <sstream>
+#include <utility>
 
 #include "base/command_line.h"
 #include "base/json/json_writer.h"
@@ -98,8 +99,13 @@ class SyncSetupChecker : public SingleClientStatusChangeChecker {
     if (HasAuthError(service())) {
       return true;
     }
-    if (service()->GetPassphraseRequiredReasonForTest() ==
-        syncer::REASON_DECRYPTION) {
+    // TODO(crbug.com/1010397): The verification of INITIALIZING is only needed
+    // due to SyncEncryptionHandlerImpl issuing an unnecessary
+    // OnPassphraseRequired() during initialization.
+    if (service()
+            ->GetUserSettings()
+            ->IsPassphraseRequiredForPreferredDataTypes() &&
+        transport_state != syncer::SyncService::TransportState::INITIALIZING) {
       LOG(FATAL)
           << "A passphrase is required for decryption but was not provided. "
              "Waiting for sync to become available won't succeed. Make sure "
@@ -658,9 +664,8 @@ std::string ProfileSyncServiceHarness::GetClientInfoString(
        << ", server conflicts: " << snap.num_server_conflicts()
        << ", num_updates_downloaded : "
        << snap.model_neutral_state().num_updates_downloaded_total
-       << ", passphrase_required_reason: "
-       << syncer::PassphraseRequiredReasonToString(
-              service()->GetPassphraseRequiredReasonForTest())
+       << ", passphrase_required: "
+       << service()->GetUserSettings()->IsPassphraseRequired()
        << ", notifications_enabled: " << status.notifications_enabled
        << ", service_is_active: " << service()->IsSyncFeatureActive();
   } else {

components/sync/driver/profile_sync_service.cc

@@ -1334,11 +1334,6 @@ SyncCycleSnapshot ProfileSyncService::GetLastCycleSnapshotForDebugging() const {
   return last_snapshot_;
 }
 
-PassphraseRequiredReason
-ProfileSyncService::GetPassphraseRequiredReasonForTest() const {
-  return crypto_.passphrase_required_reason();
-}
-
 void ProfileSyncService::HasUnsyncedItemsForTest(
     base::OnceCallback<void(bool)> cb) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

components/sync/driver/profile_sync_service.h

@@ -222,8 +222,6 @@ class ProfileSyncService : public SyncService,
   bool IsPassphrasePrompted() const;
   void SetPassphrasePrompted(bool prompted);
 
-  PassphraseRequiredReason GetPassphraseRequiredReasonForTest() const;
-
   // Returns whether or not the underlying sync engine has made any
   // local changes to items that have not yet been synced with the
   // server.

components/sync/driver/sync_service_crypto.cc

@@ -151,6 +151,18 @@ base::Time SyncServiceCrypto::GetExplicitPassphraseTime() const {
   return state_.cached_explicit_passphrase_time;
 }
 
+bool SyncServiceCrypto::IsPassphraseRequired() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  switch (state_.required_user_action) {
+    case RequiredUserAction::kNone:
+      break;
+    case RequiredUserAction::kPassphraseRequiredForDecryption:
+    case RequiredUserAction::kPassphraseRequiredForEncryption:
+      return true;
+  }
+  return false;
+}
+
 bool SyncServiceCrypto::IsUsingSecondaryPassphrase() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return IsExplicitPassphrase(state_.cached_passphrase_type);
@@ -176,17 +188,20 @@ void SyncServiceCrypto::SetEncryptionPassphrase(const std::string& passphrase) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // This should only be called when the engine has been initialized.
   DCHECK(state_.engine);
-  DCHECK(state_.passphrase_required_reason != REASON_DECRYPTION)
+  DCHECK_NE(state_.required_user_action,
+            RequiredUserAction::kPassphraseRequiredForDecryption)
       << "Can not set explicit passphrase when decryption is needed.";
 
   DVLOG(1) << "Setting explicit passphrase for encryption.";
-  if (state_.passphrase_required_reason == REASON_ENCRYPTION) {
-    // REASON_ENCRYPTION implies that the cryptographer does not have pending
-    // keys. Hence, as long as we're not trying to do an invalid passphrase
-    // change (e.g. explicit -> explicit or explicit -> implicit), we know this
-    // will succeed. If for some reason a new encryption key arrives via
-    // sync later, the SBH will trigger another OnPassphraseRequired().
-    state_.passphrase_required_reason = REASON_PASSPHRASE_NOT_REQUIRED;
+  if (state_.required_user_action ==
+      RequiredUserAction::kPassphraseRequiredForEncryption) {
+    // |kPassphraseRequiredForEncryption| implies that the cryptographer does
+    // not have pending keys. Hence, as long as we're not trying to do an
+    // invalid passphrase change (e.g. explicit -> explicit or explicit ->
+    // implicit), we know this will succeed. If for some reason a new
+    // encryption key arrives via sync later, the SyncEncryptionHandler will
+    // trigger another OnPassphraseRequired().
+    state_.required_user_action = RequiredUserAction::kNone;
     notify_observers_.Run();
   }
 
@@ -270,7 +285,17 @@ void SyncServiceCrypto::OnPassphraseRequired(
 
   DVLOG(1) << "Passphrase required with reason: "
            << PassphraseRequiredReasonToString(reason);
-  state_.passphrase_required_reason = reason;
+
+  switch (reason) {
+    case REASON_ENCRYPTION:
+      state_.required_user_action =
+          RequiredUserAction::kPassphraseRequiredForEncryption;
+      break;
+    case REASON_DECRYPTION:
+      state_.required_user_action =
+          RequiredUserAction::kPassphraseRequiredForDecryption;
+      break;
+  }
 
   // Reconfigure without the encrypted types (excluded implicitly via the
   // failed datatypes handler).
@@ -283,9 +308,9 @@ void SyncServiceCrypto::OnPassphraseAccepted() {
   // Clear our cache of the cryptographer's pending keys.
   state_.cached_pending_keys.clear_blob();
 
-  // Reset |passphrase_required_reason| since we know we no longer require the
+  // Reset |required_user_action| since we know we no longer require the
   // passphrase.
-  state_.passphrase_required_reason = REASON_PASSPHRASE_NOT_REQUIRED;
+  state_.required_user_action = RequiredUserAction::kNone;
 
   // Make sure the data types that depend on the passphrase are started at
   // this time.

components/sync/driver/sync_service_crypto.h

@@ -35,6 +35,7 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
 
   // See the SyncService header.
   base::Time GetExplicitPassphraseTime() const;
+  bool IsPassphraseRequired() const;
   bool IsUsingSecondaryPassphrase() const;
   void EnableEncryptEverything();
   bool IsEncryptEverythingEnabled() const;
@@ -69,12 +70,15 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   // Creates a proxy observer object that will post calls to this thread.
   std::unique_ptr<SyncEncryptionHandler::Observer> GetEncryptionObserverProxy();
 
-  PassphraseRequiredReason passphrase_required_reason() const {
-    return state_.passphrase_required_reason;
-  }
   bool encryption_pending() const { return state_.encryption_pending; }
 
  private:
+  enum class RequiredUserAction {
+    kNone,
+    kPassphraseRequiredForDecryption,
+    kPassphraseRequiredForEncryption,
+  };
+
   // Calls SyncServiceBase::NotifyObservers(). Never null.
   const base::RepeatingClosure notify_observers_;
 
@@ -95,11 +99,7 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
     // Not-null when the engine is initialized.
     SyncEngine* engine = nullptr;
 
-    // Was the last SYNC_PASSPHRASE_REQUIRED notification sent because it
-    // was required for encryption, decryption with a cached passphrase, or
-    // because a new passphrase is required?
-    PassphraseRequiredReason passphrase_required_reason =
-        REASON_PASSPHRASE_NOT_REQUIRED;
+    RequiredUserAction required_user_action = RequiredUserAction::kNone;
 
     // The current set of encrypted types. Always a superset of
     // Cryptographer::SensitiveTypes().

components/sync/driver/sync_user_settings_impl.cc

@@ -125,8 +125,7 @@ void SyncUserSettingsImpl::EnableEncryptEverything() {
 }
 
 bool SyncUserSettingsImpl::IsPassphraseRequired() const {
-  return crypto_->passphrase_required_reason() !=
-         REASON_PASSPHRASE_NOT_REQUIRED;
+  return crypto_->IsPassphraseRequired();
 }
 
 bool SyncUserSettingsImpl::IsPassphraseRequiredForPreferredDataTypes() const {

components/sync/engine/sync_encryption_handler.h

@@ -21,7 +21,6 @@ enum class PassphraseType;
 
 // Reasons due to which Cryptographer might require a passphrase.
 enum PassphraseRequiredReason {
-  REASON_PASSPHRASE_NOT_REQUIRED = 0,  // Initial value.
   REASON_ENCRYPTION = 1,               // The cryptographer requires a
                                        // passphrase for its first attempt at
                                        // encryption. Happens only during

components/sync/engine/sync_string_conversions.cc

@@ -25,7 +25,6 @@ const char* ConnectionStatusToString(ConnectionStatus status) {
 // Helper function that converts a PassphraseRequiredReason value to a string.
 const char* PassphraseRequiredReasonToString(PassphraseRequiredReason reason) {
   switch (reason) {
-    ENUM_CASE(REASON_PASSPHRASE_NOT_REQUIRED);
     ENUM_CASE(REASON_ENCRYPTION);
     ENUM_CASE(REASON_DECRYPTION);
   }

components/sync/engine_impl/js_sync_encryption_handler_observer_unittest.cc

@@ -63,18 +63,11 @@ TEST_F(JsSyncEncryptionHandlerObserverTest, OnPassphraseRequired) {
   base::DictionaryValue reason_encryption_details;
   base::DictionaryValue reason_decryption_details;
 
-  reason_passphrase_not_required_details.SetString(
-      "reason",
-      PassphraseRequiredReasonToString(REASON_PASSPHRASE_NOT_REQUIRED));
   reason_encryption_details.SetString(
       "reason", PassphraseRequiredReasonToString(REASON_ENCRYPTION));
   reason_decryption_details.SetString(
       "reason", PassphraseRequiredReasonToString(REASON_DECRYPTION));
 
-  EXPECT_CALL(mock_js_event_handler_,
-              HandleJsEvent("onPassphraseRequired",
-                            HasDetailsAsDictionary(
-                                reason_passphrase_not_required_details)));
   EXPECT_CALL(mock_js_event_handler_,
               HandleJsEvent("onPassphraseRequired",
                             HasDetailsAsDictionary(reason_encryption_details)));
@@ -82,9 +75,6 @@ TEST_F(JsSyncEncryptionHandlerObserverTest, OnPassphraseRequired) {
               HandleJsEvent("onPassphraseRequired",
                             HasDetailsAsDictionary(reason_decryption_details)));
 
-  js_sync_encryption_handler_observer_.OnPassphraseRequired(
-      REASON_PASSPHRASE_NOT_REQUIRED, KeyDerivationParams::CreateForPbkdf2(),
-      sync_pb::EncryptedData());
   js_sync_encryption_handler_observer_.OnPassphraseRequired(
       REASON_ENCRYPTION, KeyDerivationParams::CreateForPbkdf2(),
       sync_pb::EncryptedData());