Skip to content

Add ability to disable Bukkit permissions handling#12389

Closed
luisch444 wants to merge 1 commit into
PaperMC:mainfrom
luisch444:luisch444/Add-ability-to-disable-bukkit-permissions-managing
Closed

Add ability to disable Bukkit permissions handling#12389
luisch444 wants to merge 1 commit into
PaperMC:mainfrom
luisch444:luisch444/Add-ability-to-disable-bukkit-permissions-managing

Conversation

@luisch444
Copy link
Copy Markdown

@luisch444 luisch444 commented Apr 7, 2025

PR Details

Information

This PR intends to add the ability for server to disable Bukkit permissions checking for commands. With this will be able to fix some problems with permission levels checking in Bukkit.

Technical comments

The PermissibleBase#hasPermission(@NotNull String inName) is a Bukkit method that checks the permissions of a property, from the comment of @zml2008 permissions model predates the numbered op levels we can assume it should be thought as a refactor in the future. Because the Bukkit implementation does not allow levels of permissions.

Note: The modifications are made in the less affecting way possible, we know this might induce into some configurations changes to unintended behavior. With this I want to ask: I should enhance the hole system or only change the minimum to fix the issues

Why do not approach the full solution?

IMHO; this should be thought, when the project has fulfill all the major goals and all the important discussions has being fixed, by the contributors for having a nice permission handling without affecting vanilla implementation while containing all the features needed, Bukkit's and Paper's,

Why still in draft?

This PR is in draft because, me luisch444, have not completely tested this feature and neither added any test. Nevertheless, we would like to hear back by the contributors of project if any adjustment is needed.

As well, this modifications (with the config value active) might introduces some breaks into some plugins only supported in Bukkit.

Fixed issues (Please update/comment on need)

@electronicboy
Copy link
Copy Markdown
Member

I mean, just disabling the perm system for a select set of commands, IMHO, is just a non starter; The actual fix here would probably be to make op levels mapped over to perm nodes in order to let perm plugins deal with this stuff more fluently/properly, I just don't know if there is much care to put up the engineering effort to implement and maintain such a mechanism which basically hasn't been used for well over a decade due to much better solutions existing

@luisch444
Copy link
Copy Markdown
Author

I agree, @electronicboy, I think Fix for is not the right terminology; I'll edit the main PR comment. Nevertheless I think this should be integrated because various reasons

  • You can give a workaround to servers admins with the corresponding warnings, in the issue mentioned and possible others in the future or that we have not located yet.
  • In this case more configuration does not make too much harm, or do you think the configuration property is misleading?
  • We follow the WoW 1 of small PRs, into making the system better little by little as needed.

But yes, I also agree that the permissions handling should be factorized and I understand there is no motivation, in the sense of putting the engineering effort to it, by the Paper's team. But if anyone wants, in the future, made the refactor by his own; having more context from other issues and PRs where maintainers can reflect there opinion, might be useful and more convenient.

FYI: This change is motivated by the comment: // lazily check bukkit perms to the benefit of custom permission setups.

1 Way of Working

@luisch444
Copy link
Copy Markdown
Author

luisch444 commented Apr 7, 2025

Also having a config value to disable a mechanism that is no longer consider in use, quoting @electronicboy [...] mechanism which basically hasn't been used for well over a decade due to much better solutions existing [...], seems appropriated.

@lynxplay
Copy link
Copy Markdown
Contributor

While I am a fan of small PRs, introducing a config option to bypass a system that (presumably) is properly fixed later just for the config option to die then is not really something I am interested in.
This issue is not problematic enough to require a "hotfix" config option like this when simply using permissions plugins over operator levels has been common practice in the community for a near decade.

As cat stated, the "proper" solution for this would be a lot more work that we may or may not even be interested in maintaining (tho we do maintain CommandPermissions already so, it might be fine?) In general, fine grained permissions are a objectively better approach than vanillas operator system so I am not sure if a proper solution is worth maintaining.

I appreciate your work on this but I do not believe this approach is something I am willing to merge.
If you want to look into a proper mapping of permission levels to command permissions, feel free, tho that would be a larger change.
Thanks again for the work and the nicely explained PR and discussion!

@lynxplay lynxplay closed this Apr 11, 2025
@github-project-automation github-project-automation Bot moved this from Awaiting review to Closed in Paper PR Queue Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

3 participants