Improve Cellular support in networkingPrivate

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 <vector>
 
 #include "base/bind.h"
 #include "base/guid.h"
@@ skipping 58 matching lines @@
 }
 
 void LogErrorWithDict(const tracked_objects::Location& from_where,
                       const std::string& error_name,
                       scoped_ptr<base::DictionaryValue> error_data) {
   device_event_log::AddEntry(from_where.file_name(), from_where.line_number(),
                              device_event_log::LOG_TYPE_NETWORK,
                              device_event_log::LOG_LEVEL_ERROR, error_name);
 }
 
-const base::DictionaryValue* GetByGUID(const GuidToPolicyMap& policies,
-                                       const std::string& guid) {
+const base::DictionaryValue* GetPolicyByGUID(const GuidToPolicyMap& policies,
+                                             const std::string& guid) {
   GuidToPolicyMap::const_iterator it = policies.find(guid);
   if (it == policies.end())
     return NULL;
   return it->second;
 }
 
+scoped_ptr<base::DictionaryValue> ExtractShillDeviceSettings(
+    base::DictionaryValue* user_settings) {
+  scoped_ptr<base::DictionaryValue> shill_device_settings(
+      new base::DictionaryValue);
+  // Only extract specific properties that can be set in Shill.
+  base::DictionaryValue* cellular;
+  if (user_settings->GetDictionaryWithoutPathExpansion(
+          ::onc::network_config::kCellular, &cellular)) {
+    scoped_ptr<base::Value> value;
+    if (cellular->RemoveWithoutPathExpansion(::onc::cellular::kCarrier,
+                                             &value)) {
+      shill_device_settings->SetWithoutPathExpansion(shill::kCarrierProperty,
+                                                     value.Pass());
+    }
+    if (cellular->RemoveWithoutPathExpansion(::onc::cellular::kAllowRoaming,
+                                             &value)) {
+      shill_device_settings->SetWithoutPathExpansion(
+          shill::kCellularAllowRoamingProperty, value.Pass());
+    }
+  }
+  return shill_device_settings.Pass();
+}
+
 }  // namespace
 
 struct ManagedNetworkConfigurationHandlerImpl::Policies {
   ~Policies();
 
   GuidToPolicyMap per_network_config;
   base::DictionaryValue global_network_config;
 };
 
 ManagedNetworkConfigurationHandlerImpl::Policies::~Policies() {
@@ skipping 74 matching lines @@
   const base::DictionaryValue* network_policy = NULL;
   const base::DictionaryValue* global_policy = NULL;
   if (profile) {
     const Policies* policies = GetPoliciesForProfile(*profile);
     if (!policies) {
       InvokeErrorCallback(
           service_path, error_callback, kPoliciesNotInitialized);
       return;
     }
     if (!guid.empty())
-      network_policy = GetByGUID(policies->per_network_config, guid);
+      network_policy = GetPolicyByGUID(policies->per_network_config, guid);
     global_policy = &policies->global_network_config;
   }
 
   scoped_ptr<base::DictionaryValue> augmented_properties(
       policy_util::CreateManagedONC(global_policy,
                                     network_policy,
                                     user_settings,
                                     active_settings.get(),
                                     profile));
   callback.Run(service_path, *augmented_properties);
@@ skipping 27 matching lines @@
           &onc::kNetworkWithStateSignature));
   callback.Run(service_path, *onc_network);
 }
 
 // SetProperties
 
 void ManagedNetworkConfigurationHandlerImpl::SetProperties(
     const std::string& service_path,
     const base::DictionaryValue& user_settings,
     const base::Closure& callback,
-    const network_handler::ErrorCallback& error_callback) const {
+    const network_handler::ErrorCallback& error_callback) {
   const NetworkState* state =
       network_state_handler_->GetNetworkStateFromServicePath(
           service_path, true /* configured_only */);
   if (!state) {
     InvokeErrorCallback(service_path, error_callback, kUnknownNetwork);
     return;
   }
 
   std::string guid = state->guid();
   DCHECK(!guid.empty());
@@ skipping 20 matching lines @@
 
   // We need to ensure that required configuration properties (e.g. Type) are
   // included for ONC validation and translation to Shill properties.
   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);
 
+  // Extract any Device properties to apply separately, before validation.

[pneubeck (no reviews), 2015/03/24 16:19:52]

any reason to do this before validation?
So far, all details of validation were separated out into the validator.
I'd like to avoid any adhoc translatin/validation code accumulating in this
class (which is already too large).

+  scoped_ptr<base::DictionaryValue> shill_device_dictionary(
+      ExtractShillDeviceSettings(user_settings_copy.get()));
+
   // 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_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.";
 
   // Fill in HexSSID field from contents of SSID field if not set already.
-  if (user_settings_copy) {
-    onc::FillInHexSSIDFieldsInOncObject(onc::kNetworkConfigurationSignature,
-                                        validated_user_settings.get());
-  }
+  onc::FillInHexSSIDFieldsInOncObject(onc::kNetworkConfigurationSignature,
+                                      validated_user_settings.get());
 
   const base::DictionaryValue* network_policy =
-      GetByGUID(policies->per_network_config, guid);
+      GetPolicyByGUID(policies->per_network_config, guid);
   VLOG(2) << "This configuration is " << (network_policy ? "" : "not ")
           << "managed.";
 
   scoped_ptr<base::DictionaryValue> shill_dictionary(
       policy_util::CreateShillConfiguration(*profile,
                                             guid,
                                             &policies->global_network_config,
                                             network_policy,
                                             validated_user_settings.get()));
 
+  if (!shill_device_dictionary->empty() && network_device_handler_) {
+    SetShillDeviceProperties(
+        state->device_path(), shill_device_dictionary.Pass(),
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillServiceProperties,
+            weak_ptr_factory_.GetWeakPtr(), service_path,
+            base::Passed(&shill_dictionary), callback, error_callback),
+        error_callback);
+  } else {
+    SetShillServiceProperties(service_path, shill_dictionary.Pass(), callback,
+                              error_callback);
+  }
+}
+
+void ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties(

[pneubeck (no reviews), 2015/03/24 16:19:52]

this should really, really move to the NetworkDeviceHandler

ideally we could avoid this complexity by also having a SetDeviceProperties
exposed by Shill (at least that should be filed as a bug). This must be covered
by a test.

+    const std::string& device_path,
+    scoped_ptr<base::DictionaryValue> shill_device_dictionary,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
+  // Note: The 'Remove' calls here are crucial to ensure we do not start an
+  // infinte callback chain.
+  scoped_ptr<base::Value> shill_value;
+  if (shill_device_dictionary->RemoveWithoutPathExpansion(
+          shill::kCarrierProperty, &shill_value)) {
+    std::string carrier;
+    shill_value->GetAsString(&carrier);
+    NET_LOG(USER) << "SetCarrier: " << device_path << ": " << carrier;
+    network_device_handler_->SetCarrier(
+        device_path, carrier,
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties,
+            weak_ptr_factory_.GetWeakPtr(), device_path,
+            base::Passed(&shill_device_dictionary), callback, error_callback),
+        error_callback);
+    return;
+  }
+  if (shill_device_dictionary->empty()) {
+    callback.Run();
+    return;
+  }
+  // We have to set Device properites one at a time, but setting them is rare.
+  base::DictionaryValue::Iterator iter(*shill_device_dictionary);
+  std::string key = iter.key();
+  if (shill_device_dictionary->RemoveWithoutPathExpansion(key, &shill_value)) {
+    NET_LOG(USER) << "SetDeviceProperty: " << device_path << ": " << key << "="
+                  << *shill_value;
+    network_device_handler_->SetDeviceProperty(
+        device_path, key, *shill_value,
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties,
+            weak_ptr_factory_.GetWeakPtr(), device_path,
+            base::Passed(&shill_device_dictionary), callback, error_callback),
+        error_callback);
+  }
+}
+
+void ManagedNetworkConfigurationHandlerImpl::SetShillServiceProperties(
+    const std::string& service_path,
+    scoped_ptr<base::DictionaryValue> shill_service_dictionary,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
   network_configuration_handler_->SetShillProperties(
-      service_path, *shill_dictionary,
+      service_path, *shill_service_dictionary,
       NetworkConfigurationObserver::SOURCE_USER_ACTION, callback,
       error_callback);
 }
 
 void ManagedNetworkConfigurationHandlerImpl::CreateConfiguration(
     const std::string& userhash,
     const base::DictionaryValue& properties,
     const network_handler::StringResultCallback& callback,
     const network_handler::ErrorCallback& error_callback) const {
   const Policies* policies = GetPoliciesForUser(userhash);
@@ skipping 245 matching lines @@
 ManagedNetworkConfigurationHandlerImpl::FindPolicyByGUID(
     const std::string userhash,
     const std::string& guid,
     ::onc::ONCSource* onc_source) const {
   *onc_source = ::onc::ONC_SOURCE_NONE;
 
   if (!userhash.empty()) {
     const Policies* user_policies = GetPoliciesForUser(userhash);
     if (user_policies) {
       const base::DictionaryValue* policy =
-          GetByGUID(user_policies->per_network_config, guid);
+          GetPolicyByGUID(user_policies->per_network_config, guid);
       if (policy) {
         *onc_source = ::onc::ONC_SOURCE_USER_POLICY;
         return policy;
       }
     }
   }
 
   const Policies* device_policies = GetPoliciesForUser(std::string());
   if (device_policies) {
     const base::DictionaryValue* policy =
-        GetByGUID(device_policies->per_network_config, guid);
+        GetPolicyByGUID(device_policies->per_network_config, guid);
     if (policy) {
       *onc_source = ::onc::ONC_SOURCE_DEVICE_POLICY;
       return policy;
     }
   }
 
   return NULL;
 }
 
 const GuidToPolicyMap*
@@ skipping 24 matching lines @@
       network_profile_handler_->GetProfileForPath(profile_path);
   if (!profile) {
     NET_LOG_ERROR("Profile path unknown:" + profile_path, guid);
     return NULL;
   }
 
   const Policies* policies = GetPoliciesForProfile(*profile);
   if (!policies)
     return NULL;
 
-  return GetByGUID(policies->per_network_config, guid);
+  return GetPolicyByGUID(policies->per_network_config, guid);
 }
 
 const ManagedNetworkConfigurationHandlerImpl::Policies*
 ManagedNetworkConfigurationHandlerImpl::GetPoliciesForUser(
     const std::string& userhash) const {
   UserToPoliciesMap::const_iterator it = policies_by_user_.find(userhash);
   if (it == policies_by_user_.end())
     return NULL;
   return it->second.get();
 }
@@ skipping 159 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