Do not report NEED_PERMISSION_TO_UPDATE for non-critical OOBE updates
As part of https://crbug.com/889948, Chrome OS OOBE will start properly
handling NEED_PERMISSION_TO_UPDATE update status (instead of treating it
as any other error) - it will display a message to the user informing
them that the update can cause overage changes on mobile network, and
ask their permission to proceed.
Currently, NEED_PERMISSION_TO_UPDATE status is reported over cellular
for non-critical updates during OOBE, even though these are later
ignored by the update engine. This can produce sub-optimal UX where the
user is prompted to proceed with update, even though the update gets
skipped when attempted.
This CL makes sure that kNonCriticalUpdateInOOBE error status has
precedence over kOmahaUpdateIgnoredOverCellular in omaha_request_action
(the latter one causes NEED_PERMISSION_TO_UPDATE status)
BUG=chromium:889948
TEST=run OOBE with update engine thinking it's on cellular network,
verify that the user sees prompt about possible overage
charges for critical updates only
(dut setup included patching update engine to treat ethernet connection
as cellular, and running devserver locally with and without
--critical_update flag)
Change-Id: If87b9477fd7f826892b1aeedc1017d11b1a0af32
Reviewed-on: https://chromium-review.googlesource.com/1277708
Commit-Ready: Toni Baržić <tbarzic@chromium.org>
Tested-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 2edf8c0..de02b5e 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -1264,15 +1264,16 @@
output_object.update_exists = true;
SetOutputObject(output_object);
+ LoadOrPersistUpdateFirstSeenAtPref();
+
ErrorCode error = ErrorCode::kSuccess;
- if (ShouldIgnoreUpdate(&error, output_object)) {
+ if (ShouldIgnoreUpdate(output_object, &error)) {
// No need to change output_object.update_exists here, since the value
// has been output to the pipe.
completer.set_code(error);
return;
}
- LoadOrPersistUpdateFirstSeenAtPref();
// If Omaha says to disable p2p, respect that
if (output_object.disable_p2p_for_downloading) {
@@ -1326,17 +1327,6 @@
OmahaResponse& output_object = const_cast<OmahaResponse&>(GetOutputObject());
PayloadStateInterface* payload_state = system_state_->payload_state();
- if (system_state_->hardware()->IsOOBEEnabled() &&
- !system_state_->hardware()->IsOOBEComplete(nullptr) &&
- (output_object.deadline.empty() ||
- payload_state->GetRollbackHappened()) &&
- params_->app_version() != "ForcedUpdate") {
- output_object.update_exists = false;
- LOG(INFO) << "Ignoring non-critical Omaha updates until OOBE is done.";
- completer.set_code(ErrorCode::kNonCriticalUpdateInOOBE);
- return;
- }
-
if (ShouldDeferDownload(&output_object)) {
output_object.update_exists = false;
LOG(INFO) << "Ignoring Omaha updates as updates are deferred by policy.";
@@ -1734,8 +1724,8 @@
system_state_, result, reaction, download_error_code);
}
-bool OmahaRequestAction::ShouldIgnoreUpdate(
- ErrorCode* error, const OmahaResponse& response) const {
+bool OmahaRequestAction::ShouldIgnoreUpdate(const OmahaResponse& response,
+ ErrorCode* error) const {
// Note: policy decision to not update to a version we rolled back from.
string rollback_version =
system_state_->payload_state()->GetRollbackVersion();
@@ -1748,6 +1738,16 @@
}
}
+ if (system_state_->hardware()->IsOOBEEnabled() &&
+ !system_state_->hardware()->IsOOBEComplete(nullptr) &&
+ (response.deadline.empty() ||
+ system_state_->payload_state()->GetRollbackHappened()) &&
+ params_->app_version() != "ForcedUpdate") {
+ LOG(INFO) << "Ignoring a non-critical Omaha update before OOBE completion.";
+ *error = ErrorCode::kNonCriticalUpdateInOOBE;
+ return true;
+ }
+
if (!IsUpdateAllowedOverCurrentConnection(error, response)) {
LOG(INFO) << "Update is not allowed over current connection.";
return true;