Show blocked policies on chrome://policy

Bug: 933797
Change-Id: I01360af1e9800f023c9db3294d0a30475b17568b
Reviewed-on: https://chromium-review.googlesource.com/c/1487513
Commit-Queue: Yann Dago <ydago@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636230}

components/policy/core/common/policy_loader_win.cc

@@ -148,8 +148,7 @@ void FilterUntrustedPolicy(PolicyMap* policy) {
 
   for (size_t i = 0; i < base::size(kInsecurePolicies); ++i) {
     if (policy->Get(kInsecurePolicies[i])) {
-      // TODO(pastarmovj): Surface this issue in the about:policy page.
-      policy->Erase(kInsecurePolicies[i]);
+      policy->GetMutable(kInsecurePolicies[i])->SetBlocked();
       invalid_policies++;
       const PolicyDetails* details =
           GetChromePolicyDetails(kInsecurePolicies[i]);

components/policy/core/common/policy_map.cc

@@ -83,7 +83,7 @@ void PolicyMap::Entry::AddError(base::StringPiece error) {
 }
 
 void PolicyMap::Entry::AddError(int message_id) {
-  error_message_ids_.push_back(message_id);
+  error_message_ids_.insert(message_id);
 }
 
 void PolicyMap::Entry::AddConflictingPolicy(const Entry& conflict) {
@@ -114,6 +114,15 @@ base::string16 PolicyMap::Entry::GetLocalizedErrors(
   return error_string;
 }
 
+bool PolicyMap::Entry::IsBlocked() const {
+  return error_message_ids_.find(IDS_POLICY_BLOCKED) !=
+         error_message_ids_.end();
+}
+
+void PolicyMap::Entry::SetBlocked() {
+  error_message_ids_.insert(IDS_POLICY_BLOCKED);
+}
+
 PolicyMap::PolicyMap() {}
 
 PolicyMap::~PolicyMap() {
@@ -122,22 +131,39 @@ PolicyMap::~PolicyMap() {
 
 const PolicyMap::Entry* PolicyMap::Get(const std::string& policy) const {
   auto entry = map_.find(policy);
-  return entry == map_.end() ? nullptr : &entry->second;
+  return entry != map_.end() && !entry->second.IsBlocked() ? &entry->second
+                                                           : nullptr;
 }
 
 PolicyMap::Entry* PolicyMap::GetMutable(const std::string& policy) {
   auto entry = map_.find(policy);
-  return entry == map_.end() ? nullptr : &entry->second;
+  return entry != map_.end() && !entry->second.IsBlocked() ? &entry->second
+                                                           : nullptr;
 }
 
 const base::Value* PolicyMap::GetValue(const std::string& policy) const {
   auto entry = map_.find(policy);
-  return entry == map_.end() ? nullptr : entry->second.value.get();
+  return entry != map_.end() && !entry->second.IsBlocked()
+             ? entry->second.value.get()
+             : nullptr;
 }
 
 base::Value* PolicyMap::GetMutableValue(const std::string& policy) {
   auto entry = map_.find(policy);
-  return entry == map_.end() ? nullptr : entry->second.value.get();
+  return entry != map_.end() && !entry->second.IsBlocked()
+             ? entry->second.value.get()
+             : nullptr;
+}
+
+const PolicyMap::Entry* PolicyMap::GetUntrusted(
+    const std::string& policy) const {
+  auto entry = map_.find(policy);
+  return entry != map_.end() ? &entry->second : nullptr;
+}
+
+PolicyMap::Entry* PolicyMap::GetMutableUntrusted(const std::string& policy) {
+  auto entry = map_.find(policy);
+  return entry != map_.end() ? &entry->second : nullptr;
 }
 
 void PolicyMap::Set(
@@ -202,7 +228,7 @@ std::unique_ptr<PolicyMap> PolicyMap::DeepCopy() const {
 
 void PolicyMap::MergeFrom(const PolicyMap& other) {
   for (const auto& it : other) {
-    const Entry* entry = Get(it.first);
+    const Entry* entry = GetUntrusted(it.first);
     bool same_value = false;
     if (!entry) {
       Set(it.first, it.second.DeepCopy());
@@ -213,14 +239,14 @@ void PolicyMap::MergeFrom(const PolicyMap& other) {
         new_policy.AddConflictingPolicy(*entry);
         Set(it.first, std::move(new_policy));
       } else {
-        GetMutable(it.first)->AddConflictingPolicy(it.second);
+        GetMutableUntrusted(it.first)->AddConflictingPolicy(it.second);
       }
     }
 
     if (entry) {
-      GetMutable(it.first)->AddError(same_value
-                                         ? IDS_POLICY_CONFLICT_SAME_VALUE
-                                         : IDS_POLICY_CONFLICT_DIFF_VALUE);
+      GetMutableUntrusted(it.first)->AddError(
+          same_value ? IDS_POLICY_CONFLICT_SAME_VALUE
+                     : IDS_POLICY_CONFLICT_DIFF_VALUE);
     }
   }
 }

components/policy/core/common/policy_map.h

@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/values.h"
 #include "components/policy/core/common/external_data_fetcher.h"
@@ -21,6 +22,10 @@
 
 namespace policy {
 
+class PolicyMapTest;
+FORWARD_DECLARE_TEST(PolicyMapTest, BlockedEntry);
+FORWARD_DECLARE_TEST(PolicyMapTest, MergeFrom);
+
 // A mapping of policy names to policy values for a given policy namespace.
 class POLICY_EXPORT PolicyMap {
  public:
@@ -65,6 +70,10 @@ class POLICY_EXPORT PolicyMap {
     // Adds a conflicting policy.
     void AddConflictingPolicy(const Entry& conflict);
 
+    bool IsBlocked() const;
+
+    void SetBlocked();
+
     // Callback used to look up a localized string given its l10n message ID. It
     // should return a UTF-16 string.
     typedef base::RepeatingCallback<base::string16(int message_id)>
@@ -76,7 +85,7 @@ class POLICY_EXPORT PolicyMap {
 
    private:
     std::string error_strings_;
-    std::vector<int> error_message_ids_;
+    std::set<int> error_message_ids_;
   };
 
   typedef std::map<std::string, Entry> PolicyMapType;
@@ -86,7 +95,7 @@ class POLICY_EXPORT PolicyMap {
   virtual ~PolicyMap();
 
   // Returns a weak reference to the entry currently stored for key |policy|,
-  // or NULL if not found. Ownership is retained by the PolicyMap.
+  // or NULL if untrusted or not found. Ownership is retained by the PolicyMap.
   const Entry* Get(const std::string& policy) const;
   Entry* GetMutable(const std::string& policy);
 
@@ -169,6 +178,14 @@ class POLICY_EXPORT PolicyMap {
   void Clear();
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(PolicyMapTest, BlockedEntry);
+  FRIEND_TEST_ALL_PREFIXES(PolicyMapTest, MergeFrom);
+
+  // Returns a weak reference to the entry currently stored for key |policy|,
+  // or NULL if not found. Ownership is retained by the PolicyMap.
+  const Entry* GetUntrusted(const std::string& policy) const;
+  Entry* GetMutableUntrusted(const std::string& policy);
+
   // Helper function for Equals().
   static bool MapEntryEquals(const PolicyMapType::value_type& a,
                              const PolicyMapType::value_type& b);

components/policy/core/common/policy_map_unittest.cc

@@ -210,6 +210,10 @@ TEST_F(PolicyMapTest, MergeFrom) {
   a.Set(kTestPolicyName7, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
         POLICY_SOURCE_ENTERPRISE_DEFAULT, std::make_unique<base::Value>(false),
         nullptr);
+  a.Set(kTestPolicyName8, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
+        POLICY_SOURCE_ACTIVE_DIRECTORY,
+        std::make_unique<base::Value>("blocked AD policy"), nullptr);
+  a.GetMutable(kTestPolicyName8)->SetBlocked();
 
   PolicyMap b;
   b.Set(kTestPolicyName1, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
@@ -231,12 +235,18 @@ TEST_F(PolicyMapTest, MergeFrom) {
   b.Set(kTestPolicyName7, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
         POLICY_SOURCE_ACTIVE_DIRECTORY, std::make_unique<base::Value>(true),
         nullptr);
+  b.Set(kTestPolicyName8, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
+        POLICY_SOURCE_CLOUD,
+        std::make_unique<base::Value>("non blocked cloud policy"), nullptr);
 
   auto conflicted_policy_1 = a.Get(kTestPolicyName1)->DeepCopy();
   auto conflicted_policy_4 = a.Get(kTestPolicyName4)->DeepCopy();
   auto conflicted_policy_5 = a.Get(kTestPolicyName5)->DeepCopy();
   auto conflicted_policy_7 = a.Get(kTestPolicyName7)->DeepCopy();
+  auto conflicted_policy_8 = b.Get(kTestPolicyName8)->DeepCopy();
 
+  a.GetMutable(kTestPolicyName7)->SetBlocked();
+  b.GetMutable(kTestPolicyName7)->SetBlocked();
   a.MergeFrom(b);
 
   PolicyMap c;
@@ -279,6 +289,14 @@ TEST_F(PolicyMapTest, MergeFrom) {
         nullptr);
   c.GetMutable(kTestPolicyName7)->AddError(IDS_POLICY_CONFLICT_DIFF_VALUE);
   c.GetMutable(kTestPolicyName7)->AddConflictingPolicy(conflicted_policy_7);
+  c.GetMutable(kTestPolicyName7)->SetBlocked();
+
+  c.Set(kTestPolicyName8, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
+        POLICY_SOURCE_ACTIVE_DIRECTORY,
+        std::make_unique<base::Value>("blocked AD policy"), nullptr);
+  c.GetMutable(kTestPolicyName8)->AddError(IDS_POLICY_CONFLICT_DIFF_VALUE);
+  c.GetMutable(kTestPolicyName8)->AddConflictingPolicy(conflicted_policy_8);
+  c.GetMutable(kTestPolicyName8)->SetBlocked();
 
   EXPECT_TRUE(a.Equals(c));
 }
@@ -415,4 +433,52 @@ TEST_F(PolicyMapTest, EntryAddConflict) {
   EXPECT_TRUE(entry_b.conflicts[0].Equals(entry_c));
 }
 
+TEST_F(PolicyMapTest, BlockedEntry) {
+  PolicyMap::Entry entry_a(POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                           POLICY_SOURCE_CLOUD,
+                           std::make_unique<base::Value>("a"), nullptr);
+  PolicyMap::Entry entry_b = entry_a.DeepCopy();
+  entry_b.value = std::make_unique<base::Value>("b");
+  PolicyMap::Entry entry_c_blocked = entry_a.DeepCopy();
+  entry_c_blocked.value = std::make_unique<base::Value>("c");
+  entry_c_blocked.SetBlocked();
+
+  PolicyMap policies;
+  policies.Set("a", entry_a.DeepCopy());
+  policies.Set("b", entry_b.DeepCopy());
+  policies.Set("c", entry_c_blocked.DeepCopy());
+
+  const size_t expected_size = 3;
+  EXPECT_TRUE(policies.size() == expected_size);
+
+  EXPECT_TRUE(policies.Get("a")->Equals(entry_a));
+  EXPECT_TRUE(policies.Get("b")->Equals(entry_b));
+  EXPECT_TRUE(policies.Get("c") == nullptr);
+
+  EXPECT_TRUE(policies.GetMutable("a")->Equals(entry_a));
+  EXPECT_TRUE(policies.GetMutable("b")->Equals(entry_b));
+  EXPECT_TRUE(policies.GetMutable("c") == nullptr);
+
+  EXPECT_TRUE(policies.GetValue("a")->Equals(entry_a.value.get()));
+  EXPECT_TRUE(policies.GetValue("b")->Equals(entry_b.value.get()));
+  EXPECT_TRUE(policies.GetValue("c") == nullptr);
+
+  EXPECT_TRUE(policies.GetMutableValue("a")->Equals(entry_a.value.get()));
+  EXPECT_TRUE(policies.GetMutableValue("b")->Equals(entry_b.value.get()));
+  EXPECT_TRUE(policies.GetMutableValue("c") == nullptr);
+
+  EXPECT_TRUE(policies.GetUntrusted("a")->Equals(entry_a));
+  EXPECT_TRUE(policies.GetUntrusted("b")->Equals(entry_b));
+  EXPECT_TRUE(policies.GetUntrusted("c")->Equals(entry_c_blocked));
+
+  EXPECT_TRUE(policies.GetMutableUntrusted("a")->Equals(entry_a));
+  EXPECT_TRUE(policies.GetMutableUntrusted("b")->Equals(entry_b));
+  EXPECT_TRUE(policies.GetMutableUntrusted("c")->Equals(entry_c_blocked));
+
+  size_t iterated_values = 0;
+  for (auto it = policies.begin(); it != policies.end();
+       ++it, ++iterated_values) {
+  }
+  EXPECT_TRUE(iterated_values == expected_size);
+}
 }  // namespace policy

components/policy_strings.grdp

@@ -431,4 +431,7 @@ Additional details:
   <message name="IDS_POLICY_CONFLICT_DIFF_VALUE" desc="Text explaining that a policy had conflicting sources and values.">
     Warning: More than one source with conflicting values is present for this policy!
   </message>
+  <message name="IDS_POLICY_BLOCKED" desc="Text explaining that a policy is blocked, therefore ignored.">
+    This policy is blocked, its value will be ignored.
+  </message>
 </grit-part>