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

feat(permission): add the no one permission #3777

Draft
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Draft

Conversation

luceos
Copy link
Member

@luceos luceos commented Mar 26, 2023

One thing that has been very frustrating while developing extensions is that you cannot easily create a feature that you would like to allow disabling through the permission grid. Some solutions I've seen is allowing admins to choose the tags something should be active in or by simply creating toggles in the settings page; but neither of these solutions allow the fine grained control the permission allows.

This Proof of Concept PR is a question, whether I'm the only one running into this limitation for Flarum.

It does:

  • introduce a fake permission, similar to Guest
  • removes the whole permission entry from the db when selected
  • restores the permission visually on the frontend

It still has to:

  • confirm admins also cannot use the permission if selected User\Access\Gate::90
  • not break v1.x functionality or be moved to v2.0
  • appeal to core and community
  • identify whether introducing this permission will cause some permissions that would have been previously adopted from the top level "view" permission to become "no one" thereafter

image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

One thing that has been very frustrating while developing extensions is
that you cannot easily create a feature that you would like to allow
disabling through the permission grid.

This Proof of Concept PR is a question, whether I'm the only one running
into this limitation for Flarum.

It does:

- introduce a fake permission, similar to Guest
- removes the whole permission entry from the db when selected
- restores the permission visually on the frontend

It has to:

- confirm admins also cannot use the permission if selected
- not break v1.x functionality or be moved to v2.0
- appeal to core and community
@luceos luceos self-assigned this Mar 26, 2023
@SychO9
Copy link
Member

SychO9 commented Mar 26, 2023

The use case is definitely valid, but the introduction of a no one permission imho complicates the already complicated permission system.

The permission UI grid already has a concept of settings (see edit post permission which is a setting) so imo, the best way of solving this, is to keep such a toggle on the backend as a setting value to turn on/off said feature. And on the UI grid, add support for the permission dropdown to be linked to a specified setting to turn off X permission, the dropdown item could then be labeled no one or such.

@luceos
Copy link
Member Author

luceos commented Mar 26, 2023

so imo, the best way of solving this, is to keep such a toggle on the backend as a setting value to turn on/off said feature.

I think that the setting option is very complicated for something so easily supported and I still believe this would make sense for 2.0+. For now I'll use your suggestion 👍 thanks.

@SychO9
Copy link
Member

SychO9 commented Mar 26, 2023

It shouldn't be if we adapt the permission dropdown to turn off/on the setting with a no one item. I'm only thinking out loud though, not giving any concrete solutions.

I think since we also have the issue of setting permissions on the UI (like the post edit permission) not being applicable per tag, cause it's not an actual permission,

we should brainstorm changes to the permissions system/UI taking into consideration all issues, so that we can find a consistent approach to everything.

@garygreen
Copy link
Contributor

garygreen commented Mar 28, 2023

This is something I ran into yesterday! E.g. for the Tags plugin I want to globally disable the permission "Bypass tag requirements" permission because I don't want even admins to be able to bypass those tag requirements.

I can see the rationale behind why admins are currently forced to be allowed to do everything, like a superuser - although admins have the ability to edit all permissions, it's useful to reduce visibility of features that aren't necessary or shouldn't be easily accessed by admins.

Notice how it's greyed out/disabled, so it's impossible to remove all permissions

image

This PR implementation does seem complicated. Perhaps we can simplify the process by providing a more intuitive solution. For example, we could make it possible to untoggle all permissions in the dropdown. So when "Admins" is clicked on the above, all the groups will appear deselected and show an empty icon indicating that it's not assigned to anyone.

@garygreen
Copy link
Contributor

garygreen commented Mar 28, 2023

Just thinking about this a bit more - technically the current "admin" group is a "superuser" who is able to do ANYTHING and that cannot be changed. It seems permissions are hardcoded to always allow actions from admins, so it could be tricky changing that paradigm now.

So to work around this, you could add e.g. a "Managers"/Executives/Directors etc group and switch your current admins to use that group. This provides the ability to disable features/permissions you don't need.

That's possible to do right now, so maybe this isn't needed. Though it would be cleaner to be able to simply unassign all groups from a permission.

@luceos
Copy link
Member Author

luceos commented Mar 31, 2023

So when "Admins" is clicked on the above, all the groups will appear deselected and show an empty icon indicating that it's not assigned to anyone.

This is actually happening behind the scenes. Unlike the current behavior, when you choose No One it will remove all permissions in the background. The frontend now simply shows No One in case no groups are assigned to the permission. My implementation might not be the cleanest :)

I also have been thinking more and my opinion remains that we need this. The fact that a super admin exists that can use features they might not even want to use but are forced to see and probably even use, doesn't make Flarum really friendly. I think we should opt to add this solution or come up with a better version of something similar. But force allowing Admins to do anything through the Gate should be a thing of the past soon.

@SychO9 SychO9 changed the base branch from main to 2.x May 27, 2023 17:40
@SychO9 SychO9 added this to the 2.1 milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants