User permission cleanup

Also dropped assignUserGroups in favor of explicitly allowing each individual group assignment permission
craftcms · Feb 4, 2022 · bca8c37 · bca8c37
1 parent c23c72c
commit bca8c37
Show file tree

Hide file tree

Showing 13 changed files with 362 additions and 371 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -132,6 +132,7 @@
 - Added `craft\elements\conditions\users\UserCondition`.
 - Added `craft\elements\conditions\users\UsernameConditionRule`.
 - Added `craft\elements\User::$active`.
+- Added `craft\elements\User::canAssignUserGroups()`.
 - Added `craft\elements\User::getIsCredentialed()`.
 - Added `craft\elements\User::STATUS_INACTIVE`.
 - Added `craft\errors\FsException`.
@@ -576,6 +577,9 @@
 - Removed Matrix block queries’ `ownerLocale` param. The `site` or `siteId` param can be used instead.
 - Removed Matrix block queries’ `ownerSite` param. The `site` param can be used instead.
 - Removed Matrix block queries’ `ownerSiteId` param. The `siteId` param can be used instead.
+- Removed the `assignUserGroups` user permission, which authorized users to assign other users to their own groups. Authorization must now be explicitly granted for each group.
+- Removed the `customizeSources` user permission. Only admins can customize element sources now, and only from an environment that allows admin changes.
+- Removed the `publishPeerEntryDrafts:<uid>` permissions, as they were pointless. (If a user is authorized to save an entry and view other users’ drafts of it, there’s nothing stopping them from making the same changes themselves.)
 - Removed support for the `staticRows` editable table setting. `allowAdd`, `allowDelete`, and `allowReorder` can be used instead.
 - Removed support for the `CRAFT_LOCALE` PHP constant. `CRAFT_SITE` can be used instead.
 - Removed `Craft::Client`. `Pro` can be used instead.

diff --git a/src/controllers/ElementIndexSettingsController.php b/src/controllers/ElementIndexSettingsController.php
@@ -34,6 +34,7 @@ public function beforeAction($action): bool
         }
 
         $this->requireAcceptsJson();
+        $this->requireAdmin();
 
         return true;
     }
@@ -45,8 +46,6 @@ public function beforeAction($action): bool
      */
     public function actionGetCustomizeSourcesModalData(): Response
     {
-        $this->requirePermission('customizeSources');
-
         /** @var string|ElementInterface $elementType */
         $elementType = $this->elementType();
         $conditionsService = Craft::$app->getConditions();
@@ -139,8 +138,6 @@ public function actionGetCustomizeSourcesModalData(): Response
      */
     public function actionSaveCustomizeSourcesModalSettings(): Response
     {
-        $this->requirePermission('customizeSources');
-
         $elementType = $this->elementType();
 
         // Get the old source configs

diff --git a/src/controllers/UserSettingsController.php b/src/controllers/UserSettingsController.php
@@ -89,6 +89,14 @@ public function actionSaveGroup(): ?Response
             }
         }
 
+        // assignNewUserGroup => assignUserGroup:<uid>
+        if (!$groupId) {
+            $assignNewGroupKey = array_search('assignNewUserGroup', $permissions);
+            if ($assignNewGroupKey !== false) {
+                $permissions[$assignNewGroupKey] = "assignUserGroup:$group->uid";
+            }
+        }
+
         Craft::$app->getUserPermissions()->saveGroupPermissions($group->id, $permissions);
 
         $this->setSuccessFlash(Craft::t('app', 'Group saved.'));

diff --git a/src/controllers/UsersController.php b/src/controllers/UsersController.php
@@ -694,14 +694,14 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
         // ---------------------------------------------------------------------
 
         $edition = Craft::$app->getEdition();
-        $userSession = Craft::$app->getUser();
+        $currentUser = Craft::$app->getUser()->getIdentity();
 
         if ($user === null) {
             // Are we editing a specific user account?
             if ($userId !== null) {
                 $user = User::find()
                     ->addSelect(['users.password', 'users.passwordResetRequired'])
-                    ->id($userId === 'current' ? $userSession->getId() : $userId)
+                    ->id($userId === 'current' ? $currentUser->id : $userId)
                     ->status(null)
                     ->one();
             } else if ($edition === Craft::Pro) {
@@ -729,7 +729,6 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
             }
         }
 
-        $currentUser = $userSession->getIdentity();
         $canAdministrateUsers = $currentUser->can('administrateUsers');
         $canModerateUsers = $currentUser->can('moderateUsers');
 
@@ -847,7 +846,7 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
                     ];
                 }
 
-                if ($isCurrentUser || $userSession->checkPermission('deleteUsers')) {
+                if ($isCurrentUser || $currentUser->can('deleteUsers')) {
                     $destructiveActions[] = [
                         'id' => 'delete-btn',
                         'label' => Craft::t('app', 'Delete…'),
@@ -910,13 +909,13 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
         $tabs += $form->getTabMenu();
 
         // Show the permission tab for the users that can change them on Craft Pro editions
-        if (
+        $canAssignUserGroups = $currentUser->canAssignUserGroups();
+        $showPermissionsTab = (
             $edition === Craft::Pro &&
-            (
-                $userSession->checkPermission('assignUserPermissions') ||
-                $userSession->checkPermission('assignUserGroups')
-            )
-        ) {
+            ($currentUser->can('assignUserPermissions') || $canAssignUserGroups)
+        );
+
+        if ($showPermissionsTab) {
             $tabs['perms'] = [
                 'label' => Craft::t('app', 'Permissions'),
                 'url' => '#perms',
@@ -1037,6 +1036,8 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
             'tabs',
             'selectedTab',
             'showPhotoField',
+            'showPermissionsTab',
+            'canAssignUserGroups',
             'fieldsHtml'
         ));
     }
@@ -2099,11 +2100,6 @@ private function _saveUserPermissions(User $user, User $currentUser): void
      */
     private function _saveUserGroups(User $user, User $currentUser): void
     {
-        if (!$currentUser->can('assignUserGroups')) {
-            return;
-        }
-
-        // Save any user groups
         $groupIds = $this->request->getBodyParam('groups');
 
         if ($groupIds === null) {
@@ -2129,10 +2125,7 @@ private function _saveUserGroups(User $user, User $currentUser): void
                 $hasNewGroups = true;
 
                 // Make sure the current user is in the group or has permission to assign it
-                if (
-                    !$currentUser->isInGroup($groupId) &&
-                    !$currentUser->can("assignUserGroup:$group->uid")
-                ) {
+                if (!$currentUser->can("assignUserGroup:$group->uid")) {
                     throw new ForbiddenHttpException("Your account doesn't have permission to assign user group “{$group->name}” to a user.");
                 }
             }

diff --git a/src/elements/User.php b/src/elements/User.php
@@ -1349,6 +1349,23 @@ public function can(string $permission): bool
         return true;
     }
 
+    /**
+     * Returns whether the user is authorized to assign any user groups to users.
+     *
+     * @return bool
+     * @since 4.0.0
+     */
+    public function canAssignUserGroups(): bool
+    {
+        foreach (Craft::$app->getUserGroups()->getAllGroups() as $group) {
+            if ($this->can("assignUserGroup:$group->uid")) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * Returns whether the user has shunned a given message.
      *

diff --git a/src/migrations/m220123_213619_update_permissions.php b/src/migrations/m220123_213619_update_permissions.php
@@ -18,7 +18,10 @@ class m220123_213619_update_permissions extends Migration
     public function safeUp(): bool
     {
         $map = [];
-        $delete = [];
+        $delete = [
+            'assignUserGroups',
+            'customizeSources',
+        ];
 
         foreach (Craft::$app->getVolumes()->getAllVolumes() as $volume) {
             $map += [
@@ -95,11 +98,9 @@ public function safeUp(): bool
             $delete[] = $oldPermission;
         }
 
-        if (!empty($delete)) {
-            $this->delete(Table::USERPERMISSIONS, [
-                'name' => $delete,
-            ]);
-        }
+        $this->delete(Table::USERPERMISSIONS, [
+            'name' => $delete,
+        ]);
 
         // Don't make the same config changes twice
         $projectConfig = Craft::$app->getProjectConfig();
@@ -108,27 +109,25 @@ public function safeUp(): bool
         if (version_compare($schemaVersion, '4.0.0', '<')) {
             foreach ($projectConfig->get('users.groups') ?? [] as $uid => $group) {
                 $groupPermissions = array_flip($group['permissions'] ?? []);
-                $changed = false;
 
                 foreach ($map as $oldPermission => $newPermissions) {
                     if (isset($groupPermissions[$oldPermission])) {
                         foreach ($newPermissions as $newPermission) {
                             $groupPermissions[$newPermission] = true;
                         }
-                        $changed = true;
                     }
                 }
 
                 foreach ($delete as $permission) {
                     if (isset($groupPermissions[$permission])) {
                         unset($groupPermissions[$permission]);
-                        $changed = true;
                     }
                 }
 
-                if ($changed) {
-                    $projectConfig->set("users.groups.$uid.permissions", array_keys($groupPermissions));
-                }
+                // assignUserGroup:<uid> permissions are explicitly required going forward
+                $groupPermissions["assignUserGroup:$uid"] = true;
+
+                $projectConfig->set("users.groups.$uid.permissions", array_keys($groupPermissions));
             }
         }
 

diff --git a/src/services/UserGroups.php b/src/services/UserGroups.php
@@ -64,6 +64,10 @@ class UserGroups extends Component
      */
     public function getAllGroups(): array
     {
+        if (Craft::$app->getEdition() !== Craft::Pro) {
+            return [];
+        }
+
         $results = $this->_createUserGroupsQuery()
             ->orderBy(['name' => SORT_ASC])
             ->all();
@@ -97,10 +101,7 @@ public function getAssignableGroups(?User $user = null): array
 
         foreach ($this->getAllGroups() as $group) {
             if (
-                ($currentUser !== null && (
-                        $currentUser->isInGroup($group) ||
-                        $currentUser->can('assignUserGroup:' . $group->uid)
-                    )) ||
+                ($currentUser !== null && $currentUser->can("assignUserGroup:$group->uid")) ||
                 ($user !== null && $user->isInGroup($group))
             ) {
                 $assignableGroups[] = $group;