Incendo / cloud

Command framework & dispatcher for the JVM
https://cloud.incendo.org
MIT License
437 stars 54 forks source link

NoPermissionException fires with wrong permissions #365

Open SirSalad opened 2 years ago

SirSalad commented 2 years ago

When the command sender has no permissions relevant to the command manager, and attempts to use a command, NoPermissionException will have all permissions rather than the appropriate one.

Versions: cloud-paper 1.6.2 and 1.7.0-SNAPSHOT (bcc9d30)

Code:

public class MyPlugin extends JavaPlugin {
    @Override
    public void onEnable() {
        try {
            var commandManager = PaperCommandManager.createNative(this, CommandExecutionCoordinator.simpleCoordinator());

            commandManager.registerExceptionHandler(NoPermissionException.class, (ignored, exception) ->
                    getLogger().info(exception::getMessage)
            );

            commandManager.command(commandManager.commandBuilder("command1")
                    .permission("permission1")
                    .handler(context -> context.getSender().sendMessage("Handling command1"))
            );

            commandManager.command(commandManager.commandBuilder("command2")
                    .permission("permission2")
                    .handler(context -> context.getSender().sendMessage("Handling command2"))
            );
        } catch (Exception ignored) {}
    }
}

Output:

[11:09:44 INFO]: SirSalad issued server command: /command1
[11:09:44 INFO]: [MyPlugin] Missing permission '(permission2)|(permission1)' <-- should only have permission1
[11:09:47 INFO]: SirSalad issued server command: /command2
[11:09:47 INFO]: [MyPlugin] Missing permission '(permission2)|(permission1)' <-- should only have permission2

[11:09:58 INFO]: SirSalad issued server command: /lp user SirSalad permission set permission1 true
[11:10:03 INFO]: SirSalad issued server command: /command1
[11:10:05 INFO]: SirSalad issued server command: /command2
[11:10:05 INFO]: [MyPlugin] Missing permission 'permission2' <-- this is correct
Citymonstret commented 2 years ago

It's not really wrong per se. When building the command tree, the root note will end up with an OR-permission that is built up by the command permissions. It's failing on this OR-permission because the sender lacks permission to use any of the commands.

Because of the way parsing works, we never even attempts to traverse the tree further if we fail that early, thus we don't really have information about which command that was actually going to be executed, had the permission checks passed. Therefore I am not really sure if there's a good way to "solve" this.