Add functions to allow update over cellular (including tethered connection)
- Add an update state NEED_PERMISSION_TO_UPDATE which is broadcasted along
with the update info (version and size) when |OmahaRequestAction| aborts
update due to cellular connection. So the state transition will be:
IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT
->IDLE
(The Chrome UI prompts an alert window showing update size and asks user
whether to proceed upon receiving this state.)
- Add a dbus interface to set update over cellular target
(kPrefsUpdateOverCellularTargetVersion and kPrefsUpdateOverCellularTargetSize).
The target is the one received by Chrome UI in NEED_PERMISSION_TO_UPDATE
broadcast. By sending the target back with the dbus call, update engine can
double check the target with the server to make sure there's no new server
push after NEED_PERMISSION_TO_UPDATE is broadcasted to Chrome UI.
(This dbus call is invoked when the user chooses to proceed to update at the
alert window. The dbus call is followed by another dbus call |AttemptUpdate|)
- So, the the decision tree as to whether to allow update over cellular
connection has changed to:
IF (device policy DeviceUpdateAllowedConnectionTypes set)
follow device policy's decision
ELSE IF (kPrefsUpdateOverCellularPermission set to true)
allow update
ELSE IF (Either kPrefsUpdateOverCellularTargetVersion or
kPrefsUpdateOverCellularTargetSize is not set, or they are set but do not
match the version and size in |OmahaResponse| retrieved by
|OmahaRequestAction|)
disallow update, and broadcast NEED_PERMISSION_TO_UPDATE
ELSE
allow update
ENDIF
- This decision making happens at |OmahaRequestAction| after |OmahaResponse| is
retrieved. Since we want to separate the device policy check with the user
preferences check which depends on |OmahaResponse| during checking for update,
we modify ConnectionManager::IsUpdateAllowedOver by moving the user preferences
check to |OmahaRequestAction|. Thus, the function by default returns true for
cellular connection if device policy is not set.
- Corner case:
Adding kPrefsUpdateOverCellularPermission and
kPrefsUpdateOverCellularTargetSize seems to complicate the logic here. But
they could effectively solve a corner case where the target does not match
|OmahaResponse| due to new server push after broadcasting
NEED_PERMISSION_TO_UPDATE. In that case, we simply broadcast
NEED_PERMISSION_TO_UPDATE again along with new update info.
CQ-DEPEND=CL:481102
BUG=chromium:691108
TEST='FEATURES=test emerge-link update_engine'
Change-Id: I91b07e6030f202f7d0d48fb50787115695f228e3
Reviewed-on: https://chromium-review.googlesource.com/479467
Commit-Ready: Weidong Guo <weidongg@chromium.org>
Tested-by: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
diff --git a/UpdateEngine.conf b/UpdateEngine.conf
index 58cca09..192e6ab 100644
--- a/UpdateEngine.conf
+++ b/UpdateEngine.conf
@@ -53,6 +53,9 @@
send_member="SetUpdateOverCellularPermission"/>
<allow send_destination="org.chromium.UpdateEngine"
send_interface="org.chromium.UpdateEngineInterface"
+ send_member="SetUpdateOverCellularTarget"/>
+ <allow send_destination="org.chromium.UpdateEngine"
+ send_interface="org.chromium.UpdateEngineInterface"
send_member="GetUpdateOverCellularPermission"/>
<allow send_destination="org.chromium.UpdateEngine"
send_interface="org.chromium.UpdateEngineInterface"
diff --git a/binder_bindings/android/brillo/IUpdateEngine.aidl b/binder_bindings/android/brillo/IUpdateEngine.aidl
index b1a1b4f..7893548 100644
--- a/binder_bindings/android/brillo/IUpdateEngine.aidl
+++ b/binder_bindings/android/brillo/IUpdateEngine.aidl
@@ -33,6 +33,8 @@
void SetP2PUpdatePermission(in boolean enabled);
boolean GetP2PUpdatePermission();
void SetUpdateOverCellularPermission(in boolean enabled);
+ void SetUpdateOverCellularTarget(in String target_version,
+ in long target_size);
boolean GetUpdateOverCellularPermission();
long GetDurationSinceUpdate();
String GetPrevVersion();
diff --git a/client_library/include/update_engine/update_status.h b/client_library/include/update_engine/update_status.h
index 3e9af5b..2f6322a 100644
--- a/client_library/include/update_engine/update_status.h
+++ b/client_library/include/update_engine/update_status.h
@@ -23,6 +23,9 @@
IDLE = 0,
CHECKING_FOR_UPDATE,
UPDATE_AVAILABLE,
+ // Broadcast this state when an update aborts because user preferences does
+ // not allow update over cellular.
+ NEED_PERMISSION_TO_UPDATE,
DOWNLOADING,
VERIFYING,
FINALIZING,
diff --git a/common/constants.cc b/common/constants.cc
index c0bb874..1bb4058 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -79,6 +79,10 @@
const char kPrefsUpdateFirstSeenAt[] = "update-first-seen-at";
const char kPrefsUpdateOverCellularPermission[] =
"update-over-cellular-permission";
+const char kPrefsUpdateOverCellularTargetVersion[] =
+ "update-over-cellular-target-version";
+const char kPrefsUpdateOverCellularTargetSize[] =
+ "update-over-cellular-target-size";
const char kPrefsUpdateServerCertificate[] = "update-server-cert";
const char kPrefsUpdateStateNextDataLength[] = "update-state-next-data-length";
const char kPrefsUpdateStateNextDataOffset[] = "update-state-next-data-offset";
diff --git a/common/constants.h b/common/constants.h
index 660d9a9..208755d 100644
--- a/common/constants.h
+++ b/common/constants.h
@@ -82,6 +82,8 @@
extern const char kPrefsUpdateDurationUptime[];
extern const char kPrefsUpdateFirstSeenAt[];
extern const char kPrefsUpdateOverCellularPermission[];
+extern const char kPrefsUpdateOverCellularTargetVersion[];
+extern const char kPrefsUpdateOverCellularTargetSize[];
extern const char kPrefsUpdateServerCertificate[];
extern const char kPrefsUpdateStateNextDataLength[];
extern const char kPrefsUpdateStateNextDataOffset[];
diff --git a/common/error_code.h b/common/error_code.h
index e08ec46..bcf541c 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -73,6 +73,7 @@
kFilesystemVerifierError = 47,
kUserCanceled = 48,
kNonCriticalUpdateInOOBE = 49,
+ kOmahaUpdateIgnoredOverCellular = 50,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index ad4aeeb..0169ab8 100644
--- a/common/error_code_utils.cc
+++ b/common/error_code_utils.cc
@@ -144,6 +144,8 @@
return "ErrorCode::kUserCanceled";
case ErrorCode::kNonCriticalUpdateInOOBE:
return "ErrorCode::kNonCriticalUpdateInOOBE";
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
+ return "ErrorCode::kOmahaUpdateIgnoredOverCellular";
// Don't add a default case to let the compiler warn about newly added
// error codes which should be added here.
}
diff --git a/common_service.cc b/common_service.cc
index 20f55ab..b8bc298 100644
--- a/common_service.cc
+++ b/common_service.cc
@@ -16,7 +16,6 @@
#include "update_engine/common_service.h"
-#include <set>
#include <string>
#include <base/location.h>
@@ -41,7 +40,6 @@
using base::StringPrintf;
using brillo::ErrorPtr;
using brillo::string_utils::ToString;
-using std::set;
using std::string;
namespace chromeos_update_engine {
@@ -250,24 +248,12 @@
bool UpdateEngineService::SetUpdateOverCellularPermission(ErrorPtr* error,
bool in_allowed) {
- set<string> allowed_types;
- const policy::DevicePolicy* device_policy = system_state_->device_policy();
-
- // The device_policy is loaded in a lazy way before an update check. Load it
- // now from the libbrillo cache if it wasn't already loaded.
- if (!device_policy) {
- UpdateAttempter* update_attempter = system_state_->update_attempter();
- if (update_attempter) {
- update_attempter->RefreshDevicePolicy();
- device_policy = system_state_->device_policy();
- }
- }
+ ConnectionManagerInterface* connection_manager =
+ system_state_->connection_manager();
// Check if this setting is allowed by the device policy.
- if (device_policy &&
- device_policy->GetAllowedConnectionTypesForUpdate(&allowed_types)) {
- LogAndSetError(error,
- FROM_HERE,
+ if (connection_manager->IsAllowedConnectionTypesForUpdateSet()) {
+ LogAndSetError(error, FROM_HERE,
"Ignoring the update over cellular setting since there's "
"a device policy enforcing this setting.");
return false;
@@ -278,9 +264,9 @@
PrefsInterface* prefs = system_state_->prefs();
- if (!prefs->SetBoolean(kPrefsUpdateOverCellularPermission, in_allowed)) {
- LogAndSetError(error,
- FROM_HERE,
+ if (!prefs ||
+ !prefs->SetBoolean(kPrefsUpdateOverCellularPermission, in_allowed)) {
+ LogAndSetError(error, FROM_HERE,
string("Error setting the update over cellular to ") +
(in_allowed ? "true" : "false"));
return false;
@@ -288,25 +274,63 @@
return true;
}
-bool UpdateEngineService::GetUpdateOverCellularPermission(ErrorPtr* /* error */,
- bool* out_allowed) {
- ConnectionManagerInterface* cm = system_state_->connection_manager();
+bool UpdateEngineService::SetUpdateOverCellularTarget(
+ brillo::ErrorPtr* error, const std::string &target_version,
+ int64_t target_size) {
+ ConnectionManagerInterface* connection_manager =
+ system_state_->connection_manager();
- // The device_policy is loaded in a lazy way before an update check and is
- // used to determine if an update is allowed over cellular. Load the device
- // policy now from the libbrillo cache if it wasn't already loaded.
- if (!system_state_->device_policy()) {
- UpdateAttempter* update_attempter = system_state_->update_attempter();
- if (update_attempter)
- update_attempter->RefreshDevicePolicy();
+ // Check if this setting is allowed by the device policy.
+ if (connection_manager->IsAllowedConnectionTypesForUpdateSet()) {
+ LogAndSetError(error, FROM_HERE,
+ "Ignoring the update over cellular setting since there's "
+ "a device policy enforcing this setting.");
+ return false;
}
- // Return the current setting based on the same logic used while checking for
- // updates. A log message could be printed as the result of this test.
- LOG(INFO) << "Checking if updates over cellular networks are allowed:";
- *out_allowed = cm->IsUpdateAllowedOver(
- chromeos_update_engine::NetworkConnectionType::kCellular,
- chromeos_update_engine::NetworkTethering::kUnknown);
+ // If the policy wasn't loaded yet, then it is still OK to change the local
+ // setting because the policy will be checked again during the update check.
+
+ PrefsInterface* prefs = system_state_->prefs();
+
+ if (!prefs ||
+ !prefs->SetString(kPrefsUpdateOverCellularTargetVersion,
+ target_version) ||
+ !prefs->SetInt64(kPrefsUpdateOverCellularTargetSize, target_size)) {
+ LogAndSetError(error, FROM_HERE,
+ "Error setting the target for update over cellular.");
+ return false;
+ }
+ return true;
+}
+
+bool UpdateEngineService::GetUpdateOverCellularPermission(ErrorPtr* error,
+ bool* out_allowed) {
+ ConnectionManagerInterface* connection_manager =
+ system_state_->connection_manager();
+
+ if (connection_manager->IsAllowedConnectionTypesForUpdateSet()) {
+ // We have device policy, so ignore the user preferences.
+ *out_allowed = connection_manager->IsUpdateAllowedOver(
+ NetworkConnectionType::kCellular, NetworkTethering::kUnknown);
+ } else {
+ PrefsInterface* prefs = system_state_->prefs();
+
+ if (!prefs || !prefs->Exists(kPrefsUpdateOverCellularPermission)) {
+ // Update is not allowed as user preference is not set or not available.
+ *out_allowed = false;
+ return true;
+ }
+
+ bool is_allowed;
+
+ if (!prefs->GetBoolean(kPrefsUpdateOverCellularPermission, &is_allowed)) {
+ LogAndSetError(error, FROM_HERE,
+ "Error getting the update over cellular preference.");
+ return false;
+ }
+ *out_allowed = is_allowed;
+ }
return true;
}
diff --git a/common_service.h b/common_service.h
index 69368fb..4320d09 100644
--- a/common_service.h
+++ b/common_service.h
@@ -115,6 +115,12 @@
bool SetUpdateOverCellularPermission(brillo::ErrorPtr* error,
bool in_allowed);
+ // If there's no device policy installed, sets the update over cellular
+ // target. Otherwise, this method returns with an error.
+ bool SetUpdateOverCellularTarget(brillo::ErrorPtr* error,
+ const std::string& target_version,
+ int64_t target_size);
+
// Returns the current value of the update over cellular network setting,
// either forced by the device policy if the device is enrolled or the current
// user preference otherwise.
diff --git a/connection_manager.cc b/connection_manager.cc
index 778cba5..47bd776 100644
--- a/connection_manager.cc
+++ b/connection_manager.cc
@@ -28,6 +28,7 @@
#include "update_engine/common/prefs.h"
#include "update_engine/common/utils.h"
#include "update_engine/system_state.h"
+#include "update_engine/update_attempter.h"
using org::chromium::flimflam::ManagerProxyInterface;
using org::chromium::flimflam::ServiceProxyInterface;
@@ -79,15 +80,23 @@
case NetworkConnectionType::kCellular: {
set<string> allowed_types;
+
const policy::DevicePolicy* device_policy =
system_state_->device_policy();
- // A device_policy is loaded in a lazy way right before an update check,
- // so the device_policy should be already loaded at this point. If it's
- // not, return a safe value for this setting.
+ // The device_policy is loaded in a lazy way before an update check. Load
+ // it now from the libbrillo cache if it wasn't already loaded.
if (!device_policy) {
- LOG(INFO) << "Disabling updates over cellular networks as there's no "
- "device policy loaded yet.";
+ UpdateAttempter* update_attempter = system_state_->update_attempter();
+ if (update_attempter) {
+ update_attempter->RefreshDevicePolicy();
+ device_policy = system_state_->device_policy();
+ }
+ }
+
+ if (!device_policy) {
+ LOG(INFO) << "Disabling updates over cellular as device policy "
+ "fails to be loaded.";
return false;
}
@@ -95,38 +104,17 @@
// The update setting is enforced by the device policy.
if (!ContainsKey(allowed_types, shill::kTypeCellular)) {
- LOG(INFO) << "Disabling updates over cellular connection as it's not "
- "allowed in the device policy.";
+ LOG(INFO) << "Disabling updates over cellular per device policy.";
return false;
}
LOG(INFO) << "Allowing updates over cellular per device policy.";
- return true;
- } else {
- // There's no update setting in the device policy, using the local user
- // setting.
- PrefsInterface* prefs = system_state_->prefs();
-
- if (!prefs || !prefs->Exists(kPrefsUpdateOverCellularPermission)) {
- LOG(INFO) << "Disabling updates over cellular connection as there's "
- "no device policy setting nor user preference present.";
- return false;
- }
-
- bool stored_value;
- if (!prefs->GetBoolean(kPrefsUpdateOverCellularPermission,
- &stored_value)) {
- return false;
- }
-
- if (!stored_value) {
- LOG(INFO) << "Disabling updates over cellular connection per user "
- "setting.";
- return false;
- }
- LOG(INFO) << "Allowing updates over cellular per user setting.";
- return true;
}
+
+ // If there's no update setting in the device policy, we do not check
+ // the local user setting here, which should be checked by
+ // |OmahaRequestAction| during checking for update.
+ return true;
}
default:
@@ -141,6 +129,22 @@
}
}
+bool ConnectionManager::IsAllowedConnectionTypesForUpdateSet() const {
+ const policy::DevicePolicy* device_policy = system_state_->device_policy();
+ if (!device_policy) {
+ LOG(INFO) << "There's no device policy loaded yet.";
+ return false;
+ }
+
+ set<string> allowed_types;
+ if (!device_policy->GetAllowedConnectionTypesForUpdate(
+ &allowed_types)) {
+ return false;
+ }
+
+ return true;
+}
+
// static
const char* ConnectionManager::StringForConnectionType(
NetworkConnectionType type) {
diff --git a/connection_manager.h b/connection_manager.h
index 2057f3b..6f5fab4 100644
--- a/connection_manager.h
+++ b/connection_manager.h
@@ -49,6 +49,7 @@
NetworkTethering* out_tethering) override;
bool IsUpdateAllowedOver(NetworkConnectionType type,
NetworkTethering tethering) const override;
+ bool IsAllowedConnectionTypesForUpdateSet() const override;
private:
// Returns (via out_path) the default network path, or empty string if
diff --git a/connection_manager_interface.h b/connection_manager_interface.h
index cb60a3c..2181b5d 100644
--- a/connection_manager_interface.h
+++ b/connection_manager_interface.h
@@ -56,6 +56,10 @@
virtual bool IsUpdateAllowedOver(NetworkConnectionType type,
NetworkTethering tethering) const = 0;
+ // Returns true if the allowed connection types for update is set in the
+ // device policy. Otherwise, returns false.
+ virtual bool IsAllowedConnectionTypesForUpdateSet() const = 0;
+
protected:
ConnectionManagerInterface() = default;
diff --git a/connection_manager_unittest.cc b/connection_manager_unittest.cc
index 612929b..3e810a7 100644
--- a/connection_manager_unittest.cc
+++ b/connection_manager_unittest.cc
@@ -283,15 +283,23 @@
NetworkTethering::kConfirmed));
}
-TEST_F(ConnectionManagerTest, BlockUpdatesOverCellularByDefaultTest) {
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
+TEST_F(ConnectionManagerTest, AllowUpdatesOverCellularByDefaultTest) {
+ policy::MockDevicePolicy device_policy;
+ // Set an empty device policy.
+ fake_system_state_.set_device_policy(&device_policy);
+
+ EXPECT_TRUE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
NetworkTethering::kUnknown));
}
-TEST_F(ConnectionManagerTest, BlockUpdatesOverTetheredNetworkByDefaultTest) {
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kWifi,
+TEST_F(ConnectionManagerTest, AllowUpdatesOverTetheredNetworkByDefaultTest) {
+ policy::MockDevicePolicy device_policy;
+ // Set an empty device policy.
+ fake_system_state_.set_device_policy(&device_policy);
+
+ EXPECT_TRUE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kWifi,
NetworkTethering::kConfirmed));
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kEthernet,
+ EXPECT_TRUE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kEthernet,
NetworkTethering::kConfirmed));
EXPECT_TRUE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kWifi,
NetworkTethering::kSuspected));
@@ -320,62 +328,19 @@
NetworkTethering::kUnknown));
}
-TEST_F(ConnectionManagerTest, BlockUpdatesOver3GIfErrorInPolicyFetchTest) {
- policy::MockDevicePolicy allow_3g_policy;
+TEST_F(ConnectionManagerTest, AllowUpdatesOver3GIfPolicyIsNotSet) {
+ policy::MockDevicePolicy device_policy;
- fake_system_state_.set_device_policy(&allow_3g_policy);
-
- set<string> allowed_set;
- allowed_set.insert(
- cmut_.StringForConnectionType(NetworkConnectionType::kCellular));
+ fake_system_state_.set_device_policy(&device_policy);
// Return false for GetAllowedConnectionTypesForUpdate and see
- // that updates are still blocked for 3G despite the value being in
- // the string set above.
- EXPECT_CALL(allow_3g_policy, GetAllowedConnectionTypesForUpdate(_))
- .Times(1)
- .WillOnce(DoAll(SetArgPointee<0>(allowed_set), Return(false)));
-
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
- NetworkTethering::kUnknown));
-}
-
-TEST_F(ConnectionManagerTest, UseUserPrefForUpdatesOverCellularIfNoPolicyTest) {
- policy::MockDevicePolicy no_policy;
- testing::NiceMock<MockPrefs>* prefs = fake_system_state_.mock_prefs();
-
- fake_system_state_.set_device_policy(&no_policy);
-
- // No setting enforced by the device policy, user prefs should be used.
- EXPECT_CALL(no_policy, GetAllowedConnectionTypesForUpdate(_))
- .Times(3)
- .WillRepeatedly(Return(false));
-
- // No user pref: block.
- EXPECT_CALL(*prefs, Exists(kPrefsUpdateOverCellularPermission))
+ // that updates are allowed as device policy is not set. Further
+ // check is left to |OmahaRequestAction|.
+ EXPECT_CALL(device_policy, GetAllowedConnectionTypesForUpdate(_))
.Times(1)
.WillOnce(Return(false));
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
- NetworkTethering::kUnknown));
- // Allow per user pref.
- EXPECT_CALL(*prefs, Exists(kPrefsUpdateOverCellularPermission))
- .Times(1)
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs, GetBoolean(kPrefsUpdateOverCellularPermission, _))
- .Times(1)
- .WillOnce(DoAll(SetArgPointee<1>(true), Return(true)));
EXPECT_TRUE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
- NetworkTethering::kUnknown));
-
- // Block per user pref.
- EXPECT_CALL(*prefs, Exists(kPrefsUpdateOverCellularPermission))
- .Times(1)
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs, GetBoolean(kPrefsUpdateOverCellularPermission, _))
- .Times(1)
- .WillOnce(DoAll(SetArgPointee<1>(false), Return(true)));
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(NetworkConnectionType::kCellular,
NetworkTethering::kUnknown));
}
diff --git a/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml b/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
index 848f775..a20f33f 100644
--- a/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
+++ b/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml
@@ -67,6 +67,10 @@
<method name="SetUpdateOverCellularPermission">
<arg type="b" name="allowed" direction="in" />
</method>
+ <method name="SetUpdateOverCellularTarget">
+ <arg type="s" name="target_version" direction="in" />
+ <arg type="x" name="target_size" direction="in" />
+ </method>
<method name="GetUpdateOverCellularPermission">
<arg type="b" name="allowed" direction="out" />
</method>
diff --git a/dbus_service.cc b/dbus_service.cc
index 18b5b9e..e64481b 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -122,6 +122,14 @@
return common_->SetUpdateOverCellularPermission(error, in_allowed);
}
+bool DBusUpdateEngineService::SetUpdateOverCellularTarget(
+ brillo::ErrorPtr* error,
+ const std::string& target_version,
+ int64_t target_size) {
+ return common_->SetUpdateOverCellularTarget(error, target_version,
+ target_size);
+}
+
bool DBusUpdateEngineService::GetUpdateOverCellularPermission(
ErrorPtr* error, bool* out_allowed) {
return common_->GetUpdateOverCellularPermission(error, out_allowed);
diff --git a/dbus_service.h b/dbus_service.h
index 0b63492..efad821 100644
--- a/dbus_service.h
+++ b/dbus_service.h
@@ -114,6 +114,12 @@
bool SetUpdateOverCellularPermission(brillo::ErrorPtr* error,
bool in_allowed) override;
+ // If there's no device policy installed, sets the update over cellular
+ // target. Otherwise, this method returns with an error.
+ bool SetUpdateOverCellularTarget(brillo::ErrorPtr* error,
+ const std::string& target_version,
+ int64_t target_size) override;
+
// Returns the current value of the update over cellular network setting,
// either forced by the device policy if the device is enrolled or the current
// user preference otherwise.
diff --git a/metrics_utils.cc b/metrics_utils.cc
index e165e89..4f26758 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -109,6 +109,7 @@
case ErrorCode::kPostinstallPowerwashError:
case ErrorCode::kUpdateCanceledByChannelChange:
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
return metrics::AttemptResult::kInternalError;
// Special flags. These can't happen (we mask them out above) but
@@ -209,6 +210,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
break;
// Special flags. These can't happen (we mask them out above) but
diff --git a/mock_connection_manager.h b/mock_connection_manager.h
index 109c529..c0745b7 100644
--- a/mock_connection_manager.h
+++ b/mock_connection_manager.h
@@ -36,6 +36,7 @@
MOCK_CONST_METHOD2(IsUpdateAllowedOver, bool(NetworkConnectionType type,
NetworkTethering tethering));
+ MOCK_CONST_METHOD0(IsAllowedConnectionTypesForUpdateSet, bool());
};
} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 173d387..fb258c2 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -980,9 +980,11 @@
output_object.update_exists = true;
SetOutputObject(output_object);
- if (ShouldIgnoreUpdate(output_object)) {
- output_object.update_exists = false;
- completer.set_code(ErrorCode::kOmahaUpdateIgnoredPerPolicy);
+ ErrorCode error = ErrorCode::kSuccess;
+ if (ShouldIgnoreUpdate(&error, output_object)) {
+ // No need to change output_object.update_exists here, since the value
+ // has been output to the pipe.
+ completer.set_code(error);
return;
}
@@ -1439,6 +1441,7 @@
break;
case ErrorCode::kOmahaUpdateIgnoredPerPolicy:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
result = metrics::CheckResult::kUpdateAvailable;
reaction = metrics::CheckReaction::kIgnored;
break;
@@ -1473,7 +1476,7 @@
}
bool OmahaRequestAction::ShouldIgnoreUpdate(
- const OmahaResponse& response) const {
+ ErrorCode* error, const OmahaResponse& response) const {
// Note: policy decision to not update to a version we rolled back from.
string rollback_version =
system_state_->payload_state()->GetRollbackVersion();
@@ -1481,11 +1484,12 @@
LOG(INFO) << "Detected previous rollback from version " << rollback_version;
if (rollback_version == response.version) {
LOG(INFO) << "Received version that we rolled back from. Ignoring.";
+ *error = ErrorCode::kOmahaUpdateIgnoredPerPolicy;
return true;
}
}
- if (!IsUpdateAllowedOverCurrentConnection()) {
+ if (!IsUpdateAllowedOverCurrentConnection(error, response)) {
LOG(INFO) << "Update is not allowed over current connection.";
return true;
}
@@ -1500,7 +1504,59 @@
return false;
}
-bool OmahaRequestAction::IsUpdateAllowedOverCurrentConnection() const {
+bool OmahaRequestAction::IsUpdateAllowedOverCellularByPrefs(
+ const OmahaResponse& response) const {
+ PrefsInterface* prefs = system_state_->prefs();
+
+ if (!prefs) {
+ LOG(INFO) << "Disabling updates over cellular as the preferences are "
+ "not available.";
+ return false;
+ }
+
+ bool is_allowed;
+
+ if (prefs->Exists(kPrefsUpdateOverCellularPermission) &&
+ prefs->GetBoolean(kPrefsUpdateOverCellularPermission, &is_allowed) &&
+ is_allowed) {
+ LOG(INFO) << "Allowing updates over cellular as permission preference is "
+ "set to true.";
+ return true;
+ }
+
+ if (!prefs->Exists(kPrefsUpdateOverCellularTargetVersion) ||
+ !prefs->Exists(kPrefsUpdateOverCellularTargetSize)) {
+ LOG(INFO) << "Disabling updates over cellular as permission preference is "
+ "set to false or does not exist while target does not exist.";
+ return false;
+ }
+
+ std::string target_version;
+ int64_t target_size;
+
+ if (!prefs->GetString(kPrefsUpdateOverCellularTargetVersion,
+ &target_version) ||
+ !prefs->GetInt64(kPrefsUpdateOverCellularTargetSize,
+ &target_size)) {
+ LOG(INFO) << "Disabling updates over cellular as the target version or "
+ "size is not accessible.";
+ return false;
+ }
+
+ if (target_version == response.version && target_size == response.size) {
+ LOG(INFO) << "Allowing updates over cellular as the target matches the"
+ "omaha response.";
+ return true;
+ } else {
+ LOG(INFO) << "Disabling updates over cellular as the target does not"
+ "match the omaha response.";
+ return false;
+ }
+}
+
+bool OmahaRequestAction::IsUpdateAllowedOverCurrentConnection(
+ ErrorCode* error,
+ const OmahaResponse& response) const {
NetworkConnectionType type;
NetworkTethering tethering;
ConnectionManagerInterface* connection_manager =
@@ -1510,7 +1566,30 @@
<< "Defaulting to allow updates.";
return true;
}
+
bool is_allowed = connection_manager->IsUpdateAllowedOver(type, tethering);
+ bool is_device_policy_set = connection_manager->
+ IsAllowedConnectionTypesForUpdateSet();
+ // Treats tethered connection as if it is cellular connection.
+ bool is_over_cellular = type == NetworkConnectionType::kCellular ||
+ tethering == NetworkTethering::kConfirmed;
+
+ if (!is_over_cellular) {
+ // There's no need to further check user preferences as we are not over
+ // cellular connection.
+ if (!is_allowed)
+ *error = ErrorCode::kOmahaUpdateIgnoredPerPolicy;
+ } else if (is_device_policy_set) {
+ // There's no need to further check user preferences as the device policy
+ // is set regarding updates over cellular.
+ if (!is_allowed)
+ *error = ErrorCode::kOmahaUpdateIgnoredPerPolicy;
+ } else if (!IsUpdateAllowedOverCellularByPrefs(response)) {
+ // The user prefereces does not allow updates over cellular.
+ is_allowed = false;
+ *error = ErrorCode::kOmahaUpdateIgnoredOverCellular;
+ }
+
LOG(INFO) << "We are connected via "
<< ConnectionManager::StringForConnectionType(type)
<< ", Updates allowed: " << (is_allowed ? "Yes" : "No");
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 2915a6a..7d35c23 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -299,11 +299,17 @@
void OnLookupPayloadViaP2PCompleted(const std::string& url);
// Returns true if the current update should be ignored.
- bool ShouldIgnoreUpdate(const OmahaResponse& response) const;
+ bool ShouldIgnoreUpdate(ErrorCode* error,
+ const OmahaResponse& response) const;
+
+ // Return true if updates are allowed by user preferences.
+ bool IsUpdateAllowedOverCellularByPrefs(const OmahaResponse& response) const;
// Returns true if updates are allowed over the current type of connection.
// False otherwise.
- bool IsUpdateAllowedOverCurrentConnection() const;
+ bool IsUpdateAllowedOverCurrentConnection(
+ ErrorCode* error,
+ const OmahaResponse& response) const;
// Global system context.
SystemState* system_state_;
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index d4b166f..6c24f86 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -487,6 +487,185 @@
EXPECT_FALSE(response.update_exists);
}
+TEST_F(OmahaRequestActionTest, ValidUpdateOverCellularAllowedByDevicePolicy) {
+ // This test tests that update over cellular is allowed as device policy
+ // says yes.
+ OmahaResponse response;
+ MockConnectionManager mock_cm;
+
+ fake_system_state_.set_connection_manager(&mock_cm);
+
+ EXPECT_CALL(mock_cm, GetConnectionProperties(_, _))
+ .WillRepeatedly(
+ DoAll(SetArgumentPointee<0>(NetworkConnectionType::kCellular),
+ SetArgumentPointee<1>(NetworkTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(NetworkConnectionType::kCellular, _))
+ .WillRepeatedly(Return(true));
+
+ ASSERT_TRUE(
+ TestUpdateCheck(nullptr, // request_params
+ fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kSuccess,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kUpdating,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_TRUE(response.update_exists);
+}
+
+TEST_F(OmahaRequestActionTest, ValidUpdateOverCellularBlockedByDevicePolicy) {
+ // This test tests that update over cellular is blocked as device policy
+ // says no.
+ OmahaResponse response;
+ MockConnectionManager mock_cm;
+
+ fake_system_state_.set_connection_manager(&mock_cm);
+
+ EXPECT_CALL(mock_cm, GetConnectionProperties(_, _))
+ .WillRepeatedly(
+ DoAll(SetArgumentPointee<0>(NetworkConnectionType::kCellular),
+ SetArgumentPointee<1>(NetworkTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(NetworkConnectionType::kCellular, _))
+ .WillRepeatedly(Return(false));
+
+ ASSERT_FALSE(
+ TestUpdateCheck(nullptr, // request_params
+ fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kOmahaUpdateIgnoredPerPolicy,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kIgnored,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_FALSE(response.update_exists);
+}
+
+TEST_F(OmahaRequestActionTest,
+ ValidUpdateOverCellularAllowedByUserPermissionTrue) {
+ // This test tests that, when device policy is not set, update over cellular
+ // is allowed as permission for update over cellular is set to true.
+ OmahaResponse response;
+ MockConnectionManager mock_cm;
+
+ fake_prefs_.SetBoolean(kPrefsUpdateOverCellularPermission, true);
+ fake_system_state_.set_connection_manager(&mock_cm);
+
+ EXPECT_CALL(mock_cm, GetConnectionProperties(_, _))
+ .WillRepeatedly(
+ DoAll(SetArgumentPointee<0>(NetworkConnectionType::kCellular),
+ SetArgumentPointee<1>(NetworkTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(NetworkConnectionType::kCellular, _))
+ .WillRepeatedly(Return(true));
+
+ ASSERT_TRUE(
+ TestUpdateCheck(nullptr, // request_params
+ fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kSuccess,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kUpdating,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_TRUE(response.update_exists);
+}
+
+TEST_F(OmahaRequestActionTest,
+ ValidUpdateOverCellularBlockedByUpdateTargetNotMatch) {
+ // This test tests that, when device policy is not set and permission for
+ // update over cellular is set to false or does not exist, update over
+ // cellular is blocked as update target does not match the omaha response.
+ OmahaResponse response;
+ MockConnectionManager mock_cm;
+ // A version different from the version in omaha response.
+ string diff_version = "99.99.99";
+ // A size different from the size in omaha response.
+ int64_t diff_size = 999;
+
+ fake_prefs_.SetString(kPrefsUpdateOverCellularTargetVersion, diff_version);
+ fake_prefs_.SetInt64(kPrefsUpdateOverCellularTargetSize, diff_size);
+ // This test tests cellular (3G) being the only connection type being allowed.
+ fake_system_state_.set_connection_manager(&mock_cm);
+
+ EXPECT_CALL(mock_cm, GetConnectionProperties(_, _))
+ .WillRepeatedly(
+ DoAll(SetArgumentPointee<0>(NetworkConnectionType::kCellular),
+ SetArgumentPointee<1>(NetworkTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(NetworkConnectionType::kCellular, _))
+ .WillRepeatedly(Return(true));
+
+ ASSERT_FALSE(
+ TestUpdateCheck(nullptr, // request_params
+ fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kOmahaUpdateIgnoredOverCellular,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kIgnored,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_FALSE(response.update_exists);
+}
+
+TEST_F(OmahaRequestActionTest,
+ ValidUpdateOverCellularAllowedByUpdateTargetMatch) {
+ // This test tests that, when device policy is not set and permission for
+ // update over cellular is set to false or does not exist, update over
+ // cellular is allowed as update target matches the omaha response.
+ OmahaResponse response;
+ MockConnectionManager mock_cm;
+ // A version same as the version in omaha response.
+ string new_version = fake_update_response_.version;
+ // A size same as the size in omaha response.
+ int64_t new_size = fake_update_response_.size;
+
+ fake_prefs_.SetString(kPrefsUpdateOverCellularTargetVersion, new_version);
+ fake_prefs_.SetInt64(kPrefsUpdateOverCellularTargetSize, new_size);
+ fake_system_state_.set_connection_manager(&mock_cm);
+
+ EXPECT_CALL(mock_cm, GetConnectionProperties(_, _))
+ .WillRepeatedly(
+ DoAll(SetArgumentPointee<0>(NetworkConnectionType::kCellular),
+ SetArgumentPointee<1>(NetworkTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(NetworkConnectionType::kCellular, _))
+ .WillRepeatedly(Return(true));
+
+ ASSERT_TRUE(
+ TestUpdateCheck(nullptr, // request_params
+ fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kSuccess,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kUpdating,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_TRUE(response.update_exists);
+}
+
TEST_F(OmahaRequestActionTest, ValidUpdateBlockedByRollback) {
string rollback_version = "1234.0.0";
OmahaResponse response;
diff --git a/payload_state.cc b/payload_state.cc
index af1ad05..72f7646 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -347,6 +347,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/update_attempter.cc b/update_attempter.cc
index a7b6ef0..55ef948 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -52,6 +52,7 @@
#include "update_engine/common/prefs_interface.h"
#include "update_engine/common/subprocess.h"
#include "update_engine/common/utils.h"
+#include "update_engine/connection_manager.h"
#include "update_engine/dbus_service.h"
#include "update_engine/metrics.h"
#include "update_engine/omaha_request_action.h"
@@ -1017,9 +1018,20 @@
consecutive_failed_update_checks_ = 0;
}
+ const OmahaResponse& omaha_response =
+ omaha_request_action->GetOutputObject();
// Store the server-dictated poll interval, if any.
server_dictated_poll_interval_ =
- std::max(0, omaha_request_action->GetOutputObject().poll_interval);
+ std::max(0, omaha_response.poll_interval);
+
+ // This update is ignored by omaha request action because update over
+ // cellular connection is not allowed. Needs to ask for user's permissions
+ // to update.
+ if (code == ErrorCode::kOmahaUpdateIgnoredOverCellular) {
+ new_version_ = omaha_response.version;
+ new_payload_size_ = omaha_response.size;
+ SetStatusAndNotify(UpdateStatus::NEED_PERMISSION_TO_UPDATE);
+ }
}
}
if (code != ErrorCode::kSuccess) {
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 27f0304..e1fe398 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -132,6 +132,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";
diff --git a/update_status_utils.cc b/update_status_utils.cc
index 9685853..1b2dfbc 100644
--- a/update_status_utils.cc
+++ b/update_status_utils.cc
@@ -25,6 +25,7 @@
const char kWeaveStatusIdle[] = "idle";
const char kWeaveStatusCheckingForUpdate[] = "checkingForUpdate";
const char kWeaveStatusUpdateAvailable[] = "updateAvailable";
+const char kWeaveStatusNeedPermissionToUpdate[] = "needPermissionToUpdate";
const char kWeaveStatusDownloading[] = "downloading";
const char kWeaveStatusVerifying[] = "verifying";
const char kWeaveStatusFinalizing[] = "finalizing";
@@ -45,6 +46,8 @@
return update_engine::kUpdateStatusCheckingForUpdate;
case UpdateStatus::UPDATE_AVAILABLE:
return update_engine::kUpdateStatusUpdateAvailable;
+ case UpdateStatus::NEED_PERMISSION_TO_UPDATE:
+ return update_engine::kUpdateStatusNeedPermissionToUpdate;
case UpdateStatus::DOWNLOADING:
return update_engine::kUpdateStatusDownloading;
case UpdateStatus::VERIFYING:
@@ -73,6 +76,8 @@
return kWeaveStatusCheckingForUpdate;
case UpdateStatus::UPDATE_AVAILABLE:
return kWeaveStatusUpdateAvailable;
+ case UpdateStatus::NEED_PERMISSION_TO_UPDATE:
+ return kWeaveStatusNeedPermissionToUpdate;
case UpdateStatus::DOWNLOADING:
return kWeaveStatusDownloading;
case UpdateStatus::VERIFYING:
@@ -104,6 +109,9 @@
} else if (s == update_engine::kUpdateStatusUpdateAvailable) {
*status = UpdateStatus::UPDATE_AVAILABLE;
return true;
+ } else if (s == update_engine::kUpdateStatusNeedPermissionToUpdate) {
+ *status = UpdateStatus::NEED_PERMISSION_TO_UPDATE;
+ return true;
} else if (s == update_engine::kUpdateStatusDownloading) {
*status = UpdateStatus::DOWNLOADING;
return true;