Merged PolicyServiceWatcher into PolicyWatcher.

remoting/host/policy_hack/policy_watcher.cc

 // Copyright (c) 2012 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.
 
 // Most of this code is copied from:
 //   src/chrome/browser/policy/asynchronous_policy_loader.{h,cc}
 
 #include "remoting/host/policy_hack/policy_watcher.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_path.h"
 #include "base/location.h"
-#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/values.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_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/dns_blackhole_checker.h"
 
 #if !defined(NDEBUG)
 #include "base/json/json_reader.h"
 #endif
 
+#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;

[Sergey Ulanov, 2015/01/26 18:20:01]

style guide prohibits "using namespace":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

But you can replace it with "using policy::key;". 

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Oops.  Fixed.  FWIW, I was just retaining it from policy_watcher_chromeos.cc.

[Sergey Ulanov, 2015/01/28 07:07:53]

If I remember correctly the style guide did allow it in the past.  Don't know
when that section was changed.

+
 namespace remoting {
 namespace policy_hack {
 
 namespace {
 
 // Copies all policy values from one dictionary to another, using values from
 // |default| if they are not set in |from|, or values from |bad_type_values| if
 // the value in |from| has the wrong type.
 scoped_ptr<base::DictionaryValue> CopyGoodValuesAndAddDefaults(
     const base::DictionaryValue* from,
@@ skipping 14 matching lines @@
     if (!value->IsType(i.value().GetType())) {
       CHECK(bad_type_values->Get(i.key(), &value));
     }
 
     to->Set(i.key(), value->DeepCopy());
   }
 
 #if !defined(NDEBUG)
   // Replace values with those specified in DebugOverridePolicies, if present.
   std::string policy_overrides;
-  if (from->GetString(policy::key::kRemoteAccessHostDebugOverridePolicies,
+  if (from->GetString(key::kRemoteAccessHostDebugOverridePolicies,
                       &policy_overrides)) {
     scoped_ptr<base::Value> value(base::JSONReader::Read(policy_overrides));
     const base::DictionaryValue* override_values;
     if (value && value->GetAsDictionary(&override_values)) {
       to->MergeDictionary(override_values);
     }
   }
 #endif  // defined(NDEBUG)
 
   return to.Pass();
 }
 
+PolicyNamespace GetPolicyNamespace() {
+  return PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());
+}
+

[Łukasz Anforowicz, 2015/01/26 17:45:11]

GetPolicyNamespace is copy&pasted from policy_service_watcher.cc

 }  // namespace
 
-PolicyWatcher::PolicyWatcher(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
-    : task_runner_(task_runner),
+PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&

[Sergey Ulanov, 2015/01/26 18:20:01]

nit: Move this constructructor next to the other constructors below?

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Done.

+                                 policy_service_task_runner)
+    : policy_service_task_runner_(policy_service_task_runner),
       transient_policy_error_retry_counter_(0),
       old_policies_(new base::DictionaryValue()),
-      default_values_(new base::DictionaryValue()),
-      weak_factory_(this) {
+      default_values_(new base::DictionaryValue()) {
   // Initialize the default values for each policy.
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
-                              true);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostRequireTwoFactor,
-                              false);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostRequireCurtain,
-                              false);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostMatchUsername,
-                              false);
-  default_values_->SetString(policy::key::kRemoteAccessHostDomain,
-                             std::string());
-  default_values_->SetString(policy::key::kRemoteAccessHostTalkGadgetPrefix,
+  default_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostRequireTwoFactor, false);
+  default_values_->SetBoolean(key::kRemoteAccessHostRequireCurtain, false);
+  default_values_->SetBoolean(key::kRemoteAccessHostMatchUsername, false);
+  default_values_->SetString(key::kRemoteAccessHostDomain, std::string());
+  default_values_->SetString(key::kRemoteAccessHostTalkGadgetPrefix,
                              kDefaultHostTalkGadgetPrefix);
-  default_values_->SetString(policy::key::kRemoteAccessHostTokenUrl,
-                             std::string());
-  default_values_->SetString(policy::key::kRemoteAccessHostTokenValidationUrl,
+  default_values_->SetString(key::kRemoteAccessHostTokenUrl, std::string());
+  default_values_->SetString(key::kRemoteAccessHostTokenValidationUrl,
                              std::string());
   default_values_->SetString(
-      policy::key::kRemoteAccessHostTokenValidationCertificateIssuer,
-      std::string());
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostAllowClientPairing,
+      key::kRemoteAccessHostTokenValidationCertificateIssuer, std::string());
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowClientPairing, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowGnubbyAuth, true);
+  default_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                               true);
-  default_values_->SetBoolean(policy::key::kRemoteAccessHostAllowGnubbyAuth,
-                              true);
-  default_values_->SetBoolean(
-      policy::key::kRemoteAccessHostAllowRelayedConnection, true);
-  default_values_->SetString(policy::key::kRemoteAccessHostUdpPortRange, "");
+  default_values_->SetString(key::kRemoteAccessHostUdpPortRange, "");
 #if !defined(NDEBUG)
-  default_values_->SetString(
-      policy::key::kRemoteAccessHostDebugOverridePolicies, std::string());
+  default_values_->SetString(key::kRemoteAccessHostDebugOverridePolicies,
+                             std::string());
 #endif
 
   // Initialize the fall-back values to use for unreadable policies.
   // For most policies these match the defaults.
   bad_type_values_.reset(default_values_->DeepCopy());
-  bad_type_values_->SetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
+  bad_type_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, false);
+  bad_type_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                                false);
-  bad_type_values_->SetBoolean(
-      policy::key::kRemoteAccessHostAllowRelayedConnection, false);
-}
-
-PolicyWatcher::~PolicyWatcher() {
 }
 
 void PolicyWatcher::StartWatching(
     const PolicyUpdatedCallback& policy_updated_callback,
     const PolicyErrorCallback& policy_error_callback) {
   if (!OnPolicyWatcherThread()) {
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&PolicyWatcher::StartWatching,
-                                      base::Unretained(this),
-                                      policy_updated_callback,
-                                      policy_error_callback));
+    policy_service_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&PolicyWatcher::StartWatching, base::Unretained(this),
+                   policy_updated_callback, policy_error_callback));
     return;
   }
 
   policy_updated_callback_ = policy_updated_callback;
   policy_error_callback_ = policy_error_callback;
   StartWatchingInternal();
 }
 
 void PolicyWatcher::StopWatching(const base::Closure& stopped_callback) {
-  task_runner_->PostTaskAndReply(
+  policy_service_task_runner_->PostTaskAndReply(
       FROM_HERE, base::Bind(&PolicyWatcher::StopWatchingOnPolicyWatcherThread,
                             base::Unretained(this)),
       stopped_callback);
 }
 
 void PolicyWatcher::StopWatchingOnPolicyWatcherThread() {
   StopWatchingInternal();
-  weak_factory_.InvalidateWeakPtrs();
   policy_updated_callback_.Reset();
   policy_error_callback_.Reset();
 }
 
-const base::DictionaryValue& PolicyWatcher::Defaults() const {
-  return *default_values_;
-}
-
 bool PolicyWatcher::OnPolicyWatcherThread() const {
-  return task_runner_->BelongsToCurrentThread();
+  return policy_service_task_runner_->BelongsToCurrentThread();
 }
 
 void PolicyWatcher::UpdatePolicies(
     const base::DictionaryValue* new_policies_raw) {
   DCHECK(OnPolicyWatcherThread());
 
   transient_policy_error_retry_counter_ = 0;
 
   // Use default values for any missing policies.
   scoped_ptr<base::DictionaryValue> new_policies =
@@ skipping 28 matching lines @@
 }
 
 void PolicyWatcher::SignalTransientPolicyError() {
   const int kMaxRetryCount = 5;
   transient_policy_error_retry_counter_ += 1;
   if (transient_policy_error_retry_counter_ >= kMaxRetryCount) {
     SignalPolicyError();
   }
 }
 
+PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
+                                 policy_service_task_runner,
+                             PolicyService* policy_service)
+    : PolicyWatcher(policy_service_task_runner) {
+  policy_service_ = policy_service;
+}
+
+PolicyWatcher::PolicyWatcher(

[Sergey Ulanov, 2015/01/26 18:20:01]

This class has 3 private constructors. Consider replacing them with a single
constructor that takes all the arguments and pass nullptr for some of them. Even
if you prefer to keep multiple constructors you don't need all 3. This
constructor can call the one above, so you can merge the first two constructors.

[Łukasz Anforowicz, 2015/01/26 19:24:20]

I merged into a single constructor.

+    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();
+}

[Łukasz Anforowicz, 2015/01/26 17:45:11]

Everything below is copy&pasted from policy_service_watcher.cc except:
1. s/PolicyServiceWatcher/PolicyWatcher/g
2. minor changes to the form used by constructors (in order to reuse a "base"
constructor, I had to move field initialization of owner_policy_provider [etc]
into field assignment in the body of the constructor)

+
+PolicyWatcher::~PolicyWatcher() {
+  if (owned_policy_provider_) {
+    owned_policy_provider_->Shutdown();
+  }
+}
+
+void PolicyWatcher::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++) {

[Sergey Ulanov, 2015/01/26 18:20:01]

s/it++/++it/
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Done.

I also looked into whether I can use "auto" here, but there is no way to
explicitly say that I want a "const" iterator, so I decided not to do it. 
("auto" would still give a "const" iterator because "current" is "const", but
this seems like too much magic the reader of the code has to be aware of... :-/
).

[Sergey Ulanov, 2015/01/28 07:07:53]

I don't think it's essential that the iterator is const here, so if auto works
here I think it would make this code more readable. Up to you.

+    // TODO(lukasza): Use policy::Schema::Normalize() for schema verification.
+    policy_dict->Set(it->first, it->second.value->DeepCopy());
+  }
+  UpdatePolicies(policy_dict.get());
+}
+
+void PolicyWatcher::OnPolicyServiceInitialized(PolicyDomain domain) {
+  PolicyNamespace ns = GetPolicyNamespace();
+  const PolicyMap& current = policy_service_->GetPolicies(ns);
+  OnPolicyUpdated(ns, current, current);
+}
+
+void PolicyWatcher::StartWatchingInternal() {
+  // Listen for future policy changes.
+  policy_service_->AddObserver(POLICY_DOMAIN_CHROME, this);
+
+  // Process current policy state.
+  if (policy_service_->IsInitializationComplete(POLICY_DOMAIN_CHROME)) {
+    OnPolicyServiceInitialized(POLICY_DOMAIN_CHROME);
+  }
+}
+
+void PolicyWatcher::StopWatchingInternal() {
+  policy_service_->RemoveObserver(POLICY_DOMAIN_CHROME, this);
+}
+
+scoped_ptr<PolicyWatcher> PolicyWatcher::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(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 PolicyWatcher(policy_service_task_runner, policy_service.Pass(),
+                        policy_provider.Pass(), schema_registry.Pass()));
+}
+
+scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
+    policy::PolicyService* policy_service,
+    const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner) {
+#if defined(OS_CHROMEOS)
+  DCHECK(policy_service);
+  return make_scoped_ptr(
+      new PolicyWatcher(content::BrowserThread::GetMessageLoopProxyForThread(
+                            content::BrowserThread::UI),
+                        policy_service));
+#elif defined(OS_WIN)
+  DCHECK(!policy_service);
+  static const wchar_t kRegistryKey[] = L"SOFTWARE\\Policies\\Google\\Chrome";
+  return PolicyWatcher::CreateFromPolicyLoader(
+      network_task_runner,
+      PolicyLoaderWin::Create(network_task_runner, kRegistryKey));
+#elif defined(OS_MACOSX)
+  CFStringRef bundle_id = CFSTR("com.google.Chrome");
+  DCHECK(!policy_service);
+  return PolicyWatcher::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);
+  // Always read the Chrome policies (even on Chromium) so that policy

[Sergey Ulanov, 2015/01/26 18:20:01]

This comment applies to Win and OSX too. Move it out of the ifdef?

[Łukasz Anforowicz, 2015/01/26 19:24:20]

I moved it slightly higher + added a note that the comment applies to all 3
cases.  Note that this behavior is different from behavior of the browser and
I'll probably have to follow-up on this later on (i.e. Mattias was surprised by
this behavior here).

+  // enforcement can't be bypassed by running Chromium.
+  static const base::FilePath::CharType kPolicyDir[] =
+      FILE_PATH_LITERAL("/etc/opt/chrome/policies");
+  return PolicyWatcher::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
 }  // namespace remoting