Add 'setProperties' to InternetOptionsHandler

chromeos/network/managed_network_configuration_handler_impl.cc

 // Copyright 2013 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/managed_network_configuration_handler_impl.h"
 
 #include <set>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 12 matching lines @@
 #include "chromeos/network/network_device_handler.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_policy_observer.h"
 #include "chromeos/network/network_profile.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_ui_data.h"
 #include "chromeos/network/onc/onc_merger.h"
 #include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_translation_tables.h"
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_validator.h"
 #include "chromeos/network/policy_util.h"
 #include "chromeos/network/shill_property_util.h"
 #include "components/onc/onc_constants.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 namespace {
@@ skipping 215 matching lines @@
 
   VLOG(2) << "SetProperties: Found GUID " << guid << " and profile "
           << profile->ToDebugString();
 
   const Policies* policies = GetPoliciesForProfile(*profile);
   if (!policies) {
     InvokeErrorCallback(service_path, error_callback, kPoliciesNotInitialized);
     return;
   }
 
+  // We need to ensure that required configuration properties (e.g. Type) are
+  // included for ONC validation.
+  scoped_ptr<base::DictionaryValue> user_settings_copy(
+      network_util::TranslateNetworkConfigurationStateToONC(state));

[pneubeck (no reviews), 2014/11/11 16:09:31]

I think this is solution is not ideal yet.
We need to merge only the bare minimum that makes the dictionary parseable.
Apart from 'Type' nothing even comes to my mind. If you add something like
EAPMethod, then the UI cannot unset that value anymore because it's always
filled by this code.

Alternatively, the UI could also set the type itself.

[stevenjb, 2014/11/12 01:23:29]

Hmm. I don't like the idea of forcing the UI to specify a particular subset just
so that the ONC validation code can run correctly. We use this method in
settings e.g. just to set the Priority or APN.

This should already be a nearly minimal subset. Type is definitely needed.
Security at least produces warnings if missing.

Since we merge in the user settings, any of them could be overridden. To 'unset'
a property we set it to 'null', so the merge will still override any properties
that are explicitly 'unset'.

If this really bothers you I'll do the explicit conversion of just 'Type' (which
I know we need, I learned that the hard way), but the validation code is still a
bit opaque to me, so if you could at least confirm that we don't need anything
else I would appreciate it.

[pneubeck (no reviews), 2014/11/13 17:38:56]

Just looked at the validator, and I actually think that NOTREACHED that you ran
into is wrong.
The validator should never crash because of the dictionary that it verifies and
also should verify and repair as much as it can.

I uploaded a CL, afterwards should not have to merge anything.

+  user_settings_copy->MergeDictionary(&user_settings);
+
   // Validate the ONC dictionary. We are liberal and ignore unknown field
   // names. User settings are only partial ONC, thus we ignore missing fields.
   onc::Validator validator(false,  // Ignore unknown fields.
                            false,  // Ignore invalid recommended field names.
                            false,  // Ignore missing fields.
                            false);  // This ONC does not come from policy.
 
   onc::Validator::Result validation_result;
   scoped_ptr<base::DictionaryValue> validated_user_settings =
       validator.ValidateAndRepairObject(
           &onc::kNetworkConfigurationSignature,
-          user_settings,
+          *user_settings_copy,
           &validation_result);
 
   if (validation_result == onc::Validator::INVALID) {
     InvokeErrorCallback(service_path, error_callback, kInvalidUserSettings);
     return;
   }
   if (validation_result == onc::Validator::VALID_WITH_WARNINGS)
     LOG(WARNING) << "Validation of ONC user settings produced warnings.";
 
   const base::DictionaryValue* network_policy =
@@ skipping 468 matching lines @@
     scoped_ptr<base::DictionaryValue> network_properties,
     GetDevicePropertiesCallback send_callback,
     const std::string& error_name,
     scoped_ptr<base::DictionaryValue> error_data) {
   NET_LOG_ERROR("Error getting device properties", service_path);
   send_callback.Run(service_path, network_properties.Pass());
 }
 
 
 }  // namespace chromeos