update_engine: Add rollback parameters to OmahaRequestParamsPolicy
As part of moving all request params to this policy.
BUG=b:179419726
TEST=cros_workon_make --board reef --test update_engine
Change-Id: I417e0f6c8b5ea48cadeed19ab96940079bd086f9
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2829360
Tested-by: Amin Hassani <ahassani@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
diff --git a/cros/omaha_request_params.cc b/cros/omaha_request_params.cc
index 3ec4ade..09f6cf4 100644
--- a/cros/omaha_request_params.cc
+++ b/cros/omaha_request_params.cc
@@ -137,13 +137,9 @@
quick_fix_build_token_.clear();
- rollback_allowed_ = params.rollback_allowed;
-
- // Set whether saving data over rollback is requested.
- rollback_data_save_requested_ = params.rollback_data_save_requested;
-
- // Set how many milestones of rollback are allowed.
- rollback_allowed_milestones_ = params.rollback_allowed_milestones;
+ rollback_allowed_ = false;
+ rollback_data_save_requested_ = false;
+ rollback_allowed_milestones_ = 0;
// Set the target channel, if one was provided.
if (params.target_channel.empty()) {
diff --git a/cros/omaha_request_params.h b/cros/omaha_request_params.h
index 81350ea..ac26563 100644
--- a/cros/omaha_request_params.h
+++ b/cros/omaha_request_params.h
@@ -393,13 +393,19 @@
// to be pinned to. It's empty otherwise.
std::string target_version_prefix_;
- // Whether the client is accepting rollback images too.
+ // Whether the client is accepting rollback images defined by policy.
+ // Normally ss set by |OmahaRequestParamsPolicy|.
bool rollback_allowed_;
// Whether rollbacks should preserve some system state during powerwash.
+ // Normally ss set by |OmahaRequestParamsPolicy|.
bool rollback_data_save_requested_;
- // How many milestones the client can rollback to.
+ // Specifies the number of Chrome milestones rollback should be allowed,
+ // starting from the stable version at any time. Value is -1 if unspecified
+ // (e.g. no device policy is available yet), in this case no version
+ // roll-forward should happen.
+ // Normally ss set by |OmahaRequestParamsPolicy|.
int rollback_allowed_milestones_;
// True if scattering or staging are enabled, in which case waiting_period_
diff --git a/cros/update_attempter_unittest.cc b/cros/update_attempter_unittest.cc
index ae75e86..6a2f4fe 100644
--- a/cros/update_attempter_unittest.cc
+++ b/cros/update_attempter_unittest.cc
@@ -1649,27 +1649,6 @@
.empty());
}
-TEST_F(UpdateAttempterTest, RollbackAllowedSetAndReset) {
- attempter_.CalculateUpdateParams({
- .target_version_prefix = "1234",
- .rollback_allowed = true,
- .rollback_allowed_milestones = 4,
- });
- EXPECT_TRUE(FakeSystemState::Get()->request_params()->rollback_allowed());
- EXPECT_EQ(
- 4,
- FakeSystemState::Get()->request_params()->rollback_allowed_milestones());
-
- attempter_.CalculateUpdateParams({
- .target_version_prefix = "1234",
- .rollback_allowed_milestones = 4,
- });
- EXPECT_FALSE(FakeSystemState::Get()->request_params()->rollback_allowed());
- EXPECT_EQ(
- 4,
- FakeSystemState::Get()->request_params()->rollback_allowed_milestones());
-}
-
TEST_F(UpdateAttempterTest, ChannelDowngradeNoRollback) {
base::ScopedTempDir tempdir;
ASSERT_TRUE(tempdir.CreateUniqueTempDir());
@@ -1753,20 +1732,6 @@
attempter_.GetCurrentUpdateAttemptFlags());
}
-TEST_F(UpdateAttempterTest, RollbackNotAllowed) {
- attempter_.policy_data_.reset(new UpdateCheckAllowedPolicyData(
- {.updates_enabled = true, .rollback_allowed = false}));
- attempter_.OnUpdateScheduled(EvalStatus::kSucceeded);
- EXPECT_FALSE(FakeSystemState::Get()->request_params()->rollback_allowed());
-}
-
-TEST_F(UpdateAttempterTest, RollbackAllowed) {
- attempter_.policy_data_.reset(new UpdateCheckAllowedPolicyData(
- {.updates_enabled = true, .rollback_allowed = true}));
- attempter_.OnUpdateScheduled(EvalStatus::kSucceeded);
- EXPECT_TRUE(FakeSystemState::Get()->request_params()->rollback_allowed());
-}
-
TEST_F(UpdateAttempterTest, InteractiveUpdateUsesPassedRestrictions) {
attempter_.SetUpdateAttemptFlags(UpdateAttemptFlags::kFlagRestrictDownload);
diff --git a/update_manager/enterprise_device_policy_impl.cc b/update_manager/enterprise_device_policy_impl.cc
index 5a4b763..eecc113 100644
--- a/update_manager/enterprise_device_policy_impl.cc
+++ b/update_manager/enterprise_device_policy_impl.cc
@@ -109,44 +109,6 @@
result->target_version_prefix = *target_version_prefix_p;
}
- // Policy always overwrites whether rollback is allowed by the kiosk app
- // manifest.
- const RollbackToTargetVersion* rollback_to_target_version_p =
- ec->GetValue(dp_provider->var_rollback_to_target_version());
- if (rollback_to_target_version_p) {
- switch (*rollback_to_target_version_p) {
- case RollbackToTargetVersion::kUnspecified:
- // We leave the default or the one specified by the kiosk app.
- break;
- case RollbackToTargetVersion::kDisabled:
- LOG(INFO) << "Policy disables rollbacks.";
- result->rollback_allowed = false;
- result->rollback_data_save_requested = false;
- break;
- case RollbackToTargetVersion::kRollbackAndPowerwash:
- LOG(INFO) << "Policy allows rollbacks with powerwash.";
- result->rollback_allowed = true;
- result->rollback_data_save_requested = false;
- break;
- case RollbackToTargetVersion::kRollbackAndRestoreIfPossible:
- LOG(INFO)
- << "Policy allows rollbacks, also tries to restore if possible.";
- result->rollback_allowed = true;
- result->rollback_data_save_requested = true;
- break;
- case RollbackToTargetVersion::kMaxValue:
- NOTREACHED();
- // Don't add a default case to let the compiler warn about newly
- // added enum values which should be added here.
- }
- }
-
- // Determine allowed milestones for rollback
- const int* rollback_allowed_milestones_p =
- ec->GetValue(dp_provider->var_rollback_allowed_milestones());
- if (rollback_allowed_milestones_p)
- result->rollback_allowed_milestones = *rollback_allowed_milestones_p;
-
// Determine whether a target channel is dictated by policy and whether we
// should rollback in case that channel is more stable.
const bool* release_channel_delegated_p =
diff --git a/update_manager/enterprise_device_policy_impl_unittest.cc b/update_manager/enterprise_device_policy_impl_unittest.cc
index 4137245..2bf88bc 100644
--- a/update_manager/enterprise_device_policy_impl_unittest.cc
+++ b/update_manager/enterprise_device_policy_impl_unittest.cc
@@ -56,22 +56,6 @@
new string("1234."));
}
- // Sets up a test with the value of RollbackToTargetVersion policy (and
- // whether it's set), and returns the value of
- // UpdateCheckParams.rollback_allowed.
- bool TestRollbackAllowed(bool set_policy,
- RollbackToTargetVersion rollback_to_target_version) {
- if (set_policy) {
- // Override RollbackToTargetVersion device policy attribute.
- fake_state_.device_policy_provider()
- ->var_rollback_to_target_version()
- ->reset(new RollbackToTargetVersion(rollback_to_target_version));
- }
-
- EXPECT_EQ(EvalStatus::kContinue, evaluator_->Evaluate());
- return uca_data_->update_check_params.rollback_allowed;
- }
-
UpdateCheckAllowedPolicyData* uca_data_;
};
@@ -183,65 +167,6 @@
EXPECT_TRUE(uca_data_->update_check_params.rollback_on_channel_downgrade);
}
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedRollbackAndPowerwash) {
- EXPECT_TRUE(TestRollbackAllowed(
- true, RollbackToTargetVersion::kRollbackAndPowerwash));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedRollbackAndRestoreIfPossible) {
- // We're doing rollback even if we don't support data save and restore.
- EXPECT_TRUE(TestRollbackAllowed(
- true, RollbackToTargetVersion::kRollbackAndRestoreIfPossible));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest, UpdateCheckAllowedRollbackDisabled) {
- EXPECT_FALSE(TestRollbackAllowed(true, RollbackToTargetVersion::kDisabled));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedRollbackUnspecified) {
- EXPECT_FALSE(
- TestRollbackAllowed(true, RollbackToTargetVersion::kUnspecified));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest, UpdateCheckAllowedRollbackNotSet) {
- EXPECT_FALSE(
- TestRollbackAllowed(false, RollbackToTargetVersion::kUnspecified));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedKioskRollbackAllowed) {
- SetKioskAppControlsChromeOsVersion();
-
- EXPECT_TRUE(TestRollbackAllowed(
- true, RollbackToTargetVersion::kRollbackAndPowerwash));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedKioskRollbackDisabled) {
- SetKioskAppControlsChromeOsVersion();
-
- EXPECT_FALSE(TestRollbackAllowed(true, RollbackToTargetVersion::kDisabled));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedKioskRollbackUnspecified) {
- SetKioskAppControlsChromeOsVersion();
-
- EXPECT_FALSE(
- TestRollbackAllowed(true, RollbackToTargetVersion::kUnspecified));
-}
-
-TEST_F(UmEnterpriseDevicePolicyImplTest,
- UpdateCheckAllowedKioskRollbackNotSet) {
- SetKioskAppControlsChromeOsVersion();
-
- EXPECT_FALSE(
- TestRollbackAllowed(false, RollbackToTargetVersion::kUnspecified));
-}
-
TEST_F(UmEnterpriseDevicePolicyImplTest, UpdateCheckAllowedKioskPin) {
SetKioskAppControlsChromeOsVersion();
diff --git a/update_manager/omaha_request_params_policy.cc b/update_manager/omaha_request_params_policy.cc
index fa4f2e3..f2412c3 100644
--- a/update_manager/omaha_request_params_policy.cc
+++ b/update_manager/omaha_request_params_policy.cc
@@ -71,6 +71,44 @@
request_params->set_release_lts_tag(*release_lts_tag_p);
}
+ // Policy always overwrites whether rollback is allowed by the kiosk app
+ // manifest.
+ const RollbackToTargetVersion* rollback_to_target_version_p =
+ ec->GetValue(dp_provider->var_rollback_to_target_version());
+ // Set the default values, just in case.
+ request_params->set_rollback_allowed(false);
+ request_params->set_rollback_data_save_requested(false);
+ if (rollback_to_target_version_p) {
+ switch (*rollback_to_target_version_p) {
+ case RollbackToTargetVersion::kUnspecified:
+ break;
+ case RollbackToTargetVersion::kDisabled:
+ LOG(INFO) << "Policy disables rollbacks.";
+ break;
+ case RollbackToTargetVersion::kRollbackAndPowerwash:
+ LOG(INFO) << "Policy allows rollbacks with powerwash.";
+ request_params->set_rollback_allowed(true);
+ break;
+ case RollbackToTargetVersion::kRollbackAndRestoreIfPossible:
+ LOG(INFO)
+ << "Policy allows rollbacks, also tries to restore if possible.";
+ request_params->set_rollback_allowed(true);
+ request_params->set_rollback_data_save_requested(true);
+ break;
+ case RollbackToTargetVersion::kMaxValue:
+ NOTREACHED();
+ // Don't add a default case to let the compiler warn about newly
+ // added enum values which should be added here.
+ }
+ }
+
+ // Determine allowed milestones for rollback
+ const int* rollback_allowed_milestones_p =
+ ec->GetValue(dp_provider->var_rollback_allowed_milestones());
+ if (rollback_allowed_milestones_p)
+ request_params->set_rollback_allowed_milestones(
+ *rollback_allowed_milestones_p);
+
return EvalStatus::kSucceeded;
}
diff --git a/update_manager/omaha_request_params_policy_unittest.cc b/update_manager/omaha_request_params_policy_unittest.cc
index 58a7ab5..d08bb51 100644
--- a/update_manager/omaha_request_params_policy_unittest.cc
+++ b/update_manager/omaha_request_params_policy_unittest.cc
@@ -34,6 +34,22 @@
fake_state_.device_policy_provider()->var_device_policy_is_loaded()->reset(
new bool(true));
}
+
+ // Sets up a test with the value of RollbackToTargetVersion policy (and
+ // whether it's set), and returns the value of
+ // UpdateCheckParams.rollback_allowed.
+ bool TestRollbackAllowed(bool set_policy,
+ RollbackToTargetVersion rollback_to_target_version) {
+ if (set_policy) {
+ // Override RollbackToTargetVersion device policy attribute.
+ fake_state_.device_policy_provider()
+ ->var_rollback_to_target_version()
+ ->reset(new RollbackToTargetVersion(rollback_to_target_version));
+ }
+
+ EXPECT_EQ(EvalStatus::kSucceeded, evaluator_->Evaluate());
+ return FakeSystemState::Get()->request_params()->rollback_allowed();
+ }
};
TEST_F(UmOmahaRequestParamsPolicyTest, PolicyIsLoaded) {
@@ -95,4 +111,29 @@
kReleaseTag);
}
+TEST_F(UmOmahaRequestParamsPolicyTest, RollbackAndPowerwash) {
+ EXPECT_TRUE(TestRollbackAllowed(
+ true, RollbackToTargetVersion::kRollbackAndPowerwash));
+}
+
+TEST_F(UmOmahaRequestParamsPolicyTest, RollbackAndRestoreIfPossible) {
+ // We're doing rollback even if we don't support data save and restore.
+ EXPECT_TRUE(TestRollbackAllowed(
+ true, RollbackToTargetVersion::kRollbackAndRestoreIfPossible));
+}
+
+TEST_F(UmOmahaRequestParamsPolicyTest, RollbackDisabled) {
+ EXPECT_FALSE(TestRollbackAllowed(true, RollbackToTargetVersion::kDisabled));
+}
+
+TEST_F(UmOmahaRequestParamsPolicyTest, RollbackUnspecified) {
+ EXPECT_FALSE(
+ TestRollbackAllowed(true, RollbackToTargetVersion::kUnspecified));
+}
+
+TEST_F(UmOmahaRequestParamsPolicyTest, RollbackNotSet) {
+ EXPECT_FALSE(
+ TestRollbackAllowed(false, RollbackToTargetVersion::kUnspecified));
+}
+
} // namespace chromeos_update_manager
diff --git a/update_manager/real_device_policy_provider.cc b/update_manager/real_device_policy_provider.cc
index c08c0a6..ac71dc3 100644
--- a/update_manager/real_device_policy_provider.cc
+++ b/update_manager/real_device_policy_provider.cc
@@ -267,10 +267,6 @@
&RealDevicePolicyProvider::ConvertRollbackToTargetVersion);
UpdateVariable(&var_rollback_allowed_milestones_,
&DevicePolicy::GetRollbackAllowedMilestones);
- if (policy_provider_->IsConsumerDevice()) {
- // For consumer devices (which won't ever have policy), set value to 0.
- var_rollback_allowed_milestones_.SetValue(0);
- }
UpdateVariable(&var_scatter_factor_,
&RealDevicePolicyProvider::ConvertScatterFactor);
UpdateVariable(
diff --git a/update_manager/real_device_policy_provider_unittest.cc b/update_manager/real_device_policy_provider_unittest.cc
index 74810a9..fb61fad 100644
--- a/update_manager/real_device_policy_provider_unittest.cc
+++ b/update_manager/real_device_policy_provider_unittest.cc
@@ -241,44 +241,6 @@
provider_->var_rollback_to_target_version());
}
-TEST_F(UmRealDevicePolicyProviderTest, RollbackAllowedMilestonesOobe) {
- SetUpNonExistentDevicePolicy();
- EXPECT_CALL(mock_device_policy_, GetRollbackAllowedMilestones(_)).Times(0);
- ON_CALL(mock_policy_provider_, IsConsumerDevice())
- .WillByDefault(Return(false));
- EXPECT_TRUE(provider_->Init());
- loop_.RunOnce(false);
-
- UmTestUtils::ExpectVariableNotSet(
- provider_->var_rollback_allowed_milestones());
-}
-
-TEST_F(UmRealDevicePolicyProviderTest, RollbackAllowedMilestonesConsumer) {
- SetUpNonExistentDevicePolicy();
- EXPECT_CALL(mock_device_policy_, GetRollbackAllowedMilestones(_)).Times(0);
- ON_CALL(mock_policy_provider_, IsConsumerDevice())
- .WillByDefault(Return(true));
- EXPECT_TRUE(provider_->Init());
- loop_.RunOnce(false);
-
- UmTestUtils::ExpectVariableHasValue(
- 0, provider_->var_rollback_allowed_milestones());
-}
-
-TEST_F(UmRealDevicePolicyProviderTest,
- RollbackAllowedMilestonesEnterprisePolicySet) {
- SetUpExistentDevicePolicy();
- ON_CALL(mock_device_policy_, GetRollbackAllowedMilestones(_))
- .WillByDefault(DoAll(SetArgPointee<0>(2), Return(true)));
- ON_CALL(mock_policy_provider_, IsConsumerDevice())
- .WillByDefault(Return(false));
- EXPECT_TRUE(provider_->Init());
- loop_.RunOnce(false);
-
- UmTestUtils::ExpectVariableHasValue(
- 2, provider_->var_rollback_allowed_milestones());
-}
-
TEST_F(UmRealDevicePolicyProviderTest, ScatterFactorConverted) {
SetUpExistentDevicePolicy();
EXPECT_CALL(mock_device_policy_, GetScatterFactorInSeconds(_))
diff --git a/update_manager/update_check_allowed_policy.cc b/update_manager/update_check_allowed_policy.cc
index 992df5a..e74e43a 100644
--- a/update_manager/update_check_allowed_policy.cc
+++ b/update_manager/update_check_allowed_policy.cc
@@ -59,8 +59,6 @@
result->updates_enabled = true;
result->target_channel.clear();
result->target_version_prefix.clear();
- result->rollback_allowed = false;
- result->rollback_allowed_milestones = -1;
result->rollback_on_channel_downgrade = false;
result->interactive = false;
@@ -121,8 +119,6 @@
result->updates_enabled = true;
result->target_channel.clear();
result->target_version_prefix.clear();
- result->rollback_allowed = false;
- result->rollback_allowed_milestones = -1; // No version rolls should happen.
result->rollback_on_channel_downgrade = false;
result->interactive = false;
diff --git a/update_manager/update_check_allowed_policy_data.h b/update_manager/update_check_allowed_policy_data.h
index 061d4b4..e6dd001 100644
--- a/update_manager/update_check_allowed_policy_data.h
+++ b/update_manager/update_check_allowed_policy_data.h
@@ -34,15 +34,6 @@
//
// A target version prefix, if imposed by policy; otherwise, an empty string.
std::string target_version_prefix;
- // Specifies whether rollback images are allowed by device policy.
- bool rollback_allowed{false};
- // Specifies if rollbacks should attempt to preserve some system state.
- bool rollback_data_save_requested{false};
- // Specifies the number of Chrome milestones rollback should be allowed,
- // starting from the stable version at any time. Value is -1 if unspecified
- // (e.g. no device policy is available yet), in this case no version
- // roll-forward should happen.
- int rollback_allowed_milestones{0};
// Whether a rollback with data save should be initiated on channel
// downgrade (e.g. beta to stable).
bool rollback_on_channel_downgrade{false};
diff --git a/update_manager/update_check_allowed_policy_unittest.cc b/update_manager/update_check_allowed_policy_unittest.cc
index 0c040c2..3f2e45b 100644
--- a/update_manager/update_check_allowed_policy_unittest.cc
+++ b/update_manager/update_check_allowed_policy_unittest.cc
@@ -196,7 +196,6 @@
EXPECT_EQ(EvalStatus::kSucceeded, evaluator_->Evaluate());
EXPECT_TRUE(uca_data_->update_check_params.updates_enabled);
EXPECT_EQ("1.2", uca_data_->update_check_params.target_version_prefix);
- EXPECT_EQ(5, uca_data_->update_check_params.rollback_allowed_milestones);
EXPECT_EQ("foo-channel", uca_data_->update_check_params.target_channel);
EXPECT_FALSE(uca_data_->update_check_params.interactive);
}