Fix AutoConnectHandler for AllowOnlyPolicyNetworksTo[Auto]Connect

Fix1: If either AllowOnlyPolicyNetworksToConnect or
AllowOnlyPolicyNetworksToAutoconnect is enabled, this CL will
additionally remove the network configurations
(AllowOnlyPolicyNetworksToConnect) or disable auto-connect (for
AllowOnlyPolicyNetworksToAutoconnect) for the unmanaged networks to
prevent Shill from re-connecting when searching for a best service to
connect.

Fix2: Fixed missing disconnect, when AllowOnlyPolicyNetworksToConnect
gets enabled. This disconnect was incorrectly prevented by the
|applied_autoconnect_policy_| boolean before, which ensures that the
disconnect on AllowOnlyPolicyNetworksToAutoconnect only happens once.

Bug: 850550
Change-Id: I869ed49d58adb82707e8856e896c5a141f6f6625
Reviewed-on: https://chromium-review.googlesource.com/1091051Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Alexander Hendrich <hendrich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567199}

chromeos/network/auto_connect_handler.cc

@@ -44,6 +44,32 @@ void DisconnectErrorCallback(
                  << "Error data: " << error_data_ss.str();
 }
 
+void RemoveNetworkConfigurationErrorCallback(
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
+  std::stringstream error_data_ss;
+  if (error_data)
+    error_data_ss << *error_data;
+  else
+    error_data_ss << "<none>";
+  NET_LOG(ERROR) << "AutoConnectHandler RemoveNetworkConfiguration failed. "
+                 << "Error name: \"" << error_name << "\", "
+                 << "Error data: " << error_data_ss.str();
+}
+
+void SetPropertiesErrorCallback(
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
+  std::stringstream error_data_ss;
+  if (error_data)
+    error_data_ss << *error_data;
+  else
+    error_data_ss << "<none>";
+  NET_LOG(ERROR) << "AutoConnectHandler SetProperties failed. "
+                 << "Error name: \"" << error_name << "\", "
+                 << "Error data: " << error_data_ss.str();
+}
+
 std::string AutoConnectReasonsToString(int auto_connect_reasons) {
   std::string result;
 
@@ -139,21 +165,15 @@ void AutoConnectHandler::ConnectToNetworkRequested(
   request_best_connection_pending_ = false;
 }
 
-void AutoConnectHandler::PoliciesChanged(const std::string& userhash) {
-  // Ignore user policies.
-  if (!userhash.empty())
-    return;
-  DisconnectIfPolicyRequires();
-}
-
 void AutoConnectHandler::PoliciesApplied(const std::string& userhash) {
   if (userhash.empty()) {
     device_policy_applied_ = true;
   } else {
     user_policy_applied_ = true;
-    DisconnectIfPolicyRequires();
   }
 
+  DisconnectIfPolicyRequires();
+
   // Request to connect to the best network only if there is at least one
   // managed network. Otherwise only process existing requests.
   const ManagedNetworkConfigurationHandler::GuidToPolicyMap* managed_networks =
@@ -263,60 +283,98 @@ void AutoConnectHandler::CheckBestConnection() {
 }
 
 void AutoConnectHandler::DisconnectIfPolicyRequires() {
-  if (applied_autoconnect_policy_ || !LoginState::Get()->IsUserLoggedIn() ||
-      !user_policy_applied_) {
+  if (!LoginState::Get()->IsUserLoggedIn() || !user_policy_applied_)
     return;
-  }
 
   const base::DictionaryValue* global_network_config =
       managed_configuration_handler_->GetGlobalConfigFromPolicy(
           std::string() /* no username hash, device policy */);
-
   if (!global_network_config)
     return;  // Device policy is not set, yet.
 
-  applied_autoconnect_policy_ = true;
-
-  bool only_policy_autoconnect = false;
-  global_network_config->GetBooleanWithoutPathExpansion(
-      ::onc::global_network_config::kAllowOnlyPolicyNetworksToAutoconnect,
-      &only_policy_autoconnect);
-  bool only_policy_connect = false;
-  global_network_config->GetBooleanWithoutPathExpansion(
+  const base::Value* connect_value = global_network_config->FindKeyOfType(
       ::onc::global_network_config::kAllowOnlyPolicyNetworksToConnect,
-      &only_policy_connect);
+      base::Value::Type::BOOLEAN);
+  bool only_policy_connect = connect_value ? connect_value->GetBool() : false;
 
-  if (only_policy_autoconnect || only_policy_connect)
-    DisconnectFromUnmanagedSharedWiFiNetworks();
+  const base::Value* autoconnect_value = global_network_config->FindKeyOfType(
+      ::onc::global_network_config::kAllowOnlyPolicyNetworksToAutoconnect,
+      base::Value::Type::BOOLEAN);
+  bool only_policy_autoconnect =
+      autoconnect_value ? autoconnect_value->GetBool() : false;
+
+  // Reset |applied_autoconnect_policy_| if auto-connect policy is disabled.
+  if (!only_policy_autoconnect)
+    applied_autoconnect_policy_ = false;
+
+  if (only_policy_connect) {
+    // Disconnect and remove network configurations for all unmanaged networks.
+    DisconnectFromAllUnmanagedWiFiNetworks(true, false);
+  } else if (only_policy_autoconnect && !applied_autoconnect_policy_) {
+    // Disconnect and disable auto-connect for all unmanaged networks.
+    DisconnectFromAllUnmanagedWiFiNetworks(false, true);
+    applied_autoconnect_policy_ = true;
+  }
 }
 
-void AutoConnectHandler::DisconnectFromUnmanagedSharedWiFiNetworks() {
-  NET_LOG_DEBUG("DisconnectFromUnmanagedSharedWiFiNetworks", "");
+void AutoConnectHandler::DisconnectFromAllUnmanagedWiFiNetworks(
+    bool remove_configuration,
+    bool disable_auto_connect) {
+  NET_LOG_DEBUG("DisconnectFromAllUnmanagedWiFiNetworks", "");
 
   NetworkStateHandler::NetworkStateList networks;
-  network_state_handler_->GetVisibleNetworkListByType(
-      NetworkTypePattern::WiFi(), &networks);
+  network_state_handler_->GetNetworkListByType(NetworkTypePattern::WiFi(),
+                                               false, false, 0, &networks);
   for (const NetworkState* network : networks) {
-    if (!(network->IsConnectingState() || network->IsConnectedState()))
-      break;  // Connected and connecting networks are listed first.
-
-    if (network->IsPrivate())
-      continue;
-
-    const bool network_is_policy_managed =
+    const bool is_managed =
         !network->profile_path().empty() && !network->guid().empty() &&
         managed_configuration_handler_->FindPolicyByGuidAndProfile(
             network->guid(), network->profile_path(), nullptr /* onc_source */);
-    if (network_is_policy_managed)
+
+    if (is_managed)
       continue;
 
-    NET_LOG_EVENT("Disconnect Forced by Policy", network->path());
-    network_connection_handler_->DisconnectNetwork(
-        network->path(), base::DoNothing(),
-        base::Bind(&DisconnectErrorCallback, network->path()));
+    if (network->IsConnectingOrConnected())
+      DisconnectNetwork(network->path());
+
+    if (network->IsInProfile()) {
+      if (remove_configuration)
+        RemoveNetworkConfigurationForNetwork(network->path());
+      else if (disable_auto_connect)
+        DisableAutoconnectForWiFiNetwork(network->path());
+    }
   }
 }
 
+void AutoConnectHandler::DisconnectNetwork(const std::string& service_path) {
+  NET_LOG_EVENT("Disconnect forced by policy", service_path);
+
+  network_connection_handler_->DisconnectNetwork(
+      service_path, base::DoNothing(),
+      base::Bind(&DisconnectErrorCallback, service_path));
+}
+
+void AutoConnectHandler::RemoveNetworkConfigurationForNetwork(
+    const std::string& service_path) {
+  NET_LOG_EVENT("Disconnect forced by policy", service_path);
+
+  managed_configuration_handler_->RemoveConfiguration(
+      service_path, base::DoNothing(),
+      base::Bind(&RemoveNetworkConfigurationErrorCallback));
+}
+
+void AutoConnectHandler::DisableAutoconnectForWiFiNetwork(
+    const std::string& service_path) {
+  NET_LOG_EVENT("Disable auto-connect forced by policy", service_path);
+
+  base::DictionaryValue properties;
+  properties.SetPath({::onc::network_config::kWiFi, ::onc::wifi::kAutoConnect},
+                     base::Value(false));
+  managed_configuration_handler_->SetProperties(
+      service_path, properties, base::DoNothing(),
+      base::Bind(&SetPropertiesErrorCallback));
+}
+
 void AutoConnectHandler::CallShillConnectToBestServices() {
   NET_LOG(EVENT) << "ConnectToBestServices ["
                  << AutoConnectReasonsToString(auto_connect_reasons_) << "]";

chromeos/network/auto_connect_handler.h

@@ -48,7 +48,6 @@ class CHROMEOS_EXPORT AutoConnectHandler : public LoginState::Observer,
   void ConnectToNetworkRequested(const std::string& service_path) override;
 
   // NetworkPolicyObserver
-  void PoliciesChanged(const std::string& userhash) override;
   void PoliciesApplied(const std::string& userhash) override;
 
   // NetworkStateHandlerObserver
@@ -76,19 +75,38 @@ class CHROMEOS_EXPORT AutoConnectHandler : public LoginState::Observer,
 
   void NotifyAutoConnectInitiated(int auto_connect_reasons);
 
-  // If the user logged in already and the policy to prevent unmanaged & shared
-  // networks to autoconnect is enabled, then disconnects all such networks
-  // except wired networks. It will do this only once after the user logged in
-  // and the device policy and user policy was available.
-  // This is enforced once after a user logs in 1) to allow mananged networks to
-  // autoconnect and 2) to prevent a previous user from foisting a network on
-  // the new user. Therefore, this function is called at login and when the
-  // device policy is changed after user policy is fetched and applied.
+  // This function is called whenever the logged in state changes or when a new
+  // policy is applied. Once both device and user policy have been applied and
+  // either of AllowOnlyPolicyNetworksToConnect or
+  // AllowOnlyPolicyNetworksToAutoconnect is enabled, we disconnect from all
+  // connecting/connected unmanaged networks and either remove the network
+  // configuration (for AllowOnlyPolicyNetworksToConnect) or only disable
+  // auto-connect (for AllowOnlyPolicyNetworksToAutoconnect) for all aunmanaged
+  // networks (see |DisconnectFromAllUnmanagedWiFiNetworks(...)|).
+  // For the AllowOnlyPolicyNetworksToAutoconnect policy we only disconnect once
+  // to allow managed networks to auto-connect and prevent disconnects with
+  // manually connected unmanaged networks on every policy update.
   void DisconnectIfPolicyRequires();
 
-  // Disconnects from all unmanaged and shared WiFi networks that are currently
-  // connected or connecting.
-  void DisconnectFromUnmanagedSharedWiFiNetworks();
+  // Disconnects from all currently connected/connecting unmanaged WiFis.
+  // When |remove_configuration|==true, we also remove the corresponding network
+  // configuration for all unmanaged networks from Shill.
+  // When |disable_auto_connect|==true, we also disable auto-connect for all
+  // unmanaged networks in Shill.
+  // With both options we can prevent Shill from re-connecting to the unmanaged
+  // networks when looking for a best service to connect to.
+  void DisconnectFromAllUnmanagedWiFiNetworks(bool remove_configuration,
+                                              bool disable_auto_connect);
+
+  // Disconnects the connection to the network represented by |service_path|.
+  void DisconnectNetwork(const std::string& service_path);
+
+  // Removes the network configuration for the network represented by
+  // |service_path|.
+  void RemoveNetworkConfigurationForNetwork(const std::string& service_path);
+
+  // Sets WiFi.AutoConnect=false for the network represented by |service_path|.
+  void DisableAutoconnectForWiFiNetwork(const std::string& service_path);
 
   // Requests and if possible connects to the 'best' available network, see
   // CheckBestConnection().