update_engine: Refresh device policy when it is updated.
The session manager emits a signal whenever the device policy is
updated. This patch makes update_engine listen for that signal and
reload the device policy.
BUG=chromium:423037
TEST=deployed update_engine and checked the policy is refreshed.
Change-Id: I71dd7047e53d49f3402e1f9f4a67ec8cbd3739d3
Reviewed-on: https://chromium-review.googlesource.com/235884
Trybot-Ready: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/update_manager/real_device_policy_provider.cc b/update_manager/real_device_policy_provider.cc
index f74a7be..e3a378a 100644
--- a/update_manager/real_device_policy_provider.cc
+++ b/update_manager/real_device_policy_provider.cc
@@ -8,8 +8,10 @@
#include <base/logging.h>
#include <base/time/time.h>
+#include <chromeos/dbus/service_constants.h>
#include <policy/device_policy.h>
+#include "update_engine/glib_utils.h"
#include "update_engine/update_manager/generic_variables.h"
#include "update_engine/update_manager/real_shill_provider.h"
#include "update_engine/utils.h"
@@ -29,6 +31,12 @@
RealDevicePolicyProvider::~RealDevicePolicyProvider() {
CancelMainLoopEvent(scheduled_refresh_);
+ // Detach signal handler, free manager proxy.
+ dbus_->ProxyDisconnectSignal(manager_proxy_,
+ login_manager::kPropertyChangeCompleteSignal,
+ G_CALLBACK(HandlePropertyChangedCompletedStatic),
+ this);
+ dbus_->ProxyUnref(manager_proxy_);
}
bool RealDevicePolicyProvider::Init() {
@@ -37,9 +45,42 @@
// On Init() we try to get the device policy and keep updating it.
RefreshDevicePolicyAndReschedule();
+ // We also listen for signals from the session manager to force a device
+ // policy refresh.
+ GError* error = nullptr;
+ DBusGConnection* connection = dbus_->BusGet(DBUS_BUS_SYSTEM, &error);
+ if (!connection) {
+ LOG(ERROR) << "Failed to initialize DBus connection: "
+ << chromeos_update_engine::utils::GetAndFreeGError(&error);
+ return false;
+ }
+ manager_proxy_ = dbus_->ProxyNewForName(
+ connection,
+ login_manager::kSessionManagerServiceName,
+ login_manager::kSessionManagerServicePath,
+ login_manager::kSessionManagerInterface);
+
+ // Subscribe to the session manager's PropertyChangeComplete signal.
+ dbus_->ProxyAddSignal_1(manager_proxy_,
+ login_manager::kPropertyChangeCompleteSignal,
+ G_TYPE_STRING);
+ dbus_->ProxyConnectSignal(manager_proxy_,
+ login_manager::kPropertyChangeCompleteSignal,
+ G_CALLBACK(HandlePropertyChangedCompletedStatic),
+ this, nullptr);
return true;
}
+// static
+void RealDevicePolicyProvider::HandlePropertyChangedCompletedStatic(
+ DBusGProxy* proxy, const char* /* payload */, void* data) {
+ // We refresh the policy file even if the payload string is kSignalFailure.
+ RealDevicePolicyProvider* policy_provider =
+ reinterpret_cast<RealDevicePolicyProvider*>(data);
+ LOG(INFO) << "Reloading device policy due to signal received.";
+ policy_provider->RefreshDevicePolicy();
+}
+
void RealDevicePolicyProvider::RefreshDevicePolicyAndReschedule() {
RefreshDevicePolicy();
scheduled_refresh_ = RunFromMainLoopAfterTimeout(
diff --git a/update_manager/real_device_policy_provider.h b/update_manager/real_device_policy_provider.h
index c3e2980..f60d9eb 100644
--- a/update_manager/real_device_policy_provider.h
+++ b/update_manager/real_device_policy_provider.h
@@ -11,6 +11,7 @@
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include <policy/libpolicy.h>
+#include "update_engine/dbus_wrapper_interface.h"
#include "update_engine/update_manager/device_policy_provider.h"
#include "update_engine/update_manager/event_loop.h"
#include "update_engine/update_manager/generic_variables.h"
@@ -20,8 +21,11 @@
// DevicePolicyProvider concrete implementation.
class RealDevicePolicyProvider : public DevicePolicyProvider {
public:
- explicit RealDevicePolicyProvider(policy::PolicyProvider* policy_provider)
- : policy_provider_(policy_provider) {}
+ RealDevicePolicyProvider(
+ chromeos_update_engine::DBusWrapperInterface* const dbus,
+ policy::PolicyProvider* policy_provider)
+ : policy_provider_(policy_provider),
+ dbus_(dbus) {}
~RealDevicePolicyProvider();
// Initializes the provider and returns whether it succeeded.
@@ -73,6 +77,12 @@
FRIEND_TEST(UmRealDevicePolicyProviderTest, NonExistentDevicePolicyReloaded);
FRIEND_TEST(UmRealDevicePolicyProviderTest, ValuesUpdated);
+ // A static handler for the PropertyChangedCompleted signal from the session
+ // manager used as a callback.
+ static void HandlePropertyChangedCompletedStatic(DBusGProxy* proxy,
+ const char* payload,
+ void* data);
+
// Schedules a call to periodically refresh the device policy.
void RefreshDevicePolicyAndReschedule();
@@ -108,6 +118,10 @@
// Used to schedule refreshes of the device policy.
EventId scheduled_refresh_ = kEventIdNull;
+ // The DBus interface (mockable) and a session manager proxy.
+ chromeos_update_engine::DBusWrapperInterface* const dbus_;
+ DBusGProxy* manager_proxy_ = nullptr;
+
// Variable exposing whether the policy is loaded.
AsyncCopyVariable<bool> var_device_policy_is_loaded_{
"policy_is_loaded", false};
diff --git a/update_manager/real_device_policy_provider_unittest.cc b/update_manager/real_device_policy_provider_unittest.cc
index a0a3333..4c385f8 100644
--- a/update_manager/real_device_policy_provider_unittest.cc
+++ b/update_manager/real_device_policy_provider_unittest.cc
@@ -6,10 +6,12 @@
#include <memory>
+#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
#include <policy/mock_device_policy.h>
#include <policy/mock_libpolicy.h>
+#include "update_engine/mock_dbus_wrapper.h"
#include "update_engine/test_utils.h"
#include "update_engine/update_manager/umtest_utils.h"
@@ -22,23 +24,49 @@
using testing::Mock;
using testing::Return;
using testing::ReturnRef;
+using testing::SaveArg;
using testing::SetArgumentPointee;
+using testing::StrEq;
using testing::_;
+namespace {
+
+// Fake dbus-glib objects. These should be different values, to ease diagnosis
+// of errors.
+DBusGConnection* const kFakeConnection = reinterpret_cast<DBusGConnection*>(1);
+DBusGProxy* const kFakeManagerProxy = reinterpret_cast<DBusGProxy*>(2);
+
+} // namespace
+
namespace chromeos_update_manager {
class UmRealDevicePolicyProviderTest : public ::testing::Test {
protected:
void SetUp() override {
- provider_.reset(new RealDevicePolicyProvider(&mock_policy_provider_));
+ provider_.reset(new RealDevicePolicyProvider(&mock_dbus_,
+ &mock_policy_provider_));
// By default, we have a device policy loaded. Tests can call
// SetUpNonExistentDevicePolicy() to override this.
SetUpExistentDevicePolicy();
+
+ SetUpDBusSignalExpectations();
}
void TearDown() override {
// Check for leaked callbacks on the main loop.
EXPECT_EQ(0, RunGMainLoopMaxIterations(100));
+
+ // We need to set these expectation before the object is destroyed but
+ // after it finished running the test so the values of signal_callback_ and
+ // signal_callback_data_ are correct.
+ EXPECT_CALL(mock_dbus_, ProxyDisconnectSignal(
+ kFakeManagerProxy,
+ StrEq(login_manager::kPropertyChangeCompleteSignal),
+ signal_callback_,
+ signal_callback_data_));
+ EXPECT_CALL(mock_dbus_, ProxyUnref(kFakeManagerProxy));
+
+ provider_.reset();
}
void SetUpNonExistentDevicePolicy() {
@@ -59,9 +87,38 @@
.WillByDefault(ReturnRef(mock_device_policy_));
}
+ void SetUpDBusSignalExpectations() {
+ // Setup the DBus connection with default actions that should be performed
+ // once.
+ EXPECT_CALL(mock_dbus_, BusGet(_, _)).WillOnce(
+ Return(kFakeConnection));
+ EXPECT_CALL(mock_dbus_, ProxyNewForName(
+ kFakeConnection, StrEq(login_manager::kSessionManagerServiceName),
+ StrEq(login_manager::kSessionManagerServicePath),
+ StrEq(login_manager::kSessionManagerInterface)))
+ .WillOnce(Return(kFakeManagerProxy));
+
+ // Expect the signal to be added, registered and released.
+ EXPECT_CALL(mock_dbus_, ProxyAddSignal_1(
+ kFakeManagerProxy,
+ StrEq(login_manager::kPropertyChangeCompleteSignal),
+ G_TYPE_STRING));
+ EXPECT_CALL(mock_dbus_, ProxyConnectSignal(
+ kFakeManagerProxy,
+ StrEq(login_manager::kPropertyChangeCompleteSignal),
+ _ /* callback */, _ /* data */, _ /* free function */))
+ .WillOnce(DoAll(SaveArg<2>(&signal_callback_),
+ SaveArg<3>(&signal_callback_data_)));
+ }
+
+ chromeos_update_engine::MockDBusWrapper mock_dbus_;
testing::NiceMock<policy::MockDevicePolicy> mock_device_policy_;
testing::NiceMock<policy::MockPolicyProvider> mock_policy_provider_;
unique_ptr<RealDevicePolicyProvider> provider_;
+
+ // The registered signal handler for the signal.
+ GCallback signal_callback_ = nullptr;
+ void* signal_callback_data_ = nullptr;
};
TEST_F(UmRealDevicePolicyProviderTest, RefreshScheduledTest) {
@@ -85,6 +142,23 @@
provider_->RefreshDevicePolicy();
}
+TEST_F(UmRealDevicePolicyProviderTest, SessionManagerSignalForcesReload) {
+ // Checks that a signal from the SessionManager forces a reload.
+ SetUpNonExistentDevicePolicy();
+ EXPECT_CALL(mock_policy_provider_, Reload()).Times(2);
+ EXPECT_TRUE(provider_->Init());
+
+ ASSERT_NE(nullptr, signal_callback_);
+ // Convert the GCallback to a function pointer and call it. GCallback is just
+ // a void function pointer to ensure that the type of the passed callback is a
+ // pointer. We need to cast it back to the right function type before calling
+ // it.
+ typedef void (*StaticSignalHandler)(DBusGProxy*, const char*, void*);
+ StaticSignalHandler signal_handler = reinterpret_cast<StaticSignalHandler>(
+ signal_callback_);
+ (*signal_handler)(kFakeManagerProxy, "success", signal_callback_data_);
+}
+
TEST_F(UmRealDevicePolicyProviderTest, NonExistentDevicePolicyEmptyVariables) {
SetUpNonExistentDevicePolicy();
EXPECT_CALL(mock_policy_provider_, GetDevicePolicy()).Times(0);
diff --git a/update_manager/real_shill_provider.h b/update_manager/real_shill_provider.h
index 1cceadf..5b50bf4 100644
--- a/update_manager/real_shill_provider.h
+++ b/update_manager/real_shill_provider.h
@@ -67,7 +67,7 @@
bool ProcessDefaultService(GValue* value);
// A handler for manager PropertyChanged signal, and a static version.
- void HandlePropertyChanged(DBusGProxy* proxy, const char *name,
+ void HandlePropertyChanged(DBusGProxy* proxy, const char* name,
GValue* value);
static void HandlePropertyChangedStatic(DBusGProxy* proxy, const char* name,
GValue* value, void* data);
diff --git a/update_manager/state_factory.cc b/update_manager/state_factory.cc
index 745f0f5..54c0205 100644
--- a/update_manager/state_factory.cc
+++ b/update_manager/state_factory.cc
@@ -29,7 +29,7 @@
unique_ptr<RealConfigProvider> config_provider(
new RealConfigProvider(system_state->hardware()));
unique_ptr<RealDevicePolicyProvider> device_policy_provider(
- new RealDevicePolicyProvider(policy_provider));
+ new RealDevicePolicyProvider(dbus, policy_provider));
unique_ptr<RealRandomProvider> random_provider(new RealRandomProvider());
unique_ptr<RealShillProvider> shill_provider(
new RealShillProvider(dbus, clock));