update_engine: UM: More permissive use of P2P.
The logic we had in ChromeOSPolicy so far was not as permissive as the
original in P2PManagerImpl::IsP2PEnabled(). Specifically, we want to
allow P2P even if a device policy does not have this value set, but
otherwise indicates that the device is enterprise-enrolled.
This also renames the owner variable name so it is consistent with the
rest of the variable names (no get_ prefix).
BUG=chromium:384087
TEST=Unit tests.
Change-Id: I184ebaebc168352510ea3ed0277ea74167edf666
Reviewed-on: https://chromium-review.googlesource.com/221476
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index a3d6495..291a147 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -360,13 +360,22 @@
}
}
- // Determine whether use of P2P is allowed by policy.
+ // Determine whether use of P2P is allowed by policy. Even if P2P is not
+ // explicitly allowed, we allow it if the device is enterprise enrolled
+ // (that is, missing or empty owner string).
const bool* policy_au_p2p_enabled_p = ec->GetValue(
dp_provider->var_au_p2p_enabled());
- result->p2p_allowed = policy_au_p2p_enabled_p && *policy_au_p2p_enabled_p;
+ if (policy_au_p2p_enabled_p) {
+ result->p2p_allowed = *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())
+ result->p2p_allowed = true;
+ }
}
- // Enable P2P, if so mandated by the updater configuration.
+ // Enable P2P, if so mandated by the updater configuration. This is additive
+ // to whether or not P2P is allowed per device policy (see above).
if (!result->p2p_allowed) {
const bool* updater_p2p_enabled_p = ec->GetValue(
state->updater_provider()->var_p2p_enabled());
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index e3f7acf..ae741e7 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -1193,6 +1193,36 @@
EXPECT_FALSE(result.do_increment_failures);
}
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCanStartAllowedNoUsableUrlsButEnterpriseEnrolled) {
+ // The UpdateCanStart policy returns true; there's a single HTTP URL but its
+ // use is forbidden by policy, and P2P is unset on the policy, however the
+ // device is enterprise-enrolled so P2P is allowed. The result indicates that
+ // no URL can be used.
+ //
+ // Note: The number of failed attempts should not increase in this case (see
+ // above test).
+
+ SetUpdateCheckAllowed(false);
+
+ // 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_http_downloads_enabled()->reset(
+ new bool(false));
+
+ // Check that the UpdateCanStart returns true.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ UpdateDownloadParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ update_state);
+ EXPECT_TRUE(result.update_can_start);
+ EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_GT(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_errors);
+ EXPECT_FALSE(result.do_increment_failures);
+}
+
TEST_F(UmChromeOSPolicyTest, UpdateDownloadAllowedEthernetDefault) {
// Ethernet is always allowed.
diff --git a/update_manager/device_policy_provider.h b/update_manager/device_policy_provider.h
index ba5d4d9..b0b9d36 100644
--- a/update_manager/device_policy_provider.h
+++ b/update_manager/device_policy_provider.h
@@ -45,7 +45,7 @@
// Variable stating the name of the device owner. For enterprise enrolled
// devices, this will be an empty string.
- virtual Variable<std::string>* var_get_owner() = 0;
+ virtual Variable<std::string>* var_owner() = 0;
virtual Variable<bool>* var_http_downloads_enabled() = 0;
diff --git a/update_manager/fake_device_policy_provider.h b/update_manager/fake_device_policy_provider.h
index 54e419a..5819c43 100644
--- a/update_manager/fake_device_policy_provider.h
+++ b/update_manager/fake_device_policy_provider.h
@@ -47,8 +47,8 @@
return &var_allowed_connection_types_for_update_;
}
- FakeVariable<std::string>* var_get_owner() override {
- return &var_get_owner_;
+ FakeVariable<std::string>* var_owner() override {
+ return &var_owner_;
}
FakeVariable<bool>* var_http_downloads_enabled() override {
@@ -75,7 +75,7 @@
FakeVariable<std::set<ConnectionType>>
var_allowed_connection_types_for_update_{
"allowed_connection_types_for_update", kVariableModePoll};
- FakeVariable<std::string> var_get_owner_{"get_owner", kVariableModePoll};
+ FakeVariable<std::string> var_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 ca1f6ca..9a68956 100644
--- a/update_manager/real_device_policy_provider.cc
+++ b/update_manager/real_device_policy_provider.cc
@@ -129,7 +129,7 @@
UpdateVariable(
&var_allowed_connection_types_for_update_,
&RealDevicePolicyProvider::ConvertAllowedConnectionTypesForUpdate);
- UpdateVariable(&var_get_owner_, &DevicePolicy::GetOwner);
+ UpdateVariable(&var_owner_, &DevicePolicy::GetOwner);
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 4b75d68..c3e2980 100644
--- a/update_manager/real_device_policy_provider.h
+++ b/update_manager/real_device_policy_provider.h
@@ -56,8 +56,8 @@
return &var_allowed_connection_types_for_update_;
}
- Variable<std::string>* var_get_owner() override {
- return &var_get_owner_;
+ Variable<std::string>* var_owner() override {
+ return &var_owner_;
}
Variable<bool>* var_http_downloads_enabled() override {
@@ -123,7 +123,7 @@
AsyncCopyVariable<std::set<ConnectionType>>
var_allowed_connection_types_for_update_{
"allowed_connection_types_for_update"};
- AsyncCopyVariable<std::string> var_get_owner_{"get_owner"};
+ AsyncCopyVariable<std::string> var_owner_{"owner"};
AsyncCopyVariable<bool> var_http_downloads_enabled_{"http_downloads_enabled"};
AsyncCopyVariable<bool> var_au_p2p_enabled_{"au_p2p_enabled"};
diff --git a/update_manager/real_device_policy_provider_unittest.cc b/update_manager/real_device_policy_provider_unittest.cc
index 91453d9..0f000a8 100644
--- a/update_manager/real_device_policy_provider_unittest.cc
+++ b/update_manager/real_device_policy_provider_unittest.cc
@@ -99,7 +99,7 @@
UmTestUtils::ExpectVariableNotSet(provider_->var_scatter_factor());
UmTestUtils::ExpectVariableNotSet(
provider_->var_allowed_connection_types_for_update());
- UmTestUtils::ExpectVariableNotSet(provider_->var_get_owner());
+ UmTestUtils::ExpectVariableNotSet(provider_->var_owner());
UmTestUtils::ExpectVariableNotSet(provider_->var_http_downloads_enabled());
UmTestUtils::ExpectVariableNotSet(provider_->var_au_p2p_enabled());
}