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_watcher_chromeos.cc b/remoting/host/policy_hack/policy_service_watcher.cc
similarity index 17%
rename from remoting/host/policy_hack/policy_watcher_chromeos.cc
rename to remoting/host/policy_hack/policy_service_watcher.cc
index 39d1d97bf8bd145766b7843ba82cff181092776f..73e829f82f56d002661d2bd5ef7b45b4f16e1023 100644
--- a/remoting/host/policy_hack/policy_watcher_chromeos.cc
+++ b/remoting/host/policy_hack/policy_service_watcher.cc
@@ -2,88 +2,230 @@
 // 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"
 
-#include "components/policy/core/common/policy_service.h"
+#if defined(OS_CHROMEOS)
 #include "content/public/browser/browser_thread.h"
-#include "remoting/base/auto_thread_task_runner.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 {
 
-class PolicyWatcherChromeOS : public PolicyWatcher,
-                              public PolicyService::Observer {
+// PolicyServiceWatcher is a concrete implementation of PolicyWatcher that wraps
+// an instance of PolicyService.
+class PolicyServiceWatcher : public PolicyWatcher,

[Sergey Ulanov, 2015/01/13 01:17:04]

Why do we need this class? Can we just move all code to PolicyWatcher?

[Łukasz Anforowicz, 2015/01/13 18:28:29]

Hmmm... Mattias pointed this out as well.  

I left a separate PolicyServiceWatcher mainly as an artifact of how I arrived at
the changes (by heavily refactoring the old PolicyWatcherChromeOS which was in a
separate file/class).

Seeing how 2 people are asking me to do this, I set out to make this change, but
backed out and want to still do it in a separate changelist:
- This is a refactoring that is not needed for the core of this change (get rid
of our linux/win/mac code and use PolicyService instead).
- Unless done together with removal of FakePolicyWatcher (replacing with [new]
MockPolicyLoader or [existing in components/policy] MockPolicyProvider) this
will be very incomplete refactoring - i.e. still leaving some members of
PolicyWatcher as virtual (for FakePolicyWatcher to override) and having a weird
double-hop in StopWatchingOnPolicyWatcherThread.
- It would go against SRP - PolicyWatcher would have 2 responsibilities: 1)
filling in default values and 2) talking to PolicyService.  We should see if #1
(default values) can also be refactored into components/policy.

FWIW, I've added a TODO in policy_service_watcher.cc :-/

What do you think?

+                             public PolicyService::Observer {
  public:
-  PolicyWatcherChromeOS(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-                        PolicyService* policy_service);
-
-  ~PolicyWatcherChromeOS() override;
+  // Constructor for the case when |policy_service| is borrowed.
+  //
+  // |policy_service_task_runner| is the task runner where it is safe
+  // to call |policy_service| methods and where we expect to get callbacks
+  // from |policy_service|.
+  PolicyServiceWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
+                           policy_service_task_runner,
+                       PolicyService* policy_service);
+
+  // Constructor for the case when |policy_service| is owned (and uses also
+  // owned |owned_policy_provider| and |owned_schema_registry|.
+  //
+  // |policy_service_task_runner| is the task runner where it is safe
+  // to call |policy_service| methods and where we expect to get callbacks
+  // from |policy_service|.
+  PolicyServiceWatcher(
+      const scoped_refptr<base::SingleThreadTaskRunner>&
+          policy_service_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;
+  void OnPolicyServiceInitialized(PolicyDomain domain) override;
+
+  static PolicyNamespace GetPolicyNamespace() {
+    return PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());

[Sergey Ulanov, 2015/01/13 01:17:03]

Move definition of this method out of the class definition.

[Łukasz Anforowicz, 2015/01/13 18:28:29]

Done.

+  }
 
  protected:
   // PolicyWatcher interface.

[Sergey Ulanov, 2015/01/13 01:17:04]

s/interface/overrides/. PolicyWatcher isn't an interface.

[Łukasz Anforowicz, 2015/01/13 18:28:29]

Fair, done. (but note that I just retained this comment from
policy_watcher_chromeos.cc :-P ).

-  void Reload() override;
   void StartWatchingInternal() override;
   void StopWatchingInternal() override;
 
  private:
   PolicyService* policy_service_;
 
-  DISALLOW_COPY_AND_ASSIGN(PolicyWatcherChromeOS);
+  // 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);
 };
 
-PolicyWatcherChromeOS::PolicyWatcherChromeOS(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+PolicyServiceWatcher::PolicyServiceWatcher(
+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        policy_service_task_runner,
     PolicyService* policy_service)
-    : PolicyWatcher(task_runner), policy_service_(policy_service) {
+    : PolicyWatcher(policy_service_task_runner) {
+  policy_service_ = policy_service;
 }
 
-PolicyWatcherChromeOS::~PolicyWatcherChromeOS() {
+PolicyServiceWatcher::PolicyServiceWatcher(
+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        policy_service_task_runner,
+    scoped_ptr<PolicyService> owned_policy_service,
+    scoped_ptr<ConfigurationPolicyProvider> owned_policy_provider,
+    scoped_ptr<SchemaRegistry> owned_schema_registry)
+    : PolicyWatcher(policy_service_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();
 }
 
-void PolicyWatcherChromeOS::OnPolicyUpdated(const PolicyNamespace& ns,
-                                            const PolicyMap& previous,
-                                            const PolicyMap& current) {
+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.

[Sergey Ulanov, 2015/01/13 01:17:03]

nit: Use () after function names in comments, e.g. "Use
policy::Schema::Normalize() for ..."

[Łukasz Anforowicz, 2015/01/13 18:28:29]

Done.

     policy_dict->Set(it->first, it->second.value->DeepCopy());
   }
   UpdatePolicies(policy_dict.get());
 }
 
-void PolicyWatcherChromeOS::Reload() {
-  PolicyNamespace ns(POLICY_DOMAIN_CHROME, std::string());
+void PolicyServiceWatcher::OnPolicyServiceInitialized(PolicyDomain domain) {
+  PolicyNamespace ns = GetPolicyNamespace();
   const PolicyMap& current = policy_service_->GetPolicies(ns);
   OnPolicyUpdated(ns, current, current);
-};
+}
 
-void PolicyWatcherChromeOS::StartWatchingInternal() {
+void PolicyServiceWatcher::StartWatchingInternal() {
+  // Listen for future policy changes.
   policy_service_->AddObserver(POLICY_DOMAIN_CHROME, this);
-  Reload();
-};
 
-void PolicyWatcherChromeOS::StopWatchingInternal() {
+  // Process current policy state.
+  if (policy_service_->IsInitializationComplete(POLICY_DOMAIN_CHROME)) {
+    OnPolicyServiceInitialized(POLICY_DOMAIN_CHROME);
+  }
+}
+
+void PolicyServiceWatcher::StopWatchingInternal() {
   policy_service_->RemoveObserver(POLICY_DOMAIN_CHROME, this);
-};
+}
+
+#if !defined(OS_CHROMEOS)
+
+// Creates PolicyServiceWatcher that wraps the owned |async_policy_loader|
+// with an appropriate PolicySchema.
+//
+// |policy_service_task_runner| is passed through to the constructor
+// of PolicyServiceWatcher.
+scoped_ptr<PolicyServiceWatcher> CreateFromPolicyLoader(
+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        policy_service_task_runner,
+    scoped_ptr<AsyncPolicyLoader> async_policy_loader) {
+  // TODO(lukasza): Schema below should ideally only cover Chromoting-specific
+  // policies (expecting perf and maintanability improvement, but no functional
+  // impact).
+  Schema schema = Schema::Wrap(GetChromeSchemaData());
+
+  scoped_ptr<SchemaRegistry> schema_registry(new SchemaRegistry());
+  schema_registry->RegisterComponent(PolicyServiceWatcher::GetPolicyNamespace(),
+                                     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(
+      policy_service_task_runner, policy_service.Pass(), policy_provider.Pass(),
+      schema_registry.Pass()));
+}
+
+#endif
 
-}  // namespace
+}  // anonymous namespace
 
 scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
     policy::PolicyService* policy_service,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  return make_scoped_ptr(new PolicyWatcherChromeOS(
+    const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner) {
+#if defined(OS_CHROMEOS)
+  DCHECK(policy_service != nullptr);

[Sergey Ulanov, 2015/01/13 01:17:04]

nit: It's more common omit the nullptr comparison and instead just write 
DCHECK(!policy_service) or DCHECK(policy_service).

[Łukasz Anforowicz, 2015/01/13 18:28:29]

Done.

+  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(
+      network_task_runner,
+      scoped_ptr<AsyncPolicyLoader>(

[Sergey Ulanov, 2015/01/13 01:17:04]

nit: make_scoped_ptr()

[Łukasz Anforowicz, 2015/01/13 18:28:28]

Thanks for pointing this out.  I couldn't make it compile with make_scoped_ptr,
because I stupidly tried to wrap make_scoped_ptr around PolicyLoaderWin::Create
which already returns a scoped_ptr.  This should be fixed now.

+          PolicyLoaderWin::Create(network_task_runner, kRegistryKey)));
+#elif defined(OS_MACOSX)
+  CFStringRef bundle_id = CFSTR("com.google.Chrome");
+  DCHECK(policy_service == nullptr);
+  return CreateFromPolicyLoader(
+      network_task_runner,
+      make_scoped_ptr(new PolicyLoaderMac(
+          network_task_runner,
+          policy::PolicyLoaderMac::GetManagedPolicyPath(bundle_id),
+          new MacPreferences(), bundle_id)));
+#elif defined(OS_POSIX) && !defined(OS_ANDROID)
+  DCHECK(policy_service == nullptr);
+  // Always read the Chrome policies (even on Chromium) so that policy
+  // enforcement can't be bypassed by running Chromium.
+  static const base::FilePath::CharType kPolicyDir[] =
+      FILE_PATH_LITERAL("/etc/opt/chrome/policies");
+  return CreateFromPolicyLoader(
+      network_task_runner, make_scoped_ptr(new ConfigDirPolicyLoader(
+                               network_task_runner, base::FilePath(kPolicyDir),
+                               POLICY_SCOPE_MACHINE)));
+#else
+#error OS that is not yet supported by PolicyWatcher code.
+#endif
 }
 
 }  // namespace policy_hack