LuckPerms

LuckPerms

41.4k Downloads

Setting apply-global-groups or apply-global-world-groups to false should also apply matching group parents

Phoenix616 opened this issue ยท 6 comments

commented

Currently if you set apply-global-groups or apply-global-world-groups to false then it only checks the direct parent groups that the player is in and not the parents of these groups. (This behaviour only happens when the settings are set to false, if they are true then all the parents' parents are resolved correctly depending on the player's context)

Imo. these settings should result in the plugin including all the parents of the groups that the player is in (and their parents) if they match the context of that server/world.

commented

Yeah, I see what you mean now.

commented

The inheritance tree resolution stops as soon as a group fails the check.

If the group isn't being 'applied', then it's parents shouldn't be considered either.

commented

This makes these settings close to useless 'though. In most cases you only add your players to a single group (which is their server rank, e.g. normal player or supporter) and add all server specific permissions and groups as parents there. This makes it possible to add your players only to this single rank group e.g. when promoting instead of having to add them to all the different per-server/world groups that you have in your network.

The system works like I described it before as long as you have these settings on true, but as soon as you want a server to only apply per-server groups you end up having to configure these per server groups independent from your other rank-groups which makes setting this all up far more complex than it should be.

Currently a parent group that is only applicable on a server that has apply-global-groups set to false results in zero effects, neither on the server it applies to nor on different servers that have the setting on true so I don't see an issue that could arise when changing this behaviour?

commented

I don't see how resolving the parent groups before the actual (permission) nodes would break the ordering of these nodes. I suspect that you don't really differentiate between different node types at first but walk the parent inheritance tree as it comes up and after it was filter via their contexts? I understand that using such a concept makes more sense in an object oriented environment and see how it complicates the matter at hand.

My approach here would be to decouple the group inheritance resolution from the rest of the nodes. This should be possible to do while still keeping the ordering of the other nodes intact. You would still resolve the groups in order (closest to the user first) and then only add (non group) nodes of groups that match the local context to the returned list of localized ones when the node itself matches the context.

You could even include the group nodes in this list too, they are just not resolved at that point like they are now. One downside of this could be that it might not support negated group nodes without adding extra support for these, but I don't think that the plugin itself uses these anywhere and would question the existence of any permission setup that makes use of something like that.

However, the speed of this method & the retention of existing behaviour regarding inheritance order is more important than allowing applyGlobal to function how you suggest.

In my opinion changing this behaviour so that it is actually usable is more important, mainly because I only see that half of this will actually change: Obviously it would be a bit slower overall as you would get all the groups first and then resolve the other nodes instead of doing it all in one step. One could even go as far and use this different resolution only when the global groups are disabled as there wouldn't really be a change of the behaviour from now when global groups are included.

Thoughts on the usefulness of not including global groups in the current state:

Currently the apply-groups options are nearly unusable in my opinion. The only effect they have is that you can setup permissions completely independent from the rest of your network/worlds as you cannot use any of the other groups/ranks that a player has to define the parent groups and with that permissions on the server/in the world.

You'd basically have to add all your players to multiple groups, one global one and one for each server/world that you want to enable these options on. And if you add a new server/world you have to add all your thousands of players to a new group. Same issue with promotions: You'd have to create tracks and promote for each single server/world. At this point you might even be better of managing the permissions on the servers completely independent from the rest of the network, at least then you don't clutter your main permission setup with dozen of groups.

A note on the matter of speed:

Did you test whether or not your streams in the resolve method are actually on par/faster than the same code done with normal loops? There are a couple of cases where streams are a lot slower than normal loops.

commented

Thoughts on the usefulness of not including global groups in the current state
Yes, I see what you mean, but most people actually expect the current behaviour. (it's certainly the most logical. If a group will not "apply" according to the lookup rules, then most would expect that none of the groups parents would apply either)

Anyway, point taken, it would be nice to have the option at least.

Did you test whether or not your streams in the resolve method are actually on par/faster than the same code done with normal loops? There are a couple of cases where streams are a lot slower than normal loops.

They're on-par, or perhaps slightly slower. I've done a bit of messing around, and actually tried a number of different approaches to this around version 3.0. Using streams in this case isn't a huge deal, as the set gets reduced in size pretty quickly. The calls to Node#getValue is obviously fast, and Node#getGroupName is cached when the node instance is created. After this, the size of the stream is small enough for it to not really make a difference.

I don't see how resolving the parent groups before the actual (permission) nodes would break the ordering of these nodes.

Groups are only inherited from once. A list of included groups is passed up the inheritance resolution so the same groups are not accumulated twice. This prevents circular inheritance issues (StackOverflows in this case)

My approach here would be to decouple the group inheritance resolution from the rest of the nodes. This should be possible to do while still keeping the ordering of the other nodes intact. You would still resolve the groups in order (closest to the user first) and then only add (non group) nodes of groups that match the local context to the returned list of localized ones when the node itself matches the context.

Yeah, that will work, but it will make the process significantly slower. The accumulation has to occur twice. First to discover all of the parent nodes, and then to resolve the actual permissions.

commented

Just started to look into adding this, and it's looking pretty messy to do atm.

Currently when resolving inheritance:

  • We create a List accumulator
  • Add all of the users direct permissions to it, filtered by context
  • Flatten the list according to value. (remove duplicates with the same permission but different value)
  • Then, from these nodes, extract the ones which represent a parent group, and filter by the context options. .filter(n -> !(!contexts.isApplyGlobalGroups() && !n.isServerSpecific()) && !(!contexts.isApplyGlobalWorldGroups() && !n.isWorldSpecific()))
  • Map to a group, and recursively resolve again.

All of that code is here.
https://github.com/lucko/LuckPerms/blob/master/common/src/main/java/me/lucko/luckperms/common/core/model/PermissionHolder.java#L466

This approach works really well. Filtering by contexts early is really fast, and means instead of constantly iterating, we can just do a map lookup.

Removing values according to their value is probably the slowest part, but it's necessary if we want to allow negated group nodes to override. It's probably the fastest way to achieve that without trying to wrap group nodes in another class and manipulate their hashcode, then use #distinct.

Then the parent resolution part is fairly quick too. Specifically regarding applyGlobalGroups, this lookup is really fast, as it's just comparing booleans. If I had to do some complex calculations in the background with these methods (as I was before with shorthand world/server names), then this whole method becomes really sluggish. I'm trying to make things as fast as possible, some people have inheritance trees 100 groups tall. If you try to do anything complex, or recursively resolve in these situations, then it starts to take a few hundred ms for these methods to complete. (not good)

The issue I have here is that I don't see a way to change this apply behaviour without making this method really complex & slow.

One possible solution would be to resolve the inheritance tree in advance, to work out which groups should apply. No filtering would be done at that point, so you'd end up with a raw view of the inheritance tree. You could then filter those results by the context and apply options, and you'd end up with exactly what you're asking for.

However, although this works for this issue, it will also break another trait of the inheritance, which is that nodes closer to the user take priority. That meaning, when the list gets flattened later on, the order is really important.
https://github.com/lucko/LuckPerms/blob/master/common/src/main/java/me/lucko/luckperms/common/utils/NodeTools.java#L92

With this approach, that order may not be correct.

If you have any other ideas here about how this could be implemented, let me know. However, the speed of this method & the retention of existing behaviour regarding inheritance order is more important than allowing applyGlobal to function how you suggest.