syncer::GetUploadToGoogleState: Check data type's encryption status

Before this CL, GetUploadToGoogleState only checked whether Sync
encryption (with a custom passphrase) was enabled at all. This wasn't
accurate for data types that are never encrypted, e.g. DEVICE_INFO or
AUTOFILL_WALLET_DATA. So this CL adds a per-data type encryption check.

One problem caused by this was that credit card autofill was broken for
users with a custom Sync passphrase. Note that credit card *upload*
should still be disabled for those users, so this CL adds an explicit
check for that.

Bug: 852815
Change-Id: I0d0119596c81fe32d9085b495a2e591d149bdad8
Reviewed-on: https://chromium-review.googlesource.com/1102328Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567691}

components/autofill/core/browser/autofill_experiments.cc

@@ -147,11 +147,7 @@ bool IsCreditCardUploadEnabled(const PrefService* pref_service,
     return false;
   }
 
-  // Check if the upload to Google state is active. This also returns false
-  // for users that have a secondary passphrase. Users who have enabled a
-  // passphrase have chosen to not make their sync information accessible to
-  // Google. Since upload makes credit card data available to other Google
-  // systems, disable it for passphrase users.
+  // Check if the upload to Google state is active.
   if (!base::FeatureList::IsEnabled(
           features::kAutofillEnablePaymentsInteractionsOnAuthError) &&
       syncer::GetUploadToGoogleState(sync_service,
@@ -160,6 +156,13 @@ bool IsCreditCardUploadEnabled(const PrefService* pref_service,
     return false;
   }
 
+  // Also don't offer upload for users that have a secondary sync passphrase.
+  // Users who have enabled a passphrase have chosen to not make their sync
+  // information accessible to Google. Since upload makes credit card data
+  // available to other Google systems, disable it for passphrase users.
+  if (sync_service->IsUsingSecondaryPassphrase())
+    return false;
+
   // Check Payments integration user setting.
   if (!pref_service->GetBoolean(prefs::kAutofillWalletImportEnabled))
     return false;

components/sync/driver/sync_service_utils.cc

@@ -19,7 +19,16 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
   if (!sync_service || !sync_service->CanSyncStart() ||
       sync_service->IsLocalSyncEnabled() ||
       !sync_service->GetPreferredDataTypes().Has(type) ||
-      sync_service->GetAuthError().IsPersistentError() ||
+      sync_service->GetAuthError().IsPersistentError()) {
+    return UploadState::NOT_ACTIVE;
+  }
+  // If the given ModelType is encrypted with a custom passphrase, we also
+  // consider uploading inactive, since Google can't read the data.
+  // Note that encryption is tricky: Some data types (e.g. PASSWORDS) are always
+  // encrypted, but not necessarily with a custom passphrase. On the other hand,
+  // some data types are never encrypted (e.g. DEVICE_INFO), even if the
+  // "encrypt everything" setting is enabled.
+  if (sync_service->GetEncryptedDataTypes().Has(type) &&
       sync_service->IsUsingSecondaryPassphrase()) {
     return UploadState::NOT_ACTIVE;
   }

components/sync/driver/sync_service_utils.h

@@ -21,15 +21,16 @@ enum class UploadState {
   // initializing, so e.g. we don't know about any auth errors yet.
   INITIALIZING,
   // We are not syncing to Google, and the caller should assume that we do not
-  // have consent to do so. This can have a number of reasons: e.g. sync as a
+  // have consent to do so. This can have a number of reasons, e.g.: sync as a
   // whole is disabled, or the given model type is disabled, or we're in
-  // "local sync" mode, or encryption with a custom passphrase is enabled (in
-  // which case we're technically still uploading, but Google can't inspect the
-  // data), or we're in a persistent auth error state. As one special case of an
-  // auth error, sync may be "paused" because the user signed out of the content
-  // area.
+  // "local sync" mode, or this data type is encrypted with a custom passphrase
+  // (in which case we're technically still uploading, but Google can't inspect
+  // the data), or we're in a persistent auth error state. As one special case
+  // of an auth error, sync may be "paused" because the user signed out of the
+  // content area.
   NOT_ACTIVE,
-  // We're actively syncing data to Google servers.
+  // We're actively syncing data to Google servers, in a form that is readable
+  // by Google.
   ACTIVE,
   // Used when logging histograms. Must have this exact name.
   kMaxValue = ACTIVE

components/sync/driver/sync_service_utils_unittest.cc

@@ -43,9 +43,14 @@ class TestSyncService : public FakeSyncService {
     return preferred_data_types_;
   }
   ModelTypeSet GetEncryptedDataTypes() const override {
-    if (!custom_passphrase_enabled_)
+    if (!custom_passphrase_enabled_) {
+      // PASSWORDS are always encrypted.
       return ModelTypeSet(syncer::PASSWORDS);
-    return preferred_data_types_;
+    }
+    // Some types can never be encrypted, e.g. DEVICE_INFO and
+    // AUTOFILL_WALLET_DATA, so make sure we don't report them as encrypted.
+    return syncer::Intersection(preferred_data_types_,
+                                syncer::EncryptableUserTypes());
   }
   SyncCycleSnapshot GetLastCycleSnapshot() const override {
     if (sync_cycle_complete_) {
@@ -217,6 +222,8 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfCustomPassphraseInUse) {
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
   ASSERT_EQ(UploadState::ACTIVE,
             GetUploadToGoogleState(&service, syncer::PASSWORDS));
+  ASSERT_EQ(UploadState::ACTIVE,
+            GetUploadToGoogleState(&service, syncer::DEVICE_INFO));
 
   // Once a custom passphrase is in use, upload should be considered disabled:
   // Even if we're technically still uploading, Google can't inspect the data.
@@ -226,6 +233,9 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfCustomPassphraseInUse) {
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
   EXPECT_EQ(UploadState::NOT_ACTIVE,
             GetUploadToGoogleState(&service, syncer::PASSWORDS));
+  // But unencryptable types like DEVICE_INFO are still active.
+  EXPECT_EQ(UploadState::ACTIVE,
+            GetUploadToGoogleState(&service, syncer::DEVICE_INFO));
 }
 
 }  // namespace syncer