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..f539da6c44ccfa4fdd0c48f9ad3296ed22801f2c 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";

[Ivan Korotkov, 2012/11/28 21:40:53]

I'm very concerned about splitting users into several lists.
Not sure whether we'll end up showing local users always first or mixed with
regular, but we still should keep an LRU list of *all* users to prevent future
migrations forth and back depending on the UI decisions.

So I suggest either:
(*) keeping *all* users in LoggedInUsers and only local users in LocalUsers or
(*) keeping 3 lists (LoggedInUsers, GaiaUsers, LocalUsers or  smth like that).

[Nikita (slow), 2012/11/29 12:00:50]

I agree with Ivan. We should have single LoggedInUsers that has users in LRU.

Do you intend to place public accounts in some specific way i.e. always first
one / always last one?

[Ivan Korotkov, 2012/11/29 12:06:31]

As discussed over chat, this CL actually adds support for public accounts, which
have fixed display order and don't need to be put into LRU list.

So it's OK to have kPublicAccounts (fixed order) and a mixed kSomethingUsers
(LRU order) with all other user types (gaia, local with cryptohome).

[bartfab (slow), 2012/11/29 14:18:09]

I renamed |LocalUsers| to |PublicAccounts|.
I left |RegularUsers| as-is for now. Once local accounts are added, we can
rethink the name - I cannot think of anything better that signifies "GAIA plus
local, persistent" right now.

+
+// 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,7 +122,9 @@ void UserManager::RegisterPrefs(PrefService* local_state) {
 }
 
 UserManagerImpl::UserManagerImpl()
-    : logged_in_user_(NULL),
+    : cros_settings_(CrosSettings::Get()),
+      users_loaded_(false),
+      logged_in_user_(NULL),
       session_started_(false),
       is_current_user_owner_(false),
       is_current_user_new_(false),
@@ -137,12 +150,18 @@ UserManagerImpl::~UserManagerImpl() {
     delete logged_in_user_;
 }
 
+void UserManagerImpl::Shutdown() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  cros_settings_->RemoveSettingsObserver(kAccountsPrefDeviceLocalAccounts,
+                                         this);
+}
+
 UserImageManager* UserManagerImpl::GetUserImageManager() {
   return user_image_manager_.get();
 }
 
 const UserList& UserManagerImpl::GetUsers() const {
-  const_cast<UserManagerImpl*>(this)->EnsureUsersLoaded();
+  EnsureUsersLoaded();
   return users_;
 }
 
@@ -168,35 +187,19 @@ 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();
+  // Remove the user from the user list.
+  logged_in_user_ = RemoveUserFromListInternal(email);
 
-  // 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;
-  }
+  // Add the user to the front of the persistent user list.
+  ListPrefUpdate prefs_users_update(g_browser_process->local_state(),
+                                    kRegularUsers);
+  prefs_users_update->Insert(0, new base::StringValue(email));
 
-  if (logged_in_user == users_.end()) {
+  // If the user was not found on the user list, create a new user.
+  if (!logged_in_user_) {
     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_);
-
-  if (is_current_user_new_) {
     SaveUserDisplayName(logged_in_user_->email(),
                         UTF8ToUTF16(logged_in_user_->GetAccountName(true)));
     WallpaperManager::Get()->SetInitialUserWallpaper(email, true);
@@ -209,9 +212,6 @@ void UserManagerImpl::UserLoggedIn(const std::string& email,
     WallpaperManager::Get()->EnsureLoggedInUserWallpaperLoaded();
   }
 

[Ivan Korotkov, 2012/11/28 21:40:53]

Why are you removing this bit?

[Nikita (slow), 2012/11/29 12:00:50]

To give more context: this call ensures that if sign out is done exactly after
new user sign in we would not lost that user from login screen.

[bartfab (slow), 2012/11/29 14:18:09]

I removed this at Julian's request. The local state is always flushed to disk on
browser shutdown, even if the user logs out straight after logging in (see
ShutdownPreThreadsStop() in browser_shutdown.cc).

[Ivan Korotkov, 2012/11/29 17:25:33]

Right, but if browser crashes in the first few seconds after session start, user
will be not saved. That's the reason this line was added in the first place.

[bartfab (slow), 2012/11/29 18:56:17]

Done.

-  // Make sure we persist new user data to Local State.
-  prefs->CommitPendingWrite();
-
   NotifyOnLogin();
 }
 
@@ -252,17 +252,14 @@ void UserManagerImpl::SessionStarted() {
       chrome::NOTIFICATION_SESSION_STARTED,
       content::NotificationService::AllSources(),
       content::NotificationService::NoDetails());
-  if (is_current_user_new_) {
-    // Make sure we persist new user data to Local State.

[Ivan Korotkov, 2012/11/28 21:40:53]

And this one.

[Nikita (slow), 2012/11/29 12:00:50]

Same here: ensures that new user avatar is saved immediately.

[bartfab (slow), 2012/11/29 14:18:09]

Same as above.

[bartfab (slow), 2012/11/29 18:56:17]

Done.

-    g_browser_process->local_state()->CommitPendingWrite();
-  }
 }
 
 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 +280,8 @@ void UserManagerImpl::RemoveUser(const std::string& email,
 void UserManagerImpl::RemoveUserFromList(const std::string& email) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   EnsureUsersLoaded();
-  RemoveUserFromListInternal(email);
+  RemoveNonCryptohomeData(email);
+  delete RemoveUserFromListInternal(email);
 }
 
 bool UserManagerImpl::IsKnownUser(const std::string& email) const {
@@ -429,13 +427,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 +487,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 +560,89 @@ void UserManagerImpl::NotifyLocalStateChanged() {
                     LocalStateChanged(this));
 }
 
-void UserManagerImpl::EnsureUsersLoaded() {
+bool UserManagerImpl::ParseUserList(

[Ivan Korotkov, 2012/11/28 21:40:53]

Since this is a utility method that has nothing to do with UserManager's state,
better make it a local function in namespace {}

[bartfab (slow), 2012/11/29 14:18:09]

Done.

+    const ListValue& users_list,
+    const std::set<std::string>& existing_users,
+    const std::string& logged_in_user,
+    std::vector<std::string>* users_vector,
+    std::set<std::string>* users_set) 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 at index " << i << ".";
+      continue;
+    }
+    if (existing_users.find(email) != existing_users.end() ||
+        !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() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!users_.empty())
-    return;
   if (!g_browser_process)
     return;
 
+  if (users_loaded_)
+    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(*prefs_regular_users, std::set<std::string>(), "",
+                &regular_users, &regular_users_set);
+  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(*prefs_local_users,regular_users_set, "",
+                &local_users, &local_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));

[Ivan Korotkov, 2012/11/28 21:40:53]

Maybe CreateGaiaAccountUser? Public sounds somewhat counterintuitive.

[bartfab (slow), 2012/11/29 14:18:09]

As discussed offline, public accounts are non-GAIA.

+  }
+
   user_image_manager_->LoadUserImages(users_);
 }
 
@@ -600,42 +650,50 @@ 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);
+
+  EnsureUsersLoaded();
+
+  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);
+        delete RemoveUserFromListInternal(user_email);
         changed = true;
       }
     }
+  }
 
-    if (changed) {
-      content::NotificationService::current()->Notify(
-          chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED,
-          content::Source<UserManager>(this),
-          content::NotificationService::NoDetails());
-    }
+  if (changed) {
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_POLICY_USER_LIST_CHANGED,
+        content::Source<UserManager>(this),
+        content::NotificationService::NoDetails());
   }
+
+  cros_settings_->AddSettingsObserver(kAccountsPrefDeviceLocalAccounts,
+                                      this);
 }
 
 bool UserManagerImpl::AreEphemeralUsersEnabled() const {
@@ -681,24 +739,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 +754,110 @@ 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::RemoveUserFromListInternal(const std::string& email) {

[Ivan Korotkov, 2012/11/28 21:40:53]

This looks like it really is RemoveRegularUser...?

[bartfab (slow), 2012/11/29 14:18:09]

Done. Though note that the method will handle other user types properly - but
you are right, it is intended to be used with regular users.

+  ListPrefUpdate prefs_users_update(g_browser_process->local_state(),
+                                    kRegularUsers);
+  prefs_users_update->Clear();
+  User* user = NULL;
+  for (UserList::iterator it = users_.begin(); it != users_.end(); ) {
+    const std::string user_email = (*it)->email();
+    if (user_email == email) {
+      user = *it;
+      it = users_.erase(it);
+    } else {
+      if ((*it)->GetType() == User::USER_TYPE_REGULAR)
+        prefs_users_update->Append(new base::StringValue(user_email));
+      ++it;
+    }
   }
+  return user;
+}
+
+bool UserManagerImpl::UpdateLocalUsers(
+    const base::ListValue& local_users_list) {
+  PrefService* local_state = g_browser_process->local_state();
+
+  // Determine the currently logged-in user's email.
+  std::string logged_in_user;

[Nikita (slow), 2012/11/29 16:42:50]

Please rename to logged_in_user_email so that you don't confuse with
logged_in_user_

[bartfab (slow), 2012/11/29 18:56:17]

Done.

+  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(local_users_list, regular_users, logged_in_user,
+                     &new_local_users, &new_local_users_set) &&
+      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