Using PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac.

remoting/host/policy_hack/policy_service_watcher.cc

Index: remoting/host/policy_hack/policy_service_watcher.cc
diff --git a/remoting/host/policy_hack/policy_service_watcher.cc b/remoting/host/policy_hack/policy_service_watcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bdb68e502e7de2e0252c96d2b5faec89d6b2689d
--- /dev/null
+++ b/remoting/host/policy_hack/policy_service_watcher.cc
@@ -0,0 +1,221 @@
+// Copyright 2014 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 "base/files/file_path.h"
+#include "base/single_thread_task_runner.h"
+#include "components/policy/core/common/async_policy_loader.h"
+#include "components/policy/core/common/async_policy_provider.h"
+#include "components/policy/core/common/policy_namespace.h"
+#include "components/policy/core/common/policy_service.h"
+#include "components/policy/core/common/policy_service_impl.h"
+#include "components/policy/core/common/schema.h"
+#include "components/policy/core/common/schema_registry.h"
+#include "policy/policy_constants.h"
+#include "remoting/host/policy_hack/policy_watcher.h"
+
+#if defined(OS_CHROMEOS)
+#include "content/public/browser/browser_thread.h"
+#elif defined(OS_WIN)
+#include "components/policy/core/common/policy_loader_win.h"
+#elif defined(OS_MACOSX)
+#include "components/policy/core/common/policy_loader_mac.h"
+#include "components/policy/core/common/preferences_mac.h"
+#elif defined(OS_POSIX) && !defined(OS_ANDROID)
+#include "components/policy/core/common/config_dir_policy_loader.h"
+#endif
+
+using namespace policy;
+
+namespace remoting {
+namespace policy_hack {
+
+bool GetManagedPrefsDir(base::FilePath* result);
+
+namespace {
+
+// PolicyServiceWatcher is a concrete implementation of PolicyWatcher that wraps
+// an instance of PolicyService.

[Mattias Nissler (ping if slow), 2015/01/06 09:06:11]

Given that this is now the only implementation of PolicyWatcher, it may make
sense to remove PolicyWatch as a pure virtual base class?

[Łukasz Anforowicz, 2015/01/07 17:54:14]

I am not sure if I fully understand (my C++ is rusty, not sure if I know exactly
what you mean by "pure virtual base class").  I thought about doing
(1) Making PolicyWatcher non-virtual (i.e. removing "virtual" from its instance
methods + merging PolicyServiceWatcher into PolicyWatcher class + removing
FakePolicyWatcher class)
and
(2) Moving PolicyServiceWatcher class into policy_watcher.cc file

I think I don't want to do (1) (at least not yet):
A) I want to keep using FakePolicyWatcher for now (trying to make the current
changelist minimal)
B) We need to decide whether PolicyServiceWatcher is here to stay for the long
term.  If it stays,
   then I want to add policy_service_watcher.h and add unit tests that wrap a
MockPolicyService.
   If it goes away (i.e. we want to remove the deep copy and maybe instead pass
a thread-safe PolicyMap)
   then we'll reconsider (1)

I didn't do (2) because
i) I wasn't sure if this is the right thing to do because of (B) above
ii) I wanted to minimize the changes to
policy_watcher_chromeos.cc/policy_service_watcher.cc (I am dissapointed that
Rietveld didn't recognize this as a rename and doesn't highlight that instance
methods are retained/unchanged).

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

That is what I meant.
OK, that's fair. Although you might get a similar enough result by initializing
a PolicyServiceWatcher with a MockConfigurationPolicyProvider.

+class PolicyServiceWatcher : public PolicyWatcher,
+                             public PolicyService::Observer {
+ public:
+  // Constructor for the case when |policy_service| is borrowed.

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

It'd be nice to document what |task_runner| is used for, might also adjust the
name to make this clear.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Good point.  

I tried adding comments to constructor declarations of PolicyServiceWatcher and
PolicyWatcher and to declaration of PolicyWatcher::Create and
PolicyWatcher::StartWatching.  I am not sure if I like the current state of
things (different behavior on ChromeOS, task_runner has different meaning in
PolicyWatcher's constructor and PolicyWatcher::Create), but this just retains
current behavior and I hope is ok to leave as-is for the current changelist.

I renamed task_runner to policy_service_task_runner inside
policy_service_watcher.cc, but kept the generic task_runner name in
policy_watcher.h/.cc and fake_policy_watcher.h/.cc and
policy_watcher_unittests.cc.

+  PolicyServiceWatcher(
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+      PolicyService* policy_service);
+
+  // Constructor for the case when |policy_service| is owned.
+  PolicyServiceWatcher(
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+      scoped_ptr<PolicyService> owned_policy_service,
+      scoped_ptr<ConfigurationPolicyProvider> owned_policy_provider,
+      scoped_ptr<SchemaRegistry> owned_schema_registry);
+
+  ~PolicyServiceWatcher() override;
+
+  // PolicyService::Observer interface.
+  void OnPolicyUpdated(const PolicyNamespace& ns,
+                       const PolicyMap& previous,
+                       const PolicyMap& current) override;
+
+  static PolicyNamespace GetPolicyNamespace() {
+    return PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());
+  }
+
+ protected:
+  // PolicyWatcher interface.
+  void Reload() override;
+  void StartWatchingInternal() override;
+  void StopWatchingInternal() override;
+
+ private:
+  PolicyService* policy_service_;
+
+  // Order of fields below is important to ensure destruction takes object
+  // dependencies into account:
+  // - |owned_policy_service_| uses |owned_policy_provider_|
+  // - |owned_policy_provider_| uses |owned_schema_registry_|
+  scoped_ptr<SchemaRegistry> owned_schema_registry_;
+  scoped_ptr<ConfigurationPolicyProvider> owned_policy_provider_;
+  scoped_ptr<PolicyService> owned_policy_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyServiceWatcher);
+};
+
+PolicyServiceWatcher::PolicyServiceWatcher(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    PolicyService* policy_service)
+    : PolicyWatcher(task_runner) {
+  policy_service_ = policy_service;
+}
+
+PolicyServiceWatcher::PolicyServiceWatcher(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    scoped_ptr<PolicyService> owned_policy_service,
+    scoped_ptr<ConfigurationPolicyProvider> owned_policy_provider,
+    scoped_ptr<SchemaRegistry> owned_schema_registry)
+    : PolicyWatcher(task_runner),
+      owned_schema_registry_(owned_schema_registry.Pass()),
+      owned_policy_provider_(owned_policy_provider.Pass()),
+      owned_policy_service_(owned_policy_service.Pass()) {
+  policy_service_ = owned_policy_service_.get();
+}
+
+PolicyServiceWatcher::~PolicyServiceWatcher() {
+  if (owned_policy_provider_) {
+    owned_policy_provider_->Shutdown();
+  }
+}
+
+void PolicyServiceWatcher::OnPolicyUpdated(const PolicyNamespace& ns,
+                                           const PolicyMap& previous,
+                                           const PolicyMap& current) {
+  scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
+  for (PolicyMap::const_iterator it = current.begin(); it != current.end();
+       it++) {
+    // TODO(lukasza): Use policy::Schema::Normalize for schema verification.
+    policy_dict->Set(it->first, it->second.value->DeepCopy());
+  }
+  UpdatePolicies(policy_dict.get());
+}
+
+void PolicyServiceWatcher::Reload() {

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

What are the desired semantics for Reload()? This just pulls the current policy
settings from |policy_service_|. If you want an actual refresh, you can call
PolicyService::RefreshPolicies.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Presence of Reload method is an artifact from the code that I am removing. 
PolicyWatcher base class would operate timers that periodically call Reload.  I
think this is not needed with PolicyService (as monitoring and reloading should
be internally handled by AsyncPolicyProvider, right?).  I think I'll remove
Reload method from the base class (not needed anymore after removal of
Schedule[Fallback]ReloadTask from PolicyWatcher) and inline the Reload method in
PolicyServiceWatcher into StartWatchingInternal.  I hope this makes sense.  I
guess we are already at a point where trying to preserve old code from
PolicyWatcherChromeOS (to aid diff readability by minimizing diffs) doesn't make
much sense due to the amount of new code (*).

And to directly answer your question - the only remaining call to Refresh in
PolicyServiceWatcher is done to fullfill this part of StartWatching contract:
  // This guarantees that the |policy_updated_callback| is called at least once
  // with the current policies.  After that, |policy_updated_callback| will be
  // called whenever a change to any policy is detected. It will then be called
  // only with the changed policies.
So - pulling the current policy settings (without an actual refresh) is fine I
think
(at the very least this preserves the current behavior of PolicyWatcherChromeOS
so at least I am not introducing new bugs).  

Hmmm... actually I think I should look into using IsInitializationComplete
and/or OnPolicyServiceInitialized.  If we call UpdatePolicies in uninitialized
state, then PolicyWatcher base class will just use default values which is
probably undesirable.  Let me fix this.  This probably didn't matter this much
for PolicyWatcherChromeOS, because policy was loaded long time ago in case of
running inside a browser process.

(*) But I am still removing more old/duplicated code than adding new code, so I
think this is still good.

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

OK, thanks for the explanation. Your conclusion regarding
IsInitialziationComplete/OnPolicyServiceInitialized is correct, the code should
wait for that to be signaled before relying on policy settings in general. Note
that for the platform providers (i.e. GPO, Mac managed preferences, Linux JSON
files) we do have special startup code that reads the policy synchronously on
startup (see AsyncPolicyProvider constructor).

[Łukasz Anforowicz, 2015/01/08 23:09:26]

Thanks for pointing this out.  This is useful to know.

+  PolicyNamespace ns = GetPolicyNamespace();
+  const PolicyMap& current = policy_service_->GetPolicies(ns);
+  OnPolicyUpdated(ns, current, current);
+};
+
+void PolicyServiceWatcher::StartWatchingInternal() {
+  policy_service_->AddObserver(POLICY_DOMAIN_CHROME, this);
+  Reload();
+};
+
+void PolicyServiceWatcher::StopWatchingInternal() {
+  policy_service_->RemoveObserver(POLICY_DOMAIN_CHROME, this);
+};
+
+#if defined(OS_MACOSX)
+
+// This function is based on GetManagedPolicyPath from
+// chrome/browser/policy/chrome_browser_policy_connector.cc

[Mattias Nissler (ping if slow), 2015/01/06 09:06:11]

How about moving this as a static helper to policy_loader_mac.h? It kinda
belongs there anyways and doing so would avoid duplication.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

It probably makes sense and I am trying to do this in the most recent snapshot. 
The only concern is that to make this reusable I cannot go through
src/chrome/common/chrome_paths.cc, which means yanking out DIR_MANAGED_PREFS 
(only user was chrome_browser_policy_connector.cc).  I think that even with this
concern the code looks better (both in chrome_browser_policy_connector.cc and in
policy_service_watcher.cc [no _mac.mm anymore to access NSLibraryDirectory
constant]).

+base::FilePath GetManagedPolicyPath() {
+  // This constructs the path to the plist file in which Mac OS X stores the
+  // managed preference for the application. This is undocumented and therefore
+  // fragile, but if it doesn't work out, AsyncPolicyLoader has a task that
+  // polls periodically in order to reload managed preferences later even if we
+  // missed the change.
+  base::FilePath path;
+  if (!GetManagedPrefsDir(&path))
+    return base::FilePath();
+
+  return path.Append("com.google.Chrome.plist");
+}
+
+#endif  // defined(OS_MACOSX)
+
+#if !defined(OS_CHROMEOS)
+
+scoped_ptr<PolicyServiceWatcher> CreateFromPolicyLoader(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    scoped_ptr<AsyncPolicyLoader> async_policy_loader) {
+  scoped_ptr<SchemaRegistry> schema_registry(new SchemaRegistry());
+  schema_registry->RegisterComponent(PolicyServiceWatcher::GetPolicyNamespace(),
+                                     Schema::Wrap(GetChromeSchemaData()));

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

Side note relating to the email thread: You're configuring the providers to load
the full set of chrome policy settings here, which is unnecessary (you actually
only want chromoting policies) and Chrome doesn't need the chromoting policy
bits (so no need to include the chromoting policies in the chrome policy
schema). Probably makes sense to clean this up eventually by creating separate
schemas.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Ack, although I am not sure about details of what is needed here.
A simple (but silly/wrong?) thing to do would be to copy contents of
GetChromeSchemaData
into a new Schema that only includes policies with "RemoteAccess" prefix in
their name.
I think the right thing to do might be to have a separate, Chromoting-specific
policy_template.json
(or is that an overkill that will require quite a bit of infrastructure for
having separate generated files as well?).

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

That works, but given that the policy_templates.json file carries meta data
already a better approach would be to put a flag into policy_templates.json
indicating whether a policy is for chromoting or not and include/exclude
policies in the policy schema based on that.
I can also see a simpler approach of just leaving the definitions within the
shared policy_templates.json and have the generate_policy_source.py script be
invoked with twice with different parameters to generate the different policy
schema data for chrome vs. chromoting cases. That might be good enough for now.

On the other hand, I like what you propose as that would move us closer to the
goal of having components/policy as a re-usable library. In that world, it
doesn't make sense for policy_templates.json to reside within the component
directory, but each embedder would keep its own policy definitions and the
component would just provide the plumbing in terms of build rules to turn the
file into a usable policy schema.

+
+  scoped_ptr<AsyncPolicyProvider> policy_provider(new AsyncPolicyProvider(
+      schema_registry.get(), async_policy_loader.Pass()));
+  policy_provider->Init(schema_registry.get());
+
+  PolicyServiceImpl::Providers providers;
+  providers.push_back(policy_provider.get());
+  scoped_ptr<PolicyService> policy_service(new PolicyServiceImpl(providers));
+
+  return make_scoped_ptr(
+      new PolicyServiceWatcher(task_runner, policy_service.Pass(),
+                               policy_provider.Pass(), schema_registry.Pass()));
+}
+
+#endif
+
+}  // anonymous namespace
+
+// This function is based on CreatePlatformProvider from
+// src/chrome/browser/policy/chrome_browser_policy_connector.cc.

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

nit: This comment will get less and less useful overtime as the code diverges.
I'm fine with just dropping it.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Done.

+scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
+    policy::PolicyService* policy_service,
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
+#if defined(OS_CHROMEOS)
+  DCHECK(policy_service != nullptr);
+  return make_scoped_ptr(new PolicyServiceWatcher(
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::UI),
+      policy_service));
+#elif defined(OS_WIN)
+  DCHECK(policy_service == nullptr);
+  static const wchar_t kRegistryKey[] = L"SOFTWARE\\Policies\\Google\\Chrome";
+  return CreateFromPolicyLoader(
+      task_runner, scoped_ptr<AsyncPolicyLoader>(
+                       PolicyLoaderWin::Create(task_runner, kRegistryKey)));
+#elif defined(OS_MACOSX)
+  DCHECK(policy_service == nullptr);
+  return CreateFromPolicyLoader(
+      task_runner, make_scoped_ptr(new PolicyLoaderMac(
+                       task_runner, GetManagedPolicyPath(),
+                       new MacPreferences(), CFSTR("com.google.Chrome"))));
+#elif defined(OS_POSIX) && !defined(OS_ANDROID)
+  DCHECK(policy_service == nullptr);
+  static const base::FilePath::CharType kPolicyDir[] =
+      // Always read the Chrome policies (even on Chromium) so that policy
+      // enforcement can't be bypassed by running Chromium.

[Mattias Nissler (ping if slow), 2015/01/06 09:06:11]

nit: This comment should go before line 207.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Done (I was just copying the formatting from policy_watcher_linux.cc :-P )

+      FILE_PATH_LITERAL("/etc/opt/chrome/policies");

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

I don't think the reasoning in the comment makes sense. If you're in a position
to run Chromium, you can as well run a modified chromium binary that reads a
different directory.

[Łukasz Anforowicz, 2015/01/07 17:54:14]

Ack and we should definitely discuss this (maybe you can start a separate email
thread [with me and sergeyu@ and wez@] please).  OTOH, I am just preserving the
current behavior and I think I don't want to touch this in the current
changelist (where I am trying to replace a bunch of code while preserving the
current behavior).

Also - if the path is different for Chrome and Chromium, then maybe we can also
yank out RemoteAccessHostDebugOverridePolicies policy as well.

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

OK, fair enough.
That sounds like the right thing to do, I wasn't aware this kludge existed :)

+  return CreateFromPolicyLoader(
+      task_runner,
+      make_scoped_ptr(new ConfigDirPolicyLoader(
+          task_runner, base::FilePath(kPolicyDir), POLICY_SCOPE_MACHINE)));
+#else
+#error OS that is not yet supported by PolicyWatcher code.
+#endif
+}
+
+}  // namespace policy_hack
+}  // namespace remoting