Handle connecting to hidden networks correctly

chromeos/network/network_connection_handler.cc

 // Copyright (c) 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/network_connection_handler.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "chromeos/chromeos_switches.h"
@@ skipping 213 matching lines @@
   // Clear any existing queued connect request.
   queued_connect_.reset();
   if (HasConnectingNetwork(service_path)) {
     NET_LOG_USER("Connect Request While Pending", service_path);
     InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
     return;
   }
 
   // Check cached network state for connected, connecting, or unactivated
   // 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(
           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.
   pending_requests_.insert(std::make_pair(
       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,
                  AsWeakPtr(), service_path));
 }
 
 void NetworkConnectionHandler::DisconnectNetwork(
     const std::string& service_path,
     const base::Closure& success_callback,
     const network_handler::ErrorCallback& error_callback) {
@@ skipping 226 matching lines @@
                  AsWeakPtr(), service_path),
       base::Bind(&NetworkConnectionHandler::HandleShillConnectFailure,
                  AsWeakPtr(), service_path));
 }
 
 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);

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

considering the complexity of the overall logic and the number of accesses to
pending_request_,
it would be safer to handle request==NULL in release and log an error.

similarly in HandleShillConnect*

concrete issue could be:
CheckPendingRequest might be called with IsConnectedState (because of an
autoconnect) and remove the pending_request.

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

Hmm, definitely an edge case, but possible. Good catch. I'll fix this throughout
the code.

   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   if (!error_callback.is_null())
     error_callback.Run(kErrorConfigureFailed, error_data.Pass());
 }
 
 void NetworkConnectionHandler::HandleShillConnectSuccess(
     const std::string& service_path) {
   ConnectRequest* request = pending_request(service_path);
   DCHECK(request);
@@ skipping 14 matching lines @@
   DCHECK(request);
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   network_handler::ShillErrorCallbackFunction(
       kErrorConnectFailed, service_path, error_callback,
       dbus_error_name, dbus_error_message);
 }
 
 void NetworkConnectionHandler::CheckPendingRequest(
     const std::string service_path) {
   ConnectRequest* request = pending_request(service_path);

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

Every time I read "pending_request", I wonder what it does.
Could you rename it to e.g. GetPendingRequest(...). IMO that would be clearer.

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

Done.

   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;
   }
   if (network->IsConnectedState()) {
     NET_LOG_EVENT("Connect Request Succeeded", service_path);
     if (!request->success_callback.is_null())
       request->success_callback.Run();
     pending_requests_.erase(service_path);
     return;
@@ skipping 107 matching lines @@
 
 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