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

Permissions my plugin.yml used to use as "default: false" - anti-permissions? #2787

Closed
slipcor opened this issue Dec 23, 2020 · 12 comments
Closed
Labels
type: issue The issue identifies a bug/problem with the software.

Comments

@slipcor
Copy link

slipcor commented Dec 23, 2020

Description

Until a few versions ago when I received a bug report from a LuckPerms user, this is how I have been defining a certain "anti-permission" to basically allow a server owner to declare a select group "newbies" and restrict usage of my plugin:

https://github.com/slipcor/PVPStats/blob/4e3b954794cdc229124e9d05cde2dbe7777e18d2/src/main/resources/plugin.yml#L39

This has been working well for years - maybe only with other permissions plugins - or without even - and it seems that recently either LuckPerms changed its way of verifying permissions, or I had an influx of LuckPerms users and LuckPerms always gives OPs the '*' permission as people call it, which gives all permissions.

I assumed that this is the case and it only applies to permissions mentioned in the plugin.yml and subsequently removed that "newbie" permission and replaced this with a positive-default "nonewbie" permission.

I wanted to keep it backwards compatible for the - I have to assume - vast majority of users who already have set up their permissions system to work with what I had before.

Now I learned while testing a new feature I planned, that the default LuckPerm setup actually still returns TRUE for the nonexisting permission "newbie" which I removed to only be used for server owners who set it up to work with "newbie" anti-permission rather than "nonewbie" permission. As I wanted to have the "newbie" be backwards compatible and be of higher priority, this now still breaks the newbie check of new LuckPerms users, until they set "newbie" to false ... which is not documented

Reproduction steps

  1. download PVP Stats
  2. check OPs for the permission "pvpstats.newbie" - which is not documented, but "all is true" ?
  3. one player kill another player - either player is OP
  4. the kill will not count

Expected behaviour

As both setups did not work, I guess I expected two different results and neither was the case:

A) (plugin.yml version from back in the day) - LuckPerms should load the permissions in the "plugins.yml" and default to "false" all the permissions that are set that way. All permissions that are supposed to be for OPs, I set to "op".
B) (current plugin.yml) - LuckPerms should only return "true" for any permission nodes that the plugin actually claims to register in the yml

Environment details

  • Server type/version: Paper running version 1.12.2 build October (players have been reporting bugs like this with other server versions)
  • LuckPerms version: v5.2.35

Any other relevant details

This is the current plugin.yml

https://github.com/slipcor/PVPStats/blob/master/src/main/resources/plugin.yml#L39

So there is two ways that this could have been resolved - if I would be allowed to keep my backwards compatibility for the confusing anti-permission setup, as my expectations in A / B above:

A) Is there a way for me to ask LuckPerms to allow my yml to have anti-permissions, is there a denominator or value I could add to my permission definition so that LuckPerms would know to not add it to the mix? If not, could this become a feature request then? - If LuckPerms is not going through the plugin.yml at all anyway and just returning "yes" for all perms then that might be touch :D

B) Fixing this, expecting LuckPerms to only ever add perms that we as plugin developers specifically define would break all other plugins that currently might rely on this OP-cheat or permissions that "start with" a known permission... so that is probably not a viable option. Could plugins get an opt-in for this?


At the end of the day, I am open for tips in how to resolve this. Any change on my end that allows for backwards compatibiliy even if it inconveniences the people who have adapted in the last weeks to my attempt to fix this, it should be worth the try. I would very much appreciate a solution that would not require me to manually hook into LuckPerms, but if it is not possible otherwise, I'd rather do this than lose servers because they fall into my anti-permission trap :)

@slipcor slipcor added the type: issue The issue identifies a bug/problem with the software. label Dec 23, 2020
@emilyy-dev
Copy link
Member

emilyy-dev commented Dec 23, 2020

I can't seem to reproduce the issue, I defined in my test plugin's plugin.yml:

permissions:
  some.permission:
    default: false

image


The only thing that comes to my mind is the apply-bukkit-default-permissions setting. Make sure it is set to true in your LP config: https://github.com/lucko/LuckPerms/blob/5c443338927104da858beca9d1306628671bdcac/bukkit/src/main/resources/config.yml#L490-L495


Edit: I tested with enable-autoop set to true but that doesn't really change the results of the test; did just to quickly show that not even OPs get false defaulted permissions as long as that setting mentioned above is set to true :d


Edit 2: wildcards work as intended as long as apply-wildcards is set to true as well https://github.com/lucko/LuckPerms/blob/5c443338927104da858beca9d1306628671bdcac/bukkit/src/main/resources/config.yml#L459-L463

image

This last test was without being OP.

@slipcor
Copy link
Author

slipcor commented Dec 23, 2020

Thank you for your time!

I tried addressing this by reintroducing the permission setting, defaulting to false in my plugin.yml.

This works out of the box, as you pointed out, but then what happens is that admins give themselves the "*" permission to test out all the features, assuming that they have "all permissions", in turn giving themselves a permission that is restrictive in my logic.

Is there a way that I can set up a permission in a way that it will not be granted if a server admin gives themselves '*'? I fully understand that this is expected behaviour for all permissions that are considered allowing permissions, and not "anti-permissions" as I have it set it up to be.

For now I added a small workaround, checking for an undefined/unclaimed permission. If a player has this, we assume that they are operator / have all permissions and probably did no want to be excluded from the functionality of my plugin.

So is my workaround then the most accurate fix then?

@benwoo1110
Copy link

You just set the specific permission to false. * perm node is not recommended as well. See: https://v2.nucleuspowered.org/docs/nowildcard.html

@slipcor
Copy link
Author

slipcor commented Dec 23, 2020

I know that it is not recommended. No need to convince me :)

But what can I do when server owners do use them when setting everything up and then expect my plugin to work the way they think it should?

If there is no way to prevent this while defining or checking permissions on my end, I will just stick to my current setup that checks for undefined permissions :)

An upcoming version will display warnings about this and maybe explain this behavior to affected server owners.

@benwoo1110
Copy link

You just ask them to set the permission node to false, bcu that how permission works, its just true or false, no special whitelist/blacklist to over complicate things.

@emilyy-dev
Copy link
Member

... and then expect my plugin to work the way they think it should?

And that's the issue and why we are against wildcards, admins and owners not knowing any better blindly sticking * here and there.

Bukkit defaults are used as an ultimate fallback if the permission ends up undefined by any other means (specific permission, Regex, shorthand, wildcard, sponge wildcard, Bukkit child permissions); the wildcard (like any of the other kinds of permissions mentioned) is doing its job, this is the exact same reason in the discord server when people ask for support he don't really recommend the usage of the root wildcard and I personally suggest to use the auto-op feature instead, but of course I can't account for everyone, can I?

What you can do from the plugin side is check for both the permission you want and *, although that is a bit hacky IMO.


Thing is that everything in here is working as intended. I see where you're coming from (as I myself had to deal with this, ended up checking for * too) and I would love to see Luck's opinion on the matter :)

lucko added a commit that referenced this issue Dec 23, 2020
I think this is a good compromise. It won't apply to registered permissions that are defaulted to 'op' (Bukkit) or 'undefined' (Sponge), only to those that are specifically set to false.

The change is configurable, enabled by default for new installs of LP.

Will hopefully go some way to resolving:
- #2787
- https://v2.nucleuspowered.org/docs/nowildcard.html
- NucleusPowered/Nucleus#1093 (and related)

cc: @dualspiral @slipcor
@lucko
Copy link
Member

lucko commented Dec 23, 2020

Hopefully the above change will solve this issue for you. :)

You'll need to insert the new option into your config, as it defaults to being disabled if the section is missing.

# If the plugin should apply negated Bukkit default permissions before it considers wildcard
# assignments.
#
# - Plugin authors can define permissions which explicitly should not be given automatically to OPs.
#   This is usually used for so called "anti-permissions" - permissions which, when granted, apply
#   something negative.
# - If this option is set to true, LuckPerms will consider any negated declarations made by
#   plugins before it considers wildcards. (similar to the way the OP system works)
# - If this option is set to false, LuckPerms will consider any wildcard assignments first.
apply-default-negated-permissions-before-wildcards: true

@slipcor
Copy link
Author

slipcor commented Dec 23, 2020

Yes, this is very helpful and validating because it is in agreement with how I understand how permissions were defined to work.

I hope that this does not break functionality with plugins that might rely on the asterisk flipping their false to true - however I think we are in agreement that the absolute wildcard should not be used, and specific (parent) wildcards can and should be defined and then used to flip groups of permissions, and also, perms should be set and used as verbose as reasonably possible which would allow for expected convenience behaviour.

Thank you very much for this swift response! I am absolutely fine with this not changing everyone's config, I will include a warning in my plugin, so people can update their config if they really need to give out the asterisk.

Have a healthy rest of the year!

@lucko lucko closed this as completed Dec 23, 2020
@mcmonkey4eva
Copy link

Hi, this is in fact breaking to other plugins yes.
After the fifth or so user came into the Citizens channel to ask why they couldn't use Citizens anymore, I looked into it and found this breaking change was at fault.
While I do think it fair to change Citizens permissions to default: op and don't find the commit inherently unreasonably, I must very strongly protest the issue post here...


"Anti-permissions" do NOT EVER EVER EVER SUPER NO NOT EVER belong in the permissions system. Both that, and the nucleus link, are fundamentally opposed to the entire concept of a permissions system.

Permissions are PERMISSIONS. They are not SETTINGS. They are not USER CHOICES. They are not something a user chose to enable. They are things the user IS ALLOWED to enable. A player should never be vanished, restricted, or anything else by a PERMISSION.

The concept of things that happen automatically to a player by choice is called user configuration. (In Bukkit this is usually janked into a <uuid>.yml file somewhere, though in both modern Spigot and Sponge there are better persistent data options available.)
A permission does not vanish you, it LETS YOU use /vanish or /vanishonlogin (or whatever command, than then applies a user configuration change) if you so choose.

The opposite of a permission is... well, not having that permission. In LuckPerms that'd be setting the permission to false. In other permissions plugins that's usually just prefixing the permission with a -. Or, of course, in any permission plugin... simply not giving the relevant permission in the first place.
Restricting a user's actions is reasonable in the case of LACKING a permission, not HAVING that permission.

Granting a * permission is fundamentally saying "you are allowed to do whatever you want"... any plugin that does not obey that fundamentally does not understand what the word "permission" means.

That is what commands and user configuration data are for. Please, please... NEVER mix permissions and user settings. These are two fundamentally separate concepts.

@slipcor
Copy link
Author

slipcor commented Jan 19, 2021

I only just noticed that the default for this "fix for anti permissions" is true by default. maybe that needs to be changed? everyone setting this setting to "false" will experience the old behaviour and I think it is fair to say that the anti-permission logic is deviating from the norm :)

lucko added a commit that referenced this issue Jan 19, 2021
@lucko
Copy link
Member

lucko commented Jan 19, 2021

Permissions are PERMISSIONS

NEVER mix permissions and user settings

I agree with you on this, and think that it should just be easy to just grant * to give users access to everything permission related.

However, the reality of the situation is that plugin authors are using the system for "anti-permissions", and that's something that LuckPerms should therefore aim to support in my opinion.

The idea behind this change was exactly that - to allow plugin authors to mark in their plugin.yml permissions that explicitly should not be given in bulk to "super users" / OPs, either through /op or the * permission. (this is the way Bukkit intended for the system to work, before permission plugins added the auto wildcard resolution (root * etc) we now have)

However, it seems like on Bukkit at least, plugin authors / server admins are set in their ways and expect even the root wildcard to override plugin defined false defaults. I do understand this, and it's the reason why LP worked in the way it did until the change was made in response to this issue.

We have also noticed a fair amount of users running into unexpected issues with this in our support channels.

It's a shame because I personally think the setting should default to true, but, I appreciate we're well past the point of making improvements to the way Bukkit permissions work.

So, I've swapped it back to false by default for new installs. The setting remains true by default on Sponge though.

@BrainStone
Copy link

I think the core issue of that whole ordeal is that what permissions default to true or false are not visible to the user at all!

Mean they are tapping around in the dark with no chance of figuring out why that specific set of permissions is false even though they believe they should have those permissions. If there was a way to visualize the default permissions this whole issue should become a non-issue as then people can actually find the root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: issue The issue identifies a bug/problem with the software.
Projects
None yet
Development

No branches or pull requests

6 participants