update_engine: Indicate existence of owner instead of owner's email in log.
Logging the device owner's email is possibly a PII, so instead log
whether the device has a owner as a boolean value.
Enterprise devices do not have a device owner.
The variable var_has_owner is dependent on DevicePolicy::GetOwner().
BUG=chromium:973108
TEST=unittest
Change-Id: I535f664a4fcf75c6102346b8566605710b062255
Reviewed-on: https://chromium-review.googlesource.com/1660911
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Ready: Jae Hoon Kim <kimjae@chromium.org>
Legacy-Commit-Queue: Commit Bot <commit-bot@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index bdb88f8..08c355e 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -555,8 +555,9 @@
if (policy_au_p2p_enabled_p) {
enabled = *policy_au_p2p_enabled_p;
} else {
- const string* policy_owner_p = ec->GetValue(dp_provider->var_owner());
- if (!policy_owner_p || policy_owner_p->empty())
+ const bool* policy_has_owner_p =
+ ec->GetValue(dp_provider->var_has_owner());
+ if (!policy_has_owner_p || !*policy_has_owner_p)
enabled = true;
}
}
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index fb8c789..25c91fa 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -1365,7 +1365,7 @@
// Override specific device policy attributes.
fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(nullptr);
- fake_state_.device_policy_provider()->var_owner()->reset(nullptr);
+ fake_state_.device_policy_provider()->var_has_owner()->reset(new bool(false));
fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
new bool(false));
@@ -1610,7 +1610,7 @@
TEST_F(UmChromeOSPolicyTest, P2PEnabledAllowedDeviceEnterpriseEnrolled) {
fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(nullptr);
- fake_state_.device_policy_provider()->var_owner()->reset(nullptr);
+ fake_state_.device_policy_provider()->var_has_owner()->reset(new bool(false));
bool result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::P2PEnabled, &result);
diff --git a/update_manager/device_policy_provider.h b/update_manager/device_policy_provider.h
index 873282e..b68fe96 100644
--- a/update_manager/device_policy_provider.h
+++ b/update_manager/device_policy_provider.h
@@ -66,9 +66,9 @@
virtual Variable<std::set<chromeos_update_engine::ConnectionType>>*
var_allowed_connection_types_for_update() = 0;
- // Variable stating the name of the device owner. For enterprise enrolled
- // devices, this will be an empty string.
- virtual Variable<std::string>* var_owner() = 0;
+ // Variable stating whether the device has an owner. For enterprise enrolled
+ // devices, this will be false as the device owner has an empty string.
+ virtual Variable<bool>* var_has_owner() = 0;
virtual Variable<bool>* var_http_downloads_enabled() = 0;
diff --git a/update_manager/evaluation_context_unittest.cc b/update_manager/evaluation_context_unittest.cc
index eb42eb7..151b0b5 100644
--- a/update_manager/evaluation_context_unittest.cc
+++ b/update_manager/evaluation_context_unittest.cc
@@ -210,13 +210,11 @@
fake_const_var_.reset(new string("Hello world!"));
EXPECT_EQ(*eval_ctx_->GetValue(&fake_const_var_), "Hello world!");
- EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
// Test that reevaluation occurs when an async variable it depends on changes.
@@ -286,23 +284,19 @@
EXPECT_TRUE(value);
// Ensure that we cannot reschedule an evaluation.
- EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
// Ensure that we can reschedule an evaluation after resetting expiration.
eval_ctx_->ResetExpiration();
- EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
// Test that we clear the events when destroying the EvaluationContext.
@@ -348,13 +342,11 @@
fake_poll_var_.reset(new string("Polled value"));
eval_ctx_->GetValue(&fake_async_var_);
eval_ctx_->GetValue(&fake_poll_var_);
- EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
// TearDown() checks for leaked observers on this async_variable, which means
// that our object is still alive after removing its reference.
}
@@ -446,13 +438,11 @@
// The "false" from IsWallclockTimeGreaterThan means that's not that timestamp
// yet, so this should schedule a callback for when that happens.
- EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
TEST_F(UmEvaluationContextTest,
@@ -462,13 +452,11 @@
// The "false" from IsMonotonicTimeGreaterThan means that's not that timestamp
// yet, so this should schedule a callback for when that happens.
- EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
TEST_F(UmEvaluationContextTest,
@@ -481,13 +469,11 @@
fake_clock_.GetWallclockTime() - TimeDelta::FromSeconds(1)));
// Callback should not be scheduled.
- EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
TEST_F(UmEvaluationContextTest,
@@ -500,13 +486,11 @@
fake_clock_.GetMonotonicTime() - TimeDelta::FromSeconds(1)));
// Callback should not be scheduled.
- EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(
#if BASE_VER < 576279
- Bind(&base::DoNothing)
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&base::DoNothing)));
#else
- base::DoNothing()
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(base::DoNothing()));
#endif
- ));
}
TEST_F(UmEvaluationContextTest, DumpContext) {
diff --git a/update_manager/fake_device_policy_provider.h b/update_manager/fake_device_policy_provider.h
index 7cd4d7b..86bdef1 100644
--- a/update_manager/fake_device_policy_provider.h
+++ b/update_manager/fake_device_policy_provider.h
@@ -68,7 +68,7 @@
return &var_allowed_connection_types_for_update_;
}
- FakeVariable<std::string>* var_owner() override { return &var_owner_; }
+ FakeVariable<bool>* var_has_owner() override { return &var_has_owner_; }
FakeVariable<bool>* var_http_downloads_enabled() override {
return &var_http_downloads_enabled_;
@@ -110,7 +110,7 @@
FakeVariable<std::set<chromeos_update_engine::ConnectionType>>
var_allowed_connection_types_for_update_{
"allowed_connection_types_for_update", kVariableModePoll};
- FakeVariable<std::string> var_owner_{"owner", kVariableModePoll};
+ FakeVariable<bool> var_has_owner_{"owner", kVariableModePoll};
FakeVariable<bool> var_http_downloads_enabled_{"http_downloads_enabled",
kVariableModePoll};
FakeVariable<bool> var_au_p2p_enabled_{"au_p2p_enabled", kVariableModePoll};
diff --git a/update_manager/real_device_policy_provider.cc b/update_manager/real_device_policy_provider.cc
index 586ee3e..781e2ac 100644
--- a/update_manager/real_device_policy_provider.cc
+++ b/update_manager/real_device_policy_provider.cc
@@ -104,11 +104,12 @@
}
template <typename T>
-void RealDevicePolicyProvider::UpdateVariable(
- AsyncCopyVariable<T>* var, bool (DevicePolicy::*getter_method)(T*) const) {
+void RealDevicePolicyProvider::UpdateVariable(AsyncCopyVariable<T>* var,
+ bool (DevicePolicy::*getter)(T*)
+ const) {
T new_value;
if (policy_provider_->device_policy_is_loaded() &&
- (policy_provider_->GetDevicePolicy().*getter_method)(&new_value)) {
+ (policy_provider_->GetDevicePolicy().*getter)(&new_value)) {
var->SetValue(new_value);
} else {
var->UnsetValue();
@@ -118,10 +119,10 @@
template <typename T>
void RealDevicePolicyProvider::UpdateVariable(
AsyncCopyVariable<T>* var,
- bool (RealDevicePolicyProvider::*getter_method)(T*) const) {
+ bool (RealDevicePolicyProvider::*getter)(T*) const) {
T new_value;
if (policy_provider_->device_policy_is_loaded() &&
- (this->*getter_method)(&new_value)) {
+ (this->*getter)(&new_value)) {
var->SetValue(new_value);
} else {
var->UnsetValue();
@@ -198,6 +199,15 @@
return true;
}
+bool RealDevicePolicyProvider::ConvertHasOwner(bool* has_owner) const {
+ string owner;
+ if (!policy_provider_->GetDevicePolicy().GetOwner(&owner)) {
+ return false;
+ }
+ *has_owner = !owner.empty();
+ return true;
+}
+
void RealDevicePolicyProvider::RefreshDevicePolicy() {
if (!policy_provider_->Reload()) {
LOG(INFO) << "No device policies/settings present.";
@@ -225,7 +235,7 @@
UpdateVariable(
&var_allowed_connection_types_for_update_,
&RealDevicePolicyProvider::ConvertAllowedConnectionTypesForUpdate);
- UpdateVariable(&var_owner_, &DevicePolicy::GetOwner);
+ UpdateVariable(&var_has_owner_, &RealDevicePolicyProvider::ConvertHasOwner);
UpdateVariable(&var_http_downloads_enabled_,
&DevicePolicy::GetHttpDownloadsEnabled);
UpdateVariable(&var_au_p2p_enabled_, &DevicePolicy::GetAuP2PEnabled);
diff --git a/update_manager/real_device_policy_provider.h b/update_manager/real_device_policy_provider.h
index bda4cff..9da052d 100644
--- a/update_manager/real_device_policy_provider.h
+++ b/update_manager/real_device_policy_provider.h
@@ -34,7 +34,7 @@
namespace chromeos_update_manager {
-// DevicePolicyProvider concrete implementation.
+// |DevicePolicyProvider| concrete implementation.
class RealDevicePolicyProvider : public DevicePolicyProvider {
public:
#if USE_DBUS
@@ -89,7 +89,7 @@
return &var_allowed_connection_types_for_update_;
}
- Variable<std::string>* var_owner() override { return &var_owner_; }
+ Variable<bool>* var_has_owner() override { return &var_has_owner_; }
Variable<bool>* var_http_downloads_enabled() override {
return &var_http_downloads_enabled_;
@@ -113,12 +113,13 @@
FRIEND_TEST(UmRealDevicePolicyProviderTest, RefreshScheduledTest);
FRIEND_TEST(UmRealDevicePolicyProviderTest, NonExistentDevicePolicyReloaded);
FRIEND_TEST(UmRealDevicePolicyProviderTest, ValuesUpdated);
+ FRIEND_TEST(UmRealDevicePolicyProviderTest, HasOwnerConverted);
- // A static handler for the PropertyChangedCompleted signal from the session
+ // A static handler for the |PropertyChangedCompleted| signal from the session
// manager used as a callback.
void OnPropertyChangedCompletedSignal(const std::string& success);
- // Called when the signal in UpdateEngineLibcrosProxyResolvedInterface is
+ // Called when the signal in |UpdateEngineLibcrosProxyResolvedInterface| is
// connected.
void OnSignalConnected(const std::string& interface_name,
const std::string& signal_name,
@@ -134,36 +135,41 @@
// passed, which is a DevicePolicy getter method.
template <typename T>
void UpdateVariable(AsyncCopyVariable<T>* var,
- bool (policy::DevicePolicy::*getter_method)(T*) const);
+ bool (policy::DevicePolicy::*getter)(T*) const);
// Updates the async variable |var| based on the result value of the getter
// method passed, which is a wrapper getter on this class.
template <typename T>
void UpdateVariable(AsyncCopyVariable<T>* var,
- bool (RealDevicePolicyProvider::*getter_method)(T*)
- const);
+ bool (RealDevicePolicyProvider::*getter)(T*) const);
- // Wrapper for DevicePolicy::GetRollbackToTargetVersion() that converts the
- // result to RollbackToTargetVersion.
+ // Wrapper for |DevicePolicy::GetRollbackToTargetVersion()| that converts the
+ // result to |RollbackToTargetVersion|.
bool ConvertRollbackToTargetVersion(
RollbackToTargetVersion* rollback_to_target_version) const;
- // Wrapper for DevicePolicy::GetScatterFactorInSeconds() that converts the
- // result to a base::TimeDelta. It returns the same value as
- // GetScatterFactorInSeconds().
+ // Wrapper for |DevicePolicy::GetScatterFactorInSeconds()| that converts the
+ // result to a |base::TimeDelta|. It returns the same value as
+ // |GetScatterFactorInSeconds()|.
bool ConvertScatterFactor(base::TimeDelta* scatter_factor) const;
- // Wrapper for DevicePolicy::GetAllowedConnectionTypesForUpdate() that
- // converts the result to a set of ConnectionType elements instead of strings.
+ // Wrapper for |DevicePolicy::GetAllowedConnectionTypesForUpdate()| that
+ // converts the result to a set of |ConnectionType| elements instead of
+ // strings.
bool ConvertAllowedConnectionTypesForUpdate(
std::set<chromeos_update_engine::ConnectionType>* allowed_types) const;
- // Wrapper for DevicePolicy::GetUpdateTimeRestrictions() that converts
- // the DevicePolicy::WeeklyTimeInterval structs to WeeklyTimeInterval objects,
- // which offer more functionality.
+ // Wrapper for |DevicePolicy::GetUpdateTimeRestrictions()| that converts
+ // the |DevicePolicy::WeeklyTimeInterval| structs to |WeeklyTimeInterval|
+ // objects, which offer more functionality.
bool ConvertDisallowedTimeIntervals(
WeeklyTimeIntervalVector* disallowed_intervals_out) const;
+ // Wrapper for |DevicePolicy::GetOwner()| that converts the result to a
+ // boolean of whether the device has an owner. (Enterprise enrolled
+ // devices do not have an owner).
+ bool ConvertHasOwner(bool* has_owner) const;
+
// Used for fetching information about the device policy.
policy::PolicyProvider* policy_provider_;
@@ -181,7 +187,7 @@
AsyncCopyVariable<bool> var_device_policy_is_loaded_{"policy_is_loaded",
false};
- // Variables mapping the exposed methods from the policy::DevicePolicy.
+ // Variables mapping the exposed methods from the |policy::DevicePolicy|.
AsyncCopyVariable<std::string> var_release_channel_{"release_channel"};
AsyncCopyVariable<bool> var_release_channel_delegated_{
"release_channel_delegated"};
@@ -196,7 +202,7 @@
AsyncCopyVariable<std::set<chromeos_update_engine::ConnectionType>>
var_allowed_connection_types_for_update_{
"allowed_connection_types_for_update"};
- AsyncCopyVariable<std::string> var_owner_{"owner"};
+ AsyncCopyVariable<bool> var_has_owner_{"owner"};
AsyncCopyVariable<bool> var_http_downloads_enabled_{"http_downloads_enabled"};
AsyncCopyVariable<bool> var_au_p2p_enabled_{"au_p2p_enabled"};
AsyncCopyVariable<bool> var_allow_kiosk_app_control_chrome_version_{
diff --git a/update_manager/real_device_policy_provider_unittest.cc b/update_manager/real_device_policy_provider_unittest.cc
index 0d7b0d0..8f2c377 100644
--- a/update_manager/real_device_policy_provider_unittest.cc
+++ b/update_manager/real_device_policy_provider_unittest.cc
@@ -186,7 +186,7 @@
UmTestUtils::ExpectVariableNotSet(provider_->var_scatter_factor());
UmTestUtils::ExpectVariableNotSet(
provider_->var_allowed_connection_types_for_update());
- UmTestUtils::ExpectVariableNotSet(provider_->var_owner());
+ UmTestUtils::ExpectVariableNotSet(provider_->var_has_owner());
UmTestUtils::ExpectVariableNotSet(provider_->var_http_downloads_enabled());
UmTestUtils::ExpectVariableNotSet(provider_->var_au_p2p_enabled());
UmTestUtils::ExpectVariableNotSet(
@@ -230,6 +230,26 @@
string("myapp"), provider_->var_auto_launched_kiosk_app_id());
}
+TEST_F(UmRealDevicePolicyProviderTest, HasOwnerConverted) {
+ SetUpExistentDevicePolicy();
+ EXPECT_TRUE(provider_->Init());
+ loop_.RunOnce(false);
+ Mock::VerifyAndClearExpectations(&mock_policy_provider_);
+
+ EXPECT_CALL(mock_device_policy_, GetOwner(_))
+ .Times(2)
+ .WillOnce(DoAll(SetArgPointee<0>(string("")), Return(true)))
+ .WillOnce(DoAll(SetArgPointee<0>(string("abc@test.org")), Return(true)));
+
+ // Enterprise enrolled device.
+ provider_->RefreshDevicePolicy();
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_has_owner());
+
+ // Has a device owner.
+ provider_->RefreshDevicePolicy();
+ UmTestUtils::ExpectVariableHasValue(true, provider_->var_has_owner());
+}
+
TEST_F(UmRealDevicePolicyProviderTest, RollbackToTargetVersionConverted) {
SetUpExistentDevicePolicy();
EXPECT_CALL(mock_device_policy_, GetRollbackToTargetVersion(_))