[AF] Use account id hash as key for wallet sync transport opt-in pref.

Bug: 907929
Change-Id: I6cc9d7a908010c204fc8060802c7ee453363ffa3
Reviewed-on: https://chromium-review.googlesource.com/c/1349455
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612769}

components/autofill/core/common/BUILD.gn

@@ -58,6 +58,7 @@ jumbo_static_library("common") {
     "//components/pref_registry",
     "//components/prefs",
     "//components/variations",
+    "//crypto",
     "//ui/base",
     "//ui/gfx/geometry",
     "//url",

components/autofill/core/common/DEPS

+include_rules = [
+  "+crypto/sha2.h",
+]

components/autofill/core/common/autofill_prefs.cc

@@ -7,27 +7,32 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "crypto/sha2.h"
 
 namespace autofill {
 namespace prefs {
 namespace {
 
-// TODO(crbug.com/907929): Use a hash of the account id.
-// Returns the opt-in bitfield for the specifiec |account_id| or 0 if no entry
+// Returns the opt-in bitfield for the specific |account_id| or 0 if no entry
 // was found.
 int GetSyncTransportOptInBitFieldForAccount(const PrefService* prefs,
-                                            const std::string& account_id) {
-  auto* upload_events =
-      prefs->GetDictionary(prefs::kAutofillSyncTransportOptIn);
+                                            const std::string& account_hash) {
+  // We should always be passed an account hash, as we don't want to store the
+  // account id in clear in the prefs.
+  DCHECK_EQ(crypto::kSHA256Length, account_hash.size());
+
+  auto* dictionary = prefs->GetDictionary(prefs::kAutofillSyncTransportOptIn);
 
-  // If there is no dictionary or no entry in the dictionary, it means the
-  // account didn't opt-in. Use 0 because it's the same as not having opted-in
-  // to anything.
-  if (!upload_events) {
+  // If there is no dictionary it means the account didn't opt-in. Use 0 because
+  // it's the same as not having opted-in to anything.
+  if (!dictionary) {
     return 0;
   }
+
+  // If there is no entry in the dictionary, it means the account didn't opt-in.
+  // Use 0 because it's the same as not having opted-in to anything.
   auto* found =
-      upload_events->FindKeyOfType(account_id, base::Value::Type::INTEGER);
+      dictionary->FindKeyOfType(account_hash, base::Value::Type::INTEGER);
   return found ? found->GetInt() : 0;
 }
 
@@ -265,12 +270,17 @@ std::string GetAllProfilesValidityMapsEncodedString(const PrefService* prefs) {
 void SetUserOptedInWalletSyncTransport(PrefService* prefs,
                                        const std::string& account_id,
                                        bool opted_in) {
+  // Get the hash of the account id. The hashing here is only a secondary bit of
+  // obfuscation. The primary privacy guarantees are handled by clearing this
+  // whenever cookies are cleared.
+  const std::string account_hash = crypto::SHA256HashString(account_id);
+
   DictionaryPrefUpdate update(prefs, prefs::kAutofillSyncTransportOptIn);
-  int value = GetSyncTransportOptInBitFieldForAccount(prefs, account_id);
+  int value = GetSyncTransportOptInBitFieldForAccount(prefs, account_hash);
 
   // If the user has opted in, set that bit while leaving the others intact.
   if (opted_in) {
-    update->SetKey(account_id,
+    update->SetKey(account_hash,
                    base::Value(value | sync_transport_opt_in::kWallet));
     return;
   }
@@ -278,17 +288,20 @@ void SetUserOptedInWalletSyncTransport(PrefService* prefs,
   // Invert the mask in order to reset the Wallet bit while leaving the other
   // bits intact, or remove the key entirely if the Wallet was the only opt-in.
   if (value & ~sync_transport_opt_in::kWallet) {
-    update->SetKey(account_id,
+    update->SetKey(account_hash,
                    base::Value(value & ~sync_transport_opt_in::kWallet));
   } else {
-    update->RemoveKey(account_id);
+    update->RemoveKey(account_hash);
   }
 }
 
 bool IsUserOptedInWalletSyncTransport(const PrefService* prefs,
                                       const std::string& account_id) {
+  // Get the hash of the account id.
+  const std::string account_hash = crypto::SHA256HashString(account_id);
+
   // Return whether the wallet opt-in bit is set.
-  return GetSyncTransportOptInBitFieldForAccount(prefs, account_id) &
+  return GetSyncTransportOptInBitFieldForAccount(prefs, account_hash) &
          sync_transport_opt_in::kWallet;
 }
 

components/autofill/core/common/autofill_prefs_unittest.cc

@@ -83,48 +83,68 @@ TEST_F(AutofillPrefsTest, WalletSyncTransportPref_GetAndSet) {
   ASSERT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
   ASSERT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account2));
   // There should be no entry for the accounts in the dictionary.
-  auto* upload_events =
-      pref_service()->GetDictionary(prefs::kAutofillSyncTransportOptIn);
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account1, base::Value::Type::INTEGER));
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account2, base::Value::Type::INTEGER));
+  EXPECT_TRUE(pref_service()
+                  ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                  ->DictEmpty());
 
   // Set the opt-in for the first account.
   SetUserOptedInWalletSyncTransport(pref_service(), account1, true);
   EXPECT_TRUE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
   EXPECT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account2));
-  // There should be an entry for the first account only in the dictionary.
-  upload_events =
-      pref_service()->GetDictionary(prefs::kAutofillSyncTransportOptIn);
-  EXPECT_NE(nullptr,
-            upload_events->FindKeyOfType(account1, base::Value::Type::INTEGER));
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account2, base::Value::Type::INTEGER));
+  // There should only be one entry in the dictionary.
+  EXPECT_EQ(1U, pref_service()
+                    ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                    ->DictSize());
 
   // Unset the opt-in for the first account.
   SetUserOptedInWalletSyncTransport(pref_service(), account1, false);
   EXPECT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
   EXPECT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account2));
   // There should be no entry for the accounts in the dictionary.
-  upload_events =
-      pref_service()->GetDictionary(prefs::kAutofillSyncTransportOptIn);
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account1, base::Value::Type::INTEGER));
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account2, base::Value::Type::INTEGER));
+  EXPECT_TRUE(pref_service()
+                  ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                  ->DictEmpty());
 
   // Set the opt-in for the second account.
   SetUserOptedInWalletSyncTransport(pref_service(), account2, true);
   EXPECT_FALSE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
   EXPECT_TRUE(IsUserOptedInWalletSyncTransport(pref_service(), account2));
-  // There should be an entry for the second account only in the dictionary.
-  upload_events =
+  // There should only be one entry in the dictionary.
+  EXPECT_EQ(1U, pref_service()
+                    ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                    ->DictSize());
+
+  // Set the opt-in for the first account too.
+  SetUserOptedInWalletSyncTransport(pref_service(), account1, true);
+  EXPECT_TRUE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
+  EXPECT_TRUE(IsUserOptedInWalletSyncTransport(pref_service(), account1));
+  // There should be tow entries in the dictionary.
+  EXPECT_EQ(2U, pref_service()
+                    ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                    ->DictSize());
+}
+
+// Tests that AutofillSyncTransportOptIn is not stored using the plain text
+// account id.
+TEST_F(AutofillPrefsTest, WalletSyncTransportPref_UsesHashAccountId) {
+  const std::string account1 = "account1";
+
+  // There should be no opt-in recorded at first.
+  EXPECT_TRUE(pref_service()
+                  ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                  ->DictEmpty());
+
+  // Set the opt-in for the first account.
+  SetUserOptedInWalletSyncTransport(pref_service(), account1, true);
+  EXPECT_FALSE(pref_service()
+                   ->GetDictionary(prefs::kAutofillSyncTransportOptIn)
+                   ->DictEmpty());
+
+  // Make sure that the dictionary keys don't contain the account id.
+  auto* dictionary =
       pref_service()->GetDictionary(prefs::kAutofillSyncTransportOptIn);
-  EXPECT_EQ(nullptr,
-            upload_events->FindKeyOfType(account1, base::Value::Type::INTEGER));
-  EXPECT_NE(nullptr,
-            upload_events->FindKeyOfType(account2, base::Value::Type::INTEGER));
+  EXPECT_EQ(NULL,
+            dictionary->FindKeyOfType(account1, base::Value::Type::INTEGER));
 }
 
 // Tests that clearing the AutofillSyncTransportOptIn works as expected.