Make permission plugin detection more generic in Superperms handler #1356
Conversation
| if (specialCasePlugins.contains(plugin.getName())) { | ||
| final boolean match = plugin.getName().toLowerCase().contains("permissions") || | ||
| plugin.getName().toLowerCase().endsWith("perms") || | ||
| plugin.getName().equals("GroupManager") || |
There was a problem hiding this comment.
Might be easier to maintain if you still keep the set of special cases outside of the loop and use the .contains() call but keep the .endsWith(perms) and .contains(permissions) calls to simplify it.
There was a problem hiding this comment.
I didn't think it was worth it for just two exceptions. It's faster doing it that way too, as opposed to constructing a list each time the method is called. Not a huge deal imho, but I'll change it if that would be preferred. 🙂
|
Hey, Thanks for your contribution. After looking into this further we've come to the conclusion that defaulting Although this PR fixes the problem for LuckPerms, it doesn't fix the problem generally. So we'd rather fix this all together for everyone. What this will do is make Bukkit permission plugins a priority over EssentialsX's built-in permissions. Although I appreciate the efforts of the initial Essentials developers, I have to say this feature is one of the features I've seen rarely used, and I say that as a general fact from my experience over the years rather than hard numbers. This will NOT fix the issue immediately, this will be fixed as new configuration files are generated, or if we encourage our users to modify their Essentials config to set Thank you once more. :) <3 |
See #1356 for more information. This change was made with the confident presumption that the use of built-in EssentialsX permissions is very very low compared to those who use feature-complete permission plugins.
It probably made sense in the early days, but yeah, not really anymore. I'd never noticed it myself because my testing environment has Vault in it.
Yeah, that's not great either. Server admins expect permissions to work without having to enable them for each plugin. I agree, hacking around it & trying to detect certain substrings is a bad fix (this PR), but a hardcoded list of plugins is poor design too. 😕
If all I cared about was fixing it for my own plugin, I would've just added it to to the list... That wasn't my intention at all. I appreciate it's a tricky issue, changing the default is probably the best fix. |
|
I've already made a commit d5cbfef that changes the default. From now on all new configurations will set If you could, tell them something along the lines of "EssentialsX has been updated to fix this problem by default. Please update <here>" with a link to https://ci.drtshock.net/job/EssentialsX/ |
Previously there was just a hard coded list of permission plugins. There's a number of new permission plugins which have emerged recently, which are not in this list. Instead of adding them, I've changed the way they're detected.
Instead of checking for absolute matches, a plugin will be deemed a "permission provider" if its name:
permissionspermsGroupManagerorPrivileges(special cases)This is necessary, as if Vault is not installed and the user is running an "unsupported" permissions plugin (one that's not in the list), Essentials will fallback to using config based permissions.
https://github.com/drtshock/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/perm/PermissionsHandler.java#L112-L117
(the config option "useSuperperms" is false by default)
This means that any permissions set by the user are ignored.
However, if a plugin is detected in the list above, Superperms will be selected, and config permissions will be ignored. (the correct behaviour)
https://github.com/drtshock/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/perm/PermissionsHandler.java#L100
Related to LuckPerms/LuckPerms#348