Let AutoConnectHandler wait for user policy before disconnecting

chromeos/network/auto_connect_handler.cc

 // Copyright 2014 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/auto_connect_handler.h"
 
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
@@ skipping 82 matching lines @@
 }
 
 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())
+  if (userhash.empty()) {
     device_policy_applied_ = true;
-  else
+  } else {
     user_policy_applied_ = true;
+    DisconnectIfPolicyRequires();

[stevenjb, 2015/11/09 17:42:53]

I assume device policy is guaranteed to already be set when user_policy_applied_
is set to true? Might be worth adding a DCHECK(device_policy_applied_) here to
document that.

[cschuet (SLOW), 2015/11/09 17:44:38]

I don't think that is the case. Managed users can be signed into an unmanaged
Chromebook.

[stevenjb, 2015/11/09 18:05:51]

I see. So what happens if a device policy is updated and requires disconnecting
from the current network?

[fqj, 2015/11/09 18:26:20]

then PoliciesChanged will be called, which will call
DisconnectIfPolicyRequires().

[stevenjb, 2015/11/09 18:43:50]

I see. It looks like we never execute DisconnectIfPolicyRequires() until the
user logs in (i.e. we assume that a device policy update doesn't require a
disconnect, which may be fine).

[cschuet (SLOW), 2015/11/09 18:54:43]

Qijian, please update the comments in the header file as well to reflect the
modification in the behavior.

+  }
 
   // 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 =
       managed_configuration_handler_->GetNetworkConfigsFromPolicy(userhash);
   DCHECK(managed_networks);
   if (!managed_networks->empty()) {
     NET_LOG_DEBUG("RequestBestConnection", "Policy applied");
     RequestBestConnection();
   } else {
@@ skipping 73 matching lines @@
   if (!network_state_handler_->GetScanningByType(
           NetworkTypePattern::Primitive(shill::kTypeWifi))) {
     network_state_handler_->RequestScan();
   }
 }
 
 void AutoConnectHandler::DisconnectIfPolicyRequires() {
   if (applied_autoconnect_policy_ || !LoginState::Get()->IsUserLoggedIn())
     return;
 
+  if (!user_policy_applied_)
+    return;

[stevenjb, 2015/11/09 18:43:50]

Merge this if() with the one above; I missed the !IsUserLoggedIn() condition
above when looking at this before, it will be more clear to combine them all.

[fqj, 2015/11/09 18:50:08]

Done.

+
   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;
@@ skipping 37 matching lines @@
 void AutoConnectHandler::CallShillConnectToBestServices() const {
   NET_LOG_EVENT("ConnectToBestServices", "");
   DBusThreadManager::Get()->GetShillManagerClient()->ConnectToBestServices(
       base::Bind(&base::DoNothing),
       base::Bind(&network_handler::ShillErrorCallbackFunction,
                  "ConnectToBestServices Failed",
                  "", network_handler::ErrorCallback()));
 }
 
 }  // namespace chromeos