Skip to content

Commit

Permalink
Make Hands-Off Zero-Touch Enrollment compatibile with tests
Browse files Browse the repository at this point in the history
Specifically, the automatic nature of Hands-Off Enrollment leads to
the DUT performing an update check after reboot which interferes
with autoupdate_EndToEndTest. This CL fixes that issue.
It does so by instructing the UpdateScreen to skip checking for updates
if both of the following conditions hold:
1. The previous update check initiated by the UpdateScreen did not result
in an update.
2. This previous update check occurred during the last hour.

BUG=chromium:710716
TEST=unit test

Review-Url: https://codereview.chromium.org/2894783003
Cr-Commit-Position: refs/heads/master@{#473388}
  • Loading branch information
kumarniranjan authored and Commit bot committed May 20, 2017
1 parent 40c99bc commit f097331
Show file tree
Hide file tree
Showing 20 changed files with 489 additions and 52 deletions.
7 changes: 7 additions & 0 deletions chrome/browser/chromeos/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1526,15 +1526,21 @@ static_library("test_support") {
"login/mock_network_state_helper.h",
"login/screens/mock_base_screen_delegate.cc",
"login/screens/mock_base_screen_delegate.h",
"login/screens/mock_error_screen.cc",
"login/screens/mock_error_screen.h",
"login/screens/mock_model_view_channel.cc",
"login/screens/mock_model_view_channel.h",
"login/screens/mock_network_screen.cc",
"login/screens/mock_network_screen.h",
"login/screens/mock_update_screen.cc",
"login/screens/mock_update_screen.h",
]

deps = [
":chromeos",
"//chromeos",
"//components/policy/proto",
"//crypto:platform",
"//skia",
"//testing/gmock",
]
Expand Down Expand Up @@ -1683,6 +1689,7 @@ source_set("unit_tests") {
"login/quick_unlock/quick_unlock_storage_unittest.cc",
"login/saml/saml_offline_signin_limiter_unittest.cc",
"login/screens/network_screen_unittest.cc",
"login/screens/update_screen_unittest.cc",
"login/signin/merge_session_load_page_unittest.cc",
"login/supervised/supervised_user_authentication_unittest.cc",
"login/users/affiliation_unittest.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "chrome/test/base/testing_profile.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "chromeos/network/network_state.h"
#include "chromeos/network/portal_detector/mock_network_portal_detector.h"
#include "chromeos/network/portal_detector/network_portal_detector.h"
#include "components/image_fetcher/core/image_fetcher.h"
#include "components/image_fetcher/core/image_fetcher_delegate.h"
Expand Down Expand Up @@ -43,31 +44,6 @@ namespace chromeos {

namespace {

class MockNetworkPortalDetector : public NetworkPortalDetector {
public:
MockNetworkPortalDetector() {}
~MockNetworkPortalDetector() override {}

MOCK_METHOD1(AddObserver,
void(chromeos::NetworkPortalDetector::Observer* observer));
MOCK_METHOD1(RemoveObserver,
void(chromeos::NetworkPortalDetector::Observer* observer));
MOCK_METHOD1(AddAndFireObserver,
void(chromeos::NetworkPortalDetector::Observer* observer));
MOCK_METHOD1(GetCaptivePortalState,
chromeos::NetworkPortalDetector::CaptivePortalState(
const std::string& service_path));
MOCK_METHOD0(IsEnabled, bool());
MOCK_METHOD1(Enable, void(bool start_detection));
MOCK_METHOD0(StartDetectionIfIdle, bool());
MOCK_METHOD1(SetStrategy,
void(chromeos::PortalDetectorStrategy::StrategyId id));
MOCK_METHOD0(OnLockScreenRequest, void());

private:
DISALLOW_COPY_AND_ASSIGN(MockNetworkPortalDetector);
};

class MockImageFetcher : public image_fetcher::ImageFetcher {
public:
MockImageFetcher() {}
Expand Down
13 changes: 3 additions & 10 deletions chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,6 @@ constexpr double kMultiplyFactor = 1.5;
constexpr double kJitterFactor = 0.1; // +/- 10% jitter
constexpr int64_t kMaxDelayMS = 8 * 60 * 1000; // 8 minutes

// Helper function. Returns true if we are using Hands Off Enrollment.
bool UsingHandsOffEnrollment() {
return policy::DeviceCloudPolicyManagerChromeOS::
GetZeroTouchEnrollmentMode() ==
policy::ZeroTouchEnrollmentMode::HANDS_OFF;
}

} // namespace

namespace chromeos {
Expand Down Expand Up @@ -284,7 +277,7 @@ void EnrollmentScreen::OnEnrollmentError(policy::EnrollmentStatus status) {
Show();
} else {
view_->ShowEnrollmentStatus(status);
if (UsingHandsOffEnrollment())
if (WizardController::UsingHandsOffEnrollment())
AutomaticRetry();
}
}
Expand All @@ -293,7 +286,7 @@ void EnrollmentScreen::OnOtherError(
EnterpriseEnrollmentHelper::OtherError error) {
RecordEnrollmentErrorMetrics();
view_->ShowOtherError(error);
if (UsingHandsOffEnrollment())
if (WizardController::UsingHandsOffEnrollment())
AutomaticRetry();
}

Expand Down Expand Up @@ -368,7 +361,7 @@ void EnrollmentScreen::ShowEnrollmentStatusOnSuccess() {
retry_backoff_->InformOfRequest(true);
if (elapsed_timer_)
UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeSuccess, elapsed_timer_);
if (UsingHandsOffEnrollment()) {
if (WizardController::UsingHandsOffEnrollment()) {
OnConfirmationClosed();
} else {
view_->ShowEnrollmentStatus(
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/chromeos/login/screens/eula_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "chrome/browser/chromeos/login/screens/base_screen_delegate.h"
#include "chrome/browser/chromeos/login/screens/eula_view.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"
#include "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
#include "chromeos/dbus/cryptohome_client.h"
#include "chromeos/dbus/dbus_method_call_status.h"
#include "chromeos/dbus/dbus_thread_manager.h"
Expand Down Expand Up @@ -84,8 +83,7 @@ void EulaScreen::Show() {
// Command to own the TPM.
DBusThreadManager::Get()->GetCryptohomeClient()->TpmCanAttemptOwnership(
EmptyVoidDBusMethodCallback());
if (policy::DeviceCloudPolicyManagerChromeOS::GetZeroTouchEnrollmentMode() ==
policy::ZeroTouchEnrollmentMode::HANDS_OFF)
if (WizardController::UsingHandsOffEnrollment())
OnUserAction(kUserActionAcceptButtonClicked);
else if (view_)
view_->Show();
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/chromeos/login/screens/network_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "chrome/browser/chromeos/login/screens/network_view.h"
#include "chrome/browser/chromeos/login/ui/input_events_blocker.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"
#include "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/webui/chromeos/login/l10n_util.h"
#include "chrome/common/pref_names.h"
Expand Down Expand Up @@ -363,8 +362,7 @@ void NetworkScreen::StopWaitingForConnection(const base::string16& network_id) {

// Automatically continue if we are using Hands-Off Enrollment.
if (is_connected && continue_attempts_ == 0 &&
policy::DeviceCloudPolicyManagerChromeOS::GetZeroTouchEnrollmentMode() ==
policy::ZeroTouchEnrollmentMode::HANDS_OFF) {
WizardController::UsingHandsOffEnrollment()) {
OnContinueButtonPressed();
}
}
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/chromeos/login/screens/screen_exit_code.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ enum class ScreenExitCode {
// Connection failed while trying to load a WebPageScreen.
CONNECTION_FAILED = 2,
UPDATE_INSTALLED = 3,
// This exit code means EITHER that there was no update, OR that there
// was an update, but that it was not a "critical" update. "Critical" updates
// are those that have a deadline and require the device to reboot.
UPDATE_NOUPDATE = 4,
UPDATE_ERROR_CHECKING_FOR_UPDATE = 5,
UPDATE_ERROR_UPDATING = 6,
Expand Down
49 changes: 44 additions & 5 deletions chrome/browser/chromeos/login/screens/update_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ void UpdateScreen::UpdateStatusChanged(
HostPairingController::UPDATE_STATUS_UPDATING);
break;
case UpdateEngineClient::UPDATE_STATUS_UPDATE_AVAILABLE:
ClearUpdateCheckNoUpdateTime();
MakeSureScreenIsShown();
GetContextEditor()
.SetInteger(kContextKeyProgress, kBeforeDownloadProgress)
Expand Down Expand Up @@ -554,11 +555,19 @@ void UpdateScreen::StartUpdateCheck() {
connect_request_subscription_.reset();
if (state_ == State::STATE_ERROR)
HideErrorMessage();
state_ = State::STATE_UPDATE;
DBusThreadManager::Get()->GetUpdateEngineClient()->AddObserver(this);
VLOG(1) << "Initiate update check";
DBusThreadManager::Get()->GetUpdateEngineClient()->RequestUpdateCheck(
base::Bind(StartUpdateCallback, this));

if (ShouldCheckForUpdate()) {
state_ = State::STATE_UPDATE;
DBusThreadManager::Get()->GetUpdateEngineClient()->AddObserver(this);
VLOG(1) << "Initiate update check";
RecordUpdateCheckWithNoUpdateYet();
DBusThreadManager::Get()->GetUpdateEngineClient()->RequestUpdateCheck(
base::Bind(StartUpdateCallback, this));
} else {
LOG(WARNING) << "Skipping update check since one was done recently "
"which did not result in an update.";
CancelUpdate();
}
}

void UpdateScreen::ShowErrorMessage() {
Expand Down Expand Up @@ -629,4 +638,34 @@ void UpdateScreen::OnConnectRequested() {
}
}

void UpdateScreen::RecordUpdateCheckWithNoUpdateYet() {
StartupUtils::SaveTimeOfLastUpdateCheckWithoutUpdate(base::Time::Now());
}

void UpdateScreen::ClearUpdateCheckNoUpdateTime() {
StartupUtils::ClearTimeOfLastUpdateCheckWithoutUpdate();
}

bool UpdateScreen::ShouldCheckForUpdate() {
// Always run update check for non hands-off enrollment.
if (!WizardController::UsingHandsOffEnrollment())
return true;

// If we check for an update and there is no need to perform an update,
// this is the time in hours we should wait before checking again.
const base::TimeDelta kUpdateCheckRecencyThreshold =
base::TimeDelta::FromHours(1);

base::Time now = base::Time::Now();
base::Time last = StartupUtils::GetTimeOfLastUpdateCheckWithoutUpdate();

// Return false if enough time has not passed since the last update check.
// Otherwise, return true.
if (now > last) {
return (now - last) > kUpdateCheckRecencyThreshold;
}

return true;
}

} // namespace chromeos
13 changes: 13 additions & 0 deletions chrome/browser/chromeos/login/screens/update_screen.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class UpdateScreen : public BaseScreen,
FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestBasic);
FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestUpdateAvailable);
FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestAPReselection);
friend class UpdateScreenUnitTest;

enum class State {
STATE_IDLE = 0,
Expand Down Expand Up @@ -125,6 +126,18 @@ class UpdateScreen : public BaseScreen,
// The user requested an attempt to connect to the network should be made.
void OnConnectRequested();

// Records the fact that we performed an update check but do not yet
// know if this update check is going to result in an update.
void RecordUpdateCheckWithNoUpdateYet();

// When an update is found, this is called to clear the time of update check
// which had saved when we did not yet know whether it would result in an
// update.
void ClearUpdateCheckNoUpdateTime();

// Returns true if we should check for an update.
bool ShouldCheckForUpdate();

// Timer for the interval to wait for the reboot.
// If reboot didn't happen - ask user to reboot manually.
base::OneShotTimer reboot_timer_;
Expand Down
Loading

0 comments on commit f097331

Please sign in to comment.