Allow addons to enable SilentContainer without permission #359
Conversation
There was a problem hiding this comment.
Not exactly true - the permission check guards against permission changes after enabling the toggles. OI did drop the behavior of toggles persisting across restarts, but it does offer an addon that re-enables the behavior.
I don't mind this being changed (a lot of time permission changes require the player to rejoin anyway, and I would hope that demotions are rare), but we should be consistent and allow AnyContainer as well, plus modify the toggle API and existing addon to not persist toggles on join for players lacking permission.
/e: You could also just add a temp PermissionAttachment to the user while your plugin's mode is active, removing the need for the change and keeping the previous behavior.
Jikoo
changed the title
|
You’re absolutely right - I had forgotten about permission removal, probably because I’m used to the fact that users are generally kicked/banned before or after that anyway. That said, I was thinking that another option could be to modify the To avoid issues with plugins that already use the API, if the cause isn’t specified, the I’m not sure whether this is a good idea or not, so I’m looking forward to your feedback. |
|
If we're only going for 2 states, we may as well stick with a boolean, something like applyPerms or bypassPerms. I think the one big issue is that if we're taking an approach that doesn't modify existing behavior, the plugin you're talking about will still have issues and need modification, at which point they could add conditional permissions as a feature of the silent mode rather than modify OI at all. Perhaps instead we should consider a more granular permissions approach? That way you could grant your users access to the required node without giving them the ability to directly toggle it via OI's commands. /e: What I'm thinking right now is a subnode per container feature, |
|
Sounds good. I’ve made a commit implementing this approach and everything seems to be working fine. |
Jikoo
left a comment
Jikoo
left a comment
There was a problem hiding this comment.
Overall it looks good. I'll probably end up dropping the .toggle nodes in favor of retaining the existing nodes - it doesn't seem like they are intended to add anything in your implementation, and I really don't want to create situations where one can have access to the toggles but not have the associated behavior actually work.
As far as the actual implementation goes, just the one point about the new feedback not being able to be hit, so we don't need it or the associated translation strings.
| boolean hasTogglePermission; | ||
| if (toggle == PlayerToggles.any()) { | ||
| hasTogglePermission = Permissions.CONTAINER_ANY_TOGGLE.hasPermission(player, Permissions.CONTAINER_ANY); | ||
| } else if (toggle == PlayerToggles.silent()) { | ||
| hasTogglePermission = Permissions.CONTAINER_SILENT_TOGGLE.hasPermission(player, Permissions.CONTAINER_SILENT); | ||
| } else { | ||
| hasTogglePermission = command.testPermissionSilent(player); | ||
| } | ||
|
|
||
| if (!hasTogglePermission) { | ||
| lang.sendMessage(sender, "messages.error.permissionToggle"); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
This section won't ever provide feedback because the user has to have permission to run the command. This is a TabExecutor rather than an injected Command implementation (perhaps my bad with the class naming causing confusion here), so the Bukkit Command implementation wrapping it has already checked permissions before we get control.
|
Done! I removed the toggle as you asked me, as well as the unnecessary feedback. |
|
Ah, sounds like the permission isn't checked when the user closes the chest. That should be an easy fix, but now I'm considering if moving the permission checks inside the toggle itself might be better design. I'll play around with that idea a bit I guess. /e: Ah, I suppose by design the better idea isn't possible, because I made the toggles accept a UUID. Perhaps a new helper method is in order there. |
| if (permissible.hasPermission(permission)) return true; | ||
| if (permissible.isPermissionSet(permission) && !permissible.hasPermission(permission)) return false; | ||
| return permissible.hasPermission(parent.permission); | ||
| } |
There was a problem hiding this comment.
Oh, I didn't actually notice this at the time, but this doesn't quite do what I had intended here. This effectively does nothing (or wastes CPU time) because of how Bukkit's permissions system works and how the permissions are laid out in our plugin.yml.
If the user has the permission, it might be because it is granted by the parent node or the node directly, but Bukkit calculates it out either way. If they don't have the node, either it is denied or it is not set and the parent is denied/not set as well. In those cases this just adds 2 extra permissions checks that are guaranteed to fail.
My goal was a simple is set or parent is set, because I don't want to allow people to create permissions setups where a player can access the toggle but not access the feature.
There was a problem hiding this comment.
Oh, that's true, you're right. I had completely forgotten about that. I'm really sorry for the inconvenience
2 participants
This fix allows third-party plugins to temporarily enable a user to open containers silently using OpenInv API.
The permission check is unnecessary because silent mode is disabled by default, and enabling it via command requires a permission.
An example:
I want staff members to be able to open containers silently only while in vanish, so via the API I add the staff member to the list as soon as they enter vanish.
Previously, with the permission check in place, the shulker box would bug out: the animation would start and, once closed, it would remain open.
Now, with this change, the staff member can open the shulker silently while in vanish and, once they exit vanish mode, they will no longer be able to do so.