Signal network policy application done when it's actually done

Wait for network policy application to actually finish before signaling
that it's done. Network policy application is only finished when the
changes sent to shill have been pulled back from shill and are now
reflected in NetworkState objects. This is important because code
evaluating e.g. the OnlyAllowPolicyNetworksToAutoConnect policy uses
NetworkState objects to decide whether a network is policy-managed or
not.

Bug: 936677, 943102
Test: browsertest will be added in a follow-up CL to keep this small.
Change-Id: I734bc75eb8422e934b20cb592ca3fc5c211779b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1539839
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667328}

chromeos/network/auto_connect_handler_unittest.cc

@@ -127,6 +127,7 @@ class AutoConnectHandlerTest : public testing::Test {
     NetworkCertLoader::ForceHardwareBackedForTesting();
 
     LoginState::Initialize();
+    LoginState::Get()->set_always_logged_in(false);
 
     network_config_handler_.reset(
         NetworkConfigurationHandler::InitializeForTest(
@@ -357,12 +358,14 @@ TEST_F(AutoConnectHandlerTest, ReconnectOnCertPatternResolved) {
   SetupPolicy(std::string(),            // no device policy
               base::DictionaryValue(),  // no global config
               false);                   // load as device policy
+  EXPECT_EQ(0, test_observer_->num_auto_connect_events());
+
   LoginToRegularUser();
-  StartNetworkCertLoader();
   SetupPolicy(kPolicyCertPattern,
               base::DictionaryValue(),  // no global config
               true);                    // load as user policy
-  EXPECT_EQ(2, test_observer_->num_auto_connect_events());
+  StartNetworkCertLoader();
+  EXPECT_EQ(1, test_observer_->num_auto_connect_events());
   EXPECT_EQ(AutoConnectHandler::AUTO_CONNECT_REASON_LOGGED_IN |
                 AutoConnectHandler::AUTO_CONNECT_REASON_POLICY_APPLIED |
                 AutoConnectHandler::AUTO_CONNECT_REASON_CERTIFICATE_RESOLVED,
@@ -382,7 +385,7 @@ TEST_F(AutoConnectHandlerTest, ReconnectOnCertPatternResolved) {
 
   EXPECT_EQ(shill::kStateIdle, GetServiceState("wifi0"));
   EXPECT_EQ(shill::kStateOnline, GetServiceState("wifi1"));
-  EXPECT_EQ(3, test_observer_->num_auto_connect_events());
+  EXPECT_EQ(2, test_observer_->num_auto_connect_events());
   EXPECT_EQ(AutoConnectHandler::AUTO_CONNECT_REASON_LOGGED_IN |
                 AutoConnectHandler::AUTO_CONNECT_REASON_POLICY_APPLIED |
                 AutoConnectHandler::AUTO_CONNECT_REASON_CERTIFICATE_RESOLVED,

chromeos/network/managed_network_configuration_handler_impl.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/guid.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -72,12 +73,15 @@ void InvokeErrorCallback(const std::string& service_path,
       error_callback, service_path, error_name, error_msg);
 }
 
-void LogErrorWithDict(const base::Location& from_where,
-                      const std::string& error_name,
-                      std::unique_ptr<base::DictionaryValue> error_data) {
+void LogErrorWithDictAndCallCallback(
+    base::OnceClosure callback,
+    const base::Location& from_where,
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
   device_event_log::AddEntry(from_where.file_name(), from_where.line_number(),
                              device_event_log::LOG_TYPE_NETWORK,
                              device_event_log::LOG_LEVEL_ERROR, error_name);
+  std::move(callback).Run();
 }
 
 const base::DictionaryValue* GetByGUID(const GuidToPolicyMap& policies,
@@ -677,19 +681,26 @@ void ManagedNetworkConfigurationHandlerImpl::OnProfileRemoved(
 }
 
 void ManagedNetworkConfigurationHandlerImpl::CreateConfigurationFromPolicy(
-    const base::DictionaryValue& shill_properties) {
+    const base::DictionaryValue& shill_properties,
+    base::OnceClosure callback) {
+  base::RepeatingClosure adapted_callback =
+      base::AdaptCallbackForRepeating(std::move(callback));
   network_configuration_handler_->CreateShillConfiguration(
       shill_properties,
-      base::Bind(
+      base::BindRepeating(
           &ManagedNetworkConfigurationHandlerImpl::OnPolicyAppliedToNetwork,
-          weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&LogErrorWithDict, FROM_HERE));
+          weak_ptr_factory_.GetWeakPtr(), adapted_callback),
+      base::BindRepeating(&LogErrorWithDictAndCallCallback, adapted_callback,
+                          FROM_HERE));
 }
 
 void ManagedNetworkConfigurationHandlerImpl::
     UpdateExistingConfigurationWithPropertiesFromPolicy(
         const base::DictionaryValue& existing_properties,
-        const base::DictionaryValue& new_properties) {
+        const base::DictionaryValue& new_properties,
+        base::OnceClosure callback) {
+  base::RepeatingClosure adapted_callback =
+      base::AdaptCallbackForRepeating(std::move(callback));
   base::DictionaryValue shill_properties;
 
   std::string profile;
@@ -716,10 +727,11 @@ void ManagedNetworkConfigurationHandlerImpl::
 
   network_configuration_handler_->CreateShillConfiguration(
       shill_properties,
-      base::Bind(
+      base::BindRepeating(
           &ManagedNetworkConfigurationHandlerImpl::OnPolicyAppliedToNetwork,
-          weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&LogErrorWithDict, FROM_HERE));
+          weak_ptr_factory_.GetWeakPtr(), adapted_callback),
+      base::BindRepeating(&LogErrorWithDictAndCallCallback, adapted_callback,
+                          FROM_HERE));
 }
 
 void ManagedNetworkConfigurationHandlerImpl::OnPoliciesApplied(
@@ -949,12 +961,21 @@ void ManagedNetworkConfigurationHandlerImpl::Init(
 }
 
 void ManagedNetworkConfigurationHandlerImpl::OnPolicyAppliedToNetwork(
+    base::OnceClosure callback,
     const std::string& service_path,
     const std::string& guid) {
-  if (service_path.empty())
-    return;
+  DCHECK(!service_path.empty());
+
+  // When this is called, the policy has been fully applied and is reflected in
+  // NetworkStateHandler, so it is safe to notify obserers.
+  // Notifying observers is the last step of policy application to
+  // |service_path|.
   for (auto& observer : observers_)
     observer.PolicyAppliedToNetwork(service_path);
+
+  // Inform the caller that has requested policy application that it has
+  // finished.
+  std::move(callback).Run();
 }
 
 // Get{Managed}Properties helpers

chromeos/network/managed_network_configuration_handler_impl.h

@@ -116,11 +116,13 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
 
   // PolicyApplicator::ConfigurationHandler overrides
   void CreateConfigurationFromPolicy(
-      const base::DictionaryValue& shill_properties) override;
+      const base::DictionaryValue& shill_properties,
+      base::OnceClosure callback) override;
 
   void UpdateExistingConfigurationWithPropertiesFromPolicy(
       const base::DictionaryValue& existing_properties,
-      const base::DictionaryValue& new_properties) override;
+      const base::DictionaryValue& new_properties,
+      base::OnceClosure callback) override;
 
   void OnPoliciesApplied(const NetworkProfile& profile) override;
 
@@ -176,7 +178,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
   // user or device policies.
   const Policies* GetPoliciesForProfile(const NetworkProfile& profile) const;
 
-  void OnPolicyAppliedToNetwork(const std::string& service_path,
+  // Called when a policy identified by |guid| has been applied to the network
+  // identified by |service_path|. Notifies observers and calls |callback|.
+  void OnPolicyAppliedToNetwork(base::OnceClosure callback,
+                                const std::string& service_path,
                                 const std::string& guid);
 
   // Helper method to append associated Device properties to |properties|.

chromeos/network/network_configuration_handler.cc

@@ -523,10 +523,8 @@ void NetworkConfigurationHandler::ConfigurationCompleted(
 
   // |configure_callbacks_| will get triggered when NetworkStateHandler
   // notifies this that a state list update has occurred. |service_path|
-  // is unique per configuration. In the unlikely case that an existing
-  // configuration is reconfigured twice without a NetworkStateHandler update,
-  // (the UI should prevent that) the first callback will not get called.
-  configure_callbacks_[service_path.value()] = callback;
+  // is unique per configuration.
+  configure_callbacks_.insert(std::make_pair(service_path.value(), callback));
 }
 
 void NetworkConfigurationHandler::ProfileEntryDeleterCompleted(

chromeos/network/network_configuration_handler.h

@@ -91,6 +91,8 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConfigurationHandler
 
   // Creates a network with the given |properties| in the specified Shill
   // profile, and returns the new service_path to |callback| if successful.
+  // |callback| will only be called after the property update has been reflected
+  // in NetworkStateHandler.
   // kProfileProperty must be set in |properties|. This may also be used to
   // update an existing matching configuration, see Shill documentation for
   // Manager.ConfigureServiceForProfile. NOTE: Normally
@@ -226,8 +228,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConfigurationHandler
       profile_entry_deleters_;
 
   // Map of configuration callbacks to run once the service becomes available
-  // in the NetworkStateHandler cache.
-  std::map<std::string, network_handler::ServiceResultCallback>
+  // in the NetworkStateHandler cache. This is a multimap because there can be
+  // multiple callbacks for the same network that have to be notified.
+  std::multimap<std::string, network_handler::ServiceResultCallback>
       configure_callbacks_;
 
   base::ObserverList<NetworkConfigurationObserver, true>::Unchecked observers_;

chromeos/network/policy_applicator.h

@@ -11,12 +11,15 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
 #include "base/values.h"
 #include "chromeos/network/network_profile.h"
 
 namespace chromeos {
+class NetworkUIData;
 
 // This class compares (entry point is Run()) |modified_policies| with the
 // existing entries in the provided Shill profile |profile|. It fetches all
@@ -31,13 +34,18 @@ class PolicyApplicator {
     // Write the new configuration with the properties |shill_properties| to
     // Shill. This configuration comes from a policy. Any conflicting or
     // existing configuration for the same network will have been removed
-    // before.
+    // before. |callback| will be called after the configuration update has been
+    // reflected in NetworkStateHandler, or on error.
     virtual void CreateConfigurationFromPolicy(
-        const base::DictionaryValue& shill_properties) = 0;
+        const base::DictionaryValue& shill_properties,
+        base::OnceClosure callback) = 0;
 
+    // before. |callback| will be called after the configuration update has been
+    // reflected in NetworkStateHandler, or on error.
     virtual void UpdateExistingConfigurationWithPropertiesFromPolicy(
         const base::DictionaryValue& existing_properties,
-        const base::DictionaryValue& new_properties) = 0;
+        const base::DictionaryValue& new_properties,
+        base::OnceClosure callback) = 0;
 
     // Called after all policies for |profile| were applied. At this point, the
     // list of networks should be updated.
@@ -51,23 +59,18 @@ class PolicyApplicator {
       std::map<std::string, std::unique_ptr<base::DictionaryValue>>;
 
   // |handler| must outlive this object.
-  // |modified_policies| must not be NULL and will be empty afterwards.
+  // |modified_policy_guids| must not be nullptr and will be empty afterwards.
   PolicyApplicator(const NetworkProfile& profile,
                    const GuidToPolicyMap& all_policies,
                    const base::DictionaryValue& global_network_config,
                    ConfigurationHandler* handler,
-                   std::set<std::string>* modified_policies);
+                   std::set<std::string>* modified_policy_guids);
 
   ~PolicyApplicator();
 
   void Run();
 
  private:
-  // Removes |entry| from the list of pending profile entries.
-  // If all entries were processed, applies the remaining policies and notifies
-  // |handler_|.
-  void ProfileEntryFinished(const std::string& entry);
-
   // Called with the properties of the profile |profile_|. Requests the
   // properties of each entry, which are processed by GetEntryCallback.
   void GetProfilePropertiesCallback(
@@ -84,31 +87,68 @@ class PolicyApplicator {
                      const std::string& error_name,
                      const std::string& error_message);
 
+  // Applies |new_policy| for |entry|.
+  // |entry_properties| are the current properties for the entry. |ui_data| is
+  // the NetworkUIData extracted from |entry_properties| and is passed so it
+  // doesn't have to be re-extracted. |old_guid| is the current GUID of the
+  // entry and may be empty.
+  // |callback| will be called when policy application for |entry| has finished.
+  void ApplyNewPolicy(const std::string& entry,
+                      const base::Value& entry_properties,
+                      std::unique_ptr<NetworkUIData> ui_data,
+                      const std::string& old_guid,
+                      const std::string& new_guid,
+                      const base::Value& new_policy,
+                      base::OnceClosure callback);
+
+  // Applies the global network policy (if any) on |entry|,
+  // |entry_properties|}  are the current properties for the entry.
+  // |callback| will be called when policy application for |entry| has finished
+  // or immediately if no global network policy is present.
+  void ApplyGlobalPolicyOnUnmanagedEntry(
+      const std::string& entry,
+      const base::DictionaryValue& entry_properties,
+      base::OnceClosure callback);
+
   // Sends Shill the command to delete profile entry |entry| from |profile_|.
-  void DeleteEntry(const std::string& entry);
+  // |callback| will be called when the profile entry has been deleted in shill.
+  void DeleteEntry(const std::string& entry, base::OnceClosure callback);
+
+  // Applies |shill_dictionary| in shill. |policy_ is the ONC policy blob which
+  // lead to the policy application. |callback| will be called when policy
+  // application has finished, i.e. when the policy has been applied in shill
+  // NetworkStateHandler in chrome has reflected the changes.
+  void WriteNewShillConfiguration(base::Value shill_dictionary,
+                                  base::Value policy,
+                                  base::OnceClosure callback);
 
-  // Sends the Shill configuration |shill_dictionary| to Shill. If |write_later|
-  // is true, the configuration is queued for sending until ~PolicyApplicator.
-  void WriteNewShillConfiguration(const base::DictionaryValue& shill_dictionary,
-                                  const base::DictionaryValue& policy,
-                                  bool write_later);
+  // Removes |entry| from the list of pending profile entries.
+  // If all entries were processed, applies the remaining policies and notifies
+  // |handler_|.
+  void ProfileEntryFinished(const std::string& entry);
 
   // Creates new entries for all remaining policies, i.e. for which no matching
   // Profile entry was found.
   // This should only be called if all profile entries were processed.
   void ApplyRemainingPolicies();
 
+  // This is called when the remaining policy application for |entry| scheduled
+  // by ApplyRemainingPolicies has finished.
+  void RemainingPolicyApplied(const std::string& entry);
+
   // Called after all policies are applied or an error occurred. Notifies
   // |handler_|.
   void NotifyConfigurationHandlerAndFinish();
 
-  std::set<std::string> remaining_policies_;
+  std::set<std::string> remaining_policy_guids_;
   std::set<std::string> pending_get_entry_calls_;
   ConfigurationHandler* handler_;
   NetworkProfile profile_;
   GuidToPolicyMap all_policies_;
   base::DictionaryValue global_network_config_;
-  std::vector<std::unique_ptr<base::DictionaryValue>> new_shill_configurations_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
   base::WeakPtrFactory<PolicyApplicator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyApplicator);