Reland update over cellular changes
This merge cherrypicks two commits that was reverted in an AOSP git merge.
4b0d6032cbb86ce488c03b31936cda31283f97e3 Add functions to allow update over cellular (including tethered connection)
840703a4cc77228e2606f45665ae3a4bd75ff7dd Fix update over cellular network on guest account
Handled multi-package response.
Ran clang-format which fixed a lot of issues in those two CLs.
BUG=chromium:815356
TEST=unittests, precq
Change-Id: I54b6763c4c54755272531b558ed7628ceb0fc6c7
Reviewed-on: https://chromium-review.googlesource.com/965267
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@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 e549a4d..56e1524 100644
--- a/binder_bindings/android/brillo/IUpdateEngine.aidl
+++ b/binder_bindings/android/brillo/IUpdateEngine.aidl
@@ -34,6 +34,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/binder_service_brillo.cc b/binder_service_brillo.cc
index 3f01e42..d082add 100644
--- a/binder_service_brillo.cc
+++ b/binder_service_brillo.cc
@@ -153,6 +153,13 @@
&UpdateEngineService::SetUpdateOverCellularPermission, enabled);
}
+Status BinderUpdateEngineBrilloService::SetUpdateOverCellularTarget(
+ const String16& target_version, int64_t target_size) {
+ return CallCommonHandler(&UpdateEngineService::SetUpdateOverCellularTarget,
+ NormalString(target_version),
+ target_size);
+}
+
Status BinderUpdateEngineBrilloService::GetUpdateOverCellularPermission(
bool* out_cellular_permission) {
return CallCommonHandler(
diff --git a/binder_service_brillo.h b/binder_service_brillo.h
index c802fca..d0d0dc9 100644
--- a/binder_service_brillo.h
+++ b/binder_service_brillo.h
@@ -75,6 +75,8 @@
bool* out_p2p_permission) override;
android::binder::Status SetUpdateOverCellularPermission(
bool enabled) override;
+ android::binder::Status SetUpdateOverCellularTarget(
+ const android::String16& target_version, int64_t target_size) override;
android::binder::Status GetUpdateOverCellularPermission(
bool* out_cellular_permission) override;
android::binder::Status GetDurationSinceUpdate(
diff --git a/client_library/include/update_engine/update_status.h b/client_library/include/update_engine/update_status.h
index 41fab48..65c5874 100644
--- a/client_library/include/update_engine/update_status.h
+++ b/client_library/include/update_engine/update_status.h
@@ -27,6 +27,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 5941c93..6fd373e 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -75,6 +75,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 26773cf..1be22de 100644
--- a/common/constants.h
+++ b/common/constants.h
@@ -76,6 +76,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 adae391..d6c8080 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -73,7 +73,7 @@
kFilesystemVerifierError = 47,
kUserCanceled = 48,
kNonCriticalUpdateInOOBE = 49,
- // kOmahaUpdateIgnoredOverCellular = 50,
+ kOmahaUpdateIgnoredOverCellular = 50,
// kPayloadTimestampError = 51,
kUpdatedButNotActive = 52,
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index 983d004..2c485a2 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";
case ErrorCode::kUpdatedButNotActive:
return "ErrorCode::kUpdatedButNotActive";
// Don't add a default case to let the compiler warn about newly added
diff --git a/common_service.cc b/common_service.cc
index 9f3b862..6614622 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/bind.h>
@@ -41,7 +40,6 @@
using base::StringPrintf;
using brillo::ErrorPtr;
using brillo::string_utils::ToString;
-using std::set;
using std::string;
using update_engine::UpdateAttemptFlags;
using update_engine::UpdateEngineStatus;
@@ -258,22 +256,11 @@
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)) {
+ if (connection_manager->IsAllowedConnectionTypesForUpdateSet()) {
LogAndSetError(error,
FROM_HERE,
"Ignoring the update over cellular setting since there's "
@@ -286,7 +273,8 @@
PrefsInterface* prefs = system_state_->prefs();
- if (!prefs->SetBoolean(kPrefsUpdateOverCellularPermission, in_allowed)) {
+ if (!prefs ||
+ !prefs->SetBoolean(kPrefsUpdateOverCellularPermission, in_allowed)) {
LogAndSetError(error,
FROM_HERE,
string("Error setting the update over cellular to ") +
@@ -296,24 +284,66 @@
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(ConnectionType::kCellular,
- ConnectionTethering::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(
+ ConnectionType::kCellular, ConnectionTethering::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 544dd93..824ef97 100644
--- a/common_service.h
+++ b/common_service.h
@@ -114,6 +114,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 be707bc..4063f24 100644
--- a/connection_manager.cc
+++ b/connection_manager.cc
@@ -31,6 +31,7 @@
#include "update_engine/connection_utils.h"
#include "update_engine/shill_proxy.h"
#include "update_engine/system_state.h"
+#include "update_engine/update_attempter.h"
using org::chromium::flimflam::ManagerProxyInterface;
using org::chromium::flimflam::ServiceProxyInterface;
@@ -59,16 +60,27 @@
case ConnectionType::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.";
- return false;
+ UpdateAttempter* update_attempter = system_state_->update_attempter();
+ if (update_attempter) {
+ update_attempter->RefreshDevicePolicy();
+ device_policy = system_state_->device_policy();
+ }
+ }
+
+ if (!device_policy) {
+ // Device policy fails to be loaded (possibly due to guest account). We
+ // do not check the local user setting here, which should be checked by
+ // |OmahaRequestAction| during checking for update.
+ LOG(INFO) << "Allowing updates over cellular as device policy "
+ "fails to be loaded.";
+ return true;
}
if (device_policy->GetAllowedConnectionTypesForUpdate(&allowed_types)) {
@@ -82,31 +94,14 @@
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.
+ LOG(INFO) << "Allowing updates over cellular as device policy does "
+ "not include update setting.";
+ return true;
}
default:
@@ -121,6 +116,21 @@
}
}
+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;
+}
+
bool ConnectionManager::GetConnectionProperties(
ConnectionType* out_type, ConnectionTethering* out_tethering) {
dbus::ObjectPath default_service_path;
diff --git a/connection_manager.h b/connection_manager.h
index e5a9d49..dc563ef 100644
--- a/connection_manager.h
+++ b/connection_manager.h
@@ -17,6 +17,7 @@
#ifndef UPDATE_ENGINE_CONNECTION_MANAGER_H_
#define UPDATE_ENGINE_CONNECTION_MANAGER_H_
+#include <memory>
#include <string>
#include <base/macros.h>
@@ -43,6 +44,7 @@
ConnectionTethering* out_tethering) override;
bool IsUpdateAllowedOver(ConnectionType type,
ConnectionTethering 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_android.cc b/connection_manager_android.cc
index 2dd824a..6da4cee 100644
--- a/connection_manager_android.cc
+++ b/connection_manager_android.cc
@@ -16,6 +16,8 @@
#include "update_engine/connection_manager_android.h"
+#include <memory>
+
namespace chromeos_update_engine {
namespace connection_manager {
@@ -34,5 +36,8 @@
ConnectionType type, ConnectionTethering tethering) const {
return true;
}
+bool ConnectionManagerAndroid::IsAllowedConnectionTypesForUpdateSet() const {
+ return false;
+}
} // namespace chromeos_update_engine
diff --git a/connection_manager_android.h b/connection_manager_android.h
index 0cd5e73..006f4ea 100644
--- a/connection_manager_android.h
+++ b/connection_manager_android.h
@@ -34,6 +34,7 @@
ConnectionTethering* out_tethering) override;
bool IsUpdateAllowedOver(ConnectionType type,
ConnectionTethering tethering) const override;
+ bool IsAllowedConnectionTypesForUpdateSet() const override;
DISALLOW_COPY_AND_ASSIGN(ConnectionManagerAndroid);
};
diff --git a/connection_manager_interface.h b/connection_manager_interface.h
index df8eb4b..2faeb80 100644
--- a/connection_manager_interface.h
+++ b/connection_manager_interface.h
@@ -46,6 +46,10 @@
virtual bool IsUpdateAllowedOver(ConnectionType type,
ConnectionTethering 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 e26a686..85b8c57 100644
--- a/connection_manager_unittest.cc
+++ b/connection_manager_unittest.cc
@@ -16,8 +16,10 @@
#include "update_engine/connection_manager.h"
+#include <memory>
#include <set>
#include <string>
+#include <utility>
#include <base/logging.h>
#include <brillo/any.h>
@@ -276,16 +278,24 @@
ConnectionTethering::kConfirmed));
}
-TEST_F(ConnectionManagerTest, BlockUpdatesOverCellularByDefaultTest) {
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(ConnectionType::kCellular,
- ConnectionTethering::kUnknown));
+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(ConnectionType::kCellular,
+ ConnectionTethering::kUnknown));
}
-TEST_F(ConnectionManagerTest, BlockUpdatesOverTetheredNetworkByDefaultTest) {
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(ConnectionType::kWifi,
- ConnectionTethering::kConfirmed));
- EXPECT_FALSE(cmut_.IsUpdateAllowedOver(ConnectionType::kEthernet,
- ConnectionTethering::kConfirmed));
+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(ConnectionType::kWifi,
+ ConnectionTethering::kConfirmed));
+ EXPECT_TRUE(cmut_.IsUpdateAllowedOver(ConnectionType::kEthernet,
+ ConnectionTethering::kConfirmed));
EXPECT_TRUE(cmut_.IsUpdateAllowedOver(ConnectionType::kWifi,
ConnectionTethering::kSuspected));
}
@@ -310,62 +320,27 @@
ConnectionTethering::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(StringForConnectionType(ConnectionType::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(ConnectionType::kCellular,
- ConnectionTethering::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(ConnectionType::kCellular,
- ConnectionTethering::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(ConnectionType::kCellular,
ConnectionTethering::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(ConnectionType::kCellular,
- ConnectionTethering::kUnknown));
+TEST_F(ConnectionManagerTest, AllowUpdatesOverCellularIfPolicyFailsToBeLoaded) {
+ fake_system_state_.set_device_policy(nullptr);
+
+ EXPECT_TRUE(cmut_.IsUpdateAllowedOver(ConnectionType::kCellular,
+ ConnectionTethering::kUnknown));
}
TEST_F(ConnectionManagerTest, StringForConnectionTypeTest) {
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 47aeec7..bb4ff33 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -133,6 +133,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 b754661..e461fa6 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.
@@ -154,7 +160,7 @@
class UpdateEngineAdaptor : public org::chromium::UpdateEngineInterfaceAdaptor,
public ServiceObserverInterface {
public:
- UpdateEngineAdaptor(SystemState* system_state);
+ explicit UpdateEngineAdaptor(SystemState* system_state);
~UpdateEngineAdaptor() = default;
// Register the DBus object with the update engine service asynchronously.
diff --git a/metrics_utils.cc b/metrics_utils.cc
index 7e6b20f..1500311 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -113,6 +113,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
@@ -213,6 +214,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
break;
diff --git a/mock_connection_manager.h b/mock_connection_manager.h
index e37460b..2fff68c 100644
--- a/mock_connection_manager.h
+++ b/mock_connection_manager.h
@@ -36,6 +36,7 @@
MOCK_CONST_METHOD2(IsUpdateAllowedOver,
bool(ConnectionType type, ConnectionTethering tethering));
+ MOCK_CONST_METHOD0(IsAllowedConnectionTypesForUpdateSet, bool());
};
} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 1becb8e..d08768e 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -1175,9 +1175,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;
}
@@ -1637,6 +1639,7 @@
break;
case ErrorCode::kOmahaUpdateIgnoredPerPolicy:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
result = metrics::CheckResult::kUpdateAvailable;
reaction = metrics::CheckReaction::kIgnored;
break;
@@ -1671,7 +1674,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();
@@ -1679,11 +1682,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;
}
@@ -1698,7 +1702,62 @@
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;
+ }
+
+ uint64_t total_packages_size = 0;
+ for (const auto& package : response.packages) {
+ total_packages_size += package.size;
+ }
+ if (target_version == response.version &&
+ static_cast<uint64_t>(target_size) == total_packages_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 {
ConnectionType type;
ConnectionTethering tethering;
ConnectionManagerInterface* connection_manager =
@@ -1708,7 +1767,32 @@
<< "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 == ConnectionType::kCellular ||
+ tethering == ConnectionTethering::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 {
+ // Deivce policy is not set, so user preferences overwrite whether to
+ // allow updates over cellular.
+ is_allowed = IsUpdateAllowedOverCellularByPrefs(response);
+ if (!is_allowed)
+ *error = ErrorCode::kOmahaUpdateIgnoredOverCellular;
+ }
+
LOG(INFO) << "We are connected via "
<< connection_utils::StringForConnectionType(type)
<< ", Updates allowed: " << (is_allowed ? "Yes" : "No");
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 924da40..f1c4a02 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -292,11 +292,16 @@
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 e9d0504..c513baf 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -782,6 +782,175 @@
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(SetArgPointee<0>(ConnectionType::kCellular),
+ SetArgPointee<1>(ConnectionTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(ConnectionType::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(SetArgPointee<0>(ConnectionType::kCellular),
+ SetArgPointee<1>(ConnectionTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(ConnectionType::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(SetArgPointee<0>(ConnectionType::kCellular),
+ SetArgPointee<1>(ConnectionTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(ConnectionType::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(SetArgPointee<0>(ConnectionType::kCellular),
+ SetArgPointee<1>(ConnectionTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(ConnectionType::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(SetArgPointee<0>(ConnectionType::kCellular),
+ SetArgPointee<1>(ConnectionTethering::kUnknown),
+ Return(true)));
+ EXPECT_CALL(mock_cm, IsAllowedConnectionTypesForUpdateSet())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(mock_cm, IsUpdateAllowedOver(ConnectionType::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 cff02b1..6689cf4 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -357,6 +357,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/update_attempter.cc b/update_attempter.cc
index 7152d8f..13bc098 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -48,6 +48,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_interface.h"
#include "update_engine/libcurl_http_fetcher.h"
#include "update_engine/metrics_reporter_interface.h"
#include "update_engine/omaha_request_action.h"
@@ -1031,9 +1032,23 @@
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_ = 0;
+ for (const auto& package : omaha_response.packages) {
+ new_payload_size_ += package.size;
+ }
+ SetStatusAndNotify(UpdateStatus::NEED_PERMISSION_TO_UPDATE);
+ }
}
} else if (type == OmahaResponseHandlerAction::StaticType()) {
// Depending on the returned error code, note that an update is available.
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 842839a..51af63b 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -141,6 +141,7 @@
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kFilesystemVerifierError:
case ErrorCode::kUserCanceled:
+ case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
diff --git a/update_status_utils.cc b/update_status_utils.cc
index ff039b8..5de3381 100644
--- a/update_status_utils.cc
+++ b/update_status_utils.cc
@@ -30,6 +30,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:
@@ -61,6 +63,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;