Add public accounts to UserManager

chrome/browser/chromeos/login/user_manager_impl.cc

Index: chrome/browser/chromeos/login/user_manager_impl.cc
diff --git a/chrome/browser/chromeos/login/user_manager_impl.cc b/chrome/browser/chromeos/login/user_manager_impl.cc
index 14882c0688b8835c871a5ca38420c6e5a69598a6..853784d954093ae32c804c2d7fb81af61a34d90d 100644
--- a/chrome/browser/chromeos/login/user_manager_impl.cc
+++ b/chrome/browser/chromeos/login/user_manager_impl.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/login/user_manager_impl.h"
 
+#include <cstddef>
+
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
@@ -23,7 +25,6 @@
 #include "chrome/browser/chromeos/login/remove_user_delegate.h"
 #include "chrome/browser/chromeos/login/user_image_manager_impl.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
-#include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/policy/browser_policy_connector.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
@@ -44,8 +45,15 @@ namespace chromeos {
 
 namespace {
 
-// A vector pref of the users who have logged into the device.
-const char kLoggedInUsers[] = "LoggedInUsers";
+// A vector pref of the regular users who have logged into this device.
+const char kRegularUsers[] = "LoggedInUsers";
+
+// A vector pref of the device-local users defined on this device.
+const char kLocalUsers[] = "LocalUsers";
+
+// A string pref that gets set when a device-local user is to be removed but is
+// currently logged in, requiring the user's data to be removed after logout.
+const char kUserPendingDataRemoval[] = "UserPendingDataRemoval";
 
 // A dictionary that maps usernames to the displayed name.
 const char kUserDisplayName[] = "UserDisplayName";
@@ -101,7 +109,10 @@ void RemoveUserInternal(const std::string& user_email,
 
 // static
 void UserManager::RegisterPrefs(PrefService* local_state) {
-  local_state->RegisterListPref(kLoggedInUsers, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterListPref(kRegularUsers, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterListPref(kLocalUsers, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterStringPref(kUserPendingDataRemoval, "",
+                                  PrefService::UNSYNCABLE_PREF);
   local_state->RegisterDictionaryPref(kUserOAuthTokenStatus,
                                       PrefService::UNSYNCABLE_PREF);
   local_state->RegisterDictionaryPref(kUserDisplayName,
@@ -111,14 +122,18 @@ void UserManager::RegisterPrefs(PrefService* local_state) {
 }
 
 UserManagerImpl::UserManagerImpl()
-    : logged_in_user_(NULL),
+    : cros_settings_(CrosSettings::Get()),
+      observing_cros_settings_(false),
+      users_loaded_(false),
+      logged_in_user_(NULL),
       session_started_(false),
       is_current_user_owner_(false),
       is_current_user_new_(false),
       is_current_user_ephemeral_(false),
       ephemeral_users_enabled_(false),
       observed_sync_service_(NULL),
-      user_image_manager_(new UserImageManagerImpl) {
+      user_image_manager_(new UserImageManagerImpl),
+      weak_ptr_factory_(this) {
   // UserManager instance should be used only on UI thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   registrar_.Add(this, chrome::NOTIFICATION_OWNERSHIP_STATUS_CHANGED,
@@ -168,33 +183,8 @@ void UserManagerImpl::UserLoggedIn(const std::string& email,
 
   EnsureUsersLoaded();
 
-  // Clear the prefs view of the users.
-  PrefService* prefs = g_browser_process->local_state();
-  ListPrefUpdate prefs_users_update(prefs, kLoggedInUsers);
-  prefs_users_update->Clear();
-
-  // Make sure this user is first.
-  prefs_users_update->Append(new base::StringValue(email));
-  UserList::iterator logged_in_user = users_.end();
-  for (UserList::iterator it = users_.begin(); it != users_.end(); ++it) {
-    std::string user_email = (*it)->email();
-    // Skip the most recent user.
-    if (email != user_email)
-      prefs_users_update->Append(new base::StringValue(user_email));
-    else
-      logged_in_user = it;
-  }
-
-  if (logged_in_user == users_.end()) {
-    is_current_user_new_ = true;
-    logged_in_user_ = User::CreateRegularUser(email);
-    logged_in_user_->set_oauth_token_status(LoadUserOAuthStatus(email));
-  } else {
-    logged_in_user_ = *logged_in_user;
-    users_.erase(logged_in_user);
-  }
-  // This user must be in the front of the user list.
-  users_.insert(users_.begin(), logged_in_user_);
+  // Move the user to the front of the user list (adding the user if necessary).
+  logged_in_user_ = UpdateRegularUsers(email, true);
 
   if (is_current_user_new_) {
     SaveUserDisplayName(logged_in_user_->email(),
@@ -209,8 +199,8 @@ void UserManagerImpl::UserLoggedIn(const std::string& email,
     WallpaperManager::Get()->EnsureLoggedInUserWallpaperLoaded();
   }
 
-  // Make sure we persist new user data to Local State.
-  prefs->CommitPendingWrite();
+  // Make sure the modified data is persisted to Local State.
+  g_browser_process->local_state()->CommitPendingWrite();
 
   NotifyOnLogin();
 }
@@ -253,7 +243,7 @@ void UserManagerImpl::SessionStarted() {
       content::NotificationService::AllSources(),
       content::NotificationService::NoDetails());
   if (is_current_user_new_) {
-    // Make sure we persist new user data to Local State.
+    // Make sure the modified data is persisted to Local State.

[pastarmovj, 2012/11/28 09:57:16]

I am not convinced we need all those CommitPendingWrites all over this file. The
local state is persisted automatically often enough and those writes can only
slow down login for new users. I guess they have been added in an attempt to
prevent crashes in the browser from wiping new users but I think that instead of
doing that we should be more diligent in not allowing crashes in the first
place.

[bartfab (slow), 2012/11/28 12:30:17]

I removed the CommitPendingWrite() calls. I agree that relying on prefs doing
the right thing is much more elegant.

     g_browser_process->local_state()->CommitPendingWrite();
   }
 }
@@ -262,7 +252,8 @@ void UserManagerImpl::RemoveUser(const std::string& email,
                                  RemoveUserDelegate* delegate) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (!IsKnownUser(email))
+  const User* user = FindUser(email);
+  if (!user || user->GetType() != User::USER_TYPE_REGULAR)
     return;
 
   // Sanity check: we must not remove single user. This check may seem
@@ -283,7 +274,10 @@ void UserManagerImpl::RemoveUser(const std::string& email,
 void UserManagerImpl::RemoveUserFromList(const std::string& email) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   EnsureUsersLoaded();
-  RemoveUserFromListInternal(email);
+  RemoveNonCryptohomeData(email);
+  UpdateRegularUsers(email, false);

[pastarmovj, 2012/11/28 09:57:16]

I have a proposal about the second parameter of this function. Why not make it
an enum so that you can have more descriptive names for the operations. It seems
quite strange to me that depending on this param being true or false the
behavior changes so vastly.

[bartfab (slow), 2012/11/28 12:30:17]

I refactored the code. There is now a |RemoveUserFromListInternal| method that
does what it says on the tin only. If the user needs to be added at the front of
the list, that is done explicitly outside the method.

+  // Make sure the modified data is persisted to Local State.
+  g_browser_process->local_state()->CommitPendingWrite();
 }
 
 bool UserManagerImpl::IsKnownUser(const std::string& email) const {
@@ -429,13 +423,18 @@ void UserManagerImpl::Observe(int type,
         }
       }
       break;
+    case chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED:
+      DCHECK_EQ(*content::Details<const std::string>(details).ptr(),
+                kAccountsPrefDeviceLocalAccounts);
+      RetrieveTrustedDevicePolicies();
+      break;
     default:
       NOTREACHED();
   }
 }
 
 void UserManagerImpl::OnStateChanged() {
-  DCHECK(IsUserLoggedIn() && !IsLoggedInAsGuest());
+  DCHECK(IsLoggedInAsRegularUser());
   GoogleServiceAuthError::State state =
       observed_sync_service_->GetAuthError().state();
   if (state != GoogleServiceAuthError::NONE &&
@@ -484,6 +483,12 @@ bool UserManagerImpl::IsUserLoggedIn() const {
   return logged_in_user_;
 }
 
+bool UserManagerImpl::IsLoggedInAsRegularUser() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return IsUserLoggedIn() &&
+         logged_in_user_->GetType() == User::USER_TYPE_REGULAR;
+}
+
 bool UserManagerImpl::IsLoggedInAsDemoUser() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return IsUserLoggedIn() &&
@@ -551,48 +556,88 @@ void UserManagerImpl::NotifyLocalStateChanged() {
                     LocalStateChanged(this));
 }
 
+bool UserManagerImpl::ParseUserList(
+    std::vector<std::string>* users_vector,
+    std::set<std::string>* users_set,
+    const ListValue& users_list,
+    const std::set<std::string>& existing_users,
+    const std::string& logged_in_user) const {
+  users_vector->clear();
+  users_set->clear();
+  bool logged_in_user_on_list = false;
+  for (size_t i = 0; i < users_list.GetSize(); ++i) {
+    std::string email;
+    if (!users_list.GetString(i, &email) || email.empty()) {
+      LOG(ERROR) << "Corrupt entry in user list.";

[pastarmovj, 2012/11/28 09:57:16]

Please print the index here too. Could be helpful for some debugging.

[bartfab (slow), 2012/11/28 12:30:17]

Done.

+      continue;
+    }
+    if ((existing_users.find(email) != existing_users.end()) ||

[pastarmovj, 2012/11/28 09:57:16]

I think you don't need the extra parenthesis around a != b.

[bartfab (slow), 2012/11/28 12:30:17]

Correct. I added them for readability only. Removed now.

+        !users_set->insert(email).second) {
+      LOG(ERROR) << "Duplicate user: " << email;
+      continue;
+    }
+    if (email == logged_in_user) {
+      logged_in_user_on_list = true;
+      continue;
+    }
+    users_vector->push_back(email);
+  }
+  users_set->erase(logged_in_user);
+  return logged_in_user_on_list;
+}
+
 void UserManagerImpl::EnsureUsersLoaded() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!users_.empty())
+  if (users_loaded_)
     return;
   if (!g_browser_process)

[pastarmovj, 2012/11/28 09:57:16]

Swap the two ifs and split them with an empty line please. The one is a
precondition and the other is part of the functionality of this proc.

[bartfab (slow), 2012/11/28 12:30:17]

Done.

     return;
+  users_loaded_ = true;
 
   PrefService* local_state = g_browser_process->local_state();
-  const ListValue* prefs_users =
-      local_state->GetList(kLoggedInUsers);
+  const ListValue* prefs_regular_users = local_state->GetList(kRegularUsers);
+  const ListValue* prefs_local_users = local_state->GetList(kLocalUsers);
   const DictionaryValue* prefs_display_names =
       local_state->GetDictionary(kUserDisplayName);
   const DictionaryValue* prefs_display_emails =
       local_state->GetDictionary(kUserDisplayEmail);
 
-  if (!prefs_users)
-    return;
-
-  for (ListValue::const_iterator it = prefs_users->begin();
-       it != prefs_users->end(); ++it) {
-    std::string email;
-    if ((*it)->GetAsString(&email)) {
-      User* user = User::CreateRegularUser(email);
-      user->set_oauth_token_status(LoadUserOAuthStatus(email));
-      users_.push_back(user);
-
-      string16 display_name;
-      if (prefs_display_names &&
-          prefs_display_names->GetStringWithoutPathExpansion(
-              email, &display_name)) {
-        user->set_display_name(display_name);
-      }
+  // Load regular users.
+  std::vector<std::string> regular_users;
+  std::set<std::string> regular_users_set;
+  ParseUserList(&regular_users, &regular_users_set, *prefs_regular_users,
+                std::set<std::string>(), "");
+  for (std::vector<std::string>::const_iterator it = regular_users.begin();
+       it != regular_users.end(); ++it) {
+    User* user = User::CreateRegularUser(*it);
+    user->set_oauth_token_status(LoadUserOAuthStatus(*it));
+    users_.push_back(user);
+
+    string16 display_name;
+    if (prefs_display_names->GetStringWithoutPathExpansion(*it,
+                                                           &display_name)) {
+      user->set_display_name(display_name);
+    }
 
-      std::string display_email;
-      if (prefs_display_emails &&
-          prefs_display_emails->GetStringWithoutPathExpansion(
-              email, &display_email)) {
-        user->set_display_email(display_email);
-      }
+    std::string display_email;
+    if (prefs_display_emails->GetStringWithoutPathExpansion(*it,
+                                                            &display_email)) {
+      user->set_display_email(display_email);
     }
   }
 
+  // Load device-local users.
+  std::vector<std::string> local_users;
+  std::set<std::string> local_users_set;
+  ParseUserList(&local_users, &local_users_set, *prefs_local_users,
+                regular_users_set, "");
+  for (std::vector<std::string>::const_iterator it = local_users.begin();
+       it != local_users.end(); ++it) {
+    // Currently, all device-local users are public account users. This may
+    // change in the future.
+    users_.push_back(User::CreatePublicAccountUser(*it));
+  }
+
   user_image_manager_->LoadUserImages(users_);
 }
 
@@ -600,41 +645,53 @@ void UserManagerImpl::RetrieveTrustedDevicePolicies() {
   ephemeral_users_enabled_ = false;
   owner_email_ = "";
 
-  CrosSettings* cros_settings = CrosSettings::Get();
   // Schedule a callback if device policy has not yet been verified.
-  if (CrosSettingsProvider::TRUSTED != cros_settings->PrepareTrustedValues(
+  if (CrosSettingsProvider::TRUSTED != cros_settings_->PrepareTrustedValues(
       base::Bind(&UserManagerImpl::RetrieveTrustedDevicePolicies,
                  base::Unretained(this)))) {
     return;
   }
 
-  cros_settings->GetBoolean(kAccountsPrefEphemeralUsersEnabled,
-                            &ephemeral_users_enabled_);
-  cros_settings->GetString(kDeviceOwner, &owner_email_);
+  cros_settings_->GetBoolean(kAccountsPrefEphemeralUsersEnabled,
+                             &ephemeral_users_enabled_);
+  cros_settings_->GetString(kDeviceOwner, &owner_email_);
+  const base::ListValue* local_users;
+  cros_settings_->GetList(kAccountsPrefDeviceLocalAccounts, &local_users);
+
+  bool changed = UpdateLocalUsers(*local_users);
 
   // If ephemeral users are enabled and we are on the login screen, take this
   // opportunity to clean up by removing all users except the owner.
   if (ephemeral_users_enabled_  && !IsUserLoggedIn()) {
     scoped_ptr<base::ListValue> users(
-        g_browser_process->local_state()->GetList(kLoggedInUsers)->DeepCopy());
+        g_browser_process->local_state()->GetList(kRegularUsers)->DeepCopy());
 
-    bool changed = false;
     for (base::ListValue::const_iterator user = users->begin();
         user != users->end(); ++user) {
       std::string user_email;
       (*user)->GetAsString(&user_email);
       if (user_email != owner_email_) {
-        RemoveUserFromListInternal(user_email);
+        RemoveNonCryptohomeData(user_email);
+        UpdateRegularUsers(user_email, false);
         changed = true;
       }
     }
+  }
 
-    if (changed) {
-      content::NotificationService::current()->Notify(
-          chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED,
-          content::Source<UserManager>(this),
-          content::NotificationService::NoDetails());
-    }
+  // Make sure the modified data is persisted to Local State.
+  g_browser_process->local_state()->CommitPendingWrite();
+
+  if (changed) {
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED,
+        content::Source<UserManager>(this),
+        content::NotificationService::NoDetails());
+  }
+
+  if (!observing_cros_settings_) {
+    cros_settings_->AddSettingsObserver(kAccountsPrefDeviceLocalAccounts,

[pastarmovj, 2012/11/28 09:57:16]

Is there any reason you don't simply do this in the constructor and spare the
bool flag?

[bartfab (slow), 2012/11/28 12:30:17]

The RetrieveTrustedDevicePolicies() will always run when policy first becomes
available. If I registered as an observer in the constructor, I would also
receive a notification that the list of device-local accounts has changed at
that point, running RetrieveTrustedDevicePolicies() once more for no good
reason. By registering only after RetrieveTrustedDevicePolicies() has
successfully run once, I never receive the first notification.

I switched away from WeakPtr and as such, no longer need the bool. I simplified
the code accordingly.

+                                        weak_ptr_factory_.GetWeakPtr());
+    observing_cros_settings_ = true;
   }
 }
 
@@ -681,24 +738,11 @@ void UserManagerImpl::CheckOwnership() {
                  base::Unretained(this)));
 }
 
-void UserManagerImpl::RemoveUserFromListInternal(const std::string& email) {
-  // Clear the prefs view of the users.
-  PrefService* prefs = g_browser_process->local_state();
-  ListPrefUpdate prefs_users_update(prefs, kLoggedInUsers);
-  prefs_users_update->Clear();
-
-  UserList::iterator user_to_remove = users_.end();
-  for (UserList::iterator it = users_.begin(); it != users_.end(); ++it) {
-    std::string user_email = (*it)->email();
-    // Skip user that we would like to delete.
-    if (email != user_email)
-      prefs_users_update->Append(new base::StringValue(user_email));
-    else
-      user_to_remove = it;
-  }
-
+void UserManagerImpl::RemoveNonCryptohomeData(const std::string& email) {
   WallpaperManager::Get()->RemoveUserWallpaperInfo(email);
+  user_image_manager_->DeleteUserImage(email);
 
+  PrefService* prefs = g_browser_process->local_state();
   DictionaryPrefUpdate prefs_oauth_update(prefs, kUserOAuthTokenStatus);
   int oauth_status;
   prefs_oauth_update->GetIntegerWithoutPathExpansion(email, &oauth_status);
@@ -709,11 +753,127 @@ void UserManagerImpl::RemoveUserFromListInternal(const std::string& email) {
 
   DictionaryPrefUpdate prefs_display_email_update(prefs, kUserDisplayEmail);
   prefs_display_email_update->RemoveWithoutPathExpansion(email, NULL);
+}
 
-  if (user_to_remove != users_.end()) {
-    delete *user_to_remove;
-    users_.erase(user_to_remove);
+User *UserManagerImpl::UpdateRegularUsers(const std::string& email,
+                                          bool move_to_front) {
+  ListPrefUpdate prefs_users_update(g_browser_process->local_state(),
+                                    kRegularUsers);
+  prefs_users_update->Clear();
+  UserList::iterator user_it = users_.end();
+  for (UserList::iterator it = users_.begin(); it != users_.end(); ++it) {
+    const std::string user_email = (*it)->email();
+    if (user_email == email)
+      user_it = it;
+    else if ((*it)->GetType() == User::USER_TYPE_REGULAR)
+      prefs_users_update->Append(new base::StringValue(user_email));
   }
+  User* user = NULL;
+  if (user_it != users_.end()) {
+    user = *user_it;
+    users_.erase(user_it);
+  }
+
+  if (move_to_front) {
+    prefs_users_update->Insert(0, new base::StringValue(email));
+    if (!user) {

[pastarmovj, 2012/11/28 09:57:16]

Please also document that this function will actually create a new user if it
doesn't exist.

[bartfab (slow), 2012/11/28 12:30:17]

I refactored the code so that this function no longer creates users.

+      is_current_user_new_ = true;
+      user = User::CreateRegularUser(email);
+      user->set_oauth_token_status(LoadUserOAuthStatus(email));
+    }
+    users_.insert(users_.begin(), user);
+  } else {
+    delete user;
+    user = NULL;
+  }
+
+  return user;
+}
+
+bool UserManagerImpl::UpdateLocalUsers(
+    const base::ListValue& local_users_list) {
+  PrefService* local_state = g_browser_process->local_state();
+
+  EnsureUsersLoaded();
+
+  // Determine the currently logged-in user's email.
+  std::string logged_in_user;
+  if (IsUserLoggedIn())
+    logged_in_user = GetLoggedInUser()->email();
+
+  // If there is a user whose data is pending removal and that user is not
+  // currently logged in, take this opportunity to remove the data.
+  std::string user_pending_data_removal =
+      local_state->GetString(kUserPendingDataRemoval);
+  if (!user_pending_data_removal.empty() &&
+      user_pending_data_removal != logged_in_user) {
+    RemoveNonCryptohomeData(user_pending_data_removal);
+    local_state->ClearPref(kUserPendingDataRemoval);
+  }
+
+  // Split the current user list into device-local users and regular users.
+  std::vector<std::string> old_local_users;
+  std::set<std::string> regular_users;
+  for (UserList::const_iterator it = users_.begin(); it != users_.end(); ++it) {
+    if ((*it)->is_device_local_account())
+      old_local_users.push_back((*it)->email());
+    else
+      regular_users.insert((*it)->email());
+  }
+
+  // Get the new list of device-local users from policy.
+  std::vector<std::string> new_local_users;
+  std::set<std::string> new_local_users_set;
+  std::string pending_remove;
+  if (!ParseUserList(&new_local_users, &new_local_users_set, local_users_list,
+                     regular_users, logged_in_user) && IsUserLoggedIn()) {
+    User* user = GetLoggedInUser();
+    // If the currently logged-in user is a device-local user not found on the
+    // new list, mark that user's data as pending removal after logout.
+    if (user->is_device_local_account() && !user->is_builtin_account())
+      local_state->SetString(kUserPendingDataRemoval, logged_in_user);
+  }
+
+  // Persist the new list of device-local users in a pref.
+  ListPrefUpdate prefs_local_users_update(local_state, kLocalUsers);
+  scoped_ptr<base::ListValue> prefs_local_users(local_users_list.DeepCopy());
+  prefs_local_users_update->Swap(prefs_local_users.get());
+
+  // If the list of device-local users has not changed, return.
+  if (new_local_users.size() == old_local_users.size()) {
+    bool changed = false;
+    for (size_t i = 0; i < new_local_users.size(); ++i) {
+      if (new_local_users[i] != old_local_users[i]) {
+        changed = true;
+        break;
+      }
+    }
+    if (!changed)
+      return false;
+  }
+
+  // Remove the old device-local users from the user list.
+  for (UserList::iterator it = users_.begin(); it != users_.end(); ) {
+    if ((*it)->is_device_local_account()) {
+      delete *it;
+      it = users_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
+  // Add the new device-local users to the user list.
+  for (std::vector<std::string>::const_iterator it = new_local_users.begin();
+       it != new_local_users.end(); ++it) {
+    // Currently, all device-local users are public account users. This may
+    // change in the future.
+    users_.push_back(User::CreatePublicAccountUser(*it));
+  }
+
+  user_image_manager_->LoadUserImages(
+      UserList(users_.end() - new_local_users.size(), users_.end()));
+
+  return true;
 }
 
 }  // namespace chromeos