Handle connecting to hidden networks correctly

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 08d233e19557e4b37564746528961a99d4384868..ad7648b469b41ef9c8a945f6b49cd32b8fffa4ca 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -231,43 +231,45 @@ void NetworkConnectionHandler::ConnectToNetwork(
 
   // Check cached network state for connected, connecting, or unactivated
   // networks. These states will not be affected by a recent configuration.
+  // Note: NetworkState may not exist for a network that was recently
+  // configured, in which case these checks do not apply anyway.
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
-  if (!network) {
-    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
-    return;
-  }
-  if (network->IsConnectedState()) {
-    InvokeErrorCallback(service_path, error_callback, kErrorConnected);
-    return;
-  }
-  if (network->IsConnectingState()) {
-    InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
-    return;
-  }
-  if (NetworkRequiresActivation(network)) {
-    InvokeErrorCallback(service_path, error_callback, kErrorActivationRequired);
-    return;
-  }
 
-  if (check_error_state) {
-    const std::string& error = network->error();
-    if (error == flimflam::kErrorConnectFailed) {
-      InvokeErrorCallback(
-          service_path, error_callback, kErrorPassphraseRequired);
+  if (network) {
+    // For existing networks, perform some immediate consistency checks.
+    if (network->IsConnectedState()) {
+      InvokeErrorCallback(service_path, error_callback, kErrorConnected);
       return;
     }
-    if (error == flimflam::kErrorBadPassphrase) {
-      InvokeErrorCallback(
-          service_path, error_callback, kErrorPassphraseRequired);
+    if (network->IsConnectingState()) {
+      InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
       return;
     }
-
-    if (IsAuthenticationError(error)) {
-      InvokeErrorCallback(
-          service_path, error_callback, kErrorAuthenticationRequired);
+    if (NetworkRequiresActivation(network)) {
+      InvokeErrorCallback(service_path, error_callback,
+                          kErrorActivationRequired);
       return;
     }
+
+    if (check_error_state) {
+      const std::string& error = network->error();
+      if (error == flimflam::kErrorConnectFailed) {
+        InvokeErrorCallback(
+            service_path, error_callback, kErrorPassphraseRequired);
+        return;
+      }
+      if (error == flimflam::kErrorBadPassphrase) {
+        InvokeErrorCallback(
+            service_path, error_callback, kErrorPassphraseRequired);
+        return;
+      }
+      if (IsAuthenticationError(error)) {
+        InvokeErrorCallback(
+            service_path, error_callback, kErrorAuthenticationRequired);
+        return;
+      }
+    }
   }
 
   // All synchronous checks passed, add |service_path| to connecting list.
@@ -275,27 +277,20 @@ void NetworkConnectionHandler::ConnectToNetwork(
       service_path,
       ConnectRequest(service_path, success_callback, error_callback)));
 
-  // Connect immediately to 'connectable' networks.
-  // TODO(stevenjb): Shill needs to properly set Connectable for VPN.
-  if (network->connectable() && network->type() != flimflam::kTypeVPN) {
-    CallShillConnect(service_path);
-    return;
-  }
-
-  if (network->type() == flimflam::kTypeCellular ||
-      (network->type() == flimflam::kTypeWifi &&
-       network->security() == flimflam::kSecurityNone)) {
-    NET_LOG_ERROR("Network has no security but is not 'Connectable'",
-                  service_path);
-    CallShillConnect(service_path);
-    return;
+  if (network) {
+    // Connect immediately to 'connectable' networks.
+    // TODO(stevenjb): Shill needs to properly set Connectable for VPN.
+    if (network->connectable() && network->type() != flimflam::kTypeVPN) {

[pneubeck (no reviews), 2013/08/12 18:32:44]

NIT: combine the two ifs

+      CallShillConnect(service_path);
+      return;
+    }
   }
 
   // Request additional properties to check. VerifyConfiguredAndConnect will
   // use only these properties, not cached properties, to ensure that they
   // are up to date after any recent configuration.
   network_configuration_handler_->GetProperties(
-      network->path(),
+      service_path,
       base::Bind(&NetworkConnectionHandler::VerifyConfiguredAndConnect,
                  AsWeakPtr(), check_error_state),
       base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
@@ -351,8 +346,7 @@ void NetworkConnectionHandler::NetworkPropertiesUpdated(
 }
 
 NetworkConnectionHandler::ConnectRequest*
-NetworkConnectionHandler::pending_request(
-    const std::string& service_path) {
+NetworkConnectionHandler::GetPendingRequest(const std::string& service_path) {
   std::map<std::string, ConnectRequest>::iterator iter =
       pending_requests_.find(service_path);
   return iter != pending_requests_.end() ? &(iter->second) : NULL;
@@ -376,9 +370,22 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
     return;
   }
 
-  std::string type;
+  std::string type, security;
   service_properties.GetStringWithoutPathExpansion(
       flimflam::kTypeProperty, &type);
+  service_properties.GetStringWithoutPathExpansion(
+      flimflam::kSecurityProperty, &security);
+  bool connectable = false;
+  service_properties.GetBooleanWithoutPathExpansion(
+      flimflam::kConnectableProperty, &connectable);
+
+  // In case NetworkState was not available in ConnectToNetwork (e.g. it had
+  // been recently configured), we need to check Connectable again.
+  if (connectable && type != flimflam::kTypeVPN) {
+    // TODO(stevenjb): Shill needs to properly set Connectable for VPN.
+    CallShillConnect(service_path);
+    return;
+  }
 
   // Get VPN provider type and host (required for configuration) and ensure
   // that required VPN non-cert properties are set.
@@ -409,11 +416,12 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
 
   client_cert::ConfigType client_cert_type = client_cert::CONFIG_TYPE_NONE;
   if (type == flimflam::kTypeVPN) {
-      if (vpn_provider_type == flimflam::kProviderOpenVpn)
-        client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
-      else
-        client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
-  } else if (type == flimflam::kTypeWifi) {
+    if (vpn_provider_type == flimflam::kProviderOpenVpn)
+      client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
+    else
+      client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
+  } else if (type == flimflam::kTypeWifi &&
+             security == flimflam::kSecurity8021x) {
     client_cert_type = client_cert::CONFIG_TYPE_EAP;
   }
 
@@ -437,8 +445,11 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
 
       // If certificates have not been loaded yet, queue the connect request.
       if (!certificates_loaded_) {
-        ConnectRequest* request = pending_request(service_path);
-        DCHECK(request);
+        ConnectRequest* request = GetPendingRequest(service_path);
+        if (!request) {
+          NET_LOG_ERROR("No pending request to queue", service_path);
+          return;
+        }
         NET_LOG_EVENT("Connect Request Queued", service_path);
         queued_connect_.reset(new ConnectRequest(
             service_path, request->success_callback, request->error_callback));
@@ -483,8 +494,8 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       config_properties.SetStringWithoutPathExpansion(
           flimflam::kProviderHostProperty, vpn_provider_host);
     } else if (type == flimflam::kTypeWifi) {
-      CopyStringFromDictionary(service_properties, flimflam::kSecurityProperty,
-                               &config_properties);
+      config_properties.SetStringWithoutPathExpansion(
+          flimflam::kSecurityProperty, security);
     }
 
     network_configuration_handler_->SetProperties(
@@ -522,8 +533,12 @@ void NetworkConnectionHandler::HandleConfigurationFailure(
     const std::string& service_path,
     const std::string& error_name,
     scoped_ptr<base::DictionaryValue> error_data) {
-  ConnectRequest* request = pending_request(service_path);
-  DCHECK(request);
+  ConnectRequest* request = GetPendingRequest(service_path);
+  if (!request) {
+    NET_LOG_ERROR("HandleConfigurationFailure called with no pending request.",
+                  service_path);
+    return;
+  }
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   if (!error_callback.is_null())
@@ -532,8 +547,12 @@ void NetworkConnectionHandler::HandleConfigurationFailure(
 
 void NetworkConnectionHandler::HandleShillConnectSuccess(
     const std::string& service_path) {
-  ConnectRequest* request = pending_request(service_path);
-  DCHECK(request);
+  ConnectRequest* request = GetPendingRequest(service_path);
+  if (!request) {
+    NET_LOG_ERROR("HandleShillConnectSuccess called with no pending request.",
+                  service_path);
+    return;
+  }
   request->connect_state = ConnectRequest::CONNECT_STARTED;
   NET_LOG_EVENT("Connect Request Acknowledged", service_path);
   // Do not call success_callback here, wait for one of the following
@@ -547,8 +566,12 @@ void NetworkConnectionHandler::HandleShillConnectFailure(
     const std::string& service_path,
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
-  ConnectRequest* request = pending_request(service_path);
-  DCHECK(request);
+  ConnectRequest* request = GetPendingRequest(service_path);
+  if (!request) {
+    NET_LOG_ERROR("HandleShillConnectFailure called with no pending request.",
+                  service_path);
+    return;
+  }
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   network_handler::ShillErrorCallbackFunction(
@@ -558,16 +581,15 @@ void NetworkConnectionHandler::HandleShillConnectFailure(
 
 void NetworkConnectionHandler::CheckPendingRequest(
     const std::string service_path) {
-  ConnectRequest* request = pending_request(service_path);
+  ConnectRequest* request = GetPendingRequest(service_path);
   DCHECK(request);
   if (request->connect_state == ConnectRequest::CONNECT_REQUESTED)
     return;  // Request has not started, ignore update
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
-  if (!network) {
-    ErrorCallbackForPendingRequest(service_path, kErrorNotFound);
-    return;
-  }
+  if (!network)
+    return;  // NetworkState may not be be updated yet.
+
   if (network->IsConnectingState()) {
     request->connect_state = ConnectRequest::CONNECT_CONNECTING;
     return;
@@ -636,8 +658,12 @@ std::string NetworkConnectionHandler::CertificateIsConfigured(
 void NetworkConnectionHandler::ErrorCallbackForPendingRequest(
     const std::string& service_path,
     const std::string& error_name) {
-  ConnectRequest* request = pending_request(service_path);
-  DCHECK(request);
+  ConnectRequest* request = GetPendingRequest(service_path);
+  if (!request) {
+    NET_LOG_ERROR("ErrorCallbackForPendingRequest with no pending request.",
+                  service_path);
+    return;
+  }
   // Remove the entry before invoking the callback in case it triggers a retry.
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);