User impersonation cleanup

craftcms · Dec 18, 2024 · a7def30 · a7def30
1 parent addb9d7
commit a7def30
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 67 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
         ];