Treat pending trusted vault keys as crypto errors

Prior to this patch, DataTypeManagerImpl::Restart() used
IsPassphraseRequired() as the way to determine whether encrypted
datatypes should enter the crypto-error state.

This patch generalizes the concept to accomodate the
trusted-vault-keys case, which doesn't have a corresponding API to
IsPassphraseRequired().

To achieve this without poluting the public APIs,
DataTypeEncryptionHandler is decoupled from SyncUserSettings.

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

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

@@ -15,6 +15,7 @@
 #include "chrome/browser/sync/test/integration/encryption_helper.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/ui/browser.h"
@@ -164,6 +165,25 @@ 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() override {
+    return service()->GetActiveDataTypes().Has(syncer::PASSWORDS);
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for PASSWORDS to become active";
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PasswordsDataTypeActiveChecker);
+};
+
 class SingleClientNigoriSyncTestWithUssTests
     : public SyncTest,
       public testing::WithParamInterface<bool> {
@@ -379,6 +399,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
   ASSERT_TRUE(TrustedVaultKeyRequiredStateChecker(GetSyncService(0),
                                                   /*desired_state=*/true)
                   .Wait());
+  ASSERT_FALSE(GetSyncService(0)->GetActiveDataTypes().Has(syncer::PASSWORDS));
 
   // Mimic opening a web page where the user can interact with the retrieval
   // flow.
@@ -393,9 +414,10 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
       GetBrowser(0)->tab_strip_model()->GetActiveWebContents());
   EXPECT_TRUE(title_checker.Wait());
 
-  ASSERT_TRUE(TrustedVaultKeyRequiredStateChecker(GetSyncService(0),
+  EXPECT_TRUE(TrustedVaultKeyRequiredStateChecker(GetSyncService(0),
                                                   /*desired_state=*/false)
                   .Wait());
+  EXPECT_TRUE(PasswordsDataTypeActiveChecker(GetSyncService(0)).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
@@ -436,6 +458,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
   EXPECT_FALSE(GetSyncService(0)
                    ->GetUserSettings()
                    ->IsTrustedVaultKeyRequiredForPreferredDataTypes());
+  EXPECT_TRUE(GetSyncService(0)->GetActiveDataTypes().Has(syncer::PASSWORDS));
 }
 
 // Same as SingleClientNigoriWithWebApiTest but does NOT override

components/sync/driver/data_type_encryption_handler.h

@@ -15,9 +15,11 @@ class DataTypeEncryptionHandler {
   DataTypeEncryptionHandler();
   virtual ~DataTypeEncryptionHandler();
 
-  // Returns whether a passphrase is required for encryption or decryption to
-  // proceed.
-  virtual bool IsPassphraseRequired() const = 0;
+  // Returns whether there is an error that prevents encryption or decryption
+  // from proceeding. This does not necessarily mean that the UI will display an
+  // error state, for example if there's a user-transparent attempt to resolve
+  // the crypto error.
+  virtual bool HasCryptoError() const = 0;
 
   // Returns the current set of encrypted data types.
   virtual ModelTypeSet GetEncryptedDataTypes() const = 0;

components/sync/driver/data_type_manager_impl.cc

@@ -302,7 +302,7 @@ void DataTypeManagerImpl::Restart() {
   }
 
   // Check for new or resolved data type crypto errors.
-  if (encryption_handler_->IsPassphraseRequired()) {
+  if (encryption_handler_->HasCryptoError()) {
     ModelTypeSet encrypted_types = encryption_handler_->GetEncryptedDataTypes();
     encrypted_types.RetainAll(last_requested_types_);
     encrypted_types.RemoveAll(data_type_status_table_.GetCryptoErrorTypes());

components/sync/driver/data_type_manager_impl_unittest.cc

@@ -180,27 +180,25 @@ class FakeDataTypeEncryptionHandler : public DataTypeEncryptionHandler {
   FakeDataTypeEncryptionHandler();
   ~FakeDataTypeEncryptionHandler() override;
 
-  bool IsPassphraseRequired() const override;
+  bool HasCryptoError() const override;
   ModelTypeSet GetEncryptedDataTypes() const override;
 
-  void set_passphrase_required(bool passphrase_required) {
-    passphrase_required_ = passphrase_required;
-  }
+  void set_crypto_error(bool crypto_error) { crypto_error_ = crypto_error; }
   void set_encrypted_types(ModelTypeSet encrypted_types) {
     encrypted_types_ = encrypted_types;
   }
 
  private:
-  bool passphrase_required_;
+  bool crypto_error_;
   ModelTypeSet encrypted_types_;
 };
 
 FakeDataTypeEncryptionHandler::FakeDataTypeEncryptionHandler()
-    : passphrase_required_(false) {}
+    : crypto_error_(false) {}
 FakeDataTypeEncryptionHandler::~FakeDataTypeEncryptionHandler() {}
 
-bool FakeDataTypeEncryptionHandler::IsPassphraseRequired() const {
-  return passphrase_required_;
+bool FakeDataTypeEncryptionHandler::HasCryptoError() const {
+  return crypto_error_;
 }
 
 ModelTypeSet FakeDataTypeEncryptionHandler::GetEncryptedDataTypes() const {
@@ -321,7 +319,7 @@ class SyncDataTypeManagerImplTest : public testing::Test {
   }
 
   void FailEncryptionFor(ModelTypeSet encrypted_types) {
-    encryption_handler_.set_passphrase_required(true);
+    encryption_handler_.set_crypto_error(true);
     encryption_handler_.set_encrypted_types(encrypted_types);
   }
 

components/sync/driver/profile_sync_service.cc

@@ -864,8 +864,8 @@ void ProfileSyncService::OnEngineInitialized(
 
   data_type_manager_ =
       sync_client_->GetSyncApiComponentFactory()->CreateDataTypeManager(
-          initial_types, debug_info_listener, &data_type_controllers_,
-          user_settings_.get(), engine_.get(), this);
+          initial_types, debug_info_listener, &data_type_controllers_, &crypto_,
+          engine_.get(), this);
 
   crypto_.SetSyncEngine(GetAuthenticatedAccountInfo(), engine_.get());
 

components/sync/driver/sync_service_crypto.cc

@@ -340,6 +340,27 @@ ModelTypeSet SyncServiceCrypto::GetEncryptedDataTypes() const {
   return state_.encrypted_types;
 }
 
+bool SyncServiceCrypto::HasCryptoError() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // This determines whether DataTypeManager should issue crypto errors for
+  // encrypted datatypes. This may differ from whether the UI represents the
+  // error state or not.
+
+  switch (state_.required_user_action) {
+    case RequiredUserAction::kNone:
+      return false;
+    case RequiredUserAction::kFetchingTrustedVaultKeys:
+    case RequiredUserAction::kTrustedVaultKeyRequired:
+    case RequiredUserAction::kPassphraseRequiredForDecryption:
+    case RequiredUserAction::kPassphraseRequiredForEncryption:
+      return true;
+  }
+
+  NOTREACHED();
+  return false;
+}
+
 void SyncServiceCrypto::OnPassphraseRequired(
     PassphraseRequiredReason reason,
     const KeyDerivationParams& key_derivation_params,

components/sync/driver/sync_service_crypto.h

@@ -14,6 +14,7 @@
 #include "base/sequence_checker.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/driver/data_type_encryption_handler.h"
 #include "components/sync/engine/configure_reason.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/engine/sync_engine.h"
@@ -26,7 +27,8 @@ class TrustedVaultClient;
 // This class functions as mostly independent component of SyncService that
 // handles things related to encryption, including holding lots of state and
 // encryption communications with the sync thread.
-class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
+class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
+                          public DataTypeEncryptionHandler {
  public:
   // |sync_prefs| must not be null and must outlive this object.
   // |trusted_vault_client| may be null, but if non-null, the pointee must
@@ -55,9 +57,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   // Returns the actual passphrase type being used for encryption.
   PassphraseType GetPassphraseType() const;
 
-  // Returns the current set of encrypted data types.
-  ModelTypeSet GetEncryptedDataTypes() const;
-
   // SyncEncryptionHandler::Observer implementation.
   void OnPassphraseRequired(
       PassphraseRequiredReason reason,
@@ -76,6 +75,10 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   void OnPassphraseTypeChanged(PassphraseType type,
                                base::Time passphrase_time) override;
 
+  // DataTypeEncryptionHandler implementation.
+  bool HasCryptoError() const override;
+  ModelTypeSet GetEncryptedDataTypes() const override;
+
   // Used to provide the engine when it is initialized.
   void SetSyncEngine(const CoreAccountInfo& account_info, SyncEngine* engine);
 

components/sync/driver/sync_user_settings.h

@@ -13,7 +13,6 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/passphrase_enums.h"
 #include "components/sync/base/user_selectable_type.h"
-#include "components/sync/driver/data_type_encryption_handler.h"
 
 namespace syncer {
 
@@ -30,9 +29,9 @@ enum class SyncFirstSetupCompleteSource {
 };
 
 // This class encapsulates all the user-configurable bits of Sync.
-class SyncUserSettings : public syncer::DataTypeEncryptionHandler {
+class SyncUserSettings {
  public:
-  ~SyncUserSettings() override = default;
+  virtual ~SyncUserSettings() = default;
 
   // Whether the user wants Sync to run. This is false by default, but gets set
   // to true early in the Sync setup flow, after the user has pressed "turn on
@@ -87,11 +86,11 @@ class SyncUserSettings : public syncer::DataTypeEncryptionHandler {
   virtual void EnableEncryptEverything() = 0;
 
   // The current set of encrypted data types.
-  ModelTypeSet GetEncryptedDataTypes() const override = 0;
+  virtual ModelTypeSet GetEncryptedDataTypes() const = 0;
   // Whether a passphrase is required for encryption or decryption to proceed.
   // Note that Sync might still be working fine if the user has disabled all
   // encrypted data types.
-  bool IsPassphraseRequired() const override = 0;
+  virtual bool IsPassphraseRequired() const = 0;
   // Whether a passphrase is required to decrypt the data for any currently
   // enabled data type.
   virtual bool IsPassphraseRequiredForPreferredDataTypes() const = 0;