Implement Fabric Permission API

[Patbox]

Provides relatively simple, but way more flexible permission api for mods to use.

Using it is as simple as calling #checkPermission method on entity / CommandSourceStack, or any other PermissionContextOwner implementation. Proper contexts are implemented as a separate classes implementing PermissionContext, which allow to always receive uuid, type (player, entity, etc) and vanilla permission level, as well as optionally additional contexts provided via PermissionContext#get method (similar to PacketContext in that regard).

This api was designed to allow checks for players (online and offline), entities and other objects.

The checks themselves default to using vanilla TriState, but api allows providing any codec for it to read from (which automatically handles support for ints, strings and any other complex data). That is also one of main things that differentiate it from vanilla permission checks, which only allow checking for bools without externally defined defaults.

The implementer (think LuckPerms, PlayerRoles or some protection mod) only need to handle a single PermissionCheckCallback event.

[Juuxel]

I think there should be a code example in the API docs showing how to use this API, especially from the POV of mods defining and checking permissions. It's currently a bit unclear to me. Some of the PR desc and test mod could probably be repurposed for a smaller example.

[DrexHD]

I don't think the entity should necessarily be used for determining the type, because it can be changed through commands like /execute as @a run .... See luckperms, which used to do the same thing Luckperms#3673!

[Patbox]

Oh that is fair hmm. Through guess just looking at the command output might not be idea either. Will check if maybe patching it directly first before actually doing the check is better option

[Patbox]

Changed it to only determine the type / uuid on initial CommandSourceStack creation, so any later modification of it (calling with* methods) will inherit initial type and uuid

[lucko]

Some feedback (in no particular order) as requested:

Mojang have recently added some permissions-y APIs to Player and ServerPlayer which permission manager mods can mixin to (LuckPerms does, unsure about others). This went under the radar a bit I think, so maybe you aren't aware. For example:

private static final Permission PERM = new Permission.Atom(
    Identifier.fromNamespaceAndPath("example_mod", "command.test")
);

ServerPlayer player;
if (!player.permissions().hasPermission(PERM)) {
  // deny
}

In my view this now captures ~90% of what mods will need. Before it was added, fabric-permissions-api was quite key as an interface between mods and permissions providers, I am not sure it necessarily is anymore. At the very least, given what I understand the philosophy of Fabric API to be, it seems sensible for any new APIs added on top to look and feel similar to the Permission APIs the game now has.

Some other comments - some of these are suggestions, others I think would be blockers for LP implementing this API fully:

• Option/meta-like values, not sure it makes sense to call them "Permissions". I think I see why you did it this way but IMO it is confusing.
• Offline player checks - these should return a CompletableFuture in my opinion. Often (in LP anyway), these result in some sort of database lookup or file I/O. I would prefer to see this reflected in the API to prevent authors from accidentally doing it on the game thread.
• Context - pleased to see it as a feature, but I am not sure in it's current state how a permission mod would take what is there in PermissionContext and turn it into something queryable/comparable against a serialised data set. I am also unclear on the difference between the built-in contexts and extra/custom ones. Sponge (and LP) use simple String->String pairs for this, Forge has a higher level API but ultimately you can get down to String->String using built-in serialisation methods. Caching is another important thing, I have worries about Position being included by default for example as that feels far too granular. Maybe I have just misunderstood?

possibly others, but those are the key ones I can see so far :)

[Patbox]

Yeah I am aware of vanilla's permission checks, which is why I delayed working on this api originally. It kinda felt bit too unfinished and doesn't handle default values as nicely (which my mods depending on defaults being dynamic for example). In future if Mojang implements more of it, it might make sense to get it connected closer with vanilla permission sets.

Main reason why it's all under permission is since everything is checked the same way (as within concept of this api there is no real distinction between boolean/TriState permissions and string/number ones). Boolean/TriState just got few extra utility methods due to it being commonly checked one.

That one is slightly tricky. Let's say you have a mod that adds machines that modify world or do something that needs to check for permission. It's likely they will want to check with their owner being their context, which will happen whatever the player is online or not. So the checks being straightforward is kinda important here.
Either way having "async" check methods for context does make sense. Will need to think if there is nicer way to solve it through.

With context values being serializable, I can look into that as I do see the usecase. For simpler ones I can look into implementing it, but I feel not everything will make sense to support that (for example the CommandSourceStack or Entity don't really make sense to be represented as a string). Generally my idea of context was to pretty much work similarly to how loot tables can include optional data that then might be used for more precise logic. Think about area protection mod wanting to change player permissions locally. In that case it needs the position to determine where thing happens. Similarly to loot tables custom ones would require direct compat. Technically speaking PermissionContext isn't static in itself, as for default entity one it dynamically queries then from said entity.

[modmuss50]

I worry this is needlessly complex having two methods like this. Why not just have the method that returns a CompletableFuture and let the implimentation use CompletableFuture.completedFuture as it needs?

[Patbox]

In most cases generally you want to complete it synchroniously (player is online and you just want to check some permission), so needing to use async method is way less common. Mostly included them as I wanted permission providers to have option for better handling of them (and as it was requested from lucko). Also it allows to have them run with higher priority in cases when they are actually needed just now

[modmuss50]

I have no objection to having them async and it makes sense why you may need this, I am suggesting to go full in on CompletableFuture's. Maybe this is the way to go with the default that calls a none async version.

Maybe the middle ground is for the impl to always use the CompletableFuture path, to save the complex duplication. The onPermissionCheck function could be @ApiStatus.OverrideOnly

It doesnt make much sense to me for a CompletableFuture to be nullable either.

[Patbox]

CompletableFuture being nullable was mostly intended as shortcut to not require it going over res.thenCompose chain. Also downside of async one is that it will always go through all callbacks (but only use first result), while sync/regular will do an early return. I think keeping them as seperate still makes sense through, as async implementation will likely only be needed by few providers/users. It also allows it to work at "lower priority" / execute slower compared to sync where it's needed "right now".

[modmuss50]

I worry this premature optimisation is just making everything a lot harder to follow. I tottaly get having a synchronous user facing API on either end, but Im not keen on making everything even harder to follow maybe async maybe not like behaviour. CompletableFuture's can already be quite hard to follow and making them nulltable with a nullable value makes it really hard to reason about.

Both of these functions could be @ApiStatus.OverrideOnly, making it quite easy to change this in the future if it does turn out to be a performance issue.

[Patbox]

After looking into this I still think it makes sense to keep sync and async calls separate. Primarily since it keeps the expectations of the behaviour of called methods (sync calls should only happen on calling thread, while async ones can be executed on a different one) and allows to have them prioritized if needed (example offline player check that need to lookup into the database). Even more so with sync checks being primary ones to be used (since most checks happen when player is online or happening to non-players), them just being .join()ed async calls could become noticeable (in cases where a lot of entities/players/blocks/whatever request a permission).

[modmuss50]

Question: What happens if two mods use the same Identifier but with a codec of a different type?

[Patbox]

Similarly to test implementation it should try parse it and return that (or null if failed), but technically that is something on callback implementation part to decide

[modmuss50]

These could just call the async version and block the calling thread until it returns.

[modmuss50]

Question: Where/why are these serialised?

[Patbox]

As per lucko request to make it easier to integrate in generic way

[modmuss50]

Needs tests/an example and proper docs to say why. I dont know what lucko is.

[Patbox]

Lucko is author of luckperms, requested that in comment above.

[modmuss50]

Opps, sorry I though it was a mod :D That makes a lot of sense now.

[modmuss50]

Question: What is "name" here? Is it a display name that you would show to users, is it the name of the owner?

[Patbox]

It is name of the owner, mainly intended for distinction in cases of system/non uuid based checks, through it will match owners plain name for entity ones

[modmuss50]

Include this in the documentation.

[modmuss50]

I'm happy with this, it would be great if others who plan to use this could also give it an approval (or leave comments if you arent happy).

[modmuss50]

As discussed please add ApiStatus.Experimental for now. Dont forget to also mark it as experimental in the FMJ.

[lucko]

Maybe I misunderstood contexts here in my first review, if they aren't serialisable, I'm not sure what permission providers are meant t do with them / how to handle them?

[Patbox]

Not everything makes sense to serialize really (entity or world instance for example), these kinda require direct compatibility to work. Ideally commonly wanted/needed ones would be pr-ed into fabric api

[lucko]

I think some of the confusion (from my side) comes from LuckPerms also having a "contexts" concept (https://luckperms.net/wiki/Context), which for reference came from Sponge API initially. These use key/value string pairs, to allow, for example an admin to grant a player a permission only in certain situations (e.g only in creative gamemode, or in the nether world).

I think what you're doing with "contexts" here is a different use-case, where the context in this case is defining all of the input parameters for the permission check.

IMO it makes sense for them to remain separate. It would be cool if the Fabric approach could cover both, but if that's not a goal currently, maybe it makes more sense to remove the serialisation for now and keep it simple?

Apologies for the confusion - my bad

[Patbox]

Oh yeah this does make more sense. Wasn't exactly sure about the current serialization, so I agree that dropping it makes sense (for now)

[lucko]

I appreciate you adding an async option in, but I'm not sure the current implementation actually helps. Why would any mod use these methods if the sync version is there? There currently isn't a way for the provider to flag "hey - data's not loaded / in-memory, you should call the async method instead".

[Patbox]

Async ones are primary for offline player checks that can happen at some (short) point in time without requiring to block the current thread. I imagine they will be not as commonly used as main/sync check for non-players/online players

[lucko]

Makes sense - but again, how do you know which method to use? There's nothing in the API surface (that I can see) that guides developers towards the right option? (in contrast with https://github.com/lucko/fabric-permissions-api/blob/master/src/main/java/me/lucko/fabric/api/permissions/v0/Permissions.java#L265 which defines the return type as a future if we know the input is not an online player/entity)

[Patbox]

Idea was that dev would choose what they think makes most sense for them to use. But Javadoc for PermissionContext#offlinePlayer encourages usage of async methods for them

[lucko]

Would it work to have something similar to this:

• PermissionContextOwner -> as it is currently (although maybe renamed), only sync permission check methods
• Entities, etc -> implement PermissionContextOwner
• PermissionContext, provides an async method to resolve to PermissionContextOwner, which allows sync checks once loaded/"resolved"

the event then encapsulates that load/"resolve" operation, which could be sync or async.

some maybe better names for PermissionContextOwner:

• PermissionFunction
• PermissionObject
• PermissionHolder
• PermissionSubject
• PermissionEntity

the obvious benefit of this is it makes it efficient to, for example, load the permission data of an offline player or entity, then run lots of checks against that loaded state.

the tricky bit is I guess how to make that work nicely with multiple providers, but that's not impossible, perhaps with a CompositePermissionContextOwner (or renamed equivalent)

[Patbox]

Honestly maybe just naming it as PermissionCheckable or something like that would work. However I'm not exactly sure about making permission context owner a strict thing. Ideally PermissionContext would be only thing needed. Changing offlinePlayer method to return PermissionContext asynchroniously, while having an event to load/cache the data for faster permission checks (with reasonable behaviour, to be determined by provider) is something I would agree with. Either way for something like luckperms, the actual source of ideantity for checks should be type + uuid anyway, since mods can exclude any other context (which should be more for things like gamemode / state checks and alike).

[lucko]

Is this the entrypoint/API for mods to check permissions for offline players? Or does that not exist?

[Patbox]

The idea is that all permission checks go with generic context, so technically offline check can happen both sync and async (but it is now suggested to always handle that async if possible)

[Patbox]

Oh also this event is more of impl thing, as providers can implement async checks as optionally overridable methods (it's handled via PermissionCheckCallback)

[lucko]

I don't think you've answered the question (or I am misunderstanding the response).

If I have an offline players name + UUID, but not a current/valid entity instance for them, how do I run a permission check? Is the expectation that developers call the event directly?

[Patbox]

Events shouldn't be called directly no. PermissionContext also has all the same methods used for checks as entity/command source, so you would call them directly on it. Package info / main javadoc shows an example of that (with modified context)

[lucko]

Ok that makes sense - thanks for clarifying

[lucko]

I've left some more comments in-line - this feels a bit rushed to me :/

[modmuss50]

I've left some more comments in-line - this feels a bit rushed to me :/

Thanks for taking a look, we can let it have some more time to bake if needed. I agree it has been rushed as there was a desire to get it in before the stable 26.1.

[Patbox]

Reworked the events and how offline player is handled. Also got rid of the async methods, as they should be no longer needed.

@lucko how do you feel about current one?

[lucko]

I think this is good enough as-is, but an idea: would it make sense to make the context param mutable, to allow implementers to append extra custom context to the object when "preparing"? Then all of the associated state can remain on the PermissionContext object

[Patbox]

Making it mutable would require making sure it can be changed safely on different threads, but guess that could be handled by changing the return type to a CompletableFuture holding a consumer taking the mutable context, so it can all get modified in one go on the same thread

[Patbox]

Implemented it with a callback

[lucko]

hasPermission is the standard across all other Minecraft permission APIs, any particular reason to use checkPermission instead?

[Patbox]

Mainly as other methods also use checkPermission, while not returning boolean like value (I feel that using hasX style name doesn't really fit in these cases)

[lucko]

Do you think it would make sense to change this such that (optionally), mods can listen to this event without changing the result? e.g. listening on a high priority to log the outcome of permission checks, but not to provide the result?

[Patbox]

Returning null does work like that

[lucko]

Reworked the events and how offline player is handled. Also got rid of the async methods, as they should be no longer needed.

@lucko how do you feel about current one?

Thank you - yeah I like that a lot better :)

Left a few other minor comments / suggestions, interested to know your thoughts on those.