Allow for the preventing of non player sources from running luckperms commands#2633
Allow for the preventing of non player sources from running luckperms commands#2633Moose1301 wants to merge 1 commit into
Conversation
lucko
left a comment
lucko
left a comment
There was a problem hiding this comment.
Thanks, I really appreciate the PR.
However, there are lots of codestyle issues, random added/deleted lines in files unaffected by the change, and a concern about behaviour relating to Sender#isValid.
In future, please try to review the "files changed" tab to make sure your changes are correct, and nothing extra has creeped it's way in. I appreciate that we don't have a codestyle guide for contributions, but it should be easy enough to just match the format found in the file being edited!
| return; | ||
| } | ||
|
|
||
| if(this.plugin.getConfiguration().get(ConfigKeys.BLOCK_NON_PLAYER_SOURCES)) { |
There was a problem hiding this comment.
Codestyle:
| if(this.plugin.getConfiguration().get(ConfigKeys.BLOCK_NON_PLAYER_SOURCES)) { | |
| if (this.plugin.getConfiguration().get(ConfigKeys.BLOCK_NON_PLAYER_SOURCES)) { |
|
|
||
| #Stops Luckperms Commands from being run from non player sources. | ||
| #eg: Stops a mod from running /lp user Example permission set * | ||
| block-non-player-sources: false |
There was a problem hiding this comment.
Again - please match the style found in the rest of the file.
# Stops LuckPerms commands from being.... - space after the #, proper capitalisation.....
Should also be a new-line between the setting and the start of the next comment.
| applyConvenienceAliases(arguments, true); | ||
|
|
||
| if(this.plugin.getConfiguration().get(ConfigKeys.BLOCK_NON_PLAYER_SOURCES)) { | ||
| if(sender.isConsole() || sender.isValid()) { |
There was a problem hiding this comment.
Codestyle - should be a space after if.
Also, sender.isValid() will return true in most cases - it returns false if the player has since logged out and their player instance has been GCed.
|
Ok, I fixed the issues you said |
|
There are still a number of codestyle issues which have not been resolved.
I had a go at rebasing your PR and fixing up the issues myself. After having a closer look and think about what's going on here, I'm concerned that we're implementing a "kind of" security measure, which I am not sure can necessarily be trusted. It's possible for mods to run commands as another player, or use a custom command source, which would entirely bypass these checks. I draw your attention to the discussion / my commands on this issue: #2219 I think on this basis, it would be best to not merge these changes. Sorry to not be clear from the start, my bad. Thank you for your contribution nonetheless. |
2 participants
Adds #2551