Convert all connect code to use NetworkHandler instead of NetworkLibrary

chromeos/network/network_connection_handler.cc

Index: chromeos/network/network_connection_handler.cc
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 66e8d4fc4356a46466c07d010df7796da9c31b76..f7ec8932948ed9095cd386605c78fb1966af94ef 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -62,56 +62,40 @@ bool NetworkRequiresActivation(const NetworkState* network) {
 }
 
 bool VPNIsConfigured(const std::string& service_path,
-                     const base::DictionaryValue& service_properties) {
-  // VPN Provider values are read from the "Provider" dictionary, not the
-  // "Provider.Type", etc keys (which are used only to set the values).
-  const base::DictionaryValue* provider_properties;
-  if (!service_properties.GetDictionaryWithoutPathExpansion(
-          flimflam::kProviderProperty, &provider_properties)) {
-    NET_LOG_ERROR("VPN Provider Dictionary not present", service_path);
-    return false;
-  }
-  std::string provider_type;
-  if (!provider_properties->GetStringWithoutPathExpansion(
-          flimflam::kTypeProperty, &provider_type)) {
-    NET_LOG_ERROR("VPN Provider Type not present", service_path);
-    return false;
-  }
+                     const std::string& provider_type,
+                     const base::DictionaryValue& provider_properties) {
   if (provider_type == flimflam::kProviderOpenVpn) {
     std::string hostname;
-    provider_properties->GetStringWithoutPathExpansion(
+    provider_properties.GetStringWithoutPathExpansion(
         flimflam::kHostProperty, &hostname);
     if (hostname.empty()) {
       NET_LOG_EVENT("OpenVPN: No hostname", service_path);
       return false;
     }
     std::string username;
-    provider_properties->GetStringWithoutPathExpansion(
+    provider_properties.GetStringWithoutPathExpansion(
         flimflam::kOpenVPNUserProperty, &username);
     if (username.empty()) {
       NET_LOG_EVENT("OpenVPN: No username", service_path);
       return false;
     }
     bool passphrase_required = false;
-    provider_properties->GetBooleanWithoutPathExpansion(
+    provider_properties.GetBooleanWithoutPathExpansion(
         flimflam::kPassphraseRequiredProperty, &passphrase_required);
-    std::string passphrase;
-    provider_properties->GetStringWithoutPathExpansion(
-        flimflam::kOpenVPNPasswordProperty, &passphrase);
-    if (passphrase_required && passphrase.empty()) {
-      NET_LOG_EVENT("OpenVPN: No passphrase", service_path);
+    if (passphrase_required) {
+      NET_LOG_EVENT("OpenVPN: Passphrase Required", service_path);
       return false;
     }
     NET_LOG_EVENT("OpenVPN Is Configured", service_path);
   } else {
     bool passphrase_required = false;
     std::string passphrase;
-    provider_properties->GetBooleanWithoutPathExpansion(
+    provider_properties.GetBooleanWithoutPathExpansion(
         flimflam::kL2tpIpsecPskRequiredProperty, &passphrase_required);
-    provider_properties->GetStringWithoutPathExpansion(
-        flimflam::kL2tpIpsecPskProperty, &passphrase);
-    if (passphrase_required && passphrase.empty())
+    if (passphrase_required) {
+      NET_LOG_EVENT("VPN: Passphrase Required", service_path);
       return false;
+    }
     NET_LOG_EVENT("VPN Is Configured", service_path);
   }
   return true;
@@ -135,10 +119,8 @@ const char NetworkConnectionHandler::kErrorAuthenticationRequired[] =
     "authentication-required";
 const char NetworkConnectionHandler::kErrorShillError[] = "shill-error";
 const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
-const char NetworkConnectionHandler::kErrorDisconnectFailed[] =
-    "disconnect-failed";
-const char NetworkConnectionHandler::kErrorMissingProviderType[] =
-    "missing-provider-type";
+const char NetworkConnectionHandler::kErrorMissingProvider[] =
+    "missing-provider";
 const char NetworkConnectionHandler::kErrorUnknown[] = "unknown-error";
 
 struct NetworkConnectionHandler::ConnectRequest {
@@ -203,10 +185,6 @@ void NetworkConnectionHandler::Init(
 
 void NetworkConnectionHandler::LoggedInStateChanged(
     LoginState::LoggedInState state) {
-  NET_LOG_EVENT("NewNetworkConnectionHandler",
-                CommandLine::ForCurrentProcess()->HasSwitch(
-                    chromeos::switches::kUseNewNetworkConnectionHandler) ?
-                "enabled" : "disabled");
   if (state == LoginState::LOGGED_IN_ACTIVE) {
     logged_in_ = true;
     NET_LOG_EVENT("Logged In", "");
@@ -316,6 +294,21 @@ void NetworkConnectionHandler::DisconnectNetwork(
   CallShillDisconnect(service_path, success_callback, error_callback);
 }
 
+void NetworkConnectionHandler::ActivateNetwork(
+    const std::string& service_path,
+    const std::string& carrier,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  NET_LOG_USER("DisconnectNetwork", service_path);
+  const NetworkState* network =
+      network_state_handler_->GetNetworkState(service_path);
+  if (!network) {
+    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
+    return;
+  }
+  CallShillActivate(service_path, carrier, success_callback, error_callback);
+}
+
 bool NetworkConnectionHandler::HasConnectingNetwork(
     const std::string& service_path) {
   return pending_requests_.count(service_path) != 0;
@@ -381,14 +374,39 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
     return;
   }
 
-  // VPN requires a host and username to be set.
-  if (type == flimflam::kTypeVPN &&
-      !VPNIsConfigured(service_path, service_properties)) {
-    ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
-    return;
+  // Get VPN provider type and host (required for configuration) and ensure
+  // that required VPN non-cert properties are set.
+  std::string vpn_provider_type, vpn_provider_host;
+  if (type == flimflam::kTypeVPN) {
+    // VPN Provider values are read from the "Provider" dictionary, not the
+    // "Provider.Type", etc keys (which are used only to set the values).
+    const base::DictionaryValue* provider_properties;
+    if (service_properties.GetDictionaryWithoutPathExpansion(
+            flimflam::kProviderProperty, &provider_properties)) {
+      provider_properties->GetStringWithoutPathExpansion(
+          flimflam::kTypeProperty, &vpn_provider_type);
+      provider_properties->GetStringWithoutPathExpansion(
+          flimflam::kHostProperty, &vpn_provider_host);
+    }
+    if (vpn_provider_type.empty() || vpn_provider_host.empty()) {
+      ErrorCallbackForPendingRequest(service_path, kErrorMissingProvider);
+      return;
+    }
+    // VPN requires a host and username to be set.
+    if (!VPNIsConfigured(
+            service_path, vpn_provider_type, *provider_properties)) {
+      NET_LOG_ERROR("VPN Not Configured", service_path);
+      ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+      return;
+    }
   }
 
-  // Check certificate properties in kUIDataProperty.
+  // These will be set if they need to be configured, otherwise they will
+  // be left empty and the properties will not be set.
+  std::string pkcs11_id, tpm_slot, tpm_pin;
+
+  // Check certificate properties in kUIDataProperty if configured.
+  // Note: Wifi/VPNConfigView set these properties explicitly.
   scoped_ptr<NetworkUIData> ui_data =
       ManagedNetworkConfigurationHandler::GetUIData(service_properties);
   if (ui_data && ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
@@ -397,6 +415,7 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
       return;
     }
+
     // If certificates have not been loaded yet, queue the connect request.
     if (!certificates_loaded_) {
       ConnectRequest* request = pending_request(service_path);
@@ -408,68 +427,79 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       return;
     }
 
-    // Ensure the certificate is available.
-    std::string pkcs11_id;
+    // Ensure the certificate is available and configured.
     if (!CertificateIsConfigured(ui_data.get(), &pkcs11_id)) {
       ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
       return;
     }
+  }
 
-    // The network may not be 'Connectable' because the certificate data is
-    // not set up, so configure tpm slot/pin and pkcs11_id before connecting.
-    // TODO(stevenjb): Remove this code once NetworkConfigurationHandler
-    // handles this.
-    NET_LOG_EVENT("Configuring Network", service_path);
-    const std::string& tpm_slot = cert_loader_->tpm_token_slot();
-    const std::string& tpm_pin = cert_loader_->tpm_user_pin();
-    base::DictionaryValue config_properties;
-    // Set configuration properties required by Shill to identify the network.
-    config_properties.SetStringWithoutPathExpansion(
-        flimflam::kTypeProperty, type);
-    CopyStringFromDictionary(service_properties, flimflam::kNameProperty,
-                             &config_properties);
-    CopyStringFromDictionary(service_properties, flimflam::kSecurityProperty,
-                             &config_properties);
-    CopyStringFromDictionary(service_properties, flimflam::kGuidProperty,
-                             &config_properties);
+  // The network may not be 'Connectable' because the TPM properties are
+  // not set up, so configure tpm slot/pin before connecting.
+  if (cert_loader_) {
+    tpm_slot = cert_loader_->tpm_token_slot();
+    tpm_pin = cert_loader_->tpm_user_pin();
+  }
 
-    if (type == flimflam::kTypeVPN) {
-      // VPN Provider values are read from the "Provider" dictionary, not the
-      // "Provider.Type", etc keys (which are used only to set the values).
-      std::string provider_type;
-      const base::DictionaryValue* provider_properties;
-      if (service_properties.GetDictionaryWithoutPathExpansion(
-              flimflam::kProviderProperty, &provider_properties)) {
-        provider_properties->GetStringWithoutPathExpansion(
-            flimflam::kTypeProperty, &provider_type);
-      }
-      if (provider_type.empty()) {
-        ErrorCallbackForPendingRequest(service_path, kErrorMissingProviderType);
-        return;
-      }
-      if (provider_type == flimflam::kProviderOpenVpn) {
+  base::DictionaryValue config_properties;
+
+  if (type == flimflam::kTypeVPN) {
+    if (vpn_provider_type == flimflam::kProviderOpenVpn) {
+      if (!pkcs11_id.empty()) {
         config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertSlotProperty, tpm_slot);
+            flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
+      }
+      if (!tpm_pin.empty()) {
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kOpenVPNPinProperty, tpm_pin);
+      }
+    } else {
+      if (!pkcs11_id.empty()) {
         config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
-      } else {
+            flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
+      }
+      if (!tpm_slot.empty()) {
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kL2tpIpsecClientCertSlotProperty, tpm_slot);
+      }
+      if (!tpm_pin.empty()) {
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kL2tpIpsecPinProperty, tpm_pin);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
       }
-    } else if (type == flimflam::kTypeWifi) {
-      config_properties.SetStringWithoutPathExpansion(
-          flimflam::kEapPinProperty, cert_loader_->tpm_user_pin());
+    }
+  } else if (type == flimflam::kTypeWifi) {
+    if (!pkcs11_id.empty()) {
       config_properties.SetStringWithoutPathExpansion(
           flimflam::kEapCertIdProperty, pkcs11_id);
       config_properties.SetStringWithoutPathExpansion(
           flimflam::kEapKeyIdProperty, pkcs11_id);
     }
+    if (!tpm_pin.empty()) {
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kEapPinProperty, tpm_pin);
+    }
+  }
+
+  if (!config_properties.empty()) {
+    NET_LOG_EVENT("Configuring Network", service_path);
+
+    // Set configuration properties required by Shill to identify the network.
+    config_properties.SetStringWithoutPathExpansion(
+        flimflam::kTypeProperty, type);
+    CopyStringFromDictionary(service_properties, flimflam::kNameProperty,
+                             &config_properties);
+    CopyStringFromDictionary(service_properties, flimflam::kGuidProperty,
+                             &config_properties);
+    if (type == flimflam::kTypeVPN) {
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kProviderTypeProperty, vpn_provider_type);
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kProviderHostProperty, vpn_provider_host);
+    } else if (type == flimflam::kTypeWifi) {
+      CopyStringFromDictionary(service_properties, flimflam::kSecurityProperty,
+                               &config_properties);
+    }
+
     network_configuration_handler_->SetProperties(
         service_path,
         config_properties,
@@ -480,8 +510,14 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
    return;
   }
 
-  // Otherwise, we need to configure the network.
-  ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+  // Otherwise, we probably still need to configure the network since
+  // 'Connectable' is false. If |check_error_state| is true, signal an
+  // error, otherwise attempt to connect to possibly gain additional error
+  // state from Shill (or in case 'Connectable' is improperly unset).
+  if (check_error_state)
+    ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+  else
+    CallShillConnect(service_path);
 }
 
 void NetworkConnectionHandler::CallShillConnect(
@@ -627,7 +663,7 @@ void NetworkConnectionHandler::CallShillDisconnect(
       base::Bind(&NetworkConnectionHandler::HandleShillDisconnectSuccess,
                  AsWeakPtr(), service_path, success_callback),
       base::Bind(&network_handler::ShillErrorCallbackFunction,
-                 kErrorDisconnectFailed, service_path, error_callback));
+                 kErrorShillError, service_path, error_callback));
 }
 
 void NetworkConnectionHandler::HandleShillDisconnectSuccess(
@@ -638,4 +674,29 @@ void NetworkConnectionHandler::HandleShillDisconnectSuccess(
     success_callback.Run();
 }
 
+// Activate
+
+void NetworkConnectionHandler::CallShillActivate(

[gauravsh, 2013/08/07 23:33:37]

Talking to Arman, it looks like this would be a better fit for a separate
NetworkActivationHandler which lets us make activation related shill calls and
manage activation state. (The WIP MobileActivator change I have needs this and
CompleteCellularActivation as well).

Since connection and activation are very different things, I am inclined to
agree with that as well.

[stevenjb, 2013/08/08 01:02:51]

That sounds reasonable to me, although I am hesitant to add that to this CL
also. Would you be OK with adding a TODO to move this later?

+    const std::string& service_path,
+    const std::string& carrier,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  NET_LOG_USER("Activate Request", service_path + ": '" + carrier + "'");
+  DBusThreadManager::Get()->GetShillServiceClient()->ActivateCellularModem(
+      dbus::ObjectPath(service_path),
+      carrier,
+      base::Bind(&NetworkConnectionHandler::HandleShillActivateSuccess,
+                 AsWeakPtr(), service_path, success_callback),
+      base::Bind(&network_handler::ShillErrorCallbackFunction,
+                 kErrorShillError, service_path, error_callback));
+}
+
+void NetworkConnectionHandler::HandleShillActivateSuccess(
+    const std::string& service_path,
+    const base::Closure& success_callback) {
+  NET_LOG_EVENT("Activate Request Sent", service_path);
+  if (!success_callback.is_null())
+    success_callback.Run();
+}
+
 }  // namespace chromeos