diff options
author | pneubeck <pneubeck@chromium.org> | 2014-10-29 04:59:10 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-10-29 11:59:36 +0000 |
commit | 9e212cc602be181755013bfec28f89fef6d2b611 (patch) | |
tree | 8e24fa5b76c80aaad637283f3177d4f632844fc2 /chromeos/network | |
parent | d9c9be31fbe444be76489631344847aed62fef43 (diff) | |
download | chromium_src-9e212cc602be181755013bfec28f89fef6d2b611.zip chromium_src-9e212cc602be181755013bfec28f89fef6d2b611.tar.gz chromium_src-9e212cc602be181755013bfec28f89fef6d2b611.tar.bz2 |
ManagedNetworkConfigurationHandler: Expose status about policy application.
Before, it was hard or impossible to track whether a policy application is currently running.
Now a notification (PoliciesApplied) and the current status (AnyPolicyApplicationRunning) are exposed.
BUG=425049
Review URL: https://codereview.chromium.org/661803005
Cr-Commit-Position: refs/heads/master@{#301817}
Diffstat (limited to 'chromeos/network')
-rw-r--r-- | chromeos/network/client_cert_resolver.cc | 5 | ||||
-rw-r--r-- | chromeos/network/client_cert_resolver.h | 2 | ||||
-rw-r--r-- | chromeos/network/managed_network_configuration_handler.h | 6 | ||||
-rw-r--r-- | chromeos/network/managed_network_configuration_handler_impl.cc | 89 | ||||
-rw-r--r-- | chromeos/network/managed_network_configuration_handler_impl.h | 67 | ||||
-rw-r--r-- | chromeos/network/managed_network_configuration_handler_unittest.cc | 126 | ||||
-rw-r--r-- | chromeos/network/mock_managed_network_configuration_handler.h | 3 | ||||
-rw-r--r-- | chromeos/network/network_policy_observer.h | 12 | ||||
-rw-r--r-- | chromeos/network/policy_applicator.cc | 96 | ||||
-rw-r--r-- | chromeos/network/policy_applicator.h | 71 |
10 files changed, 355 insertions, 122 deletions
diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc index 97a0cb0..cd84217 100644 --- a/chromeos/network/client_cert_resolver.cc +++ b/chromeos/network/client_cert_resolver.cc @@ -345,8 +345,9 @@ void ClientCertResolver::OnCertificatesLoaded( ResolveNetworks(networks); } -void ClientCertResolver::PolicyApplied(const std::string& service_path) { - VLOG(2) << "PolicyApplied " << service_path; +void ClientCertResolver::PolicyAppliedToNetwork( + const std::string& service_path) { + VLOG(2) << "PolicyAppliedToNetwork " << service_path; if (!ClientCertificatesLoaded()) return; // Compare this network with all certificates. diff --git a/chromeos/network/client_cert_resolver.h b/chromeos/network/client_cert_resolver.h index fc22d9b..9731693 100644 --- a/chromeos/network/client_cert_resolver.h +++ b/chromeos/network/client_cert_resolver.h @@ -66,7 +66,7 @@ class CHROMEOS_EXPORT ClientCertResolver : public NetworkStateHandlerObserver, bool initial_load) override; // NetworkPolicyObserver overrides - virtual void PolicyApplied(const std::string& service_path) override; + virtual void PolicyAppliedToNetwork(const std::string& service_path) override; // Check which networks of |networks| are configured with a client certificate // pattern. Search for certificates, on the worker thread, and configure the diff --git a/chromeos/network/managed_network_configuration_handler.h b/chromeos/network/managed_network_configuration_handler.h index 7ed8c17..4fb0eff 100644 --- a/chromeos/network/managed_network_configuration_handler.h +++ b/chromeos/network/managed_network_configuration_handler.h @@ -1,4 +1,4 @@ - // Copyright (c) 2013 The Chromium Authors. All rights reserved. +// Copyright (c) 2013 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. @@ -113,6 +113,10 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler { const base::ListValue& network_configs_onc, const base::DictionaryValue& global_network_config) = 0; + // Returns true if any policy application is currently running or pending. + // NetworkPolicyObservers are notified about applications finishing. + virtual bool IsAnyPolicyApplicationRunning() const = 0; + // Returns the user policy for user |userhash| or device policy, which has // |guid|. If |userhash| is empty, only looks for a device policy. If such // doesn't exist, returns NULL. Sets |onc_source| accordingly. diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc index 44f3da7..b47f2dd 100644 --- a/chromeos/network/managed_network_configuration_handler_impl.cc +++ b/chromeos/network/managed_network_configuration_handler_impl.cc @@ -11,8 +11,8 @@ #include "base/guid.h" #include "base/location.h" #include "base/logging.h" -#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" +#include "base/message_loop/message_loop.h" #include "base/stl_util.h" #include "base/values.h" #include "chromeos/dbus/shill_manager_client.h" @@ -403,24 +403,53 @@ void ManagedNetworkConfigurationHandlerImpl::SetPolicy( } STLDeleteValues(&old_per_network_config); + ApplyOrQueuePolicies(userhash, &modified_policies); + FOR_EACH_OBSERVER(NetworkPolicyObserver, observers_, PolicyChanged(userhash)); +} + +bool ManagedNetworkConfigurationHandlerImpl::IsAnyPolicyApplicationRunning() + const { + return !policy_applicators_.empty() || !queued_modified_policies_.empty(); +} + +bool ManagedNetworkConfigurationHandlerImpl::ApplyOrQueuePolicies( + const std::string& userhash, + std::set<std::string>* modified_policies) { + DCHECK(modified_policies); const NetworkProfile* profile = network_profile_handler_->GetProfileForUserhash(userhash); - if (profile) { - scoped_refptr<PolicyApplicator> applicator = - new PolicyApplicator(weak_ptr_factory_.GetWeakPtr(), - *profile, - policies->per_network_config, - policies->global_network_config, - &modified_policies); - applicator->Run(); - } else { + if (!profile) { VLOG(1) << "The relevant Shill profile isn't initialized yet, postponing " << "policy application."; - // See OnProfileAdded. + // OnProfileAdded will apply all policies for this userhash. + return false; } - FOR_EACH_OBSERVER(NetworkPolicyObserver, observers_, PolicyChanged(userhash)); + if (ContainsKey(policy_applicators_, userhash)) { + // A previous policy application is still running. Queue the modified + // policies. + // Note, even if |modified_policies| is empty, this means that a policy + // application will be queued. + queued_modified_policies_[userhash].insert(modified_policies->begin(), + modified_policies->end()); + VLOG(1) << "Previous PolicyApplicator still running. Postponing policy " + "application."; + return false; + } + + const Policies* policies = policies_by_user_[userhash].get(); + DCHECK(policies); + + PolicyApplicator* applicator = + new PolicyApplicator(*profile, + policies->per_network_config, + policies->global_network_config, + this, + modified_policies); + policy_applicators_[userhash] = make_linked_ptr(applicator); + applicator->Run(); + return true; } void ManagedNetworkConfigurationHandlerImpl::OnProfileAdded( @@ -442,13 +471,9 @@ void ManagedNetworkConfigurationHandlerImpl::OnProfileAdded( policy_guids.insert(it->first); } - scoped_refptr<PolicyApplicator> applicator = - new PolicyApplicator(weak_ptr_factory_.GetWeakPtr(), - profile, - policies->per_network_config, - policies->global_network_config, - &policy_guids); - applicator->Run(); + const bool started_policy_application = + ApplyOrQueuePolicies(profile.userhash, &policy_guids); + DCHECK(started_policy_application); } void ManagedNetworkConfigurationHandlerImpl::OnProfileRemoved( @@ -503,7 +528,25 @@ void ManagedNetworkConfigurationHandlerImpl:: base::Bind(&LogErrorWithDict, FROM_HERE)); } -void ManagedNetworkConfigurationHandlerImpl::OnPoliciesApplied() { +void ManagedNetworkConfigurationHandlerImpl::OnPoliciesApplied( + const NetworkProfile& profile) { + const std::string& userhash = profile.userhash; + VLOG(1) << "Policy application for user '" << userhash << "' finished."; + + base::MessageLoop::current()->DeleteSoon( + FROM_HERE, policy_applicators_[userhash].release()); + policy_applicators_.erase(userhash); + + if (ContainsKey(queued_modified_policies_, userhash)) { + std::set<std::string> modified_policies; + queued_modified_policies_[userhash].swap(modified_policies); + // Remove |userhash| from the queue. + queued_modified_policies_.erase(userhash); + ApplyOrQueuePolicies(userhash, &modified_policies); + } else { + FOR_EACH_OBSERVER( + NetworkPolicyObserver, observers_, PoliciesApplied(userhash)); + } } const base::DictionaryValue* @@ -587,7 +630,9 @@ ManagedNetworkConfigurationHandlerImpl::ManagedNetworkConfigurationHandlerImpl() network_profile_handler_(NULL), network_configuration_handler_(NULL), network_device_handler_(NULL), - weak_ptr_factory_(this) {} + weak_ptr_factory_(this) { + CHECK(base::MessageLoop::current()); +} ManagedNetworkConfigurationHandlerImpl:: ~ManagedNetworkConfigurationHandlerImpl() { @@ -611,7 +656,7 @@ void ManagedNetworkConfigurationHandlerImpl::OnPolicyAppliedToNetwork( if (service_path.empty()) return; FOR_EACH_OBSERVER( - NetworkPolicyObserver, observers_, PolicyApplied(service_path)); + NetworkPolicyObserver, observers_, PolicyAppliedToNetwork(service_path)); } // Get{Managed}Properties helpers diff --git a/chromeos/network/managed_network_configuration_handler_impl.h b/chromeos/network/managed_network_configuration_handler_impl.h index aaa8635..8c56fb8 100644 --- a/chromeos/network/managed_network_configuration_handler_impl.h +++ b/chromeos/network/managed_network_configuration_handler_impl.h @@ -6,6 +6,7 @@ #define CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_IMPL_H_ #include <map> +#include <set> #include <string> #include "base/basictypes.h" @@ -34,71 +35,72 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl public NetworkProfileObserver, public PolicyApplicator::ConfigurationHandler { public: - virtual ~ManagedNetworkConfigurationHandlerImpl(); + ~ManagedNetworkConfigurationHandlerImpl() override; // ManagedNetworkConfigurationHandler overrides - virtual void AddObserver(NetworkPolicyObserver* observer) override; - virtual void RemoveObserver(NetworkPolicyObserver* observer) override; + void AddObserver(NetworkPolicyObserver* observer) override; + void RemoveObserver(NetworkPolicyObserver* observer) override; - virtual void GetProperties( + void GetProperties( const std::string& service_path, const network_handler::DictionaryResultCallback& callback, const network_handler::ErrorCallback& error_callback) override; - virtual void GetManagedProperties( + void GetManagedProperties( const std::string& userhash, const std::string& service_path, const network_handler::DictionaryResultCallback& callback, const network_handler::ErrorCallback& error_callback) override; - virtual void SetProperties( + void SetProperties( const std::string& service_path, const base::DictionaryValue& user_settings, const base::Closure& callback, const network_handler::ErrorCallback& error_callback) const override; - virtual void CreateConfiguration( + void CreateConfiguration( const std::string& userhash, const base::DictionaryValue& properties, const network_handler::StringResultCallback& callback, const network_handler::ErrorCallback& error_callback) const override; - virtual void RemoveConfiguration( + void RemoveConfiguration( const std::string& service_path, const base::Closure& callback, const network_handler::ErrorCallback& error_callback) const override; - virtual void SetPolicy( - onc::ONCSource onc_source, - const std::string& userhash, - const base::ListValue& network_configs_onc, - const base::DictionaryValue& global_network_config) override; + void SetPolicy(onc::ONCSource onc_source, + const std::string& userhash, + const base::ListValue& network_configs_onc, + const base::DictionaryValue& global_network_config) override; + + bool IsAnyPolicyApplicationRunning() const override; - virtual const base::DictionaryValue* FindPolicyByGUID( + const base::DictionaryValue* FindPolicyByGUID( const std::string userhash, const std::string& guid, onc::ONCSource* onc_source) const override; - virtual const base::DictionaryValue* GetGlobalConfigFromPolicy( + const base::DictionaryValue* GetGlobalConfigFromPolicy( const std::string userhash) const override; - virtual const base::DictionaryValue* FindPolicyByGuidAndProfile( + const base::DictionaryValue* FindPolicyByGuidAndProfile( const std::string& guid, const std::string& profile_path) const override; // NetworkProfileObserver overrides - virtual void OnProfileAdded(const NetworkProfile& profile) override; - virtual void OnProfileRemoved(const NetworkProfile& profile) override; + void OnProfileAdded(const NetworkProfile& profile) override; + void OnProfileRemoved(const NetworkProfile& profile) override; // PolicyApplicator::ConfigurationHandler overrides - virtual void CreateConfigurationFromPolicy( + void CreateConfigurationFromPolicy( const base::DictionaryValue& shill_properties) override; - virtual void UpdateExistingConfigurationWithPropertiesFromPolicy( + void UpdateExistingConfigurationWithPropertiesFromPolicy( const base::DictionaryValue& existing_properties, const base::DictionaryValue& new_properties) override; - virtual void OnPoliciesApplied() override; + void OnPoliciesApplied(const NetworkProfile& profile) override; private: friend class ClientCertResolverTest; @@ -107,10 +109,14 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl friend class NetworkHandler; struct Policies; - typedef std::map<std::string, linked_ptr<Policies> > UserToPoliciesMap; typedef base::Callback<void(const std::string& service_path, scoped_ptr<base::DictionaryValue> properties)> GetDevicePropertiesCallback; + typedef std::map<std::string, linked_ptr<Policies> > UserToPoliciesMap; + typedef std::map<std::string, linked_ptr<PolicyApplicator>> + UserToPolicyApplicatorMap; + typedef std::map<std::string, std::set<std::string>> + UserToModifiedPoliciesMap; ManagedNetworkConfigurationHandlerImpl(); @@ -169,6 +175,13 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl const std::string& error_name, scoped_ptr<base::DictionaryValue> error_data); + // Applies policies for |userhash|. |modified_policies| must be not null and + // contain the GUIDs of the network configurations that changed since the last + // policy application. Returns true if policy application was started and + // false if it was queued or delayed. + bool ApplyOrQueuePolicies(const std::string& userhash, + std::set<std::string>* modified_policies); + // If present, the empty string maps to the device policy. UserToPoliciesMap policies_by_user_; @@ -178,6 +191,16 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl NetworkConfigurationHandler* network_configuration_handler_; NetworkDeviceHandler* network_device_handler_; + // Owns the currently running PolicyApplicators. + UserToPolicyApplicatorMap policy_applicators_; + + // Per userhash (or empty string for device policy), contains the GUIDs of the + // policies that were modified. + // If this map contains a userhash as key, it means that a policy application + // for this userhash is pending even if no policies were modified and the + // associated set of GUIDs is empty. + UserToModifiedPoliciesMap queued_modified_policies_; + ObserverList<NetworkPolicyObserver> observers_; // For Shill client callbacks diff --git a/chromeos/network/managed_network_configuration_handler_unittest.cc b/chromeos/network/managed_network_configuration_handler_unittest.cc index b72fb86..94f9098 100644 --- a/chromeos/network/managed_network_configuration_handler_unittest.cc +++ b/chromeos/network/managed_network_configuration_handler_unittest.cc @@ -6,6 +6,7 @@ #include <sstream> #include "base/bind.h" +#include "base/bind_helpers.h" #include "base/location.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop/message_loop.h" @@ -18,6 +19,7 @@ #include "chromeos/dbus/shill_client_helper.h" #include "chromeos/network/managed_network_configuration_handler_impl.h" #include "chromeos/network/network_configuration_handler.h" +#include "chromeos/network/network_policy_observer.h" #include "chromeos/network/network_profile_handler.h" #include "chromeos/network/onc/onc_test_utils.h" #include "chromeos/network/onc/onc_utils.h" @@ -48,6 +50,12 @@ std::string ValueToString(const base::Value* value) { return str.str(); } +void DereferenceAndCall( + base::Callback<void(const base::DictionaryValue& result)> callback, + const base::DictionaryValue* value) { + callback.Run(*value); +} + const char kUser1[] = "user1"; const char kUser1ProfilePath[] = "/profile/user1/shill"; @@ -108,7 +116,10 @@ class ShillProfileTestClient { const std::string& userhash = profile_to_user_[profile_path.value()]; result->SetStringWithoutPathExpansion(shill::kUserHashProperty, userhash); - callback.Run(*result); + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(base::Bind(&DereferenceAndCall, callback), + base::Owned(result.release()))); } void GetEntry(const dbus::ObjectPath& profile_path, @@ -123,7 +134,10 @@ class ShillProfileTestClient { base::DictionaryValue* entry = NULL; entries->GetDictionaryWithoutPathExpansion(entry_path, &entry); ASSERT_TRUE(entry); - callback.Run(*entry); + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(base::Bind(&DereferenceAndCall, callback), + base::Owned(entry->DeepCopy()))); } protected: @@ -167,6 +181,26 @@ class TestNetworkProfileHandler : public NetworkProfileHandler { DISALLOW_COPY_AND_ASSIGN(TestNetworkProfileHandler); }; +class TestNetworkPolicyObserver : public NetworkPolicyObserver { + public: + TestNetworkPolicyObserver() : policies_applied_count_(0) {} + + void PoliciesApplied(const std::string& userhash) override { + policies_applied_count_++; + }; + + int GetPoliciesAppliedCountAndReset() { + int count = policies_applied_count_; + policies_applied_count_ = 0; + return count; + } + + private: + int policies_applied_count_; + + DISALLOW_COPY_AND_ASSIGN(TestNetworkPolicyObserver); +}; + } // namespace class ManagedNetworkConfigurationHandlerTest : public testing::Test { @@ -180,7 +214,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test { virtual ~ManagedNetworkConfigurationHandlerTest() { } - virtual void SetUp() override { + void SetUp() override { scoped_ptr<DBusThreadManagerSetter> dbus_setter = DBusThreadManager::GetSetterForTesting(); mock_manager_client_ = new StrictMock<MockShillManagerClient>(); @@ -218,11 +252,14 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test { network_profile_handler_.get(), network_configuration_handler_.get(), NULL /* no DeviceHandler */); + managed_network_configuration_handler_->AddObserver(&policy_observer_); message_loop_.RunUntilIdle(); } - virtual void TearDown() override { + void TearDown() override { + if (managed_network_configuration_handler_) + managed_network_configuration_handler_->RemoveObserver(&policy_observer_); managed_network_configuration_handler_.reset(); network_configuration_handler_.reset(); network_profile_handler_.reset(); @@ -321,6 +358,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test { MockShillServiceClient* mock_service_client_; ShillProfileTestClient profiles_stub_; ShillServiceTestClient services_stub_; + TestNetworkPolicyObserver policy_observer_; scoped_ptr<TestNetworkProfileHandler> network_profile_handler_; scoped_ptr<NetworkConfigurationHandler> network_configuration_handler_; scoped_ptr<ManagedNetworkConfigurationHandlerImpl> @@ -428,6 +466,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmodified) { SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); VerifyAndClearExpectations(); SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json", @@ -441,7 +480,82 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmodified) { GetEntry(dbus::ObjectPath(kUser1ProfilePath), "some_entry_path", _, _)); SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); + + message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); +} + +TEST_F(ManagedNetworkConfigurationHandlerTest, PolicyApplicationRunning) { + InitializeStandardProfiles(); + EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)).Times(AnyNumber()); + EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _)) + .Times(AnyNumber()); + EXPECT_CALL(*mock_profile_client_, GetEntry(_, _, _, _)).Times(AnyNumber()); + + EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning()); + + SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); + managed_handler()->SetPolicy( + ::onc::ONC_SOURCE_DEVICE_POLICY, + std::string(), // no userhash + base::ListValue(), // no device network policy + base::DictionaryValue()); // no device global config + + EXPECT_TRUE(managed_handler()->IsAnyPolicyApplicationRunning()); + message_loop_.RunUntilIdle(); + EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning()); + + SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json", + kUser1ProfilePath, + "some_entry_path"); + + SetPolicy( + ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc"); + EXPECT_TRUE(managed_handler()->IsAnyPolicyApplicationRunning()); + message_loop_.RunUntilIdle(); + EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning()); +} + +TEST_F(ManagedNetworkConfigurationHandlerTest, UpdatePolicyAfterFinished) { + InitializeStandardProfiles(); + EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)); + EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _)); + + SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); + message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); + VerifyAndClearExpectations(); + + SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json", + kUser1ProfilePath, + "some_entry_path"); + + EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)); + EXPECT_CALL( + *mock_profile_client_, + GetEntry(dbus::ObjectPath(kUser1ProfilePath), "some_entry_path", _, _)); + EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _)); + + SetPolicy( + ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc"); + message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); +} + +TEST_F(ManagedNetworkConfigurationHandlerTest, UpdatePolicyBeforeFinished) { + InitializeStandardProfiles(); + EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)).Times(2); + EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _)) + .Times(2); + + SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); + // Usually the first call will cause a profile entry to be created, which we + // don't fake here. + SetPolicy( + ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc"); + message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); } TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyManageUnmanaged) { @@ -690,6 +804,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetEmptyPolicyIgnoreUnmanaged) { SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, ""); message_loop_.RunUntilIdle(); + EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset()); } TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmanaged) { @@ -809,7 +924,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, LateProfileLoading) { class ManagedNetworkConfigurationHandlerShutdownTest : public ManagedNetworkConfigurationHandlerTest { public: - virtual void SetUp() override { + void SetUp() override { ManagedNetworkConfigurationHandlerTest::SetUp(); ON_CALL(*mock_profile_client_, GetProperties(_, _, _)).WillByDefault( Invoke(&ManagedNetworkConfigurationHandlerShutdownTest::GetProperties)); @@ -840,6 +955,7 @@ TEST_F(ManagedNetworkConfigurationHandlerShutdownTest, GetProperties(dbus::ObjectPath(kUser1ProfilePath), _, _)); SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc"); + managed_network_configuration_handler_->RemoveObserver(&policy_observer_); managed_network_configuration_handler_.reset(); message_loop_.RunUntilIdle(); } diff --git a/chromeos/network/mock_managed_network_configuration_handler.h b/chromeos/network/mock_managed_network_configuration_handler.h index bc8954a..11d5342 100644 --- a/chromeos/network/mock_managed_network_configuration_handler.h +++ b/chromeos/network/mock_managed_network_configuration_handler.h @@ -5,6 +5,8 @@ #ifndef CHROMEOS_NETWORK_MOCK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_ #define CHROMEOS_NETWORK_MOCK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_ +#include <string> + #include "base/basictypes.h" #include "base/values.h" #include "chromeos/chromeos_export.h" @@ -53,6 +55,7 @@ class CHROMEOS_EXPORT MockManagedNetworkConfigurationHandler const std::string& userhash, const base::ListValue& network_configs_onc, const base::DictionaryValue& global_network_config)); + MOCK_CONST_METHOD0(IsAnyPolicyApplicationRunning, bool()); MOCK_CONST_METHOD3(FindPolicyByGUID, const base::DictionaryValue*( const std::string userhash, diff --git a/chromeos/network/network_policy_observer.h b/chromeos/network/network_policy_observer.h index defbb26..c7af61e 100644 --- a/chromeos/network/network_policy_observer.h +++ b/chromeos/network/network_policy_observer.h @@ -16,13 +16,17 @@ class NetworkPolicyObserver { // Called when the policy for |userhash| was set (also when it was updated). // Note that the policy might not have been applied yet at that time. // An empty |userhash| designates the device policy. - virtual void PolicyChanged(const std::string& userhash) {}; + virtual void PolicyChanged(const std::string& userhash) {} - // Called every time a network is create or updated because of a policy. - virtual void PolicyApplied(const std::string& service_path) {}; + // Called every time a policy application for |userhash| finished. This is + // only called once no more policies are pending for |userhash|. + virtual void PoliciesApplied(const std::string& userhash) {} + + // Called every time a network is created or updated because of a policy. + virtual void PolicyAppliedToNetwork(const std::string& service_path) {} protected: - virtual ~NetworkPolicyObserver() {}; + virtual ~NetworkPolicyObserver() {} private: DISALLOW_ASSIGN(NetworkPolicyObserver); diff --git a/chromeos/network/policy_applicator.cc b/chromeos/network/policy_applicator.cc index 2c1e720..334c106 100644 --- a/chromeos/network/policy_applicator.cc +++ b/chromeos/network/policy_applicator.cc @@ -46,12 +46,12 @@ const base::DictionaryValue* GetByGUID( } // namespace PolicyApplicator::PolicyApplicator( - base::WeakPtr<ConfigurationHandler> handler, const NetworkProfile& profile, const GuidToPolicyMap& all_policies, const base::DictionaryValue& global_network_config, + ConfigurationHandler* handler, std::set<std::string>* modified_policies) - : handler_(handler), profile_(profile) { + : handler_(handler), profile_(profile), weak_ptr_factory_(this) { global_network_config_.MergeDictionary(&global_network_config); remaining_policies_.swap(*modified_policies); for (GuidToPolicyMap::const_iterator it = all_policies.begin(); @@ -60,21 +60,30 @@ PolicyApplicator::PolicyApplicator( } } +PolicyApplicator::~PolicyApplicator() { + STLDeleteValues(&all_policies_); + VLOG(1) << "Destroying PolicyApplicator for " << profile_.userhash; +} + void PolicyApplicator::Run() { DBusThreadManager::Get()->GetShillProfileClient()->GetProperties( dbus::ObjectPath(profile_.path), - base::Bind(&PolicyApplicator::GetProfilePropertiesCallback, this), - base::Bind(&LogErrorMessage, FROM_HERE)); + base::Bind(&PolicyApplicator::GetProfilePropertiesCallback, + weak_ptr_factory_.GetWeakPtr()), + base::Bind(&PolicyApplicator::GetProfilePropertiesError, + weak_ptr_factory_.GetWeakPtr())); } -void PolicyApplicator::GetProfilePropertiesCallback( - const base::DictionaryValue& profile_properties) { - if (!handler_) { - LOG(WARNING) << "Handler destructed during policy application to profile " - << profile_.ToDebugString(); - return; +void PolicyApplicator::ProfileEntryFinished(const std::string& entry) { + pending_get_entry_calls_.erase(entry); + if (pending_get_entry_calls_.empty()) { + ApplyRemainingPolicies(); + NotifyConfigurationHandlerAndFinish(); } +} +void PolicyApplicator::GetProfilePropertiesCallback( + const base::DictionaryValue& profile_properties) { VLOG(2) << "Received properties for profile " << profile_.ToDebugString(); const base::ListValue* entries = NULL; if (!profile_properties.GetListWithoutPathExpansion( @@ -82,6 +91,7 @@ void PolicyApplicator::GetProfilePropertiesCallback( LOG(ERROR) << "Profile " << profile_.ToDebugString() << " doesn't contain the property " << shill::kEntriesProperty; + NotifyConfigurationHandlerAndFinish(); return; } @@ -90,23 +100,34 @@ void PolicyApplicator::GetProfilePropertiesCallback( std::string entry; (*it)->GetAsString(&entry); + pending_get_entry_calls_.insert(entry); DBusThreadManager::Get()->GetShillProfileClient()->GetEntry( dbus::ObjectPath(profile_.path), entry, - base::Bind(&PolicyApplicator::GetEntryCallback, this, entry), - base::Bind(&LogErrorMessage, FROM_HERE)); + base::Bind(&PolicyApplicator::GetEntryCallback, + weak_ptr_factory_.GetWeakPtr(), + entry), + base::Bind(&PolicyApplicator::GetEntryError, + weak_ptr_factory_.GetWeakPtr(), + entry)); + } + if (pending_get_entry_calls_.empty()) { + ApplyRemainingPolicies(); + NotifyConfigurationHandlerAndFinish(); } } +void PolicyApplicator::GetProfilePropertiesError( + const std::string& error_name, + const std::string& error_message) { + LOG(ERROR) << "Could not retrieve properties of profile " << profile_.path + << ": " << error_message; + NotifyConfigurationHandlerAndFinish(); +} + void PolicyApplicator::GetEntryCallback( const std::string& entry, const base::DictionaryValue& entry_properties) { - if (!handler_) { - LOG(WARNING) << "Handler destructed during policy application to profile " - << profile_.ToDebugString(); - return; - } - VLOG(2) << "Received properties for entry " << entry << " of profile " << profile_.ToDebugString(); @@ -235,6 +256,16 @@ void PolicyApplicator::GetEntryCallback( entry_properties, shill_properties_to_update); } } + + ProfileEntryFinished(entry); +} + +void PolicyApplicator::GetEntryError(const std::string& entry, + const std::string& error_name, + const std::string& error_message) { + LOG(ERROR) << "Could not retrieve entry " << entry << " of profile " + << profile_.path << ": " << error_message; + ProfileEntryFinished(entry); } void PolicyApplicator::DeleteEntry(const std::string& entry) { @@ -271,21 +302,8 @@ void PolicyApplicator::WriteNewShillConfiguration( handler_->CreateConfigurationFromPolicy(shill_dictionary); } -PolicyApplicator::~PolicyApplicator() { - ApplyRemainingPolicies(); - STLDeleteValues(&all_policies_); - // Notify the handler about all policies being applied, so that the network - // lists can be updated. - if (handler_) - handler_->OnPoliciesApplied(); -} - void PolicyApplicator::ApplyRemainingPolicies() { - if (!handler_) { - LOG(WARNING) << "Handler destructed during policy application to profile " - << profile_.ToDebugString(); - return; - } + DCHECK(pending_get_entry_calls_.empty()); // Write all queued configurations now. for (ScopedVector<base::DictionaryValue>::const_iterator it = @@ -294,12 +312,12 @@ void PolicyApplicator::ApplyRemainingPolicies() { ++it) { handler_->CreateConfigurationFromPolicy(**it); } + new_shill_configurations_.clear(); - if (remaining_policies_.empty()) - return; + VLOG_IF(2, !remaining_policies_.empty()) + << "Create new managed network configurations in profile" + << profile_.ToDebugString() << "."; - VLOG(2) << "Create new managed network configurations in profile" - << profile_.ToDebugString() << "."; // All profile entries were compared to policies. |remaining_policies_| // contains all modified policies that didn't match any entry. For these // remaining policies, new configurations have to be created. @@ -320,6 +338,12 @@ void PolicyApplicator::ApplyRemainingPolicies() { WriteNewShillConfiguration( *shill_dictionary, *network_policy, false /* write now */); } + remaining_policies_.clear(); +} + +void PolicyApplicator::NotifyConfigurationHandlerAndFinish() { + weak_ptr_factory_.InvalidateWeakPtrs(); + handler_->OnPoliciesApplied(profile_); } } // namespace chromeos diff --git a/chromeos/network/policy_applicator.h b/chromeos/network/policy_applicator.h index b9401a4..bca3b06 100644 --- a/chromeos/network/policy_applicator.h +++ b/chromeos/network/policy_applicator.h @@ -9,7 +9,6 @@ #include <set> #include <string> -#include "base/memory/ref_counted.h" #include "base/memory/scoped_vector.h" #include "base/memory/weak_ptr.h" #include "base/values.h" @@ -22,54 +21,65 @@ namespace chromeos { // entries in parallel (GetProfilePropertiesCallback), compares each entry with // the current policies (GetEntryCallback) and adds all missing policies // (~PolicyApplicator). -class PolicyApplicator : public base::RefCounted<PolicyApplicator> { +class PolicyApplicator { public: class ConfigurationHandler { - public: - virtual ~ConfigurationHandler() {} - // 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. - virtual void CreateConfigurationFromPolicy( - const base::DictionaryValue& shill_properties) = 0; - - virtual void UpdateExistingConfigurationWithPropertiesFromPolicy( - const base::DictionaryValue& existing_properties, - const base::DictionaryValue& new_properties) = 0; - - // Called after all policies were applied. At this point, the list of - // networks should be updated. - virtual void OnPoliciesApplied() = 0; - - private: - DISALLOW_ASSIGN(ConfigurationHandler); + public: + virtual ~ConfigurationHandler() {} + // 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. + virtual void CreateConfigurationFromPolicy( + const base::DictionaryValue& shill_properties) = 0; + + virtual void UpdateExistingConfigurationWithPropertiesFromPolicy( + const base::DictionaryValue& existing_properties, + const base::DictionaryValue& new_properties) = 0; + + // Called after all policies for |profile| were applied. At this point, the + // list of networks should be updated. + virtual void OnPoliciesApplied(const NetworkProfile& profile) = 0; + + private: + DISALLOW_ASSIGN(ConfigurationHandler); }; typedef std::map<std::string, const base::DictionaryValue*> GuidToPolicyMap; + // |handler| must outlive this object. // |modified_policies| must not be NULL and will be empty afterwards. - PolicyApplicator(base::WeakPtr<ConfigurationHandler> handler, - const NetworkProfile& profile, + PolicyApplicator(const NetworkProfile& profile, const GuidToPolicyMap& all_policies, const base::DictionaryValue& global_network_config, + ConfigurationHandler* handler, std::set<std::string>* modified_policies); + ~PolicyApplicator(); + void Run(); private: - friend class base::RefCounted<PolicyApplicator>; + // 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( const base::DictionaryValue& profile_properties); + void GetProfilePropertiesError(const std::string& error_name, + const std::string& error_message); // Called with the properties of the profile entry |entry|. Checks whether the // entry was previously managed, whether a current policy applies and then // either updates, deletes or not touches the entry. void GetEntryCallback(const std::string& entry, const base::DictionaryValue& entry_properties); + void GetEntryError(const std::string& entry, + const std::string& error_name, + const std::string& error_message); // Sends Shill the command to delete profile entry |entry| from |profile_|. void DeleteEntry(const std::string& entry); @@ -80,20 +90,23 @@ class PolicyApplicator : public base::RefCounted<PolicyApplicator> { const base::DictionaryValue& policy, bool write_later); - // Called once all Profile entries are processed. Calls - // ApplyRemainingPolicies. - virtual ~PolicyApplicator(); - // 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(); + // Called after all policies are applied or an error occurred. Notifies + // |handler_|. + void NotifyConfigurationHandlerAndFinish(); + std::set<std::string> remaining_policies_; - base::WeakPtr<ConfigurationHandler> handler_; + std::set<std::string> pending_get_entry_calls_; + ConfigurationHandler* handler_; NetworkProfile profile_; GuidToPolicyMap all_policies_; base::DictionaryValue global_network_config_; ScopedVector<base::DictionaryValue> new_shill_configurations_; + base::WeakPtrFactory<PolicyApplicator> weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(PolicyApplicator); }; |