Move user creation from session creation.
Currently, if missing and some condition meets, a User instance is created in UserManager::UserLoggedIn called from SessionManager::CreateSession. This approach is difficult to write proper test, because the expecting condition is too complex. This CL moves that part to the callers of CreateSession family. Now, UserLoggedIn handles log-in. Callers now have responsibility to register User before creating the session. In the production, no behavior change is expected. In the tests, the behavior of UserManager is now much aligned with fake implementation. BUG=278643115 TEST=Tryjob Change-Id: Ia746f845a6967288ff5626f5507b9da71c5d8487 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6239080 Commit-Queue: Hidehiko Abe <hidehiko@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Cr-Commit-Position: refs/heads/main@{#1417065}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
c5f8015ca0
commit
d5f6403f66
chrome/browser
ash
accessibility
child_accounts
file_manager
login
lock
session
users
main_parts
system
download
notification
net
profiles
ssl
ui
ash
assistant
new_window
session
views
relaunch_notification
web_applications
chromeos/ash
experiences
services
components
@ -57,17 +57,15 @@ void FakeUserManager::UserLoggedIn(const AccountId& account_id,
|
||||
}
|
||||
}
|
||||
|
||||
if (!active_user_ && IsEphemeralAccountId(account_id)) {
|
||||
// TODO(crbug.com/278643115): Temporarily duplicate the logic
|
||||
// of ephemeral user creation. This method should be unified with
|
||||
// UserManagerImpl::UserLoggedIn eventually.
|
||||
active_user_ = AddEphemeralUser(account_id, UserType::kRegular);
|
||||
SetIsCurrentUserNew(true);
|
||||
}
|
||||
|
||||
NotifyOnLogin();
|
||||
}
|
||||
|
||||
bool FakeUserManager::EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) {
|
||||
NOTREACHED();
|
||||
}
|
||||
|
||||
void FakeUserManager::SwitchActiveUser(const AccountId& account_id) {
|
||||
for (UserList::const_iterator it = logged_in_users_.begin();
|
||||
it != logged_in_users_.end(); ++it) {
|
||||
|
@ -35,6 +35,9 @@ class USER_MANAGER_EXPORT FakeUserManager : public UserManagerImpl {
|
||||
const std::string& username_hash,
|
||||
bool browser_restart,
|
||||
bool is_child) override;
|
||||
bool EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) override;
|
||||
void SwitchActiveUser(const AccountId& account_id) override;
|
||||
|
||||
// Just make it public for tests.
|
||||
|
@ -10,6 +10,7 @@
|
||||
#include "components/user_manager/known_user.h"
|
||||
#include "components/user_manager/user_manager.h"
|
||||
#include "components/user_manager/user_manager_pref_names.h"
|
||||
#include "components/user_manager/user_names.h"
|
||||
|
||||
namespace user_manager {
|
||||
|
||||
@ -31,6 +32,34 @@ TestHelper::TestHelper(UserManager& user_manager)
|
||||
|
||||
TestHelper::~TestHelper() = default;
|
||||
|
||||
User* TestHelper::AddRegularUser(const AccountId& account_id) {
|
||||
return AddUserInternal(account_id, UserType::kRegular);
|
||||
}
|
||||
|
||||
User* TestHelper::AddChildUser(const AccountId& account_id) {
|
||||
return AddUserInternal(account_id, UserType::kChild);
|
||||
}
|
||||
|
||||
User* TestHelper::AddGuestUser() {
|
||||
return AddUserInternal(GuestAccountId(), UserType::kGuest);
|
||||
}
|
||||
|
||||
User* TestHelper::AddUserInternal(const AccountId& account_id,
|
||||
UserType user_type) {
|
||||
if (user_manager_->FindUser(account_id)) {
|
||||
LOG(ERROR) << "User for " << account_id << " already exists";
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
if (!user_manager_->EnsureUser(account_id, user_type,
|
||||
/*is_ephemeral=*/false)) {
|
||||
LOG(ERROR) << "Failed to create a user " << user_type << " for "
|
||||
<< account_id;
|
||||
return nullptr;
|
||||
}
|
||||
return user_manager_->FindUserAndModify(account_id);
|
||||
}
|
||||
|
||||
User* TestHelper::AddKioskAppUser(std::string_view user_id) {
|
||||
// Quick check that the `user_id` satisfies kiosk-app type.
|
||||
auto type = policy::GetDeviceLocalAccountType(user_id);
|
||||
|
@ -8,6 +8,7 @@
|
||||
#include <string_view>
|
||||
|
||||
#include "base/memory/raw_ref.h"
|
||||
#include "components/user_manager/user_type.h"
|
||||
|
||||
class AccountId;
|
||||
class PrefService;
|
||||
@ -32,11 +33,24 @@ class TestHelper {
|
||||
explicit TestHelper(UserManager& user_manager);
|
||||
~TestHelper();
|
||||
|
||||
// Creates and adds a new Kiosk user.
|
||||
// On failure, nullptr is returned.
|
||||
// Creates and adds a regular (persisted) user, and returns it.
|
||||
// On failure, returns nullptr.
|
||||
[[nodiscard]] User* AddRegularUser(const AccountId& account_id);
|
||||
|
||||
// Creates and adds a child user, and returns it.
|
||||
[[nodiscard]] User* AddChildUser(const AccountId& account_id);
|
||||
|
||||
// Creates and adds a guest user, and returns it.
|
||||
// On failure, returns nullptr.
|
||||
[[nodiscard]] User* AddGuestUser();
|
||||
|
||||
// Creates and adds a new Kiosk user, and returns it.
|
||||
// On failure, returns nullptr.
|
||||
[[nodiscard]] User* AddKioskAppUser(std::string_view user_id);
|
||||
|
||||
private:
|
||||
User* AddUserInternal(const AccountId& account_id, UserType user_type);
|
||||
|
||||
raw_ref<UserManager> user_manager_;
|
||||
};
|
||||
|
||||
|
@ -249,16 +249,26 @@ class USER_MANAGER_EXPORT UserManager {
|
||||
// Returns account Id of the user that was active in the previous session.
|
||||
virtual const AccountId& GetLastSessionActiveAccountId() const = 0;
|
||||
|
||||
// Indicates that a user with the given |account_id| has just logged in. The
|
||||
// persistent list is updated accordingly if the user is not ephemeral.
|
||||
// |browser_restart| is true when reloading Chrome after crash to distinguish
|
||||
// from normal sign in flow.
|
||||
// |username_hash| is used to identify homedir mount point.
|
||||
// Indicates that a user with the given `account_id` has just logged in.
|
||||
// `username_hash` is used to identify homedir mount point.
|
||||
// TODO(crbug.com/278643115): `browser_restart` and `is_child` is no longer
|
||||
// used. Remove them.
|
||||
virtual void UserLoggedIn(const AccountId& account_id,
|
||||
const std::string& username_hash,
|
||||
bool browser_restart,
|
||||
bool is_child) = 0;
|
||||
|
||||
// If there's no user for the given `account_id`, a new is created with
|
||||
// the given `user_type`. `is_ephemeral` is respected only if the `user_type`
|
||||
// is either kRegular or kChild.
|
||||
// If there's the user of `account_id` already (i.e. persisted),
|
||||
// the user is kRegular or kChild, and the given `user_type` is either one,
|
||||
// the type will be updated properly.
|
||||
// Returns whether the new user is created.
|
||||
virtual bool EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) = 0;
|
||||
|
||||
// Called when the Profile instance for a user identified by `account_id`
|
||||
// is created. `prefs` should be the one that is owned by Profile.
|
||||
// The 'prefs' must be kept alive until OnUserProfileWillBeDestroyed
|
||||
|
@ -273,56 +273,32 @@ void UserManagerImpl::UserLoggedIn(const AccountId& account_id,
|
||||
last_session_active_account_id_initialized_ = true;
|
||||
}
|
||||
|
||||
if (!logged_in_users_.empty()) {
|
||||
// Handle multi sign-in case. For multi-sign in, there already should be
|
||||
// active_user_ set, and the user to be logged in should be found
|
||||
// in the persistent user list.
|
||||
User* user = FindUserInListAndModify(account_id);
|
||||
CHECK(active_user_);
|
||||
CHECK(user);
|
||||
bool is_primary_login = logged_in_users_.empty();
|
||||
// For primary login case, i.e., there was no previous login, so there should
|
||||
// be no active user. Otherwise, there was some previous login, so there
|
||||
// should be an active user.
|
||||
CHECK_EQ(!active_user_, is_primary_login);
|
||||
|
||||
OnUserLoggedIn(*user, username_hash);
|
||||
|
||||
// Reset the new user flag if the user already exists.
|
||||
SetIsCurrentUserNew(false);
|
||||
SendMultiUserSignInMetrics();
|
||||
|
||||
NotifyUserAddedToSession(user);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
// There are no logged in users. `active_user_` should not be set.
|
||||
CHECK(!active_user_);
|
||||
|
||||
// Ensure User is there.
|
||||
const UserType user_type =
|
||||
CalculateUserType(account_id, FindUserInListAndModify(account_id),
|
||||
browser_restart, is_child);
|
||||
// Check whether the user should be ephemeral based on account_id.
|
||||
// If this is for browser restarting, and new user needs to be created,
|
||||
// that means, in precious chrome process, the user was ephemeral
|
||||
// so disappeared (i.e. not in the persisted user list).
|
||||
bool is_ephemeral = IsEphemeralAccountId(account_id) || browser_restart;
|
||||
auto [user, created] = EnsureUser(account_id, user_type, is_ephemeral);
|
||||
|
||||
SetIsCurrentUserNew((created && (user->GetType() == UserType::kRegular ||
|
||||
user->GetType() == UserType::kChild)) ||
|
||||
user->GetType() == UserType::kPublicAccount);
|
||||
User* user = FindUserAndModify(account_id);
|
||||
CHECK(user);
|
||||
OnUserLoggedIn(*user, username_hash);
|
||||
OnPrimaryUserLoggedIn(*user);
|
||||
OnActiveUserSwitched(*user);
|
||||
|
||||
local_state_->CommitPendingWrite();
|
||||
NotifyOnLogin();
|
||||
if (is_primary_login) {
|
||||
OnPrimaryUserLoggedIn(*user);
|
||||
OnActiveUserSwitched(*user);
|
||||
|
||||
local_state_->CommitPendingWrite();
|
||||
NotifyOnLogin();
|
||||
} else {
|
||||
SendMultiUserSignInMetrics();
|
||||
NotifyUserAddedToSession(user);
|
||||
}
|
||||
}
|
||||
|
||||
UserManagerImpl::EnsuredUser UserManagerImpl::EnsureUser(
|
||||
const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) {
|
||||
bool UserManagerImpl::EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) {
|
||||
User* user = FindUserInListAndModify(account_id);
|
||||
bool created = user == nullptr;
|
||||
switch (user_type) {
|
||||
case UserType::kRegular:
|
||||
case UserType::kChild:
|
||||
@ -346,16 +322,17 @@ UserManagerImpl::EnsuredUser UserManagerImpl::EnsureUser(
|
||||
} else {
|
||||
user = AddGaiaUser(account_id, user_type);
|
||||
}
|
||||
break;
|
||||
return true;
|
||||
|
||||
case UserType::kGuest:
|
||||
CHECK(!user);
|
||||
user = AddGuestUser();
|
||||
break;
|
||||
return true;
|
||||
|
||||
case UserType::kPublicAccount:
|
||||
if (!user) {
|
||||
user = AddPublicAccountUser(account_id);
|
||||
return true;
|
||||
}
|
||||
break;
|
||||
|
||||
@ -369,7 +346,7 @@ UserManagerImpl::EnsuredUser UserManagerImpl::EnsureUser(
|
||||
NOTREACHED() << "Unhandled usert type " << user_type;
|
||||
}
|
||||
CHECK(user);
|
||||
return {user, created};
|
||||
return false;
|
||||
}
|
||||
|
||||
void UserManagerImpl::OnUserLoggedIn(User& user,
|
||||
|
@ -154,6 +154,9 @@ class USER_MANAGER_EXPORT UserManagerImpl : public UserManager {
|
||||
const std::string& user_id_hash,
|
||||
bool browser_restart,
|
||||
bool is_child) override;
|
||||
bool EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral) override;
|
||||
bool OnUserProfileCreated(const AccountId& account_id,
|
||||
PrefService* prefs) override;
|
||||
void OnUserProfileWillBeDestroyed(const AccountId& account_id) override;
|
||||
@ -391,21 +394,6 @@ class USER_MANAGER_EXPORT UserManagerImpl : public UserManager {
|
||||
// Subsequent calls have no effect. Must be called on the UI thread.
|
||||
void EnsureUsersLoaded();
|
||||
|
||||
// If there's the user of `account_id` already (i.e. persisted), the user
|
||||
// will be returned with `created` = false. If the user is kRegular or kChild,
|
||||
// and given `user_type` is either one, the type will be updated properly.
|
||||
// `is_ephemeral` param will be ignored in this case.
|
||||
// If there's no such user, a new user of the given `user_type` will be
|
||||
// created and returned with `created` = true. On creation, if the user
|
||||
// type is kRegular or kChild, is_ephemeral is respected.
|
||||
struct EnsuredUser {
|
||||
raw_ptr<User> user;
|
||||
bool created;
|
||||
};
|
||||
EnsuredUser EnsureUser(const AccountId& account_id,
|
||||
UserType user_type,
|
||||
bool is_ephemeral);
|
||||
|
||||
// Returns a list of users who have logged into this device previously.
|
||||
// Same as GetUsers but used if you need to modify User from that list.
|
||||
UserList& GetUsersAndModify();
|
||||
|
Reference in New Issue
Block a user