Revert 204994 "Configure networks requiring a certificate."

> Configure networks requiring a certificate.
> 
> The current flow does not guarantee that a configured network will have
> its tpm / pkcs11 credentials configured. We need to set these 
> properties in order to reliably connect to networks requiring 
> certificates. The patch also includes some minor changes to improve
> error reporting and configuration triggering for failure cases.
> 
> BUG=247104
> For /wifi_data_provider_chromeos_unittest.cc:
> TBR=joth@chromium.org
> 
> Review URL: https://chromiumcodereview.appspot.com/16512003

TBR=stevenjb@chromium.org

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

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

7 files changed, 66 insertions, 223 deletions

diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 16b90af..97cc9f8 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -100,6 +100,13 @@ bool VPNIsConfigured(const std::string& service_path,
       NET_LOG_EVENT("OpenVPN: No passphrase", service_path);
       return false;
     }
+    std::string client_cert_id;
+    properties->GetStringWithoutPathExpansion(
+        flimflam::kOpenVPNClientCertIdProperty, &client_cert_id);
+    if (client_cert_id.empty()) {
+      NET_LOG_EVENT("OpenVPN: No cert id", service_path);
+      return false;
+    }
     NET_LOG_EVENT("OpenVPN Is Configured", service_path);
   } else {
     bool passphrase_required = false;
@@ -115,6 +122,27 @@ bool VPNIsConfigured(const std::string& service_path,
   return true;
 }
 
+bool CertificateIsConfigured(NetworkUIData* ui_data) {
+  if (ui_data->certificate_type() != CLIENT_CERT_TYPE_PATTERN)
+    return true;  // No certificate or a reference.
+  if (ui_data->onc_source() == onc::ONC_SOURCE_DEVICE_POLICY) {
+    // We skip checking 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.
+    return true;
+  }
+  if (ui_data->certificate_pattern().Empty())
+    return false;
+
+  // Find the matching certificate.
+  scoped_refptr<net::X509Certificate> matching_cert =
+      certificate_pattern::GetCertificateMatch(
+          ui_data->certificate_pattern());
+  if (!matching_cert.get())
+    return false;
+  return true;
+}
+
 }  // namespace
 
 const char NetworkConnectionHandler::kErrorNotFound[] = "not-found";
@@ -134,11 +162,9 @@ const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
 const char NetworkConnectionHandler::kErrorUnknown[] = "unknown-error";
 
 struct NetworkConnectionHandler::ConnectRequest {
-  ConnectRequest(const std::string& service_path,
-                 const base::Closure& success,
+  ConnectRequest(const base::Closure& success,
                  const network_handler::ErrorCallback& error)
-      : service_path(service_path),
-        connect_state(CONNECT_REQUESTED),
+      : connect_state(CONNECT_REQUESTED),
         success_callback(success),
         error_callback(error) {
   }
@@ -147,44 +173,29 @@ struct NetworkConnectionHandler::ConnectRequest {
     CONNECT_STARTED = 1,
     CONNECT_CONNECTING = 2
   };
-  std::string service_path;
   ConnectState connect_state;
   base::Closure success_callback;
   network_handler::ErrorCallback error_callback;
 };
 
 NetworkConnectionHandler::NetworkConnectionHandler()
-    : cert_loader_(NULL),
-      network_state_handler_(NULL),
-      network_configuration_handler_(NULL),
-      logged_in_(false),
-      certificates_loaded_(false) {
+    : network_state_handler_(NULL),
+      network_configuration_handler_(NULL) {
+  const char* new_handler_enabled =
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kUseNewNetworkConnectionHandler) ?
+      "enabled" : "disabled";
+  NET_LOG_EVENT("NewNetworkConnectionHandler", new_handler_enabled);
 }
 
 NetworkConnectionHandler::~NetworkConnectionHandler() {
   if (network_state_handler_)
     network_state_handler_->RemoveObserver(this);
-  if (cert_loader_)
-    cert_loader_->RemoveObserver(this);
-  if (LoginState::IsInitialized())
-    LoginState::Get()->RemoveObserver(this);
 }
 
 void NetworkConnectionHandler::Init(
-    CertLoader* cert_loader,
     NetworkStateHandler* network_state_handler,
     NetworkConfigurationHandler* network_configuration_handler) {
-  LoginState::Get()->AddObserver(this);
-  logged_in_ =
-      LoginState::Get()->GetLoggedInState() == LoginState::LOGGED_IN_ACTIVE;
-  if (cert_loader) {
-    cert_loader_ = cert_loader;
-    cert_loader_->AddObserver(this);
-    certificates_loaded_ = cert_loader->certificates_loaded();
-  } else {
-    // TODO(stevenjb): Require a mock or stub cert_loader in tests.
-    certificates_loaded_ = true;
-  }
   if (network_state_handler) {
     network_state_handler_ = network_state_handler;
     network_state_handler_->AddObserver(this);
@@ -192,44 +203,12 @@ void NetworkConnectionHandler::Init(
   network_configuration_handler_ = network_configuration_handler;
 }
 
-void NetworkConnectionHandler::LoggedInStateChanged(
-    LoginState::LoggedInState state) {
-  NET_LOG_EVENT("NewNetworkConnectionHandler",
-                CommandLine::ForCurrentProcess()->HasSwitch(
-                    chromeos::switches::kUseNewNetworkConnectionHandler) ?
-                "enabled" : "disabled");
-  if (state == LoginState::LOGGED_IN_ACTIVE) {
-    logged_in_ = true;
-    NET_LOG_EVENT("Logged In", "");
-  }
-}
-
-void NetworkConnectionHandler::OnCertificatesLoaded(
-    const net::CertificateList& cert_list,
-    bool initial_load) {
-  certificates_loaded_ = true;
-  NET_LOG_EVENT("Certificates Loaded", "");
-  if (queued_connect_) {
-    NET_LOG_EVENT("Connecting to Queued Network",
-                  queued_connect_->service_path);
-    ConnectToNetwork(queued_connect_->service_path,
-                     queued_connect_->success_callback,
-                     queued_connect_->error_callback,
-                     true /* ignore_error_state */);
-  } else if (initial_load) {
-    // Once certificates have loaded, connect to the "best" available network.
-    NetworkHandler::Get()->network_state_handler()->ConnectToBestWifiNetwork();
-  }
-}
-
 void NetworkConnectionHandler::ConnectToNetwork(
     const std::string& service_path,
     const base::Closure& success_callback,
     const network_handler::ErrorCallback& error_callback,
     bool ignore_error_state) {
   NET_LOG_USER("ConnectToNetwork", service_path);
-  // Clear any existing queued connect request.
-  queued_connect_.reset();
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
@@ -278,11 +257,9 @@ void NetworkConnectionHandler::ConnectToNetwork(
 
   // 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)));
+      service_path, ConnectRequest(success_callback, error_callback)));
 
-  if (!NetworkConnectable(network) &&
-      NetworkMayNeedCredentials(network)) {
+  if (!NetworkConnectable(network) && NetworkMayNeedCredentials(network)) {
     // Request additional properties to check.
     network_configuration_handler_->GetProperties(
         network->path(),
@@ -343,101 +320,43 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
     const std::string& service_path,
     const base::DictionaryValue& properties) {
   NET_LOG_EVENT("VerifyConfiguredAndConnect", service_path);
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  network_handler::ErrorCallback error_callback = request->error_callback;
 
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
-    ErrorCallbackForPendingRequest(service_path, kErrorNotFound);
+    pending_requests_.erase(service_path);
+    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
     return;
   }
 
   // VPN requires a host and username to be set.
   if (network->type() == flimflam::kTypeVPN &&
       !VPNIsConfigured(service_path, properties)) {
-    ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+    pending_requests_.erase(service_path);
+    InvokeErrorCallback(service_path, error_callback,
+                        kErrorConfigurationRequired);
     return;
   }
 
   // Check certificate properties in kUIDataProperty.
   scoped_ptr<NetworkUIData> ui_data =
       ManagedNetworkConfigurationHandler::GetUIData(properties);
-  if (ui_data && ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
-    // User must be logged in to connect to a network requiring a certificate.
-    if (!logged_in_ || !cert_loader_) {
-      ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
-      return;
-    }
-    // If certificates have not been loaded yet, queue the connect request.
-    if (!certificates_loaded_) {
-      ConnectRequest* request = pending_request(service_path);
-      DCHECK(request);
-      NET_LOG_EVENT("Connect Request Queued", service_path);
-      queued_connect_.reset(new ConnectRequest(
-          service_path, request->success_callback, request->error_callback));
-      pending_requests_.erase(service_path);
-      return;
-    }
-
-    // Ensure the certificate is available.
-    std::string pkcs11_id;
-    if (!CertificateIsConfigured(ui_data.get(), &pkcs11_id)) {
-      ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
-      return;
-    }
-
-    // The network may not be 'Connectable' because the certificate data is
-    // not set up, so configure tpm slot/pin and pkcs11_id before connecting.
-    // TODO(stevenjb): Remove this code once NetworkConfigurationHandler
-    // handles this.
-    NET_LOG_EVENT("Configuring Network", service_path);
-    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) {
-      std::string provider_type;
-      properties.GetString(flimflam::kTypeProperty, &provider_type);
-      if (provider_type == flimflam::kProviderOpenVpn) {
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertSlotProperty, tpm_slot);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNPinProperty, tpm_pin);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kOpenVPNClientCertIdProperty, pkcs11_id);
-      } else {
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kL2tpIpsecClientCertSlotProperty, tpm_slot);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kL2tpIpsecPinProperty, tpm_pin);
-        config_properties.SetStringWithoutPathExpansion(
-            flimflam::kL2tpIpsecClientCertIdProperty, pkcs11_id);
-      }
-    } else if (network->type() == flimflam::kTypeWifi) {
-      config_properties.SetStringWithoutPathExpansion(
-          flimflam::kEapPinProperty, cert_loader_->tpm_user_pin());
-      config_properties.SetStringWithoutPathExpansion(
-          flimflam::kEapCertIdProperty, pkcs11_id);
-      config_properties.SetStringWithoutPathExpansion(
-          flimflam::kEapKeyIdProperty, pkcs11_id);
-    }
-    network_configuration_handler_->SetProperties(
-        service_path,
-        config_properties,
-        base::Bind(&NetworkConnectionHandler::CallShillConnect,
-                   AsWeakPtr(), service_path),
-        base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
-                   AsWeakPtr(), service_path));
-   return;
+  if (ui_data && !CertificateIsConfigured(ui_data.get())) {
+    pending_requests_.erase(service_path);
+    InvokeErrorCallback(service_path, error_callback,
+                        kErrorCertificateRequired);
+    return;
   }
 
-  // Otherwise, we need to configure the network.
-  ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+  CallShillConnect(service_path);
 }
 
 void NetworkConnectionHandler::CallShillConnect(
     const std::string& service_path) {
-  NET_LOG_EVENT("Sending Connect Request to Shill", service_path);
+  NET_LOG_EVENT("Connect Request", service_path);
   DBusThreadManager::Get()->GetShillServiceClient()->Connect(
       dbus::ObjectPath(service_path),
       base::Bind(&NetworkConnectionHandler::HandleShillConnectSuccess,
@@ -463,7 +382,7 @@ void NetworkConnectionHandler::HandleShillConnectSuccess(
   ConnectRequest* request = pending_request(service_path);
   DCHECK(request);
   request->connect_state = ConnectRequest::CONNECT_STARTED;
-  NET_LOG_EVENT("Connect Request Acknowledged", service_path);
+  NET_LOG_EVENT("Connect Request Sent", service_path);
   // Do not call success_callback here, wait for one of the following
   // conditions:
   // * State transitions to a non connecting state indicating succes or failure
@@ -493,7 +412,9 @@ void NetworkConnectionHandler::CheckPendingRequest(
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
-    ErrorCallbackForPendingRequest(service_path, kErrorNotFound);
+    network_handler::ErrorCallback error_callback = request->error_callback;
+    pending_requests_.erase(service_path);
+    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
     return;
   }
   if (network->IsConnectingState()) {
@@ -541,31 +462,6 @@ void NetworkConnectionHandler::CheckAllPendingRequests() {
   }
 }
 
-bool NetworkConnectionHandler::CertificateIsConfigured(NetworkUIData* ui_data,
-                                                       std::string* pkcs11_id) {
-  if (ui_data->certificate_pattern().Empty())
-    return false;
-
-  // Find the matching certificate.
-  scoped_refptr<net::X509Certificate> matching_cert =
-      certificate_pattern::GetCertificateMatch(ui_data->certificate_pattern());
-  if (!matching_cert.get())
-    return false;
-  *pkcs11_id = cert_loader_->GetPkcs11IdForCert(*matching_cert.get());
-  return true;
-}
-
-void NetworkConnectionHandler::ErrorCallbackForPendingRequest(
-    const std::string& service_path,
-    const std::string& error_name) {
-  ConnectRequest* request = pending_request(service_path);
-  DCHECK(request);
-  // Remove the entry before invoking the callback in case it triggers a retry.
-  network_handler::ErrorCallback error_callback = request->error_callback;
-  pending_requests_.erase(service_path);
-  InvokeErrorCallback(service_path, error_callback, error_name);
-}
-
 // Disconnect
 
 void NetworkConnectionHandler::CallShillDisconnect(
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index ec6ea4f..42c2bdf 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -14,8 +14,6 @@
 #include "base/values.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
-#include "chromeos/login/login_state.h"
-#include "chromeos/network/cert_loader.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_handler_callbacks.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -23,7 +21,6 @@
 namespace chromeos {
 
 class NetworkState;
-class NetworkUIData;
 
 // The NetworkConnectionHandler class is used to manage network connection
 // requests. This is the only class that should make Shill Connect calls.
@@ -32,19 +29,16 @@ class NetworkUIData;
 //    known to be available to connect to the network.
 // 2. Request additional information (e.g. user data which contains certificate
 //    information) and determine whether sufficient information is available.
-// 3. Possibly configure the network certificate info (tpm slot and pkcs11 id).
-// 4. Send the connect request.
-// 5. Wait for the network state to change to a non connecting state.
-// 6. Invoke the appropriate callback (always) on success or failure.
+// 3. Send the connect request.
+// 4. Wait for the network state to change to a non connecting state.
+// 5. Invoke the appropriate callback (always) on success or failure.
 //
 // NetworkConnectionHandler depends on NetworkStateHandler for immediately
 // available State information, and NetworkConfigurationHandler for any
 // configuration calls.
 
 class CHROMEOS_EXPORT NetworkConnectionHandler
-    : public LoginState::Observer,
-      public CertLoader::Observer,
-      public NetworkStateHandlerObserver,
+    : public NetworkStateHandlerObserver,
       public base::SupportsWeakPtr<NetworkConnectionHandler> {
  public:
   // Constants for |error_name| from |error_callback| for Connect.
@@ -102,13 +96,6 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   virtual void NetworkListChanged() OVERRIDE;
   virtual void NetworkPropertiesUpdated(const NetworkState* network) OVERRIDE;
 
-  // LoginState::Observer
-  virtual void LoggedInStateChanged(LoginState::LoggedInState state) OVERRIDE;
-
-  // CertLoader::Observer
-  virtual void OnCertificatesLoaded(const net::CertificateList& cert_list,
-                                    bool initial_load) OVERRIDE;
-
  private:
   friend class NetworkHandler;
   friend class NetworkConnectionHandlerTest;
@@ -117,8 +104,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
 
   NetworkConnectionHandler();
 
-  void Init(CertLoader* cert_loader,
-            NetworkStateHandler* network_state_handler,
+  void Init(NetworkStateHandler* network_state_handler,
             NetworkConfigurationHandler* network_configuration_handler);
 
   ConnectRequest* pending_request(const std::string& service_path);
@@ -147,9 +133,6 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
 
   void CheckPendingRequest(const std::string service_path);
   void CheckAllPendingRequests();
-  bool CertificateIsConfigured(NetworkUIData* ui_data, std::string* pkcs11_id);
-  void ErrorCallbackForPendingRequest(const std::string& service_path,
-                                      const std::string& error_name);
 
   // Calls Shill.Manager.Disconnect asynchronously.
   void CallShillDisconnect(
@@ -167,19 +150,13 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
       const std::string& error_message);
 
   // Local references to the associated handler instances.
-  CertLoader* cert_loader_;
   NetworkStateHandler* network_state_handler_;
   NetworkProfileHandler* network_profile_handler_;
   NetworkConfigurationHandler* network_configuration_handler_;
 
-  // Map of pending connect requests, used to prevent repeated attempts while
+  // Map of pending connect requests, used to prevent repeat attempts while
   // waiting for Shill and to trigger callbacks on eventual success or failure.
   std::map<std::string, ConnectRequest> pending_requests_;
-  scoped_ptr<ConnectRequest> queued_connect_;
-
-  // Track certificate loading state.
-  bool logged_in_;
-  bool certificates_loaded_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandler);
 };
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index 8d8fd7a..5c09b9b 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -38,15 +38,12 @@ class NetworkConnectionHandlerTest : public testing::Test {
     DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface()
         ->ClearServices();
     message_loop_.RunUntilIdle();
-    LoginState::Initialize();
     network_state_handler_.reset(NetworkStateHandler::InitializeForTest());
     network_configuration_handler_.reset(
         NetworkConfigurationHandler::InitializeForTest(
             network_state_handler_.get()));
     network_connection_handler_.reset(new NetworkConnectionHandler);
-    // TODO(stevenjb): Test integration with CertLoader using a stub or mock.
-    network_connection_handler_->Init(NULL /* cert_loader */,
-                                      network_state_handler_.get(),
+    network_connection_handler_->Init(network_state_handler_.get(),
                                       network_configuration_handler_.get());
   }
 
@@ -54,7 +51,6 @@ class NetworkConnectionHandlerTest : public testing::Test {
     network_connection_handler_.reset();
     network_configuration_handler_.reset();
     network_state_handler_.reset();
-    LoginState::Shutdown();
     DBusThreadManager::Shutdown();
   }
 
diff --git a/chromeos/network/network_handler.cc b/chromeos/network/network_handler.cc
index cbe0595..5eb2a87 100644
--- a/chromeos/network/network_handler.cc
+++ b/chromeos/network/network_handler.cc
@@ -46,8 +46,7 @@ void NetworkHandler::Init() {
       network_state_handler_.get(),
       network_profile_handler_.get(),
       network_configuration_handler_.get());
-  network_connection_handler_->Init(cert_loader_.get(),
-                                    network_state_handler_.get(),
+  network_connection_handler_->Init(network_state_handler_.get(),
                                     network_configuration_handler_.get());
   geolocation_handler_->Init();
 }
@@ -60,15 +59,6 @@ void NetworkHandler::Initialize() {
 }
 
 // static
-void NetworkHandler::InitializeForTest() {
-  CHECK(!g_network_handler);
-  if (!LoginState::IsInitialized())
-    LoginState::Initialize();  // OK not to shutdown LoginState for tests.
-  g_network_handler = new NetworkHandler();
-  g_network_handler->Init();
-}
-
-// static
 void NetworkHandler::Shutdown() {
   CHECK(g_network_handler);
   delete g_network_handler;
diff --git a/chromeos/network/network_handler.h b/chromeos/network/network_handler.h
index 5499589..e3df697 100644
--- a/chromeos/network/network_handler.h
+++ b/chromeos/network/network_handler.h
@@ -27,9 +27,6 @@ class CHROMEOS_EXPORT NetworkHandler {
   // Sets the global instance. Must be called before any calls to Get().
   static void Initialize();
 
-  // Sets the global instance for testing.
-  static void InitializeForTest();
-
   // Destroys the global instance.
   static void Shutdown();
 
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index 427ab84..e0ac196 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -232,15 +232,6 @@ void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
                                              cellular_out_of_credits_);
 }
 
-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_);
 }
diff --git a/chromeos/network/network_state.h b/chromeos/network/network_state.h
index e87cba6..31ea5f9 100644
--- a/chromeos/network/network_state.h
+++ b/chromeos/network/network_state.h
@@ -35,10 +35,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& ip_address() const { return ip_address_; }