Rejecting networks/certificates individually from ONC during validation.

chromeos/network/onc/onc_validator.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 "chromeos/network/onc/onc_validator.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/json/json_writer.h"
@@ skipping 159 matching lines @@
         + "Field name '" + field_name + "' is unknown.";
     if (error_on_unknown_field_)
       LOG(ERROR) << message;
     else
       LOG(WARNING) << message;
   }
 
   return result.Pass();
 }
 
+scoped_ptr<base::ListValue> Validator::MapArray(
+    const OncValueSignature& array_signature,
+    const base::ListValue& onc_array,
+    bool* nested_error) {
+  bool nested_error_in_current_array = false;
+  scoped_ptr<base::ListValue> result = Mapper::MapArray(
+      array_signature, onc_array, &nested_error_in_current_array);
+
+  // Drop individual networks and certificates instead of rejecting all of
+  // the configuration.
+  if (nested_error_in_current_array &&
+      &array_signature != &kNetworkConfigurationListSignature &&
+      &array_signature != &kCertificateListSignature) {
+    *nested_error = nested_error_in_current_array;

[stevenjb, 2012/12/14 01:06:36]

It seems odd that nested_error is only sometimes set. This should either set it
to false in an else clause, or we should document that it doesn't always set it
(but I much prefer the former).

[pneubeck (no reviews), 2012/12/14 18:55:46]

That's currently the behavior of all other methods in Validator and Mapper.
Clarified that |error| or |nested_error| is _only_ set if an error occurred.

The original reason was:
- The caller has to initialize the variable in anyway.
- This way, an error variable can be used for several methods in sequence and
the result is the OR of all errors.
(see e.g. Mapper::MapArray)

If we change it here, we must/should change all methods.

[stevenjb, 2012/12/14 20:21:40]

OK, as long as the comment is clear that's fine.

+  }
+  return result.Pass();
+}
+
 scoped_ptr<base::Value> Validator::MapEntry(int index,
                                             const OncValueSignature& signature,
                                             const base::Value& onc_value,
                                             bool* error) {
   std::string str = base::IntToString(index);
   path_.push_back(str);
   scoped_ptr<base::Value> result = Mapper::MapEntry(index, signature,
                                                     onc_value, error);
   DCHECK_EQ(str, path_.back());
   path_.pop_back();
@@ skipping 32 matching lines @@
                                           &recommended_value)) {
     return true;
   }
   base::ListValue* recommended_list;
   recommended_value->GetAsList(&recommended_list);
   CHECK(recommended_list != NULL);
 
   recommended.reset(recommended_list);
 
   if (!managed_onc_) {
+    error_or_warning_found_ = true;
     LOG(WARNING) << WarningHeader() << "Found the field '" << onc::kRecommended
                  << "' in an unmanaged ONC. Removing it.";
     return true;
   }
 
   scoped_ptr<base::ListValue> repaired_recommended(new base::ListValue);
   for (base::ListValue::iterator it = recommended->begin();
        it != recommended->end(); ++it) {
     std::string field_name;
     if (!(*it)->GetAsString(&field_name)) {
@@ skipping 460 matching lines @@
 }
 
 std::string Validator::MessageHeader(bool is_error) {
   std::string path = path_.empty() ? "toplevel" : JoinString(path_, ".");
   std::string message = "At " + path + ": ";
   return message;
 }
 
 }  // namespace onc
 }  // namespace chromeos