Adding error handling to ONC validation.

chrome/browser/policy/configuration_policy_handler_chromeos.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.
 
 #include "chrome/browser/policy/configuration_policy_handler_chromeos.h"
 
 #include <string>
 
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
@@ skipping 35 matching lines @@
   if (!CheckAndGetValue(policies, errors, &value))
     return false;
 
   if (value) {
     std::string onc_blob;
     value->GetAsString(&onc_blob);
     std::string json_error;
     scoped_ptr<base::DictionaryValue> root_dict =
         onc::ReadDictionaryFromJson(onc_blob, &json_error);
     if (root_dict.get() == NULL) {
-      errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_PARSE_ERROR,
+      errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_PARSE_FAILED,
                        json_error);
       return false;
     }
 
     // Validate the ONC dictionary. We are liberal and ignore unknown field
     // names and ignore invalid field names in kRecommended arrays.
     onc::Validator validator(false,  // Ignore unknown fields.
                              false,  // Ignore invalid recommended field names.
                              true,  // Fail on missing fields.
                              true);  // Validate for managed ONC
 
     // ONC policies are always unencrypted.
+    std::string messages;
     root_dict = validator.ValidateAndRepairObject(
-        &onc::kUnencryptedConfigurationSignature,
-        *root_dict);
+        &onc::kToplevelConfigurationSignature, *root_dict, &messages);
+    if (!messages.empty()) {
+      errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_PARSE_ERROR,
+                       messages);

[Mattias Nissler (ping if slow), 2012/11/29 12:49:24]

It seems weird that instead of adding individual errors here, you instead add
just one error that holds the concatenation of all the errors you encountered.

Have you considered using something like PolicyErrorMap for gathering and
returning errors during parsing/validation?

[pneubeck (no reviews), 2012/11/29 12:58:02]

What for?
I could imagine, that having a list of errors (sth. like vector<string>) would
help layouting of the message. However, the win seems minimal compared to having
linebreaks between the errors.

[Mattias Nissler (ping if slow), 2012/11/29 13:09:18]

Right, now what if you want to format this in HTML? Linebreaks in HTML don't
cause linebreaks ;)

about:policy just joins errors at the moment (using a blank as the separator, so
this is already inconsistent with your joining), but once we're making it more
user-consumable, we'd have to make it into an itemized list in HTML anyways, and
then we need to have individual items.

+    }
 
     if (root_dict.get() == NULL) {
-      errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_PARSE_ERROR);
       // Don't reject the policy, as some networks or certificates could still
       // be applied.
       return true;
     }
   }
 
   return true;
 }
 
 void NetworkConfigurationPolicyHandler::ApplyPolicySettings(
@@ skipping 110 matching lines @@
         base::DictionaryValue* app_dict = new base::DictionaryValue();
         app_dict->SetString(ash::kPinnedAppsPrefAppIDPath, id);
         pinned_apps_list->Append(app_dict);
       }
     }
     prefs->SetValue(pref_path(), pinned_apps_list);
   }
 }
 
 }  // namespace policy