edit a user without "view hidden group bages" permission can break membership of the user

[Ornanovitch]

discuss ref

Bug Report

Current Behavior
When a user has the edit users permission but doesn't have the view hidden group badges one, he can involuntary kick off users from group with a hidden badge simply by add them or remove them to/from an other group.

Steps to Reproduce

1. make 3 groups and 2 users
   group A has the Edit user groups permission but doesn't have the View hidden group badges one
   group B has the Hide on forum attribute
   group C is a control group without special attribute nor permission
2. add @user1 to group A (can edit user groups) and @user2 to group B (hidden badge)
3. with @user1, edit @user2 and add him to group C.
4. @user2 is kicked off the group B

Expected Behavior
@user2 should remains into group B.

Environment
nightly

[Ornanovitch]

@askvortsov1 @clarkwinkelmann sorry to bump you about this but IMHO it's a major problem to fix (before stable ?), and it's still happening on nightly (post beta.16 = flarum/framework#2620), so I edited the steps to reproduce in OP.

at least this issue should be triaged 🏷️

[clarkwinkelmann]

I'm not sure what we should do with this.

My suggestion would be to make "edit users" dependent on having "view hidden group badges". I know we have a feature to visually have a permission be dependent on another in the admin panel (for example, you can't have reply permission if you don't have view permission), but I'm not sure if we manually apply it in the backend as well or if it's just a helper for the Permissions page.

Ideally we should refactor the group assignment to something that doesn't depend on sending the existing groups again, but that's a big refactor and would likely require new API endpoints instead of using the relationships.groups json:api format on the edit user endpoint.

[Ornanovitch]

My suggestion would be to make "edit users" dependent on having "view hidden group badges"

I would not expect more, it sounds like something natural : "Hidden group badges" is not "Hidden group" and user who can manage groups should at least be able to see them 👀

[askvortsov1]

Proposed solution makes sense, permission dependencies are currently located in https://github.com/flarum/core/blob/b3de5157fec8498ba938dffe6bc7d105bf3d5890/js/src/admin/AdminApplication.js#L57-L71. PRs are welcome for this.

[Ornanovitch]

I tried something but this is my first PR and I usually work with gitlab. Although I don't have a flarum dev environment, so I hope the required change is that simple. If it's more complicated, I'll close and I'll apologize for this useless attempt^^

[luceos]

As pointed out here: flarum/framework#2880 (comment) I have some doubts about the current implementation. Based on what has been said here:

• Depending permissions on the frontend don't make a lot of sense, something like that should be handled by the backend so that it won't cause issues for api interaction?
• Requiring edit users depending on hidden groups visibility removes the flexibility those separate permissions offer, a solution:

If the actor lacks group visibility identify which groups of the editing user are hidden and remember those in a seperate array, then update the visible groups and merge them before saving with the enabled hidden ones. This requires on depending permissions on the frontend or whatever, the backend takes care of permissions for both types of actors (elevated and non elevated).