Implement CrosNetworkConfig.StartConnect/Disconnect and use in Settings

This CL also fixes a minor bug the Cellular UI to show a disabled
'Connect' button if Cellular is not connectable (since it is never
configurable).

Bug: 853953
Change-Id: I218f34268b27a4a8fde10956a055d9235d329d77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769549
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692082}

chrome/browser/resources/settings/internet_page/internet_detail_page.js

@@ -701,12 +701,18 @@ Polymer({
     if (this.isArcVpn_(managedProperties)) {
       return false;
     }
-
+    if (managedProperties.connectionState !=
+        mojom.ConnectionStateType.kNotConnected) {
+      return false;
+    }
+    // Cellular is not configurable, so we show a disabled connect button of
+    // connectable is false.
+    if (managedProperties.type != mojom.NetworkType.kCellular) {
+      return true;
+    }
     // If 'connectable' is false we show the configure button.
     return managedProperties.connectable &&
-        managedProperties.type != mojom.NetworkType.kEthernet &&
-        managedProperties.connectionState ==
-        mojom.ConnectionStateType.kNotConnected;
+        managedProperties.type != mojom.NetworkType.kEthernet;
   },
 
   /**
@@ -909,6 +915,12 @@ Polymer({
     if (!propertiesReceived || outOfRange) {
       return false;
     }
+    // Cellular networks are not configurable, so we show a disabled 'Connect'
+    // button when not connectable.
+    if (managedProperties.type == mojom.NetworkType.kCellular &&
+        !managedProperties.connectable) {
+      return false;
+    }
     if (managedProperties.type == mojom.NetworkType.kVPN && !defaultNetwork) {
       return false;
     }
@@ -992,7 +1004,11 @@ Polymer({
 
   /** @private */
   onDisconnectTap_: function() {
-    this.networkingPrivate.startDisconnect(this.guid);
+    this.networkConfig_.startDisconnect(this.guid).then(response => {
+      if (!response.success) {
+        console.error('Disconnect failed for: ' + this.guid);
+      }
+    });
   },
 
   /** @private */

chrome/browser/resources/settings/internet_page/internet_page.js

@@ -640,24 +640,32 @@ Polymer({
       return;
     }
 
-    this.networkingPrivate.startConnect(networkState.guid, () => {
-      if (chrome.runtime.lastError) {
-        const message = chrome.runtime.lastError.message;
-        if (message == 'connecting' || message == 'connect-canceled' ||
-            message == 'connected' || message == 'Error.InvalidNetworkGuid') {
+    this.networkConfig_.startConnect(networkState.guid).then(response => {
+      switch (response.result) {
+        case mojom.StartConnectResult.kSuccess:
+          return;
+        case mojom.StartConnectResult.kInvalidGuid:
+        case mojom.StartConnectResult.kInvalidState:
+        case mojom.StartConnectResult.kCanceled:
+          // TODO(stevenjb/khorimoto): Consider handling these cases.
+          return;
+        case mojom.StartConnectResult.kNotConfigured:
+          if (!isMobile) {
+            this.showConfig_(
+                true /* configAndConnect */, oncType, networkState.guid,
+                displayName);
+          }
+          return;
+        case mojom.StartConnectResult.kBlocked:
+          // This shouldn't happen, the UI should prevent this, fall through and
+          // show the error.
+        case mojom.StartConnectResult.kUnknown:
+          console.error(
+              'startConnect failed for: ' + networkState.guid +
+              ' Error: ' + response.message);
           return;
-        }
-        console.error(
-            'networkingPrivate.startConnect error: ' + message +
-            ' For: ' + networkState.guid);
-
-        // There is no configuration flow for Mobile Networks.
-        if (!isMobile) {
-          this.showConfig_(
-              true /* configAndConnect */, oncType, networkState.guid,
-              displayName);
-        }
       }
+      assertNotReached();
     });
   },
 });

chromeos/network/network_connection_handler.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -17,6 +18,7 @@
 #include "chromeos/network/client_cert_util.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_configuration_handler.h"
+#include "chromeos/network/network_connection_handler_impl.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state.h"
@@ -132,4 +134,16 @@ void NetworkConnectionHandler::InitiateTetherNetworkDisconnection(
                  error_callback));
 }
 
+// static
+std::unique_ptr<NetworkConnectionHandler>
+NetworkConnectionHandler::InitializeForTesting(
+    NetworkStateHandler* network_state_handler,
+    NetworkConfigurationHandler* network_configuration_handler,
+    ManagedNetworkConfigurationHandler* managed_network_configuration_handler) {
+  NetworkConnectionHandlerImpl* handler = new NetworkConnectionHandlerImpl();
+  handler->Init(network_state_handler, network_configuration_handler,
+                managed_network_configuration_handler);
+  return base::WrapUnique(handler);
+}
+
 }  // namespace chromeos

chromeos/network/network_connection_handler.h

@@ -39,6 +39,10 @@ namespace chromeos {
 
 enum class ConnectCallbackMode { ON_STARTED, ON_COMPLETED };
 
+class NetworkStateHandler;
+class NetworkConfigurationHandler;
+class ManagedNetworkConfigurationHandler;
+
 class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConnectionHandler {
  public:
   // Constants for |error_name| from |error_callback| for Connect.
@@ -173,6 +177,13 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConnectionHandler {
                     ManagedNetworkConfigurationHandler*
                         managed_network_configuration_handler) = 0;
 
+  // Construct and initialize an instance for testing.
+  static std::unique_ptr<NetworkConnectionHandler> InitializeForTesting(
+      NetworkStateHandler* network_state_handler,
+      NetworkConfigurationHandler* network_configuration_handler,
+      ManagedNetworkConfigurationHandler*
+          managed_network_configuration_handler);
+
  protected:
   NetworkConnectionHandler();
 

chromeos/network/network_connection_handler_impl.h

@@ -46,7 +46,6 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConnectionHandlerImpl
   // NetworkCertLoader::Observer
   void OnCertificatesLoaded() override;
 
- protected:
   void Init(NetworkStateHandler* network_state_handler,
             NetworkConfigurationHandler* network_configuration_handler,
             ManagedNetworkConfigurationHandler*

chromeos/services/network_config/cros_network_config.h

@@ -19,6 +19,7 @@ class DictionaryValue;
 namespace chromeos {
 
 class ManagedNetworkConfigurationHandler;
+class NetworkConnectionHandler;
 class NetworkDeviceHandler;
 class NetworkStateHandler;
 
@@ -36,7 +37,8 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
   CrosNetworkConfig(
       NetworkStateHandler* network_state_handler,
       NetworkDeviceHandler* network_device_handler,
-      ManagedNetworkConfigurationHandler* network_configuration_handler);
+      ManagedNetworkConfigurationHandler* network_configuration_handler,
+      NetworkConnectionHandler* network_connection_handler);
   ~CrosNetworkConfig() override;
 
   void BindRequest(mojom::CrosNetworkConfigRequest request);
@@ -65,6 +67,10 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
       SelectCellularMobileNetworkCallback callback) override;
   void RequestNetworkScan(mojom::NetworkType type) override;
   void GetGlobalPolicy(GetGlobalPolicyCallback callback) override;
+  void StartConnect(const std::string& guid,
+                    StartConnectCallback callback) override;
+  void StartDisconnect(const std::string& guid,
+                       StartDisconnectCallback callback) override;
 
  private:
   void GetManagedPropertiesSuccess(int callback_id,
@@ -91,6 +97,16 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
       const std::string& error_name,
       std::unique_ptr<base::DictionaryValue> error_data);
 
+  void StartConnectSuccess(int callback_id);
+  void StartConnectFailure(int callback_id,
+                           const std::string& error_name,
+                           std::unique_ptr<base::DictionaryValue> error_data);
+  void StartDisconnectSuccess(int callback_id);
+  void StartDisconnectFailure(
+      int callback_id,
+      const std::string& error_name,
+      std::unique_ptr<base::DictionaryValue> error_data);
+
   // NetworkStateHandlerObserver
   void NetworkListChanged() override;
   void DeviceListChanged() override;
@@ -100,10 +116,13 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
   void DevicePropertiesUpdated(const DeviceState* device) override;
   void OnShuttingDown() override;
 
+  const std::string& GetServicePathFromGuid(const std::string& guid);
+
   NetworkStateHandler* network_state_handler_;    // Unowned
   NetworkDeviceHandler* network_device_handler_;  // Unowned
   ManagedNetworkConfigurationHandler*
-      network_configuration_handler_;  // Unowned
+      network_configuration_handler_;                     // Unowned
+  NetworkConnectionHandler* network_connection_handler_;  // Unowned
   mojo::InterfacePtrSet<mojom::CrosNetworkConfigObserver> observers_;
   mojo::BindingSet<mojom::CrosNetworkConfig> bindings_;
   int callback_id_ = 1;
@@ -114,6 +133,8 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
       set_cellular_sim_state_callbacks_;
   base::flat_map<int, SelectCellularMobileNetworkCallback>
       select_cellular_mobile_network_callbacks_;
+  base::flat_map<int, StartConnectCallback> start_connect_callbacks_;
+  base::flat_map<int, StartDisconnectCallback> start_disconnect_callbacks_;
   base::WeakPtrFactory<CrosNetworkConfig> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CrosNetworkConfig);

chromeos/services/network_config/cros_network_config_unittest.cc

@@ -15,6 +15,7 @@
 #include "chromeos/login/login_state/login_state.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_configuration_handler.h"
+#include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_device_handler.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state_handler.h"
@@ -54,9 +55,15 @@ class CrosNetworkConfigTest : public testing::Test {
             helper_.network_state_handler(), network_profile_handler_.get(),
             network_device_handler_.get(),
             network_configuration_handler_.get());
+    network_connection_handler_ =
+        NetworkConnectionHandler::InitializeForTesting(
+            helper_.network_state_handler(),
+            network_configuration_handler_.get(),
+            managed_network_configuration_handler_.get());
     cros_network_config_ = std::make_unique<CrosNetworkConfig>(
         helper_.network_state_handler(), network_device_handler_.get(),
-        managed_network_configuration_handler_.get());
+        managed_network_configuration_handler_.get(),
+        network_connection_handler_.get());
     SetupPolicy();
     SetupNetworks();
   }
@@ -285,6 +292,38 @@ class CrosNetworkConfigTest : public testing::Test {
     return result;
   }
 
+  mojom::StartConnectResult StartConnect(const std::string& guid) {
+    mojom::StartConnectResult result;
+    base::RunLoop run_loop;
+    cros_network_config()->StartConnect(
+        guid,
+        base::BindOnce(
+            [](mojom::StartConnectResult* resultp,
+               base::OnceClosure quit_closure, mojom::StartConnectResult result,
+               const std::string& message) {
+              *resultp = result;
+              std::move(quit_closure).Run();
+            },
+            &result, run_loop.QuitClosure()));
+    run_loop.Run();
+    return result;
+  }
+
+  bool StartDisconnect(const std::string& guid) {
+    bool success = false;
+    base::RunLoop run_loop;
+    cros_network_config()->StartDisconnect(
+        guid,
+        base::BindOnce(
+            [](bool* successp, base::OnceClosure quit_closure, bool success) {
+              *successp = success;
+              std::move(quit_closure).Run();
+            },
+            &success, run_loop.QuitClosure()));
+    run_loop.Run();
+    return success;
+  }
+
   NetworkStateTestHelper& helper() { return helper_; }
   CrosNetworkConfigTestObserver* observer() { return observer_.get(); }
   CrosNetworkConfig* cros_network_config() {
@@ -303,6 +342,7 @@ class CrosNetworkConfigTest : public testing::Test {
   std::unique_ptr<NetworkConfigurationHandler> network_configuration_handler_;
   std::unique_ptr<ManagedNetworkConfigurationHandler>
       managed_network_configuration_handler_;
+  std::unique_ptr<NetworkConnectionHandler> network_connection_handler_;
   std::unique_ptr<CrosNetworkConfig> cros_network_config_;
   std::unique_ptr<CrosNetworkConfigTestObserver> observer_;
   std::string wifi1_path_;
@@ -791,6 +831,46 @@ TEST_F(CrosNetworkConfigTest, GetGlobalPolicy) {
   EXPECT_EQ("blocked_ssid2", policy->blocked_hex_ssids[1]);
 }
 
+TEST_F(CrosNetworkConfigTest, StartConnect) {
+  // wifi1 is already connected, StartConnect should fail.
+  mojom::StartConnectResult result = StartConnect("wifi1_guid");
+  EXPECT_EQ(mojom::StartConnectResult::kInvalidState, result);
+
+  // wifi2 is not connected, StartConnect should succeed and connection_state
+  // should change to connecting.
+  mojom::NetworkStatePropertiesPtr network = GetNetworkState("wifi2_guid");
+  ASSERT_TRUE(network);
+  EXPECT_EQ(mojom::ConnectionStateType::kNotConnected,
+            network->connection_state);
+  result = StartConnect("wifi2_guid");
+  EXPECT_EQ(mojom::StartConnectResult::kSuccess, result);
+  network = GetNetworkState("wifi2_guid");
+  EXPECT_EQ(mojom::ConnectionStateType::kConnecting, network->connection_state);
+  // Wait for disconnect to complete.
+  base::RunLoop().RunUntilIdle();
+  network = GetNetworkState("wifi2_guid");
+  EXPECT_EQ(mojom::ConnectionStateType::kOnline, network->connection_state);
+}
+
+TEST_F(CrosNetworkConfigTest, StartDisconnect) {
+  // wifi1 is connected, StartDisconnect should succeed and connection_state
+  // should change to disconnected.
+  mojom::NetworkStatePropertiesPtr network = GetNetworkState("wifi1_guid");
+  ASSERT_TRUE(network);
+  EXPECT_EQ(mojom::ConnectionStateType::kConnected, network->connection_state);
+  bool success = StartDisconnect("wifi1_guid");
+  EXPECT_TRUE(success);
+  // Wait for disconnect to complete.
+  base::RunLoop().RunUntilIdle();
+  network = GetNetworkState("wifi1_guid");
+  EXPECT_EQ(mojom::ConnectionStateType::kNotConnected,
+            network->connection_state);
+
+  // wifi1 is now disconnected, StartDisconnect should fail.
+  success = StartDisconnect("wifi1_guid");
+  EXPECT_FALSE(success);
+}
+
 TEST_F(CrosNetworkConfigTest, NetworkListChanged) {
   SetupObserver();
   base::RunLoop().RunUntilIdle();

chromeos/services/network_config/public/cpp/cros_network_config_test_helper.cc

@@ -19,7 +19,8 @@ CrosNetworkConfigTestHelper::CrosNetworkConfigTestHelper() {
       std::make_unique<chromeos::network_config::CrosNetworkConfig>(
           network_state_helper_.network_state_handler(),
           network_device_handler_.get(),
-          /*network_configuration_handler=*/nullptr);
+          /*network_configuration_handler=*/nullptr,
+          /*network_connection_handler=*/nullptr);
   OverrideInProcessInstanceForTesting(cros_network_config_impl_.get());
 }
 

chromeos/services/network_config/public/mojom/cros_network_config.mojom

@@ -143,6 +143,26 @@ enum FilterType {
   kAll,
 };
 
+enum StartConnectResult {
+  // The request was successfully sent.
+  kSuccess,
+  // The provided GUID is not associated with an existing network configuration.
+  kInvalidGuid,
+  // The network is not in a connectable state (e.g. already connected or
+  // connecting, or a required service is unavailable).
+  kInvalidState,
+  // The connect request was cancelled before being sent, e.g. due to another
+  // connect request.
+  kCanceled,
+  // The network is in an error state because invalid credentials were provided
+  // or a certificate is unavailable.
+  kNotConfigured,
+  // Connecting to the network is blocked, e.g. by policy.
+  kBlocked,
+  // Unknown or other error.
+  kUnknown,
+};
+
 struct CaptivePortalProvider {
   // Id used to identify the captive portal provider (i.e. an extension id).
   string id;
@@ -740,6 +760,19 @@ interface CrosNetworkConfig {
   // Returns the global policy properties. These properties are not expected to
   // change during a session.
   GetGlobalPolicy() => (GlobalPolicy result);
+
+  // Starts a connection to the network matching |guid|. The response returns
+  // |result| when the connect request is sent or is determined unable to be
+  // sent, not when the operation completes. Use CrosNetworkConfigObserver to
+  // observe the connection state after a successful request. If the result is
+  // not kSuccess, |message| will contain a descriptive string for debugging.
+  StartConnect(string guid) => (StartConnectResult result, string message);
+
+  // Starts a disconnect from the network matching |guid|. The response returns
+  // true when the disconnect request is sent if the network exists and is
+  // connected, or false if not. Use CrosNetworkConfigObserver to observe the
+  // connection state after a successful request.
+  StartDisconnect(string guid) => (bool success);
 };
 
 interface CrosNetworkConfigObserver {