ArcanePlugins / Treasury

🏦 A powerful multi-platform library for next-level plugin integrations.
https://hangar.papermc.io/ArcanePlugins/Treasury
Other
56 stars 13 forks source link

Implement Permissions API #229

Open MrIvanPlays opened 1 year ago

MrIvanPlays commented 1 year ago

Currently, this lays the ground of a Permissions & Chat API for Treasury. I'm opening this not just for discussion, but to implement sane ideas suggested in this PR.

MrIvanPlays commented 1 year ago

there's another problem we need to discuss: that's about prefix and suffix nodes. their data type is a component. but the problem is that spigot and bungeecord utilize bungeecord-chat whilst any other server/proxy implementation (sponge, velocity, paper, etc.) utilizes adventure. if we opt in for bungeecord-chat components, then we need to download bungeecord-chat for the platforms where they don't have it and translate bungeecord-chat -> adventure, or the reverse, if we opt in for adventure components, then we need to download adventure for the platforms where they don't have it and translate adventure -> bungeecord-chat. a solution i thought of is create our own chat api, but that's basically just unnecessary reinvention of the wheel.

MrNemo64 commented 1 year ago

Sorry for avandoning Treasury for so long. I just dont have time to keep up. But I saw that you're asking for help. I haven't got time to check what ideas you have proposed but:

solution i thought of is create our own chat api, but that's basically just unnecessary reinvention of the wheel.

Are these two systems too diferent from each other? Couldn't you just abstract the common/neccessary functionality and use the abstraction on the api, but on the code have it call the system of the platform? Sorry if this doesn't help much, but I don't have time to keep up with all the progress

MrIvanPlays commented 1 year ago

Are these two systems too diferent from each other?

Yes. Completely.

Couldn't you just abstract the common/neccessary functionality and use the abstraction on the api, but on the code have it call the system of the platform?

No.

montlikadani commented 1 year ago

a solution i thought of is create our own chat api

The only solution this ^. We could make a simplifed and small Component/NameComponent for these prefix/suffix to retrieve and change things. Or just a plain String as the last resort.

MrIvanPlays commented 1 year ago

a solution i thought of is create our own chat api

The only solution this ^. We could make a simplifed and small Component/NameComponent for these prefix/suffix to retrieve and change things. Or just a plain String as the last resort.

I really don't want to think this is the only solution. An idea just passed through my mind and that is create a ComponentCreator that will spit out the component json, then consumers and implementors can shove that in bungee-chat or adventure and get a proper component.

MrIvanPlays commented 1 year ago

@Jikoo any thoughts/ideas?

Jikoo commented 1 year ago

I think we have to have a concept of groups and inheritance if we are going to allow write actions. What happens if I try to remove a node that is declared via a parent group? Is there an error? Does it succeed with no effect because the node is still declared via the parent? Is the parent group affected instead of the individual user? Should the permission implementation negate the node or explicitly declare it undefined for the user? In its current state, the API does not really define the actual behavior to expect, which to me means it isn't good.

As far as groups go, the only major downside I can think of is that they add implementation complexity. However, pretty much every established permissions plugin has a concept of groups because manually managing individual players' permissions would be an administrative nightmare. The other fiddly bit is that a group is effectively the same as a NodeHolder, it just happens to be an abstract construct used for the sake of holding multiple nodes, potentially in a hierarchy. I think the best solution to this would be to have an empty interface NodeGroup subclassing NodeHolder.

If Treasury wants groups, we need effective nodes (what we already have), explicit nodes, and inheritance (CompletableFuture<Collection<NodeGroup>> NodeHolder#getParents?). It would also be nice to have a way to fetch groups, although I'm not sure how full a system we want this to be. Do we want to then allow group creation and destruction? It makes sense if we're allowing general manipulation. There's a lot to consider.

MrIvanPlays commented 1 year ago

I think we have to have a concept of groups and inheritance if we are going to allow write actions. What happens if I try to remove a node that is declared via a parent group? Is there an error? Does it succeed with no effect because the node is still declared via the parent? Is the parent group affected instead of the individual user? Should the permission implementation negate the node or explicitly declare it undefined for the user? In its current state, the API does not really define the actual behavior to expect, which to me means it isn't good.

I'm gonna break down this into few smaller quotes so it is more clear to what I respond. The general response here is that this is still a work-in-progress, and documentation of which method does "x" and what method does "y" is not yet done, but you get the base representation of what these methods do by their names.

What happens if I try to remove a node that is declared via a parent group?

It shall be removed only for that node holder it has been removed from and it should stay in the parent one. E.g. group a has /treasury info, player b has group a as parent, removing node treasury.command.treasury.info from player b results in player b not having access to /treasury info, but all other players (player c and player d for example) that also inherit group a have access to /treasury info.

Is there an error? Does it succeed with no effect because the node is still declared via the parent? Is the parent group affected instead of the individual user?

No to all of these.

As far as groups go, the only major downside I can think of is that they add implementation complexity. However, pretty much every established permissions plugin has a concept of groups because manually managing individual players' permissions would be an administrative nightmare. The other fiddly bit is that a group is effectively the same as a NodeHolder, it just happens to be an abstract construct used for the sake of holding multiple nodes, potentially in a hierarchy. I think the best solution to this would be to have an empty interface NodeGroup subclassing NodeHolder.

Actually, they don't really add complexity. The name would be NodeHolderGroup instead because we already have NodeHolderPlayer. Consistency :) .

If Treasury wants groups, we need effective nodes (what we already have), explicit nodes, and inheritance (CompletableFuture<Collection<NodeGroup>> NodeHolder#getParents?). It would also be nice to have a way to fetch groups, although I'm not sure how full a system we want this to be. Do we want to then allow group creation and destruction? It makes sense if we're allowing general manipulation.

Nodes are nodes. Why shall we break them down? "explicit nodes, inheritance nodes" sounds like bullshit and bloat to me. re: accessing : Perhaps I can code something that's more like the AccountAccessor in the economy API.

There's a lot to consider.

Agreed.

Jikoo commented 1 year ago

What happens if I try to remove a node that is declared via a parent group?

It shall be removed only for that node holder it has been removed from and it should stay in the parent one. E.g. group a has /treasury info, player b has group a as parent, removing node treasury.command.treasury.info from player b results in player b not having access to /treasury info, but all other players (player c and player d for example) that also inherit group a have access to /treasury info.

Is there an error? Does it succeed with no effect because the node is still declared via the parent? Is the parent group affected instead of the individual user?

No to all of these.

Right, right, but the point is that all are interpretations that are "valid" to some degree. More to highlight the fact that the behavior is very ambiguous without a concept of inheritance. If I say "remove X permission from user" I would expect the end result to be that they no longer have the permission set. If I can't check where they're getting it from, I can't take any action to remove it correctly.

Nodes are nodes. Why shall we break them down? "explicit nodes, inheritance nodes" sounds like bullshit and bloat to me.

Explicit nodes would be nodes that are actually declared in a state for the holder. I do think it's important to give access to that information if we are going to permit manipulation of nodes' states. Not important for most cases, but anyone doing manipulation of nodes is likely going to care about the actual internal states. See the questions I posed for possible results. What if one of those results is actually what I want to have happen? How do I make sure that I've actually identified the true source of the node for removal/negation? Additionally, if we are not offering explicit states, permission nodes should not be a TriState at all. Undeclared states are good in the context of "is a permission explicitly set" but it's annoying in the context of "do I have permission to perform this action" because then I need to perform additional logic (presumably platform-specific, too) to figure out what undefined behavior is. The permission plugin (should have) baked this information already to speed up queries, the user manually recalculating it is either inaccurate or an expensive waste.

How do I determine a permission's state for a context? Ex.: remotebank.use is true globally. remotebank.use is false in the world "creative." How do I query that node? Do I fetch all nodes for that context and manually try to find it? That then loops back to me having to find out what undefined behavior is, but specifically for that context. It also seems inefficient when most plugins would bake states to be queriable by key, then context.

MrIvanPlays commented 1 year ago

you are free to push to this branch. I am unable to fully understand what you're trying to state but if you implement it then I will understand what you're talking about. I approve any changes, this is a WIP after all, we can always change stuff. my understanding of contexts and nodes have never been complete.

Jikoo commented 1 year ago

All right, will do. May end up just creating a separate branch because I imagine I'll end up changing a lot and I don't want to hold you back if it takes me a forever.

MrIvanPlays commented 1 year ago

Another idea for components: Have a ComponentAccessor service in the API that gives essential methods for creating components and outputs the raw JSON. That service will be implemented and maintained by the treasury plugin.

MrIvanPlays commented 1 year ago

Another idea for components: Have a ComponentAccessor service in the API that gives essential methods for creating components and outputs the raw JSON. That service will be implemented and maintained by the treasury plugin.

No responses here, any opinions on this?

lokka30 commented 1 year ago

So long it is very small and easy to maintain.

MrIvanPlays commented 1 year ago

@Jikoo when will you be able to implement your ideas here?

Jikoo commented 1 year ago

Probably not for at least a month or two based on how things are looking - my GPU is failing, which in turn makes pretty much everything quite frustrating. I was very spoiled by dual 1440p monitors, a single at 1024x768 is practically unusable.

Please don't hold back on my account.

MrIvanPlays commented 1 year ago

This is not gonna push back the 2.0.0 release anymore. We will release this when it's ready.

MrIvanPlays commented 1 year ago

@Jikoo please implement your ideas whenever you can

Jikoo commented 1 year ago

This is the next major item on my list, but my development time is currently being devoted to a plugin of mine that has apparently been partially broken since like 1.14 due to changes that weren't listed on the MC wiki (whoops).

MrIvanPlays commented 1 year ago

This is the next major item on my list, but my development time is currently being devoted to a plugin of mine that has apparently been partially broken since like 1.14 due to changes that weren't listed on the MC wiki (whoops).

okay, I just reworked nodes btw

MrIvanPlays commented 1 year ago

it looks like to me there was 0 time to work on this :( . Anyone still interested in giving his ideas into writing this API?