Wire JSON parsing errors to chrome://policy

cloud_policy_generated.cc parses JSON for certain policies in a
function called DecodeJSON - however, if the JSON is invalid, it
simply drops the invalid policy with no visible warning. This CL
wires errors through by adding an error field to PolicyMap.
The different possible designs are compared internally at
go/decodejsonfix

Bug: 853719
Change-Id: I98979183ac54278710f1ac57e5a48f2a9eee0b59
Reviewed-on: https://chromium-review.googlesource.com/1104354Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571449}

chrome/browser/policy/policy_conversions.cc

@@ -7,6 +7,7 @@
 #include <unordered_set>
 
 #include "base/json/json_writer.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
@@ -110,30 +111,35 @@ void GetPolicyValues(
     bool convert_values,
     std::unique_ptr<std::unordered_set<std::string>> policy_names) {
   for (const auto& entry : map) {
-    if (entry.second.scope == policy::POLICY_SCOPE_USER && !with_user_policies)
+    const std::string& policy_name = entry.first;
+    const PolicyMap::Entry& policy = entry.second;
+    if (policy.scope == policy::POLICY_SCOPE_USER && !with_user_policies)
       continue;
+
     std::unique_ptr<base::DictionaryValue> value(new base::DictionaryValue);
     value->Set("value",
-               CopyAndMaybeConvert(entry.second.value.get(), convert_values));
-    if (entry.second.scope == policy::POLICY_SCOPE_USER)
-      value->SetString("scope", "user");
-    else
-      value->SetString("scope", "machine");
-    if (entry.second.level == policy::POLICY_LEVEL_RECOMMENDED)
-      value->SetString("level", "recommended");
-    else
-      value->SetString("level", "mandatory");
-    value->SetString("source", kPolicySources[entry.second.source].key);
+               CopyAndMaybeConvert(policy.value.get(), convert_values));
+    value->SetString("scope", (policy.scope == policy::POLICY_SCOPE_USER)
+                                  ? "user"
+                                  : "machine");
+    value->SetString("level", (policy.level == policy::POLICY_LEVEL_RECOMMENDED)
+                                  ? "recommended"
+                                  : "mandatory");
+    value->SetString("source", kPolicySources[policy.source].key);
     base::string16 error;
     if (policy_names &&
-        policy_names->find(entry.first) == policy_names->end()) {
+        policy_names->find(policy_name) == policy_names->end()) {
+      // We don't know what this policy is. This is an important error to show.
       error = l10n_util::GetStringUTF16(IDS_POLICY_UNKNOWN);
     } else {
-      error = errors->GetErrors(entry.first);
+      // The PolicyMap contains errors about retrieving the policy, while the
+      // PolicyErrorMap contains validation errors. Give priority to PolicyMap.
+      error = !policy.error.empty() ? base::UTF8ToUTF16(policy.error)
+                                    : errors->GetErrors(policy_name);
     }
     if (!error.empty())
       value->SetString("error", error);
-    values->SetWithoutPathExpansion(entry.first, std::move(value));
+    values->SetWithoutPathExpansion(policy_name, std::move(value));
   }
 }
 

components/policy/core/common/policy_map.cc

@@ -25,6 +25,7 @@ PolicyMap::Entry PolicyMap::Entry::DeepCopy() const {
   copy.source = source;
   if (value)
     copy.value = value->CreateDeepCopy();
+  copy.error = error;
   if (external_data_fetcher) {
     copy.external_data_fetcher.reset(
         new ExternalDataFetcher(*external_data_fetcher));
@@ -47,6 +48,7 @@ bool PolicyMap::Entry::Equals(const PolicyMap::Entry& other) const {
   return level == other.level && scope == other.scope &&
          source == other.source &&  // Necessary for PolicyUIHandler observers.
                                     // They have to update when sources change.
+         error == other.error &&
          ((!value && !other.value) ||
           (value && other.value && *value == *other.value)) &&
          ExternalDataFetcher::Equals(external_data_fetcher.get(),
@@ -99,6 +101,10 @@ void PolicyMap::Set(const std::string& policy, Entry entry) {
   map_[policy] = std::move(entry);
 }
 
+void PolicyMap::SetError(const std::string& policy, const std::string& error) {
+  map_[policy].error = error;
+}
+
 void PolicyMap::SetSourceForAll(PolicySource source) {
   for (auto& it : map_) {
     it.second.source = source;

components/policy/core/common/policy_map.h

@@ -30,6 +30,7 @@ class POLICY_EXPORT PolicyMap {
     PolicyLevel level = POLICY_LEVEL_RECOMMENDED;
     PolicyScope scope = POLICY_SCOPE_USER;
     std::unique_ptr<base::Value> value;
+    std::string error;
     std::unique_ptr<ExternalDataFetcher> external_data_fetcher;
 
     // For debugging and displaying only. Set by provider delivering the policy.
@@ -69,16 +70,23 @@ class POLICY_EXPORT PolicyMap {
   const base::Value* GetValue(const std::string& policy) const;
   base::Value* GetMutableValue(const std::string& policy);
 
-  // Overwrites any existing information stored in the map for the key
-  // |policy|.
+  // Overwrites any existing information stored in the map for the key |policy|.
+  // Resets the error for that policy to the empty string.
   void Set(const std::string& policy,
            PolicyLevel level,
            PolicyScope scope,
            PolicySource source,
            std::unique_ptr<base::Value> value,
            std::unique_ptr<ExternalDataFetcher> external_data_fetcher);
+
   void Set(const std::string& policy, Entry entry);
 
+  // Adds an |error| to the map for the key |policy| that should be shown to the
+  // user alongside the value in the policy UI. This is equivalent to calling
+  // |GetMutableValue(policy)->error = error|, so should only be called for
+  // policies that are already stored in this map.
+  void SetError(const std::string& policy, const std::string& error);
+
   // For all policies, overwrite the PolicySource with |source|.
   void SetSourceForAll(PolicySource source);
 

components/policy/core/common/policy_map_unittest.cc

@@ -27,6 +27,9 @@ const char kTestPolicyName6[] = "policy.test.6";
 const char kTestPolicyName7[] = "policy.test.7";
 const char kTestPolicyName8[] = "policy.test.8";
 
+// Dummy error message.
+const char kTestError[] = "Test error message";
+
 // Utility functions for the tests.
 void SetPolicy(PolicyMap* map,
                const char* name,
@@ -65,12 +68,14 @@ TEST_F(PolicyMapTest, SetAndGet) {
   base::Value expected_b("bbb");
   EXPECT_TRUE(expected_b.Equals(map.GetValue(kTestPolicyName1)));
   SetPolicy(&map, kTestPolicyName1, CreateExternalDataFetcher("dummy"));
+  map.SetError(kTestPolicyName1, kTestError);
   EXPECT_FALSE(map.GetValue(kTestPolicyName1));
   const PolicyMap::Entry* entry = map.Get(kTestPolicyName1);
   ASSERT_TRUE(entry != nullptr);
   EXPECT_EQ(POLICY_LEVEL_MANDATORY, entry->level);
   EXPECT_EQ(POLICY_SCOPE_USER, entry->scope);
   EXPECT_EQ(POLICY_SOURCE_CLOUD, entry->source);
+  EXPECT_EQ(kTestError, entry->error);
   EXPECT_TRUE(
       ExternalDataFetcher::Equals(entry->external_data_fetcher.get(),
                                   CreateExternalDataFetcher("dummy").get()));
@@ -82,6 +87,7 @@ TEST_F(PolicyMapTest, SetAndGet) {
   EXPECT_EQ(POLICY_LEVEL_RECOMMENDED, entry->level);
   EXPECT_EQ(POLICY_SCOPE_MACHINE, entry->scope);
   EXPECT_EQ(POLICY_SOURCE_ENTERPRISE_DEFAULT, entry->source);
+  EXPECT_EQ("", entry->error);
   EXPECT_FALSE(entry->external_data_fetcher);
 }
 

components/policy/tools/generate_policy_source.py

@@ -1153,12 +1153,12 @@ namespace em = enterprise_management;
 namespace {
 
 std::unique_ptr<base::Value> DecodeIntegerValue(
-    google::protobuf::int64 value) {
+    google::protobuf::int64 value, std::string* error) {
   if (value < std::numeric_limits<int>::min() ||
       value > std::numeric_limits<int>::max()) {
-    LOG(WARNING) << "Integer value " << value
-                 << " out of numeric limits, ignoring.";
-    return nullptr;
+    LOG(WARNING) << "Integer value out of numeric limits: " << value;
+    *error = "Number out of range - invalid int32";
+    return std::make_unique<base::Value>(std::to_string(value));
   }
 
   return base::WrapUnique(
@@ -1173,16 +1173,23 @@ std::unique_ptr<base::ListValue> DecodeStringList(
   return list_value;
 }
 
-std::unique_ptr<base::Value> DecodeJson(const std::string& json) {
-  std::unique_ptr<base::Value> root =
-      base::JSONReader::Read(json, base::JSON_ALLOW_TRAILING_COMMAS);
+std::unique_ptr<base::Value> DecodeJson(const std::string& json,
+                                        std::string* error) {
 
-  if (!root)
-    LOG(WARNING) << "Invalid JSON string, ignoring: " << json;
+  std::unique_ptr<base::Value> parsed_value =
+      base::JSONReader::ReadAndReturnError(
+          json, base::JSON_ALLOW_TRAILING_COMMAS, nullptr, error);
+
+  if (!parsed_value) {
+    // Can't parse as JSON so return it as a string for the handler to validate.
+    LOG(WARNING) << "Invalid JSON: " << json;
+    return std::make_unique<base::Value>(json);
+  }
 
   // Accept any Value type that parsed as JSON, and leave it to the handler to
   // convert and check the concrete type.
-  return root;
+  error->clear();
+  return parsed_value;
 }
 
 // Returns true and sets |level| to a PolicyLevel if the policy has been set
@@ -1228,13 +1235,13 @@ def _CreateValue(type, arg):
   if type == 'Type::BOOLEAN':
     return 'new base::Value(%s)' % arg
   elif type == 'Type::INTEGER':
-    return 'DecodeIntegerValue(%s)' % arg
+    return 'DecodeIntegerValue(%s, &error)' % arg
   elif type == 'Type::STRING':
     return 'new base::Value(%s)' % arg
   elif type == 'Type::LIST':
     return 'DecodeStringList(%s)' % arg
   elif type == 'Type::DICTIONARY' or type == 'TYPE_EXTERNAL':
-    return 'DecodeJson(%s)' % arg
+    return 'DecodeJson(%s, &error)' % arg
   else:
     raise NotImplementedError('Unknown type %s' % type)
 
@@ -1252,23 +1259,21 @@ def _WriteCloudPolicyDecoderCode(f, policy):
   f.write('    const em::%s& policy_proto = policy.%s();\n' %
           (proto_type, membername))
   f.write('    PolicyLevel level;\n'
-          '    if (GetPolicyLevel(policy_proto, &level)) {\n')
+          '    if (GetPolicyLevel(policy_proto, &level)) {\n'
+          '      std::string error;\n')
   f.write('      std::unique_ptr<base::Value> value(%s);\n' %
           (_CreateValue(policy.policy_type, 'policy_proto.value()')))
-  # TODO(bartfab): |value| == NULL indicates that the policy value could not be
-  # parsed successfully. Surface such errors in the UI.
-  f.write('      if (value) {\n')
-  f.write('        std::unique_ptr<ExternalDataFetcher>\n')
-  f.write('            external_data_fetcher(%s);\n' %
+  f.write('      std::unique_ptr<ExternalDataFetcher>\n')
+  f.write('          external_data_fetcher(%s);\n' %
           _CreateExternalDataFetcher(policy.policy_type, policy.name))
-  f.write('        map->Set(key::k%s, \n' % policy.name)
-  f.write('                 level, \n'
-          '                 scope, \n'
-          '                 POLICY_SOURCE_CLOUD, \n'
-          '                 std::move(value), \n'
-          '                 std::move(external_data_fetcher));\n'
-          '      }\n'
-          '    }\n'
+  f.write('      map->Set(key::k%s, \n' % policy.name)
+  f.write('               level, \n'
+          '               scope, \n'
+          '               POLICY_SOURCE_CLOUD, \n'
+          '               std::move(value), \n'
+          '               std::move(external_data_fetcher));\n')
+  f.write('      map->SetError(key::k%s, error);\n' % policy.name)
+  f.write('    }\n'
           '  }\n')