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..6f411446833e2744e7e9945e7500c8ea92f5eb24 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,8 +119,6 @@ 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::kErrorUnknown[] = "unknown-error";
@@ -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;
@@ -347,9 +340,11 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
     const base::DictionaryValue& service_properties) {
   NET_LOG_EVENT("VerifyConfiguredAndConnect", service_path);
 
-  std::string type;
+  std::string type, security;
   service_properties.GetStringWithoutPathExpansion(
       flimflam::kTypeProperty, &type);
+  service_properties.GetStringWithoutPathExpansion(
+      flimflam::kSecurityProperty, &security);
 
   if (check_error_state) {
     std::string error;
@@ -381,14 +376,35 @@ 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) {
+    const base::DictionaryValue* provider_properties;
+    if (service_properties.GetDictionaryWithoutPathExpansion(
+            flimflam::kProviderProperty, &provider_properties)) {
+      provider_properties->GetStringWithoutPathExpansion(
+          flimflam::kTypeProperty, &vpn_provider_type);
+      provider_properties->GetStringWithoutPathExpansion(

[gauravsh, 2013/08/02 23:40:22]

Ok if there's no vpn_provider_host?

[stevenjb, 2013/08/06 00:32:48]

Probably not. Fixed.

+          flimflam::kHostProperty, &vpn_provider_host);
+    }
+    if (vpn_provider_type.empty()) {
+      ErrorCallbackForPendingRequest(service_path, kErrorMissingProviderType);
+      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.
+  base::DictionaryValue config_properties;
+
+  // 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 +413,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 +425,76 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       return;
     }
 
-    // Ensure the certificate is available.
+    // Ensure the certificate is available and configured.
     std::string pkcs11_id;
     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);
+    if (type == flimflam::kTypeVPN) {

[gauravsh, 2013/08/02 23:40:22]

Lets make all property settings for a given type-vpn provider be in a single
place (like in the previous code). Even though that means duplicating the if
(cert_loader_) check, it's more readable and scan that all required properties
are correctly being set.

// Configure tpm slots/pins if needed.
string tpm_slot = ...
string tpm_pin = ...

// Set properties
if (type == flimflam::kTypeVPN) {
   if (vpn_provider_type == flimflam::kProviderOpenVpn) {
    // set all required config_properties for OpenVPN type.
    // set properties needing tpm if cert_loader_
   } else {
    // set all required config_properties for L2IPSec type.
    // set properties needing tpm if cert_loader_
   }
} else if (type == flimflam::kTypeWifi) {
  // set all required config_properties for 8021x
  // set properties needing tpm if cert_loader_
}

[stevenjb, 2013/08/06 00:32:48]

Yeah, getting this all right was a bit tricky, bit I should be able to do this
by setting strings first then checking if they are empty.

+      if (vpn_provider_type == flimflam::kProviderOpenVpn) {
+        config_properties.SetStringWithoutPathExpansion(
+            flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
+      } else {
+        config_properties.SetStringWithoutPathExpansion(
+            flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
+      }
+    } else if (type == flimflam::kTypeWifi) {
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kEapCertIdProperty, pkcs11_id);
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kEapKeyIdProperty, pkcs11_id);
+    }
+  }
+
+  // The network may not be 'Connectable' because the TPM properties are
+  // not set up, so configure tpm slot/pin before connecting.
+  if (cert_loader_ &&
+      (type == flimflam::kTypeVPN ||
+       (type == flimflam::kTypeWifi && security == flimflam::kSecurity8021x))) {
     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);
-
     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) {
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertSlotProperty, tpm_slot);
+      if (vpn_provider_type == flimflam::kProviderOpenVpn) {
+        // config_properties.SetStringWithoutPathExpansion(
+        //     flimflam::kOpenVPNClientCertSlotProperty, tpm_slot);
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kOpenVPNPinProperty, tpm_pin);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
       } else {
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kL2tpIpsecClientCertSlotProperty, tpm_slot);
         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());
+    }
+  }
+
+  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::kEapCertIdProperty, pkcs11_id);
+          flimflam::kProviderTypeProperty, vpn_provider_type);
       config_properties.SetStringWithoutPathExpansion(
-          flimflam::kEapKeyIdProperty, pkcs11_id);
+          flimflam::kProviderHostProperty, vpn_provider_host);
+    } else if (type == flimflam::kTypeWifi) {
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kSecurityProperty, security);
     }
+
     network_configuration_handler_->SetProperties(
         service_path,
         config_properties,
@@ -480,8 +505,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 +658,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 +669,29 @@ void NetworkConnectionHandler::HandleShillDisconnectSuccess(
     success_callback.Run();
 }
 
+// Activate
+
+void NetworkConnectionHandler::CallShillActivate(
+    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