Fix reconnect in case of empty global network policy.

While there, remove some error messages because of incomplete test setup or incorrect fake behavior.

BUG=425049

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

Cr-Commit-Position: refs/heads/master@{#300284}

chromeos/dbus/fake_shill_service_client.cc

@@ -400,6 +400,9 @@ base::DictionaryValue* FakeShillServiceClient::SetServiceProperties(
     properties->SetWithoutPathExpansion(
         shill::kSecurityProperty,
         new base::StringValue(shill::kSecurityNone));
+    properties->SetWithoutPathExpansion(
+        shill::kModeProperty,
+        new base::StringValue(shill::kModeManaged));
   }
   return properties;
 }
@@ -441,11 +444,13 @@ bool FakeShillServiceClient::SetServiceProperty(const std::string& service_path,
   dict->MergeDictionary(&new_properties);
 
   // Add or update the profile entry.
+  ShillProfileClient::TestInterface* profile_test =
+      DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface();
   if (property == shill::kProfileProperty) {
     std::string profile_path;
     if (value.GetAsString(&profile_path)) {
-      DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface()->
-          AddService(profile_path, service_path);
+      if (!profile_path.empty())
+        profile_test->AddService(profile_path, service_path);
     } else {
       LOG(ERROR) << "Profile value is not a String!";
     }
@@ -453,8 +458,7 @@ bool FakeShillServiceClient::SetServiceProperty(const std::string& service_path,
     std::string profile_path;
     if (dict->GetStringWithoutPathExpansion(
             shill::kProfileProperty, &profile_path) && !profile_path.empty()) {
-      DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface()->
-          UpdateService(profile_path, service_path);
+      profile_test->UpdateService(profile_path, service_path);
     }
   }
 

chromeos/network/network_connection_handler.cc

@@ -787,14 +787,15 @@ void NetworkConnectionHandler::DisconnectIfPolicyRequires() {
       ::onc::global_network_config::kAllowOnlyPolicyNetworksToAutoconnect,
       &only_policy_autoconnect);
 
-  if (!only_policy_autoconnect)
-    return;
+  if (only_policy_autoconnect)
+    DisconnectFromUnmanagedSharedWiFiNetworks();
+
+  ConnectToBestNetworkAfterLogin();
+}
 
-  NET_LOG_DEBUG("DisconnectIfPolicyRequires",
-                "Disconnecting unmanaged and shared networks if any exist.");
+void NetworkConnectionHandler::DisconnectFromUnmanagedSharedWiFiNetworks() {
+  NET_LOG_DEBUG("DisconnectFromUnmanagedSharedWiFiNetworks", "");
 
-  // Get the list of unmanaged & shared networks that are connected or
-  // connecting.
   NetworkStateHandler::NetworkStateList networks;
   network_state_handler_->GetVisibleNetworkListByType(
       NetworkTypePattern::Wireless(), &networks);
@@ -820,8 +821,6 @@ void NetworkConnectionHandler::DisconnectIfPolicyRequires() {
     CallShillDisconnect(
         network->path(), base::Closure(), network_handler::ErrorCallback());
   }
-
-  ConnectToBestNetworkAfterLogin();
 }
 
 }  // namespace chromeos

chromeos/network/network_connection_handler.h

@@ -215,6 +215,10 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // when the device policy is changed.
   void DisconnectIfPolicyRequires();
 
+  // Disconnects from all unmanaged and shared WiFi networks that are currently
+  // connected or connecting.
+  void DisconnectFromUnmanagedSharedWiFiNetworks();
+
   // Requests a connect to the 'best' available network once after login and
   // after any disconnect required by policy is executed (see
   // DisconnectIfPolicyRequires()). To include networks with client

chromeos/network/network_connection_handler_unittest.cc

@@ -434,10 +434,11 @@ TEST_F(NetworkConnectionHandlerTest,
 namespace {
 
 const char* kConfigUnmanagedSharedConnected =
-    "{ \"GUID\": \"wifi0\", \"Type\": \"wifi\", \"State\": \"online\" }";
+    "{ \"GUID\": \"wifi0\", \"Type\": \"wifi\", \"State\": \"online\", "
+    "  \"Security\": \"wpa\" }";
 const char* kConfigManagedSharedConnectable =
     "{ \"GUID\": \"wifi1\", \"Type\": \"wifi\", \"State\": \"idle\", "
-    "  \"Connectable\": true }";
+    "  \"Connectable\": true, \"Security\": \"wpa\" }";
 
 const char* kPolicy =
     "[ { \"GUID\": \"wifi1\","
@@ -452,12 +453,13 @@ const char* kPolicy =
 
 }  // namespace
 
-TEST_F(NetworkConnectionHandlerTest, ReconnectOnLoginEarlyPolicyLoading) {
+TEST_F(NetworkConnectionHandlerTest, ReconnectOnCertLoading) {
   EXPECT_TRUE(Configure(kConfigUnmanagedSharedConnected));
   EXPECT_TRUE(Configure(kConfigManagedSharedConnectable));
   test_manager_client_->SetBestServiceToConnect("wifi1");
 
-  // User login shouldn't trigger any change because policy is not loaded yet.
+  // User login shouldn't trigger any change until the certificates and policy
+  // are loaded.
   LoginToRegularUser();
   EXPECT_EQ(shill::kStateOnline,
             GetServiceStringProperty("wifi0", shill::kStateProperty));
@@ -485,10 +487,9 @@ TEST_F(NetworkConnectionHandlerTest, ReconnectOnLoginEarlyPolicyLoading) {
             GetServiceStringProperty("wifi1", shill::kStateProperty));
 }
 
-TEST_F(NetworkConnectionHandlerTest, ReconnectOnLoginLatePolicyLoading) {
+TEST_F(NetworkConnectionHandlerTest, DisconnectOnPolicyLoading) {
   EXPECT_TRUE(Configure(kConfigUnmanagedSharedConnected));
   EXPECT_TRUE(Configure(kConfigManagedSharedConnectable));
-  test_manager_client_->SetBestServiceToConnect("wifi1");
 
   // User login and certificate loading shouldn't trigger any change until the
   // policy is loaded.
@@ -499,13 +500,38 @@ TEST_F(NetworkConnectionHandlerTest, ReconnectOnLoginLatePolicyLoading) {
   EXPECT_EQ(shill::kStateIdle,
             GetServiceStringProperty("wifi1", shill::kStateProperty));
 
-  // Applying the policy which restricts autoconnect should disconnect from the
-  // shared, unmanaged network.
   base::DictionaryValue global_config;
   global_config.SetBooleanWithoutPathExpansion(
       ::onc::global_network_config::kAllowOnlyPolicyNetworksToAutoconnect,
       true);
 
+  // Applying the policy which restricts autoconnect should disconnect from the
+  // shared, unmanaged network.
+  // Because no best service is set, the fake implementation of
+  // ConnectToBestServices will be a no-op.
+  SetupPolicy(kPolicy, global_config, false /* load as device policy */);
+  EXPECT_EQ(shill::kStateIdle,
+            GetServiceStringProperty("wifi0", shill::kStateProperty));
+  EXPECT_EQ(shill::kStateIdle,
+            GetServiceStringProperty("wifi1", shill::kStateProperty));
+}
+
+TEST_F(NetworkConnectionHandlerTest, ReconnectOnEmptyPolicyLoading) {
+  EXPECT_TRUE(Configure(kConfigUnmanagedSharedConnected));
+  EXPECT_TRUE(Configure(kConfigManagedSharedConnectable));
+  test_manager_client_->SetBestServiceToConnect("wifi1");
+
+  // User login and certificate loading shouldn't trigger any change until the
+  // policy is loaded.
+  LoginToRegularUser();
+  StartCertLoader();
+  EXPECT_EQ(shill::kStateOnline,
+            GetServiceStringProperty("wifi0", shill::kStateProperty));
+  EXPECT_EQ(shill::kStateIdle,
+            GetServiceStringProperty("wifi1", shill::kStateProperty));
+
+  // Apply an empty policy should trigger connecting to the 'best' network.
+  base::DictionaryValue global_config;
   SetupPolicy(kPolicy, global_config, false /* load as device policy */);
   EXPECT_EQ(shill::kStateIdle,
             GetServiceStringProperty("wifi0", shill::kStateProperty));
@@ -515,4 +541,4 @@ TEST_F(NetworkConnectionHandlerTest, ReconnectOnLoginLatePolicyLoading) {
 
 }  // namespace chromeos
 
-#endif
\ No newline at end of file
+#endif