Added code to persist downloaded cloud policy to disk.

chrome/browser/policy/user_cloud_policy_store.cc

Index: chrome/browser/policy/user_cloud_policy_store.cc
diff --git a/chrome/browser/policy/user_cloud_policy_store.cc b/chrome/browser/policy/user_cloud_policy_store.cc
index 69d4461bcc20af5679231c1abff39d601f69d077..4388f729cf077de9a5f25db34389255db9c6c3f2 100644
--- a/chrome/browser/policy/user_cloud_policy_store.cc
+++ b/chrome/browser/policy/user_cloud_policy_store.cc
@@ -5,47 +5,203 @@
 #include "chrome/browser/policy/user_cloud_policy_store.h"
 
 #include "base/bind.h"
+#include "base/file_path.h"

[Joao da Silva, 2012/08/22 08:07:00]

Nit: already in .h

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Done.

+#include "base/file_util.h"
 #include "chrome/browser/policy/proto/cloud_policy.pb.h"
+#include "chrome/browser/policy/proto/device_management_backend.pb.h"
+#include "chrome/browser/policy/proto/device_management_local.pb.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "content/public/browser/browser_thread.h"
 
-using enterprise_management::PolicyData;
+namespace em = enterprise_management;
+
+namespace {
+// Subdirectory in the user's profile for storing user policies.
+const FilePath::CharType kPolicyDir[] = FILE_PATH_LITERAL("Policy");
+// File in the above directory for storing user policy data.
+const FilePath::CharType kPolicyCacheFile[] = FILE_PATH_LITERAL("User Policy");
+
+// Helper class that is used to persist and reload policy on disk.
+class PolicyBackingFile
+    : public base::RefCountedThreadSafe<PolicyBackingFile> {

[Mattias Nissler (ping if slow), 2012/08/22 08:21:23]

I don't see the need for making this a RefCountedThreadSafe class. Two static
functions and PostTaskAndReplyWithResult should work? The old
UserPolicyDiskCache is from the times when we didn't have a modern callback
system that works nice with weak ptrs.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Hah, I knew about PostTaskAndReply but not PostTaskAndReplyWithResult. OK, I'll
give it a shot.

+ public:
+  explicit PolicyBackingFile(const FilePath& path)
+      : backing_file_path_(path),
+        error_(false) {}
+
+  // Loads policy from the backing file (must be called via a task on
+  // the FILE thread). Once complete, loaded policy is in |policy_| and
+  // error() will return true if an error was encountered.
+  void LoadPolicyFromDiskOnFileThread() {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+    // If the backing file did not exist, just return.
+    if (!file_util::PathExists(backing_file_path_))
+      return;
+
+    std::string data;
+    if (!file_util::ReadFileToString(backing_file_path_, &data)) {
+      DLOG(WARNING) << "Failed to read policy data from "

[Mattias Nissler (ping if slow), 2012/08/22 08:21:23]

Make this and the one below a LOG(WARNING)? That'd help diagnose issues in
release builds, which we've struggled with on Chrome OS due to insufficient
logging a number of times.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Hmmm. I thought we (chromium) were trying to avoid LOG() since it bloats the
executable?

[Mattias Nissler (ping if slow), 2012/08/23 09:28:34]

So why do we have crash reporting code then given that it adds bytes to the
executable? :) I agree with the general idea of avoiding logs, but I think these
would be actually helpful in the wild.

[Andrew T Wilson (Slow), 2012/08/23 18:00:08]

Done.

+                    << backing_file_path_.value();
+      error_ = true;
+      return;
+    }
+    policy_.reset(new em::PolicyFetchResponse());
+    if (!policy_->ParseFromArray(data.c_str(), data.size())) {
+      DLOG(WARNING) << "Failed to parse policy data from "
+                    << backing_file_path_.value();
+      error_ = true;
+      return;

[Joao da Silva, 2012/08/22 08:07:00]

policy_.reset() before returning here?

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

No longer necessary after latest refactoring.

+    }
+  }
+
+  // Stores policy to the backing file (must be called via a task on
+  // the FILE thread).
+  void StorePolicyToDiskOnFileThread(const em::PolicyFetchResponse& policy) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+    std::string data;
+    if (!policy.SerializeToString(&data)) {
+      DLOG(WARNING) << "Failed to serialize policy data";
+      return;
+    }
+
+    if (!file_util::CreateDirectory(backing_file_path_.DirName())) {
+      DLOG(WARNING) << "Failed to create directory "
+                    << backing_file_path_.DirName().value();
+      return;
+    }
+
+    int size = data.size();
+    if (file_util::WriteFile(
+            backing_file_path_, data.c_str(), size) != size) {
+      DLOG(WARNING) << "Failed to write " << backing_file_path_.value();
+    }
+  }
+
+  // Returns the loaded policy blob, or NULL if there was no policy file.
+  scoped_ptr<em::PolicyFetchResponse> policy() { return policy_.Pass(); }
+
+  // Returns true if there was an error reading/writing the policy blob
+  // (false if the file did not exist).
+  bool error() { return error_; }
+
+ private:
+  // The path of the file containing the policy blob.
+  FilePath backing_file_path_;
+
+  // If true, an error was encountered reading the file.
+  bool error_;
+
+  // The policy blob to store/return.
+  scoped_ptr<em::PolicyFetchResponse> policy_;
+};
+
+}  // namespace
 
 namespace policy {
 
-UserCloudPolicyStore::UserCloudPolicyStore(Profile* profile)
+UserCloudPolicyStore::UserCloudPolicyStore(Profile* profile,
+                                           const FilePath& path)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      profile_(profile) {
+      profile_(profile),
+      backing_file_path_(path) {
 }
 
 UserCloudPolicyStore::~UserCloudPolicyStore() {
 }
 
+static void PolicyLoadedFromDisk(

[Joao da Silva, 2012/08/22 08:07:00]

Nit: namespace { ... } instead of static?

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Removed.

+    UserCloudPolicyStore* store,

[Joao da Silva, 2012/08/22 08:07:00]

Shouldn't this be a WeakPtr?

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Interesting - I thought that the WeakPtr->regular ptr translation was handled by
the calling template code, so all I needed to do was check for null? If not, I
wonder how this compiles - *somebody* must be converting between the types.

In any case, it's gone now.

+    scoped_refptr<PolicyBackingFile> backing_file) {
+  if (store)
+    store->PolicyLoaded(backing_file->error(), backing_file->policy());
+}
+
 void UserCloudPolicyStore::Load() {
-  // TODO(atwilson): Read policy from file.
-  policy_.reset();
-  policy_map_.Clear();
+  DVLOG(1) << "Initiating policy load from disk";
+  // Cancel any pending Load/Store/Validate operations.
+  weak_factory_.InvalidateWeakPtrs();
+
+  // Start a new Load operation and have us get called back when it is
+  // complete.
+  scoped_refptr<PolicyBackingFile> policy_backing_file =
+      new PolicyBackingFile(backing_file_path_);
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&PolicyBackingFile::LoadPolicyFromDiskOnFileThread,
+                 policy_backing_file),
+      base::Bind(&PolicyLoadedFromDisk,
+                 weak_factory_.GetWeakPtr(),
+                 policy_backing_file));

[Joao da Silva, 2012/08/22 08:07:00]

Two suggestions, feel free to ignore :-)

1) Instead of making PolicyBackingFile a refcounted object, you can just make it
a "regular" object and pass as base::Owned() to the 2nd bind.

2) PolicyLoadedFromDisk() can be a method of UserCloudPolicyStore, and the
|this| object can be passed as a WeakPtr. Bind() takes care of testing the
WeakPtr before dispatching.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

The reason I did not do this is because I did not want to expose
PolicyBackingFile in the header file (which means I'd have to keep it outside
the anonymous namespace).

I'll look at this again as part of the suggestion to use
PostTaskAndReplyWithResult().

+}
+
+void UserCloudPolicyStore::PolicyLoaded(
+    bool error, scoped_ptr<em::PolicyFetchResponse> cloud_policy) {

[Mattias Nissler (ping if slow), 2012/08/22 08:21:23]

each parameter on separate line.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Done.

+  if (error) {
+    DLOG(WARNING) << "Error reading policy from disk";
+    status_ = STATUS_LOAD_ERROR;
+    NotifyStoreError();
+    return;
+  }
+
+  status_ = STATUS_OK;
+  if (!cloud_policy.get()) {
+    // No policy found on disk.
+    DVLOG(1) << "No policy found on disk";
+    NotifyStoreLoaded();
+    return;
+  }
+
+  // Found policy on disk - need to validate it before it can be used.
+  Validate(cloud_policy.Pass(),
+           base::Bind(
+               &UserCloudPolicyStore::InstallLoadedPolicyAfterValidation,
+               weak_factory_.GetWeakPtr()));
+}
+
+void UserCloudPolicyStore::InstallLoadedPolicyAfterValidation(
+    UserCloudPolicyValidator* validator) {
+  validation_status_ = validator->status();
+  if (!validator->success()) {
+    DVLOG(1) << "Validation failed: status=" << validation_status_;
+    status_ = STATUS_VALIDATION_ERROR;
+    NotifyStoreError();
+    return;
+  }
+
+  DVLOG(1) << "Validation succeeded - installing policy with dm_token: " <<
+      validator->policy_data()->request_token();
+  DVLOG(1) << "Device ID: " << validator->policy_data()->device_id();
+
+  InstallPolicy(validator->policy_data().Pass(), validator->payload().Pass());
+  status_ = STATUS_OK;
   NotifyStoreLoaded();
 }
 
+static void DeleteFileOnFileThread(const FilePath& path) {

[Joao da Silva, 2012/08/22 08:07:00]

Nit: namespace { ... }  instead of static?

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Removed in favor of base::IgnoreResult

+  file_util::Delete(path, false);
+}
+
 void UserCloudPolicyStore::RemoveStoredPolicy() {
-  // TODO(atwilson): Remove policy from disk.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DeleteFileOnFileThread, backing_file_path_));

[Joao da Silva, 2012/08/22 08:07:00]

Why not just bind file_util::Delete()?

[Mattias Nissler (ping if slow), 2012/08/22 08:21:23]

That should work if you wrap it in a base::IgnoreResult.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

I could not get it to compile - I thought it was because file_util::Delete() is
not a void() function. I guess I could have thrown some casting at it.

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Ah, there you go. Thanks.

 }
 
-void UserCloudPolicyStore::Store(
-    const enterprise_management::PolicyFetchResponse& policy) {
+void UserCloudPolicyStore::Store(const em::PolicyFetchResponse& policy) {
   // Stop any pending requests to store policy, then validate the new policy
   // before storing it.
   weak_factory_.InvalidateWeakPtrs();
-  scoped_ptr<enterprise_management::PolicyFetchResponse> policy_copy(
-      new enterprise_management::PolicyFetchResponse(policy));
+  scoped_ptr<em::PolicyFetchResponse> policy_copy(
+      new em::PolicyFetchResponse(policy));
   Validate(policy_copy.Pass(),
            base::Bind(&UserCloudPolicyStore::StorePolicyAfterValidation,
                       weak_factory_.GetWeakPtr()));
 }
 
 void UserCloudPolicyStore::Validate(
-    scoped_ptr<enterprise_management::PolicyFetchResponse> policy,
+    scoped_ptr<em::PolicyFetchResponse> policy,
     const UserCloudPolicyValidator::CompletionCallback& callback) {
   // Configure the validator.
   scoped_ptr<UserCloudPolicyValidator> validator =
@@ -63,12 +219,21 @@ void UserCloudPolicyStore::Validate(
 void UserCloudPolicyStore::StorePolicyAfterValidation(
     UserCloudPolicyValidator* validator) {
   validation_status_ = validator->status();
+  DVLOG(1) << "Policy validation complete: status = " << validation_status_;
   if (!validator->success()) {
     status_ = STATUS_VALIDATION_ERROR;
     NotifyStoreError();
     return;
   }
-  // TODO(atwilson): Write policy to file.
+
+  // Persist the validated policy (just fire a task - don't bother getting a
+  // reply because we can't do anything if it fails).
+  scoped_refptr<PolicyBackingFile> policy_backing_file =
+      new PolicyBackingFile(backing_file_path_);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&PolicyBackingFile::StorePolicyToDiskOnFileThread,
+                 policy_backing_file, *validator->policy()));

[Joao da Silva, 2012/08/22 08:07:00]

Same here: |policy_backing_file| can be passed with base::Owned() so that it is
deleted once the task is dropped (whether it executed before or not).

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Thanks for the tips. I haven't used the new bind stuff much so I'm still
learning the best practices.

   InstallPolicy(validator->policy_data().Pass(), validator->payload().Pass());
   status_ = STATUS_OK;
   NotifyStoreLoaded();
@@ -77,7 +242,10 @@ void UserCloudPolicyStore::StorePolicyAfterValidation(
 // static
 scoped_ptr<CloudPolicyStore> CloudPolicyStore::CreateUserPolicyStore(
     Profile* profile) {
-  return scoped_ptr<CloudPolicyStore>(new UserCloudPolicyStore(profile));
+  FilePath path = profile->GetPath();
+  path = path.Append(kPolicyDir);
+  path = path.Append(kPolicyCacheFile);

[Joao da Silva, 2012/08/22 08:07:00]

Suggestion:

FilePath path = profile->GetPath().Append(kPolicyDir).Append(kPolicyCacheFile);

[Andrew T Wilson (Slow), 2012/08/22 23:39:30]

Done. And, tragically, it comes to 81 characters or otherwise it'd fit on one
line :)

+  return scoped_ptr<CloudPolicyStore>(new UserCloudPolicyStore(profile, path));
 }
 
 }  // namespace policy