Permission documentation #536
Conversation
| * ``myPlugin.commands.teleport.others`` | ||
|
|
||
| Only grants the user the permission to teleport others. | ||
| (He still needs the ``myPlugin.commands.teleport.execute`` to actually execute the command.) |
There was a problem hiding this comment.
It's great to have examples, but this gets a bit long - and is bloated by the repetition of this line. Perhaps you can state that it is a prerequisite for the following permissions. However, is it truly necessary to have an .execute permission, or is that just a choice in your (theoretical) implementation?
There was a problem hiding this comment.
It's great to have examples, but this gets a bit long
I try to shorten it a bit.
However, is it truly necessary to have an .execute permission
How else would you grant the user the permission to execute that command?
Of course you could check each sub permission (other, worlds etc, but thats way to much work IMO)
| Example | ||
| ~~~~~~~ | ||
|
|
||
| In the permission plugin's config this might look like this. |
There was a problem hiding this comment.
This sentence is a bit clumsy. Drop the "In" and the first "this" and it makes more sense to me.
| - myPlugin.commands.teleport.worlds | ||
| - !myPlugin.commands.teleport.worlds.protectedworld | ||
|
|
||
| In this case the user `Spongie` was granted the permission to execute the teleport command for himself and other players |
| - !myPlugin.commands.teleport.worlds.protectedworld | ||
|
|
||
| In this case the user `Spongie` was granted the permission to execute the teleport command for himself and other players | ||
| to teleport him to all worlds, but `protectedworld`. |
There was a problem hiding this comment.
all worlds, other than protectedworld.
|
|
||
| .. note:: | ||
|
|
||
| Persistent permission assignments always take precedence before transient ones. |
There was a problem hiding this comment.
... "take precedence over transient ones."
There was a problem hiding this comment.
Was I really that tired while writing it? Fixed
| Permission | ||
| ========== | ||
|
|
||
| A permission is a case-sensitive??? hierarchical string key that is used to determine whether a Subject can do a specific action or not. |
There was a problem hiding this comment.
|
|
||
| Examples | ||
| ~~~~~~~~ | ||
|
|
There was a problem hiding this comment.
I think it's worth stressing that these are only examples, not requirements.
It is not absolutely necessary that plugins implement "myplugin.commands", nor is it necessary to have ".execute" or ".all".
They're simply suggestions.
| Subject | ||
| ======= | ||
|
|
||
| A Subject is a holder of assigned permissions it can be obtained from all CommandSources. |
There was a problem hiding this comment.
Also note that anything that should, or can hold permissions is a subject. Subjects are not just command sources.
Additionally, important to know that they can be inherited themselves, or inherit other subjects.
There was a problem hiding this comment.
I hope i fixed this one properly now.
| Example | ||
| ~~~~~~~ | ||
|
|
||
| The permission plugin's config might look like this. |
There was a problem hiding this comment.
Poor example IMO and totally unnecessary.
Individual implementations can document their own layouts, it's got nothing to do with Sponge Permissions.
|
|
||
| .. note:: | ||
|
|
||
| Persistent permission assignments always take precedence over transient ones. |
There was a problem hiding this comment.
It's not a requirement.
For defaults this might make sense, but for regular Subjects, it would make more sense that transient permissions would override permanent ones.
Either way, it's not documented, nor a requirement. It's totally down to the implementing plugin to decide.
There was a problem hiding this comment.
Fixed for now. But IMO the written word always have more weight that the transient one.
If a server owner (persistently) defines that a user should not have a specific permission than the transient assignment of a plugin should be ignored.
Could you give me an example in which case it would be better the other way round.
There was a problem hiding this comment.
Example: A user usually has a permission set to true, but for the duration of their session, you want to override it with a negated value.
You're forgetting that transient values aren't just for defaults. They might have other uses, where you want to temporarily override a persisting permission.
Usually with permissions, more specific rules would take priority over more generic ones, and I'd consider a transient ruling to be more specific.
Either way, it's not a requirement that providers implement it in a specific way. 😉
| * Group | ||
| * System | ||
| * Command Block | ||
| * Role Template |
There was a problem hiding this comment.
Perhaps actually explain what these are. What goes in a Role Template SubjectCollection? What's it's use? 😕
|
|
||
| A Subject is a holder of assigned permissions it can be obtained from all CommandSources. | ||
| Permissions can be granted or denied. Plugin authors will usually only deal with Subjects. | ||
|
|
There was a problem hiding this comment.
I'd also disagree with that. You can't get a Subject without using the main PermissionService and then a SubjectCollection.
|
|
||
| .. code-block:: text | ||
|
|
||
| users: |
There was a problem hiding this comment.
Additionally this is YAML, which isn't really used by any plugins, afaik.
Most providers currently are storing in json.
Either way, it's pointless documenting it, as it will be of no use at all. Different providers will use different formats.
|
|
||
| Subjects also inherit permissions from their parents, unless the given permission is defined in the subject itself. | ||
| Following that rule if any of his parents is granted the ``myPlugin.commands.teleport`` permission the user would | ||
| automatically inherit the ``myPlugin.commands.teleport.all`` permission allowing him to teleport all players at once. |
There was a problem hiding this comment.
This bit should ideally go in it's own section IMO.
We should separate "vital" information, about plugin's checking for and providing their own permissions from information about how to use PermissionService.
| Contexts | ||
| ======== | ||
|
|
||
| TODO: A context (name) that may or may not apply to a `Subject` and can be used to assign permissions only in specific |
There was a problem hiding this comment.
My best description would be:
If you consider each permission to a privilege or ability to be able to do something, a Context is the circumstances where that privilege is usable.
You might want to give a Subject permission to do something, but only when the Subject is in a certain world, or in a certain Region.
Contexts are accumulated by a Subject, and are then used by the PermissionService to decide if the Subject has a privilege or not.
Sponge provides some contexts by default, but it is generally down to other plugins to provide additional contexts to the PermissionService, through a ContextCalculator.
There was a problem hiding this comment.
Thanks. I guess I will just use this.
|
To anwer the question you asked on the last paragraph on this PR: |
|
@Tzky Thanks for the info. I guess I will stick to http://rst.ninjs.org until I get time to setup my own system. |
|
I updated the definitions. Please check them once more. A few questions in between:
|
|
I'm pretty sure the system subject contains the server console. I managed to set up PEX in a way that denied console commands, early in testing. What use that is, is anybody's guess. |
| ``Subject`` that might be. | ||
|
|
||
| If neither the subject itself nor any parent subjects grant or deny a permission then it will be inherited from the | ||
| default ``Subject``. The default subject can be obtained from the ``PermisionService``. |
There was a problem hiding this comment.
You left an s out of PermissionService
| SubjectData | ||
| =========== | ||
|
|
||
| SubjectData are the actual permission store connected to the Subject. There are two types of Subject stores. The |
There was a problem hiding this comment.
I'd use a colon, ie. "Subject stores:".
Additionally, a quibble: should we be saying "saved to disk" or "saved to file"? Maybe it's easier just to say "saved" and stay system agnostic.
|
|
||
| SubjectData are the actual permission store connected to the Subject. There are two types of Subject stores. The | ||
| transient one, that won't be persistent to the disk, and the persistent one that is loaded from and saved to disk. | ||
| Plugin authors should think whether it is necessary to persist a value when choosing between them. Except for the |
| Inheritance | ||
| ~~~~~~~~~~~ | ||
|
|
||
| If a user has the permission ``myPlugin.commands`` then he automatically has all sub permissions such as |
There was a problem hiding this comment.
replace "he" --> "they", "has" --> "have"
| ===================== | ||
|
|
||
| The permission description is an utility that is meant to provide the server owner with details on a certain permission. | ||
| PermissionDescriptions is an optional feature a PermissionService can provide. The creation of PermissionDescriptions |
There was a problem hiding this comment.
replace "is" --> "are"
Or, replace "PermissionDescriptions" with "A PermissionDescription"
|
|
||
| * the target permission id | ||
| * a Text description | ||
| * and an assigned role |
| Subject | ||
| ======= | ||
|
|
||
| A ``Subject`` is a holder of assigned permissions it can be obtained from the ``PermissionService`` via |
There was a problem hiding this comment.
New sentence needed between "permissions" and "it".
|
|
||
| A ``Subject`` is a holder of assigned permissions it can be obtained from the ``PermissionService`` via | ||
| ``SubjectCollections``. | ||
| ``CommandSources`` such as ``Players`` are ``Subjects`` by default, but there are many other types of ``Subjects`` as |
There was a problem hiding this comment.
Remove "as well". Doesn't make grammatical sense.
| SubjectCollections | ||
| ================== | ||
|
|
||
| A container for subjects that can be used obtained a Subject by name. |
There was a problem hiding this comment.
Replace "used obtained" --> "used to obtain"
| These are the default Subject Collections: | ||
|
|
||
| * User | ||
| * Contains all on-line and off-line ``User`` / ``Player`` that exist. |
There was a problem hiding this comment.
Not necessarily. The likelihood is that this will only contain online players.
Some implementations may still persist any changes made, or they might just return a transient dummy subject.
There was a problem hiding this comment.
I think not having support for offline users is a mistake, and should not be encouraged in the docs, but you are correct in saying it's not in the spec.
| SubjectData | ||
| =========== | ||
|
|
||
| SubjectData are the actual permission store connected to the Subject. |
| There are two types of Subject stores: | ||
|
|
||
| * The transient one, that won't be persistent at all | ||
| * and the persistent one, that is loaded from and saved to a persistent storage |
There was a problem hiding this comment.
You're right, but it's not a very clear description IMO.
Transient = Only lasts for the duration of the session, is never saved
Regular (persistent) = Might be saved somewhere, and therefore be persisted and exist forever.
(It is not a requirement that data is persisted.)
Some users might not even understand what "persistent" means, so explaining in terms of the session's duration is probably easier for most users to understand.
| Subject Options | ||
| =============== | ||
|
|
||
| Subjects may also provide the possibility to store string options. These are basically key value pairs that can be |
|
|
||
| @Override | ||
| public void accumulateContexts(Subject subject, Set<Context> accumulator) { |
There was a problem hiding this comment.
Should I simplify the example to:
@Override
public void accumulateContexts(Subject subject, Set<Context> accumulator) {
Arena arena = arenasByPlayer.get(subject);
if (arena != null) {
accumulator.add(IN_ANY_ARENA);
accumulator.add(arena.getContext());
}
}?
There was a problem hiding this comment.
Not a great example IMO. A better example would be to map the name of the arena to the Context, instead of a *, and then only add the context to the accumulator if the user was in an arena.
They're meant to be used as key value pairs, not just a collection of keys.
So:
if (user in arena):
add context where key="myArenaPlugin_arena", value="arenaName"
There was a problem hiding this comment.
Well I'm quite unsure about it myself.
I used two contexts here to help the permission plugin / server owner to assign permissions.
One context for a general "well he is playing right now" and one for "he is playing in arena x".
Should I add code comments?
I had issues finding an actual definition in what way/how exactly the key value pairs are supposed to be used.
Is it possible for the permission service to check for a wildcard context value? IMO no, because the context calculator does not specify wildcards.
I'm not sure about your example either. IMO both describe the same thing in an almost identical way.
There was a problem hiding this comment.
Ok, better example.
You want a permission to only apply on the "world_nether" world, (I know Sponge gives them DIM-x names, whatever, just an example.)
The key world be "world", the value would be "world_nether".
There is no such thing as "wildcard" contexts, it's down to the plugin who provides the calculator, or the plugin implementing permissions.
My point is, instead of: key="in_arena", value="*", it would make more sense to have key="in_arena", value="true", or have the value equal the name of the arena.
There's no concept of wildcard Contexts, and especially in the documentation, it's important to show that Contexts are a collection of key value pairs, not a collection of keys.
(I actually think the world example is the easiest to understand, however I understand you're trying to document the calculator here, not the concept of Contexts)
It's also misleading to have a static Context. The value should vary between Subjects.
|
@lucko in response to #536 (comment)
I changed that. I hope its now easier to understand.
Maybe I can do both.^^
Why would they vary? IMO you are either in a arena or not. |
|
@ST-DDT RE: Static contexts, yeah, you're right. If you've only got a boolean as a value, then yes, I guess a static context is fine, however, as previously mentioned, I think an example where you have more than just two possible values is a better demonstration of the API. e.g. key=myPlugin_arena, value=some_arena_name Swapping the "*" for true was my main point, really. Plugins can use Contexts however they like, it's just a case of giving a wide range of appropriate examples so they can understand the point behind them. (and IMO, the current example with just true/false keys doesn't demonstrate Context to the best possible extent. 😄 ) Edit: Sorry, my bad. I hadn't actually read your most recent changes before posting. The current example is perfect. 👍 |
|
Then i think this ready for a final review. Or shall I squash it first? |
|
|
||
| public class MyArenaPlugin implements ContextCalculator<Subject> { | ||
|
|
||
| private final Context IN_ANY_ARENA = new Context("myArenaPlugin_is_in_arena", "true"); |
There was a problem hiding this comment.
Is this meant to be static? Otherwise should not be ALL_UPPER.
Inscrutable
added
needs review
|
Just waiting on you @kashike then we can pull the lever and make this creation live! |
|
|
||
| The permission description is an utility that is meant to provide the server owner with details on a certain permission. | ||
| PermissionDescriptions are an optional feature a PermissionService can provide. The creation of a PermissionDescription | ||
| does **not** have any impact on whether a permission exists, who has access or what its default value is. |
There was a problem hiding this comment.
Change does to should?
Sure a permissions implementation isn't expected to act based on the permission descriptions currently, but they could, and I don't think it would be outside the spec.
There was a problem hiding this comment.
There was a problem hiding this comment.
Re-reading it I realize that it's not so much talking about possible inheritance from templates, but more for people who may have thought they were required to create a permission. I'm alright with the wording now.
|
|
||
| If neither the subject itself nor any parent subjects grant or deny a permission then it will be inherited from the | ||
| default ``Subjects``. Each ``SubjectCollection`` defines its own defaults. The global and weakest default subject can be | ||
| obtained from the ``PermissionService``. It is recommended that plugins define their own permissions to the default's |
There was a problem hiding this comment.
Is it really?
It makes sense for plugins with complex context hierarchy, but honestly having default permissions that you need to remove as a server owner is a bigger pain then them having none and explicitly having to grant each one.
There is a reason why we didn't follow Bukkits paths of having permissions be registered and on by default, and used the permission descriptions instead.
There was a problem hiding this comment.
This is meant to be about documenting the service, not complaining about the way it operates. (I do agree with you though. It was mentioned in a SpongeAPI issue too.)
If you don't register your defaults, then players will not have them by default. It's pretty simple.
Regarding needing to remove them as a server owner, the idea is that plugins register their defaults to the transient SubjectData, and then the persistent SubjectData will override.
There was a problem hiding this comment.
I hope the permission plugins will come up with a way to export/dump the transient and persistent permission settings so that you can adjust them as needed.
Also I'm not sure whether the default is required to be in the hierarchy of all Subjects.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, but I mean, we shouldn't be recommending it.
It is recommended that plugins define their own permissions to the default's
There was a problem hiding this comment.
Well, it's a requirement if an author wants their permission nodes to be given to users by default, but some plugins (e.g. admin tool type plugins) wouldn't want that. Some might also just provide a list of permissions in their documentation.
So, it's not really a requirement, nor a recommendation. Either way you're right, it needs to be clarified.
|
|
||
| * User | ||
| * Contains all on-line ``Players``. It may also return subjects for off-line ``Users``. However there aren't any | ||
| specifications regarding this, so they might be fully transient dummy subjects. |
There was a problem hiding this comment.
I don't think this is simple enough to understand.
There was a problem hiding this comment.
I don't see how you could make it any more simple.
- contains users
- guaranteed to contain online users
- might contain offline users too
- if a user is not loaded, a transient dummy (probably) gets returned. changing the state of the returned subject has no impact.
There was a problem hiding this comment.
How about this:
Contains all on-line
Players. It may also return subjects for off-lineUsers. However there aren't any specifications regarding this, so transient dummy subjects might be created on the fly for users that are currently off-line, if they are requested from theSubjectCollection..
There was a problem hiding this comment.
The way PEX implements this is that it treats online and offline players the same -- the only difference is that online players are preloaded into cache on login and offline players may take some time to load. I think it makes sense to mandate that offline players return subjects just as well as online players, since internally they work pretty much the same.
There was a problem hiding this comment.
IMO for the current version of the API this should absolutely be returning valid subjects for offline users, In a future revision maybe we can use futures/optionals, but for this version we should be documenting that it returns valid subjects.
But... on the other hand for large servers that could be an awful lot of data returned that's just duplicated due to users having default permissions..
There was a problem hiding this comment.
Two assumptions:
- All methods within PermissionService should be "main thread" friendly. I assume that's the same for the rest of Sponge.
- You should absolutely not be doing any sort of I/O, or any blocking operation for that matter, on the main Minecraft server thread.
The two conflict with the idea that SubjectCollection#get should be able to return offline users. So, which of my assumptions is wrong?
There was a problem hiding this comment.
If you aren't returning valid users for everyone your permissions plugin is wrong.
With a file backend everything is loaded into memory anyways, there's just a little more deserialization. The async handling here really only becomes an issue once you get to using a database backend or something -- which is not that common on Sponge yet. While the methods here should eventually be improved, for now the only choice that makes sense is to return valid subjects for any query.
By requesting a subject, you're not requesting all its parents, and subjects with the default group only that haven't had changes explicitly made shouldn't be automatically registered so wouldn't appear in the list of all subjects in a collection, so that won't be as massive
There was a problem hiding this comment.
I hope this explanation is now easier to understand and closer to the requirements.
| * Contains all on-line ``Players``. It may also return subjects for off-line ``Users``. However there aren't any | ||
| specifications regarding this, so they might be fully transient dummy subjects. | ||
| * Group | ||
| * Contains all group ``Subject``. Groups are a simple way of structuring a ``Subject's`` inheritance tree. |
There was a problem hiding this comment.
The main difference between a group and a random subject, is that groups have a name that can be accessed by other plugins.
Groups should be used where it makes sense to have a collection of subjects belonging to a team, faction, or role.
Personally I prefer checking for a permission, but getting all players with a defined permission can be costly if you really do need to be able to get a group, as opposed to checking for a permission.
There was a problem hiding this comment.
I added some explanations regarding the first part of this.
However for the second one: Getting a group will not help you with the goal of "finding all players with a given permission", because you cannot obtain the children from a Subject. And if you query the inheritance of each player than you can query for the permission directly as well.
| with the permissions of the creator. | ||
| * Role Template | ||
| * Contains all role template subjects that are used in ``PermissionDescriptions``. Useful to lookup all recommended | ||
| permissions for a user. These should not be used for inheritance. |
There was a problem hiding this comment.
Why not? I mean, sure they contain the template permissions, but other then that, inheriting off a role should be a decision that the server owner / permissions provider can make.
There was a problem hiding this comment.
They're called "templates" for a reason. 😉
| * If it is for a long time or forever (ex a promotion to VIP) use the regular (persistent) one. | ||
|
|
||
| Except for the default's subject data it is not specified which one takes precedence over the other. | ||
| It is not recommended to set it to both stores at the same time to overcome the unspecified precedence. |
There was a problem hiding this comment.
You specified the order above, Which is it? Does it have precedence or not?
There was a problem hiding this comment.
It's completely at the discretion of the implementing plugin. The JavaDocs have no mention of which should take priority.
There was a problem hiding this comment.
Currently PEX has transient having priority over persistent, but I'm debating whether or not that's the best way to do things, so we should probably figure that out among the permissions plugins before specifying it in the documentation.
There was a problem hiding this comment.
nitpick "not recommended" or "discouraged"?
we should probably figure that out among the permissions plugins before specifying it in the documentation
+1 , possibly remove this for now and track it in an issue for later so we can get this published?
There was a problem hiding this comment.
I think it was agreed among current plugin authors that for defaults only, having persistent take priority makes the most sense. IMO, this should therefore become a requirement within the API. It's not in the JDs yet, so idk if we want to exclude this section from SpongeDocs until it gets PRed into SpongeAPI.
I'm not sure if zml has made this change for PEX yet, but as far as I know, he plans to.
There was a problem hiding this comment.
There are quite a few places in the docs that are effected in a similar way.
I will change it to "discouraged" for now and wait for a decision on this topic (precedence).
|
|
||
| Subjects also provide the possibility to store string options. These are basically key value pairs that can be | ||
| assigned and inherited. Unlike the permission strings the keys are not hierarchical and don't provide any inheritance | ||
| mechanisms themselves. |
There was a problem hiding this comment.
I'm not sure this is true.
Options should absolutely be hierarchical and follow the same rules that permissions do.
There was a problem hiding this comment.
They are not hierarchical in the same sense that permission nodes are.
myPlugin will also grant myPlugin.commands and myPlugin.commands.something. This is not true for options.
There was a problem hiding this comment.
Oh right. I think a distinction needs to be made here on node hierarchy and parent hierarchy.
There was a problem hiding this comment.
Agreed. That's a much better way to phrasing it too.
| Sponge provides some contexts by default, but it is generally down to other plugins to provide additional contexts to | ||
| the PermissionService, through a ``ContextCalculator``. | ||
|
|
||
| When creating contexts for your own plugin please try to avoid conflicts with other plugins (ex by choosing a unique |
There was a problem hiding this comment.
Recommend prefixing with plugin id?
| @@ -223,7 +225,8 @@ Subject Options | |||
|
|
|||
| Subjects also provide the possibility to store string options. These are basically key value pairs that can be | |||
| assigned and inherited. Unlike the permission strings the keys are not hierarchical and don't provide any inheritance | |||
| mechanisms themselves. | |||
| mechanisms themselves. But the key value pairs itself are inherited from parent ``Subjects`` in the same way permissions | |||
| are. | |||
There was a problem hiding this comment.
Needs a comma here :P
themselves. But the --> themselves, but the
There was a problem hiding this comment.
Thx for your fast review. Fixed.
| obtained from the ``PermissionService``. It is recommended that plugins define their own permissions to the default's | ||
| transient ``SubjectData`` during every server start-up. This allows the server owner to overwrite the defaults defined | ||
| by plugins according to his needs using the default's persistent ``SubjectData``. | ||
| obtained from the ``PermissionService``. It is recommended (but not mandatory) that plugins define their own permissions |
There was a problem hiding this comment.
I'm heavily against even having the word recommended even be here.
It is possible for plugins to define their own permissions on the default transient ``SubjectData`` during every server start-up, however care should be taken not to introduce unnecessary complexity.
This allows the server owner to overwrite the defaults defined by plugins according to their needs using the default's persistent ``SubjectData``.
This however requires the server owner to have knowledge of the default permissions your plugin is granting in the first place, so must be carefully documented.
It is recommended that you do not have any default permissions assigned, unless you are very sure that it will reduce the complexity of your plugin, most plugins should never need to do this.
There was a problem hiding this comment.
It's a case of whether we want to encourage plugins that work "out of the box" or plugins where some work has to go into configuring their permissions.
I think it's better that you have to explicitly give each permission; it avoids a situation where your players get automatically given access to something you weren't even aware of. It also forces people to read documentation, which is only a good thing.
However, if that's the stance we want to take, then what's the point in having default subjects in the first place?
I'm not really sure how I feel. I can see the benefits and drawbacks of each approach.
There was a problem hiding this comment.
You have seen the default permissions that GP grants right? That's why.
99 times out of 100 anything that people are doing with default permissions should be done with roles and permission descriptions instead.
There was a problem hiding this comment.
Eh, the JavaDocs need to be changed then. It's made very clear that PermissionDescriptions should not be used for inheritance or permission calculation. Same with role templates - they're not meant to be used directly.
The API (maybe?) suggests that it would be acceptable to use roles as a template, by copying their SubjectData to a group or the default subjects, but that's really the limit of their use, within the bounds of the JDs.
So to clarify, roles and permission descriptions should not be used for default permissions, ever. The API specifically prohibits that.
There was a problem hiding this comment.
I wasn't suggesting that here.
I was suggesting that giving players permissions by default is a bad idea, as opposed to letting the server admin explicitly define them, with help from the roles.
There was a problem hiding this comment.
Ah, I misunderstood slightly.
Still, goes back to my previous point:
However, if that's the stance we want to take, then what's the point in having default subjects in the first place?
There was a problem hiding this comment.
IMO the defaults are useful for the times where you dont have a custom PermissionService or dont want to configure them. Install it and it works.
There was a problem hiding this comment.
What's install it and it works for one person isn't necessarily install it and it works for another.
Take for example 2 plugins that have some overlapping functionality, you only want players to use one of them, but the other plugin has already granted the permissions.
Because both of them took the install it and it works attitude you now have multiple commands from the very first time that you run the server with the plugin installed.
For the majority of Minecraft server owners who "test in production" this can end in ruin.
There was a problem hiding this comment.
Thats why i asked whether the default one is required to be set.
IMHO As long as there is a default its recommended to set it. However I agree with you that the specification needs to be revised to better reflect the actual needs.
There was a problem hiding this comment.
@zml2008 Would you mind giving your opinions here? It's not very clear either way, and I think both sides are pretty valid options.
In attempt to tl;dr, it's basically: plugins should declare default permissions to the default subjects vs server admins should have to add them manually, after consulting the plugins documentation.
|
|
||
| * the target permission id | ||
| * a Text description | ||
| * an assigned role |
There was a problem hiding this comment.
multiple assigned roles (rename?) are possible or am I wrong?
Inscrutable
removed
the
needs review
|
OK, I just want to confirm that everyone is done reviewing, and that we can pull this beast. |
Inscrutable
added
the
needs review
| * If it is for a long time or forever (ex a promotion to VIP) use the regular (persistent) one. | ||
|
|
||
| Except for the default's subject data it is not specified which one takes precedence over the other. | ||
| It is not recommended to set it to both stores at the same time to overcome the unspecified precedence. |
There was a problem hiding this comment.
nitpick "not recommended" or "discouraged"?
we should probably figure that out among the permissions plugins before specifying it in the documentation
+1 , possibly remove this for now and track it in an issue for later so we can get this published?
|
|
||
| * User | ||
| * Contains all on-line ``Players``. It may also return subjects for off-line ``Users``. However there aren't any | ||
| specifications regarding this, so they might be fully transient dummy subjects. |
There was a problem hiding this comment.
IMO for the current version of the API this should absolutely be returning valid subjects for offline users, In a future revision maybe we can use futures/optionals, but for this version we should be documenting that it returns valid subjects.
But... on the other hand for large servers that could be an awful lot of data returned that's just duplicated due to users having default permissions..
|
|
||
| The permission description is an utility that is meant to provide the server owner with details on a certain permission. | ||
| PermissionDescriptions are an optional feature a PermissionService can provide. The creation of a PermissionDescription | ||
| does **not** have any impact on whether a permission exists, who has access or what its default value is. |
There was a problem hiding this comment.
Re-reading it I realize that it's not so much talking about possible inheritance from templates, but more for people who may have thought they were required to create a permission. I'm alright with the wording now.
|
|
||
| If neither the subject itself nor any parent subjects grant or deny a permission then it will be inherited from the | ||
| default ``Subjects``. Each ``SubjectCollection`` defines its own defaults. The global and weakest default subject can be | ||
| obtained from the ``PermissionService``. It is recommended (but not mandatory) that plugins define their own permissions |
There was a problem hiding this comment.
This still has the word recommended here.. Until @zml2008 confirms one way or another, can we just leave it neutral?
Plugins may define their own permissions to the default's transient SubjectData during every server start-up. This allows server owners to overwrite the defaults defined by plugins according to their needs using the default's persistent SubjectData.
My personal preference is that we don't even leave it neutral and actively discourage it unless the plugin author can think of a valid usecase other then "defaulting all my user role perms to true is helpful"
ie: complex hierarchy, specific contexts, hard to do right stuff.
Plugins may define their own permissions to the default's transient SubjectData during every server start-up. This allows server owners to overwrite the defaults defined by plugins according to their needs using the default's persistent SubjectData.
warning::
You should think carefully before granting default permissions to users. By granting the permissions you are assuming that all server owners will want these defaults (at least the first time the plugin runs) and that exceptions will require server owners to explicitly deny the permissions (which can't even be done without a custom permissions service implementation) This should roughly correspond to a guest on a single player lan world without cheats.
There was a problem hiding this comment.
I don't think it's right to merge this Docs PR until this issue gets resolved. It's a major part of the API. You know my opinions from before, we just need a decision from zml, really.
There was a problem hiding this comment.
Yeah, I agree with @ryantheleach here. Allowing permissions by default should not be done in most cases -- primarily if you have a plugin that restricts normal game functionality by permission (like FeatherChat gives chatting a permission, so without defaults installing FeatherChat would block all users from chatting until the server admin configured the plugin), or want to explicitly block permissions from being included when a user grants all permissions (VNP did this on Bukkit iirc).
There was a problem hiding this comment.
👍 Thanks zml.
The concept of re-adding normal game behaviour makes perfect sense - and yeah, avoids the mess that Bukkit had.
There was a problem hiding this comment.
Perhaps add the example of using defaults to re-add default game behaviour? I think it's an important point to be made. @zml2008 probably gave the best example of this, referring to use of the chat. Idk, probably would be good to have somewhere on that page?
| Order of precedence in descending order: | ||
|
|
||
| * Subject itself | ||
| * Parent Subjects |
There was a problem hiding this comment.
Mention transients take precedence over Persistent for subject and parents.
There was a problem hiding this comment.
Has this been decided yet?
There was a problem hiding this comment.
For default subjects: transient takes priority over persistent.
For all other subject types: persistent takes priority over transient.
There was a problem hiding this comment.
the other way around?
I feel transient has a higher priority than persistent data as its more specific. (except for defaults)
There was a problem hiding this comment.
IMO the server owner should always have the ability to overwrite any plugin provided permission value.
=> Persistent is always stronger than transient
If a plugin wants to temporarily grant a permission it should do so in an overwriteable+transient way or should use custom contexts calculators.
There was a problem hiding this comment.
That's exactly what has been said already, just in slightly different words. A server owner can override the transient defaults, by adding a default that persists. 😉
There was a problem hiding this comment.
@lucko Not just the defaults. Every subject's persistent values takes priority over transient ones.
I just want to make sure before changing the docs.
There was a problem hiding this comment.
No. For all subjects except defaults, transient takes priority over persistent. The reasoning was given above by Faithcaio; transient is considered more specific, and therefore takes priority.
In which way is it more specific? Lets use a real world example.
Or am I wrong here at any point? |
|
That's a pretty weird example IMO. (maybe I'm just too stupid to understand) The basic idea is that a transient permission will not last for as long as a persistent one, and therefore is more "temporary". As it's more "temporary" than a standard permission that just sits in storage, it's therefore more specific. My example of that is: I think the actual question we should be asking is whether transient/persistent nodes are flattened before individual node inheritance is applied. Assuming that transient does in fact take priority, which is more specific in this case? I know it's certainly not documented in the API. Maybe for the sake of this PR, and @ST-DDT's sanity, I shouldn't have even asked. Huh, let's ignore that for now. 😆 |
|
I think some permissions providers have been incorrectly assuming that somePlugin.something would be more specific. Some world rules for arena building. A transient permission for the round while the arena is in play. This example it's clear what the plugins and by proxy server owner had in mind. and that is that all building should be denied for the duration of the arena session. Doing otherwise assumes that the arena plugin knows there are sub-nodes, how to get them, and how to iterate over all of them in order to blanket deny / allow something temporarily. That said, I'm sure with some effort we can come up with a counter-example to this as well... |
Why would plugin Z want to do that if not because someone with enough permissions(the server owner) asked it to? If a subject has I still feel that for normal Subjects (maybe defaults too) the transient data should have higher priority. |
|
|
||
| .. code-block:: java | ||
|
|
||
| public canTeleport(Player subject, World targetWorld) { |
There was a problem hiding this comment.
Urr, that's not a proper method declaration. I think you're missing boolean
|
|
||
| .. note:: | ||
|
|
||
| If ``SubjectCollections are queried for a ``Subject`` it will automatically be created, if it does not already |
There was a problem hiding this comment.
If --> When
it will automatically be created, if it does not --> they will automatically be created, if they do not
|
I think stuff being discussed about priority ordering (which priority takes priority… :c) is probably best discussed over at SpongeAPI. Even if a conclusive decision is made about it, it’s not anywhere in the Java documentation, so I think you just have to assume that implementations can decide either way - in which case, any discussions here are pointless. Just taking a slight step back; @ST-DDT has done a great job at explaining above and beyond what was already explained in the JDs. The issues being raised aren’t really about the documentation - rather with the API itself. IMO, it’s probably best to take these discussions to SpongeAPI & allow the docs to be merged. I know zml has started on a 6.x branch, hopefully the remaining ambiguities will be resolved then. 🙂 IMHO this has probably gone on long enough now (hypocritical I know, I’ve definitely been a main source of problems/issues - sorry!), I think it’s probably ready. The remaining issues can be ironed out over time, as and when the appropriate changes are made to the API side of things. 😄 |
|
Minor question regarding #541 Or shall we decide on/document this later? |
|
The API doesn't really mention anything about implementations of I would guess that most implementations wouldn't allow more than once instance of the same Subject, in which case, comparing a Subject's identifier and the name of its containing collection is probably the most reliable approach. I would probably discourage storing any reference to Subjects, especially in Sets and Maps, (you're relying on implementations implementing equals & hashcode properly) but, that's just my personal opinion. I guess it can just be left out for now. (and no, as far as I know, the issue raised in #541 does not apply to anything in PS.) |
|
@lucko I ask because
But I guess this is something for the API to specify... |
|
No. SubjectCollections will return raw subjects (the implementation's class that just Players get wrapped with the Subject provided by the SubjectCollection. SubjectCollection should never return Player instances. I don't think that needs to be documented - it's the illogical thing to do. |
|
Oh yeah, you are right. I thought that you can check with equals whether a Subject belongs to a Player, but this is not possible. So basically a |
|
Sure, although actually, that's a good point to be made about ContextCalculators. Simply checking Your example of ContextCalculator violates this, and also violates the stuff mentioned in #541. You store Here's my modified version of the example already in the docs. IMO It better conforms the standards raised above: https://gist.github.com/lucko/39582eb6e2c22bf90b233363d62aaf83 |
|
Updated the precedence parts for |
|
I guess its (hopefully) ready for a final review and merge. I excluded the parts specified in SpongePowered/SpongeAPI#1411 for now. |
| Its only specified for the default ``Subjects`` that the persistent ``SubjectData`` takes precedence over transient | ||
| ones. | ||
| The default ``Subjects'`` persistent ``SubjectDatas`` take precedence over the transient ones. | ||
| For all other ``Subjects`` the transient ``SubjectDatas`` take precedence over the persistent ones. | ||
|
|
||
| If neither the Subject, nor any of its parents, nor the defaults grant and deny a permission, then it is automatically |
There was a problem hiding this comment.
"grant and deny" is just weird. Suggested alternatives: specify, "grant or deny", "assign a value to", set.
| @@ -231,8 +235,8 @@ Plugin authors should consider whether it is necessary to persist a value when c | |||
| * If it is only for a short time (ex during a minigame) then use the transient one. | |||
There was a problem hiding this comment.
ex should be e.g. (exempli gratia, "for example"). Latin and all that jazz.
| @@ -231,8 +235,8 @@ Plugin authors should consider whether it is necessary to persist a value when c | |||
| * If it is only for a short time (ex during a minigame) then use the transient one. | |||
| * If it is for a long time or forever (ex a promotion to VIP) use the regular (persistent) one. | |||
|
|
|||
| Except for the default's subject data it is not specified which one takes precedence over the other. | |||
| It is discouraged to set it to both stores at the same time to overcome the unspecified precedence. | |||
| Please refer to the Inheritance chapter if want to know more about the inheritance and precedence of the transient | |||
There was a problem hiding this comment.
It gets a whole chapter? I hope you mean page (or docs).
There was a problem hiding this comment.
Is the term section more appropriate?
Or maybe paragraph?
|
@Inscrutable Are the imports fine like this? |
Inscrutable
removed
input wanted
|
Has anybody some more change requests? If not then I will squash and merge it tomorrow evening. |
| ===================== | ||
|
|
||
| The :javadoc:`PermissionDescription` is an utility that is meant to provide the server owner with details on a certain | ||
| permission. ``PermissionDescriptions`` are an optional feature a :javadoc:`PermissionService` can provide. The creation |
There was a problem hiding this comment.
Minor and only nitpick with this pr, but I believe you can use PermissionDescription\ s to plural PermissionDescription. Same for other plurals and possessions later on in this page.
i.e. PermissionDescription\ 's and Subject\ s
There was a problem hiding this comment.
Yesterday I had issues with that because the tag needs to be surrounded by whitespace, but I will try it again.
There was a problem hiding this comment.
@12awsomeman34 I might have understood you wrong
Are you referring to section headline or the first sentence in those sections?
Or are you referring to the plurals in the Highlights?
Permission System
Creates some documentation on the permission system. Fixes: #32 , #136
If you would like to help me just go ahead.
TODO
TBD
SubjectDataPS
Please note that this is my first participation on this docs, so I would appreciate early feedback and suggestions for improvements.
Links