Skip to content

Commit bca8c37

Browse files
brandonkellybrandonkelly
committed
User permission cleanup
Also dropped assignUserGroups in favor of explicitly allowing each individual group assignment permission
1 parent c23c72c commit bca8c37

File tree

13 files changed

+362
-371
lines changed

13 files changed

+362
-371
lines changed

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@
132132
- Added `craft\elements\conditions\users\UserCondition`.
133133
- Added `craft\elements\conditions\users\UsernameConditionRule`.
134134
- Added `craft\elements\User::$active`.
135+
- Added `craft\elements\User::canAssignUserGroups()`.
135136
- Added `craft\elements\User::getIsCredentialed()`.
136137
- Added `craft\elements\User::STATUS_INACTIVE`.
137138
- Added `craft\errors\FsException`.
@@ -576,6 +577,9 @@
576577
- Removed Matrix block queries’ `ownerLocale` param. The `site` or `siteId` param can be used instead.
577578
- Removed Matrix block queries’ `ownerSite` param. The `site` param can be used instead.
578579
- Removed Matrix block queries’ `ownerSiteId` param. The `siteId` param can be used instead.
580+
- 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.
581+
- Removed the `customizeSources` user permission. Only admins can customize element sources now, and only from an environment that allows admin changes.
582+
- 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.)
579583
- Removed support for the `staticRows` editable table setting. `allowAdd`, `allowDelete`, and `allowReorder` can be used instead.
580584
- Removed support for the `CRAFT_LOCALE` PHP constant. `CRAFT_SITE` can be used instead.
581585
- Removed `Craft::Client`. `Pro` can be used instead.

‎src/controllers/ElementIndexSettingsController.php

+1-4
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ public function beforeAction($action): bool
3434
}
3535

3636
$this->requireAcceptsJson();
37+
$this->requireAdmin();
3738

3839
return true;
3940
}
@@ -45,8 +46,6 @@ public function beforeAction($action): bool
4546
*/
4647
public function actionGetCustomizeSourcesModalData(): Response
4748
{
48-
$this->requirePermission('customizeSources');
49-
5049
/** @var string|ElementInterface $elementType */
5150
$elementType = $this->elementType();
5251
$conditionsService = Craft::$app->getConditions();
@@ -139,8 +138,6 @@ public function actionGetCustomizeSourcesModalData(): Response
139138
*/
140139
public function actionSaveCustomizeSourcesModalSettings(): Response
141140
{
142-
$this->requirePermission('customizeSources');
143-
144141
$elementType = $this->elementType();
145142

146143
// Get the old source configs

‎src/controllers/UserSettingsController.php

+8
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,14 @@ public function actionSaveGroup(): ?Response
8989
}
9090
}
9191

92+
// assignNewUserGroup => assignUserGroup:<uid>
93+
if (!$groupId) {
94+
$assignNewGroupKey = array_search('assignNewUserGroup', $permissions);
95+
if ($assignNewGroupKey !== false) {
96+
$permissions[$assignNewGroupKey] = "assignUserGroup:$group->uid";
97+
}
98+
}
99+
92100
Craft::$app->getUserPermissions()->saveGroupPermissions($group->id, $permissions);
93101

94102
$this->setSuccessFlash(Craft::t('app', 'Group saved.'));

‎src/controllers/UsersController.php

+12-19
Original file line numberDiff line numberDiff line change
@@ -694,14 +694,14 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
694694
// ---------------------------------------------------------------------
695695

696696
$edition = Craft::$app->getEdition();
697-
$userSession = Craft::$app->getUser();
697+
$currentUser = Craft::$app->getUser()->getIdentity();
698698

699699
if ($user === null) {
700700
// Are we editing a specific user account?
701701
if ($userId !== null) {
702702
$user = User::find()
703703
->addSelect(['users.password', 'users.passwordResetRequired'])
704-
->id($userId === 'current' ? $userSession->getId() : $userId)
704+
->id($userId === 'current' ? $currentUser->id : $userId)
705705
->status(null)
706706
->one();
707707
} else if ($edition === Craft::Pro) {
@@ -729,7 +729,6 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
729729
}
730730
}
731731

732-
$currentUser = $userSession->getIdentity();
733732
$canAdministrateUsers = $currentUser->can('administrateUsers');
734733
$canModerateUsers = $currentUser->can('moderateUsers');
735734

@@ -847,7 +846,7 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
847846
];
848847
}
849848

850-
if ($isCurrentUser || $userSession->checkPermission('deleteUsers')) {
849+
if ($isCurrentUser || $currentUser->can('deleteUsers')) {
851850
$destructiveActions[] = [
852851
'id' => 'delete-btn',
853852
'label' => Craft::t('app', 'Delete…'),
@@ -910,13 +909,13 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
910909
$tabs += $form->getTabMenu();
911910

912911
// Show the permission tab for the users that can change them on Craft Pro editions
913-
if (
912+
$canAssignUserGroups = $currentUser->canAssignUserGroups();
913+
$showPermissionsTab = (
914914
$edition === Craft::Pro &&
915-
(
916-
$userSession->checkPermission('assignUserPermissions') ||
917-
$userSession->checkPermission('assignUserGroups')
918-
)
919-
) {
915+
($currentUser->can('assignUserPermissions') || $canAssignUserGroups)
916+
);
917+
918+
if ($showPermissionsTab) {
920919
$tabs['perms'] = [
921920
'label' => Craft::t('app', 'Permissions'),
922921
'url' => '#perms',
@@ -1037,6 +1036,8 @@ public function actionEditUser($userId = null, ?User $user = null, ?array $error
10371036
'tabs',
10381037
'selectedTab',
10391038
'showPhotoField',
1039+
'showPermissionsTab',
1040+
'canAssignUserGroups',
10401041
'fieldsHtml'
10411042
));
10421043
}
@@ -2099,11 +2100,6 @@ private function _saveUserPermissions(User $user, User $currentUser): void
20992100
*/
21002101
private function _saveUserGroups(User $user, User $currentUser): void
21012102
{
2102-
if (!$currentUser->can('assignUserGroups')) {
2103-
return;
2104-
}
2105-
2106-
// Save any user groups
21072103
$groupIds = $this->request->getBodyParam('groups');
21082104

21092105
if ($groupIds === null) {
@@ -2129,10 +2125,7 @@ private function _saveUserGroups(User $user, User $currentUser): void
21292125
$hasNewGroups = true;
21302126

21312127
// Make sure the current user is in the group or has permission to assign it
2132-
if (
2133-
!$currentUser->isInGroup($groupId) &&
2134-
!$currentUser->can("assignUserGroup:$group->uid")
2135-
) {
2128+
if (!$currentUser->can("assignUserGroup:$group->uid")) {
21362129
throw new ForbiddenHttpException("Your account doesn't have permission to assign user group “{$group->name}” to a user.");
21372130
}
21382131
}

‎src/elements/User.php

+17
Original file line numberDiff line numberDiff line change
@@ -1349,6 +1349,23 @@ public function can(string $permission): bool
13491349
return true;
13501350
}
13511351

1352+
/**
1353+
* Returns whether the user is authorized to assign any user groups to users.
1354+
*
1355+
* @return bool
1356+
* @since 4.0.0
1357+
*/
1358+
public function canAssignUserGroups(): bool
1359+
{
1360+
foreach (Craft::$app->getUserGroups()->getAllGroups() as $group) {
1361+
if ($this->can("assignUserGroup:$group->uid")) {
1362+
return true;
1363+
}
1364+
}
1365+
1366+
return false;
1367+
}
1368+
13521369
/**
13531370
* Returns whether the user has shunned a given message.
13541371
*

‎src/migrations/m220123_213619_update_permissions.php

+11-12
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ class m220123_213619_update_permissions extends Migration
1818
public function safeUp(): bool
1919
{
2020
$map = [];
21-
$delete = [];
21+
$delete = [
22+
'assignUserGroups',
23+
'customizeSources',
24+
];
2225

2326
foreach (Craft::$app->getVolumes()->getAllVolumes() as $volume) {
2427
$map += [
@@ -95,11 +98,9 @@ public function safeUp(): bool
9598
$delete[] = $oldPermission;
9699
}
97100

98-
if (!empty($delete)) {
99-
$this->delete(Table::USERPERMISSIONS, [
100-
'name' => $delete,
101-
]);
102-
}
101+
$this->delete(Table::USERPERMISSIONS, [
102+
'name' => $delete,
103+
]);
103104

104105
// Don't make the same config changes twice
105106
$projectConfig = Craft::$app->getProjectConfig();
@@ -108,27 +109,25 @@ public function safeUp(): bool
108109
if (version_compare($schemaVersion, '4.0.0', '<')) {
109110
foreach ($projectConfig->get('users.groups') ?? [] as $uid => $group) {
110111
$groupPermissions = array_flip($group['permissions'] ?? []);
111-
$changed = false;
112112

113113
foreach ($map as $oldPermission => $newPermissions) {
114114
if (isset($groupPermissions[$oldPermission])) {
115115
foreach ($newPermissions as $newPermission) {
116116
$groupPermissions[$newPermission] = true;
117117
}
118-
$changed = true;
119118
}
120119
}
121120

122121
foreach ($delete as $permission) {
123122
if (isset($groupPermissions[$permission])) {
124123
unset($groupPermissions[$permission]);
125-
$changed = true;
126124
}
127125
}
128126

129-
if ($changed) {
130-
$projectConfig->set("users.groups.$uid.permissions", array_keys($groupPermissions));
131-
}
127+
// assignUserGroup:<uid> permissions are explicitly required going forward
128+
$groupPermissions["assignUserGroup:$uid"] = true;
129+
130+
$projectConfig->set("users.groups.$uid.permissions", array_keys($groupPermissions));
132131
}
133132
}
134133

‎src/services/UserGroups.php

+5-4
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ class UserGroups extends Component
6464
*/
6565
public function getAllGroups(): array
6666
{
67+
if (Craft::$app->getEdition() !== Craft::Pro) {
68+
return [];
69+
}
70+
6771
$results = $this->_createUserGroupsQuery()
6872
->orderBy(['name' => SORT_ASC])
6973
->all();
@@ -97,10 +101,7 @@ public function getAssignableGroups(?User $user = null): array
97101

98102
foreach ($this->getAllGroups() as $group) {
99103
if (
100-
($currentUser !== null && (
101-
$currentUser->isInGroup($group) ||
102-
$currentUser->can('assignUserGroup:' . $group->uid)
103-
)) ||
104+
($currentUser !== null && $currentUser->can("assignUserGroup:$group->uid")) ||
104105
($user !== null && $user->isInGroup($group))
105106
) {
106107
$assignableGroups[] = $group;

0 commit comments

Comments
 (0)