Integrate registry_hash_store_contents with the rest of tracked prefs.

This change adds Windows-only logic to the PrefHashFilter such that it
verifies preferences against MACs stored in the registry. Unlike the
current tracked preference logic, this extra check does NOT reset
settings.

To avoid inconsistent state with the MACs in secure_preferences, we
clear the registry MACs before writing secure_preferences, and write
the registry MACs after the file is successfully written.

BUG=624858

Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c
Review-Url: https://codereview.chromium.org/2204943002
Cr-Original-Commit-Position: refs/heads/master@{#422240}
Cr-Commit-Position: refs/heads/master@{#422957}

chrome/browser/prefs/profile_pref_store_manager.cc

@@ -11,6 +11,7 @@
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "build/build_config.h"
@@ -23,6 +24,11 @@
 #include "components/user_prefs/tracked/segregated_pref_store.h"
 #include "components/user_prefs/tracked/tracked_preferences_migration.h"
 
+#if defined(OS_WIN)
+#include "chrome/installer/util/browser_distribution.h"
+#include "components/user_prefs/tracked/registry_hash_store_contents_win.h"
+#endif
+
 namespace {
 
 void RemoveValueSilently(const base::WeakPtr<JsonPrefStore> pref_store,
@@ -33,6 +39,13 @@ void RemoveValueSilently(const base::WeakPtr<JsonPrefStore> pref_store,
   }
 }
 
+#if defined(OS_WIN)
+// Forces a different registry key to be used for storing preference validation
+// MACs. See |SetPreferenceValidationRegistryPathForTesting|.
+const base::string16* g_preference_validation_registry_path_for_testing =
+    nullptr;
+#endif  // OS_WIN
+
 }  // namespace
 
 // Preference tracking and protection is not required on platforms where other
@@ -77,6 +90,15 @@ void ProfilePrefStoreManager::ClearResetTime(PrefService* pref_service) {
   PrefHashFilter::ClearResetTime(pref_service);
 }
 
+#if defined(OS_WIN)
+// static
+void ProfilePrefStoreManager::SetPreferenceValidationRegistryPathForTesting(
+    const base::string16* path) {
+  DCHECK(!path->empty());
+  g_preference_validation_registry_path_for_testing = path;
+}
+#endif  // OS_WIN
+
 PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
     const base::Closure& on_reset_on_load,
@@ -108,12 +130,14 @@ PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
   }
 
   std::unique_ptr<PrefHashFilter> unprotected_pref_hash_filter(
-      new PrefHashFilter(GetPrefHashStore(false), unprotected_configuration,
-                         base::Closure(), validation_delegate,
-                         reporting_ids_count_, false));
+      new PrefHashFilter(GetPrefHashStore(false),
+                         GetExternalVerificationPrefHashStorePair(),
+                         unprotected_configuration, base::Closure(),
+                         validation_delegate, reporting_ids_count_, false));
   std::unique_ptr<PrefHashFilter> protected_pref_hash_filter(new PrefHashFilter(
-      GetPrefHashStore(true), protected_configuration, on_reset_on_load,
-      validation_delegate, reporting_ids_count_, true));
+      GetPrefHashStore(true), GetExternalVerificationPrefHashStorePair(),
+      protected_configuration, on_reset_on_load, validation_delegate,
+      reporting_ids_count_, true));
 
   PrefHashFilter* raw_unprotected_pref_hash_filter =
       unprotected_pref_hash_filter.get();
@@ -159,11 +183,10 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
     copy.reset(master_prefs.DeepCopy());
     to_serialize = copy.get();
     PrefHashFilter(GetPrefHashStore(false),
-                   tracking_configuration_,
-                   base::Closure(),
-                   NULL,
-                   reporting_ids_count_,
-                   false).Initialize(copy.get());
+                   GetExternalVerificationPrefHashStorePair(),
+                   tracking_configuration_, base::Closure(), NULL,
+                   reporting_ids_count_, false)
+        .Initialize(copy.get());
   }
 
   // This will write out to a single combined file which will be immediately
@@ -189,3 +212,23 @@ std::unique_ptr<PrefHashStore> ProfilePrefStoreManager::GetPrefHashStore(
   return std::unique_ptr<PrefHashStore>(
       new PrefHashStoreImpl(seed_, device_id_, use_super_mac));
 }
+
+std::pair<std::unique_ptr<PrefHashStore>, std::unique_ptr<HashStoreContents>>
+ProfilePrefStoreManager::GetExternalVerificationPrefHashStorePair() {
+  DCHECK(kPlatformSupportsPreferenceTracking);
+#if defined(OS_WIN)
+  return std::make_pair(
+      base::MakeUnique<PrefHashStoreImpl>(
+          "ChromeRegistryHashStoreValidationSeed", device_id_,
+          false /* use_super_mac */),
+      g_preference_validation_registry_path_for_testing
+          ? base::MakeUnique<RegistryHashStoreContentsWin>(
+                *g_preference_validation_registry_path_for_testing,
+                profile_path_.BaseName().LossyDisplayName())
+          : base::MakeUnique<RegistryHashStoreContentsWin>(
+                BrowserDistribution::GetDistribution()->GetRegistryPath(),
+                profile_path_.BaseName().LossyDisplayName()));
+#else
+  return std::make_pair(nullptr, nullptr);
+#endif
+}

chrome/browser/prefs/profile_pref_store_manager.h

@@ -16,6 +16,7 @@
 #include "base/memory/ref_counted.h"
 #include "components/user_prefs/tracked/pref_hash_filter.h"
 
+class HashStoreContents;
 class PersistentPrefStore;
 class PrefHashStore;
 class PrefService;
@@ -70,6 +71,14 @@ class ProfilePrefStoreManager {
   // was built by ProfilePrefStoreManager.
   static void ClearResetTime(PrefService* pref_service);
 
+#if defined(OS_WIN)
+  // Call before startup tasks kick in to use a different registry path for
+  // storing and validating tracked preference MACs. Callers are responsible
+  // for ensuring that the key is deleted on shutdown. For testing only.
+  static void SetPreferenceValidationRegistryPathForTesting(
+      const base::string16* path);
+#endif
+
   // Creates a PersistentPrefStore providing access to the user preferences of
   // the managed profile. If |on_reset| is provided, it will be invoked if a
   // reset occurs as a result of loading the profile's prefs.
@@ -99,6 +108,12 @@ class ProfilePrefStoreManager {
   // TrustedInitialized).
   std::unique_ptr<PrefHashStore> GetPrefHashStore(bool use_super_mac);
 
+  // Returns a PrefHashStore and HashStoreContents which can be be used for
+  // extra out-of-band verifications, or nullptrs if not available on this
+  // platform.
+  std::pair<std::unique_ptr<PrefHashStore>, std::unique_ptr<HashStoreContents>>
+  GetExternalVerificationPrefHashStorePair();
+
   const base::FilePath profile_path_;
   const std::vector<PrefHashFilter::TrackedPreferenceMetadata>
       tracking_configuration_;

components/prefs/json_pref_store.cc

@@ -172,8 +172,7 @@ JsonPrefStore::JsonPrefStore(
       filtering_in_progress_(false),
       pending_lossy_write_(false),
       read_error_(PREF_READ_ERROR_NONE),
-      has_pending_successful_write_reply_(false),
-      has_pending_write_callbacks_(false),
+      has_pending_write_reply_(false),
       write_count_histogram_(writer_.commit_interval(), path_) {
   DCHECK(!path_.empty());
 }
@@ -330,21 +329,22 @@ void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback(
     bool write_success) {
   DCHECK(CalledOnValidThread());
 
-  has_pending_write_callbacks_ = false;
-  if (has_pending_successful_write_reply_) {
-    has_pending_successful_write_reply_ = false;
+  has_pending_write_reply_ = false;
+  if (!on_next_successful_write_reply_.is_null()) {
+    base::Closure on_successful_write =
+        std::move(on_next_successful_write_reply_);
     if (write_success) {
-      on_next_successful_write_reply_.Run();
+      on_successful_write.Run();
     } else {
-      RegisterOnNextSuccessfulWriteReply(on_next_successful_write_reply_);
+      RegisterOnNextSuccessfulWriteReply(on_successful_write);
     }
   }
 }
 
 // static
 void JsonPrefStore::PostWriteCallback(
-    const base::Callback<void(bool success)>& on_next_write_reply,
     const base::Callback<void(bool success)>& on_next_write_callback,
+    const base::Callback<void(bool success)>& on_next_write_reply,
     scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
     bool write_success) {
   if (!on_next_write_callback.is_null())
@@ -359,22 +359,21 @@ void JsonPrefStore::PostWriteCallback(
 void JsonPrefStore::RegisterOnNextSuccessfulWriteReply(
     const base::Closure& on_next_successful_write_reply) {
   DCHECK(CalledOnValidThread());
-  DCHECK(!has_pending_successful_write_reply_);
+  DCHECK(on_next_successful_write_reply_.is_null());
 
-  has_pending_successful_write_reply_ = true;
   on_next_successful_write_reply_ = on_next_successful_write_reply;
 
   // If there are pending callbacks, avoid erasing them; the reply will be used
   // as we set |on_next_successful_write_reply_|. Otherwise, setup a reply with
   // an empty callback.
-  if (!has_pending_write_callbacks_) {
+  if (!has_pending_write_reply_) {
+    has_pending_write_reply_ = true;
     writer_.RegisterOnNextWriteCallbacks(
         base::Closure(),
         base::Bind(
-            &PostWriteCallback,
+            &PostWriteCallback, base::Callback<void(bool success)>(),
             base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback,
                        AsWeakPtr()),
-            base::Callback<void(bool success)>(),
             base::SequencedTaskRunnerHandle::Get()));
   }
 }
@@ -382,17 +381,16 @@ void JsonPrefStore::RegisterOnNextSuccessfulWriteReply(
 void JsonPrefStore::RegisterOnNextWriteSynchronousCallbacks(
     OnWriteCallbackPair callbacks) {
   DCHECK(CalledOnValidThread());
-  DCHECK(!has_pending_write_callbacks_);
 
-  has_pending_write_callbacks_ = true;
+  has_pending_write_reply_ = true;
 
   writer_.RegisterOnNextWriteCallbacks(
       callbacks.first,
       base::Bind(
-          &PostWriteCallback,
+          &PostWriteCallback, callbacks.second,
           base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback,
                      AsWeakPtr()),
-          callbacks.second, base::SequencedTaskRunnerHandle::Get()));
+          base::SequencedTaskRunnerHandle::Get()));
 }
 
 void JsonPrefStore::ClearMutableValues() {

components/prefs/json_pref_store.h

@@ -190,11 +190,11 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
   void RunOrScheduleNextSuccessfulWriteCallback(bool write_success);
 
   // Handles the result of a write with result |write_success|. Runs
-  // |on_next_write| callback on the current thread and posts
-  // |RunOrScheduleNextSuccessfulWriteCallback| on |reply_task_runner|.
+  // |on_next_write_callback| on the current thread and posts
+  // |on_next_write_reply| on |reply_task_runner|.
   static void PostWriteCallback(
-      const base::Callback<void(bool success)>& on_next_write_reply,
       const base::Callback<void(bool success)>& on_next_write_callback,
+      const base::Callback<void(bool success)>& on_next_write_reply,
       scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
       bool write_success);
 
@@ -252,8 +252,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
 
   std::set<std::string> keys_need_empty_value_;
 
-  bool has_pending_successful_write_reply_;
-  bool has_pending_write_callbacks_;
+  bool has_pending_write_reply_ = true;
   base::Closure on_next_successful_write_reply_;
 
   WriteCountHistogram write_count_histogram_;

components/user_prefs/tracked/dictionary_hash_store_contents.cc

@@ -28,6 +28,16 @@ void DictionaryHashStoreContents::RegisterProfilePrefs(
   registry->RegisterStringPref(kSuperMACPref, std::string());
 }
 
+bool DictionaryHashStoreContents::IsCopyable() const {
+  return false;
+}
+
+std::unique_ptr<HashStoreContents> DictionaryHashStoreContents::MakeCopy()
+    const {
+  NOTREACHED() << "DictionaryHashStoreContents does not support MakeCopy";
+  return nullptr;
+}
+
 base::StringPiece DictionaryHashStoreContents::GetUMASuffix() const {
   // To stay consistent with existing reported data, do not append a suffix
   // when reporting UMA stats for this content.
@@ -121,4 +131,4 @@ base::DictionaryValue* DictionaryHashStoreContents::GetMutableContents(
     storage_->Set(kPreferenceMACs, macs_dict);
   }
   return macs_dict;
-}
\ No newline at end of file
+}

components/user_prefs/tracked/dictionary_hash_store_contents.h

@@ -31,6 +31,8 @@ class DictionaryHashStoreContents : public HashStoreContents {
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
   // HashStoreContents implementation
+  bool IsCopyable() const override;
+  std::unique_ptr<HashStoreContents> MakeCopy() const override;
   base::StringPiece GetUMASuffix() const override;
   void Reset() override;
   bool GetMac(const std::string& path, std::string* out_value) override;

components/user_prefs/tracked/hash_store_contents.h

@@ -26,6 +26,15 @@ class HashStoreContents {
  public:
   virtual ~HashStoreContents() {}
 
+  // Returns true if this implementation of HashStoreContents can be copied via
+  // MakeCopy().
+  virtual bool IsCopyable() const = 0;
+
+  // Returns a copy of this HashStoreContents. Must only be called on
+  // lightweight implementations (which return true from IsCopyable()) and only
+  // in scenarios where a copy cannot be avoided.
+  virtual std::unique_ptr<HashStoreContents> MakeCopy() const = 0;
+
   // Returns the suffix to be appended to UMA histograms for this store type.
   // The returned value must either be an empty string or one of the values in
   // histograms.xml's TrackedPreferencesExternalValidators.

components/user_prefs/tracked/pref_hash_filter.h

@@ -15,7 +15,10 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/containers/scoped_ptr_hash_map.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/optional.h"
+#include "components/user_prefs/tracked/hash_store_contents.h"
 #include "components/user_prefs/tracked/interceptable_pref_filter.h"
 #include "components/user_prefs/tracked/tracked_preference.h"
 
@@ -64,6 +67,9 @@ class PrefHashFilter : public InterceptablePrefFilter {
     ValueType value_type;
   };
 
+  using StoreContentsPair = std::pair<std::unique_ptr<PrefHashStore>,
+                                      std::unique_ptr<HashStoreContents>>;
+
   // Constructs a PrefHashFilter tracking the specified |tracked_preferences|
   // using |pref_hash_store| to check/store hashes. An optional |delegate| is
   // notified of the status of each preference as it is checked.
@@ -73,8 +79,11 @@ class PrefHashFilter : public InterceptablePrefFilter {
   // than |tracked_preferences.size()|). If |report_super_mac_validity| is true,
   // the state of the super MAC will be reported via UMA during
   // FinalizeFilterOnLoad.
+  // |external_validation_hash_store_pair_| will be used (if non-null) to
+  // perform extra validations without triggering resets.
   PrefHashFilter(
       std::unique_ptr<PrefHashStore> pref_hash_store,
+      StoreContentsPair external_validation_hash_store_pair_,
       const std::vector<TrackedPreferenceMetadata>& tracked_preferences,
       const base::Closure& on_reset_on_load,
       TrackedPreferenceValidationDelegate* delegate,
@@ -111,6 +120,25 @@ class PrefHashFilter : public InterceptablePrefFilter {
       std::unique_ptr<base::DictionaryValue> pref_store_contents,
       bool prefs_altered) override;
 
+  // Helper function to generate FilterSerializeData()'s pre-write and
+  // post-write callbacks. The returned callbacks are thread-safe.
+  OnWriteCallbackPair GetOnWriteSynchronousCallbacks(
+      base::DictionaryValue* pref_store_contents);
+
+  // Clears the MACs contained in |external_validation_hash_store_contents|
+  // which are present in |paths_to_clear|.
+  static void ClearFromExternalStore(
+      HashStoreContents* external_validation_hash_store_contents,
+      const base::DictionaryValue* changed_paths_and_macs);
+
+  // Flushes the MACs contained in |changed_paths_and_mac| to
+  // external_hash_store_contents if |write_success|, otherwise discards the
+  // changes.
+  static void FlushToExternalStore(
+      std::unique_ptr<HashStoreContents> external_hash_store_contents,
+      std::unique_ptr<base::DictionaryValue> changed_paths_and_macs,
+      bool write_success);
+
   // Callback to be invoked only once (and subsequently reset) on the next
   // FilterOnLoad event. It will be allowed to modify the |prefs| handed to
   // FilterOnLoad before handing them back to this PrefHashFilter.
@@ -121,12 +149,18 @@ class PrefHashFilter : public InterceptablePrefFilter {
   typedef base::ScopedPtrHashMap<std::string,
                                  std::unique_ptr<TrackedPreference>>
       TrackedPreferencesMap;
+
   // A map from changed paths to their corresponding TrackedPreferences (which
   // aren't owned by this map).
   typedef std::map<std::string, const TrackedPreference*> ChangedPathsMap;
 
   std::unique_ptr<PrefHashStore> pref_hash_store_;
 
+  // A store and contents on which to perform extra validations without
+  // triggering resets.
+  // Will be null if the platform does not support external validation.
+  const base::Optional<StoreContentsPair> external_validation_hash_store_pair_;
+
   // Invoked if a reset occurs in a call to FilterOnLoad.
   const base::Closure on_reset_on_load_;
 

components/user_prefs/tracked/pref_hash_store.h

@@ -12,6 +12,7 @@
 #include "components/user_prefs/tracked/pref_hash_store_transaction.h"
 
 class HashStoreContents;
+class PrefHashStoreTransaction;
 
 // Holds the configuration and implementation used to calculate and verify
 // preference MACs.

components/user_prefs/tracked/registry_hash_store_contents_win.cc

@@ -7,6 +7,7 @@
 #include <windows.h>
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
@@ -19,7 +20,7 @@ using base::win::RegistryValueIterator;
 
 namespace {
 
-constexpr size_t kMacSize = 32;
+constexpr size_t kMacSize = 64;
 
 base::string16 GetSplitPrefKeyName(const base::string16& reg_key_name,
                                    const std::string& split_key_name) {
@@ -72,6 +73,18 @@ RegistryHashStoreContentsWin::RegistryHashStoreContentsWin(
     const base::string16& store_key)
     : preference_key_name_(registry_path + L"\\PreferenceMACs\\" + store_key) {}
 
+RegistryHashStoreContentsWin::RegistryHashStoreContentsWin(
+    const RegistryHashStoreContentsWin& other) = default;
+
+bool RegistryHashStoreContentsWin::IsCopyable() const {
+  return true;
+}
+
+std::unique_ptr<HashStoreContents> RegistryHashStoreContentsWin::MakeCopy()
+    const {
+  return base::WrapUnique(new RegistryHashStoreContentsWin(*this));
+}
+
 base::StringPiece RegistryHashStoreContentsWin::GetUMASuffix() const {
   return user_prefs::tracked::kTrackedPrefRegistryValidationSuffix;
 }
@@ -141,7 +154,6 @@ void RegistryHashStoreContentsWin::SetSplitMac(const std::string& path,
 }
 
 bool RegistryHashStoreContentsWin::RemoveEntry(const std::string& path) {
-  // ClearSplitMac is first to avoid short-circuit issues.
   return ClearAtomicMac(preference_key_name_, path) ||
          ClearSplitMac(preference_key_name_, path);
 }
@@ -149,22 +161,22 @@ bool RegistryHashStoreContentsWin::RemoveEntry(const std::string& path) {
 void RegistryHashStoreContentsWin::ImportEntry(const std::string& path,
                                                const base::Value* in_value) {
   NOTREACHED()
-      << "RegistryHashStore does not support the ImportEntry operation";
+      << "RegistryHashStoreContents does not support the ImportEntry operation";
 }
 
 const base::DictionaryValue* RegistryHashStoreContentsWin::GetContents() const {
   NOTREACHED()
-      << "RegistryHashStore does not support the GetContents operation";
+      << "RegistryHashStoreContents does not support the GetContents operation";
   return NULL;
 }
 
 std::string RegistryHashStoreContentsWin::GetSuperMac() const {
   NOTREACHED()
-      << "RegistryHashStore does not support the GetSuperMac operation";
+      << "RegistryHashStoreContents does not support the GetSuperMac operation";
   return NULL;
 }
 
 void RegistryHashStoreContentsWin::SetSuperMac(const std::string& super_mac) {
   NOTREACHED()
-      << "RegistryHashStore does not support the SetSuperMac operation";
+      << "RegistryHashStoreContents does not support the SetSuperMac operation";
 }

components/user_prefs/tracked/registry_hash_store_contents_win.h

@@ -18,6 +18,8 @@ class RegistryHashStoreContentsWin : public HashStoreContents {
                                         const base::string16& store_key);
 
   // HashStoreContents overrides:
+  bool IsCopyable() const override;
+  std::unique_ptr<HashStoreContents> MakeCopy() const override;
   base::StringPiece GetUMASuffix() const override;
   void Reset() override;
   bool GetMac(const std::string& path, std::string* out_value) override;
@@ -37,9 +39,11 @@ class RegistryHashStoreContentsWin : public HashStoreContents {
   void SetSuperMac(const std::string& super_mac) override;
 
  private:
-  const base::string16 preference_key_name_;
+  // Helper constructor for |MakeCopy|.
+  explicit RegistryHashStoreContentsWin(
+      const RegistryHashStoreContentsWin& other);
 
-  DISALLOW_COPY_AND_ASSIGN(RegistryHashStoreContentsWin);
+  const base::string16 preference_key_name_;
 };
 
 #endif  // COMPONENTS_USER_PREFS_TRACKED_PREF_REGISTRY_HASH_STORE_CONTENTS_H_

components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc

@@ -16,9 +16,11 @@ namespace {
 constexpr base::char16 kRegistryPath[] = L"Foo\\TestStore";
 constexpr base::char16 kStoreKey[] = L"test_store_key";
 
-// MACs are 32 characters long.
-constexpr char kTestStringA[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
-constexpr char kTestStringB[] = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb";
+// Hex-encoded MACs are 64 characters long.
+constexpr char kTestStringA[] =
+    "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+constexpr char kTestStringB[] =
+    "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb";
 
 constexpr char kAtomicPrefPath[] = "path1";
 constexpr char kSplitPrefPath[] = "extension";
@@ -114,4 +116,4 @@ TEST_F(RegistryHashStoreContentsWinTest, TestReset) {
   split_macs.clear();
   EXPECT_FALSE(contents->GetSplitMacs(kSplitPrefPath, &split_macs));
   EXPECT_EQ(0U, split_macs.size());
-}
\ No newline at end of file
+}

components/user_prefs/tracked/tracked_atomic_preference.cc

@@ -24,6 +24,10 @@ TrackedAtomicPreference::TrackedAtomicPreference(
       delegate_(delegate) {
 }
 
+TrackedPreferenceType TrackedAtomicPreference::GetType() const {
+  return TrackedPreferenceType::ATOMIC;
+}
+
 void TrackedAtomicPreference::OnNewValue(
     const base::Value* value,
     PrefHashStoreTransaction* transaction) const {
@@ -32,20 +36,32 @@ void TrackedAtomicPreference::OnNewValue(
 
 bool TrackedAtomicPreference::EnforceAndReport(
     base::DictionaryValue* pref_store_contents,
-    PrefHashStoreTransaction* transaction) const {
+    PrefHashStoreTransaction* transaction,
+    PrefHashStoreTransaction* external_validation_transaction) const {
   const base::Value* value = NULL;
   pref_store_contents->Get(pref_path_, &value);
   PrefHashStoreTransaction::ValueState value_state =
       transaction->CheckValue(pref_path_, value);
-
   helper_.ReportValidationResult(value_state, transaction->GetStoreUMASuffix());
 
-  TrackedPreferenceHelper::ResetAction reset_action =
-      helper_.GetAction(value_state);
+  PrefHashStoreTransaction::ValueState external_validation_value_state =
+      PrefHashStoreTransaction::UNCHANGED;
+  if (external_validation_transaction) {
+    external_validation_value_state =
+        external_validation_transaction->CheckValue(pref_path_, value);
+    helper_.ReportValidationResult(
+        external_validation_value_state,
+        external_validation_transaction->GetStoreUMASuffix());
+
+    // TODO(proberge): Call delegate_->OnAtomicPreferenceValidation.
+  }
+
   if (delegate_) {
     delegate_->OnAtomicPreferenceValidation(pref_path_, value, value_state,
                                             helper_.IsPersonal());
   }
+  TrackedPreferenceHelper::ResetAction reset_action =
+      helper_.GetAction(value_state);
   helper_.ReportAction(reset_action);
 
   bool was_reset = false;
@@ -61,5 +77,16 @@ bool TrackedAtomicPreference::EnforceAndReport(
     transaction->StoreHash(pref_path_, new_value);
   }
 
+  // Update MACs in the external store if there is one and there either was a
+  // reset or external validation failed.
+  if (external_validation_transaction &&
+      (was_reset ||
+       external_validation_value_state !=
+           PrefHashStoreTransaction::UNCHANGED)) {
+    const base::Value* new_value = nullptr;
+    pref_store_contents->Get(pref_path_, &new_value);
+    external_validation_transaction->StoreHash(pref_path_, new_value);
+  }
+
   return was_reset;
 }

components/user_prefs/tracked/tracked_atomic_preference.h

@@ -30,10 +30,13 @@ class TrackedAtomicPreference : public TrackedPreference {
                           TrackedPreferenceValidationDelegate* delegate);
 
   // TrackedPreference implementation.
+  TrackedPreferenceType GetType() const override;
   void OnNewValue(const base::Value* value,
                   PrefHashStoreTransaction* transaction) const override;
-  bool EnforceAndReport(base::DictionaryValue* pref_store_contents,
-                        PrefHashStoreTransaction* transaction) const override;
+  bool EnforceAndReport(
+      base::DictionaryValue* pref_store_contents,
+      PrefHashStoreTransaction* transaction,
+      PrefHashStoreTransaction* external_validation_transaction) const override;
 
  private:
   const std::string pref_path_;

components/user_prefs/tracked/tracked_preference.h

@@ -12,12 +12,16 @@ class DictionaryValue;
 class Value;
 }
 
+enum class TrackedPreferenceType { ATOMIC, SPLIT };
+
 // A TrackedPreference tracks changes to an individual preference, reporting and
 // reacting to them according to preference-specific and browser-wide policies.
 class TrackedPreference {
  public:
   virtual ~TrackedPreference() {}
 
+  virtual TrackedPreferenceType GetType() const = 0;
+
   // Notifies the underlying TrackedPreference about its new |value| which
   // can update hashes in the corresponding hash store via |transaction|.
   virtual void OnNewValue(const base::Value* value,
@@ -27,10 +31,14 @@ class TrackedPreference {
   // is valid. Responds to verification failures according to
   // preference-specific and browser-wide policy and reports results to via UMA.
   // May use |transaction| to check/modify hashes in the corresponding hash
-  // store.
+  // store. Performs validation and reports results without enforcing for
+  // |external_validation_transaction|. This call assumes exclusive access to
+  // |external_validation_transaction| and its associated state and as such
+  // should only be called before any other subsystem is made aware of it.
   virtual bool EnforceAndReport(
       base::DictionaryValue* pref_store_contents,
-      PrefHashStoreTransaction* transaction) const = 0;
+      PrefHashStoreTransaction* transaction,
+      PrefHashStoreTransaction* external_validation_transaction) const = 0;
 };
 
 #endif  // COMPONENTS_USER_PREFS_TRACKED_TRACKED_PREFERENCE_H_

components/user_prefs/tracked/tracked_split_preference.cc

@@ -27,6 +27,10 @@ TrackedSplitPreference::TrackedSplitPreference(
       delegate_(delegate) {
 }
 
+TrackedPreferenceType TrackedSplitPreference::GetType() const {
+  return TrackedPreferenceType::SPLIT;
+}
+
 void TrackedSplitPreference::OnNewValue(
     const base::Value* value,
     PrefHashStoreTransaction* transaction) const {
@@ -40,7 +44,8 @@ void TrackedSplitPreference::OnNewValue(
 
 bool TrackedSplitPreference::EnforceAndReport(
     base::DictionaryValue* pref_store_contents,
-    PrefHashStoreTransaction* transaction) const {
+    PrefHashStoreTransaction* transaction,
+    PrefHashStoreTransaction* external_validation_transaction) const {
   base::DictionaryValue* dict_value = NULL;
   if (!pref_store_contents->GetDictionary(pref_path_, &dict_value) &&
       pref_store_contents->Get(pref_path_, NULL)) {
@@ -58,12 +63,26 @@ bool TrackedSplitPreference::EnforceAndReport(
 
   helper_.ReportValidationResult(value_state, transaction->GetStoreUMASuffix());
 
-  TrackedPreferenceHelper::ResetAction reset_action =
-      helper_.GetAction(value_state);
+  PrefHashStoreTransaction::ValueState external_validation_value_state =
+      PrefHashStoreTransaction::UNCHANGED;
+  if (external_validation_transaction) {
+    std::vector<std::string> invalid_external_validation_keys;
+    external_validation_value_state =
+        external_validation_transaction->CheckSplitValue(
+            pref_path_, dict_value, &invalid_external_validation_keys);
+    helper_.ReportValidationResult(
+        external_validation_value_state,
+        external_validation_transaction->GetStoreUMASuffix());
+
+    // TODO(proberge): Call delegate_->OnSplitPreferenceValidation.
+  }
+
   if (delegate_) {
     delegate_->OnSplitPreferenceValidation(pref_path_, dict_value, invalid_keys,
                                            value_state, helper_.IsPersonal());
   }
+  TrackedPreferenceHelper::ResetAction reset_action =
+      helper_.GetAction(value_state);
   helper_.ReportAction(reset_action);
 
   bool was_reset = false;
@@ -88,5 +107,16 @@ bool TrackedSplitPreference::EnforceAndReport(
     transaction->StoreSplitHash(pref_path_, new_dict_value);
   }
 
+  // Update MACs in the external store if there is one and there either was a
+  // reset or external validation failed.
+  if (external_validation_transaction &&
+      (was_reset ||
+       external_validation_value_state !=
+           PrefHashStoreTransaction::UNCHANGED)) {
+    const base::DictionaryValue* new_dict_value = nullptr;
+    pref_store_contents->GetDictionary(pref_path_, &new_dict_value);
+    external_validation_transaction->StoreSplitHash(pref_path_, new_dict_value);
+  }
+
   return was_reset;
 }

components/user_prefs/tracked/tracked_split_preference.h

@@ -33,10 +33,13 @@ class TrackedSplitPreference : public TrackedPreference {
                          TrackedPreferenceValidationDelegate* delegate);
 
   // TrackedPreference implementation.
+  TrackedPreferenceType GetType() const override;
   void OnNewValue(const base::Value* value,
                   PrefHashStoreTransaction* transaction) const override;
-  bool EnforceAndReport(base::DictionaryValue* pref_store_contents,
-                        PrefHashStoreTransaction* transaction) const override;
+  bool EnforceAndReport(
+      base::DictionaryValue* pref_store_contents,
+      PrefHashStoreTransaction* transaction,
+      PrefHashStoreTransaction* external_validation_transaction) const override;
 
  private:
   const std::string pref_path_;