diff options
29 files changed, 476 insertions, 133 deletions
diff --git a/UpdateEngine.conf b/UpdateEngine.conf index 58cca090..192e6ab7 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 b1a1b4fa..7893548d 100644 --- a/binder_bindings/android/brillo/IUpdateEngine.aidl +++ b/binder_bindings/android/brillo/IUpdateEngine.aidl @@ -33,6 +33,8 @@ interface IUpdateEngine { 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 5e74159f..14df7254 100644 --- a/binder_service_brillo.cc +++ b/binder_service_brillo.cc @@ -151,6 +151,14 @@ Status BinderUpdateEngineBrilloService::SetUpdateOverCellularPermission( &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 982c7b18..4cc007ba 100644 --- a/binder_service_brillo.h +++ b/binder_service_brillo.h @@ -76,6 +76,9 @@ class BinderUpdateEngineBrilloService : public android::brillo::BnUpdateEngine, 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 3e9af5b6..2f6322a3 100644 --- a/client_library/include/update_engine/update_status.h +++ b/client_library/include/update_engine/update_status.h @@ -23,6 +23,9 @@ enum class UpdateStatus { 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 88d04459..d6d1c6d3 100644 --- a/common/constants.cc +++ b/common/constants.cc @@ -74,6 +74,10 @@ const char kPrefsUpdateDurationUptime[] = "update-duration-uptime"; 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 ab66921e..c571403d 100644 --- a/common/constants.h +++ b/common/constants.h @@ -75,6 +75,8 @@ extern const char kPrefsUpdateCompletedOnBootId[]; 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 e08ec460..bcf541ce 100644 --- a/common/error_code.h +++ b/common/error_code.h @@ -73,6 +73,7 @@ enum class ErrorCode : int { 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 ad4aeeb0..0169ab89 100644 --- a/common/error_code_utils.cc +++ b/common/error_code_utils.cc @@ -144,6 +144,8 @@ string ErrorCodeToString(ErrorCode code) { 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 1a41b633..ee890e12 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 { @@ -248,24 +246,12 @@ bool UpdateEngineService::GetP2PUpdatePermission(ErrorPtr* error, 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; @@ -276,9 +262,9 @@ bool UpdateEngineService::SetUpdateOverCellularPermission(ErrorPtr* error, 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; @@ -286,24 +272,63 @@ bool UpdateEngineService::SetUpdateOverCellularPermission(ErrorPtr* error, 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 69368fb6..4320d09c 100644 --- a/common_service.h +++ b/common_service.h @@ -115,6 +115,12 @@ class UpdateEngineService { 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 f72d9e84..f0a2c923 100644 --- a/connection_manager.cc +++ b/connection_manager.cc @@ -30,6 +30,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; @@ -58,15 +59,23 @@ bool ConnectionManager::IsUpdateAllowedOver( 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) { + 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 networks as there's no " - "device policy loaded yet."; + LOG(INFO) << "Disabling updates over cellular as device policy " + "fails to be loaded."; return false; } @@ -74,38 +83,17 @@ bool ConnectionManager::IsUpdateAllowedOver( // 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: @@ -120,6 +108,22 @@ bool ConnectionManager::IsUpdateAllowedOver( } } +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 e5a9d493..864985ed 100644 --- a/connection_manager.h +++ b/connection_manager.h @@ -43,6 +43,7 @@ class ConnectionManager : public ConnectionManagerInterface { 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 2dd824a9..c594b53d 100644 --- a/connection_manager_android.cc +++ b/connection_manager_android.cc @@ -34,5 +34,9 @@ bool ConnectionManagerAndroid::IsUpdateAllowedOver( 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 0cd5e73c..006f4ead 100644 --- a/connection_manager_android.h +++ b/connection_manager_android.h @@ -34,6 +34,7 @@ class ConnectionManagerAndroid : public ConnectionManagerInterface { 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 df8eb4be..2faeb804 100644 --- a/connection_manager_interface.h +++ b/connection_manager_interface.h @@ -46,6 +46,10 @@ class ConnectionManagerInterface { 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 0bb55479..f8146674 100644 --- a/connection_manager_unittest.cc +++ b/connection_manager_unittest.cc @@ -277,16 +277,24 @@ TEST_F(ConnectionManagerTest, AllowUpdatesOver3GAndOtherTypesPerPolicyTest) { 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)); } @@ -311,62 +319,20 @@ TEST_F(ConnectionManagerTest, BlockUpdatesOver3GPerPolicyTest) { 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, StringForConnectionTypeTest) { diff --git a/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml b/dbus_bindings/org.chromium.UpdateEngineInterface.dbus-xml index 848f7755..a20f33fa 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 0a7ad5b6..731d489a 100644 --- a/dbus_service.cc +++ b/dbus_service.cc @@ -123,6 +123,14 @@ bool DBusUpdateEngineService::SetUpdateOverCellularPermission(ErrorPtr* error, 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 2b36ae9d..644dceeb 100644 --- a/dbus_service.h +++ b/dbus_service.h @@ -114,6 +114,12 @@ class DBusUpdateEngineService 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 263bacd2..15e3a8a9 100644 --- a/metrics_utils.cc +++ b/metrics_utils.cc @@ -109,6 +109,7 @@ metrics::AttemptResult GetAttemptResult(ErrorCode code) { 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 @@ metrics::DownloadErrorCode GetDownloadErrorCode(ErrorCode code) { 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 e37460b2..2fff68c6 100644 --- a/mock_connection_manager.h +++ b/mock_connection_manager.h @@ -36,6 +36,7 @@ class MockConnectionManager : public ConnectionManagerInterface { 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 b06de097..ed3c716a 100644 --- a/omaha_request_action.cc +++ b/omaha_request_action.cc @@ -1001,9 +1001,11 @@ void OmahaRequestAction::TransferComplete(HttpFetcher *fetcher, 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; } @@ -1461,6 +1463,7 @@ void OmahaRequestAction::ActionCompleted(ErrorCode code) { break; case ErrorCode::kOmahaUpdateIgnoredPerPolicy: + case ErrorCode::kOmahaUpdateIgnoredOverCellular: result = metrics::CheckResult::kUpdateAvailable; reaction = metrics::CheckReaction::kIgnored; break; @@ -1495,7 +1498,7 @@ void OmahaRequestAction::ActionCompleted(ErrorCode code) { } 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(); @@ -1503,11 +1506,12 @@ bool OmahaRequestAction::ShouldIgnoreUpdate( 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; } @@ -1522,7 +1526,59 @@ bool OmahaRequestAction::ShouldIgnoreUpdate( 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 { ConnectionType type; ConnectionTethering tethering; ConnectionManagerInterface* connection_manager = @@ -1532,7 +1588,30 @@ bool OmahaRequestAction::IsUpdateAllowedOverCurrentConnection() const { << "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 if (!IsUpdateAllowedOverCellularByPrefs(response)) { + // The user prefereces does not allow updates over cellular. + is_allowed = false; + *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 2915a6a8..7d35c238 100644 --- a/omaha_request_action.h +++ b/omaha_request_action.h @@ -299,11 +299,17 @@ class OmahaRequestAction : public Action<OmahaRequestAction>, 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 1c1d25c7..cb3ef71b 100644 --- a/omaha_request_action_unittest.cc +++ b/omaha_request_action_unittest.cc @@ -518,6 +518,185 @@ TEST_F(OmahaRequestActionTest, ValidUpdateBlockedByConnection) { 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>(ConnectionType::kCellular), + SetArgumentPointee<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(SetArgumentPointee<0>(ConnectionType::kCellular), + SetArgumentPointee<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(SetArgumentPointee<0>(ConnectionType::kCellular), + SetArgumentPointee<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(SetArgumentPointee<0>(ConnectionType::kCellular), + SetArgumentPointee<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(SetArgumentPointee<0>(ConnectionType::kCellular), + SetArgumentPointee<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 1da472f4..846f9015 100644 --- a/payload_state.cc +++ b/payload_state.cc @@ -348,6 +348,7 @@ void PayloadState::UpdateFailed(ErrorCode error) { 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 d9a8d048..2cc53086 100644 --- a/update_attempter.cc +++ b/update_attempter.cc @@ -49,6 +49,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.h" #include "update_engine/omaha_request_action.h" @@ -1005,9 +1006,20 @@ void UpdateAttempter::ActionCompleted(ActionProcessor* processor, 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 ffd378c1..e3893d31 100644 --- a/update_manager/chromeos_policy.cc +++ b/update_manager/chromeos_policy.cc @@ -134,6 +134,7 @@ bool HandleErrorCode(ErrorCode err_code, int* url_num_error_p) { 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 ff039b8c..5de3381f 100644 --- a/update_status_utils.cc +++ b/update_status_utils.cc @@ -30,6 +30,8 @@ const char* UpdateStatusToString(const UpdateStatus& status) { 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 @@ bool StringToUpdateStatus(const std::string& s, } 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; |