User edit permission split #2620

tankerkiller125 · 2021-02-21T02:14:25Z

**Fixes #1965 **

Changes proposed in this pull request:

Currently only prevents non-admins from editing admin
Splits the edit.user permission into three permissions (username, credentials, groups)

Reviewers should focus on:

Is this the best way of doing this?

Screenshot

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).

askvortsov1

This also needs some integration tests for EditUserController

js/src/admin/components/PermissionGrid.js

src/Api/Serializer/UserSerializer.php

src/User/Command/EditUserHandler.php

js/src/admin/components/PermissionGrid.js

locale/core.yml

js/src/common/models/User.js

askvortsov1

Code looks fine, but since it's dealing with API stuff (and quite important API stuff at that) it still needs integration tests

[ci skip] [skip ci]

askvortsov1

Note for reviewers: the vast majority of changes is tests

askvortsov1 · 2021-02-26T01:58:07Z

src/User/Command/EditUserHandler.php

@@ -82,7 +82,8 @@ public function handle(EditUser $command)
            }
        }

-        if ($actor->isAdmin() && ! empty($attributes['isEmailConfirmed'])) {
+        if (!empty($attributes['isEmailConfirmed'])) {


This change allows us to more cleanly test editing the user

askvortsov1 · 2021-02-26T02:01:33Z

src/User/Command/EditUserHandler.php

            $newGroupIds = [];
            foreach ($relationships['groups']['data'] as $group) {
                if ($id = Arr::get($group, 'id')) {
                    $newGroupIds[] = $id;
                }
            }

+            $adminChanged = in_array('1', array_diff($oldGroupIds, $newGroupIds)) || in_array('1', array_diff($newGroupIds, $oldGroupIds));


Should this be moved out to a guard (like SelfDemotionGuard)? We decided to leave it here since that felt simpler. Alternatively, we could move SelfDemotionGuard here for simplicity?

I don't feel strongly about this, but I wouldn't remove the SelfDemotionGuard either, maybe just leave things as is ?

askvortsov1 · 2021-02-26T02:05:26Z

tests/integration/api/users/UpdateTest.php

@@ -26,7 +27,24 @@ protected function setUp(): void
        $this->prepareDatabase([
            'users' => [
                $this->normalUser(),
-            ]
+                [


We decided to put this here and not in RetrievesAuthorizedUsers since it's only used in one test case

askvortsov1 · 2021-02-26T02:10:23Z

We also added restrictions that stop non-admins from editing credentials of admins, and stop non-admins from promoting/demoting admins

askvortsov1

On a side note, when this is merged it should be squashed into 3 commits: one for the split, the other 2 for the other restrictions introduced.

askvortsov1 · 2021-02-26T05:12:11Z

src/Api/Serializer/UserSerializer.php

+            'joinTime'           => $this->formatDate($user->joined_at),
+            'discussionCount'    => (int) $user->discussion_count,
+            'commentCount'       => (int) $user->comment_count,
+            'canEdit'            => $this->actor->can('edit', $user),


Should this be serialized as canEditAttributes? I think that might be more consistent.

I think I didn't do that to prevent breaking extensions that might use canEdit.

We're not losing the method on the frontend user model, so we'd be preserving this for API consumers. But the meaning of the value has changed, so I'm not sure it's worth it to preserve; renaming would force consumers to adapt to the new system.

we should rename this, it's quiet confusing right now what the difference between canEdit and canEditAttributes is, on the frontend canEdit is true if the user has the ability to edit any of the three new permissions, on the backend (serialization) it just refers to the canEditAttributes ability.

Are we gonna keep canEdit the way it is on the frontend ? (ie any of the three abilities), I'm assuming we're deprecating it, if so, can we comment on the frontend model that it's deprecated please ?

askvortsov1

Temporarily removing approval as per #2620 (comment), we should either rename the value returned by the API to canEditAttributes, or rename the frontend methods to canEdit (and use something else for the combined method name).

The second option might actually be better; that way, we are consistent across permission name, frontend, and backend.

SychO9

The backend code looks solid, but the frontend can be improved, here are some issues I noticed while testing the permissions:

The EditUserModal is empty if the only permission given to the user is edit attributes.
By giving the user only permission to edit credentials, they can open the EditUserModal on normal members, but can't actually change credentials. (haven't looked too deep into it, but the frontend seems to still send an array of groups relationship which naturally fails).
Giving a user edit credentials permission does not allow to edit an admin's credentials (as it should) but once the user is given edit attributes permission, they can open the admin's EditUserModal (it fails on edit of course, because the backend seems solid in the behaviour).
If a user have permission to edit groups only, and they try to edit groups (anything but the admin group ofc) they're not allowed to (because the username is included in the payload).

locale/core.yml

SychO9 · 2021-02-27T21:11:46Z

src/Api/Serializer/UserSerializer.php

+            'joinTime'           => $this->formatDate($user->joined_at),
+            'discussionCount'    => (int) $user->discussion_count,
+            'commentCount'       => (int) $user->comment_count,
+            'canEdit'            => $this->actor->can('edit', $user),


we should rename this, it's quiet confusing right now what the difference between canEdit and canEditAttributes is, on the frontend canEdit is true if the user has the ability to edit any of the three new permissions, on the backend (serialization) it just refers to the canEditAttributes ability.

Are we gonna keep canEdit the way it is on the frontend ? (ie any of the three abilities), I'm assuming we're deprecating it, if so, can we comment on the frontend model that it's deprecated please ?

askvortsov1 · 2021-03-01T03:43:03Z

The EditUserModal is empty if the only permission given to the user is edit attributes.

Edited to show a message

Are we gonna keep canEdit the way it is on the frontend ? (ie any of the three abilities), I'm assuming we're deprecating it, if so, can we comment on the frontend model that it's deprecated please ?

Renamed everything to editUser for consistency.

All the other stuff has been fixed, at least based on the tests I ran locally.

SychO9

Code looks good, did some tests locally as well.

SychO9 · 2021-03-01T20:31:19Z

src/User/Command/EditUserHandler.php

        if (isset($attributes['password'])) {
-            $actor->assertPermission($canEdit);
+            $actor->assertPermission($actor->can('editCredentials', $user));
            $user->changePassword($attributes['password']);


off topic observation: we force users to use the change password button (reset password email) on the frontend but not on the backend, so they could just send a payload and directly edit it.

I don't know if this was done on purpose.

I don't disagree, but this PR already contains a lot so I'll leave this alone for now

Yea, I wouldn't want that changed here anyway :P was an observation as it looked odd.

SychO9 · 2021-03-01T20:32:04Z

src/User/Command/EditUserHandler.php

            $newGroupIds = [];
            foreach ($relationships['groups']['data'] as $group) {
                if ($id = Arr::get($group, 'id')) {
                    $newGroupIds[] = $id;
                }
            }

+            $adminChanged = in_array('1', array_diff($oldGroupIds, $newGroupIds)) || in_array('1', array_diff($newGroupIds, $oldGroupIds));


I don't feel strongly about this, but I wouldn't remove the SelfDemotionGuard either, maybe just leave things as is ?

Initial user permission split

c93436b

tankerkiller125 mentioned this pull request Feb 21, 2021

Fix Mods being able to edit Admins, Split permissions #2113

Closed

3 tasks

tankerkiller125 changed the title ~~Initial user permission split~~ User edit permission split Feb 21, 2021

tankerkiller125 requested review from askvortsov1 and luceos February 21, 2021 02:15

Fix handler

423ee74

askvortsov1 requested changes Feb 21, 2021

View reviewed changes

js/src/admin/components/PermissionGrid.js Outdated Show resolved Hide resolved

src/Api/Serializer/UserSerializer.php Outdated Show resolved Hide resolved

src/User/Command/EditUserHandler.php Outdated Show resolved Hide resolved

More fixes

b86fd47

askvortsov1 requested changes Feb 21, 2021

View reviewed changes

js/src/admin/components/PermissionGrid.js Show resolved Hide resolved

locale/core.yml Outdated Show resolved Hide resolved

locale/core.yml Outdated Show resolved Hide resolved

Minor fixes

9dbedc4

tankerkiller125 requested a review from askvortsov1 February 21, 2021 23:41

askvortsov1 requested changes Feb 21, 2021

View reviewed changes

js/src/common/models/User.js Outdated Show resolved Hide resolved

Another minor fix

149c6f3

tankerkiller125 requested a review from askvortsov1 February 23, 2021 22:54

askvortsov1 requested changes Feb 23, 2021

View reviewed changes

tankerkiller125 and others added 4 commits February 25, 2021 20:34

Add Test

a8eb8ed

Only Admins can edit Admins Credentials

15976de

Only Admins can Promote/Demote to/from Admin

e803a70

Apply fixes from StyleCI

9af11c6

[ci skip] [skip ci]

tankerkiller125 requested review from askvortsov1 and SychO9 February 26, 2021 01:54

Add additional test

9e39119

askvortsov1 approved these changes Feb 26, 2021

View reviewed changes

askvortsov1 requested changes Feb 27, 2021

View reviewed changes

SychO9 requested changes Feb 27, 2021

View reviewed changes

askvortsov1 added 3 commits February 28, 2021 22:29

Group edit user items together

3c211a8

Fix permission title

3ee005c

Use consistent naming for permissions

c6ff98b

askvortsov1 added 2 commits February 28, 2021 22:35

Enforce permissions in edit user modal UX

34e112a

Show message if no fields available to edit.

9e502c8

askvortsov1 approved these changes Mar 1, 2021

View reviewed changes

askvortsov1 requested a review from SychO9 March 1, 2021 03:43

SychO9 approved these changes Mar 1, 2021

View reviewed changes

Minor cleanup

2ff95b6

askvortsov1 merged commit 9627eb7 into master Mar 1, 2021

askvortsov1 deleted the mk/1965-split-permissions branch March 1, 2021 20:52

Ornanovitch mentioned this pull request May 13, 2021

edit a user without "view hidden group bages" permission can break membership of the user flarum/issue-archive#149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User edit permission split #2620

User edit permission split #2620

tankerkiller125 commented Feb 21, 2021

askvortsov1 left a comment

askvortsov1 left a comment

askvortsov1 left a comment

askvortsov1 Feb 26, 2021

askvortsov1 Feb 26, 2021

SychO9 Mar 1, 2021

askvortsov1 Feb 26, 2021

askvortsov1 commented Feb 26, 2021

askvortsov1 left a comment

askvortsov1 Feb 26, 2021

tankerkiller125 Feb 26, 2021

askvortsov1 Feb 26, 2021

SychO9 Feb 27, 2021

askvortsov1 left a comment

SychO9 left a comment •

edited

Loading

SychO9 Feb 27, 2021

askvortsov1 commented Mar 1, 2021

SychO9 left a comment

SychO9 Mar 1, 2021

askvortsov1 Mar 1, 2021 •

edited

Loading

SychO9 Mar 1, 2021

SychO9 Mar 1, 2021

User edit permission split #2620

User edit permission split #2620

Conversation

tankerkiller125 commented Feb 21, 2021

askvortsov1 left a comment

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 commented Feb 26, 2021

askvortsov1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

SychO9 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 commented Mar 1, 2021

SychO9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SychO9 left a comment •

edited

Loading

askvortsov1 Mar 1, 2021 •

edited

Loading