Implement a policy to autologin a public account.

chrome/browser/chromeos/login/existing_user_controller.cc

Index: chrome/browser/chromeos/login/existing_user_controller.cc
diff --git a/chrome/browser/chromeos/login/existing_user_controller.cc b/chrome/browser/chromeos/login/existing_user_controller.cc
index 0cf370e5eaffb717f5cfdc32ff4e795bfeae8f30..648d568fa2dcd472c2e331b9732091ca4ecdbf3c 100644
--- a/chrome/browser/chromeos/login/existing_user_controller.cc
+++ b/chrome/browser/chromeos/login/existing_user_controller.cc
@@ -144,7 +144,8 @@ ExistingUserController::ExistingUserController(LoginDisplayHost* host)
       offline_failed_(false),
       is_login_in_progress_(false),
       password_changed_(false),
-      do_auto_enrollment_(false) {
+      do_auto_enrollment_(false),
+      signin_screen_ready_(false) {
   DCHECK(current_controller_ == NULL);
   current_controller_ = this;
 
@@ -164,6 +165,12 @@ ExistingUserController::ExistingUserController(LoginDisplayHost* host)
   cros_settings_->AddSettingsObserver(kAccountsPrefAllowNewUser, this);
   cros_settings_->AddSettingsObserver(kAccountsPrefAllowGuest, this);
   cros_settings_->AddSettingsObserver(kAccountsPrefUsers, this);
+  cros_settings_->AddSettingsObserver(
+      kAccountsPrefDeviceLocalAccountAutoLoginId,
+      this);
+  cros_settings_->AddSettingsObserver(
+      kAccountsPrefDeviceLocalAccountAutoLoginDelay,
+      this);
 }
 
 void ExistingUserController::Init(const UserList& users) {
@@ -229,6 +236,12 @@ void ExistingUserController::Observe(
     registrar_.RemoveAll();
     return;
   }
+  if (type == chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED) {
+    std::string setting = *content::Details<const std::string>(details).ptr();
+    if (setting == kAccountsPrefDeviceLocalAccountAutoLoginId ||
+        setting == kAccountsPrefDeviceLocalAccountAutoLoginDelay)
+      StartAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

nit: add {}

[dconnelly, 2013/02/11 16:17:56]

Done.

+  }
   if (type == chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED ||
       type == chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED) {
     if (host_ != NULL) {
@@ -278,6 +291,12 @@ ExistingUserController::~ExistingUserController() {
   cros_settings_->RemoveSettingsObserver(kAccountsPrefAllowNewUser, this);
   cros_settings_->RemoveSettingsObserver(kAccountsPrefAllowGuest, this);
   cros_settings_->RemoveSettingsObserver(kAccountsPrefUsers, this);
+  cros_settings_->RemoveSettingsObserver(
+      kAccountsPrefDeviceLocalAccountAutoLoginId,
+      this);
+  cros_settings_->RemoveSettingsObserver(
+      kAccountsPrefDeviceLocalAccountAutoLoginDelay,
+      this);
 
   if (current_controller_ == this) {
     current_controller_ = NULL;
@@ -412,6 +431,9 @@ void ExistingUserController::PerformLogin(
   login_display_->SetUIEnabled(false);
   resume_login_callback_.Reset();
 
+  // Stop the autologin timer when attempting login.

[bartfab (slow), 2013/02/11 09:46:01]

Nit: You used the term "autologin" in this file and the term "auto-login" in the
header.

[dconnelly, 2013/02/11 16:17:56]

Done.

+  StopAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

I think that we want to stop this timer as soon as possible. Consider it placing
in these methods instead:

CompleteLogin(...) - entry point for any new user sign in final stage. That gets
eventually into PerformLogin() but a bit later.
However in this case I worry that user might start "Add new user" flow (if
configuration supports that) and that gets aborted with auto login timer. You
should probably stop time in that case too.


Login(...) - entry point for existing user sign in.

[dconnelly, 2013/02/11 16:17:56]

Done.

+
   // Use the same LoginPerformer for subsequent login as it has state
   // such as Authenticator instance.
   if (!login_performer_.get() || num_login_attempts_ <= 1) {
@@ -440,6 +462,9 @@ void ExistingUserController::LoginAsRetailModeUser() {
   // TODO(rkc): Add a CHECK to make sure retail mode logins are allowed once
   // the enterprise policy wiring is done for retail mode.
 
+  // Stop the autologin timer when attempting login.
+  StopAutoLoginTimer();

[bartfab (slow), 2013/02/11 09:46:01]

Just an idea: Instead of having to stop the timer all over the place, how about
you leave it running and then check whether a login attempt is already in
progress if/when it fires? This should be easy as |login_performer_| will
evaluate to |true| when a login is going on and |false| otherwise.

+
   // Only one instance of LoginPerformer should exist at a time.
   login_performer_.reset(NULL);
   login_performer_.reset(new LoginPerformer(this));
@@ -485,6 +510,9 @@ void ExistingUserController::LoginAsGuest() {
     return;
   }
 
+  // Stop the autologin timer when attempting login.
+  StopAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

Why not place this call to the beginning of this method?

[dconnelly, 2013/02/11 16:17:56]

Done.

+
   // Only one instance of LoginPerformer should exist at a time.
   login_performer_.reset(NULL);
   login_performer_.reset(new LoginPerformer(this));
@@ -532,6 +560,9 @@ void ExistingUserController::LoginAsPublicAccount(
     return;
   }
 
+  // Stop the autologin timer when attempting login.
+  StopAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

Why not place this call to the beginning of this method?

[dconnelly, 2013/02/11 16:17:56]

Done.

+
   // Only one instance of LoginPerformer should exist at a time.
   login_performer_.reset(NULL);
   login_performer_.reset(new LoginPerformer(this));
@@ -541,6 +572,11 @@ void ExistingUserController::LoginAsPublicAccount(
       l10n_util::GetStringUTF8(IDS_CHROMEOS_ACC_LOGIN_SIGNIN_PUBLIC_ACCOUNT));
 }
 
+void ExistingUserController::OnSigninScreenReady() {
+  signin_screen_ready_ = true;
+  StartAutoLoginTimer();
+}
+
 void ExistingUserController::OnUserSelected(const std::string& username) {
   login_performer_.reset(NULL);
   num_login_attempts_ = 0;
@@ -681,6 +717,8 @@ void ExistingUserController::OnLoginFailure(const LoginFailure& failure) {
 
   // Clear the recorded displayed email so it won't affect any future attempts.
   display_email_.clear();
+
+  StartAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

Are you sure this is what you want? This callback will be called for any other
user sign in attempt failure.

So current implementation stops auto-login timer in Login(...)  (PerformLogin)
but then starts that again when this user enters incorrect credentials?

[dconnelly, 2013/02/11 16:17:56]

Yes, this is the intended behavior.

[Nikita (slow), 2013/02/11 17:30:29]

I think that has some potential issues when one user has started sign in and is
in the process of migrating cryptohome from one password to another.

 }
 
 void ExistingUserController::OnLoginSuccess(
@@ -840,6 +878,9 @@ void ExistingUserController::PolicyLoadFailed() {
   login_display_->SetUIEnabled(true);
 
   display_email_.clear();
+
+  // Policy load failure stops login attempts--restart the timer.

[bartfab (slow), 2013/02/11 09:46:01]

Nit: spaces around the dash. Also, -- for an em dash is... unusual.

[dconnelly, 2013/02/11 16:17:56]

My bad, ---.  You don't need the spaces, though, according to
http://en.wikipedia.org/wiki/Dash

+  StartAutoLoginTimer();

[Nikita (slow), 2013/02/11 09:56:58]

I thought that this error is kind of unrecoverable?

[dconnelly, 2013/02/11 16:17:56]

I ran into a problem during debugging where I screwed up the policy loading on
the backend, but I was able to SSH into the VM image and also fiddle with the
policy server and fix the problem.  So it's unrecoverable at the UI but perhaps
not in the backend.

 }
 
 void ExistingUserController::OnOnlineChecked(const std::string& username,
@@ -881,6 +922,33 @@ void ExistingUserController::ActivateWizard(const std::string& screen_name) {
   host_->StartWizard(screen_name, params);
 }
 
+void ExistingUserController::StopAutoLoginTimer() {
+  auto_login_timer_.Stop();
+}
+
+void ExistingUserController::StartAutoLoginTimer() {
+  if (!signin_screen_ready_)
+    return;
+
+  std::string username;
+  if (cros_settings_->GetString(
+          kAccountsPrefDeviceLocalAccountAutoLoginId,
+          &username) && !username.empty()) {
+    int timer;
+    if (!cros_settings_->GetInteger(
+            kAccountsPrefDeviceLocalAccountAutoLoginDelay,
+            &timer)) {
+      timer = 0;
+    }
+    auto_login_timer_.Start(FROM_HERE,
+                            base::TimeDelta::FromMilliseconds(timer),
+                            base::Bind(
+                                &ExistingUserController::LoginAsPublicAccount,
+                                weak_factory_.GetWeakPtr(),
+                                username));

[bartfab (slow), 2013/02/11 09:46:01]

Nit: Rather than indenting deeper and deeper, I think it would be easier to
return when one of the conditions is not met.

[dconnelly, 2013/02/11 16:17:56]

Done.

+  }
+}
+
 gfx::NativeWindow ExistingUserController::GetNativeWindow() const {
   return host_->GetNativeWindow();
 }