Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/profile_pref_store_manager_unittest.cc

Index: chrome/browser/prefs/profile_pref_store_manager_unittest.cc
diff --git a/chrome/browser/prefs/profile_pref_store_manager_unittest.cc b/chrome/browser/prefs/profile_pref_store_manager_unittest.cc
index c7bb7459d8f62393318a7f95b3d853a5451b601c..c04e6c8154b1e4f9095848484e96214abbbd207c 100644
--- a/chrome/browser/prefs/profile_pref_store_manager_unittest.cc
+++ b/chrome/browser/prefs/profile_pref_store_manager_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/file_util.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -15,6 +16,7 @@
 #include "base/prefs/json_pref_store.h"
 #include "base/prefs/persistent_pref_store.h"
 #include "base/prefs/pref_service.h"
+#include "base/prefs/pref_service_factory.h"
 #include "base/prefs/pref_store.h"
 #include "base/prefs/testing_pref_service.h"
 #include "base/run_loop.h"
@@ -60,6 +62,7 @@ class RegistryVerifier : public PrefStore::Observer {
   scoped_refptr<PrefRegistry> pref_registry_;
 };
 
+const char kUnprotectedAtomic[] = "unprotected_atomic";
 const char kTrackedAtomic[] = "tracked_atomic";
 const char kProtectedAtomic[] = "protected_atomic";
 const char kProtectedSplit[] = "protected_split";

[gab, 2014/04/01 20:19:52]

So this value is never actually tested?

[erikwright (departed), 2014/04/02 15:03:17]

Done.

@@ -101,8 +104,16 @@ class ProfilePrefStoreManagerTest : public testing::Test {
             it->name, user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
       }
     }
+    profile_pref_registry_->RegisterStringPref(
+        kUnprotectedAtomic,
+        std::string(),
+        user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+
     ASSERT_TRUE(profile_dir_.CreateUniqueTempDir());
+    ReloadConfiguration();
+  }
 
+  void ReloadConfiguration() {
     manager_.reset(new ProfilePrefStoreManager(profile_dir_.path(),
                                                configuration_,
                                                configuration_.size(),
@@ -111,7 +122,29 @@ class ProfilePrefStoreManagerTest : public testing::Test {
                                                &local_state_));
   }
 
-  virtual void TearDown() OVERRIDE {
+  virtual void TearDown() OVERRIDE { DestroyPrefStore(); }
+
+ protected:
+  bool ResetRecorded() {

[gab, 2014/04/01 20:19:52]

s/ResetRecorded/WasResetRecorded

Otherwise I thought from reading the name of this method that it was itself
performing the action of resetting some recorded state.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+    base::PrefServiceFactory pref_service_factory;
+    pref_service_factory.set_user_prefs(pref_store_);
+
+    scoped_ptr<PrefService> pref_service(
+        pref_service_factory.Create(profile_pref_registry_));
+
+    return !ProfilePrefStoreManager::GetResetTime(pref_service.get()).is_null();
+  }
+
+  void InitializePrefs() {
+    scoped_refptr<PersistentPrefStore> pref_store =

[gab, 2014/04/01 20:19:52]

Add a comment stating that this initializes the SegregatedPrefStore. I think it
matters, yet it wasn't obvious to me on first read (I had to come back up with
the question "so where is the segregated store tested from?" to realize it was
here.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+        manager_->CreateProfilePrefStore(
+            main_message_loop_.message_loop_proxy());
+    InitializePrefStore(pref_store);
+    pref_store = NULL;
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void DestroyPrefStore() {
     if (pref_store_) {
       // Force everything to be written to disk, triggering the PrefHashFilter
       // while our RegistryVerifier is watching.
@@ -126,42 +159,51 @@ class ProfilePrefStoreManagerTest : public testing::Test {
     }
   }
 
- protected:
-  void InitializePrefs() {
+  void InitializeDeprecatedCombinedProfilePrefStore() {
     scoped_refptr<PersistentPrefStore> pref_store =
-        manager_->CreateProfilePrefStore(
+        manager_->CreateDeprecatedCombinedProfilePrefStore(
             main_message_loop_.message_loop_proxy());
+    InitializePrefStore(pref_store);
+    pref_store = NULL;
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void InitializePrefStore(PersistentPrefStore* pref_store) {
     pref_store->AddObserver(&registry_verifier_);
     PersistentPrefStore::PrefReadError error = pref_store->ReadPrefs();
-    ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NO_FILE, error);
+    EXPECT_EQ(PersistentPrefStore::PREF_READ_ERROR_NO_FILE, error);
     pref_store->SetValue(kTrackedAtomic, new base::StringValue(kFoobar));
     pref_store->SetValue(kProtectedAtomic, new base::StringValue(kHelloWorld));
+    pref_store->SetValue(kUnprotectedAtomic, new base::StringValue(kFoobar));
     pref_store->RemoveObserver(&registry_verifier_);
-    pref_store = NULL;
+    pref_store->CommitPendingWrite();
     base::RunLoop().RunUntilIdle();
   }
 
   void LoadExistingPrefs() {
+    DestroyPrefStore();
     pref_store_ = manager_->CreateProfilePrefStore(
         main_message_loop_.message_loop_proxy());
     pref_store_->AddObserver(&registry_verifier_);
-    EXPECT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE,
-              pref_store_->ReadPrefs());
+    pref_store_->ReadPrefs();
   }
 
   void ReplaceStringInPrefs(const std::string& find,
                             const std::string& replace) {
-    // Tamper with the file's contents
-    base::FilePath pref_file_path =
-        ProfilePrefStoreManager::GetPrefFilePathFromProfilePath(
-            profile_dir_.path());
-    std::string pref_file_contents;
-    EXPECT_TRUE(base::ReadFileToString(pref_file_path, &pref_file_contents));
-    ReplaceSubstringsAfterOffset(&pref_file_contents, 0u, find, replace);
-    EXPECT_EQ(static_cast<int>(pref_file_contents.length()),
-              base::WriteFile(pref_file_path,
-                              pref_file_contents.c_str(),
-                              pref_file_contents.length()));
+    base::FileEnumerator file_enumerator(
+        profile_dir_.path(), true, base::FileEnumerator::FILES);

[gab, 2014/04/01 20:19:52]

Why recursive?

[erikwright (departed), 2014/04/02 15:03:17]

Assuming that the files are stored at the top-level seems an unnecessary
assumption to me.

+
+    base::FilePath pref_file_path;
+    while (!(pref_file_path = file_enumerator.Next()).empty()) {

[gab, 2014/04/01 20:19:52]

Use the pattern suggested @
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_en...
to loop over the files in the FileEnumerator.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+      // Tamper with the file's contents
+      std::string pref_file_contents;
+      EXPECT_TRUE(base::ReadFileToString(pref_file_path, &pref_file_contents));
+      ReplaceSubstringsAfterOffset(&pref_file_contents, 0u, find, replace);
+      EXPECT_EQ(static_cast<int>(pref_file_contents.length()),
+                base::WriteFile(pref_file_path,
+                                pref_file_contents.c_str(),
+                                pref_file_contents.length()));
+    }
   }
 
   void ExpectStringValueEquals(const std::string& name,
@@ -194,6 +236,7 @@ TEST_F(ProfilePrefStoreManagerTest, StoreValues) {
 
   ExpectStringValueEquals(kTrackedAtomic, kFoobar);
   ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+  EXPECT_FALSE(ResetRecorded());
 }
 
 TEST_F(ProfilePrefStoreManagerTest, GetPrefFilePathFromProfilePath) {
@@ -201,11 +244,11 @@ TEST_F(ProfilePrefStoreManagerTest, GetPrefFilePathFromProfilePath) {
       ProfilePrefStoreManager::GetPrefFilePathFromProfilePath(
           profile_dir_.path());
 
-  ASSERT_FALSE(base::PathExists(pref_file_path));
+  EXPECT_FALSE(base::PathExists(pref_file_path));
 
   InitializePrefs();
 
-  ASSERT_TRUE(base::PathExists(pref_file_path));
+  EXPECT_TRUE(base::PathExists(pref_file_path));
 }
 
 TEST_F(ProfilePrefStoreManagerTest, ProtectValues) {
@@ -222,8 +265,10 @@ TEST_F(ProfilePrefStoreManagerTest, ProtectValues) {
 
   // If preference tracking is supported, the tampered value of kProtectedAtomic
   // will be discarded at load time, leaving this preference undefined.
-  EXPECT_EQ(!ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+  EXPECT_NE(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
             pref_store_->GetValue(kProtectedAtomic, NULL));
+  EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            ResetRecorded());
 }
 
 TEST_F(ProfilePrefStoreManagerTest, ResetPrefHashStore) {
@@ -237,8 +282,10 @@ TEST_F(ProfilePrefStoreManagerTest, ResetPrefHashStore) {
   ExpectStringValueEquals(kTrackedAtomic, kFoobar);
   // If preference tracking is supported, the tampered value of kProtectedAtomic
   // will be discarded at load time, leaving this preference undefined.
-  EXPECT_EQ(!ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+  EXPECT_NE(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
             pref_store_->GetValue(kProtectedAtomic, NULL));
+  EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            ResetRecorded());
 }
 
 TEST_F(ProfilePrefStoreManagerTest, ResetAllPrefHashStores) {
@@ -252,14 +299,32 @@ TEST_F(ProfilePrefStoreManagerTest, ResetAllPrefHashStores) {
   ExpectStringValueEquals(kTrackedAtomic, kFoobar);
   // If preference tracking is supported, kProtectedAtomic will be undefined
   // because the value was discarded due to loss of the hash store contents.
-  EXPECT_EQ(!ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+  EXPECT_NE(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
             pref_store_->GetValue(kProtectedAtomic, NULL));
+  EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            ResetRecorded());
 }
 
-TEST_F(ProfilePrefStoreManagerTest, UpdateProfileHashStoreIfRequired) {
-  InitializePrefs();
+TEST_F(ProfilePrefStoreManagerTest, MigrateFromOneFile) {
+  InitializeDeprecatedCombinedProfilePrefStore();
 
-  manager_->ResetPrefHashStore();
+  LoadExistingPrefs();
+
+  ExpectStringValueEquals(kTrackedAtomic, kFoobar);
+  ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+  EXPECT_FALSE(ResetRecorded());
+}
+
+TEST_F(ProfilePrefStoreManagerTest, UpdateProfileHashStoreIfRequired) {
+  scoped_refptr<JsonPrefStore> legacy_prefs(
+      new JsonPrefStore(ProfilePrefStoreManager::GetPrefFilePathFromProfilePath(
+                            profile_dir_.path()),
+                        main_message_loop_.message_loop_proxy(),
+                        scoped_ptr<PrefFilter>()));
+  legacy_prefs->SetValue(kTrackedAtomic, new base::StringValue(kFoobar));
+  legacy_prefs->SetValue(kProtectedAtomic, new base::StringValue(kHelloWorld));
+  legacy_prefs = NULL;
+  base::RunLoop().RunUntilIdle();
 
   // This is a no-op if !kPlatformSupportsPreferenceTracking.
   manager_->UpdateProfileHashStoreIfRequired(
@@ -273,6 +338,7 @@ TEST_F(ProfilePrefStoreManagerTest, UpdateProfileHashStoreIfRequired) {
   // These expectations hold whether or not tracking is supported.
   ExpectStringValueEquals(kTrackedAtomic, kFoobar);
   ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+  EXPECT_FALSE(ResetRecorded());
 }
 
 TEST_F(ProfilePrefStoreManagerTest, InitializePrefsFromMasterPrefs) {
@@ -280,7 +346,7 @@ TEST_F(ProfilePrefStoreManagerTest, InitializePrefsFromMasterPrefs) {
       new base::DictionaryValue);
   master_prefs->Set(kTrackedAtomic, new base::StringValue(kFoobar));
   master_prefs->Set(kProtectedAtomic, new base::StringValue(kHelloWorld));
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager_->InitializePrefsFromMasterPrefs(*master_prefs));
 
   LoadExistingPrefs();
@@ -289,4 +355,113 @@ TEST_F(ProfilePrefStoreManagerTest, InitializePrefsFromMasterPrefs) {
   // necessary to authenticate these values.
   ExpectStringValueEquals(kTrackedAtomic, kFoobar);
   ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+  EXPECT_FALSE(ResetRecorded());
+}
+
+TEST_F(ProfilePrefStoreManagerTest, UnprotectedToProtected) {
+  InitializePrefs();
+  LoadExistingPrefs();
+  ExpectStringValueEquals(kUnprotectedAtomic, kFoobar);
+
+  // Ensure everything is written out to disk.
+  DestroyPrefStore();
+
+  ReplaceStringInPrefs(kFoobar, kBarfoo);
+
+  // It's unprotected, so we can load the modified value.
+  LoadExistingPrefs();
+  ExpectStringValueEquals(kUnprotectedAtomic, kBarfoo);
+
+  // Now update the configuration to protect it.
+  PrefHashFilter::TrackedPreferenceMetadata new_protected = {
+      99u, kUnprotectedAtomic, PrefHashFilter::ENFORCE_ON_LOAD,

[gab, 2014/04/01 20:19:52]

Why 99u? This is wrong in practice, although it doesn't actually matter in this
test, as you'll report that the reporting_ids_count is configuration.size()
which is 1.

0u works just the same as is more correct IMO.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+      PrefHashFilter::TRACKING_STRATEGY_ATOMIC};
+  configuration_.push_back(new_protected);
+  ReloadConfiguration();
+
+  // And try loading with the new configuration.
+  LoadExistingPrefs();
+
+  // Since there was a valid super MAC we were able to extend the existing trust
+  // to the newly proteted preference.
+  ExpectStringValueEquals(kUnprotectedAtomic, kBarfoo);
+  EXPECT_FALSE(ResetRecorded());
+
+  // Ensure everything is written out to disk.
+  DestroyPrefStore();
+
+  // It's protected now, so (if the platform supports it) any tampering should
+  // lead to a reset.
+  ReplaceStringInPrefs(kBarfoo, kFoobar);
+  LoadExistingPrefs();
+  EXPECT_NE(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            pref_store_->GetValue(kUnprotectedAtomic, NULL));
+  EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            ResetRecorded());
+}
+
+TEST_F(ProfilePrefStoreManagerTest, UnprotectedToProtectedWithoutTrust) {
+  InitializePrefs();
+
+  // Now update the configuration to protect it.
+  PrefHashFilter::TrackedPreferenceMetadata new_protected = {
+      99u, kUnprotectedAtomic, PrefHashFilter::ENFORCE_ON_LOAD,
+      PrefHashFilter::TRACKING_STRATEGY_ATOMIC};
+  configuration_.push_back(new_protected);
+  ReloadConfiguration();
+  ProfilePrefStoreManager::ResetAllPrefHashStores(&local_state_);
+
+  // And try loading with the new configuration.
+  LoadExistingPrefs();
+
+  // If preference tracking is supported, kUnprotectedAtomic will have been
+  // discarded because new values are not accepted without a valid super MAC.
+  EXPECT_NE(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            pref_store_->GetValue(kUnprotectedAtomic, NULL));
+  EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
+            ResetRecorded());
+}
+
+// This test does not directly verify that the values are moved from one pref
+// store to the other. segregated_pref_store_unittest.cc _does_ verify that
+// functionality.
+//
+// _This_ test verifies that preference values are correctly maintained when a
+// preference's protection state changes from protected to unprotected.
+TEST_F(ProfilePrefStoreManagerTest, ProtectedToUnprotected) {
+  InitializePrefs();
+  DestroyPrefStore();
+
+  // Unconfigure protection for kProtectedAtomic
+  for (std::vector<PrefHashFilter::TrackedPreferenceMetadata>::iterator it =
+           configuration_.begin();
+       it != configuration_.end();
+       ++it) {
+    if (it->name == std::string(kProtectedAtomic)) {

[gab, 2014/04/01 20:19:52]

I don't think a string compare is required here; a pointer comparison is enough,
the configs also use the pointer to the constant.

[erikwright (departed), 2014/04/02 15:03:17]

Done.

+      configuration_.erase(it);
+      break;
+    }
+  }
+  ReloadConfiguration();
+
+  // Reset the hash stores and then try loading the prefs.
+  ProfilePrefStoreManager::ResetAllPrefHashStores(&local_state_);
+  LoadExistingPrefs();
+
+  // Verify that the value was not reset.
+  ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+  EXPECT_FALSE(ResetRecorded());
+
+  // Accessing the value of the previously protected pref didn't trigger its
+  // move to the unprotected preferences file, though the loading of the pref
+  // store should still have caused the MAC store to be recalculated.
+  LoadExistingPrefs();
+  ExpectStringValueEquals(kProtectedAtomic, kHelloWorld);
+
+  // Trigger the logic that migrates it back to the unprotected preferences
+  // file.
+  pref_store_->SetValue(kProtectedAtomic, new base::StringValue(kGoodbyeWorld));
+  LoadExistingPrefs();
+  ExpectStringValueEquals(kProtectedAtomic, kGoodbyeWorld);
+  EXPECT_FALSE(ResetRecorded());
 }