From a7def30b1ca3789f9ce2868ca7e2362114abc213 Mon Sep 17 00:00:00 2001 From: brandonkelly Date: Tue, 17 Dec 2024 16:27:26 -0800 Subject: [PATCH] User impersonation cleanup --- CHANGELOG-WIP.md | 4 ++ src/controllers/UsersController.php | 44 ++++++++---------- src/elements/User.php | 38 ++++++--------- src/services/Auth.php | 6 +-- src/web/User.php | 72 +++++++++++++++++++++++++++-- src/web/assets/cp/CpAsset.php | 12 +---- 6 files changed, 109 insertions(+), 67 deletions(-) diff --git a/CHANGELOG-WIP.md b/CHANGELOG-WIP.md index a829fb2abdd..a82608829ed 100644 --- a/CHANGELOG-WIP.md +++ b/CHANGELOG-WIP.md @@ -86,6 +86,9 @@ - Added `craft\models\MailSettings::$siteOverrides`. - Added `craft\services\Elements::canSaveCanonical()`. - Added `craft\services\Gql::getFieldLayoutArguments()`. +- Added `craft\web\User::getImpersonator()`. +- Added `craft\web\User::getImpersonatorId()`. +- Added `craft\web\User::setImpersonatorId()`. - Added `craft\web\View::setTwig()`. - `craft\elements\NestedElementManager::getIndexHtml()` now supports passing `defaultSort` in the `$config` array. ([#16236](https://github.com/craftcms/cms/discussions/16236)) - `craft\elements\conditions\entries\MatrixFieldConditionRule` is now an alias of `FieldConditionRule`. @@ -97,6 +100,7 @@ - Deprecated `craft\elements\User::EVENT_REGISTER_USER_ACTIONS`. - Deprecated `craft\fields\Color::$presets`. ([#16249](https://github.com/craftcms/cms/pull/16249)) - Deprecated `craft\fields\Link::$showTargetField`. +- Deprecatod `craft\elements\User::IMPERSONATE_KEY`. `craft\web\User::getImpersonatorId()` should be used instead. - `_includes/forms/autosuggest.twig` now supports a `suggestTemplates` variable. - `_includes/forms/colorSelect.twig` now supports `options` and `withBlankOption` variables. - `_includes/forms/selectize.twig` now supports a `color` property in option data, which can be set to a hex value or a color name. diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php index d505944cca6..c696c68a5b0 100644 --- a/src/controllers/UsersController.php +++ b/src/controllers/UsersController.php @@ -244,8 +244,9 @@ public function actionLogin(): ?Response } // if we're impersonating, pass the user we're impersonating to the complete method - if (Session::get(User::IMPERSONATE_KEY) !== null) { - $user = Craft::$app->getUser()->getIdentity() ?? $user; + $impersonator = Craft::$app->getUser()->getImpersonator(); + if ($impersonator !== null) { + $user = $impersonator; } return $this->_completeLogin($user, $duration); @@ -283,8 +284,9 @@ public function actionLoginWithPasskey(): ?Response } // if we're impersonating, pass the user we're impersonating to the complete method - if (Session::get(User::IMPERSONATE_KEY) !== null) { - $user = Craft::$app->getUser()->getIdentity(); + $userSession = Craft::$app->getUser(); + if ($userSession->getImpersonator() !== null) { + $user = $userSession->getIdentity(); } return $this->_completeLogin($user, $duration); @@ -365,10 +367,10 @@ public function actionImpersonate(): ?Response // Save the original user ID to the session now so User::findIdentity() // knows not to worry if the user isn't active yet - Session::set(User::IMPERSONATE_KEY, $userSession->getId()); + $userSession->setImpersonatorId($userSession->getId()); if (!$userSession->loginByUserId($userId)) { - Session::remove(User::IMPERSONATE_KEY); + $userSession->setImpersonatorId(null); $this->setFailFlash(Craft::t('app', 'There was a problem impersonating this user.')); Craft::error($userSession->getIdentity()->username . ' tried to impersonate userId: ' . $userId . ' but something went wrong.', __METHOD__); return null; @@ -440,10 +442,10 @@ public function actionImpersonateWithToken(int $userId, int $prevUserId): ?Respo if ($user) { // Save the original user ID to the session now so User::findIdentity() // knows not to worry if the user isn't active yet - Session::set(User::IMPERSONATE_KEY, $prevUserId); + $userSession->setImpersonatorId($prevUserId); $success = $userSession->login($user); if (!$success) { - Session::remove(User::IMPERSONATE_KEY); + $userSession->setImpersonatorId(null); } } @@ -518,13 +520,9 @@ public function actionLoginModal(): Response $forElevatedSession = (bool)$this->request->getBodyParam('forElevatedSession'); - // If the current user is being impersonated get the "original" user instead - if ($forElevatedSession && $previousUserId = Session::get(User::IMPERSONATE_KEY)) { - /** @var User $user */ - $user = User::find() - ->id($previousUserId) - ->one(); - $staticEmail = $user->email; + // If the current user is being impersonated, get the impersonator instead + if ($forElevatedSession && ($impersonator = Craft::$app->getUser()->getImpersonator())) { + $staticEmail = $impersonator->email; } else { $staticEmail = $this->request->getRequiredBodyParam('email'); } @@ -1910,8 +1908,7 @@ public function actionUnlockUser(): Response } // And admins can't unlock themselves by impersonating another admin - $previousUserId = Session::get(User::IMPERSONATE_KEY); - if ($previousUserId && $user->id == $previousUserId) { + if ($user->id === Craft::$app->getUser()->getImpersonatorId()) { throw new ForbiddenHttpException('You can’t unlock yourself via impersonation.'); } } @@ -2347,14 +2344,9 @@ private function _handleLoginFailure(?string $authError = null, ?User $user = nu public function actionAuthForm(): Response { - $user = null; - // If the current user is being impersonated get the "original" user instead - if ($previousUserId = Session::get(User::IMPERSONATE_KEY)) { - /** @var User $user */ - $user = User::find() - ->id($previousUserId) - ->one(); - } + // If the current user is being impersonated, use the impersonator + $userSession = Craft::$app->getUser(); + $user = $userSession->getImpersonator() ?? $userSession->getIdentity(); $activeMethods = Craft::$app->getAuth()->getActiveMethods($user); $methodClass = $this->request->getParam('method'); @@ -2391,7 +2383,7 @@ public function actionAuthForm(): Response 'authForm' => $html, 'headHtml' => $view->getHeadHtml(), 'bodyHtml' => $view->getBodyHtml(), - 'returnUrl' => Craft::$app->getUser()->getReturnUrl($defaultReturnUrl), + 'returnUrl' => $userSession->getReturnUrl($defaultReturnUrl), ]); } diff --git a/src/elements/User.php b/src/elements/User.php index 8228a531e0c..70ecded581f 100644 --- a/src/elements/User.php +++ b/src/elements/User.php @@ -41,7 +41,6 @@ use craft\helpers\StringHelper; use craft\helpers\Template; use craft\helpers\UrlHelper; -use craft\helpers\User as UserHelper; use craft\i18n\Formatter; use craft\models\FieldLayout; use craft\models\Site; @@ -68,7 +67,6 @@ use yii\validators\Validator; use yii\web\BadRequestHttpException; use yii\web\IdentityInterface; -use yii\web\Response; /** * User represents a user element. @@ -118,7 +116,10 @@ class User extends Element implements IdentityInterface */ public const EVENT_DEFINE_FRIENDLY_NAME = 'defineFriendlyName'; - public const IMPERSONATE_KEY = 'Craft.UserSessionService.prevImpersonateUserId'; + /** + * @deprecated in 5.6.0. [[\craft\web\User:getImpersonatorId()]] should be used instead. + */ + public const IMPERSONATE_KEY = '__impersonator_id'; /** * @event RegisterUserActionsEvent The event that is triggered when a user’s available actions are being registered @@ -622,25 +623,15 @@ public static function findIdentity($id): ?self return null; } - /** @var static $user */ - if ($user->getStatus() === self::STATUS_ACTIVE) { - return $user; - } - - // If the current user is being impersonated, ignore their status - if ($previousUserId = Session::get(self::IMPERSONATE_KEY)) { - /** @var self|null $previousUser */ - $previousUser = self::find() - ->id($previousUserId) - ->status(null) - ->one(); - - if ($previousUser && $previousUser->can('impersonateUsers')) { - return $user; - } + // Only accept active users, unless they're being impersonated + if ( + $user->getStatus() !== self::STATUS_ACTIVE && + !Craft::$app->getUser()->getImpersonator() + ) { + return null; } - return null; + return $user; } /** @@ -1869,6 +1860,7 @@ protected function safeActionMenuItems(): array $currentUser = Craft::$app->getUser()->getIdentity(); $view = Craft::$app->getView(); $usersService = Craft::$app->getUsers(); + $userSession = Craft::$app->getUser(); $canAdministrateUsers = $currentUser->can('administrateUsers'); $canModerateUsers = $currentUser->can('moderateUsers'); @@ -1944,8 +1936,8 @@ protected function safeActionMenuItems(): array ($currentUser->admin || !$this->admin) && $canModerateUsers && ( - ($previousUserId = Session::get(self::IMPERSONATE_KEY)) === null || - $this->id != $previousUserId + ($impersonatorId = $userSession->getImpersonatorId()) === null || + $this->id !== $impersonatorId ) ) { $statusItems[] = [ @@ -1958,7 +1950,7 @@ protected function safeActionMenuItems(): array } } - if (!$isCurrentUser && Craft::$app->getUser()->checkPermission('editUsers')) { + if (!$isCurrentUser && $userSession->checkPermission('editUsers')) { $statusItems[] = [ 'icon' => 'paperplane', 'label' => Craft::t('app', 'Send password reset email'), diff --git a/src/services/Auth.php b/src/services/Auth.php index e585431ac5c..42035f8bbb2 100644 --- a/src/services/Auth.php +++ b/src/services/Auth.php @@ -20,7 +20,6 @@ use craft\helpers\Component as ComponentHelper; use craft\helpers\DateTimeHelper; use craft\helpers\Json; -use craft\helpers\Session as SessionHelper; use craft\models\UserGroup; use craft\records\WebAuthn as WebAuthnRecord; use craft\web\Session; @@ -200,12 +199,13 @@ public function verify(string $methodClass, mixed ...$args): bool $this->setUser(null); // if we're impersonating, pass the user we're impersonating to the complete the login - if (SessionHelper::get(User::IMPERSONATE_KEY) !== null) { + $userSession = Craft::$app->getUser(); + if ($userSession->getImpersonator() !== null) { /** @var User $user */ $user = Craft::$app->getUser()->getIdentity(); } - Craft::$app->getUser()->login($user, $sessionDuration); + $userSession->login($user, $sessionDuration); } return true; diff --git a/src/web/User.php b/src/web/User.php index c6af8146ca1..db45d7eed2c 100644 --- a/src/web/User.php +++ b/src/web/User.php @@ -54,6 +54,17 @@ class User extends \yii\web\User */ public string $elevatedSessionTimeoutParam = '__elevated_timeout'; + /** + * @var string The session variable name used to store the original user ID, when impersonating another user. + * @since 5.6.0 + */ + public string $impersonatorIdParam = '__impersonator_id'; + + /** + * @see getImpersonator() + */ + private UserElement|false $impersonator; + // Authentication // ------------------------------------------------------------------------- @@ -240,6 +251,58 @@ public function getRemainingSessionTime(): int return 0; } + /** + * Returns the original user, if the current user is being impersonated. + * + * @return UserElement|null + * @since 5.6.0 + */ + public function getImpersonator(): ?UserElement + { + if (!isset($this->impersonator)) { + $impersonatorId = SessionHelper::get($this->impersonatorIdParam); + if (!$impersonatorId) { + return null; + } + + $impersonator = UserElement::find() + ->id($impersonatorId) + ->one(); + + $this->impersonator = $impersonator?->can('impersonateUsers') + ? $impersonator + : false; + } + + return $this->impersonator ?: null; + } + + /** + * Returns the ID of the original user, if the current user is being impersonated. + * + * @return int|null + * @since 5.6.0 + */ + public function getImpersonatorId(): ?int + { + return $this->getImpersonator()?->id; + } + + /** + * Sets the ID of the original user, if the current user is being impersonated. + * + * @param int|null $id + * @since 5.6.0 + */ + public function setImpersonatorId(?int $id): void + { + if ($id) { + SessionHelper::set($this->impersonatorIdParam, $id); + } else { + SessionHelper::remove($this->impersonatorIdParam); + } + } + // Authorization // ------------------------------------------------------------------------- @@ -367,13 +430,13 @@ protected function afterLogin($identity, $cookieBased, $duration): void $this->_clearOtherSessionParams(); // Save the username cookie if they're not being impersonated - $impersonating = SessionHelper::get(UserElement::IMPERSONATE_KEY) !== null; - if (!$impersonating) { + $impersonator = $this->getImpersonator(); + if (!$impersonator) { $this->sendUsernameCookie($identity); } // Update the user record - if (!$impersonating) { + if (!$impersonator) { Craft::$app->getUsers()->handleValidLogin($identity); } @@ -488,7 +551,8 @@ protected function afterLogout($identity): void { /** @var UserElement $identity */ // Delete the impersonation session, if there is one - SessionHelper::remove(UserElement::IMPERSONATE_KEY); + SessionHelper::remove($this->impersonatorIdParam); + $this->impersonator = false; $this->_clearOtherSessionParams(); diff --git a/src/web/assets/cp/CpAsset.php b/src/web/assets/cp/CpAsset.php index 4147e9498d6..5b87205067c 100644 --- a/src/web/assets/cp/CpAsset.php +++ b/src/web/assets/cp/CpAsset.php @@ -18,7 +18,6 @@ use craft\helpers\DateTimeHelper; use craft\helpers\Html; use craft\helpers\Json; -use craft\helpers\Session; use craft\helpers\StringHelper; use craft\helpers\UrlHelper; use craft\i18n\Locale; @@ -517,15 +516,6 @@ private function _craftData(): array ]; } - $impersonator = null; - // if we're impersonating, we need to check if the original user has passkey - if ($previousUserId = Session::get(User::IMPERSONATE_KEY)) { - /** @var User|null $impersonator */ - $impersonator = User::find() - ->id($previousUserId) - ->one(); - } - $data += [ 'allowAdminChanges' => $generalConfig->allowAdminChanges, 'allowUpdates' => $generalConfig->allowUpdates, @@ -561,7 +551,7 @@ private function _craftData(): array 'siteToken' => $generalConfig->siteToken, 'slugWordSeparator' => $generalConfig->slugWordSeparator, 'userEmail' => $currentUser->email, - 'userHasPasskeys' => Craft::$app->getAuth()->hasPasskeys($impersonator ?? $currentUser), + 'userHasPasskeys' => Craft::$app->getAuth()->hasPasskeys($userSession->getImpersonator() ?? $currentUser), 'userIsAdmin' => $currentUser->admin, 'username' => $currentUser->username, ];