API 8: Tweaks to permissions API

[zml2008]

SpongeAPI | Sponge

These are some QOL tweaks, plus a few methods to make the API more defined.

I have a few more things I'd like to add -- the idea of a fallback permission for subjects, and a context refactor based on work I've been testing out in PEX.

The changes so far fall into a few categories:

Context handling

In order to handle the possibility of a command being executed in a state remote from that of its current environment, I've changed the primary context calculation source from the Subject itself to a provided Cause.

When querying permissions on a subject itself, this Cause is built from the subject's current state, but when using a proxy such as a CommandCause, the Cause used is the game state in play.

This allows directly passing the Cause from an event to perform permissions checks, making accurate context calculations more straightforward.

Permissions descriptions / role template changes

Permissions descriptions are currently quite limited in what they can do. These changes make permissions descriptions more powerful by more thoroughly linking some relatively spread out portions of the API. New features include:

• Permissions descriptions can supply a default value
• Role templates are now explicitly namespaced by plugin -- so a staff role for Nucleus would be stored internally as nucleus:staff. This allows more fine-grained control over templates, or, for example, only copying the permissions provided by a role from one plugin.
• There are methods on PermissionService that allow querying role template information without having to know the details of how the implementation stores that data.

Bulk data editing methods

Having these operations allows plugins to more efficiently work with permissions subjects. Having copy/move operations explicitly as part of the API will hopefully encourage permissions plugins to provide that functionality.

• Allow setting all permissions, options, or parents on a SubjectData
• Allow copying/moving all data from another subject data entry
• Added a new TransferMethod to customize the behaviour when overwriting existing subject data -- either replacing everything or merging with existing values

QOL tweaks

• Make most generic parameters wildcards to allow implementations to interoperate more easily with native objects

[ST-DDT]

This contradicts the classes javadoc.
https://github.com/zml2008/SpongeAPI/blob/f0634898c257d89733f6a6d090f04c23efcac56a/src/main/java/org/spongepowered/api/service/permission/PermissionDescription.java#L42

Also this might cause issues with the generic descriptions.
Foo.<world>.doX

For that reason we assigned only role strings with an actual value
https://docs.spongepowered.org/stable/en/plugin/permissions.html#permissiondescription

[dualspiral]

This contradicts the classes javadoc.

It doesn't really though - these aren't intended as defaults that are automatically applied, but as templates, i.e. "here is the recommendation". They are still informational by the strictest sense of the word - as far as I understand it, they are not intended to be used as concrete groups, instead enabling a more global system akin to nucleus setupperms.

Also this might cause issues with the generic descriptions. Foo.<world>.doX

Nothing says they have to be registered.

[ST-DDT]

This contradicts the classes javadoc.

It doesn't really though - these aren't intended as defaults that are automatically applied, but as templates, i.e. "here is the recommendation". They are still informational by the strictest sense of the word - as far as I understand it, they are not intended to be used as concrete groups, instead enabling a more global system akin to nucleus setupperms.

If this only informational the an assignRole("ALL", true) would do the same and would also allow defining it for other roles. IIRC we even have some predefined role names somewhere.

[zml2008]

Thanks for pointing that out -- I'll change the class javadoc to match. Setting the default value in a description should update the default subject data.

Currently permissions descriptions are pretty useless -- and a good part of the reason why is that they don't really solve any problems that a list in a plugin's docs couldn't solve more easily.

@ST-DDT 's example permission is also something that's unlikely to make sense in the real world. I don't expect there to be many permissions that would incorporate a world parameter, since that's provided by contexts. I also expect most template parameters will be at the end of a permission string, not somewhere in the middle, so that wildcards are easier to apply -- meaning they can most likely automatically be replaced.

I'm going to add some query methods that take a Subject and check permissions based on the definition -- with support for replacing template parameters that are Strings or ContextKeys.

My end goal with permissions descriptions is to be able to replace the boilerplate in a class like https://gitlab.com/zml/featherchat/blob/master/src/main/java/ca/stellardrift/featherchat/Permissions.java#L49 or https://github.com/PEXPlugins/PermissionsEx/blob/master/permissionsex-fabric/src/main/kotlin/ca/stellardrift/permissionsex/fabric/hooks.kt#L91 -- with a list of PermissionDescription definitions.

---

As a side note regarding /nucleus setupperms -- that command should not exist, nucleus should provide its roles by registering PermissionDescriptions, and a permissions plugin should be implementing the ability to copy data from a role template to a specific group.

[Zidane]

@lucko @zml2008 Is this PR going to go anywhere or set this aside? Need something done on this soon else it won't be in API 8.0.0

[zml2008]

I'd almost forgotten I had this -- i'll take some time this week to get everything I can buttoned down. The improvements already in are still useful -- and i have a couple more design changes I'd like to explore.

[zml2008]

Alright, I've pared this down a bit and taken out the context changes -- everything that's left should be good to go. Impl PR coming soon.

[lucko]

Looks good to me overall, couple of questions/suggestions though..

[lucko]

If we need to use join in the impl, maybe this method should return a CompletableFuture instead?

[zml2008]

I originally wrote this a while ago, but I think my reasoning was that these subjects would most likely be cached because they only exist from having transient data registered -- and transient data is expected to only be stored in memory.

[lucko]

I wonder whether it would be worth providing a default implementation of the PermissionDescription query methods - the functionality seems a bit complex - which is fine, but it's not overly clear how they should behave or be implemented - regex & string replacement I guess? Maybe provide a Pattern that can match the "template parameter" format?

[ST-DDT]

Queries on descriptions with template parameter should fail to execute (or always return false).
What would be your expected result? All in all, I think this method shouldn't exist:

The description is meant to provide human readable descriptions and meta data for a permission. Descriptions are primarily informational [...]

So don't put any logic into them.

[zml2008]

I looked at doing a default implementation -- but that would involve exposing a "stripped id" to the api that isn't currently exposed.

Take a look at the default implementation in SpongeCommon. I've put in some Patterns for a valid permission id and trailing template, but those could be pulled up to the api if you're interested.

---

ST, the reason for these permission description changes is to answer the question "Why should a plugin developer register their permissions?". With the existing system, there are very few benefits for plugins from these registrations, so nobody's going to use them. These helper methods and the default value method below aim to give plugin developers the ability to work entirely in permissions descriptions -- including basic parameterized permissions checks and setting metadata.

[lucko]

Sooo, is the permissions implementation required to take this into account when resolving permission checks?

It was my understanding that the default subject / subjectcollection was the way to do this - it seems unnecessary to duplicate that functionality.

[dualspiral]

It was my impression that permission descriptions could become more like a "template" that a plugin can supply and a server admin can apply - basically so that nucleus setupperms can go the way of the dodo. I'm not sure how this is intended, whether templates can act as immutable groups or the permissions just get copied across to the group. I would not want templates to be applied by default, however.

On this specifically, the intention is that a default that some templates might want to set for permissions is explicit false.

[ST-DDT]

I wished, that plugins would not mess with the default subject either.
The plugins should only define the descriptions and suggest specific values for certain ranks.
If no perm plugin is present, then Sponge's PermissionService should only check the *:USER (or *:DEFAULT) rank (or a list of configured ranks) and take that value and else depend on isOP().
If a perm plugin is present, then it might initially setup itself in a similar fashion, but if the admin removes a rank/permission from the default, then it should be gone for good. Plugins shouldn't sneakily add it back (at least not as the recommended way). That way you don't have any issues with newly added default true permissions.
The server admin added the permission plugin for a reason.

[zml2008]

This is designed as a helper method that delegates to the default subject -- so a PermissionDescription does not hold that data itself -- it just provides convenient access so all the plugin developer-focused metadata about a permission can be set up in one place.

The reason the default subject is exposed in the API is so that plugin developers can register permissions that should be granted by default on its transient data. This has been used since the beginning by SpongeCommon itself for permissions registrations. It's up to the specific permissions plugin to let users override -- for example PEX makes the persistent data take priority over the transient data for the default subject.

in the event no permissions plugin is present, spongecommon already performs a simplified version of permissions resolution logic based on transient data and the user's op level, and that has not changed since the api was originally implemented.

[XakepSDK]

for example, only copying the permissions provided by a role from one plugin.

Can it not only be copied, but applied transparently? So when plugin update introduces new permission, it would be applied to user / group / etc automatically.

[zml2008]

Can it not only be copied, but applied transparently? So when plugin u[date introduces new permission, it would be applied to user / group / etc automatically.

Yes, it would be possible to add the role template subject as a parent of the user.

[ejm]

Heya, SubjectData::getPermissions gives me all directly assigned permissions of a player. Is there a way to get all inherited permissions as well?

From #dev on 2020-08-03 00:06

It would be nice to have some way of getting all permissions for a given Subject, regardless of the source. Right now, the only way to do it would be to check through each parent individually

[XakepSDK]

Why do you want to get all permissions?

[ejm]

I'm not convinced the reasoning is relevant, but, at the time I wanted to run a regex against it on certain users.

[lucko]

I have some concerns about the move to using Cause instead of Set<Context for Subject query methods.

Loose example:

Subject s1 = service.getUserSubjects().loadSubject("--some-uuid--").join();
Subject s2 = service.getUserSubjects().loadSubject("--some-other-uuid--").join();

boolean c1 = s1.hasPermission("some.permission");
boolean c2 = s2.hasPermission("some.permission");

It seems reasonable to expect that the hasPermission method calls will use cause/context corresponding to the Subject (and associated object) being queried, however, unless the user pushes these onto the Cause stack beforehand, that won't actually happen.

Maybe the fix is for these methods to push this and this.getAssociatedObject() onto the stack before getting the current cause stack?

Also, the methods can be called async, so we need to account for that too.

[zml2008]

@gabizou would appreciate your input since this is an issue me and luck haven't been able to reach a conclusion on.

There are two main cases for permissions checks, with different contexts applicable for each:

1. Current game state (for protection checks, etc): this works fine using the normal cause tracker
2. State of the subject itself (for notifications, announcements, etc):
   This is a bit trickier, since we have to build a custom cause representing the subject's associated object. I've tried to do that here -- is this at all correct?

Right now I've tried to do different defaults depending on how the cause is queried -- a CommandCause uses its stored Cause which represents game state, while a direct query to Subject will use the associated object (aiming to be similar to API 7 behaviour). This split is inconvenient, but I think it's the only way to provide behaviour that is close to what developers are looking for most of the time. Do you have any thoughts on a better way to capture/pass around a representation of game state?