Follow up on AllowKioskAppControlChromeVersion policy CL
- Update header includes, adding missed and removing unused.
- UpdateCheckAllowedKioskPinWithNoRequiredVersion verifies target
version prefix is empty.
- Change RealSystemProvider's required platform version variable to
be a RetryPollVariable to handle D-Bus call failure.
- Update ChromeOSPolicy to defer update check when kiosk pin policy
is effective but system provider does not set the required platform
version.
BUG=chromium:577783
Change-Id: Id5400d09fff69001a37e367fff9ede1664c2e060
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 3fb680a..900a845 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -236,8 +236,13 @@
// Get the required platform version from Chrome.
const string* kiosk_required_platform_version_p =
ec->GetValue(system_provider->var_kiosk_required_platform_version());
- if (kiosk_required_platform_version_p)
- result->target_version_prefix = *kiosk_required_platform_version_p;
+ if (!kiosk_required_platform_version_p) {
+ LOG(INFO) << "Kiosk app required platform version is not fetched, "
+ "blocking update checks";
+ return EvalStatus::kAskMeAgainLater;
+ }
+
+ result->target_version_prefix = *kiosk_required_platform_version_p;
LOG(INFO) << "Allow kiosk app to control Chrome version policy is set,"
<< ", target version is "
<< (kiosk_required_platform_version_p
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index 9177136..8a1796f 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -516,21 +516,41 @@
SetUpdateCheckAllowed(true);
// AU disabled, allow kiosk to pin but there is no kiosk required platform
- // version (due to Chrome crash or app does not provide the info). Update to
- // latest in such case.
+ // version (i.e. app does not provide the info). Update to latest in such
+ // case.
fake_state_.device_policy_provider()->var_update_disabled()->reset(
new bool(true));
fake_state_.device_policy_provider()
->var_allow_kiosk_app_control_chrome_version()
->reset(new bool(true));
+ fake_state_.system_provider()->var_kiosk_required_platform_version()->reset(
+ new string());
UpdateCheckParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCheckAllowed, &result);
EXPECT_TRUE(result.updates_enabled);
+ EXPECT_TRUE(result.target_version_prefix.empty());
EXPECT_FALSE(result.is_interactive);
}
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCheckAllowedKioskPinWithFailedGetRequiredVersionCall) {
+ // AU disabled, allow kiosk to pin but D-Bus call to get required platform
+ // version failed. Defer update check in this case.
+ fake_state_.device_policy_provider()->var_update_disabled()->reset(
+ new bool(true));
+ fake_state_.device_policy_provider()
+ ->var_allow_kiosk_app_control_chrome_version()
+ ->reset(new bool(true));
+ fake_state_.system_provider()->var_kiosk_required_platform_version()->reset(
+ nullptr);
+
+ UpdateCheckParams result;
+ ExpectPolicyStatus(EvalStatus::kAskMeAgainLater,
+ &Policy::UpdateCheckAllowed, &result);
+}
+
TEST_F(UmChromeOSPolicyTest, UpdateCanStartFailsCheckAllowedError) {
// The UpdateCanStart policy fails, not being able to query
// UpdateCheckAllowed.
diff --git a/update_manager/real_system_provider.cc b/update_manager/real_system_provider.cc
index f90e1ed..294a947 100644
--- a/update_manager/real_system_provider.cc
+++ b/update_manager/real_system_provider.cc
@@ -16,26 +16,81 @@
#include "update_engine/update_manager/real_system_provider.h"
-#include <string.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
-
-#include <string>
-#include <vector>
-
+#include <base/bind.h>
+#include <base/callback.h>
#include <base/logging.h>
-#include <base/strings/stringprintf.h>
#include <base/time/time.h>
#include "update_engine/common/utils.h"
#include "update_engine/libcros_proxy.h"
#include "update_engine/update_manager/generic_variables.h"
+#include "update_engine/update_manager/variable.h"
using std::string;
namespace chromeos_update_manager {
+namespace {
+
+// The RetryPollVariable variable is a polling variable that allows the function
+// returning the value to fail a few times and shortens the polling rate when
+// that happens.
+template <typename T>
+class RetryPollVariable : public Variable<T> {
+ public:
+ RetryPollVariable(const string& name,
+ const base::TimeDelta poll_interval,
+ base::Callback<bool(T* res)> func)
+ : Variable<T>(name, poll_interval),
+ func_(func),
+ base_interval_(poll_interval) {
+ DCHECK_LT(kRetryIntervalSeconds, base_interval_.InSeconds());
+ }
+
+ protected:
+ // Variable override.
+ const T* GetValue(base::TimeDelta /* timeout */,
+ string* /* errmsg */) override {
+ std::unique_ptr<T> result(new T());
+ if (!func_.Run(result.get())) {
+ if (failed_attempts_ >= kMaxRetry) {
+ // Give up on the retries, set back the desired polling interval and
+ // return the default.
+ this->SetPollInterval(base_interval_);
+ return result.release();
+ }
+ this->SetPollInterval(
+ base::TimeDelta::FromSeconds(kRetryIntervalSeconds));
+ failed_attempts_++;
+ return nullptr;
+ }
+ failed_attempts_ = 0;
+ this->SetPollInterval(base_interval_);
+ return result.release();
+ }
+
+ private:
+ // The function to be called, stored as a base::Callback.
+ base::Callback<bool(T*)> func_;
+
+ // The desired polling interval when |func_| works and returns true.
+ base::TimeDelta base_interval_;
+
+ // The number of consecutive failed attempts made.
+ int failed_attempts_ = 0;
+
+ // The maximum number of consecutive failures before returning the default
+ // constructor value for T instead of failure.
+ static const int kMaxRetry = 5;
+
+ // The polling interval to be used whenever GetValue() returns an error.
+ static const int kRetryIntervalSeconds = 5 * 60;
+
+ DISALLOW_COPY_AND_ASSIGN(RetryPollVariable);
+};
+
+} // namespace
+
bool RealSystemProvider::Init() {
var_is_normal_boot_mode_.reset(
new ConstCopyVariable<bool>("is_normal_boot_mode",
@@ -55,7 +110,7 @@
new ConstCopyVariable<unsigned int>(
"num_slots", boot_control_->GetNumSlots()));
- var_kiosk_required_platform_version_.reset(new CallCopyVariable<std::string>(
+ var_kiosk_required_platform_version_.reset(new RetryPollVariable<string>(
"kiosk_required_platform_version",
base::TimeDelta::FromHours(5), // Same as Chrome's CWS poll.
base::Bind(&RealSystemProvider::GetKioskAppRequiredPlatformVersion,
@@ -64,19 +119,20 @@
return true;
}
-std::string RealSystemProvider::GetKioskAppRequiredPlatformVersion() {
- std::string required_platform_version;
-
+bool RealSystemProvider::GetKioskAppRequiredPlatformVersion(
+ string* required_platform_version) {
#if USE_LIBCROS
brillo::ErrorPtr error;
if (!libcros_proxy_->service_interface_proxy()
- ->GetKioskAppRequiredPlatformVersion(&required_platform_version,
+ ->GetKioskAppRequiredPlatformVersion(required_platform_version,
&error)) {
LOG(WARNING) << "Failed to get kiosk required platform version";
+ required_platform_version->clear();
+ return false;
}
#endif
- return required_platform_version;
+ return true;
}
} // namespace chromeos_update_manager
diff --git a/update_manager/real_system_provider.h b/update_manager/real_system_provider.h
index d6c2e24..083943b 100644
--- a/update_manager/real_system_provider.h
+++ b/update_manager/real_system_provider.h
@@ -64,7 +64,8 @@
}
private:
- std::string GetKioskAppRequiredPlatformVersion();
+ bool GetKioskAppRequiredPlatformVersion(
+ std::string* required_platform_version);
std::unique_ptr<Variable<bool>> var_is_normal_boot_mode_;
std::unique_ptr<Variable<bool>> var_is_official_build_;
diff --git a/update_manager/real_system_provider_unittest.cc b/update_manager/real_system_provider_unittest.cc
index 112eba5..bb42817 100644
--- a/update_manager/real_system_provider_unittest.cc
+++ b/update_manager/real_system_provider_unittest.cc
@@ -36,6 +36,10 @@
using testing::Return;
using testing::SetArgPointee;
+namespace {
+const char kRequiredPlatformVersion[] ="1234.0.0";
+} // namespace
+
namespace chromeos_update_manager {
class UmRealSystemProviderTest : public ::testing::Test {
@@ -47,6 +51,10 @@
unique_ptr<
org::chromium::
UpdateEngineLibcrosProxyResolvedInterfaceProxyInterface>()));
+ ON_CALL(*service_interface_mock_,
+ GetKioskAppRequiredPlatformVersion(_, _, _))
+ .WillByDefault(
+ DoAll(SetArgPointee<0>(kRequiredPlatformVersion), Return(true)));
provider_.reset(new RealSystemProvider(&fake_hardware_, &fake_boot_control_,
libcros_proxy_.get()));
@@ -83,14 +91,8 @@
#if USE_LIBCROS
TEST_F(UmRealSystemProviderTest, KioskRequiredPlatformVersion) {
- const std::string kRequiredPlatformVersion("1234.0.0");
- EXPECT_CALL(*service_interface_mock_,
- GetKioskAppRequiredPlatformVersion(_, _, _))
- .WillOnce(
- DoAll(SetArgPointee<0>(kRequiredPlatformVersion), Return(true)));
-
UmTestUtils::ExpectVariableHasValue(
- kRequiredPlatformVersion,
+ std::string(kRequiredPlatformVersion),
provider_->var_kiosk_required_platform_version());
}
@@ -99,6 +101,29 @@
GetKioskAppRequiredPlatformVersion(_, _, _))
.WillOnce(Return(false));
+ UmTestUtils::ExpectVariableNotSet(
+ provider_->var_kiosk_required_platform_version());
+}
+
+TEST_F(UmRealSystemProviderTest,
+ KioskRequiredPlatformVersionRecoveryFromFailure) {
+ EXPECT_CALL(*service_interface_mock_,
+ GetKioskAppRequiredPlatformVersion(_, _, _))
+ .WillOnce(Return(false));
+ UmTestUtils::ExpectVariableNotSet(
+ provider_->var_kiosk_required_platform_version());
+ testing::Mock::VerifyAndClearExpectations(service_interface_mock_);
+
+ EXPECT_CALL(*service_interface_mock_,
+ GetKioskAppRequiredPlatformVersion(_, _, _))
+ .WillOnce(
+ DoAll(SetArgPointee<0>(kRequiredPlatformVersion), Return(true)));
+ UmTestUtils::ExpectVariableHasValue(
+ std::string(kRequiredPlatformVersion),
+ provider_->var_kiosk_required_platform_version());
+}
+#else
+TEST_F(UmRealSystemProviderTest, KioskRequiredPlatformVersion) {
UmTestUtils::ExpectVariableHasValue(
std::string(), provider_->var_kiosk_required_platform_version());
}
diff --git a/update_manager/variable.h b/update_manager/variable.h
index 98774ef..7109692 100644
--- a/update_manager/variable.h
+++ b/update_manager/variable.h
@@ -114,6 +114,13 @@
BaseVariable(const std::string& name, base::TimeDelta poll_interval)
: BaseVariable(name, kVariableModePoll, poll_interval) {}
+ // Reset the poll interval on a polling variable to the given one.
+ void SetPollInterval(base::TimeDelta poll_interval) {
+ DCHECK_EQ(kVariableModePoll, mode_) << "Can't set the poll_interval on a "
+ << mode_ << " variable";
+ poll_interval_ = poll_interval;
+ }
+
// Calls ValueChanged on all the observers.
void NotifyValueChanged() {
// Fire all the observer methods from the main loop as single call. In order
@@ -166,7 +173,7 @@
// The variable's polling interval for VariableModePoll variable and 0 for
// other modes.
- const base::TimeDelta poll_interval_;
+ base::TimeDelta poll_interval_;
// The list of value changes observers.
std::list<BaseVariable::ObserverInterface*> observer_list_;