search

RypoFalem / ArmorStandEditor

Bukkit plugin to allow players to edit armorstands without commands.
GNU General Public License v2.0
19 stars 68 forks source link

Don't give any player access by default. #2

Closed Janmm14 closed 8 years ago

Janmm14 commented 8 years ago

This caused issues with the children permissions as we could not deny the permissions properly.

Also its unlogical if you give every permission default: true and the all permission default:op

Alos adds a permisison check for the inventory opening.

RypoFalem commented 8 years ago

The reason players have access by default is that is the way the plugin is intended to be used: to give non-OP players the ability to be able to edit armor stands. I believe this will be the most common way servers will use this plugin so that is why it grants the available permissions to all users by default: so it works for most people "out of the box". Some server admins won't want that and they are free to flip the nodes to false so that it will fit the server's needs.

The reason the .all permission is limited to OPs rather than default is 1: so admins can easily remove an unwanted permission from the general populous (like if they don't want to risk invisible armorstands everywhere) and 2: I plan on possibly adding permissions in the future that might be best left off by default (such as the ability to toggle the "Marker" entity data, which would make the armorstand effectively invincible and impossible to target with the editing tool due to extremely small hitbox).

Just because it's not what you want for your server doesn't make it "unlogical".

I'm interested in hearing about your issues with not being able to deny child permissions. I need more info to understand the problem. Show me which permissions you use exactly? If I had to guess without looking at your setup, you probably didn't disable the child AND parent permissions. If you want to disable only the left arm, "positionarms" should be set to false, "leftarm" should be set to false and "rightarm" should be set to true. Remember that positionarms is a convenience permission to grant access to both arms.

I like the idea of denying opening the menu to players without the .basic permission. However if that is implemented, it should be accompanied by an error message stating they lack asedit.basic, rather than silently doing nothing and leaving the player wondering why "it won't work". In anycase, if you submit a pull request with only that small change change (the three lines of code you have now will be fine, I can add the error messages myself), I will be happy to merge it.

Janmm14 commented 8 years ago

I'm sorry for overreacting a bit here and there.

I like the idea of denying opening the menu to players without the .basic permission. However if that is implemented, it should be accompanied by an error message stating they lack asedit.basic, rather than silently doing nothing and leaving the player wondering why "it won't work".

We had the issue that the players were able to edit anything with the flint and the gui even through we denied all permissions (child and parent) with pex; not only -asedit.*, but also -asedit.<insert perm here>

In case "normal" players do not have the permission (and should not have it), it might be a good idea to not send any message as it might get annoying if they click with flint anywhere.

As you like to have the permissions on by default, I think ther will not be anyone wondering why it does not work.

To the marker thing: I suggest you then add the ability to target an armorstand another way, possibly with a radius search command and a select command with the entity id.

If there are permission checks using the gui, we had issues disabling those permissions and its likely the child-parent setup in your plugin.yml

If there are no permission checks using the gui, it might be the time to add them, regardless of gui or command usage.

In anycase, if you submit a pull request with only that small change change (the three lines of code you have now will be fine, I can add the error messages myself), I will be happy to merge it.

Just cherry pick the commit https://github.com/minotopiame/ArmorStandEditor/commit/f59224453519cd0d517a4246b312fb329155a7f5; you're allowed to do so.

RypoFalem commented 8 years ago

I'm not sure what issues you are having with permissions. In my tests they work as intended. Setting everything to "false" and then opening the chest GUI, I only sees a blank inventory. It could be something with your PEX setup. Keep in mind that "false" is effectively "This node does not grant permission, but you might get the same permission from somewhere else"; you only need one "true" to apply to a user no matter how many "falses" there are. That setting the defaults to OP fixes the issue for you means it is probably something to do with PEX not properly changing the default to false.

The permission check happens in 2 places and they both use the same permission nodes: the GUI (to show/hide the icon, just so the user only sees what is available) and via commands (clicking the icon in the chest GUI actually makes the user perform a command so it blocks commands entered via the GUI as well, even though this technically should never happen because of the first check).

The error message I spoke of would only happen when you right clicked something that isn't an armorstand. Since most players don't go around right clicking things with flint, I don't expect excessive error messages to be an issue. If flint is used for something else on your server, you can change the editing tool in the plugin's config.yml.

Targeting armorstands is already on the todo list for that a few reasons.