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 10 matching lines @@
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/network_configuration_handler.h"
 #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/network_util.h"
 #include "chromeos/network/onc/onc_merger.h"
 #include "chromeos/network/onc/onc_signature.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 {
@@ skipping 177 matching lines @@
     scoped_ptr<base::DictionaryValue> shill_properties) {
   scoped_ptr<base::DictionaryValue> onc_network(
       onc::TranslateShillServiceToONCPart(
           *shill_properties, ::onc::ONC_SOURCE_UNKNOWN,
           &onc::kNetworkWithStateSignature));
   callback.Run(service_path, *onc_network);
 }
 
 // SetProperties
 
 void ManagedNetworkConfigurationHandlerImpl::SetProperties(

[pneubeck (no reviews), 2014/11/18 13:21:38]

since you seem to prefer SetProperties to do a merge of settings, we should
merge with the old user_settings.

I'll prepare a CL where we can discuss that.

[stevenjb, 2014/11/18 23:12:06]

I'm not sure I understand what you mean.

If by "merge" you mean "send a partial set of properties to change" then yes,
that is what I prefer because:
a) It means the UI does not need to keep track of all current properties (which
has all sorts of edge cases if two settings apps are running).
b) Requiring a "complete" set of properties is misleading because some
properties like Passphrase are not provided to the UI and so must be optional.

We could certainly address those issues, but I am unclear what the benefit would
be.

Perhaps we should discuss in email before you put up a CL (it is unclear to me
what that CL would be?)

     const std::string& service_path,
     const base::DictionaryValue& user_settings,
     const base::Closure& callback,
     const network_handler::ErrorCallback& error_callback) const {
   const NetworkState* state =
       network_state_handler_->GetNetworkStateFromServicePath(
           service_path, true /* configured_only */);
   if (!state) {
     InvokeErrorCallback(service_path, error_callback, kUnknownNetwork);
     return;
@@ skipping 19 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.

[pneubeck (no reviews), 2014/11/18 13:21:38]

as you mentioned in your review-comment you need it (also or solely) for the
ONC->Shill translation later on.
please mention that in this comment.

[stevenjb, 2014/11/18 23:12:06]

Sorry, I assumed it was the validation that was failing not the translation. I
will update the comment.

[pneubeck (no reviews), 2014/11/19 06:59:39]

I guess, originally you ran into issues with the validation. But after the
validation fix, it's the translation :-)

+  scoped_ptr<base::DictionaryValue> user_settings_copy(
+      user_settings.DeepCopy());
+  user_settings_copy->SetStringWithoutPathExpansion(
+      ::onc::network_config::kType,
+      network_util::TranslateShillTypeToONC(state->type()));
+  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 421 matching lines @@
   properties->SetWithoutPathExpansion(shill::kIPConfigsProperty, ip_configs);
 }
 
 void ManagedNetworkConfigurationHandlerImpl::GetPropertiesCallback(
     GetDevicePropertiesCallback send_callback,
     const std::string& service_path,
     const base::DictionaryValue& shill_properties) {
   scoped_ptr<base::DictionaryValue> shill_properties_copy(
       shill_properties.DeepCopy());
 
-  // Add associated Device properties before the ONC translation.
-  GetDeviceStateProperties(service_path, shill_properties_copy.get());
+  std::string type;
+  shill_properties_copy->GetStringWithoutPathExpansion(shill::kTypeProperty,
+                                                       &type);
+  // Add associated DeviceState properties for non-VPN networks.
+  if (type != shill::kTypeVPN)
+    GetDeviceStateProperties(service_path, shill_properties_copy.get());
 
-  // Only request Device properties for Cellular networks with a valid device.
-  std::string type, device_path;
+  // Only request additional Device properties for Cellular networks with a
+  // valid device.
+  std::string device_path;
   if (!network_device_handler_ ||
-      !shill_properties_copy->GetStringWithoutPathExpansion(
-          shill::kTypeProperty, &type) ||
       type != shill::kTypeCellular ||
       !shill_properties_copy->GetStringWithoutPathExpansion(
           shill::kDeviceProperty, &device_path) ||
       device_path.empty()) {
     send_callback.Run(service_path, shill_properties_copy.Pass());
     return;
   }
 
   // Request the device properties. On success or failure pass (a possibly
   // modified) |shill_properties| to |send_callback|.
@@ skipping 32 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