Differentiate between 'connect-failed' and 'bad-passphrase'

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 100 matching lines @@
     "passphrase-required";
 const char NetworkConnectionHandler::kErrorActivationRequired[] =
     "activation-required";
 const char NetworkConnectionHandler::kErrorCertificateRequired[] =
     "certificate-required";
 const char NetworkConnectionHandler::kErrorConfigurationRequired[] =
     "configuration-required";
 const char NetworkConnectionHandler::kErrorAuthenticationRequired[] =
     "authentication-required";
 const char NetworkConnectionHandler::kErrorShillError[] = "shill-error";
-const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
 const char NetworkConnectionHandler::kErrorConfigureFailed[] =
     "configure-failed";
-const char NetworkConnectionHandler::kErrorActivateFailed[] =
-    "activate-failed";
-const char NetworkConnectionHandler::kErrorMissingProvider[] =
-    "missing-provider";
 const char NetworkConnectionHandler::kErrorConnectCanceled[] =
     "connect-canceled";
-const char NetworkConnectionHandler::kErrorUnknown[] = "unknown-error";
 
 struct NetworkConnectionHandler::ConnectRequest {
   ConnectRequest(const std::string& service_path,
                  const base::Closure& success,
                  const network_handler::ErrorCallback& error)
       : service_path(service_path),
         connect_state(CONNECT_REQUESTED),
         success_callback(success),
         error_callback(error) {
   }
@@ skipping 106 matching lines @@
       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) {

[pneubeck (no reviews), 2013/08/26 08:14:11]

for non-VPN this looks fine, as it's handled later in
VerifyConfiguredAndConnect.

But I'm a bit concerned that we silently ignore a connect-failed error in case
of VPN. See my comment in VerifyConfiguredAndConnect.

[stevenjb, 2013/08/27 15:52:08]

The purpose of this code was to try to guess the user's intent:
* If we know that the network is misconfigured (i.e. Error == bad-passphrase),
then we should show the configure UI instead of attempting to connect.
* Sometimes, when Error == connect-failed, it was because of a configuration
error, in which case we would like to show the configure UI. However, unless we
know that the failure was due to a configuration error, showing the configure UI
will be confusing (e.g for WiMax).
* If we try to connect and it fails (again), we will show the configure UI
anyway, it just may take a little longer.

[pneubeck (no reviews), 2013/08/28 10:02:35]

My bad.. I should have placed this comment to the new code and not the old... I
meant: Removing this looks fine for non-VPN.

On 2013/08/27 15:52:08, stevenjb (chromium) wrote:

-        InvokeErrorCallback(
-            service_path, error_callback, kErrorPassphraseRequired);
-        return;
-      }
       if (error == flimflam::kErrorBadPassphrase) {
-        InvokeErrorCallback(
-            service_path, error_callback, kErrorPassphraseRequired);
+        InvokeErrorCallback(service_path, error_callback, error);
         return;
       }
       if (IsAuthenticationError(error)) {
         InvokeErrorCallback(
             service_path, error_callback, kErrorAuthenticationRequired);
         return;
       }
     }
   }
 
@@ skipping 123 matching lines @@
     // "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);
+      ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       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;
     }
   }
@@ skipping 47 matching lines @@
         ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
         return;
       }
     }
 
     // The network may not be 'Connectable' because the TPM properties are not
     // set up, so configure tpm slot/pin before connecting.
     if (cert_loader_ && cert_loader_->IsHardwareBacked()) {
       // Pass NULL if pkcs11_id is empty, so that it doesn't clear any
       // previously configured client cert.
       client_cert::SetShillProperties(client_cert_type,

[pneubeck (no reviews), 2013/08/26 08:14:11]

For VPN with certs, we will always reach this point...

                                       cert_loader_->tpm_token_slot(),
                                       cert_loader_->tpm_user_pin(),
                                       pkcs11_id.empty() ? NULL : &pkcs11_id,
                                       &config_properties);
     }
   }
 
   if (!config_properties.empty()) {
     NET_LOG_EVENT("Configuring Network", service_path);
 
@@ skipping 14 matching lines @@
           flimflam::kSecurityProperty, security);
     }
 
     network_configuration_handler_->SetProperties(
         service_path,
         config_properties,
         base::Bind(&NetworkConnectionHandler::CallShillConnect,
                    AsWeakPtr(), service_path),
         base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
                    AsWeakPtr(), service_path));
    return;

[pneubeck (no reviews), 2013/08/26 08:14:11]

...and then call CallShillConnect/return here.

Instead (in another CL), we should at first check if the new cert configuration
is different from the existing one.
If not and Shill reported a failure from a previous connect attempt, then we
should return with an error.

[stevenjb, 2013/08/27 15:52:08]

I'm not sure that we can rely on 'connect-failed' meaning 'bad certificate' even
if the certificate hasn't changed. We have seen issues with Google-A failing to
connect due to AP flakiness, then showing the configure dialog which is
confusing. Even with this change we will still show the configure UI on failure
to connect, but instead of showing the configure UI again on the next connect
attempt, we will try to connect again first, which seems more correct.

Ideally we would like more information from Shill (e.g. Error =
'bad-configuration') if we know that the certificate is incorrect, but I do not
know whether we can get that information.

[pneubeck (no reviews), 2013/08/28 10:02:35]

Still, it doesn't look right to change control flow or set cert properties, if
we did so during an earlier attempt.

On 2013/08/27 15:52:08, stevenjb (chromium) wrote:
Where in the code is this "configure UI on failure to connect" in case of VPN
with CertificatePattern?

   }
 
   // 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);
@@ skipping 49 matching lines @@
     const std::string& dbus_error_message) {
   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(
-      kErrorConnectFailed, service_path, error_callback,
+      flimflam::kErrorConnectFailed, service_path, error_callback,
       dbus_error_name, dbus_error_message);
 }
 
 void NetworkConnectionHandler::CheckPendingRequest(
     const std::string 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 =
@@ skipping 19 matching lines @@
   }
 
   // Network is neither connecting or connected; an error occurred.
   std::string error_name, error_detail;
   if (network->connection_state() == flimflam::kStateIdle &&
       pending_requests_.size() > 1) {
     // Another connect request canceled this one.
     error_name = kErrorConnectCanceled;
     error_detail = "";
   } else {
-    error_name = kErrorConnectFailed;
+    error_name = flimflam::kErrorConnectFailed;
     error_detail = network->error();
     if (error_detail.empty()) {
       if (network->connection_state() == flimflam::kStateFailure)
         error_detail = flimflam::kUnknownString;
       else
         error_detail = "Unexpected State: " + network->connection_state();
     }
   }
   std::string error_msg = error_name + ": " + error_detail;
   NET_LOG_ERROR(error_msg, service_path);
@@ skipping 83 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