Add 'setProperties' to InternetOptionsHandler

chromeos/network/managed_network_configuration_handler_impl.cc

Index: chromeos/network/managed_network_configuration_handler_impl.cc
diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index b47f2dd9b216a89fc3876014d53771d93de0bbae..c02a714c2f7baf4cad1592fe3b782f128aec20f8 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -30,6 +30,7 @@
 #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"
@@ -265,6 +266,12 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
     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.
@@ -276,7 +283,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
   scoped_ptr<base::DictionaryValue> validated_user_settings =
       validator.ValidateAndRepairObject(
           &onc::kNetworkConfigurationSignature,
-          user_settings,
+          *user_settings_copy,
           &validation_result);
 
   if (validation_result == onc::Validator::INVALID) {