From 3f68e1d3dcce03511cf1560d9415060afb762c8b Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Thu, 13 Jun 2019 21:12:59 +0200
Subject: [PATCH 01/18] >.< Forgot about the actions

---
 CHANGELOG-v3.md                     | 1 +
 src/controllers/UsersController.php | 4 ++--
 src/services/UserPermissions.php    | 3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG-v3.md b/CHANGELOG-v3.md
index bf2ef658a9a..d0726fab5a7 100644
--- a/CHANGELOG-v3.md
+++ b/CHANGELOG-v3.md
@@ -5,6 +5,7 @@
 ### Added
 - Added `craft\base\ElementInterface::getIsDraft()`.
 - Added `craft\base\ElementInterface::getIsRevision()`.
+- Added the `impersonateUsers` user permission.
 
 ### Fixed
 - Fixed a bug where entry drafts weren’t getting updated slug values once their initial slug had been saved, if their entry type had a custom title format. ([#4373](https://github.com/craftcms/cms/issues/4373))
diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php
index eefe22db324..fe0d7c18c8e 100644
--- a/src/controllers/UsersController.php
+++ b/src/controllers/UsersController.php
@@ -180,7 +180,7 @@ public function actionLogin()
     public function actionImpersonate()
     {
         $this->requireLogin();
-        $this->requireAdmin(false);
+        $this->requirePermission('impersonateUsers');
         $this->requirePostRequest();
 
         $userSession = Craft::$app->getUser();
@@ -697,7 +697,7 @@ public function actionEditUser($userId = null, User $user = null, array $errors
             }
 
             if (!$isCurrentUser) {
-                if ($userSession->getIsAdmin()) {
+                if ($userSession->checkPermission('impersonateUsers')) {
                     $sessionActions[] = [
                         'action' => 'users/impersonate',
                         'label' => Craft::t('app', 'Login as {user}', ['user' => $user->getName()])
diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index 3108a27c477..e99a1daca0c 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -128,6 +128,9 @@ public function getAllPermissions(): array
                             'info' => Craft::t('app', 'Includes activating user accounts, resetting passwords, and changing email addresses.'),
                             'warning' => Craft::t('app', 'Accounts with this permission could use it to escalate their own permissions.'),
                         ],
+                        'impersonateUsers' => [
+                            'label' => Craft::t('app', 'Impersonate users')
+                        ]
                     ],
                 ],
                 'deleteUsers' => [

From 15df1e44e0705e9c02269fb087332883d8c6b023 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:47:16 +0200
Subject: [PATCH 02/18] [WIP] Added the required services for checking
 impersonation

---
 src/services/UserPermissions.php | 48 ++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index e99a1daca0c..52b852c961d 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -349,6 +349,54 @@ public function getPermissionsByUserId(int $userId): array
         return $this->_permissionsByUserId[$userId];
     }
 
+    /**
+     * @param User $user
+     * @param User $userToImpersonate
+     * @return bool
+     */
+    public function isImpersonationAllowed(User $impersonator, User $userToImpersonate) : bool
+    {
+        // Too easy
+        if ($impersonator->admin) {
+            return true;
+        }
+
+        // Is the impersonator not an admin but the impersonatee is - then no.
+        if ($userToImpersonate->admin) {
+            return false;
+        }
+
+        // Be gone!
+        if (!$impersonator->can('impersonateUsers')) {
+            return false;
+        }
+
+        // The user that is going to get impersonated cannot have more rights than the impersonator.
+        if ($this->doesFirstUserHaveMorePermissions($userToImpersonate->id, $impersonator->id)) {
+            return false;
+        }
+
+        // Mkay
+        return true;
+    }
+
+    /**
+     * Checks whether a user with id ($userId1) has more permissions than a user with id ($userId2)
+     *
+     * @param int $userId1
+     * @param int $userId2
+     * @return bool
+     */
+    public function doesFirstUserHaveMorePermissions(int $userId1, int $userId2) : bool
+    {
+        $mainUser = $this->getPermissionsByUserId($userId1);
+        $secondaryUser = $this->getPermissionsByUserId($userId2);
+
+        $difference = array_diff($mainUser, $secondaryUser);
+
+        return $difference ? true : false;
+    }
+
     /**
      * Returns whether a given user has a given permission.
      *

From 970a1d2502bf0b3096f748d913c34af78e851ef9 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:47:44 +0200
Subject: [PATCH 03/18] [WIP] Add those checks in the user controller.

---
 src/controllers/UsersController.php | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php
index fe0d7c18c8e..806d01fc921 100644
--- a/src/controllers/UsersController.php
+++ b/src/controllers/UsersController.php
@@ -180,7 +180,6 @@ public function actionLogin()
     public function actionImpersonate()
     {
         $this->requireLogin();
-        $this->requirePermission('impersonateUsers');
         $this->requirePostRequest();
 
         $userSession = Craft::$app->getUser();
@@ -189,6 +188,17 @@ public function actionImpersonate()
 
         $userId = $request->getBodyParam('userId');
 
+        // Are they actually allowed to impersonate this user?
+        $currentUser = $userSession->getIdentity();
+
+        // Fast track people who are admins - so we dont have to call any DB.
+        if (!$currentUser->admin) {
+            $desiredImpersonationSubject = Craft::$app->getUsers()->getUserById($userId);
+            if (!Craft::$app->getUserPermissions()->isImpersonationAllowed($currentUser, $desiredImpersonationSubject)) {
+                throw new ForbiddenHttpException('You do not have suffecient priveleges to impersonate this 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, $userSession->getId());
@@ -697,7 +707,7 @@ public function actionEditUser($userId = null, User $user = null, array $errors
             }
 
             if (!$isCurrentUser) {
-                if ($userSession->checkPermission('impersonateUsers')) {
+                if (Craft::$app->getUserPermissions()->isImpersonationAllowed($currentUser, $user)) {
                     $sessionActions[] = [
                         'action' => 'users/impersonate',
                         'label' => Craft::t('app', 'Login as {user}', ['user' => $user->getName()])

From fc6eeac7d808f3bdd1a5192f53c062d491db4d79 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:48:55 +0200
Subject: [PATCH 04/18] Add an extra check in checkPermission() that ensures
 people themselves have the right permissions

---
 src/web/User.php | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index 74fbc0a2585..4a827d2f5f5 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -12,6 +12,7 @@
 use craft\db\Query;
 use craft\db\Table;
 use craft\elements\User as UserElement;
+use craft\errors\MissingComponentException;
 use craft\errors\UserLockedException;
 use craft\events\LoginFailureEvent;
 use craft\helpers\ConfigHelper;
@@ -19,6 +20,7 @@
 use craft\helpers\UrlHelper;
 use craft\helpers\User as UserHelper;
 use craft\validators\UserPasswordValidator;
+use yii\base\InvalidArgumentException;
 use yii\web\Cookie;
 
 /**
@@ -236,13 +238,26 @@ public function getIsAdmin(): bool
     /**
      * Returns whether the current user has a given permission.
      *
-     * @param string $permissionName The name of the permission.
-     * @return bool Whether the current user has the permission.
+     * @param string $permissionName
+     * @return bool
+     * @throws \craft\errors\MissingComponentException
      */
     public function checkPermission(string $permissionName): bool
     {
         $user = $this->getIdentity();
 
+
+        if ($previousUserId = Craft::$app->getSession()->get(UserElement::IMPERSONATE_KEY)) {
+            $user = UserElement::find()
+                ->addSelect(['users.password'])
+                ->id($previousUserId)
+                ->one();
+
+            if (!$user || !$user->can($permissionName)) {
+                return false;
+            }
+        }
+
         return ($user && $user->can($permissionName));
     }
 

From d187ca7a97295129b59860a091e9ac4e9267a4aa Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:49:21 +0200
Subject: [PATCH 05/18] @brandonkelly Need you to check this. the admin() call
 would fail for non admin impersonators.

---
 src/web/User.php | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index 4a827d2f5f5..029646e7e5b 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -309,9 +309,11 @@ public function getHasElevatedSession(): bool
     /**
      * Starts an elevated user session for the current user.
      *
-     * @param string $password the current user’s password
-     * @return bool Whether the password was valid, and the user session has been elevated
-     * @throws UserLockedException if the user is locked.
+     * @param string $password
+     * @return bool
+     * @throws UserLockedException
+     * @throws MissingComponentException
+     * @throws InvalidArgumentException
      */
     public function startElevatedSession(string $password): bool
     {
@@ -322,8 +324,14 @@ public function startElevatedSession(string $password): bool
             $user = UserElement::find()
                 ->addSelect(['users.password'])
                 ->id($previousUserId)
-                ->admin(true)
                 ->one();
+
+            // TODO: @brandonkelly - have a look at this. I removed the ->admin() call in favour of this.
+            // TODO: It seems more appropriate
+            if (!$user->can('impersonateUsers')) {
+                $this->_handleLoginFailure(UserElement::AUTH_INVALID_CREDENTIALS);
+                return false;
+            }
         } else {
             // Get the current user
             $user = $this->getIdentity();

From 9f083ab718e98ab8d07e5cae4c563064f23df932 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:50:07 +0200
Subject: [PATCH 06/18] Change log the changes

---
 CHANGELOG-v3.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG-v3.md b/CHANGELOG-v3.md
index d0726fab5a7..5c135062cae 100644
--- a/CHANGELOG-v3.md
+++ b/CHANGELOG-v3.md
@@ -6,6 +6,8 @@
 - Added `craft\base\ElementInterface::getIsDraft()`.
 - Added `craft\base\ElementInterface::getIsRevision()`.
 - Added the `impersonateUsers` user permission.
+- Added `craft\services\UserPermissions::doesFirstUserHaveMorePermissions()`
+- Added `craft\services\UserPermissions::isImpersonationAllowed()`
 
 ### Fixed
 - Fixed a bug where entry drafts weren’t getting updated slug values once their initial slug had been saved, if their entry type had a custom title format. ([#4373](https://github.com/craftcms/cms/issues/4373))

From 19408c6e70b7a732904d4da56d679b8a85509736 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:50:15 +0200
Subject: [PATCH 07/18] Whoops

---
 src/services/UserPermissions.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index 52b852c961d..08e2f8898d3 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -350,7 +350,7 @@ public function getPermissionsByUserId(int $userId): array
     }
 
     /**
-     * @param User $user
+     * @param User $impersonator
      * @param User $userToImpersonate
      * @return bool
      */

From d5e95d1bcec7feec00c1aff3628135d7076d7600 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 15:57:42 +0200
Subject: [PATCH 08/18] Cleanup

---
 src/services/UserPermissions.php | 3 ++-
 src/web/User.php                 | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index 08e2f8898d3..f35ef61dadc 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -129,7 +129,8 @@ public function getAllPermissions(): array
                             'warning' => Craft::t('app', 'Accounts with this permission could use it to escalate their own permissions.'),
                         ],
                         'impersonateUsers' => [
-                            'label' => Craft::t('app', 'Impersonate users')
+                            'label' => Craft::t('app', 'Impersonate users'),
+                            'warning' => Craft::t('app', 'Once impersonated the user can perform all actions through a child account for which you have assigned them permissions.'),
                         ]
                     ],
                 ],
diff --git a/src/web/User.php b/src/web/User.php
index 029646e7e5b..d8915b34b11 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -240,7 +240,7 @@ public function getIsAdmin(): bool
      *
      * @param string $permissionName
      * @return bool
-     * @throws \craft\errors\MissingComponentException
+     * @throws MissingComponentException
      */
     public function checkPermission(string $permissionName): bool
     {
@@ -253,6 +253,7 @@ public function checkPermission(string $permissionName): bool
                 ->id($previousUserId)
                 ->one();
 
+            // Ensure that the impersonator can also access this resource.
             if (!$user || !$user->can($permissionName)) {
                 return false;
             }

From 522629f655fa5a8e65b28f4450b9fcbcd7d47652 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 16:01:32 +0200
Subject: [PATCH 09/18] Comment cleanup

---
 src/services/UserPermissions.php | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index f35ef61dadc..560a3414ee7 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -377,22 +377,23 @@ public function isImpersonationAllowed(User $impersonator, User $userToImpersona
             return false;
         }
 
-        // Mkay
         return true;
     }
 
     /**
      * Checks whether a user with id ($userId1) has more permissions than a user with id ($userId2)
      *
-     * @param int $userId1
-     * @param int $userId2
+     * @param int $userId1 The user that should have more permissions
+     * @param int $userId2 - The user to compare against.
      * @return bool
      */
     public function doesFirstUserHaveMorePermissions(int $userId1, int $userId2) : bool
     {
+        // Get both their permissions
         $mainUser = $this->getPermissionsByUserId($userId1);
         $secondaryUser = $this->getPermissionsByUserId($userId2);
 
+        // Does $mainUser have more than $secondaryUser
         $difference = array_diff($mainUser, $secondaryUser);
 
         return $difference ? true : false;

From df1c4d7e759f6d61547a8870036cf6f5140aac99 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 16:04:11 +0200
Subject: [PATCH 10/18] Oops. Accidentally overode the craft\web\User docblocks

---
 src/web/User.php | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index d8915b34b11..23cf63a2110 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -238,8 +238,8 @@ public function getIsAdmin(): bool
     /**
      * Returns whether the current user has a given permission.
      *
-     * @param string $permissionName
-     * @return bool
+     * @param string $permissionName The name of the permission.
+     * @return bool Whether the current user has the permission.
      * @throws MissingComponentException
      */
     public function checkPermission(string $permissionName): bool
@@ -310,9 +310,9 @@ public function getHasElevatedSession(): bool
     /**
      * Starts an elevated user session for the current user.
      *
-     * @param string $password
-     * @return bool
-     * @throws UserLockedException
+     * @param string $password the current user’s password
+     * @return bool Whether the password was valid, and the user session has been elevated
+     * @throws UserLockedException if the user is locked.
      * @throws MissingComponentException
      * @throws InvalidArgumentException
      */

From 7d5627663d2a8eb8394d096309af783e38a67b1b Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 16:36:00 +0200
Subject: [PATCH 11/18] I refuse to admit how much time i spent on this >.<

---
 src/web/User.php | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index 23cf63a2110..3d74ceca92e 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -246,15 +246,14 @@ public function checkPermission(string $permissionName): bool
     {
         $user = $this->getIdentity();
 
-
         if ($previousUserId = Craft::$app->getSession()->get(UserElement::IMPERSONATE_KEY)) {
-            $user = UserElement::find()
+            $impersonatingUser = UserElement::find()
                 ->addSelect(['users.password'])
                 ->id($previousUserId)
                 ->one();
 
             // Ensure that the impersonator can also access this resource.
-            if (!$user || !$user->can($permissionName)) {
+            if (!$impersonatingUser || !$impersonatingUser->can($permissionName)) {
                 return false;
             }
         }
@@ -325,6 +324,7 @@ public function startElevatedSession(string $password): bool
             $user = UserElement::find()
                 ->addSelect(['users.password'])
                 ->id($previousUserId)
+                ->admin(true)
                 ->one();
 
             // TODO: @brandonkelly - have a look at this. I removed the ->admin() call in favour of this.

From 5b89c17560fac66832d7b737bc26f82657755839 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 16:50:50 +0200
Subject: [PATCH 12/18] Oops. That wasn't meant to go

---
 src/web/User.php | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/web/User.php b/src/web/User.php
index 3d74ceca92e..9d89b760bb0 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -324,7 +324,6 @@ public function startElevatedSession(string $password): bool
             $user = UserElement::find()
                 ->addSelect(['users.password'])
                 ->id($previousUserId)
-                ->admin(true)
                 ->one();
 
             // TODO: @brandonkelly - have a look at this. I removed the ->admin() call in favour of this.

From 6f8f4ae215390da234d11e8b21d90efbf9bbb73e Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 17:00:44 +0200
Subject: [PATCH 13/18] Note

---
 src/web/User.php | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/web/User.php b/src/web/User.php
index 9d89b760bb0..e54320cb8c9 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -253,6 +253,8 @@ public function checkPermission(string $permissionName): bool
                 ->one();
 
             // Ensure that the impersonator can also access this resource.
+            // TODO: This could cause a problem where if the IMPERSONATE_KEY wasn't flushed
+            // TODO: Then `checkPermission` could be unreliable. 
             if (!$impersonatingUser || !$impersonatingUser->can($permissionName)) {
                 return false;
             }

From a908e04f4f46a561061372df7d5d50b5c2de80fc Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 17:03:26 +0200
Subject: [PATCH 14/18] Dont need this

---
 src/web/User.php | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index e54320cb8c9..919adf9a9bd 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -248,13 +248,12 @@ public function checkPermission(string $permissionName): bool
 
         if ($previousUserId = Craft::$app->getSession()->get(UserElement::IMPERSONATE_KEY)) {
             $impersonatingUser = UserElement::find()
-                ->addSelect(['users.password'])
                 ->id($previousUserId)
                 ->one();
 
             // Ensure that the impersonator can also access this resource.
             // TODO: This could cause a problem where if the IMPERSONATE_KEY wasn't flushed
-            // TODO: Then `checkPermission` could be unreliable. 
+            // TODO: Then `checkPermission` could be unreliable.
             if (!$impersonatingUser || !$impersonatingUser->can($permissionName)) {
                 return false;
             }

From 9a68425c0b150238f555440bd5915e806e815a8c Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 17:11:10 +0200
Subject: [PATCH 15/18] Explanation

---
 src/services/UserPermissions.php | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index 560a3414ee7..79869e1166a 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -351,6 +351,8 @@ public function getPermissionsByUserId(int $userId): array
     }
 
     /**
+     * Checks whether the $impersonator can impersonate the $userToImpersonate
+     *
      * @param User $impersonator
      * @param User $userToImpersonate
      * @return bool

From 75e6c48ee3f66385cf083758d28de845dafc9584 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 19:04:38 +0200
Subject: [PATCH 16/18] Remove unneeded check

---
 src/web/User.php | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index 919adf9a9bd..988a16eb248 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -326,13 +326,6 @@ public function startElevatedSession(string $password): bool
                 ->addSelect(['users.password'])
                 ->id($previousUserId)
                 ->one();
-
-            // TODO: @brandonkelly - have a look at this. I removed the ->admin() call in favour of this.
-            // TODO: It seems more appropriate
-            if (!$user->can('impersonateUsers')) {
-                $this->_handleLoginFailure(UserElement::AUTH_INVALID_CREDENTIALS);
-                return false;
-            }
         } else {
             // Get the current user
             $user = $this->getIdentity();

From d36a1d9aaa6b50c8ce513e091e41b8156de20d69 Mon Sep 17 00:00:00 2001
From: lemiwinkz <giel@yellowflash.net>
Date: Fri, 14 Jun 2019 19:19:21 +0200
Subject: [PATCH 17/18] revert the checkPermission changes.

---
 src/web/User.php | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/web/User.php b/src/web/User.php
index 988a16eb248..392c50499f9 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -240,25 +240,11 @@ public function getIsAdmin(): bool
      *
      * @param string $permissionName The name of the permission.
      * @return bool Whether the current user has the permission.
-     * @throws MissingComponentException
      */
     public function checkPermission(string $permissionName): bool
     {
         $user = $this->getIdentity();
 
-        if ($previousUserId = Craft::$app->getSession()->get(UserElement::IMPERSONATE_KEY)) {
-            $impersonatingUser = UserElement::find()
-                ->id($previousUserId)
-                ->one();
-
-            // Ensure that the impersonator can also access this resource.
-            // TODO: This could cause a problem where if the IMPERSONATE_KEY wasn't flushed
-            // TODO: Then `checkPermission` could be unreliable.
-            if (!$impersonatingUser || !$impersonatingUser->can($permissionName)) {
-                return false;
-            }
-        }
-
         return ($user && $user->can($permissionName));
     }
 

From a01397e8446b5f54b6af0ef75be0b7dda2b1eadc Mon Sep 17 00:00:00 2001
From: Brandon Kelly <brandon@pixelandtonic.com>
Date: Mon, 17 Jun 2019 15:58:52 -0700
Subject: [PATCH 18/18] Cleanup

---
 CHANGELOG-v3.md                     |  5 ++-
 src/controllers/UsersController.php | 17 ++++-----
 src/services/UserPermissions.php    | 54 +----------------------------
 src/services/Users.php              | 40 ++++++++++++++++++++-
 src/web/User.php                    |  4 ---
 5 files changed, 49 insertions(+), 71 deletions(-)

diff --git a/CHANGELOG-v3.md b/CHANGELOG-v3.md
index 5c135062cae..632cc7709f8 100644
--- a/CHANGELOG-v3.md
+++ b/CHANGELOG-v3.md
@@ -3,11 +3,10 @@
 ## Unreleased (3.2)
 
 ### Added
+- Added the “Impersonate users” permission. ([#3501](https://github.com/craftcms/cms/issues/3501))
 - Added `craft\base\ElementInterface::getIsDraft()`.
 - Added `craft\base\ElementInterface::getIsRevision()`.
-- Added the `impersonateUsers` user permission.
-- Added `craft\services\UserPermissions::doesFirstUserHaveMorePermissions()`
-- Added `craft\services\UserPermissions::isImpersonationAllowed()`
+- Added `craft\services\Users::canImpersonate()`.
 
 ### Fixed
 - Fixed a bug where entry drafts weren’t getting updated slug values once their initial slug had been saved, if their entry type had a custom title format. ([#4373](https://github.com/craftcms/cms/issues/4373))
diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php
index 806d01fc921..491da9d5ca1 100644
--- a/src/controllers/UsersController.php
+++ b/src/controllers/UsersController.php
@@ -176,6 +176,7 @@ public function actionLogin()
      * Logs a user in for impersonation. Requires you to be an administrator.
      *
      * @return Response|null
+     * @throws ForbiddenHttpException
      */
     public function actionImpersonate()
     {
@@ -188,15 +189,11 @@ public function actionImpersonate()
 
         $userId = $request->getBodyParam('userId');
 
-        // Are they actually allowed to impersonate this user?
-        $currentUser = $userSession->getIdentity();
-
-        // Fast track people who are admins - so we dont have to call any DB.
-        if (!$currentUser->admin) {
-            $desiredImpersonationSubject = Craft::$app->getUsers()->getUserById($userId);
-            if (!Craft::$app->getUserPermissions()->isImpersonationAllowed($currentUser, $desiredImpersonationSubject)) {
-                throw new ForbiddenHttpException('You do not have suffecient priveleges to impersonate this user');
-            }
+        // Make sure they're allowed to impersonate this user
+        $usersService = Craft::$app->getUsers();
+        $impersonatee = $usersService->getUserById($userId);
+        if (!$usersService->canImpersonate($userSession->getIdentity(), $impersonatee)) {
+            throw new ForbiddenHttpException('You do not have sufficient permissions to impersonate this user');
         }
 
         // Save the original user ID to the session now so User::findIdentity()
@@ -707,7 +704,7 @@ public function actionEditUser($userId = null, User $user = null, array $errors
             }
 
             if (!$isCurrentUser) {
-                if (Craft::$app->getUserPermissions()->isImpersonationAllowed($currentUser, $user)) {
+                if (Craft::$app->getUsers()->canImpersonate($currentUser, $user)) {
                     $sessionActions[] = [
                         'action' => 'users/impersonate',
                         'label' => Craft::t('app', 'Login as {user}', ['user' => $user->getName()])
diff --git a/src/services/UserPermissions.php b/src/services/UserPermissions.php
index 79869e1166a..7fd16940b53 100644
--- a/src/services/UserPermissions.php
+++ b/src/services/UserPermissions.php
@@ -130,8 +130,7 @@ public function getAllPermissions(): array
                         ],
                         'impersonateUsers' => [
                             'label' => Craft::t('app', 'Impersonate users'),
-                            'warning' => Craft::t('app', 'Once impersonated the user can perform all actions through a child account for which you have assigned them permissions.'),
-                        ]
+                        ],
                     ],
                 ],
                 'deleteUsers' => [
@@ -350,57 +349,6 @@ public function getPermissionsByUserId(int $userId): array
         return $this->_permissionsByUserId[$userId];
     }
 
-    /**
-     * Checks whether the $impersonator can impersonate the $userToImpersonate
-     *
-     * @param User $impersonator
-     * @param User $userToImpersonate
-     * @return bool
-     */
-    public function isImpersonationAllowed(User $impersonator, User $userToImpersonate) : bool
-    {
-        // Too easy
-        if ($impersonator->admin) {
-            return true;
-        }
-
-        // Is the impersonator not an admin but the impersonatee is - then no.
-        if ($userToImpersonate->admin) {
-            return false;
-        }
-
-        // Be gone!
-        if (!$impersonator->can('impersonateUsers')) {
-            return false;
-        }
-
-        // The user that is going to get impersonated cannot have more rights than the impersonator.
-        if ($this->doesFirstUserHaveMorePermissions($userToImpersonate->id, $impersonator->id)) {
-            return false;
-        }
-
-        return true;
-    }
-
-    /**
-     * Checks whether a user with id ($userId1) has more permissions than a user with id ($userId2)
-     *
-     * @param int $userId1 The user that should have more permissions
-     * @param int $userId2 - The user to compare against.
-     * @return bool
-     */
-    public function doesFirstUserHaveMorePermissions(int $userId1, int $userId2) : bool
-    {
-        // Get both their permissions
-        $mainUser = $this->getPermissionsByUserId($userId1);
-        $secondaryUser = $this->getPermissionsByUserId($userId2);
-
-        // Does $mainUser have more than $secondaryUser
-        $difference = array_diff($mainUser, $secondaryUser);
-
-        return $difference ? true : false;
-    }
-
     /**
      * Returns whether a given user has a given permission.
      *
diff --git a/src/services/Users.php b/src/services/Users.php
index 89a306141d5..dc53b7c7143 100644
--- a/src/services/Users.php
+++ b/src/services/Users.php
@@ -1067,6 +1067,44 @@ public function saveLayout(FieldLayout $layout)
         return true;
     }
 
+    /**
+     * Returns whether a user is allowed to impersonate another user.
+     *
+     * @param User $impersonator
+     * @param User $impersonatee
+     * @return bool
+     * @since 3.2
+     */
+    public function canImpersonate(User $impersonator, User $impersonatee): bool
+    {
+        // Admins can do whatever they want
+        if ($impersonator->admin) {
+            return true;
+        }
+
+        // Only admins are allowed to impersonate another admin
+        if ($impersonatee->admin) {
+            return false;
+        }
+
+        // impersonateUsers permission is obviously required
+        if (!$impersonator->can('impersonateUsers')) {
+            return false;
+        }
+
+        // Make sure the impersonator has at least all the same permissions as the impersonatee
+        $permissionsService = Craft::$app->getUserPermissions();
+        $impersonatorPermissions = array_flip($permissionsService->getPermissionsByUserId($impersonator->id));
+        $impersonateePermissions = $permissionsService->getPermissionsByUserId($impersonatee->id);
+
+        foreach ($impersonateePermissions as $permission) {
+            if (!isset($impersonatorPermissions[$permission])) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 
     /**
      * Prune a deleted field from user group layout.
@@ -1208,4 +1246,4 @@ private function _getUserUrl(User $user, string $action): string
 
         return UrlHelper::siteUrl($path, $params, $scheme, $siteId);
     }
-}
\ No newline at end of file
+}
diff --git a/src/web/User.php b/src/web/User.php
index 392c50499f9..a978033374a 100644
--- a/src/web/User.php
+++ b/src/web/User.php
@@ -12,7 +12,6 @@
 use craft\db\Query;
 use craft\db\Table;
 use craft\elements\User as UserElement;
-use craft\errors\MissingComponentException;
 use craft\errors\UserLockedException;
 use craft\events\LoginFailureEvent;
 use craft\helpers\ConfigHelper;
@@ -20,7 +19,6 @@
 use craft\helpers\UrlHelper;
 use craft\helpers\User as UserHelper;
 use craft\validators\UserPasswordValidator;
-use yii\base\InvalidArgumentException;
 use yii\web\Cookie;
 
 /**
@@ -299,8 +297,6 @@ public function getHasElevatedSession(): bool
      * @param string $password the current user’s password
      * @return bool Whether the password was valid, and the user session has been elevated
      * @throws UserLockedException if the user is locked.
-     * @throws MissingComponentException
-     * @throws InvalidArgumentException
      */
     public function startElevatedSession(string $password): bool
     {