Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: master
from

Conversation

@tankerkiller125
Copy link
Member

tankerkiller125 commented Apr 2, 2020

**Fixes #1965 **

Changes proposed in this pull request:

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

Reviewers should focus on:

  • Is this the best way of doing this?
  • Do we want to limit what shows up on the edit screen to only what users have permissions to do?

Screenshot

image

Confirmed

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

Required changes:

@tankerkiller125 tankerkiller125 changed the title [WIP] Fix Mods being able to edit Admins, Split permissions Fix Mods being able to edit Admins, Split permissions Apr 3, 2020
Copy link
Member

luceos left a comment

For some reason I cannot add comments to the Serializer.

$canEdit = $gate->allows('edit.credentials') || $gate->allows('edit.groups') || $gate->allows('edit.username') ? true : false;

The last ?: is superfluous. I also highly doubt it does what you expect. The condition applies only to the last gate allow method, not all three.

Why exactly are there no new policy methods, are these magically applied? Would you have kept the user as an argument to the allows method and created the methods on the UserPolicy you could have moved the EditUserHandler change to the Policy to enforce it globally, I think that's the better solution here..

@tankerkiller125
Copy link
Member Author

tankerkiller125 commented Apr 3, 2020

Thanks for the feedback @luceos I'll work on implementing your suggestions, I think the problem stems from my lack of experience working with Flarums permission systems and not know exactly how policies got applied. With your insight I think I can do this way better.

Copy link
Member

askvortsov1 left a comment

In theory this could work without a policy: if no policy governs a permission, there'll be a check as to whether the user belongs to a group that has that permission.

However, I agree that a policy could be useful here, as that'd be a better place to put the if user->isAdmin() => assertAdmin($actor) logic, so that we keep the permission layer separate from the editing logic.

src/User/Command/EditUserHandler.php Outdated Show resolved Hide resolved
src/User/Command/EditUserHandler.php Outdated Show resolved Hide resolved
src/Api/Serializer/UserSerializer.php Outdated Show resolved Hide resolved
src/Api/Serializer/UserSerializer.php Show resolved Hide resolved
src/User/Command/EditUserHandler.php Outdated Show resolved Hide resolved
@clarkwinkelmann
Copy link
Member

clarkwinkelmann commented Apr 9, 2020

I agree with the existing comments. Please ping me again and I'll do an actual review when more code is pushed.

@franzliedke franzliedke marked this pull request as draft Apr 10, 2020
tankerkiller125 added a commit that referenced this pull request Apr 12, 2020
tankerkiller125 added a commit that referenced this pull request Apr 12, 2020
@tankerkiller125 tankerkiller125 force-pushed the mk/1965-fix-permissions branch from 7c943eb to 674417c Apr 12, 2020
@tankerkiller125 tankerkiller125 marked this pull request as ready for review Apr 12, 2020
@tankerkiller125 tankerkiller125 requested a review from luceos Apr 12, 2020
@tankerkiller125
Copy link
Member Author

tankerkiller125 commented Apr 15, 2020

@clarkwinkelmann I've refactored it almost completely

@franzliedke
Copy link
Member

franzliedke commented Apr 17, 2020

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

tankerkiller125 and others added 7 commits Apr 2, 2020
@tankerkiller125 tankerkiller125 force-pushed the mk/1965-fix-permissions branch from e01422e to a543389 Apr 17, 2020
@tankerkiller125
Copy link
Member Author

tankerkiller125 commented Apr 22, 2020

Rebase was completed

@franzliedke
Copy link
Member

franzliedke commented Apr 24, 2020

I'd like to discuss the naming convention for the discussions permissions in our next meeting, that's why I'm not merging this yet.

@askvortsov1
Copy link
Member

askvortsov1 commented Jun 4, 2020

@franzliedke when you get back from vacation, what's the verdict on permission naming?

@tankerkiller125 tankerkiller125 requested a review from askvortsov1 Jun 17, 2020
Copy link
Member

askvortsov1 left a comment

I think Clark brings up a good point here: #2113 (comment), a warning message would be nice. Other than that, the only thing left is Franz's decision on the ability naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.