Introduce PermissionFunction#getPermissionMap

[WouterGritter]

This PR introduces PermissionFunction#getPermissionMap, and delegates like [Connected]Player#getPermissionMap.

I would like to see this be a feature of Velocity, because I'm currently trying to optimize a bit of code that uses the Velocity permission API:

private int getTimeoutInSeconds(Player player) {
  for (int i = 86400; i > 0; i--) {
    if (player.hasPermission("velocity.queue.timeout." + i)) {
      return i;
    }
  }
  return -1;
}

Of course having up to 86.400 permission checks is not good, even if it's just a one-off, and especially because we don't know how the permission provider implements this (each call might reach out to a redis/SQL db).

Unfortunately because lots of servers are already relying on being able to specify a permission like this, it's not trivial for me to fix this issue by reducing the amount of permission checks. Ideally I'd like to see Velocity and permission providers like LuckPerms expose the permission map, as I'm sure I'm not the first one with this kind of permission check as well.

After this PR, a better implementation of such a method in a plugin would be:

private int getTimeoutInSeconds(final Player player) {
  return getPermissionMap().entrySet().stream()
          .filter(e -> e.getValue() == true)
          .map(Map.Entry::getKey)
          .filter(s -> s.startsWith("velocity.queue.timeout."))
          .map(this::parseOptionalInt)
          .flatMapToInt(OptionalInt::stream)
          .max()
          .orElse(-1);
}

Resulting in at most a couple hundred checks, and no possibility of spamming a DB or something.

It would be trivial for a permission provider like LuckPerms to implement this, as an example here is their PermissionFunction implementation: PlayerPermissionProvider.
They reference their internal CachedPermissionData, which already exposes @NonNull @Unmodifiable Map<String, Boolean> getPermissionMap().

Of course by leaving the API @Nullable on the Velocity side, with a default return of null, we don't break any existing code. Even though Velocity's PermissionFunction is a @FunctionalInterface, it's possible to have additional default methods and allow other APIs implement them.

Having this be a part of the Velocity permission API would avoid having a dependency on the underlying permission providers like LuckPerms, which I think is a good thing.

I'm also wondering if it might be better to introduce Map<String, TriState> getPermissionMap() instead of Map<String, Boolean> getPermissionMap() instead. Should we allow permission providers to force a permission to be "unset", and expose that via the permissionMap getter?

[electronicboy]

This is something permission plugins would need to weight in on, but really, this is just abuse of permissions, basically every single competent permission plugin has its own mechanism for storing a proper K->V for this stuff

[WouterGritter]

this is just abuse of permissions

I assume you're referring to the hasPermission call in a giant for loop, if so, I agree so this needs to be fixed.

It would be good to see what permission plugin devs have to say, but we can also look at the bukkit api which already contains a similar feature with PermissibleBase#getEffectivePermissions.

BungeeCord has CommandSender#getPermissions.

I'm not too familiar with fabric, and if this is a feature fabric explicitly supports or not, but LuckPerms also exposes similar functionality in their fabric implementation here. So I think there's president for permission plugins (at least LuckPerms) wanting/trying to expose this information.

[electronicboy]

using permissions themselves to define what is essentially a KV is the abuse

I know that several other implementations of permissions expose such a feature, and that is why plugins like LP generally have some means of trying to populate a list of those values to appease the fact that they're often hijacking an existing implementation; I want to see what plugin devs have to say moreso on the ABI rather than the existence of such a thing, I would ideally rather have some 3rd party vault replacement which can offer abstractions for what really should be the proper solution here, however

[WouterGritter]

using permissions themselves to define what is essentially a KV is the abuse

I see. I've read through the LuckPerms docs, and it looks like for this usecase their meta system would be a perfect fit.

int timeout = user.getCachedData()
    .getMetaData(qo)
    .getMetaValue("queue.timeout", Integer::parseInt)
    .orElse(0);

Would be set with a command like
/lp user <name> meta set queue.timeout 120

I guess it would be out of the question to provide this functionality through a Velocity-native API. Something like what Vault accomplished would probably be the ideal solution here.

Though, that's for my specific use-case. I think it's still worth while to see if we can merge a feature similar to what I'm proposing here, exposing the permission map.

[WouterGritter]

@lucko I wonder what your thoughts on this are

[WouterGritter]

@4drian3d do you mind elaborating on your decision? Does anyone else still have a say in this? Another maintainer talked about getting the view of permission plugin maintainers, I've pinged the head maintainer of the largest permission plugin and we haven't even heard back yet.

[4drian3d]

@4drian3d do you mind elaborating on your decision? Does anyone else still have a say in this? Another maintainer talked about getting the view of permission plugin maintainers, I've pinged the head maintainer of the largest permission plugin and we haven't even heard back yet.

It seems to me that what you want to do is add an API to Velocity so that you can use a LuckPerms feature without relying directly on LuckPerms. But you have to keep in mind that not everyone uses LuckPerms (even though it is one of the best and most popular permission plugins), and that there may be other permission plugins that do not necessarily use a simple key-value system for their permissions, but may declare other types of conditions to represent them.
Also, you are giving examples of platforms that are not known for having such good API infrastructure, placing many limitations on what developers can create.
For your specific use, it is best to create an abstraction in your project that can optimize the use of permissions using LuckPerms, if it is detected that it is installed.

[lucko]

My thoughts (for what they're worth):

• Using permissions to represent a KV pair is not great, most permissions plugins have "metadata" or "options" concepts that are designed for this sort of data (in LP for example /lp user lucko meta set queue.timeout 100)
• Meta/options probably do not belong in the Velocity PermissionFunction API, but not my call to make (obviously)
• Exposing a Map<String, Boolean> goes against the original aims of Velocity's permissions API (Permissions! #41), which was to make as few assumptions as possible about how permission checks might be performed/evaluated by the implementation. The thinking was that this would allow permission plugins be creative and do useful things like wildcards, regex permissions, shorthand/glob syntax, etc without breaking any API contracts. That was my view ~8 years ago and it hasn't changed. 😛 I took the same approach with fabric-permissions-api.

[WouterGritter]

Thank you lucko, loud and clear.

Obviously introducing meta getters in the Velocity permission API is not a good idea. But I'm wondering how the Velocity team looks at exposing a separate, generic, meta api for players with a generic MetaHolder?

One of Velocity's strengths I think is that its stateless, at least between reboots. But obviously there's the need to keep some kind of state, hence the permission API. It might not be that far of a fetch to also include a meta system here? Again, leaving it up to plugins like LP to implement this.

Of course this would also require a setter, something which the permission api lacks. If we have the contract be "no relation between setting and getting a meta value is guaranteed" the default behavior can just be a NOP setter and a getter which returns "unset".

[electronicboy]

I mean, the permission API inside of Velocity is pretty minimal, we didn't really want to impose any API decisions onto plugin devs and figured either everybody would just back something like LP or the community would re-vault; there was just a minimal acceptance of velocity itself needing to do permission lookups for the few builtins we provided, as well as plugins are going to need to do permission lookups too.

All of the methods you linked to from other systems having such methods aren't raw permission APIs by the platform, they're APIs for the platforms implementation of a permission system, and so LP has to implement that stuff as the platforms implementation dictated it; if any API expansion to velocities permission stuff came, it would need to be spearheaded by permission plugin devs for a reason (hence why I said that this really needs to be checked and back by permission plugin devs)

Velocity not dictating this kinda stuff, and being stateless in general, is part of our philosophy that this sorta thing should be solved by plugins, us adding trivial API like a MetaHolder type interface would dictate what information plugins need to be able to have to look something up (basically nothing), as well as basically being highly restricted in the type of information they return, which is generally just not API I really want to design

[WouterGritter]

as well as basically being highly restricted in the type of information they return, which is generally just not API I really want to design

That would not necessarily have to be the case of course. Look at Paper's PersistentDataContainer, they solved it quite well by providing some primitive data types, whilst allowing plugin devs to map any type they'd like onto a String/int/long (array) which means basically everything can fit in there. And that's while Paper also had to solve the issue of storing this data. Velocity doesn't have this; it'd just need to pass custom types along to the underlying plugin so they can deal with it. But that's another question. Do we want to force plugin devs to have to worry about any generic type, or should we just limit it to the Java primitives.

I still think it's worth it to explore something like a MetaHolder API interface for Velocity. However as that's entirely different than what this PR is proposing I'm closing the PR for now.