Fix handling of device cloud signing policy key rotation

chromeos/dbus/fake_session_manager_client.cc

Index: chromeos/dbus/fake_session_manager_client.cc
diff --git a/chromeos/dbus/fake_session_manager_client.cc b/chromeos/dbus/fake_session_manager_client.cc
index 5cc0066993b5bb057dabf2bc36887fbe31e0aeb7..6e6e285ba764c3bae85164a5b5370b6831c1598e 100644
--- a/chromeos/dbus/fake_session_manager_client.cc
+++ b/chromeos/dbus/fake_session_manager_client.cc
@@ -5,14 +5,40 @@
 #include "chromeos/dbus/fake_session_manager_client.h"
 
 #include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/location.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/path_service.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/cryptohome_client.h"
+#include "components/policy/proto/device_management_backend.pb.h"

[stevenjb, 2017/02/23 18:14:28]

I'm not super keen on adding this dependency, small as it may be. If this is
just for a test, can we have it subclass FakeSessionManagerCLient and implement
this there instead? Or maybe use an observer and  device_policy() to do any
validation?

[emaxx, 2017/02/23 19:20:42]

Hmm, I understand that having less dependencies is always better.

But the whole FakeSessionManagerClient class is dedicated for tests, so I don't
really get what conceptual difference would be there between these two fake
client implementations.

Also note that the new functionality that I add here to FakeSessionManagerClient
(storing the owner key in the file) is effectively just closing the gap between
the API contract of the real session manager and this FakeSessionManagerClient.
The consumer code in the policy stack expects the key to be stored in that file,
and IMO it was an oversight that no test actually covers this step.


But if you insist on getting rid of this dependency, then maybe the same can be
achieved by adding an observer or a callback injector into
FakeSessionManagerClient...

 
 namespace chromeos {
 
+namespace {
+
+bool StoreOwnerKey(const std::string& public_key) {
+  base::FilePath owner_key_path;
+  DCHECK(base::PathService::Get(FILE_OWNER_KEY, &owner_key_path));
+  if (!base::CreateDirectory(owner_key_path.DirName())) {
+    LOG(ERROR) << "Failed to create the directory for the owner key file";
+    return false;
+  }
+  if (base::WriteFile(owner_key_path, public_key.c_str(),
+                      public_key.length()) !=
+      base::checked_cast<int>(public_key.length())) {
+    LOG(ERROR) << "Failed to store the owner key file";
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
 FakeSessionManagerClient::FakeSessionManagerClient()
     : start_device_wipe_call_count_(0),
       request_lock_screen_call_count_(0),
@@ -121,11 +147,27 @@ void FakeSessionManagerClient::RetrieveDeviceLocalAccountPolicy(
 void FakeSessionManagerClient::StoreDevicePolicy(
     const std::string& policy_blob,
     const StorePolicyCallback& callback) {
+  enterprise_management::PolicyFetchResponse policy;
+  if (!policy.ParseFromString(policy_blob)) {
+    LOG(ERROR) << "Unable to parse policy protobuf";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, false /* success */));
+    return;
+  }
+
+  bool owner_key_store_success = false;
+  if (policy.has_new_public_key())
+    owner_key_store_success = StoreOwnerKey(policy.new_public_key());
   device_policy_ = policy_blob;
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::Bind(callback, true));
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, true /* success */));
+  if (policy.has_new_public_key()) {
+    for (auto& observer : observers_)
+      observer.OwnerKeySet(owner_key_store_success);
+  }
   for (auto& observer : observers_)
-    observer.PropertyChangeComplete(true);
+    observer.PropertyChangeComplete(true /* success */);
 }
 
 void FakeSessionManagerClient::StorePolicyForUser(