Fix Mods being able to edit Admins, Split permissions

[tankerkiller125]

**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

Confirmed

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

Required changes:

• Related core extension PRs: (Remove if irrelevant)
  • Remove old permission label, add new ones lang-english#164

[luceos]

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]

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.

[askvortsov1]

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.

[clarkwinkelmann]

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

[tankerkiller125]

@clarkwinkelmann I've refactored it almost completely

[franzliedke]

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]

Rebase was completed

[franzliedke]

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]

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

[askvortsov1]

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.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

I think we should change it to camel case, and then switch back to dot separated later if we decide that to be the new convention. This also needs to be updated for mithril 2.

One major concern as I thought back to this: we currently have extensions using $actor->can('edit', $user), including our own nicknames. If we remove edit without a BC layer, we could run into issues. I think we should re-think this split, keeping user.edit as the main user edit permission, and requiring individual permissions (like user.editGroups) for higher level things.

[tankerkiller125]

Closing in favor of #2620