update_engine: UM: Separate global P2P usage from payload-specific attributes.
This change is needed for two reasons: (a) The decision regarding the
global P2P enabled state and those pertaining to whether individual
payloads may be downloaded and/or shared via P2P have distinct and not
necessarily nested lifespans. (b) Some parts of the update engine are
concerned with the former and some with the latter, and so we need
separate entry points in the Update Manager to accommodate that.
This also introduces two Omaha-derived values, denoting whether P2P
downloading and/or sharing should be disabled for the current payload,
into the UpdateCanStart policy.
BUG=chromium:425233
TEST=Unit tests.
Change-Id: I0ba0090bd4c5ceb0c812ea218b070945083abd95
Reviewed-on: https://chromium-review.googlesource.com/225150
Tested-by: Gilad Arnold <[email protected]>
Reviewed-by: Alex Deymo <[email protected]>
Commit-Queue: Gilad Arnold <[email protected]>
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 0284c22..a304276 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -356,42 +356,38 @@
is_scattering_active = true;
}
}
+ }
- // 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());
- if (policy_au_p2p_enabled_p) {
- result->p2p_sharing_allowed = *policy_au_p2p_enabled_p;
+ // Find out whether P2P is globally enabled.
+ bool p2p_enabled;
+ EvalStatus p2p_enabled_status = P2PEnabled(ec, state, error, &p2p_enabled);
+ if (p2p_enabled_status != EvalStatus::kSucceeded)
+ return EvalStatus::kFailed;
+
+ // Is P2P is enabled, consider allowing it for downloading and/or sharing.
+ if (p2p_enabled) {
+ // Sharing via P2P is allowed if not disabled by Omaha.
+ if (update_state.p2p_sharing_disabled) {
+ LOG(INFO) << "Blocked P2P sharing because it is disabled by Omaha.";
} else {
- const string* policy_owner_p = ec->GetValue(dp_provider->var_owner());
- if (!policy_owner_p || policy_owner_p->empty())
- result->p2p_sharing_allowed = true;
+ result->p2p_sharing_allowed = true;
}
- }
- // 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_sharing_allowed) {
- const bool* updater_p2p_enabled_p = ec->GetValue(
- state->updater_provider()->var_p2p_enabled());
- result->p2p_sharing_allowed =
- updater_p2p_enabled_p && *updater_p2p_enabled_p;
- }
-
- // Finally, download via P2P is enabled iff P2P is enabled (sharing allowed),
- // an update is not interactive, and other limits haven't been reached.
- if (result->p2p_sharing_allowed) {
- if (update_state.is_interactive) {
- LOG(INFO) << "Blocked P2P download because update is interactive.";
+ // Downloading via P2P is allowed if not disabled by Omaha, an update is not
+ // interactive, and other limits haven't been reached.
+ if (update_state.p2p_downloading_disabled) {
+ LOG(INFO) << "Blocked P2P downloading because it is disabled by Omaha.";
+ } else if (update_state.is_interactive) {
+ LOG(INFO) << "Blocked P2P downloading because update is interactive.";
} else if (update_state.p2p_num_attempts >= kMaxP2PAttempts) {
- LOG(INFO) << "Blocked P2P download as it was attempted too many times.";
+ LOG(INFO) << "Blocked P2P downloading as it was attempted too many "
+ "times.";
} else if (!update_state.p2p_first_attempted.is_null() &&
ec->IsWallclockTimeGreaterThan(
update_state.p2p_first_attempted +
TimeDelta::FromSeconds(kMaxP2PAttemptsPeriodInSeconds))) {
- LOG(INFO) << "Blocked P2P download as its usage timespan exceeds limit.";
+ LOG(INFO) << "Blocked P2P downloading as its usage timespan exceeds "
+ "limit.";
} else {
// P2P download is allowed; if backoff or scattering are active, be sure
// to suppress them, yet prevent any download URL from being used.
@@ -513,6 +509,53 @@
return (*result ? EvalStatus::kSucceeded : EvalStatus::kAskMeAgainLater);
}
+EvalStatus ChromeOSPolicy::P2PEnabled(EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result) const {
+ bool enabled = false;
+
+ // 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).
+ DevicePolicyProvider* const dp_provider = state->device_policy_provider();
+ const bool* device_policy_is_loaded_p = ec->GetValue(
+ dp_provider->var_device_policy_is_loaded());
+ if (device_policy_is_loaded_p && *device_policy_is_loaded_p) {
+ const bool* policy_au_p2p_enabled_p = ec->GetValue(
+ dp_provider->var_au_p2p_enabled());
+ 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())
+ enabled = true;
+ }
+ }
+
+ // Enable P2P, if so mandated by the updater configuration. This is additive
+ // to whether or not P2P is enabled by device policy.
+ if (!enabled) {
+ const bool* updater_p2p_enabled_p = ec->GetValue(
+ state->updater_provider()->var_p2p_enabled());
+ enabled = updater_p2p_enabled_p && *updater_p2p_enabled_p;
+ }
+
+ *result = enabled;
+ return EvalStatus::kSucceeded;
+}
+
+EvalStatus ChromeOSPolicy::P2PEnabledChanged(EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result,
+ bool prev_result) const {
+ EvalStatus status = P2PEnabled(ec, state, error, result);
+ if (status == EvalStatus::kSucceeded && *result == prev_result)
+ return EvalStatus::kAskMeAgainLater;
+ return status;
+}
+
EvalStatus ChromeOSPolicy::NextUpdateCheckTime(EvaluationContext* ec,
State* state, string* error,
Time* next_update_check) const {
diff --git a/update_manager/chromeos_policy.h b/update_manager/chromeos_policy.h
index 5108bc2..6f6fbb2 100644
--- a/update_manager/chromeos_policy.h
+++ b/update_manager/chromeos_policy.h
@@ -60,6 +60,19 @@
std::string* error,
bool* result) const override;
+ EvalStatus P2PEnabled(
+ EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result) const override;
+
+ EvalStatus P2PEnabledChanged(
+ EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result,
+ bool prev_result) const override;
+
protected:
// Policy override.
std::string PolicyName() const override { return "ChromeOSPolicy"; }
@@ -86,9 +99,9 @@
FRIEND_TEST(UmChromeOSPolicyTest,
UpdateCanStartAllowedInteractivePreventsScattering);
FRIEND_TEST(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PDownloadBlockedDueToNumAttempts);
+ UpdateCanStartAllowedP2PDownloadingBlockedDueToNumAttempts);
FRIEND_TEST(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PDownloadBlockedDueToAttemptsPeriod);
+ UpdateCanStartAllowedP2PDownloadingBlockedDueToAttemptsPeriod);
// Auxiliary constant (zero by default).
const base::TimeDelta kZeroInterval;
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index e5d6749..77adfbe 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -142,6 +142,9 @@
update_state.last_download_url_num_errors = 0;
// There were no download errors.
update_state.download_errors = vector<tuple<int, ErrorCode, Time>>();
+ // P2P is not disabled by Omaha.
+ update_state.p2p_downloading_disabled = false;
+ update_state.p2p_sharing_disabled = false;
// P2P was not attempted.
update_state.p2p_num_attempts = 0;
update_state.p2p_first_attempted = Time();
@@ -996,7 +999,57 @@
}
TEST_F(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PDownloadBlockedDueToNumAttempts) {
+ UpdateCanStartAllowedP2PDownloadingBlockedDueToOmaha) {
+ // The UpdateCanStart policy returns true; device policy permits HTTP, but
+ // policy blocks P2P downloading because Omaha forbids it. P2P sharing is
+ // still permitted.
+
+ SetUpdateCheckAllowed(false);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(true));
+ fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
+ new bool(true));
+
+ // Check that the UpdateCanStart returns true.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.p2p_downloading_disabled = true;
+ UpdateDownloadParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ update_state);
+ EXPECT_TRUE(result.update_can_start);
+ EXPECT_FALSE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
+}
+
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCanStartAllowedP2PSharingBlockedDueToOmaha) {
+ // The UpdateCanStart policy returns true; device policy permits HTTP, but
+ // policy blocks P2P sharing because Omaha forbids it. P2P downloading is
+ // still permitted.
+
+ SetUpdateCheckAllowed(false);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(true));
+ fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
+ new bool(true));
+
+ // Check that the UpdateCanStart returns true.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.p2p_sharing_disabled = true;
+ UpdateDownloadParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ update_state);
+ EXPECT_TRUE(result.update_can_start);
+ EXPECT_TRUE(result.p2p_downloading_allowed);
+ EXPECT_FALSE(result.p2p_sharing_allowed);
+}
+
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCanStartAllowedP2PDownloadingBlockedDueToNumAttempts) {
// The UpdateCanStart policy returns true; device policy permits HTTP but
// blocks P2P download, because the max number of P2P downloads have been
// attempted. P2P sharing is still permitted.
@@ -1021,7 +1074,7 @@
}
TEST_F(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PDownloadBlockedDueToAttemptsPeriod) {
+ UpdateCanStartAllowedP2PDownloadingBlockedDueToAttemptsPeriod) {
// The UpdateCanStart policy returns true; device policy permits HTTP but
// blocks P2P download, because the max period for attempt to download via P2P
// has elapsed. P2P sharing is still permitted.
@@ -1507,4 +1560,42 @@
EXPECT_LT(curr_time, result.backoff_expiry);
}
+TEST_F(UmChromeOSPolicyTest, P2PEnabledNotAllowed) {
+ bool result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::P2PEnabled, &result);
+ EXPECT_FALSE(result);
+}
+
+TEST_F(UmChromeOSPolicyTest, P2PEnabledAllowedByDevicePolicy) {
+ fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
+ new bool(true));
+
+ bool result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::P2PEnabled, &result);
+ EXPECT_TRUE(result);
+}
+
+TEST_F(UmChromeOSPolicyTest, P2PEnabledAllowedByUpdater) {
+ fake_state_.updater_provider()->var_p2p_enabled()->reset(new bool(true));
+
+ bool result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::P2PEnabled, &result);
+ EXPECT_TRUE(result);
+}
+
+TEST_F(UmChromeOSPolicyTest, P2PEnabledAllowedDeviceEnterpriseEnrolled) {
+ fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(nullptr);
+ fake_state_.device_policy_provider()->var_owner()->reset(nullptr);
+
+ bool result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::P2PEnabled, &result);
+ EXPECT_TRUE(result);
+}
+
+TEST_F(UmChromeOSPolicyTest, P2PEnabledChangedBlocks) {
+ bool result;
+ ExpectPolicyStatus(EvalStatus::kAskMeAgainLater, &Policy::P2PEnabledChanged,
+ &result, false);
+}
+
} // namespace chromeos_update_manager
diff --git a/update_manager/default_policy.cc b/update_manager/default_policy.cc
index 1300e14..d49591a 100644
--- a/update_manager/default_policy.cc
+++ b/update_manager/default_policy.cc
@@ -70,4 +70,26 @@
return EvalStatus::kSucceeded;
}
+EvalStatus DefaultPolicy::P2PEnabled(
+ EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result) const {
+ *result = false;
+ return EvalStatus::kSucceeded;
+}
+
+EvalStatus DefaultPolicy::P2PEnabledChanged(
+ EvaluationContext* ec,
+ State* state,
+ std::string* error,
+ bool* result,
+ bool prev_result) const {
+ // This policy will always prohibit P2P, so this is signaling to the caller
+ // that the decision is final (because the current value is the same as the
+ // previous one) and there's no need to issue another call.
+ *result = false;
+ return EvalStatus::kSucceeded;
+}
+
} // namespace chromeos_update_manager
diff --git a/update_manager/default_policy.h b/update_manager/default_policy.h
index 1673609..dccbc87 100644
--- a/update_manager/default_policy.h
+++ b/update_manager/default_policy.h
@@ -66,6 +66,14 @@
EvaluationContext* ec, State* state, std::string* error,
bool* result) const override;
+ EvalStatus P2PEnabled(
+ EvaluationContext* ec, State* state, std::string* error,
+ bool* result) const override;
+
+ EvalStatus P2PEnabledChanged(
+ EvaluationContext* ec, State* state, std::string* error,
+ bool* result, bool prev_result) const override;
+
protected:
// Policy override.
std::string PolicyName() const override { return "DefaultPolicy"; }
diff --git a/update_manager/mock_policy.h b/update_manager/mock_policy.h
index af0c9e7..6cfac98 100644
--- a/update_manager/mock_policy.h
+++ b/update_manager/mock_policy.h
@@ -26,13 +26,20 @@
MOCK_CONST_METHOD5(UpdateCanStart,
EvalStatus(EvaluationContext*, State*, std::string*,
- UpdateDownloadParams*,
- UpdateState));
+ UpdateDownloadParams*, UpdateState));
MOCK_CONST_METHOD4(UpdateDownloadAllowed,
EvalStatus(EvaluationContext*, State*, std::string*,
bool*));
+ MOCK_CONST_METHOD4(P2PEnabled,
+ EvalStatus(EvaluationContext*, State*, std::string*,
+ bool*));
+
+ MOCK_CONST_METHOD5(P2PEnabledChanged,
+ EvalStatus(EvaluationContext*, State*, std::string*,
+ bool*, bool));
+
private:
DISALLOW_COPY_AND_ASSIGN(MockPolicy);
};
diff --git a/update_manager/policy.h b/update_manager/policy.h
index 42f4e15..be30f26 100644
--- a/update_manager/policy.h
+++ b/update_manager/policy.h
@@ -86,6 +86,9 @@
// timestamp when it occurred.
std::vector<std::tuple<int, chromeos_update_engine::ErrorCode, base::Time>>
download_errors;
+ // Whether Omaha forbids use of P2P for downloading and/or sharing.
+ bool p2p_downloading_disabled;
+ bool p2p_sharing_disabled;
// The number of P2P download attempts and wallclock-based time when P2P
// download was first attempted.
int p2p_num_attempts;
@@ -195,6 +198,12 @@
if (reinterpret_cast<typeof(&Policy::UpdateDownloadAllowed)>(
policy_method) == &Policy::UpdateDownloadAllowed)
return class_name + "UpdateDownloadAllowed";
+ if (reinterpret_cast<typeof(&Policy::P2PEnabled)>(
+ policy_method) == &Policy::P2PEnabled)
+ return class_name + "P2PEnabled";
+ if (reinterpret_cast<typeof(&Policy::P2PEnabledChanged)>(
+ policy_method) == &Policy::P2PEnabledChanged)
+ return class_name + "P2PEnabledChanged";
NOTREACHED();
return class_name + "(unknown)";
@@ -239,6 +248,21 @@
std::string* error,
bool* result) const = 0;
+ // Checks whether P2P is enabled. This may consult device policy and other
+ // global settings.
+ virtual EvalStatus P2PEnabled(
+ EvaluationContext* ec, State* state, std::string* error,
+ bool* result) const = 0;
+
+ // Checks whether P2P is enabled, but blocks (returns
+ // |EvalStatus::kAskMeAgainLater|) until it is different from |prev_result|.
+ // If the P2P enabled status is not expected to change, will return
+ // immediately with |EvalStatus::kSucceeded|. This internally uses the
+ // P2PEnabled() policy above.
+ virtual EvalStatus P2PEnabledChanged(
+ EvaluationContext* ec, State* state, std::string* error,
+ bool* result, bool prev_result) const = 0;
+
protected:
Policy() {}
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 42edc40..3534cfa 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -170,6 +170,8 @@
update_state.failures_last_updated = Time();
update_state.download_urls = vector<string>{"http://fake/url/"};
update_state.download_errors_max = 10;
+ update_state.p2p_downloading_disabled = false;
+ update_state.p2p_sharing_disabled = false;
update_state.p2p_num_attempts = 0;
update_state.p2p_first_attempted = Time();
update_state.last_download_url_idx = -1;
