Update PolicyWatcher to provide platform policy values

The current PolicyWatcher exposes a set of 'current' policies which
is actually the default policies with overrides from any platform
policy which is actually set.  This works fine for driving behavior
but in some cases we actually want to know whether a policy value
we receive is the 'default' or if an admin has set it explicitly.

To allow this, the PolicyWatcher will now store the set of platform
policies on the machine and provide it on request.  I plan to use
this for logging (to get an idea of policy usage and popularity)
and for cases where we want to provide a flag from the website to
the NMH but also want admins to override the value if needed.

Since the PolicyWatcher provides the platform policies now, I renamed
GetCurrentPolicies() to GetEffectivePolicies() as I think that name
reflects the fact that it is a merged set of policies.

Change-Id: Id2fd578ac42d925922e8beb5f028374d7bcdcbf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587614Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837116}

remoting/host/it2me/it2me_native_messaging_host.cc

@@ -324,7 +324,7 @@ void It2MeNativeMessagingHost::ProcessConnect(
   }
 
   std::unique_ptr<base::DictionaryValue> policies =
-      policy_watcher_->GetCurrentPolicies();
+      policy_watcher_->GetEffectivePolicies();
   if (policies->size() == 0) {
     // At this point policies have been read, so if there are none set then
     // it indicates an error. Since this can be fixed by end users it has a

remoting/host/policy_watcher.cc

@@ -159,8 +159,12 @@ void PolicyWatcher::StartWatching(
   }
 }
 
-std::unique_ptr<base::DictionaryValue> PolicyWatcher::GetCurrentPolicies() {
-  return old_policies_->CreateDeepCopy();
+std::unique_ptr<base::DictionaryValue> PolicyWatcher::GetEffectivePolicies() {
+  return effective_policies_->CreateDeepCopy();
+}
+
+std::unique_ptr<base::DictionaryValue> PolicyWatcher::GetPlatformPolicies() {
+  return platform_policies_->CreateDeepCopy();
 }
 
 std::unique_ptr<base::DictionaryValue> PolicyWatcher::GetDefaultPolicies() {
@@ -191,7 +195,8 @@ std::unique_ptr<base::DictionaryValue> PolicyWatcher::GetDefaultPolicies() {
 }
 
 void PolicyWatcher::SignalPolicyError() {
-  old_policies_->Clear();
+  effective_policies_->Clear();
+  platform_policies_->Clear();
   policy_error_callback_.Run();
 }
 
@@ -200,7 +205,8 @@ PolicyWatcher::PolicyWatcher(
     std::unique_ptr<policy::PolicyService> owned_policy_service,
     std::unique_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider,
     std::unique_ptr<policy::SchemaRegistry> owned_schema_registry)
-    : old_policies_(new base::DictionaryValue()),
+    : effective_policies_(new base::DictionaryValue()),
+      platform_policies_(new base::DictionaryValue()),
       default_values_(GetDefaultPolicies()),
       policy_service_(policy_service),
       owned_schema_registry_(std::move(owned_schema_registry)),
@@ -303,7 +309,7 @@ PolicyWatcher::StoreNewAndReturnChangedPolicies(
   base::DictionaryValue::Iterator iter(*new_policies);
   while (!iter.IsAtEnd()) {
     base::Value* old_policy;
-    if (!(old_policies_->Get(iter.key(), &old_policy) &&
+    if (!(effective_policies_->Get(iter.key(), &old_policy) &&
           old_policy->Equals(&iter.value()))) {
       changed_policies->Set(iter.key(), iter.value().CreateDeepCopy());
     }
@@ -324,7 +330,7 @@ PolicyWatcher::StoreNewAndReturnChangedPolicies(
   }
 
   // Save the new policies.
-  old_policies_.swap(new_policies);
+  effective_policies_.swap(new_policies);
 
   return changed_policies;
 }
@@ -341,6 +347,8 @@ void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
     return;
   }
 
+  platform_policies_ = new_policies->CreateDeepCopy();
+
   // Use default values for any missing policies.
   std::unique_ptr<base::DictionaryValue> filled_policies =
       CopyValuesAndAddDefaults(*new_policies, *default_values_);

remoting/host/policy_watcher.h

@@ -59,10 +59,16 @@ class PolicyWatcher : public policy::PolicyService::Observer {
       const PolicyErrorCallback& policy_error_callback);
 
   // Return the current policies. If the policies have not yet been read, or if
+  // an error occurred, the returned dictionary will be empty.  The dictionary
+  // returned is the union of |platform_policies_| and |default_values_|.
+  std::unique_ptr<base::DictionaryValue> GetEffectivePolicies();
+
+  // Return the set of policies which have been explicitly set on the machine.
+  // If the policies have not yet been read, no policies have been set, or if
   // an error occurred, the returned dictionary will be empty.
-  std::unique_ptr<base::DictionaryValue> GetCurrentPolicies();
+  std::unique_ptr<base::DictionaryValue> GetPlatformPolicies();
 
-  // Return the default policies.
+  // Return the default policy values.
   static std::unique_ptr<base::DictionaryValue> GetDefaultPolicies();
 
   // Specify a |policy_service| to borrow (on Chrome OS, from the browser
@@ -100,8 +106,7 @@ class PolicyWatcher : public policy::PolicyService::Observer {
   // Normalizes policies using Schema::Normalize and converts deprecated
   // policies.
   //
-  // - Returns false if |dict| is invalid (i.e. contains mistyped policy
-  // values).
+  // - Returns false if |dict| is invalid, e.g. contains mistyped policy values.
   // - Returns true if |dict| was valid or got normalized.
   bool NormalizePolicies(base::DictionaryValue* dict);
 
@@ -109,8 +114,9 @@ class PolicyWatcher : public policy::PolicyService::Observer {
   // replacement policy is not set, and removes deprecated policied from dict.
   void HandleDeprecatedPolicies(base::DictionaryValue* dict);
 
-  // Stores |new_policies| into |old_policies_|.  Returns dictionary with items
-  // from |new_policies| that are different from the old |old_policies_|.
+  // Stores |new_policies| into |effective_policies_|.  Returns dictionary with
+  // items from |new_policies| that are different from the old
+  // |effective_policies_|.
   std::unique_ptr<base::DictionaryValue> StoreNewAndReturnChangedPolicies(
       std::unique_ptr<base::DictionaryValue> new_policies);
 
@@ -143,7 +149,14 @@ class PolicyWatcher : public policy::PolicyService::Observer {
   PolicyUpdatedCallback policy_updated_callback_;
   PolicyErrorCallback policy_error_callback_;
 
-  std::unique_ptr<base::DictionaryValue> old_policies_;
+  // The combined set of policies (|platform_policies_| + |default_values_|)
+  // which define the effective policy set.
+  std::unique_ptr<base::DictionaryValue> effective_policies_;
+
+  // The policies which have had their values explicitly set via a policy entry.
+  std::unique_ptr<base::DictionaryValue> platform_policies_;
+
+  // The set of policy values to use if a policy has not been explicitly set.
   std::unique_ptr<base::DictionaryValue> default_values_;
 
   policy::PolicyService* policy_service_;

remoting/host/policy_watcher_unittest.cc

@@ -76,6 +76,8 @@ class PolicyWatcherTest : public testing::Test {
     policy_watcher_ = PolicyWatcher::CreateFromPolicyLoaderForTesting(
         base::WrapUnique(policy_loader_));
 
+    policy_watcher_default_values_ = PolicyWatcher::GetDefaultPolicies();
+
     base::ListValue host_domain;
     host_domain.AppendString(kHostDomain);
     base::ListValue client_domain;
@@ -240,9 +242,8 @@ class PolicyWatcherTest : public testing::Test {
     return policy_watcher_->GetPolicySchema();
   }
 
-  // TODO(jamiewalch): Update this to use PolicyWatcher::GetDefaultValues()
   const base::DictionaryValue& GetDefaultValues() {
-    return *(policy_watcher_->default_values_);
+    return *policy_watcher_default_values_;
   }
 
   MOCK_METHOD0(PostPolicyWatcherShutdown, void());
@@ -329,6 +330,8 @@ class PolicyWatcherTest : public testing::Test {
         << "Sanity check that defaults expected by the test code "
         << "match what is stored in PolicyWatcher::default_values_";
   }
+
+  std::unique_ptr<base::DictionaryValue> policy_watcher_default_values_;
 };
 
 const char* PolicyWatcherTest::kHostDomain = "google.com";
@@ -777,7 +780,7 @@ TEST_F(PolicyWatcherTest, DeprecatedEmpty) {
   StartWatching();
 }
 
-TEST_F(PolicyWatcherTest, GetCurrentPolicies) {
+TEST_F(PolicyWatcherTest, GetEffectivePolicies) {
   testing::InSequence sequence;
   EXPECT_CALL(mock_policy_callback_,
               OnPolicyUpdatePtr(IsPolicies(&nat_true_others_default_)));
@@ -786,19 +789,61 @@ TEST_F(PolicyWatcherTest, GetCurrentPolicies) {
 
   StartWatching();
   SetPolicies(nat_false_);
-  std::unique_ptr<base::DictionaryValue> current_policies =
-      policy_watcher_->GetCurrentPolicies();
-  ASSERT_TRUE(*current_policies == nat_false_others_default_);
+  std::unique_ptr<base::DictionaryValue> effective_policies =
+      policy_watcher_->GetEffectivePolicies();
+  ASSERT_TRUE(*effective_policies == nat_false_others_default_);
+}
+
+TEST_F(PolicyWatcherTest, GetEffectivePoliciesError) {
+  EXPECT_CALL(mock_policy_callback_, OnPolicyError());
+
+  SetPolicies(nat_one_);
+  StartWatching();
+  std::unique_ptr<base::DictionaryValue> effective_policies =
+      policy_watcher_->GetEffectivePolicies();
+  ASSERT_EQ(0u, effective_policies->size());
+}
+
+TEST_F(PolicyWatcherTest, GetPlatformPolicies) {
+  testing::InSequence sequence;
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&GetDefaultValues())));
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&nat_false_)));
+
+  StartWatching();
+  ASSERT_EQ(0u, policy_watcher_->GetPlatformPolicies()->size());
+  SetPolicies(nat_false_);
+  ASSERT_EQ(1u, policy_watcher_->GetPlatformPolicies()->size());
+}
+
+TEST_F(PolicyWatcherTest, GetPlatformPoliciesMultipleOverrides) {
+  testing::InSequence sequence;
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&GetDefaultValues())));
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&domain_full_)));
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&nat_false_)));
+  EXPECT_CALL(mock_policy_callback_,
+              OnPolicyUpdatePtr(IsPolicies(&nat_true_domain_empty_)));
+
+  StartWatching();
+  ASSERT_EQ(0u, policy_watcher_->GetPlatformPolicies()->size());
+  SetPolicies(domain_full_);
+  ASSERT_EQ(1u, policy_watcher_->GetPlatformPolicies()->size());
+  SetPolicies(nat_false_domain_full_);
+  ASSERT_EQ(2u, policy_watcher_->GetPlatformPolicies()->size());
+  SetPolicies(nat_true_domain_empty_);
+  ASSERT_EQ(2u, policy_watcher_->GetPlatformPolicies()->size());
 }
 
-TEST_F(PolicyWatcherTest, GetCurrentPoliciesError) {
+TEST_F(PolicyWatcherTest, GetPlatformPoliciesError) {
   EXPECT_CALL(mock_policy_callback_, OnPolicyError());
 
   SetPolicies(nat_one_);
   StartWatching();
-  std::unique_ptr<base::DictionaryValue> current_policies =
-      policy_watcher_->GetCurrentPolicies();
-  ASSERT_EQ(0u, current_policies->size());
+  ASSERT_EQ(0u, policy_watcher_->GetPlatformPolicies()->size());
 }
 
 }  // namespace remoting