Add NetworkConnectionHandler class

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 "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/cert_loader.h"
+#include "chromeos/network/certificate_pattern_matcher.h"
+#include "chromeos/network/network_configuration_handler.h"
+#include "chromeos/network/network_event_log.h"
+#include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_ui_data.h"
+#include "dbus/object_path.h"
+#include "net/cert/x509_certificate.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace {
+const char kLogModule[] = "NetworkConnectionHandler";
+}
+
+namespace chromeos {
+
+NetworkConnectionHandler* g_connection_handler_instance = NULL;
+
+const char NetworkConnectionHandler::kNetworkNotFound[] = "not-found";
+const char NetworkConnectionHandler::kConnected[] = "connected";
+const char NetworkConnectionHandler::kConnecting[] = "connecting";
+const char NetworkConnectionHandler::kNotConnected[] = "not-connected";
+const char NetworkConnectionHandler::kPassphraseRequired[] =
+    "passphrase-required";
+const char NetworkConnectionHandler::kActivationRequired[] =
+    "activation-required";
+const char NetworkConnectionHandler::kCertificateRequired[] =
+    "certificate-required";
+const char NetworkConnectionHandler::kConfigurationRequired[] =
+    "configuration-required";
+const char NetworkConnectionHandler::kShillError[] = "shill-error";
+
+// static
+void NetworkConnectionHandler::Initialize() {
+  CHECK(!g_connection_handler_instance);
+  g_connection_handler_instance = new NetworkConnectionHandler;
+}
+
+// static
+void NetworkConnectionHandler::Shutdown() {
+  CHECK(g_connection_handler_instance);
+  delete g_connection_handler_instance;
+  g_connection_handler_instance = NULL;
+}
+
+// static
+NetworkConnectionHandler* NetworkConnectionHandler::Get() {
+  CHECK(g_connection_handler_instance)
+      << "NetworkConnectionHandler::Get() called before Initialize()";
+  return g_connection_handler_instance;
+}
+
+NetworkConnectionHandler::NetworkConnectionHandler() {
+}
+
+NetworkConnectionHandler::~NetworkConnectionHandler() {
+}
+
+void NetworkConnectionHandler::ConnectToNetwork(
+    const std::string& service_path,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  const NetworkState* network =
+      NetworkStateHandler::Get()->GetNetworkState(service_path);
+  if (!network) {
+    InvokeErrorCallback(service_path, error_callback, kNetworkNotFound);
+    return;
+  }
+  if (network->IsConnectedState()) {
+    InvokeErrorCallback(service_path, error_callback, kConnected);
+    return;
+  }
+  if (network->IsConnectingState() ||
+      pending_requests_.find(service_path) != pending_requests_.end()) {
+    InvokeErrorCallback(service_path, error_callback, kConnecting);
+    return;
+  }
+  if (network->passphrase_required()) {
+    InvokeErrorCallback(service_path, error_callback, kPassphraseRequired);
+    return;
+  }
+  if (NetworkRequiresActivation(network)) {
+    InvokeErrorCallback(service_path, error_callback, kActivationRequired);
+    return;
+  }
+
+  // All synchronous checks passed, add |service_path| to connecting list.
+  pending_requests_.insert(service_path);
+
+  if (!network->connectable() && NetworkMayNeedCredentials(network)) {
+    // Request additional properties to check.
+    NetworkConfigurationHandler::Get()->GetProperties(
+        network->path(),
+        base::Bind(&NetworkConnectionHandler::GetPropertiesCallback,
+                   AsWeakPtr(), success_callback, error_callback),
+        base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
+                   AsWeakPtr(), network->path(), error_callback));
+    return;
+  }
+  // All checks passed, send connect request.
+  CallShillConnect(service_path, success_callback, error_callback);
+}
+
+void NetworkConnectionHandler::DisconnectNetwork(
+    const std::string& service_path,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  const NetworkState* network =
+      NetworkStateHandler::Get()->GetNetworkState(service_path);
+  if (!network) {
+    InvokeErrorCallback(service_path, error_callback, kNetworkNotFound);
+    return;
+  }
+  if (!network->IsConnectedState()) {
+    InvokeErrorCallback(service_path, error_callback, kNotConnected);
+    return;
+  }
+  CallShillDisconnect(service_path, success_callback, error_callback);
+}
+
+void NetworkConnectionHandler::CallShillConnect(
+    const std::string& service_path,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  // TODO(stevenjb): Remove SetConnectingNetwork and use this class to maintain
+  // the connecting network(s) once NetworkLibrary path is eliminated.
+  NetworkStateHandler::Get()->SetConnectingNetwork(service_path);
+  network_event_log::AddEntry(kLogModule, "Connect Request", service_path);
+  DBusThreadManager::Get()->GetShillServiceClient()->Connect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkConnectionHandler::HandleShillSuccess,
+                 AsWeakPtr(), service_path, success_callback),
+      base::Bind(&NetworkConnectionHandler::HandleShillFailure,
+                 AsWeakPtr(), service_path, error_callback));
+}
+
+void NetworkConnectionHandler::CallShillDisconnect(
+    const std::string& service_path,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  network_event_log::AddEntry(kLogModule, "Disconnect Request", service_path);
+  DBusThreadManager::Get()->GetShillServiceClient()->Disconnect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkConnectionHandler::HandleShillSuccess,
+                 AsWeakPtr(), service_path, success_callback),
+      base::Bind(&NetworkConnectionHandler::HandleShillFailure,
+                 AsWeakPtr(), service_path, error_callback));
+}
+
+void NetworkConnectionHandler::InvokeErrorCallback(

[pneubeck (no reviews), 2013/05/07 08:46:39]

cc-local

[stevenjb, 2013/05/08 01:57:22]

Done.

+    const std::string& service_path,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& error_name) {
+  std::string error_message = "Connect Error: " + error_name;

[pneubeck (no reviews), 2013/05/07 08:46:39]

These error_messages are so useless. Not only here but in general. Maybe we can
do a cleanup soon and remove them all from the handler layer and above.

I wonder if Shill adds more useful messages.

[stevenjb, 2013/05/08 01:57:22]

I'm not quite sure what your complaint is, I have found the entries in the Event
Log very helpful for debugging, but having error_name in error_message was
redundant so I fixed that.

[pneubeck (no reviews), 2013/05/08 08:07:46]

In many places of chrome where we create (not forward) error_name and
error_message pairs, their content is redundant.
If Shill really puts useful information in both fields, then it's fine.

[stevenjb, 2013/05/08 18:48:00]

I see. In this case, it is helpful to know that the shill error originated from
a connect attempt (but appending the error name again was in fact redundant)

+  network_handler::ShillErrorCallbackFunction(
+      kLogModule, service_path, error_callback, error_name, error_message);
+}
+
+bool NetworkConnectionHandler::NetworkMayNeedCredentials(

[pneubeck (no reviews), 2013/05/07 08:46:39]

cc-local

[stevenjb, 2013/05/08 01:57:22]

Done.

+    const NetworkState* network) const {

[pneubeck (no reviews), 2013/05/07 08:46:39]

should be
const NetworkState&

[stevenjb, 2013/05/08 01:57:22]

Even though cost& is generally preferred, const* is allowed, and I prefer to
keep the code internally consistent, i.e. use only network->foo() instead of
mixing with network.foo().

+  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;
+}
+
+bool NetworkConnectionHandler::NetworkRequiresActivation(

[pneubeck (no reviews), 2013/05/07 08:46:39]

cc-local

[stevenjb, 2013/05/08 01:57:22]

Done.

+    const NetworkState* network) const {

[pneubeck (no reviews), 2013/05/07 08:46:39]

should be
const NetworkState&

[stevenjb, 2013/05/08 01:57:22]

ditto.

+  return (network->type() == flimflam::kTypeCellular &&
+          (network->activation_state() != flimflam::kActivationStateActivated ||
+           network->cellular_out_of_credits()));
+}
+
+void NetworkConnectionHandler::GetPropertiesCallback(

[pneubeck (no reviews), 2013/05/07 08:46:39]

this should use ONC as the NetworkConfigurationHandler in this form will be
replaced by ManagedNetworkConfigurationHandler.

compared to the SetProperties call below, this should remain a
ConfigurationHandler call, as it depends indeed on configuration (e.g. the
certificate pattern).

[stevenjb, 2013/05/08 01:57:22]

I don't fully understand your comment. 

I am fine with using ManagedNetworkConfigurationHandler instead.

More details below.

[pneubeck (no reviews), 2013/05/08 08:07:46]

NetworkConfigurationHandler::GetProperties returns a Shill dictionary.

ManagedNetworkConfigurationHandler::GetProperties returns a ONC dictionary.

The code in this function shouldn't work anymore, e.g.
properties.GetString(flimflam::kProviderHostProperty, &hostname);

must be

properties -> network_config::kVPN -> vpn::kHost

[stevenjb, 2013/05/08 18:48:00]

I see. Somehow I didn't realize that. OK, I'll switch this back to
NetworkConfigurationHandler and we can migrate later, is that reasonable?

[pneubeck (no reviews), 2013/05/13 09:29:35]

yes, fine with me.

+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& service_path,
+    const base::DictionaryValue& properties) {
+  const NetworkState* network =
+      NetworkStateHandler::Get()->GetNetworkState(service_path);
+  if (!network) {
+    InvokeErrorCallback(service_path, error_callback, kNetworkNotFound);
+    return;
+  }
+
+  std::string provider_type;  // For VPN
+  // Note: we use Value path expansion to extract Provider.Type.
+  properties.GetString(flimflam::kProviderTypeProperty, &provider_type);
+
+  // VPN requires a host and username to be set.
+  if (network->type() == flimflam::kTypeVPN) {
+    std::string hostname;
+    properties.GetString(flimflam::kProviderHostProperty, &hostname);

[pneubeck (no reviews), 2013/05/07 08:46:39]

I'm strongly against adding general configuration checks here (hostname,
username). Why should username and client_cert_id even be required?

For that we need a more complete solution that is based on ONC. E.g. the ONC
validator already checks for config completeness.

[stevenjb, 2013/05/08 01:57:22]

I am duplicating the logic from NetworkLibrary. I would like to minimze changes
to the logic in this pass.

In a later pass I would be fine with moving or removing this logic (after
careful consideration), but until/unless it exists elsewhere I would like to
preserve it.

[pneubeck (no reviews), 2013/05/13 09:29:35]

then better add a comment that this code is preliminary and needs
removal/revision. Otherwise, we may wonder in the future about, which parts were
on purpose and which not.

+    bool config_required;
+    if (provider_type == flimflam::kProviderOpenVpn) {
+      std::string username, client_cert_id;
+      properties.GetStringWithoutPathExpansion(
+          flimflam::kOpenVPNUserProperty, &username);
+      properties.GetStringWithoutPathExpansion(
+          flimflam::kOpenVPNClientCertIdProperty, &client_cert_id);
+      config_required =
+          hostname.empty() || username.empty() || client_cert_id.empty();
+    } else {
+      bool passphrase_required = false;
+      std::string passphrase;
+      properties.GetBooleanWithoutPathExpansion(
+          flimflam::kL2tpIpsecPskRequiredProperty, &passphrase_required);
+      properties.GetStringWithoutPathExpansion(
+          flimflam::kL2tpIpsecPskProperty, &passphrase);
+      config_required = passphrase_required && passphrase.empty();
+    }
+    if (config_required) {
+      InvokeErrorCallback(service_path, error_callback, kConfigurationRequired);
+      return;
+    }
+  }
+
+  // Get certificate properties from kUIDataProperty.
+  scoped_ptr<NetworkUIData> ui_data =
+      NetworkUIData::CreateUIDataFromShill(properties);

[pneubeck (no reviews), 2013/05/07 08:46:39]

UIData is and should be read by the config handler.
If we use ONC here (or better in the CertificateHandler), then we don't need
this dependency.

[stevenjb, 2013/05/08 01:57:22]

Same comment as above - that sounds like a good follow-up improvement.

[pneubeck (no reviews), 2013/05/13 09:29:35]

then also add a comment here, please.

+  if (!ui_data.get()) {
+    std::string error_message = "Error parsing UIData";
+    HandleShillFailure(
+        service_path, error_callback, kConfigurationRequired, error_message);

[pneubeck (no reviews), 2013/05/07 08:46:39]

is that really "ConfigurationRequired"

[stevenjb, 2013/05/08 01:57:22]

I screwed up this logic; what were intended to be early exits became failures. I
combined the logic to simplify this.

+    return;
+  }
+  if (ui_data->certificate_type() != CLIENT_CERT_TYPE_PATTERN ||

[pneubeck (no reviews), 2013/05/07 08:46:39]

Why is that an error? Certificate references should be fine too.

+      ui_data->certificate_pattern().Empty()) {
+    InvokeErrorCallback(service_path, error_callback, kConfigurationRequired);
+    return;
+  }
+
+  // We skip certificate patterns for device policy ONC so that an unmanaged
+  // user can't get to the place where a cert is presented for them
+  // involuntarily.
+  if (ui_data->onc_source() == onc::ONC_SOURCE_DEVICE_POLICY) {
+    InvokeErrorCallback(service_path, error_callback, kConfigurationRequired);
+    return;
+  }
+
+  // Find the matching certificate.
+  scoped_refptr<net::X509Certificate> matching_cert =

[pneubeck (no reviews), 2013/05/07 08:46:39]

I don't like this code to be here: investigating cert properties, cert matching
and writing the cert related Shill properties (at least lines 233-240 and
256-307).

The CertificateLoader/Handler will have to apply and update certificates as
well. I would prefer to request the CertificateLoader to apply the pattern ASAP
with a return callback. Things are also more complicated because of certificate
references which are local per ONC file. That complexity should be very local in
one class.

[stevenjb, 2013/05/08 01:57:22]

This also seems like something that can be improved in a follow-up CL; I'd like
to maintain the existing logic for now.

+      certificate_pattern::GetCertificateMatch(ui_data->certificate_pattern());
+  if (!matching_cert.get()) {
+    InvokeErrorCallback(service_path, error_callback, kCertificateRequired);
+    return;
+  }
+
+  // Set the appropriate propeties, then call connect on success.

[pneubeck (no reviews), 2013/05/07 08:46:39]

nit: propeties -> properties

+  std::string pkcs11_id =
+      CertLoader::Get()->GetPkcs11IdForCert(*matching_cert.get());
+  base::DictionaryValue new_properties;
+  network->GetConfigProperties(&new_properties);

[pneubeck (no reviews), 2013/05/07 08:46:39]

why is this necessary?

+  if (network->type() == flimflam::kTypeWifi) {
+    // shill requires both CertID and KeyID for TLS connections.
+    new_properties.SetStringWithoutPathExpansion(
+        flimflam::kEapCertIdProperty, pkcs11_id);
+    new_properties.SetStringWithoutPathExpansion(
+        flimflam::kEapKeyIdProperty, pkcs11_id);
+    if (network->security() == flimflam::kSecurity8021x) {
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kEapPinProperty, CertLoader::Get()->tpm_user_pin());
+    }
+  } else if (network->type() == flimflam::kTypeVPN) {
+    if (provider_type == flimflam::kProviderOpenVpn) {
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kOpenVPNPinProperty,
+          CertLoader::Get()->tpm_user_pin());
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kOpenVPNClientCertSlotProperty,
+          CertLoader::Get()->tpm_token_slot());
+    } else {
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kL2tpIpsecPinProperty,
+          CertLoader::Get()->tpm_user_pin());
+      new_properties.SetStringWithoutPathExpansion(
+          flimflam::kL2tpIpsecClientCertSlotProperty,
+          CertLoader::Get()->tpm_token_slot());
+    }
+  }
+  NetworkConfigurationHandler::Get()->SetProperties(

[pneubeck (no reviews), 2013/05/07 08:46:39]

CertId, KeyId, Pin, ... are not configuration properties and have to be written
directly using the ShillServiceClient.

Password is a configuration however and needs different treatment.

[stevenjb, 2013/05/08 01:57:22]

I removed the configuration from this flow, as I mentioned I think we can
migrate it to NetworkConfigurationHandler when we update NetworkConfigView.

+      network->path(),
+      new_properties,
+      base::Bind(&NetworkConnectionHandler::CallShillConnect,
+                 AsWeakPtr(), network->path(),
+                 success_callback, error_callback),
+      base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
+                 AsWeakPtr(), network->path(), error_callback));
+}
+
+void NetworkConnectionHandler::HandleConfigurationFailure(
+    const std::string& service_path,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& error_name,
+    scoped_ptr<base::DictionaryValue> error_data) {
+  pending_requests_.erase(service_path);
+  if (!error_callback.is_null())
+    error_callback.Run(error_name, error_data.Pass());
+}
+
+void NetworkConnectionHandler::HandleShillSuccess(
+    const std::string& service_path,
+    const base::Closure& success_callback) {
+  // TODO(stevenjb): Currently, this only indicates that the connect request
+  // succeeded. It might be preferable to wait for the actually connect
+  // attempt to succeed or fail here and only call |success_callback| at that
+  // point (or maybe call it twice, once indicating in-progress, then success
+  // or failure).
+  pending_requests_.erase(service_path);
+  network_event_log::AddEntry(kLogModule, "Connected", service_path);
+  success_callback.Run();
+}
+
+void NetworkConnectionHandler::HandleShillFailure(
+    const std::string& service_path,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  pending_requests_.erase(service_path);
+  std::string error = "Connect Failure: " + error_name + ": " + error_message;
+  network_handler::ShillErrorCallbackFunction(
+      kLogModule, service_path, error_callback, error_name, error_message);
+}
+
+}  // namespace chromeos