UpdateManager: Move logic from UpdateCanStart to UpdateCheckAllowed.
The following should be part of the policy that is consulted before
performing an update check:
1) Whether updates are disabled by device policy.
2) Whether a specific target channel is dictated by the device policy.
This CL moves it from UpdateCanStart into UpdateCheckAllowed.
Another change is renaming the output construct of UpdateCanStart into
'UpdateDownloadParams'; this is in line with the naming of the output
struct of UpdateCheckAllowed, and reflects the fact that it contains
information regarding the download of an update, to be used by the
caller.
BUG=chromium:388386
TEST=Unit tests.
Change-Id: I0631a4464800db77807d7da9a2a2c256b519c5c3
Reviewed-on: https://chromium-review.googlesource.com/205728
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index e2de820..5273ea4 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -152,17 +152,44 @@
EvalStatus ChromeOSPolicy::UpdateCheckAllowed(
EvaluationContext* ec, State* state, string* error,
UpdateCheckParams* result) const {
+ // Set the default return values.
+ result->updates_enabled = true;
+ result->target_channel.clear();
+
+ 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) {
+ // Check whether updates are disabled by policy.
+ const bool* update_disabled_p = ec->GetValue(
+ dp_provider->var_update_disabled());
+ if (update_disabled_p && *update_disabled_p) {
+ result->updates_enabled = false;
+ return EvalStatus::kAskMeAgainLater;
+ }
+
+ // Determine whether a target channel is dictated by policy.
+ const bool* release_channel_delegated_p = ec->GetValue(
+ dp_provider->var_release_channel_delegated());
+ if (release_channel_delegated_p && !(*release_channel_delegated_p)) {
+ const string* release_channel_p = ec->GetValue(
+ dp_provider->var_release_channel());
+ if (release_channel_p)
+ result->target_channel = *release_channel_p;
+ }
+ }
+
+ // Ensure that update checks are timed properly.
Time next_update_check;
if (NextUpdateCheckTime(ec, state, error, &next_update_check) !=
EvalStatus::kSucceeded) {
return EvalStatus::kFailed;
}
-
if (!ec->IsTimeGreaterThan(next_update_check))
return EvalStatus::kAskMeAgainLater;
// It is time to check for an update.
- result->updates_enabled = true;
return EvalStatus::kSucceeded;
}
@@ -170,13 +197,12 @@
EvaluationContext* ec,
State* state,
string* error,
- UpdateCanStartResult* result,
+ UpdateDownloadParams* result,
const bool interactive,
const UpdateState& update_state) const {
// Set the default return values.
result->update_can_start = true;
result->p2p_allowed = false;
- result->target_channel.clear();
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
result->scatter_wait_period = kZeroInterval;
result->scatter_check_threshold = 0;
@@ -200,15 +226,6 @@
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) {
- // Ensure that update is enabled.
- const bool* update_disabled_p = ec->GetValue(
- dp_provider->var_update_disabled());
- if (update_disabled_p && *update_disabled_p) {
- result->update_can_start = false;
- result->cannot_start_reason = UpdateCannotStartReason::kDisabledByPolicy;
- return EvalStatus::kAskMeAgainLater;
- }
-
// Check whether scattering applies to this update attempt. We should not be
// scattering if this is an interactive update check, or if OOBE is enabled
// but not completed.
@@ -251,16 +268,6 @@
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;
-
- // Determine whether a target channel is dictated by policy.
- const bool* release_channel_delegated_p = ec->GetValue(
- dp_provider->var_release_channel_delegated());
- if (release_channel_delegated_p && !(*release_channel_delegated_p)) {
- const string* release_channel_p = ec->GetValue(
- dp_provider->var_release_channel());
- if (release_channel_p)
- result->target_channel = *release_channel_p;
- }
}
// Enable P2P, if so mandated by the updater configuration.
diff --git a/update_manager/chromeos_policy.h b/update_manager/chromeos_policy.h
index aa076ec..d296689 100644
--- a/update_manager/chromeos_policy.h
+++ b/update_manager/chromeos_policy.h
@@ -43,7 +43,7 @@
EvaluationContext* ec,
State* state,
std::string* error,
- UpdateCanStartResult* result,
+ UpdateDownloadParams* result,
const bool interactive,
const UpdateState& update_state) const override;
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index 0d19720..eef2250 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -205,6 +205,38 @@
&Policy::UpdateCheckAllowed, &result);
}
+TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedWithAttributes) {
+ // Update check is allowed, reponse includes attributes for use in the
+ // request.
+ SetUpdateCheckAllowed(true);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_release_channel_delegated()->
+ reset(new bool(false));
+ fake_state_.device_policy_provider()->var_release_channel()->
+ reset(new string("foo-channel"));
+
+ UpdateCheckParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded,
+ &Policy::UpdateCheckAllowed, &result);
+ EXPECT_TRUE(result.updates_enabled);
+ EXPECT_EQ("foo-channel", result.target_channel);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedUpdatesDisabled) {
+ // UpdateCheckAllowed should return kAskMeAgainLater because a device policy
+ // is loaded and prohibits updates.
+
+ SetUpdateCheckAllowed(false);
+ fake_state_.device_policy_provider()->var_update_disabled()->reset(
+ new bool(true));
+
+ // Check that the UpdateCanStart returns false.
+ UpdateCheckParams result;
+ ExpectPolicyStatus(EvalStatus::kAskMeAgainLater,
+ &Policy::UpdateCheckAllowed, &result);
+}
+
TEST_F(UmChromeOSPolicyTest, UpdateCanStartFailsCheckAllowedError) {
// The UpdateCanStart policy fails, not being able to query
// UpdateCheckAllowed.
@@ -214,7 +246,7 @@
// Check that the UpdateCanStart fails.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kFailed,
&Policy::UpdateCanStart, &result, false, update_state);
}
@@ -227,7 +259,7 @@
// Check that the UpdateCanStart returns false.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, false, update_state);
EXPECT_FALSE(result.update_can_start);
@@ -243,12 +275,11 @@
// Check that the UpdateCanStart returns true with no further attributes.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, false, update_state);
EXPECT_TRUE(result.update_can_start);
EXPECT_FALSE(result.p2p_allowed);
- EXPECT_TRUE(result.target_channel.empty());
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_failures);
}
@@ -261,34 +292,15 @@
// Check that the UpdateCanStart returns true.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, false, update_state);
EXPECT_TRUE(result.update_can_start);
EXPECT_FALSE(result.p2p_allowed);
- EXPECT_TRUE(result.target_channel.empty());
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_failures);
}
-TEST_F(UmChromeOSPolicyTest, UpdateCanStartNotAllowedUpdatesDisabled) {
- // The UpdateCanStart should return false (kAskMeAgainlater) because a device
- // policy is loaded and prohibits updates.
-
- SetUpdateCheckAllowed(false);
- fake_state_.device_policy_provider()->var_update_disabled()->reset(
- new bool(true));
-
- // Check that the UpdateCanStart returns false.
- UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
- ExpectPolicyStatus(EvalStatus::kAskMeAgainLater,
- &Policy::UpdateCanStart, &result, false, update_state);
- EXPECT_FALSE(result.update_can_start);
- EXPECT_EQ(UpdateCannotStartReason::kDisabledByPolicy,
- result.cannot_start_reason);
-}
-
TEST_F(UmChromeOSPolicyTest, UpdateCanStartFailsScatteringFailed) {
// The UpdateCanStart policy fails because the UpdateScattering policy it
// depends on fails (unset variable).
@@ -301,7 +313,7 @@
// Check that the UpdateCanStart fails.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromSeconds(1));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kFailed,
&Policy::UpdateCanStart, &result, false, update_state);
}
@@ -321,7 +333,7 @@
// Check that the UpdateCanStart returns false and a new wait period
// generated.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_FALSE(result.update_can_start);
@@ -345,7 +357,7 @@
// Check that the UpdateCanStart returns false and a new wait period
// generated.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kAskMeAgainLater, &Policy::UpdateCanStart,
&result, false, update_state);
EXPECT_FALSE(result.update_can_start);
@@ -371,7 +383,7 @@
update_state.scatter_check_threshold_max = 5;
// Check that the UpdateCanStart returns false.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_FALSE(result.update_can_start);
@@ -395,7 +407,7 @@
update_state.scatter_check_threshold_max = 5;
// Check that the UpdateCanStart returns false.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_FALSE(result.update_can_start);
@@ -420,7 +432,7 @@
update_state.scatter_check_threshold_max = 5;
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -446,7 +458,7 @@
update_state.scatter_check_threshold_max = 5;
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
true, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -473,7 +485,7 @@
update_state.scatter_check_threshold_max = 5;
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
true, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -494,19 +506,14 @@
new bool(true));
fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
new bool(true));
- fake_state_.device_policy_provider()->var_release_channel_delegated()->
- reset(new bool(false));
- fake_state_.device_policy_provider()->var_release_channel()->
- reset(new string("foo-channel"));
// Check that the UpdateCanStart returns true.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
EXPECT_TRUE(result.p2p_allowed);
- EXPECT_EQ("foo-channel", result.target_channel);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_failures);
}
@@ -523,7 +530,7 @@
// Check that the UpdateCanStart returns true.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -548,7 +555,7 @@
// Check that the UpdateCanStart returns true.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -572,7 +579,7 @@
update_state.download_urls.push_back("https://secure/url/");
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -601,7 +608,7 @@
ErrorCode::kDownloadWriteError);
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -625,7 +632,7 @@
ErrorCode::kPayloadHashMismatchError);
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -650,7 +657,7 @@
ErrorCode::kPayloadHashMismatchError);
// Check that the UpdateCanStart returns true.
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
@@ -671,7 +678,7 @@
// Check that the UpdateCanStart returns false.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kFailed, &Policy::UpdateCanStart, &result,
false, update_state);
}
@@ -691,7 +698,7 @@
// Check that the UpdateCanStart returns true.
UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
diff --git a/update_manager/default_policy.h b/update_manager/default_policy.h
index 54bbe3e..ba78724 100644
--- a/update_manager/default_policy.h
+++ b/update_manager/default_policy.h
@@ -26,6 +26,7 @@
EvaluationContext* ec, State* state, std::string* error,
UpdateCheckParams* result) const override {
result->updates_enabled = true;
+ result->target_channel.clear();
return EvalStatus::kSucceeded;
}
@@ -33,12 +34,11 @@
EvaluationContext* ec,
State* state,
std::string* error,
- UpdateCanStartResult* result,
+ UpdateDownloadParams* result,
const bool interactive,
const UpdateState& update_state) const override {
result->update_can_start = true;
result->p2p_allowed = false;
- result->target_channel.clear();
result->download_url_idx = 0;
result->download_url_num_failures = 0;
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
diff --git a/update_manager/mock_policy.h b/update_manager/mock_policy.h
index 28c5cec..f6acb7f 100644
--- a/update_manager/mock_policy.h
+++ b/update_manager/mock_policy.h
@@ -26,7 +26,7 @@
MOCK_CONST_METHOD6(UpdateCanStart,
EvalStatus(EvaluationContext*, State*, std::string*,
- UpdateCanStartResult*,
+ UpdateDownloadParams*,
const bool, const UpdateState&));
MOCK_CONST_METHOD4(UpdateCanStart,
diff --git a/update_manager/policy.h b/update_manager/policy.h
index 42a10bf..64869c0 100644
--- a/update_manager/policy.h
+++ b/update_manager/policy.h
@@ -27,6 +27,11 @@
// UpdateCheckAllowed policy.
struct UpdateCheckParams {
bool updates_enabled; // Whether the auto-updates are enabled on this build.
+
+ // Attributes pertaining to the case where update checks are allowed.
+ //
+ // A target channel, if so imposed by policy; otherwise, an empty string.
+ std::string target_channel;
};
// Input arguments to UpdateCanStart.
@@ -77,12 +82,11 @@
enum class UpdateCannotStartReason {
kUndefined,
kCheckDue,
- kDisabledByPolicy,
kScattering,
kCannotDownload,
};
-struct UpdateCanStartResult {
+struct UpdateDownloadParams {
// Whether the update attempt is allowed to proceed.
bool update_can_start;
@@ -90,7 +94,6 @@
// engine uses them to choose the means for downloading and applying an
// update.
bool p2p_allowed;
- std::string target_channel;
// The index of the download URL to use, and the number of failures associated
// with this URL. An index value of -1 indicates that no suitable URL is
// available, but there may be other means for download (like P2P).
@@ -164,7 +167,7 @@
EvaluationContext* ec,
State* state,
std::string* error,
- UpdateCanStartResult* result,
+ UpdateDownloadParams* result,
const bool interactive,
const UpdateState& update_state) const = 0;
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 640fbb3..4a15e93 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -113,7 +113,7 @@
TimeDelta::FromSeconds(15), TimeDelta::FromSeconds(60),
4, 2, 8
};
- UpdateCanStartResult result;
+ UpdateDownloadParams result;
EXPECT_EQ(EvalStatus::kSucceeded,
umut_->PolicyRequest(&Policy::UpdateCanStart, &result, true,
update_state));