Add Forge permission handler #1660

kashike · 2017-07-18T19:48:27Z

kashike · 2017-07-18T19:53:28Z

@luacs1998: Could I get your input here? You're familiar with the Forge permission API.

yuuka-miya · 2017-07-22T16:08:52Z

src/main/java/org/spongepowered/mod/command/SpongePermissionHandler.java

+
+    @Override
+    public void registerNode(final String node, final DefaultPermissionLevel level, final String desc) {
+        // TODO


Does Sponge have the concept of registering permissions?

Still trying to acquaint myself with the Sponge PermissionService, so...

We have the concept of creating descriptions which requires the mod/plugin, the permission node, and a description.

To add on to this, descriptions can designate generic like permissions with placeholders for variable permissions. e.g. nucleus.teleport.world. to have per dimension permissions.

yuuka-miya · 2017-07-22T16:10:33Z

Looks okay to me, but if you want a second opinion - @LatvianModder?

kashike · 2017-07-22T16:11:47Z

src/main/java/org/spongepowered/mod/command/SpongePermissionHandler.java

+            return false;
+        }
+
+        // TODO contexts?


How should we handle this? Sponge has a Context concept as well, not sure if we want to convert IContext data to a Context?

kashike · 2017-07-22T16:12:28Z

src/main/java/org/spongepowered/mod/command/SpongePermissionHandler.java

+
+    @Override
+    public Collection<String> getRegisteredNodes() {
+        return Collections.emptySet(); // TODO


Should we keep track of this, just for Forge mods wishing to get this?

LatvianModder · 2017-07-22T17:03:40Z

I guess this works. I wouldnt use that many lambdas in getNodeDescription, but that isn't used frequently anyway, so I guess its fine. Im ok, as long as it properly handles registered permissions. Only not sure why is that try{}catch{} required, just register your handler in PREINIT, and it will be fine, because mods are expected to register nodes in INIT+

Zidane · 2017-07-22T17:16:21Z

src/main/java/org/spongepowered/mod/command/SpongePermissionHandler.java

+        try {
+            PermissionAPI.setPermissionHandler(this);
+        } catch (IllegalStateException ignored) {
+            RegistryHelper.setFinalStatic(PermissionAPI.class, "permissionHandler", this);


This is bad, if setting the permission handler fails then log it to the console as that likely means someone else is doing so. Let the server admin figure out how to proceed but don't blindly blow away another mod whose purpose is to do permissions.

No, I actually agree with Sponge being the strongest permission handler (Config option for this would be nice tho), but I dont agree with aggressively changing the private field. You only should set permission handler in PREINIT state, as I said earlier. Hopefully this is changed

@luacs1998 also suggested that we be the strongest. There's the possibility that another mod will replace our handler, so my thought was to do it after pre-init. I can add a config option to disable our replacing of other permission handlers if one is registered before Sponge.

src/main/java/org/spongepowered/mod/service/permission/SpongePermissionHandler.java

phit · 2018-02-10T17:59:00Z

@kashike status of this?

BrainStone · 2018-10-06T16:28:55Z

Yes, what’s the status on this? We still need a plugin for this.

BrainStone · 2019-02-15T19:44:05Z

Bump...

tonnic · 2019-02-15T20:10:43Z

please add this....we need this implemented

maxanier · 2019-06-07T22:40:09Z

What exactly has to be tested?
I could try to verify the general functionality by adding some Forge permission checks to a mod of mine. Then build this branch, setup a server with it, my mod and LuckPerms and check if LuckPerms can access the mod's permissions?

gabizou · 2019-06-07T23:23:11Z

I could try to verify the general functionality by adding some Forge permission checks to a mod of mine. Then build this branch, setup a server with it, my mod and LuckPerms and check if LuckPerms can access the mod's permissions?

Correct

maxanier · 2019-06-08T10:55:49Z

So, I did that and (after I found out I have to enable this in the Sponge configuration) it works fine. I can give/deny the permissions registered by my mod using LuckPerms and they are correctly identified.

Only issue is that, the default permission level passed to the registerNode method is ignored and instead the default permission level is undefined (which results in deny).
Most, if not all, permissions introduced by my mod should be available to all players. So one would have to manually grant "vampirism.*" to every player/group. Before they are actually able to play the mod with this enabled.

However, since this has to be manually enabled anyway, I guess that is ok.
I am very happy there is finally a way to integrate mods with permission plugins.

phit · 2019-06-08T15:26:11Z

you could already do so https://docs.spongepowered.org/stable/en/plugin/permissions.html#for-forge-mods you don't need this PR for that

LatvianModder · 2019-06-09T15:14:58Z

If I had known about this way before the Forge PermissionAPI PR was merged, I would have simply just added a hook in checkPermission() method for forge mods, which would make things 10000x easier. That's why I suggest to try this approach in 1.13/1.14+ and basically just not care about it in 1.12. Mods would most likely not care about it this late anyway, but if new PR was made for 1.13, maybe then things can change

BrainStone · 2019-06-09T21:52:09Z

The Forge Permission API is pretty well used already on 1.12.2. So it would make sense to already implement it in API 7, so people don't have to use an unreleased version of an abandonned plugin that bridges the gap.
Keep in mind that 1.12.2 will be a long term version for modded MC.

BrainStone · 2019-06-30T12:09:40Z

@gabizou what's the holdup? This PR is a strict improvement of the project, has been confirmed to work and been reviewed by several people.

Zidane · 2019-06-30T14:41:32Z

@phit has objections to merging this and can elaborate why.

BrainStone · 2019-07-29T18:27:07Z

Sure. Let's keep it rotting until it can't be merged anymore.

pie-flavor · 2019-07-29T20:50:01Z

@BrainStone oy. No need to be rude.

Adds #1049

ImMorpheus

Tested with FTB Utilities.
Tried to change ftbutilities.chat.nickname.set with Luckperms, everything is working as intended.

kashike added status: wip system: command type: enhancement labels Jul 18, 2017

kashike self-assigned this Jul 18, 2017

yuuka-miya reviewed Jul 22, 2017

View reviewed changes

kashike commented Jul 22, 2017

View reviewed changes

Zidane reviewed Jul 22, 2017

View reviewed changes

kashike force-pushed the forge/permission branch 2 times, most recently from 7367b6b to aaf4b9c Compare July 23, 2017 10:09

kashike mentioned this pull request Jul 23, 2017

Add configuration option to force the adoption of the Sponge permission handler SpongePowered/Sponge#1461

Merged

kashike added system: permission and removed system: command labels Jul 23, 2017

Xfel reviewed Aug 2, 2017

View reviewed changes

src/main/java/org/spongepowered/mod/service/permission/SpongePermissionHandler.java Outdated Show resolved Hide resolved

kashike force-pushed the forge/permission branch from aaf4b9c to 48daccb Compare October 2, 2017 00:54

lucko mentioned this pull request Oct 4, 2017

Dynmap permissions lookup fail? LuckPerms/LuckPerms#494

Closed

lucko mentioned this pull request Oct 6, 2018

No permission to use Commands from Mods LuckPerms/LuckPerms#1254

Closed

ImMorpheus added status: needs review status: needs testing and removed status: wip labels Feb 15, 2019

ImMorpheus requested a review from lucko February 15, 2019 22:00

phit force-pushed the forge/permission branch from 48daccb to 7fe7a62 Compare April 19, 2019 00:29

kashike requested a review from Faithcaio as a code owner April 19, 2019 00:29

kashike requested review from gabizou and stephan-gh as code owners April 19, 2019 00:29

phit changed the base branch from bleeding to stable-7 April 19, 2019 00:30

phit added resolution: wishful thinking and removed status: needs review status: needs testing labels May 28, 2019

Zidane mentioned this pull request Jun 6, 2019

Forge permissions API #1049

Closed

ImMorpheus force-pushed the forge/permission branch from 5f57abf to dae5743 Compare July 29, 2019 20:35

ImMorpheus force-pushed the forge/permission branch from dae5743 to 90066af Compare July 29, 2019 21:24

kashike and others added 4 commits July 29, 2019 23:25

Add Forge permission handler

bd4cb94

Adds #1049

Finish Implementation, needs testing

ee6ad5e

Fix compile issues

90066af

Bump SpongeCommon for permission handler

8b1888d

ImMorpheus approved these changes Jul 29, 2019

View reviewed changes

ImMorpheus merged commit 65f05e0 into stable-7 Jul 29, 2019

maxanier mentioned this pull request Feb 17, 2020

Permission API TeamLapen/Vampirism#207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Forge permission handler #1660

Add Forge permission handler #1660

kashike commented Jul 18, 2017 •

edited by ryantheleach

kashike commented Jul 18, 2017

yuuka-miya Jul 22, 2017

kashike Jul 22, 2017

ryantheleach Jul 22, 2017

yuuka-miya commented Jul 22, 2017

kashike Jul 22, 2017

kashike Jul 22, 2017

LatvianModder commented Jul 22, 2017

Zidane Jul 22, 2017

LatvianModder Jul 22, 2017

kashike Jul 22, 2017

phit commented Feb 10, 2018

BrainStone commented Oct 6, 2018

BrainStone commented Feb 15, 2019

tonnic commented Feb 15, 2019

maxanier commented Jun 7, 2019

gabizou commented Jun 7, 2019

maxanier commented Jun 8, 2019

phit commented Jun 8, 2019

LatvianModder commented Jun 9, 2019

BrainStone commented Jun 9, 2019 •

edited

BrainStone commented Jun 30, 2019

Zidane commented Jun 30, 2019

BrainStone commented Jul 29, 2019

pie-flavor commented Jul 29, 2019

ImMorpheus left a comment

Add Forge permission handler #1660

Add Forge permission handler #1660

Conversation

kashike commented Jul 18, 2017 • edited by ryantheleach

kashike commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuuka-miya commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LatvianModder commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phit commented Feb 10, 2018

BrainStone commented Oct 6, 2018

BrainStone commented Feb 15, 2019

tonnic commented Feb 15, 2019

maxanier commented Jun 7, 2019

gabizou commented Jun 7, 2019

maxanier commented Jun 8, 2019

phit commented Jun 8, 2019

LatvianModder commented Jun 9, 2019

BrainStone commented Jun 9, 2019 • edited

BrainStone commented Jun 30, 2019

Zidane commented Jun 30, 2019

BrainStone commented Jul 29, 2019

pie-flavor commented Jul 29, 2019

ImMorpheus left a comment

Choose a reason for hiding this comment

kashike commented Jul 18, 2017 •

edited by ryantheleach

BrainStone commented Jun 9, 2019 •

edited