Update NetworkConnectionHandler to rely less on NetworkState

This addresses potential problems with connecting to a network
immediately after configuring by only using NetworkState properties
for checking non configurable properties.

It also cleans up some unused members of NetworkState.

BUG=263978
For networking_private:
TBR=gauravsh@chromium.org, pneubeck@chromium.org

Review URL: https://codereview.chromium.org/21315002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215049 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 108 insertions, 134 deletions

diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 924c161..66e8d4f 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -39,20 +39,20 @@ void InvokeErrorCallback(const std::string& service_path,
   error_callback.Run(error_name, error_data.Pass());
 }
 
-bool NetworkConnectable(const NetworkState* network) {
-  if (network->type() == flimflam::kTypeVPN)
-    return false;  // TODO(stevenjb): Shill needs to properly set Connectable.
-  return network->connectable();
+bool IsAuthenticationError(const std::string& error) {
+  return (error == flimflam::kErrorBadWEPKey ||
+          error == flimflam::kErrorPppAuthFailed ||
+          error == shill::kErrorEapLocalTlsFailed ||
+          error == shill::kErrorEapRemoteTlsFailed ||
+          error == shill::kErrorEapAuthenticationFailed);
 }
 
-bool NetworkMayNeedCredentials(const NetworkState* network) {
-  if (network->type() == flimflam::kTypeWifi &&
-      (network->security() == flimflam::kSecurity8021x ||
-       network->security() == flimflam::kSecurityWep /* For dynamic WEP*/))
-    return true;
-  if (network->type() == flimflam::kTypeVPN)
-    return true;
-  return false;
+void CopyStringFromDictionary(const base::DictionaryValue& source,
+                              const std::string& key,
+                              base::DictionaryValue* dest) {
+  std::string string_value;
+  if (source.GetStringWithoutPathExpansion(key, &string_value))
+    dest->SetStringWithoutPathExpansion(key, string_value);
 }
 
 bool NetworkRequiresActivation(const NetworkState* network) {
@@ -131,6 +131,8 @@ 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::kErrorDisconnectFailed[] =
@@ -222,7 +224,7 @@ void NetworkConnectionHandler::OnCertificatesLoaded(
     ConnectToNetwork(queued_connect_->service_path,
                      queued_connect_->success_callback,
                      queued_connect_->error_callback,
-                     true /* ignore_error_state */);
+                     false /* check_error_state */);
   } else if (initial_load) {
     // Once certificates have loaded, connect to the "best" available network.
     network_state_handler_->ConnectToBestWifiNetwork();
@@ -233,21 +235,24 @@ void NetworkConnectionHandler::ConnectToNetwork(
     const std::string& service_path,
     const base::Closure& success_callback,
     const network_handler::ErrorCallback& error_callback,
-    bool ignore_error_state) {
+    bool check_error_state) {
   NET_LOG_USER("ConnectToNetwork", service_path);
   // 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 =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
     InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
     return;
   }
-  if (HasConnectingNetwork(service_path)) {
-    NET_LOG_USER("Connect Request While Pending", service_path);
-    InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
-    return;
-  }
   if (network->IsConnectedState()) {
     InvokeErrorCallback(service_path, error_callback, kErrorConnected);
     return;
@@ -260,47 +265,37 @@ void NetworkConnectionHandler::ConnectToNetwork(
     InvokeErrorCallback(service_path, error_callback, kErrorActivationRequired);
     return;
   }
-  if (!ignore_error_state) {
-    if (network->passphrase_required()) {
-      InvokeErrorCallback(service_path, error_callback,
-                          kErrorPassphraseRequired);
-      return;
-    }
-    if (network->error() == flimflam::kErrorConnectFailed) {
-      InvokeErrorCallback(service_path, error_callback,
-                          kErrorPassphraseRequired);
-      return;
-    }
-    if (network->error() == flimflam::kErrorBadPassphrase) {
-      InvokeErrorCallback(service_path, error_callback,
-                          kErrorPassphraseRequired);
-      return;
-    }
-    if (network->HasAuthenticationError()) {
-      InvokeErrorCallback(service_path, error_callback,
-                          kErrorConfigurationRequired);
-      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)));
 
-  if (!NetworkConnectable(network) &&
-      NetworkMayNeedCredentials(network)) {
-    // Request additional properties to check.
-    network_configuration_handler_->GetProperties(
-        network->path(),
-        base::Bind(&NetworkConnectionHandler::VerifyConfiguredAndConnect,
-                   AsWeakPtr()),
-        base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
-                   AsWeakPtr(), network->path()));
+  // 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;
   }
-  // All checks passed, send connect request.
-  CallShillConnect(service_path);
+
+  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;
+  }
+
+  // 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(),
+      base::Bind(&NetworkConnectionHandler::VerifyConfiguredAndConnect,
+                 AsWeakPtr(), check_error_state),
+      base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
+                 AsWeakPtr(), service_path));
 }
 
 void NetworkConnectionHandler::DisconnectNetwork(
@@ -347,19 +342,47 @@ NetworkConnectionHandler::pending_request(
 // ConnectToNetwork implementation
 
 void NetworkConnectionHandler::VerifyConfiguredAndConnect(
+    bool check_error_state,
     const std::string& service_path,
     const base::DictionaryValue& service_properties) {
   NET_LOG_EVENT("VerifyConfiguredAndConnect", service_path);
 
-  const NetworkState* network =
-      network_state_handler_->GetNetworkState(service_path);
-  if (!network) {
-    ErrorCallbackForPendingRequest(service_path, kErrorNotFound);
+  std::string type;
+  service_properties.GetStringWithoutPathExpansion(
+      flimflam::kTypeProperty, &type);
+
+  if (check_error_state) {
+    std::string error;
+    service_properties.GetStringWithoutPathExpansion(
+        flimflam::kErrorProperty, &error);
+    if (error == flimflam::kErrorConnectFailed) {
+      ErrorCallbackForPendingRequest(service_path, kErrorPassphraseRequired);
+      return;
+    }
+    if (error == flimflam::kErrorBadPassphrase) {
+      ErrorCallbackForPendingRequest(service_path, kErrorPassphraseRequired);
+      return;
+    }
+
+    if (IsAuthenticationError(error)) {
+      ErrorCallbackForPendingRequest(service_path,
+                                     kErrorAuthenticationRequired);
+      return;
+    }
+  }
+
+  // If 'passphrase_required' is still true, then the 'Passphrase' property
+  // has not been set to a minimum length value.
+  bool passphrase_required = false;
+  service_properties.GetBooleanWithoutPathExpansion(
+      flimflam::kPassphraseRequiredProperty, &passphrase_required);
+  if (passphrase_required) {
+    ErrorCallbackForPendingRequest(service_path, kErrorPassphraseRequired);
     return;
   }
 
   // VPN requires a host and username to be set.
-  if (network->type() == flimflam::kTypeVPN &&
+  if (type == flimflam::kTypeVPN &&
       !VPNIsConfigured(service_path, service_properties)) {
     ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
     return;
@@ -400,9 +423,17 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
     const std::string& tpm_slot = cert_loader_->tpm_token_slot();
     const std::string& tpm_pin = cert_loader_->tpm_user_pin();
     base::DictionaryValue config_properties;
-    network->GetConfigProperties(&config_properties);
-
-    if (network->type() == flimflam::kTypeVPN) {
+    // 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;
@@ -431,7 +462,7 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
         config_properties.SetStringWithoutPathExpansion(
             flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
       }
-    } else if (network->type() == flimflam::kTypeWifi) {
+    } else if (type == flimflam::kTypeWifi) {
       config_properties.SetStringWithoutPathExpansion(
           flimflam::kEapPinProperty, cert_loader_->tpm_user_pin());
       config_properties.SetStringWithoutPathExpansion(
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index cc77f80..8f00a8a 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -54,6 +54,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   static const char kErrorPassphraseRequired[];
   static const char kErrorActivationRequired[];
   static const char kErrorCertificateRequired[];
+  static const char kErrorAuthenticationRequired[];
   static const char kErrorConfigurationRequired[];
   static const char kErrorShillError[];
   static const char kErrorConnectFailed[];
@@ -74,16 +75,18 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   //  kErrorConnected if already connected to the network.
   //  kErrorConnecting if already connecting to the network.
   //  kErrorCertificateRequired if the network requires a cert and none exists.
-  //  kErrorPassphraseRequired if passphrase only is required.
+  //  kErrorPassphraseRequired if passphrase only is missing or incorrect.
+  //  kErrorAuthenticationRequired if other authentication is required.
   //  kErrorConfigurationRequired if additional configuration is required.
   //  kErrorShillError if a DBus or Shill error occurred.
   // |error_message| will contain an additional error string for debugging.
-  // If |ignore_error_state| is true, error state for the network is ignored
-  //  (e.g. for repeat attempts).
+  // If |check_error_state| is true, the current state of the network is
+  //  checked for errors, otherwise current state is ignored (e.g. for recently
+  //  configured networks or repeat attempts).
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
                         const network_handler::ErrorCallback& error_callback,
-                        bool ignore_error_state);
+                        bool check_error_state);
 
   // DisconnectNetwork() will send a Disconnect request to Shill.
   // On success, |success_callback| will be called.
@@ -127,8 +130,10 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // Callback from Shill.Service.GetProperties. Parses |properties| to verify
   // whether or not the network appears to be configured. If configured,
   // attempts a connection, otherwise invokes error_callback from
-  // pending_requests_[service_path].
-  void VerifyConfiguredAndConnect(const std::string& service_path,
+  // pending_requests_[service_path]. |check_error_state| is passed from
+  // ConnectToNetwork(), see comment for info.
+  void VerifyConfiguredAndConnect(bool check_error_state,
+                                  const std::string& service_path,
                                   const base::DictionaryValue& properties);
 
   // Calls Shill.Manager.Connect asynchronously.
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index 8379d2f..a321fa4 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -81,14 +81,14 @@ class NetworkConnectionHandlerTest : public testing::Test {
   }
 
   void Connect(const std::string& service_path) {
-    const bool ignore_error_state = false;
+    const bool check_error_state = true;
     network_connection_handler_->ConnectToNetwork(
         service_path,
         base::Bind(&NetworkConnectionHandlerTest::SuccessCallback,
                    base::Unretained(this)),
         base::Bind(&NetworkConnectionHandlerTest::ErrorCallback,
                    base::Unretained(this)),
-        ignore_error_state);
+        check_error_state);
     message_loop_.RunUntilIdle();
   }
 
@@ -141,14 +141,15 @@ class NetworkConnectionHandlerTest : public testing::Test {
 namespace {
 
 const char* kConfigConnectable =
-    "{ \"GUID\": \"wifi0\", \"Type\": \"wifi\", \"State\": \"idle\" }";
+    "{ \"GUID\": \"wifi0\", \"Type\": \"wifi\", \"State\": \"idle\", "
+    "  \"Connectable\": true }";
 const char* kConfigConnected =
     "{ \"GUID\": \"wifi1\", \"Type\": \"wifi\", \"State\": \"online\" }";
 const char* kConfigConnecting =
     "{ \"GUID\": \"wifi2\", \"Type\": \"wifi\", \"State\": \"association\" }";
 const char* kConfigRequiresPassphrase =
     "{ \"GUID\": \"wifi3\", \"Type\": \"wifi\", "
-    "\"PassphraseRequired\": true }";
+    "  \"PassphraseRequired\": true }";
 const char* kConfigRequiresActivation =
     "{ \"GUID\": \"cellular1\", \"Type\": \"cellular\","
     "  \"Cellular.ActivationState\": \"not-activated\" }";
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index 275b0c8..2dbb0a5 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -81,7 +81,6 @@ NetworkState::NetworkState(const std::string& path)
       prefix_length_(0),
       signal_strength_(0),
       connectable_(false),
-      passphrase_required_(false),
       activate_over_non_cellular_networks_(false),
       cellular_out_of_credits_(false) {
 }
@@ -100,27 +99,6 @@ bool NetworkState::PropertyChanged(const std::string& key,
     return GetStringValue(key, value, &connection_state_);
   } else if (key == flimflam::kConnectableProperty) {
     return GetBooleanValue(key, value, &connectable_);
-  } else if (key == flimflam::kPassphraseRequiredProperty) {
-    return GetBooleanValue(key, value, &passphrase_required_);
-  } else if (key == shill::kWifiFrequencyListProperty) {
-    const base::ListValue* frequencies;
-    if (!value.GetAsList(&frequencies)) {
-      NET_LOG_ERROR("Failed to parse " + key, path());
-      return false;
-    }
-    wifi_frequencies_.clear();
-    for (base::ListValue::const_iterator iter = frequencies->begin();
-         iter != frequencies->end(); ++iter) {
-      int frequency;
-      if ((*iter)->GetAsInteger(&frequency))
-        wifi_frequencies_.push_back(frequency);
-    }
-    if (!wifi_frequencies_.empty()) {
-      std::string str;
-      base::JSONWriter::Write(frequencies, &str);
-      NET_LOG_DEBUG("WifiFrequencies for " + path(), str);
-    }
-    return true;
   } else if (key == flimflam::kErrorProperty) {
     return GetStringValue(key, value, &error_);
   } else if (key == shill::kErrorDetailsProperty) {
@@ -226,16 +204,6 @@ void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
                                             connection_state_);
   dictionary->SetBooleanWithoutPathExpansion(flimflam::kConnectableProperty,
                                              connectable_);
-  dictionary->SetBooleanWithoutPathExpansion(
-      flimflam::kPassphraseRequiredProperty, passphrase_required_);
-
-  base::ListValue* frequencies = new base::ListValue;
-  for (FrequencyList::const_iterator iter = wifi_frequencies_.begin();
-       iter != wifi_frequencies_.end(); ++iter) {
-    frequencies->AppendInteger(*iter);
-  }
-  dictionary->SetWithoutPathExpansion(shill::kWifiFrequencyListProperty,
-                                      frequencies);
 
   dictionary->SetStringWithoutPathExpansion(flimflam::kErrorProperty,
                                             error_);
@@ -302,15 +270,6 @@ void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
                                       payment_portal_properties);
 }
 
-void NetworkState::GetConfigProperties(
-    base::DictionaryValue* dictionary) const {
-  dictionary->SetStringWithoutPathExpansion(flimflam::kNameProperty, name());
-  dictionary->SetStringWithoutPathExpansion(flimflam::kTypeProperty, type());
-  dictionary->SetStringWithoutPathExpansion(flimflam::kSecurityProperty,
-                                            security());
-  dictionary->SetStringWithoutPathExpansion(flimflam::kGuidProperty, guid());
-}
-
 bool NetworkState::IsConnectedState() const {
   return StateIsConnected(connection_state_);
 }
@@ -343,15 +302,6 @@ std::string NetworkState::GetNetmask() const {
   return network_util::PrefixLengthToNetmask(prefix_length_);
 }
 
-bool NetworkState::HasAuthenticationError() const {
-  return (error_ == flimflam::kErrorBadPassphrase ||
-          error_ == flimflam::kErrorBadWEPKey ||
-          error_ == flimflam::kErrorPppAuthFailed ||
-          error_ == shill::kErrorEapLocalTlsFailed ||
-          error_ == shill::kErrorEapRemoteTlsFailed ||
-          error_ == shill::kErrorEapAuthenticationFailed);
-}
-
 bool NetworkState::UpdateName(const base::DictionaryValue& properties) {
   std::string hex_ssid;
   properties.GetStringWithoutPathExpansion(flimflam::kWifiHexSsid, &hex_ssid);
diff --git a/chromeos/network/network_state.h b/chromeos/network/network_state.h
index 8278e5f..9151131 100644
--- a/chromeos/network/network_state.h
+++ b/chromeos/network/network_state.h
@@ -38,10 +38,6 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // stored.
   void GetProperties(base::DictionaryValue* dictionary) const;
 
-  // Fills |dictionary| with the state properties required to configure a
-  // network.
-  void GetConfigProperties(base::DictionaryValue* dictionary) const;
-
   // Accessors
   const std::string& security() const { return security_; }
   const std::string& device_path() const { return device_path_; }
@@ -63,9 +59,6 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // Wireless property accessors
   int signal_strength() const { return signal_strength_; }
   bool connectable() const { return connectable_; }
-  // Wifi property accessors
-  bool passphrase_required() const { return passphrase_required_; }
-  const FrequencyList& wifi_frequencies() const { return wifi_frequencies_; }
   // Cellular property accessors
   const std::string& network_technology() const {
     return network_technology_;
@@ -97,9 +90,6 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // Converts the prefix length to a netmaks string.
   std::string GetNetmask() const;
 
-  // Returns true if |error_| contains an authentication error.
-  bool HasAuthenticationError() const;
-
   // Helpers (used e.g. when a state is cached)
   static bool StateIsConnected(const std::string& connection_state);
   static bool StateIsConnecting(const std::string& connection_state);
@@ -147,9 +137,6 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // Wireless properties
   int signal_strength_;
   bool connectable_;
-  // Wifi properties
-  bool passphrase_required_;
-  FrequencyList wifi_frequencies_;
   // Cellular properties
   std::string network_technology_;
   std::string activation_state_;