Implement FakeAuthPolicyClient::RefreshDevicePolicy()

chromeos/dbus/fake_auth_policy_client.cc

Index: chromeos/dbus/fake_auth_policy_client.cc
diff --git a/chromeos/dbus/fake_auth_policy_client.cc b/chromeos/dbus/fake_auth_policy_client.cc
index edcf52857d7b7d204e333fc71bf0a3470840f50b..de8d4521cd0786db1e379b3a6025e036ad73d394 100644
--- a/chromeos/dbus/fake_auth_policy_client.cc
+++ b/chromeos/dbus/fake_auth_policy_client.cc
@@ -1,11 +1,64 @@
 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+
 #include "chromeos/dbus/fake_auth_policy_client.h"
 
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/location.h"
+#include "base/path_service.h"
+#include "base/task_runner_util.h"
+#include "base/threading/worker_pool.h"
+#include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h"
+#include "chromeos/chromeos_paths.h"
+#include "components/policy/proto/device_management_backend.pb.h"
+
+namespace em = enterprise_management;
+
+namespace {
+
+bool WriteDevicePolicyFile() {
+  // Create minimal stub device policy file and drop it at the place where

[achuithb, 2016/11/30 20:22:49]

This comment may be better served as a function comment right before line 22

[Thiemo Nagel, 2016/12/01 09:52:04]

Good point.  Done.

+  // SessionManagerClientStubImpl is looking for it.
+  em::ChromeDeviceSettingsProto policy;
+  em::PolicyData data;
+  policy.SerializeToString(data.mutable_policy_value());
+  data.set_policy_type("google/chromeos/device");
+
+  em::PolicyFetchResponse response;
+  data.SerializeToString(response.mutable_policy_data());
+  std::string serialized_response;
+  response.SerializeToString(&serialized_response);
+
+  base::FilePath owner_key_path;
+  if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path))
+    return false;
+
+  const base::FilePath device_policy_path =
+      owner_key_path.DirName().AppendASCII("stub_device_policy");
+
+  // Note that in theory there could be a short time window in which a
+  // concurrent reader sees a partial (and thus invalid) file, but given the
+  // small file size that seems very unlikely in practice.
+  const int bytes_written =
+      base::WriteFile(device_policy_path, serialized_response.c_str(),
+                      serialized_response.size());
+  if (bytes_written < 0)
+    return false;
+  static_assert(std::numeric_limits<int>::max() <=

[achuithb, 2016/11/30 20:22:49]

What's your concern here?

At a minimum we need a comment.

[Thiemo Nagel, 2016/12/01 09:52:04]

I was afraid that a narrowing cast could lead to undefined behavior.  Looking it
up on the web [1], it seems that a narrowing cast is implementation-defined
which sgtm since the numbers will always be small anyways.  Thus removing the
static_assert().

[1]
http://stackoverflow.com/questions/19273658/integer-conversionsnarrowing-wide...

+                    std::numeric_limits<std::string::size_type>::max(),
+                "unsafe cast");
+  return static_cast<std::string::size_type>(bytes_written) ==
+         serialized_response.size();
+}
+
+}  // namespace
+
 namespace chromeos {
 
-FakeAuthPolicyClient::FakeAuthPolicyClient() {}
+FakeAuthPolicyClient::FakeAuthPolicyClient() : weak_factory_(this) {}
 
 FakeAuthPolicyClient::~FakeAuthPolicyClient() {}
 
@@ -20,13 +73,25 @@ void FakeAuthPolicyClient::JoinAdDomain(const std::string& machine_name,
 
 void FakeAuthPolicyClient::RefreshDevicePolicy(
     const RefreshPolicyCallback& callback) {
-  callback.Run(true);
-};
+  if (!base::PostTaskAndReplyWithResult(
+          base::WorkerPool::GetTaskRunner(false /* task_is_slow */).get(),
+          FROM_HERE, base::Bind(&WriteDevicePolicyFile),
+          base::Bind(&FakeAuthPolicyClient::OnDevicePolicyFileWritten,

[achuithb, 2016/11/30 20:22:50]

Curious: do we need OnDevicePolicyFileWritten? Can't this argument simply be
callback?

At a minimum, it looks like OnDevicePolicyFileWritten can also be an anonymous
function, so we don't need WeakPtrFactory at all.

[achuithb, 2016/11/30 20:26:18]

Ok, I don't think just passing callback will work, but maybe you could use a
lambda instead of an anonymous function?

https://groups.google.com/a/chromium.org/forum/#!msg/cxx/QxjsPELDYdQ/IH8wNQln...

[Thiemo Nagel, 2016/12/01 09:52:04]

Good point, and passing the callback actually does work. :)

[achuithb, 2016/12/01 20:26:14]

I'll never understand the voodoo of functors.

+                     weak_factory_.GetWeakPtr(), callback))) {
+    callback.Run(false);
+  }
+}
+
+void FakeAuthPolicyClient::OnDevicePolicyFileWritten(
+    const RefreshPolicyCallback& callback,
+    bool success) {
+  callback.Run(success);
+}
 
 void FakeAuthPolicyClient::RefreshUserPolicy(
     const std::string& account_id,
     const RefreshPolicyCallback& callback) {
   callback.Run(true);
-};
+}
 
 }  // namespace chromeos