inconsistency between playerHas methods
Opened this issue ยท 20 comments
Hi! I only tested this with GroupManager.
There are two cases:
- when using specific permission (ie. "factionsplus.xyz" )
- when using wildcard permission (ie. "factionsplus.*" )
(using means: typed/added those perms in the groups.yml file of groupmanager for that specific world for the default group)
In case 1, there is no inconsistency, but in case 2 the following calls have different returns:
org.bukkit.entity.Player player=...
net.milkbowl.vault.permission.Permission perm=...
///...
perm.playerHas( player, "factionsplus.xyz" ); //returns "false" (it's true in case 1 though)
perm.playerHas( (World)null, player.getName(), "factionsplus.xyz" ); // returns "true" (just as in case 1)
perm.playerHas( player.getWorld(), player.getName(), "factionsplus.xyz" ) ); // returns "true" (same for case 1)
I've noticed that the first call is equivalent with
player.hasPermission("factionsplus.xyz")
but the other calls are doing something like this:
@Override
public boolean playerHas(String worldName, String playerName, String permission) {
AnjoPermissionsHandler handler;
if (worldName == null) {
handler = groupManager.getWorldsHolder().getWorldPermissionsByPlayerName(playerName);
}
else {
handler = groupManager.getWorldsHolder().getWorldPermissions(worldName);
}
if (handler == null) {
return false;
}
return handler.permission(playerName, permission);
}
perhaps the first call should instead be equivalent to the second call ? that is, internally the first call should do a call similar to the second call above, or equivalent to the third call even.
I might be missing something, so I didn't bother making a pull request, also because I wasn't sure which variant you'd use to solve this, if it even needs solving :) Maybe it's a groupmanager bug for not properly hooking into the player.hasPermission or whatever this may be called
Thanks.
EDIT: I also reported this as a groupmanger ticket: https://www.assembla.com/spaces/essentials/support/tickets/2442
just in case
I don't actually know what superperms is, but I'm assuming it's the thing inside bukkit which handles the player.hasPermission() method ? and that GM somehow interfaces with this feeding it all the perms starting with factionsplus. when "factionsplus.*" is encountered in the config, and since it takes those from FactionsPlus' plugin.yml which in my case there are none defined in the .yml and thus no permissions are set. Makes sense.
For my case I see 3 fixes:
one: add that permission "factionsplus.xyz" in my plugin.yml which will then cause GM to work when using player.hasPermission() right ? because of this "GM will read and set superperms set in a plugin.yml"
two: have Vault make this call:
perm.playerHas( player, "factionsplus.xyz" );
act like one of:
perm.playerHas( (World)null, player.getName(), "factionsplus.xyz" );
perm.playerHas( player.getWorld(), player.getName(), "factionsplus.xyz" ) );
rather than:
player.hasPermission("factionsplus.xyz")
three: me avoiding using perm.playerHas( player, "factionsplus.xyz" ); and instead use the other 2 calls.
Honestly, I believe that fix two would be best, but that's just my opinion.
Superperms doesn't support wildcards, and when dealing with wildcards there is no way know way to know the 'factionsplus.xyz' permission exists.
GM simply gives superperms a list of permissions for each player, if it doesn't know the permission exists, it cant set it.
GM will read and set superperms set in a plugin.yml for example
Normally i'd suggest not using vault at all, and just use the built in bukkit permission check, because the vault interface just isn't needed, and using the gm full lookups are much slower than the bukkit method.
It's fairly easy to code something inside of your plugin to handle wildcards if you refuse to implement superperms properly in your plugin.
Essentials relies on wildcard permissions working in all permissions plugins, even in plugins which specifically dont support wildcards (such as permissionsbukkit). Normally plugins would register a permissions group in its plugin.yml to 'enable' wildcards, but because essentials uses dynamic permissions (think essentials.warp.) it is not possible to know in advance all the permissions that will exist.
Instead we forward all superperms lookups via this method:
https://github.com/essentials/Essentials/blob/2.9/Essentials/src/com/earth2me/essentials/perm/SuperpermsHandler.java#L33
The result being, if 'essentials.warp.fish' doesn't exist, try checking for 'essentials.warp.*' etc...
If you want to see how bukkit recommends plugins deal with permissions and permissions groups, vanish is a good example:
https://github.com/mbax/VanishNoPacket/blob/master/src/plugin.yml
that's definitely something we should do with FactionsPlus, currently there's none defined there so nobody knows which permissions we do use. This should've been done all along if you ask me. The fact that it wasn't done made me wanna guard against someone forgetting to add a permission there in plugin.yml and thus a wildcard of factionsplus.* will not have that permission set.
I'll definitely consider using the built in bukkit permissions check, coupled with this.
Thank you khobbits.
I might've misunderstood something, but I'm having this in "my" plugin
permissions:
factionsplus.xyz:
default: false
description: something
and using bukkit's builtin I'm still getting the "false", unless I set that default to "true"
while groups.yml of GM has "factionsplus.*" only. But if I do set the default to "true" the GM part can be commented out because the permission exists by default. (tested)
having " default: false" line or not having it at all are equivalent (from my tests)
and having "default: true" makes that permission exist for every player regardless of it's existance in GM's groups.yml even if it's negated like "- -factionsplus.*" and/or "- -fationsplus.xyz"
So, one thing's for sure, I'll be keeping the default on false or not even specifying it in plugin.yml
Back to the main question: having "factionsplus.xyz" in plugin.yml and setting "factionsplus.*" in GM's groups.yml shouldn't that actually have player.hasPermission("factionsplus.xyz") return true ? that was my understanding so far about what GM does. I'm gonna give it a rest until I recover from the confusion :)
That should be right, gm should pick that up and apply the wildcard perm.
I dont mean to be condescending, but are you sure your plugin.yml is valid yaml (indents etc), and that you tested the right jar, after restarting the server?
For best support across all permissions plugins, its best to define the plugin.* permission in the file as well (see the last perm in the vanishnopacket example for a howto....
If you need more assistance, your best bet would be to try IRC, and contacting the GM developer 'ElgarL'. The live chat link on the ess forum post would take you to the right network if your unfamiliar with IRC.
That's exactly what I missing: defining the factions.* in .yml
In other words:
permissions:
factionsplus.xyz:
default: false
description: something
factionsplus.*:
default: false
children:
factionsplus.xyz: true
This works with negating also. So basically, for every new permission, we have to add it in 2 places in the .yml or it will just silently fail when wildcards are used. I'll be honest, I don't like this because of the high risk of human error without warnings.
This will correctly handle case one from #223 (comment)
About the being sure about the yaml, I get that, there are so many ways it could get the old version of the .yml, for example if editing the .yml outside of eclipse and forgetting to refresh before exporting .jar (didn't test this, it may not apply). But I use some eclipse plugin for editing yaml files and it makes each tab do 2 spaces and also avoids the need to refresh .yml every time because the editing is done inside eclipse, and I use a *.jardesc file to automate the .jar exporting and reduce mistakes with selecting which files to be included in the .jar So in that respect, i was pretty confident that I had that covered. The restarting the server part however almost never happened. I always just use 'reload' and only after like 30 reloads running out of memory will force me to kill and start the server again heh. Yeah reload is evil but it's safer than plugman :) I should use stop/start for 100% certainty but I'm too lazy when I believe a reload would do same.
I'll do the IRC thing if anything more, thank you! You've actually helped me a lot! ++khobbits.karma
You shouldn't NEED to define the 'plugin.*' permission for gm to work, just some of the others like bpermissions and permissionsbukkit, groupmanager should automatically calculate wildcard permissions.
There have been a few bugs in the past with it not finding permissions correctly, but i think they are all fixed in the last few stable builds of gm.
I can't say for certain since I haven't worked on the GM code, nor relied on this GM feature (we use the code shown above which does manual wildcard checking, or access the gm api directly).
you are right when you say that GM's methods work without needing plugin.* to be defined in the plugin.yml those methods that Vault uses in the last 2 calls (see first comment) but when Vault uses the 1st call (which maps to player.hasPermission() ) then it uses superperms aka permissionsbukkit(as far as I can tell) and that will return false if that plugin.* isn't defined in plugin.yml
@Sleaker
Vault uses 2 different implementations for the same overridden method playerHas(), one implementation calls GM's methods, and the other calls bukkit Player.hasPermission() aka superperms/permissionsbukkit which you would think they should all return the same value if they are being called with equivalent forms (see the 3 calls in the first comment).
The way I see it, GM cannot fix the player.hasPermission() to return the same value as if you were calling GM's methods. The user himself can fix this by adding stuff in plugin.yml as we talked above.
Or maybe Vault can fix the first call to map to one of the other 2 calls to ensure same value is returned. But as I understand it using GM's methods is slower than by just using player.hasPermission() as khobbits said above. I see this as one somewhat valid reason to keep Vault as it is.
Bottom line: shit :)) why do I bother :D
The issue is that the first call which defers to bukkit is the correct one. Not the one that checks GM internally.
Look, GM SHOULD ALWAYS return proper values as if the bukkit player.hasPermission() was called in all cases of permission checking. If it doesn't, then it doesn't actually support bukkit's permission system even if it says it does. This isn't an issue in Vault's checking or that Bukkit is being asked what the player permissions are, it's an issue in how GM handles permission checks.
@basicsensei GM SHOULD populate superperms with any perm defined in the plugin.yml, when using a wildcard.
This is one thing that GM does do, that the others dont.
@khobbits when you say "when using a wildcard" do you mean inside GM's groups.yml ? instead of also in plugin.yml ?
@basicsensei yes, GM assumes by default you have a 'plugin.*' permission in your plugin.yml, but it only automatically adds any other permissions listed in the plugin.yml
Or at least it should.
not seeing how this relates to Vault really.
If you pass a player object, Vault assumes the player is online (because you have the object), and defers the check to bukkit. This will result in the correct response 100% of the time. So the response from perms.playerHas(player, permission) will always return the result that is registered in bukkit.
The only time you shouldn't use this check is if the player is offline, and you need to see if a player would have permission for something. Then you can use the second checks, due to them being passed off to the permission handler the server is using there's no guarantee their logic matches bukkit.
You're right I missed 2 edits. On my tests the player was always online. What should I use for checking if the online player has permission in the world X ?
unfortunately bukkit doesn't do that. If the player is in the world that you want to check for you can use the normal check as it should include world-specific permissions. Otherwise you have to trust that the permission system actually returns correct values for what you're defining (which may not be the case).
I was doing a test on whether the player.hasPermission (well the Vault equivalent) was actually returning true/false for different worlds depending on which one the player was in(and it does, as you also said), and this is what eventually led me to opening the issue.
Ok, all is well.
Well, I mean, from my tests, the player.hasPermission("factionsplus.xyz") works correctly(aka returns true) only when I've also added the "factionsplus." inside plugin.yml with children "factionsplus.xyz: true" (while ofc the groups.yml for that world and default group has the "- factionsplus." line in "permissions:" ). This would at least suggest that, either defining "factionsplus.*" in plugin.yml is required for wildcards in groups.yml to work for that plugin, or if it's not supposed to be required(as you said) then it's a bug in GM. This still however means that Vault is using two different implementations which will return different values for playerHas... does it matter which one is the right one?
I don't know what else to say on the matter guys, regardless of my giving up on the issue, I still do appreciate your input so far.
Peace out.
EDIT: I just tested with PEX only, and it works fine all 3 calls are consistent when using
groups:
default:
default: true
permissions:
- factionsplus.*
however when using
groups:
default:
default: true
permissions:
- modifyworld.*
- factionsplus.*
- factions.*
- multiverse.*
worlds:
world_nether:
permissions:
- -factionsplus.*
in nether, the first and third calls (see first comment) return false, while the second returns true; in other worlds all are true as expected.
or something like:
groups:
default:
default: true
permissions:
- modifyworld.*
- -factionsplus.xyz
- factions.*
- multiverse.*
worlds:
world_nether:
permissions:
- factionsplus.*
in nether, will return true for 1st&3rd and false for 2nd call.
ok, let's see bpermissions:
maybe I don't know how to configure it but I get false for first call and true for 2nd and 3rd
I won't bother anymore, I'll finish off what I started and then rq:)
@basicsensei - I think you missed one of my post edits.
Basically, never use playerHas(world, name, perm) if the player is online (you should have no reason to). This method is only for offline permission checking and is not necessarily going to accurately reflect the player's current permissions unless the permission implementation is 100% compatible with BukkitPerms.
Furthermore, playerHas(player, perm), is just a pass-through method for player.hasPermission(perm), so if all you're checking is if a specific player has a permission, generally don't bother using Vault checks.