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..af7d483cae3f7e6b71c31c142fae5da56e056e7b 100644
--- a/chrome/browser/chromeos/login/user_manager_impl.cc
+++ b/chrome/browser/chromeos/login/user_manager_impl.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/chromeos/login/user_manager_impl.h"
 
+#include <cstddef>
+#include <set>
+#include <vector>
+
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
@@ -23,7 +27,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 +47,18 @@ namespace chromeos {
 
 namespace {
 
-// A vector pref of the users who have logged into the device.
-const char kLoggedInUsers[] = "LoggedInUsers";
+// A vector pref of the the regular users known on this device, arranged in LRU
+// order.
+const char kRegularUsers[] = "LoggedInUsers";
+
+// A vector pref of the public accounts defined on this device.
+const char kPublicAccounts[] = "PublicAccounts";
+
+// A string pref that gets set when a public account is removed but a user is
+// currently logged into that account, requiring the account's data to be
+// removed after logout.
+const char kPublicAccountPendingDataRemoval[] =
+    "PublicAccountPendingDataRemoval";
 
 // A dictionary that maps usernames to the displayed name.
 const char kUserDisplayName[] = "UserDisplayName";
@@ -97,11 +110,47 @@ void RemoveUserInternal(const std::string& user_email,
     delegate->OnUserRemoved(user_email);
 }
 
+// Helper function that copies users from |users_list| to |users_vector| and
+// |users_set|. Duplicates and users already present in |existing_users| are
+// skipped. The |logged_in_user| is also skipped and the return value
+// indicates whether that user was found in |users_list|.
+bool ParseUserList(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) {
+  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;
+}
+
 }  // namespace
 
 // static
 void UserManager::RegisterPrefs(PrefService* local_state) {
-  local_state->RegisterListPref(kLoggedInUsers, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterListPref(kRegularUsers, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterListPref(kPublicAccounts, PrefService::UNSYNCABLE_PREF);
+  local_state->RegisterStringPref(kPublicAccountPendingDataRemoval, "",
+                                  PrefService::UNSYNCABLE_PREF);
   local_state->RegisterDictionaryPref(kUserOAuthTokenStatus,
                                       PrefService::UNSYNCABLE_PREF);
   local_state->RegisterDictionaryPref(kUserDisplayName,
@@ -111,7 +160,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),
@@ -130,11 +181,19 @@ UserManagerImpl::UserManagerImpl()
 
 UserManagerImpl::~UserManagerImpl() {
   // Can't use STLDeleteElements because of the private destructor of User.
-  for (size_t i = 0; i < users_.size(); ++i)
-    delete users_[i];
-  users_.clear();
-  if (is_current_user_ephemeral_)
-    delete logged_in_user_;
+  for (UserList::iterator it = users_.begin(); it != users_.end();
+       it = users_.erase(it)) {
+    if (logged_in_user_ == *it)
+      logged_in_user_ = NULL;

[bartfab (slow), 2012/12/03 10:39:21]

This makes things more robust: The |logged_in_user_| should be deleted iff that
user is not deleted as part of the |users_| clean-up. Rather than relying on a
somewhat unrelated flag that happens to be |true| if the user is not on the
list, explicitly compare the |users_| entries to |logged_in_user_|.

[Ivan Korotkov, 2012/12/03 11:04:45]

Hmm, so it looks like now |logged_in_user| can point to somewhere else outside
of |users_| or a static stub user? In that case, please update it's comment in
header file.

[bartfab (slow), 2012/12/03 11:34:12]

The description in user_manager_impl.h is still accurate. The |logged_in_user_|
can be one of the |users_| instances or an ephemeral user. There were three
types of ephemeral users before this CL: stub, guest, users made ephemeral by
policy. This CL adds a fourth type of ephemeral user, the public account user.

I still think it is safer to base the deletion on whether the user is found in
the |users_| list or not. That way, we can guarantee ro clean up all users and
we can ensure that there will be no double-free accidents here.

+    delete *it;
+  }
+  delete logged_in_user_;
+}
+
+void UserManagerImpl::Shutdown() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  cros_settings_->RemoveSettingsObserver(kAccountsPrefDeviceLocalAccounts,
+                                         this);
 }
 
 UserImageManager* UserManagerImpl::GetUserImageManager() {
@@ -168,40 +227,25 @@ 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_ = RemoveRegularUserFromList(email);

[bartfab (slow), 2012/12/03 10:39:21]

This caused a memory leak because my reading of the original code was mistaken:
The logged-in user should be added to both the persistent list (in prefs) and
the |users_| list (in memory). Lines 246 and 247 take care of this now.

 
-  // 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()) {
+  // 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);
   }
 
+  // Add the user to the front of the user list.
+  ListPrefUpdate prefs_users_update(g_browser_process->local_state(),
+                                    kRegularUsers);
+  prefs_users_update->Insert(0, new base::StringValue(email));
+  users_.insert(users_.begin(), logged_in_user_);
+
   user_image_manager_->UserLoggedIn(email, is_current_user_new_);
 
   if (!browser_restart) {
@@ -209,8 +253,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 that new data is persisted to Local State.
+  g_browser_process->local_state()->CommitPendingWrite();
 
   NotifyOnLogin();
 }
@@ -253,7 +297,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 that the new user's data is persisted to Local State.
     g_browser_process->local_state()->CommitPendingWrite();
   }
 }
@@ -262,7 +306,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 +328,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);
+  delete RemoveRegularUserFromList(email);
+  // Make sure that new data is persisted to Local State.
+  g_browser_process->local_state()->CommitPendingWrite();
 }
 
 bool UserManagerImpl::IsKnownUser(const std::string& email) const {
@@ -429,13 +477,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 +537,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() &&
@@ -553,46 +612,56 @@ void UserManagerImpl::NotifyLocalStateChanged() {
 
 void UserManagerImpl::EnsureUsersLoaded() {
   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_public_accounts =
+      local_state->GetList(kPublicAccounts);
   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 public accounts.
+  std::vector<std::string> public_accounts;
+  std::set<std::string> public_accounts_set;
+  ParseUserList(*prefs_public_accounts, regular_users_set, "",
+                &public_accounts, &public_accounts_set);
+  for (std::vector<std::string>::const_iterator it = public_accounts.begin();
+       it != public_accounts.end(); ++it) {
+    users_.push_back(User::CreatePublicAccountUser(*it));
+  }
+
   user_image_manager_->LoadUserImages(users_);
 }
 
@@ -600,42 +669,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* public_accounts;
+  cros_settings_->GetList(kAccountsPrefDeviceLocalAccounts, &public_accounts);
+
+  EnsureUsersLoaded();
+
+  bool changed = UpdateAndCleanUpPublicAccounts(*public_accounts);
 
   // 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.
+  // opportunity to clean up by removing all regular users except the owner.
   if (ephemeral_users_enabled_  && !IsUserLoggedIn()) {
-    scoped_ptr<base::ListValue> users(
-        g_browser_process->local_state()->GetList(kLoggedInUsers)->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);
+    ListPrefUpdate prefs_users_update(g_browser_process->local_state(),
+                                      kRegularUsers);
+    prefs_users_update->Clear();
+    for (UserList::iterator it = users_.begin(); it != users_.end(); ) {
+      const std::string user_email = (*it)->email();
+      if ((*it)->GetType() == User::USER_TYPE_REGULAR &&
+          user_email != owner_email_) {
+        RemoveNonCryptohomeData(user_email);
+        delete *it;
+        it = users_.erase(it);
         changed = true;
+      } else {
+        prefs_users_update->Append(new base::StringValue(user_email));
+        ++it;
       }
     }
+  }
 
-    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 +761,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 +776,112 @@ void UserManagerImpl::RemoveUserFromListInternal(const std::string& email) {
 
   DictionaryPrefUpdate prefs_display_email_update(prefs, kUserDisplayEmail);
   prefs_display_email_update->RemoveWithoutPathExpansion(email, NULL);
+}
+
+User *UserManagerImpl::RemoveRegularUserFromList(const std::string& email) {
+  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;
+}
 
-  if (user_to_remove != users_.end()) {
-    delete *user_to_remove;
-    users_.erase(user_to_remove);
+bool UserManagerImpl::UpdateAndCleanUpPublicAccounts(
+    const base::ListValue& public_accounts) {
+  PrefService* local_state = g_browser_process->local_state();
+
+  // Determine the currently logged-in user's email.
+  std::string logged_in_user_email;
+  if (IsUserLoggedIn())
+    logged_in_user_email = GetLoggedInUser()->email();
+
+  // If there is a public account whose data is pending removal and the user is
+  // not currently logged in with that account, take this opportunity to remove
+  // the data.
+  std::string public_account_pending_data_removal =
+      local_state->GetString(kPublicAccountPendingDataRemoval);
+  if (!public_account_pending_data_removal.empty() &&
+      public_account_pending_data_removal != logged_in_user_email) {
+    RemoveNonCryptohomeData(public_account_pending_data_removal);
+    local_state->ClearPref(kPublicAccountPendingDataRemoval);
+  }
+
+  // Split the current user list public accounts and regular users.
+  std::vector<std::string> old_public_accounts;
+  std::set<std::string> regular_users;
+  for (UserList::const_iterator it = users_.begin(); it != users_.end(); ++it) {
+    if ((*it)->GetType() == User::USER_TYPE_PUBLIC_ACCOUNT)
+      old_public_accounts.push_back((*it)->email());
+    else
+      regular_users.insert((*it)->email());
+  }
+
+  // Get the new list of public accounts from policy.
+  std::vector<std::string> new_public_accounts;
+  std::set<std::string> new_public_accounts_set;
+  if (!ParseUserList(public_accounts, regular_users, logged_in_user_email,
+                     &new_public_accounts, &new_public_accounts_set) &&
+      IsUserLoggedIn()) {
+    User* user = GetLoggedInUser();
+    // If the user is currently logged into a public account that has been
+    // removed from the list, mark the account's data as pending removal after
+    // logout.
+    if (user->GetType() == User::USER_TYPE_PUBLIC_ACCOUNT) {
+      local_state->SetString(kPublicAccountPendingDataRemoval,
+                             logged_in_user_email);
+    }
+  }
+
+  // Persist the new list of public accounts in a pref.
+  ListPrefUpdate prefs_public_accounts_update(local_state, kPublicAccounts);
+  scoped_ptr<base::ListValue> prefs_public_accounts(public_accounts.DeepCopy());
+  prefs_public_accounts_update->Swap(prefs_public_accounts.get());
+
+  // If the list of public accounts has not changed, return.
+  if (new_public_accounts.size() == old_public_accounts.size()) {
+    bool changed = false;
+    for (size_t i = 0; i < new_public_accounts.size(); ++i) {
+      if (new_public_accounts[i] != old_public_accounts[i]) {
+        changed = true;
+        break;
+      }
+    }
+    if (!changed)
+      return false;
   }
+
+  // Remove the old public accounts from the user list.
+  for (UserList::iterator it = users_.begin(); it != users_.end(); ) {
+    if ((*it)->GetType() == User::USER_TYPE_PUBLIC_ACCOUNT) {
+      delete *it;
+      it = users_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
+  // Add the new public accounts to the front of the user list.
+  for (std::vector<std::string>::const_reverse_iterator
+           it = new_public_accounts.rbegin();
+       it != new_public_accounts.rend(); ++it) {
+    users_.insert(users_.begin(), User::CreatePublicAccountUser(*it));
+  }
+
+  user_image_manager_->LoadUserImages(
+      UserList(users_.begin(), users_.begin() + new_public_accounts.size()));
+
+  return true;
 }
 
 }  // namespace chromeos