Reporting a policy error after detecting mistyped policies.

remoting/host/policy_watcher.cc

 // Copyright 2015 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_watcher.h"
 
 #include "base/bind.h"
@@ skipping 24 matching lines @@
 #include "components/policy/core/common/config_dir_policy_loader.h"
 #endif
 
 namespace remoting {
 
 namespace key = ::policy::key;
 
 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(
+// |default_values| if they are not set in |from|.
+scoped_ptr<base::DictionaryValue> CopyValuesAndAddDefaults(
     const base::DictionaryValue* from,
-    const base::DictionaryValue* default_values,
-    const base::DictionaryValue* bad_type_values) {
+    const base::DictionaryValue* default_values) {
   scoped_ptr<base::DictionaryValue> to(default_values->DeepCopy());
   for (base::DictionaryValue::Iterator i(*default_values); !i.IsAtEnd();
        i.Advance()) {
     const base::Value* value = nullptr;
 
     // If the policy isn't in |from|, use the default.
     if (!from->Get(i.key(), &value)) {
       continue;
     }
 
-    // If the policy is the wrong type, use the value from |bad_type_values|.
-    if (!value->IsType(i.value().GetType())) {
-      CHECK(bad_type_values->Get(i.key(), &value));
-    }
-
+    CHECK(value->IsType(i.value().GetType()));

[Sergey Ulanov, 2015/02/12 19:22:58]

Does this need to be a CHECK() instead of DCHECK()?
Do we have unittests to verify that |default_values| matches schema?

[Łukasz Anforowicz, 2015/02/12 20:34:13]

Good question.  I was wondering about it myself.

Arguments for leaving this as a CHECK:
- It was a CHECK before, even though in theory bad_type_values_ should cover all
the policies.
- Continuing with bad policies is so bad, that it should be fatal to the
process.

Arguments for changing to DCHECK:
- This should already be enforced by other parts of the code + should be already
verified via unit tests (well... after I add more tests as you suggested below).
We have PolicySchemaAndPolicyWatcherShouldBeInSync unit test which landed in
crrev.com/903883005.  Currently this only verifies that |default_values| matches
the schema wrt policy names.  I should have added type match verfication as
well.  On it! :-)

     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(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();
 }
 
 policy::PolicyNamespace GetPolicyNamespace() {
   return policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME, std::string());
 }
 
+scoped_ptr<policy::SchemaRegistry> CreateSchemaRegistry() {
+  // TODO(lukasza): Schema below should ideally only cover Chromoting-specific
+  // policies (expecting perf and maintanability improvement, but no functional
+  // impact).
+  policy::Schema schema = policy::Schema::Wrap(policy::GetChromeSchemaData());
+
+  scoped_ptr<policy::SchemaRegistry> schema_registry(
+      new policy::SchemaRegistry());
+  schema_registry->RegisterComponent(GetPolicyNamespace(), schema);
+  return schema_registry.Pass();
+}
+
 }  // namespace
 
 void PolicyWatcher::StartWatching(
     const PolicyUpdatedCallback& policy_updated_callback,
     const PolicyErrorCallback& policy_error_callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(!policy_updated_callback.is_null());
   DCHECK(!policy_error_callback.is_null());
   DCHECK(policy_updated_callback_.is_null());
 
   policy_updated_callback_ = policy_updated_callback;
   policy_error_callback_ = policy_error_callback;
 
   // Listen for future policy changes.
   policy_service_->AddObserver(policy::POLICY_DOMAIN_CHROME, this);
 
   // Process current policy state.
   if (policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) {
     OnPolicyServiceInitialized(policy::POLICY_DOMAIN_CHROME);
   }
 }
 
 void PolicyWatcher::UpdatePolicies(
     const base::DictionaryValue* new_policies_raw) {
   DCHECK(CalledOnValidThread());
 
-  transient_policy_error_retry_counter_ = 0;
-
   // Use default values for any missing policies.
-  scoped_ptr<base::DictionaryValue> new_policies = CopyGoodValuesAndAddDefaults(
-      new_policies_raw, default_values_.get(), bad_type_values_.get());
+  scoped_ptr<base::DictionaryValue> new_policies =
+      CopyValuesAndAddDefaults(new_policies_raw, default_values_.get());
 
   // Find the changed policies.
   scoped_ptr<base::DictionaryValue> changed_policies(
       new base::DictionaryValue());
   base::DictionaryValue::Iterator iter(*new_policies);
   while (!iter.IsAtEnd()) {
     base::Value* old_policy;
     if (!(old_policies_->Get(iter.key(), &old_policy) &&
           old_policy->Equals(&iter.value()))) {
       changed_policies->Set(iter.key(), iter.value().DeepCopy());
     }
     iter.Advance();
   }
 
   // Save the new policies.
   old_policies_.swap(new_policies);
 
   // Notify our client of the changed policies.
   if (!changed_policies->empty()) {
     policy_updated_callback_.Run(changed_policies.Pass());
   }
 }
 
 void PolicyWatcher::SignalPolicyError() {
-  transient_policy_error_retry_counter_ = 0;
   policy_error_callback_.Run();
 }
 
-void PolicyWatcher::SignalTransientPolicyError() {
-  const int kMaxRetryCount = 5;
-  transient_policy_error_retry_counter_ += 1;
-  if (transient_policy_error_retry_counter_ >= kMaxRetryCount) {
-    SignalPolicyError();
-  }
-}
-
 PolicyWatcher::PolicyWatcher(
     policy::PolicyService* policy_service,
     scoped_ptr<policy::PolicyService> owned_policy_service,
     scoped_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider,
     scoped_ptr<policy::SchemaRegistry> owned_schema_registry)
-    : transient_policy_error_retry_counter_(0),
-      old_policies_(new base::DictionaryValue()),
+    : old_policies_(new base::DictionaryValue()),
       default_values_(new base::DictionaryValue()),
       policy_service_(policy_service),
       owned_schema_registry_(owned_schema_registry.Pass()),
       owned_policy_provider_(owned_policy_provider.Pass()),
       owned_policy_service_(owned_policy_service.Pass()) {
+  DCHECK(policy_service_);
+  DCHECK(owned_schema_registry_);
+
   // Initialize the default values for each policy.
   default_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, true);
   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(key::kRemoteAccessHostTokenUrl, std::string());
   default_values_->SetString(key::kRemoteAccessHostTokenValidationUrl,
                              std::string());
   default_values_->SetString(
       key::kRemoteAccessHostTokenValidationCertificateIssuer, std::string());
   default_values_->SetBoolean(key::kRemoteAccessHostAllowClientPairing, true);
   default_values_->SetBoolean(key::kRemoteAccessHostAllowGnubbyAuth, true);
   default_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                               true);
   default_values_->SetString(key::kRemoteAccessHostUdpPortRange, "");
 #if !defined(NDEBUG)
   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(key::kRemoteAccessHostFirewallTraversal, false);
-  bad_type_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
-                               false);
 }
 
 PolicyWatcher::~PolicyWatcher() {
   // Stop observing |policy_service_| if StartWatching() has been called.
   if (!policy_updated_callback_.is_null()) {
     policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
   }
 
   if (owned_policy_provider_) {
     owned_policy_provider_->Shutdown();
   }
 }
 
+const policy::Schema* PolicyWatcher::GetPolicySchema() const {
+  policy::PolicyNamespace ns = GetPolicyNamespace();
+
+  const scoped_refptr<policy::SchemaMap>& schema_map =
+      owned_schema_registry_->schema_map();
+  const policy::Schema* schema = schema_map->GetSchema(ns);

[Sergey Ulanov, 2015/02/12 19:22:59]

nit: You can write this whole function in one line:

  return owned_schema_registry_->schema_map()->GetSchema(GetPolicyNamespace());

Even if you'd prefer keeping |schema_map|, you don't need |schema|

[Łukasz Anforowicz, 2015/02/12 20:34:13]

Thanks.  I don't know why I did it this way...

+
+  return schema;
+}
+
 void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
                                     const policy::PolicyMap& previous,
                                     const policy::PolicyMap& current) {
+  const std::string policy_name_prefix = "RemoteAccessHost";

[Sergey Ulanov, 2015/02/12 19:22:59]

This doesn't need to be std::string. Style guide asks to use kConstName for
consts.
  const char kPolicyNamePrefix[] = "RemoteAccessHost";
  

[Łukasz Anforowicz, 2015/02/12 20:34:13]

Done.

BTW: I initially started replying that your suggestion would repeatedly
construct std::string inside the loop, but didn't notice that std::string::find
has an overload that accepts const char* arguments.  And maybe the repeated
std::string construction would not be that bad either in this particular case
(AFAIK std::string doesn't do heap allocation for strings smaller than 32
bytes).

   scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
   for (auto 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());
+    const std::string& key = it->first;
+    const base::Value* value = it->second.value;
+
+    // Copying only Chromoting-specific policies helps avoid false alarms
+    // raised by Schema::Normalize below (such alarms shutdown the host).
+    // TODO(lukasza): Removing this somewhat brittle filtering will be possible
+    //                after having separate, Chromoting-specific schema.
+    if (key.find(policy_name_prefix) != std::string::npos) {
+      policy_dict->Set(key, value->DeepCopy());
+    }
   }
-  UpdatePolicies(policy_dict.get());
+
+  // Allowing unrecognized policy names allows presence of
+  // 1) comments,

[Sergey Ulanov, 2015/02/12 19:22:58]

Comments should be dropped by the parser, no?

[Łukasz Anforowicz, 2015/02/12 20:34:13]

By "comments" I meant JSON entries like these:

{
  "_comment": "$Id:
//depot/google3/googledata/corp/puppet/goobuntu/common/modules/chrome/files/chromoting.json#2
$",
  "RemoteAccessClientFirewallTraversal": false,
...
}

I've added the example to the comment.

+  // 2) policies intended for future/newer versions of the host,
+  // 3) policies not supported on all OS-s (i.e. RemoteAccessHostMatchUsername
+  //    is not supported on Windows and therefore policy_templates.json omits
+  //    schema for this policy on this particular platform).
+  auto strategy = policy::SCHEMA_ALLOW_UNKNOWN_TOPLEVEL;
+
+  std::string path;
+  std::string error;
+  bool changed;
+  const policy::Schema* schema = GetPolicySchema();
+  if (schema->Normalize(policy_dict.get(), strategy, &path, &error, &changed)) {
+    if (changed) {
+      LOG(WARNING) << "Unknown (unrecognized or unsupported) policy: " << path
+                   << ": " << error;
+    }
+    UpdatePolicies(policy_dict.get());
+  } else {
+    LOG(ERROR) << "Invalid policy contents: " << path << ": " << error;
+    SignalPolicyError();
+  }
 }
 
 void PolicyWatcher::OnPolicyServiceInitialized(policy::PolicyDomain domain) {
   policy::PolicyNamespace ns = GetPolicyNamespace();
   const policy::PolicyMap& current = policy_service_->GetPolicies(ns);
   OnPolicyUpdated(ns, current, current);
 }
 
 scoped_ptr<PolicyWatcher> PolicyWatcher::CreateFromPolicyLoader(
     scoped_ptr<policy::AsyncPolicyLoader> async_policy_loader) {
-  // TODO(lukasza): Schema below should ideally only cover Chromoting-specific
-  // policies (expecting perf and maintanability improvement, but no functional
-  // impact).
-  policy::Schema schema = policy::Schema::Wrap(policy::GetChromeSchemaData());
-
-  scoped_ptr<policy::SchemaRegistry> schema_registry(
-      new policy::SchemaRegistry());
-  schema_registry->RegisterComponent(GetPolicyNamespace(), schema);
-
+  scoped_ptr<policy::SchemaRegistry> schema_registry = CreateSchemaRegistry();
   scoped_ptr<policy::AsyncPolicyProvider> policy_provider(
       new policy::AsyncPolicyProvider(schema_registry.get(),
                                       async_policy_loader.Pass()));
   policy_provider->Init(schema_registry.get());
 
   policy::PolicyServiceImpl::Providers providers;
   providers.push_back(policy_provider.get());
   scoped_ptr<policy::PolicyService> policy_service(
       new policy::PolicyServiceImpl(providers));
 
   policy::PolicyService* borrowed_policy_service = policy_service.get();
   return make_scoped_ptr(
       new PolicyWatcher(borrowed_policy_service, policy_service.Pass(),
                         policy_provider.Pass(), schema_registry.Pass()));
 }
 
 scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
     policy::PolicyService* policy_service,
     const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner) {
 #if defined(OS_CHROMEOS)
   // On Chrome OS the PolicyService is owned by the browser.
   DCHECK(policy_service);
-  return make_scoped_ptr(
-      new PolicyWatcher(policy_service, nullptr, nullptr, nullptr));
+  return make_scoped_ptr(new PolicyWatcher(policy_service, nullptr, nullptr,
+                                           CreateSchemaRegistry()));
 #else  // !defined(OS_CHROMEOS)
   DCHECK(!policy_service);
 
   // Create platform-specific PolicyLoader. Always read the Chrome policies
   // (even on Chromium) so that policy enforcement can't be bypassed by running
   // Chromium.
   scoped_ptr<policy::AsyncPolicyLoader> policy_loader;
 #if defined(OS_WIN)
   policy_loader = policy::PolicyLoaderWin::Create(
       file_task_runner, L"SOFTWARE\\Policies\\Google\\Chrome");
@@ skipping 10 matching lines @@
       policy::POLICY_SCOPE_MACHINE));
 #else
 #error OS that is not yet supported by PolicyWatcher code.
 #endif
 
   return PolicyWatcher::CreateFromPolicyLoader(policy_loader.Pass());
 #endif  // !(OS_CHROMEOS)
 }
 
 }  // namespace remoting