Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brigadier commands registered through Commands#getDispatcher require incorrect permission when executed by Bukkit#dispatchCommand #11378

Open
willkroboth opened this issue Sep 8, 2024 · 0 comments · May be fixed by #11385
Labels
status: needs triage type: bug Something doesn't work as it was intended to. version: 1.21.1

Comments

@willkroboth
Copy link
Contributor

willkroboth commented Sep 8, 2024

Expected behavior

When I execute my /test command as a player with no permissions, it runs successfully.

Observed/Actual behavior

When I execute my /test command using Bukkit#dispatchCommand, the command fails with the message I'm sorry, but you do not have permission to ...

image

Steps/models to reproduce

I created this plugin that registers two commands:

public final class BukkitTestPlugin extends JavaPlugin {
    @Override
    public void onEnable() {
        getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
            Commands commands = event.registrar();
            CommandDispatcher<CommandSourceStack> dispatcher = commands.getDispatcher();

            dispatcher.register(Commands.literal("test")
                .executes(context -> {
                    context.getSource().getSender().sendMessage("You ran the test command");

                    return 1;
                })
            );

            dispatcher.register(Commands.literal("run")
                .then(Commands.argument("command", StringArgumentType.greedyString())
                    .executes(context -> {
                        String command = context.getArgument("command", String.class);

                        Bukkit.dispatchCommand(context.getSource().getSender(), command);

                        return 1;
                    })
                )
            );
        });
    }
}

The /test command can be executed without any permissions. The /run command executes the given command string using Bukkit#dispatchCommand.

Plugin and Datapack List

> plugins
[13:44:11 INFO]: Server Plugins (1):
[13:44:11 INFO]: Bukkit Plugins:
[13:44:11 INFO]:  - BukkitTestPlugin
> datapack list
[13:44:20 INFO]: There are 3 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)], [paper (built-in)]
[13:44:20 INFO]: There are no more data packs available

Paper version

> version
[13:44:39 INFO]: Checking version, please wait...
[13:44:40 INFO]: This server is running Paper version 1.21.1-69-master@925c3b9 (2024-09-07T20:43:21Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)
You are running the latest version

Other

Server log: https://paste.gg/p/anonymous/19898849d44b4a5e884d147aeb2882ad

I'm looking into this problem to try to resolve JorelAli/CommandAPI#583. My analysis of the Paper code that causes that issue and the one here can be found in this comment on that issue. In summary, when Bukkit#dispatchCommand is run, the BukkitBrigForwardingMap added by #8235 converts the CommandNodes in the Brigadier dispatcher into Bukkit Commands. For vanilla commands, CommandAPI commands, and my /test and /run commands mentioned above, this constructor in VanillaCommandWrapper is used:

public class VanillaCommandWrapper extends BukkitCommand {
    public VanillaCommandWrapper(Commands dispatcher, CommandNode<CommandSourceStack> vanillaCommand) {
        super(vanillaCommand.getName(), "A Mojang provided command.", vanillaCommand.getUsageText(), Collections.EMPTY_LIST);
        this.vanillaCommand = vanillaCommand;
        // Incorrectly sets the permission to, for example, `minecraft.commands.test`
        this.setPermission(VanillaCommandWrapper.getPermission(vanillaCommand));
    }
}

This constructor calls setPermission. In the case of my /test command, this means the permission minecraft.commands.test is required to run it through Bukkit#dispatchCommand, rather than being null, or no permission required.

On Spigot, the CommandAPI fixes this by looping through the CommandMap and calling setPermission again to give CommandAPI commands the correct permission String. This works because Spigot only creates the VanillaCommandWrappers once, just before the server finishes start up. The same does not work on Paper, because a new VanillaCommandWrapper is created each time a vanilla node is retrieved from the BukkitBrigForwardingMap, so calls to setPermission do not persist to the next time the Command is retrieved and used.

@willkroboth willkroboth added status: needs triage type: bug Something doesn't work as it was intended to. labels Sep 8, 2024
willkroboth added a commit to willkroboth/Paper that referenced this issue Sep 9, 2024
Resolves PaperMC#11378 by "restoring" the Spigot behavior where VanillaCommandNodes are only created once. This allows command frameworks that insert CommandNodes directly into the Brigadier dispatcher to change the permission String of the VanillaCommandNodes created for their commands, rather than it always being the default `"minecraft.commands.<name>"`.

Previously, BukkitBrigForwardingMap would create a new VanillaCommandWrapper each time a CommandNode was requested via the Bukkit CommandMap. This means that calls to `Command#setPermission` would not persist between retrievals from the map.
@willkroboth willkroboth linked a pull request Sep 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs triage type: bug Something doesn't work as it was intended to. version: 1.21.1
Projects
Status: 🕑 Needs Triage
Development

Successfully merging a pull request may close this issue.

1 participant