Sync+SupervisedUsers: Invert "EncryptEverythingAllowed" dependency

Previously, SupervisedUserService explicitly called
SetEncryptEverythingAllowed, requiring a dependency to SyncService.
Now, the "EncryptEverythingAllowed" bit is instead propagated via
SyncTypePreferenceProvider, which SupervisedUserService already
implements.

After this, there is only one remaining dependency from
SupervisedUserService to SyncService, namely adding/removing itself
as a PreferenceProvider. That will be cleaned up in a followup by
wiring it through the factories instead.

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

chrome/browser/supervised_user/supervised_user_service.cc

@@ -396,10 +396,6 @@ void SupervisedUserService::SetActive(bool active) {
     theme_service->UseDefaultTheme();
 #endif
 
-  syncer::SyncService* sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  sync_service->GetUserSettings()->SetEncryptEverythingAllowed(!active_);
-
   GetSettingsService()->SetActive(active_);
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -1000,6 +996,10 @@ syncer::ModelTypeSet SupervisedUserService::GetForcedDataTypes() const {
   return result;
 }
 
+bool SupervisedUserService::IsEncryptEverythingAllowed() const {
+  return !active_;
+}
+
 #if !defined(OS_ANDROID)
 void SupervisedUserService::OnBrowserSetLastActive(Browser* browser) {
   bool profile_became_active = profile_->IsSameProfile(browser->profile());

chrome/browser/supervised_user/supervised_user_service.h

@@ -175,6 +175,7 @@ class SupervisedUserService : public KeyedService,
 
   // SyncTypePreferenceProvider implementation:
   syncer::ModelTypeSet GetForcedDataTypes() const override;
+  bool IsEncryptEverythingAllowed() const override;
 
 #if !defined(OS_ANDROID)
   // BrowserListObserver implementation:

components/browser_sync/profile_sync_service.cc

@@ -247,6 +247,8 @@ void ProfileSyncService::Initialize() {
   user_settings_ = std::make_unique<syncer::SyncUserSettingsImpl>(
       &crypto_, &sync_prefs_, GetRegisteredDataTypes(),
       base::BindRepeating(&ProfileSyncService::SyncAllowedByPlatformChanged,
+                          base::Unretained(this)),
+      base::BindRepeating(&ProfileSyncService::IsEncryptEverythingAllowed,
                           base::Unretained(this)));
 
   sync_prefs_.AddSyncPrefObserver(this);
@@ -1311,6 +1313,17 @@ void ProfileSyncService::SyncAllowedByPlatformChanged(bool allowed) {
   }
 }
 
+bool ProfileSyncService::IsEncryptEverythingAllowed() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  for (const syncer::SyncTypePreferenceProvider* provider :
+       preference_providers_) {
+    if (!provider->IsEncryptEverythingAllowed()) {
+      return false;
+    }
+  }
+  return true;
+}
+
 void ProfileSyncService::ConfigureDataTypeManager(
     syncer::ConfigureReason reason) {
   syncer::ConfigureContext configure_context;

components/browser_sync/profile_sync_service.h

@@ -306,8 +306,9 @@ class ProfileSyncService : public syncer::SyncService,
   void AccountStateChanged();
   void CredentialsChanged();
 
-  // Callback for SyncPrefs.
+  // Callbacks for SyncUserSettingsImpl.
   void SyncAllowedByPlatformChanged(bool allowed);
+  bool IsEncryptEverythingAllowed() const;
 
   bool IsEngineAllowedToStart() const;
 

components/sync/driver/sync_service_crypto.cc

@@ -160,7 +160,6 @@ bool SyncServiceCrypto::IsUsingSecondaryPassphrase() const {
 
 void SyncServiceCrypto::EnableEncryptEverything() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(IsEncryptEverythingAllowed());
   DCHECK(state_.engine);
 
   // TODO(atwilson): Persist the encryption_pending flag to address the various
@@ -251,17 +250,6 @@ PassphraseType SyncServiceCrypto::GetPassphraseType() const {
   return state_.cached_passphrase_type;
 }
 
-bool SyncServiceCrypto::IsEncryptEverythingAllowed() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return state_.encrypt_everything_allowed;
-}
-
-void SyncServiceCrypto::SetEncryptEverythingAllowed(bool allowed) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(allowed || !state_.engine || !IsEncryptEverythingEnabled());
-  state_.encrypt_everything_allowed = allowed;
-}
-
 ModelTypeSet SyncServiceCrypto::GetEncryptedDataTypes() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(state_.encrypted_types.Has(PASSWORDS));
@@ -323,7 +311,6 @@ void SyncServiceCrypto::OnEncryptedTypesChanged(ModelTypeSet encrypted_types,
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   state_.encrypted_types = encrypted_types;
   state_.encrypt_everything = encrypt_everything;
-  DCHECK(state_.encrypt_everything_allowed || !state_.encrypt_everything);
   DVLOG(1) << "Encrypted types changed to "
            << ModelTypeSetToString(state_.encrypted_types)
            << " (encrypt everything is set to "

components/sync/driver/sync_service_crypto.h

@@ -44,13 +44,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   // Returns the actual passphrase type being used for encryption.
   PassphraseType GetPassphraseType() const;
 
-  // Returns true if encrypting all the sync data is allowed. If this method
-  // returns false, EnableEncryptEverything() should not be called.
-  bool IsEncryptEverythingAllowed() const;
-
-  // Sets whether encrypting all the sync data is allowed or not.
-  void SetEncryptEverythingAllowed(bool allowed);
-
   // Returns the current set of encrypted data types.
   ModelTypeSet GetEncryptedDataTypes() const;
 
@@ -114,9 +107,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
     // Cryptographer::SensitiveTypes().
     ModelTypeSet encrypted_types = SyncEncryptionHandler::SensitiveTypes();
 
-    // Whether encrypting everything is allowed.
-    bool encrypt_everything_allowed = true;
-
     // Whether we want to encrypt everything.
     bool encrypt_everything = false;
 

components/sync/driver/sync_type_preference_provider.h

@@ -12,6 +12,7 @@ namespace syncer {
 class SyncTypePreferenceProvider {
  public:
   virtual ModelTypeSet GetForcedDataTypes() const = 0;
+  virtual bool IsEncryptEverythingAllowed() const = 0;
 
  protected:
   virtual ~SyncTypePreferenceProvider() {}

components/sync/driver/sync_user_settings.h

@@ -57,7 +57,6 @@ class SyncUserSettings : public syncer::DataTypeEncryptionHandler {
   // Whether the user is allowed to encrypt all their Sync data. For example,
   // child accounts are not allowed to encrypt their data.
   virtual bool IsEncryptEverythingAllowed() const = 0;
-  virtual void SetEncryptEverythingAllowed(bool allowed) = 0;
   // Whether we are currently set to encrypt all the Sync data.
   virtual bool IsEncryptEverythingEnabled() const = 0;
   // Turns on encryption for all data. Callers must call SetChosenDataTypes()

components/sync/driver/sync_user_settings_impl.cc

@@ -14,11 +14,13 @@ SyncUserSettingsImpl::SyncUserSettingsImpl(
     SyncServiceCrypto* crypto,
     SyncPrefs* prefs,
     ModelTypeSet registered_types,
-    const base::RepeatingCallback<void(bool)>& sync_allowed_by_platform_changed)
+    const base::RepeatingCallback<void(bool)>& sync_allowed_by_platform_changed,
+    const base::RepeatingCallback<bool()>& is_encrypt_everything_allowed)
     : crypto_(crypto),
       prefs_(prefs),
       registered_types_(registered_types),
-      sync_allowed_by_platform_changed_cb_(sync_allowed_by_platform_changed) {
+      sync_allowed_by_platform_changed_cb_(sync_allowed_by_platform_changed),
+      is_encrypt_everything_allowed_cb_(is_encrypt_everything_allowed) {
   DCHECK(crypto_);
   DCHECK(prefs_);
 }
@@ -73,11 +75,7 @@ void SyncUserSettingsImpl::SetChosenDataTypes(bool sync_everything,
 }
 
 bool SyncUserSettingsImpl::IsEncryptEverythingAllowed() const {
-  return crypto_->IsEncryptEverythingAllowed();
-}
-
-void SyncUserSettingsImpl::SetEncryptEverythingAllowed(bool allowed) {
-  crypto_->SetEncryptEverythingAllowed(allowed);
+  return is_encrypt_everything_allowed_cb_.Run();
 }
 
 bool SyncUserSettingsImpl::IsEncryptEverythingEnabled() const {
@@ -85,6 +83,7 @@ bool SyncUserSettingsImpl::IsEncryptEverythingEnabled() const {
 }
 
 void SyncUserSettingsImpl::EnableEncryptEverything() {
+  DCHECK(IsEncryptEverythingAllowed());
   crypto_->EnableEncryptEverything();
 }
 

components/sync/driver/sync_user_settings_impl.h

@@ -19,11 +19,13 @@ class SyncServiceCrypto;
 class SyncUserSettingsImpl : public SyncUserSettings {
  public:
   // Both |crypto| and |prefs| must not be null, and must outlive this object.
-  SyncUserSettingsImpl(SyncServiceCrypto* crypto,
-                       SyncPrefs* prefs,
-                       ModelTypeSet registered_types,
-                       const base::RepeatingCallback<void(bool)>&
-                           sync_allowed_by_platform_changed);
+  SyncUserSettingsImpl(
+      SyncServiceCrypto* crypto,
+      SyncPrefs* prefs,
+      ModelTypeSet registered_types,
+      const base::RepeatingCallback<void(bool)>&
+          sync_allowed_by_platform_changed,
+      const base::RepeatingCallback<bool()>& is_encrypt_everything_allowed);
   ~SyncUserSettingsImpl() override;
 
   bool IsSyncRequested() const override;
@@ -40,7 +42,6 @@ class SyncUserSettingsImpl : public SyncUserSettings {
   void SetChosenDataTypes(bool sync_everything, ModelTypeSet types) override;
 
   bool IsEncryptEverythingAllowed() const override;
-  void SetEncryptEverythingAllowed(bool allowed) override;
   bool IsEncryptEverythingEnabled() const override;
   void EnableEncryptEverything() override;
 
@@ -65,6 +66,7 @@ class SyncUserSettingsImpl : public SyncUserSettings {
   SyncPrefs* const prefs_;
   const ModelTypeSet registered_types_;
   base::RepeatingCallback<void(bool)> sync_allowed_by_platform_changed_cb_;
+  base::RepeatingCallback<bool()> is_encrypt_everything_allowed_cb_;
 
   // Whether sync is currently allowed on this platform.
   bool sync_allowed_by_platform_ = true;

components/sync/driver/sync_user_settings_mock.h

@@ -31,7 +31,6 @@ class SyncUserSettingsMock : public SyncUserSettings {
   MOCK_METHOD2(SetChosenDataTypes, void(bool, syncer::ModelTypeSet));
 
   MOCK_CONST_METHOD0(IsEncryptEverythingAllowed, bool());
-  MOCK_METHOD1(SetEncryptEverythingAllowed, void(bool));
   MOCK_CONST_METHOD0(IsEncryptEverythingEnabled, bool());
   MOCK_METHOD0(EnableEncryptEverything, void());
 

components/sync/driver/test_sync_user_settings.h

@@ -34,7 +34,6 @@ class TestSyncUserSettings : public SyncUserSettings {
   void SetChosenDataTypes(bool sync_everything, ModelTypeSet types) override;
 
   bool IsEncryptEverythingAllowed() const override;
-  void SetEncryptEverythingAllowed(bool allowed) override;
   bool IsEncryptEverythingEnabled() const override;
   void EnableEncryptEverything() override;
 
@@ -49,6 +48,7 @@ class TestSyncUserSettings : public SyncUserSettings {
   bool SetDecryptionPassphrase(const std::string& passphrase) override;
 
   void SetFirstSetupComplete(bool first_setup_complete);
+  void SetEncryptEverythingAllowed(bool allowed);
   void SetPassphraseRequired(bool required);
   void SetPassphraseRequiredForDecryption(bool required);
   void SetIsUsingSecondaryPassphrase(bool enabled);