SupervisedUsers : Fix transaction cleanup

chrome/browser/chromeos/login/user_manager_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/user_manager_impl.h"
 
 #include <cstddef>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 108 matching lines @@
 void OnRemoveUserComplete(const std::string& user_email,
                           bool success,
                           cryptohome::MountError return_code) {
   // Log the error, but there's not much we can do.
   if (!success) {
     LOG(ERROR) << "Removal of cryptohome for " << user_email
                << " failed, return code: " << return_code;
   }
 }
 
-// This method is used to implement UserManager::RemoveUser.
-void RemoveUserInternal(const std::string& user_email,
-                        chromeos::RemoveUserDelegate* delegate) {
-  CrosSettings* cros_settings = CrosSettings::Get();
-
-  // Ensure the value of owner email has been fetched.
-  if (CrosSettingsProvider::TRUSTED != cros_settings->PrepareTrustedValues(
-          base::Bind(&RemoveUserInternal, user_email, delegate))) {
-    // Value of owner email is not fetched yet.  RemoveUserInternal will be
-    // called again after fetch completion.
-    return;
-  }
-  std::string owner;
-  cros_settings->GetString(kDeviceOwner, &owner);
-  if (user_email == owner) {
-    // Owner is not allowed to be removed from the device.
-    return;
-  }
-
-  if (delegate)
-    delegate->OnBeforeUserRemoved(user_email);
-
-  chromeos::UserManager::Get()->RemoveUserFromList(user_email);
-  cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
-      user_email, base::Bind(&OnRemoveUserComplete, user_email));
-
-  if (delegate)
-    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.
 void ParseUserList(const ListValue& users_list,
                    const std::set<std::string>& existing_users,
                    std::vector<std::string>* users_vector,
                    std::set<std::string>* users_set) {
   users_vector->clear();
   users_set->clear();
   for (size_t i = 0; i < users_list.GetSize(); ++i) {
@@ skipping 335 matching lines @@
   // Sanity check: do not allow any of the the logged in users to be removed.
   for (UserList::const_iterator it = logged_in_users_.begin();
        it != logged_in_users_.end(); ++it) {
     if ((*it)->email() == user_id)
       return;
   }
 
   RemoveUserInternal(user_id, delegate);
 }
 
+// This method is used to implement UserManager::RemoveUser.

[Nikita (slow), 2013/11/13 01:37:19]

nit: drop comment.

+void UserManagerImpl::RemoveUserInternal(const std::string& user_email,
+                                         RemoveUserDelegate* delegate) {
+  CrosSettings* cros_settings = CrosSettings::Get();
+
+  // Ensure the value of owner email has been fetched.
+  if (CrosSettingsProvider::TRUSTED != cros_settings->PrepareTrustedValues(
+          base::Bind(&UserManagerImpl::RemoveUserInternal,
+                     base::Unretained(this),
+                     user_email, delegate))) {
+    // Value of owner email is not fetched yet.  RemoveUserInternal will be
+    // called again after fetch completion.
+    return;
+  }
+  std::string owner;
+  cros_settings->GetString(kDeviceOwner, &owner);
+  if (user_email == owner) {
+    // Owner is not allowed to be removed from the device.
+    return;
+  }
+  RemoveNonOwnerUserInternal(user_email, delegate);
+}
+
+void UserManagerImpl::RemoveNonOwnerUserInternal(const std::string& user_email,
+                                                 RemoveUserDelegate* delegate) {
+  if (delegate)
+    delegate->OnBeforeUserRemoved(user_email);
+  RemoveUserFromList(user_email);
+  cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
+      user_email, base::Bind(&OnRemoveUserComplete, user_email));
+
+  if (delegate)
+    delegate->OnUserRemoved(user_email);
+}
+
 void UserManagerImpl::RemoveUserFromList(const std::string& user_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  EnsureUsersLoaded();

[Nikita (slow), 2013/11/13 01:37:19]

Do you think whether removing this call might have some side effects?

   RemoveNonCryptohomeData(user_id);
-  User* user = RemoveRegularOrLocallyManagedUserFromList(user_id);
-  delete user;
+  if (users_loaded_) {
+    User* user = RemoveRegularOrLocallyManagedUserFromList(user_id);
+    delete user;
+  } else {
+    DCHECK(gaia::ExtractDomainName(user_id) ==
+        UserManager::kLocallyManagedUserDomain);
+    // Special case, removing partially-constructed supervised user during user
+    // list loading.
+    ListPrefUpdate users_update(g_browser_process->local_state(),
+                                kRegularUsers);
+    users_update->Remove(base::StringValue(user_id), NULL);
+  }
   // Make sure that new data is persisted to Local State.
   g_browser_process->local_state()->CommitPendingWrite();
 }
 
 bool UserManagerImpl::IsKnownUser(const std::string& user_id) const {
   return FindUser(user_id) != NULL;
 }
 
 const User* UserManagerImpl::FindUser(const std::string& user_id) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ skipping 420 matching lines @@
   // Data belonging to the guest, retail mode and stub users is always
   // ephemeral.
   if (user_id == UserManager::kGuestUserName ||
       user_id == UserManager::kRetailModeUserName ||
       user_id == kStubUser) {
     return true;
   }
 
   // Data belonging to the owner, anyone found on the user list and obsolete
   // public accounts whose data has not been removed yet is not ephemeral.
-  if (user_id == owner_email_  || FindUserInList(user_id) ||
+  if (user_id == owner_email_  || UserExistsInList(user_id) ||
       user_id == g_browser_process->local_state()->
           GetString(kPublicAccountPendingDataRemoval)) {
     return false;
   }
 
   // Data belonging to the currently logged-in user is ephemeral when:
   // a) The user logged into a regular account while the ephemeral users policy
   //    was enabled.
   //    - or -
   // b) The user logged into any other account type.
@@ skipping 55 matching lines @@
   RestorePendingUserSessions();
 }
 
 void UserManagerImpl::EnsureUsersLoaded() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!g_browser_process || !g_browser_process->local_state())
     return;
 
   if (users_loaded_)
     return;
-  users_loaded_ = true;
-
-  // Clean up user list first.
+  // Clean up user list first. All code down the path should be synchronous,
+  // so that local state after transaction rollback is in consistent state.
+  // This process also should not trigger EnsureUsersLoaded again.
   if (supervised_user_manager_->HasFailedUserCreationTransaction())
     supervised_user_manager_->RollbackUserCreationTransaction();
 
   PrefService* local_state = g_browser_process->local_state();
   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 =
@@ skipping 31 matching lines @@
   // 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));
     UpdatePublicAccountDisplayName(*it);
   }
+  users_loaded_ = true;
 
   user_image_manager_->LoadUserImages(users_);
 }
 
 void UserManagerImpl::RetrieveTrustedDevicePolicies() {
   ephemeral_users_enabled_ = false;
   owner_email_ = "";
 
   // Schedule a callback if device policy has not yet been verified.
   if (CrosSettingsProvider::TRUSTED != cros_settings_->PrepareTrustedValues(
@@ skipping 57 matching lines @@
 
 const User* UserManagerImpl::FindUserInList(const std::string& user_id) const {
   const UserList& users = GetUsers();
   for (UserList::const_iterator it = users.begin(); it != users.end(); ++it) {
     if ((*it)->email() == user_id)
       return *it;
   }
   return NULL;
 }
 
+const bool UserManagerImpl::UserExistsInList(const std::string& user_id) const {
+  PrefService* local_state = g_browser_process->local_state();
+  const ListValue* user_list = local_state->GetList(kRegularUsers);
+  for (size_t i = 0; i < user_list->GetSize(); ++i) {
+    std::string email;
+    if (user_list->GetString(i, &email) && (user_id == email))
+      return true;
+  }
+  return false;
+}
+
 User* UserManagerImpl::FindUserInListAndModify(const std::string& user_id) {
   UserList& users = GetUsersAndModify();
   for (UserList::iterator it = users.begin(); it != users.end(); ++it) {
     if ((*it)->email() == user_id)
       return *it;
   }
   return NULL;
 }
 
 void UserManagerImpl::GuestUserLoggedIn() {
@@ skipping 627 matching lines @@
   }
 }
 
 void UserManagerImpl::OnUserNotAllowed() {
   LOG(ERROR) << "Shutdown session because a user is not allowed to be in the "
                 "current session";
   chrome::AttemptUserExit();
 }
 
 }  // namespace chromeos