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 25e03defdc509e9f05915969de7b70d1cc8b2c24..4f58d96f168e53c1b4d415dca39abe273bc7be08 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -233,24 +233,29 @@ void NetworkConnectionHandler::ConnectToNetwork(
   // networks. These states will not be affected by a recent configuration.
   const NetworkState* network =

[pneubeck (no reviews), 2013/08/10 19:44:11]

add a comment somewhere what it means if network==NULL.

Actually, I don't know exactly what Shill's behavior is here:
I thought Shill searches for Endpoints of hidden networks. If it finds an
endpoint the service will be listed in kServicesProperty and here it should end
up with network!=NULL.
If Shill didn't find an Endpoint, shouldn't then any connect attempt be in vain?


If network==NULL, you might consider checking if the service path is in the
FavoriteList to prevent handling and forwarding invalid service paths (from
potentially insecure sources, like JS) to Shill.

[stevenjb, 2013/08/12 17:48:29]

This should only happen if we call ConnectToNetwork immediately after
configuring a network. We have two choices here:
1. Request a NetworkState update and wait for it to complete (requires an adding
a callback to RequestUpdateForNetwork).
2. Skip the checks that require |network| and rely on the error handling for
VerifyConfiguredAndConnect.

I went with 2, since:
a) We already do a round-trip request with VerifyConfiguredAndConnect.
b) The checks requiring 'network' mostly don't apply in this case, except
'Connectable' as you noted.
c) This removes any direct dependency on NetworkState, i.e. we use NetworkState
when available but don't rely on it.

I will improve the comments.

[pneubeck (no reviews), 2013/08/12 18:35:01]

Sounds reasonable. In particular c) is IMO very important.

       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 (network) {
+    // For existing networks, perform some immediate consistency checks.
+    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) {
+    if (!network) {

[pneubeck (no reviews), 2013/08/10 19:44:11]

this looks a bit contradicting to the handling if !check_error_state.

This really needs some comments what your intend is.

[stevenjb, 2013/08/12 17:48:29]

Yeah, this just ends up being confusing, and really doesn't inform the UI, so I
removed this check.

+      InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
+      return;
+    }
     const std::string& error = network->error();
     if (error == flimflam::kErrorConnectFailed) {
       InvokeErrorCallback(
@@ -262,7 +267,6 @@ void NetworkConnectionHandler::ConnectToNetwork(
           service_path, error_callback, kErrorPassphraseRequired);
       return;
     }
-
     if (IsAuthenticationError(error)) {
       InvokeErrorCallback(
           service_path, error_callback, kErrorAuthenticationRequired);
@@ -275,27 +279,31 @@ 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) {
+    // 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/10 19:44:11]

For network==NULL, the connectable property will never be checked, although it's
available in VerifyConfiguredAndConnect.

[stevenjb, 2013/08/12 17:48:29]

Yes, I should add that there.
Done.

+      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->type() == flimflam::kTypeCellular ||
+        (network->type() == flimflam::kTypeWifi &&
+         network->security() == flimflam::kSecurityNone)) {
+      NET_LOG_ERROR("Network has no security but is not 'Connectable'",
+                    service_path);
+      // Try to connect anyway. Failure may succeed anyway, or provide a

[pneubeck (no reviews), 2013/08/10 19:44:11]

"Failure may succeed"
sounds wrong to me.

[stevenjb, 2013/08/12 17:48:29]

Oops, bad edit. 
Looking at the code, I actually removed this check, since we need to handle this
case in VerifyConfiguredAndConnect anyway.

+      // helpful error from Shill.
+      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,
@@ -583,10 +591,9 @@ void NetworkConnectionHandler::CheckPendingRequest(
     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;