Move preference MACs to the protected preference stores.

chrome/browser/prefs/pref_hash_store_impl.cc

Index: chrome/browser/prefs/pref_hash_store_impl.cc
diff --git a/chrome/browser/prefs/pref_hash_store_impl.cc b/chrome/browser/prefs/pref_hash_store_impl.cc
index 4f6f6d6754beddc52616934a6156f4cf9dd425fb..b0164521513d12803362dabf27eba812f4256c94 100644
--- a/chrome/browser/prefs/pref_hash_store_impl.cc
+++ b/chrome/browser/prefs/pref_hash_store_impl.cc
@@ -8,31 +8,15 @@
 #include "base/metrics/histogram.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/pref_hash_store_transaction.h"
-#include "chrome/browser/prefs/tracked/hash_store_contents.h"
-
-namespace {
-
-// Returns true if the dictionary of hashes stored in |contents| is trusted
-// (which implies unknown values can be trusted as newly tracked values).
-bool IsHashDictionaryTrusted(const PrefHashCalculator& calculator,
-                             const HashStoreContents& contents) {
-  const base::DictionaryValue* store_contents = contents.GetContents();
-  std::string super_mac = contents.GetSuperMac();
-  // The store must be initialized and have a valid super MAC to be trusted.
-  return store_contents && !super_mac.empty() &&
-         calculator.Validate(contents.hash_store_id(),
-                             store_contents,
-                             super_mac) == PrefHashCalculator::VALID;
-}
-
-}  // namespace
+#include "chrome/browser/prefs/tracked/dictionary_hash_store_contents.h"
 
 class PrefHashStoreImpl::PrefHashStoreTransactionImpl
     : public PrefHashStoreTransaction {
  public:
   // Constructs a PrefHashStoreTransactionImpl which can use the private
   // members of its |outer| PrefHashStoreImpl.
-  explicit PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer);
+  PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer,
+                               base::DictionaryValue* storage);
   virtual ~PrefHashStoreTransactionImpl();
 
   // PrefHashStoreTransaction implementation.
@@ -40,6 +24,7 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
                                 const base::Value* value) const OVERRIDE;
   virtual void StoreHash(const std::string& path,
                          const base::Value* value) OVERRIDE;
+  virtual bool StampSuperMac() OVERRIDE;
   virtual ValueState CheckSplitValue(
       const std::string& path,
       const base::DictionaryValue* initial_split_value,
@@ -47,76 +32,76 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
   virtual void StoreSplitHash(
       const std::string& path,
       const base::DictionaryValue* split_value) OVERRIDE;
+  virtual bool HasHash(const std::string& path) const OVERRIDE;
+  virtual void ImportHash(const std::string& path,
+                          const base::Value* hash) OVERRIDE;
+  virtual void ClearHash(const std::string& path) OVERRIDE;
 
  private:
   bool GetSplitMacs(const std::string& path,
                     std::map<std::string, std::string>* split_macs) const;
+
   PrefHashStoreImpl* outer_;
-  bool has_changed_;
+  DictionaryHashStoreContents contents_;
+
+  bool super_mac_valid_;
+  bool super_mac_dirty_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefHashStoreTransactionImpl);
 };
 
 PrefHashStoreImpl::PrefHashStoreImpl(const std::string& seed,
                                      const std::string& device_id,
-                                     scoped_ptr<HashStoreContents> contents,
                                      bool use_super_mac)
     : pref_hash_calculator_(seed, device_id),
-      contents_(contents.Pass()),
-      initial_hashes_dictionary_trusted_(
-          use_super_mac
-              ? IsHashDictionaryTrusted(pref_hash_calculator_, *contents_)
-              : false),
-      use_super_mac_(use_super_mac),
-      has_pending_write_(false) {
-  DCHECK(contents_);
-  UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted",

[gab, 2014/06/13 01:57:43]

Regarding what to do with this histogram: I think we should expose an
IsSuperMacTrusted() method on the PrefHashStoreTransaction and let
FilterOnLoad() report it.

-                        initial_hashes_dictionary_trusted_);
+      use_super_mac_(use_super_mac) {
 }
 
-PrefHashStoreImpl::~PrefHashStoreImpl() {}
-
-void PrefHashStoreImpl::Reset() {
-  contents_->Reset();
+PrefHashStoreImpl::~PrefHashStoreImpl() {
 }
 
-scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction() {
+scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction(
+    base::DictionaryValue* storage) {
   return scoped_ptr<PrefHashStoreTransaction>(
-      new PrefHashStoreTransactionImpl(this));
-}
-
-void PrefHashStoreImpl::CommitPendingWrite() {
-  if (has_pending_write_) {
-    contents_->CommitPendingWrite();
-    has_pending_write_ = false;
-  }
+      new PrefHashStoreTransactionImpl(this, storage));
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::PrefHashStoreTransactionImpl(
-    PrefHashStoreImpl* outer) : outer_(outer), has_changed_(false) {
+    PrefHashStoreImpl* outer,
+    base::DictionaryValue* storage)
+    : outer_(outer),
+      contents_(storage),
+      super_mac_valid_(false),
+      super_mac_dirty_(false) {
+  if (outer_->use_super_mac_) {
+    const base::DictionaryValue* store_contents = contents_.GetContents();
+    std::string super_mac = contents_.GetSuperMac();
+    // The store must be initialized and have a valid super MAC to be trusted.
+    super_mac_valid_ =

[gab, 2014/06/13 01:57:43]

So if a previous transaction stamped a super MAC, all follow-up transactions
will trust the store, this *may* be okay since this state only matters in the
initial FilterOnLoad(), but this is not the way this used to work (where the
initial trust was the only thing that mattered throughout the lifetime of the
PrefHashStore).

Something to keep in mind as we refine this CL.

[erikwright (departed), 2014/06/16 20:51:26]

Yeah, there is kind of limited choice. I don't think it's appropriate to leave
state in the PrefHashStore instance since it is no longer bound to a single
underlying storage (even if, in principle, it will only be used with one
storage).

I had considered introducing PrefHashStore::BeginFiltering which would
essentially be BeginTransaction but with the side-effect of updating the super
MAC. But I don't think it's that useful unless it provides the _only_ way to
perform the operations associated with filtering
(Check(Split)Value/Store(Split)Hash).

[gab, 2014/06/17 02:00:05]

How about adding a PrefHashFilter::filter_on_load_performed_ member to
PrefHashFilter and DCHECK'ing that FilterOnLoad() only happens once?

This depends on that fact (and much of the framework kind of assumes this as
well).

[erikwright (departed), 2014/06/17 17:54:05]

Personally, I'm not worried about a single instance of PrefHashFilter calling
filter_on_load twice. The real risk is that something in the migration code ends
up triggering an unintended super MAC update, given that that component has a
completely distinct instance of PrefHashStore.

To protect against this, I would change the object model for a PrefHashStore to
guarantee that it can only be used once for load-time enforcement. For example,
forcing you to give up your only reference to the object at the end of
enforcement.

I don't think it's critical at the moment. Probably gold-plating, especially if
there are regression tests to catch it.

But I definitely think the boolean "did this once" flag is at best superficial
and a false sense of security.

+        store_contents && !super_mac.empty() &&
+        outer_->pref_hash_calculator_.Validate(
+            contents_.hash_store_id(), store_contents, super_mac) ==
+            PrefHashCalculator::VALID;
+  }
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::
     ~PrefHashStoreTransactionImpl() {
   // Update the super MAC if and only if the hashes dictionary has been
   // modified in this transaction.

[gab, 2014/06/13 01:57:43]

Remove the comment above, the new variable name makes this explicit :-)!

[erikwright (departed), 2014/06/16 20:51:26]

Done.

-  if (has_changed_) {
-    if (outer_->use_super_mac_) {
-      // Get the dictionary of hashes (or NULL if it doesn't exist).
-      const base::DictionaryValue* hashes_dict =
-          outer_->contents_->GetContents();
-      outer_->contents_->SetSuperMac(outer_->pref_hash_calculator_.Calculate(
-          outer_->contents_->hash_store_id(), hashes_dict));
-    }
-    outer_->has_pending_write_ = true;
+  if (super_mac_dirty_ && outer_->use_super_mac_) {
+    // Get the dictionary of hashes (or NULL if it doesn't exist).
+    const base::DictionaryValue* hashes_dict = contents_.GetContents();
+    contents_.SetSuperMac(outer_->pref_hash_calculator_.Calculate(
+        contents_.hash_store_id(), hashes_dict));
   }
-
 }
 
 PrefHashStoreTransaction::ValueState
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue(
-    const std::string& path, const base::Value* initial_value) const {
-  const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
+    const std::string& path,
+    const base::Value* initial_value) const {
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
 
   std::string last_hash;
   if (hashed_prefs)
@@ -125,8 +110,8 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue(
   if (last_hash.empty()) {
     // In the absence of a hash for this pref, always trust a NULL value, but
     // only trust an existing value if the initial hashes dictionary is trusted.
-    return (!initial_value || outer_->initial_hashes_dictionary_trusted_) ?
-               TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
+    return (!initial_value || super_mac_valid_) ? TRUSTED_UNKNOWN_VALUE
+                                                : UNTRUSTED_UNKNOWN_VALUE;
   }
 
   PrefHashCalculator::ValidationResult validation_result =
@@ -147,11 +132,43 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue(
 }
 
 void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreHash(
-    const std::string& path, const base::Value* new_value) {
+    const std::string& path,
+    const base::Value* new_value) {
   const std::string mac =
       outer_->pref_hash_calculator_.Calculate(path, new_value);
-  (*outer_->contents_->GetMutableContents())->SetString(path, mac);
-  has_changed_ = true;
+  (*contents_.GetMutableContents())->SetString(path, mac);
+  super_mac_dirty_ = true;
+}
+
+bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::HasHash(
+    const std::string& path) const {
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
+  return hashed_prefs && hashed_prefs->Get(path, NULL);
+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ImportHash(
+    const std::string& path,
+    const base::Value* hash) {
+  if (!hash)

[gab, 2014/06/13 01:57:43]

Start if/else block with the positive condition when possible (so I was told to
do by my C++ readability reviewer).

[erikwright (departed), 2014/06/16 20:51:26]

Done.

+    (*contents_.GetMutableContents())->RemovePath(path, NULL);
+  else
+    (*contents_.GetMutableContents())->Set(path, hash->DeepCopy());
+  super_mac_dirty_ = super_mac_dirty_ || super_mac_valid_;

[gab, 2014/06/13 01:57:43]

I find:

if (super_mac_valid_)
  super_mac_dirty_ = true;

to be more readable here (and I suspect it may even generate slightly more
efficient assembly code since it doesn't always force a write).

[erikwright (departed), 2014/06/16 20:51:26]

Done.

+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ClearHash(
+    const std::string& path) {
+  if ((*contents_.GetMutableContents())->RemovePath(path, NULL) &&
+      super_mac_valid_) {
+    super_mac_dirty_ = true;
+  }
+}
+
+bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::StampSuperMac() {
+  if (!outer_->use_super_mac_ || super_mac_valid_)
+    return false;
+  super_mac_dirty_ = true;
+  return true;
 }
 
 PrefHashStoreTransaction::ValueState
@@ -171,10 +188,8 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue(
   if (!initial_split_value || initial_split_value->empty())
     return has_hashes ? CLEARED : UNCHANGED;
 
-  if (!has_hashes) {
-    return outer_->initial_hashes_dictionary_trusted_ ?
-        TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
-  }
+  if (!has_hashes)
+    return super_mac_valid_ ? TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
 
   bool has_secure_legacy_id_hashes = false;
   std::string keyed_path(path);
@@ -226,15 +241,15 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue(
   }
 
   return invalid_keys->empty()
-      ? (has_secure_legacy_id_hashes ? SECURE_LEGACY : UNCHANGED)
-      : CHANGED;
+             ? (has_secure_legacy_id_hashes ? SECURE_LEGACY : UNCHANGED)
+             : CHANGED;
 }
 
 void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreSplitHash(
     const std::string& path,
     const base::DictionaryValue* split_value) {
   scoped_ptr<HashStoreContents::MutableDictionary> mutable_dictionary =
-      outer_->contents_->GetMutableContents();
+      contents_.GetMutableContents();
   (*mutable_dictionary)->Remove(path, NULL);
 
   if (split_value) {
@@ -251,7 +266,7 @@ void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreSplitHash(
           outer_->pref_hash_calculator_.Calculate(keyed_path, &it.value()));
     }
   }
-  has_changed_ = true;
+  super_mac_dirty_ = true;
 }
 
 bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::GetSplitMacs(
@@ -260,7 +275,7 @@ bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::GetSplitMacs(
   DCHECK(split_macs);
   DCHECK(split_macs->empty());
 
-  const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
   const base::DictionaryValue* split_mac_dictionary = NULL;
   if (!hashed_prefs || !hashed_prefs->GetDictionary(key, &split_mac_dictionary))
     return false;