JorelAli / CommandAPI

A Bukkit/Spigot API for the command UI introduced in Minecraft 1.13
https://commandapi.jorel.dev
MIT License
529 stars 66 forks source link

Commands in the CommandMap don't have the correct permission on Paper #578

Open willkroboth opened 1 month ago

willkroboth commented 1 month ago

CommandAPI version

9.5.1

Minecraft version

1.20.5

Are you shading the CommandAPI?

Yes

What I did

I added this code to my onEnable and started the server:

new CommandAPICommand("test")
        .withPermission("custom.permission")
        .executes(info -> {
            info.sender().sendMessage("Hello");
        })
        .register();

// Run later, since on Spigot commands are only moved once the server enables
//  On Paper you can access the command immediately
Bukkit.getScheduler().runTaskLater(
        this,
        // Spigot does not expose the CommandMap, though Paper does
        //  We can access it in both cases using the CommandAPI
        () -> getLogger().info(CommandAPIBukkit.get().getSimpleCommandMap().getCommand("test").getPermission()),
        0
);

What actually happened

On a Paper 1.20.6 or 1.21 server, the log message says that the command's permission is command.minecraft.test.

What should have happened

On a Paper 1.20.4 or Spigot server, the log message correctly reports that the permission is custom.permission.

Server logs and CommandAPI config

No response

Other

Note that the permission is correctly applied when running commands. This only affects the String returned when trying to see what the permission is via the CommandMap.

On Spigot, the CommandAPI has logic that makes sure the permissions of its commands in the CommandMap match up with the assigned Bukkit permissions, rather than keeping the default "vanilla-type" permission assigned when Bukkit moves commands from the Brigadier CommandDispatcher to its CommandMap:

https://github.com/JorelAli/CommandAPI/blob/d130bdee6647cb046fd326058b96cedfa56de59d/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/SpigotCommandRegistration.java#L154-L184

There is also logic when the CommandAPI registers commands after the server is enabled to correctly set the permission.

A similar thing does not immediately work on Paper. Paper's new Brigadier-centered command system allows accessing all registered CommandNodes as if they were Bukkit Commands through their BukkitBrigForwardingMap class:

// Paper-Server, package io.papermc.paper.command.brigadier.bukkit
public class BukkitBrigForwardingMap extends HashMap<String, Command> {
    @Override
    public Command get(Object key) {
        CommandNode<?> node = this.getDispatcher().getRoot().getChild((String) key);
        if (node == null) {
            return null;
        }

        if (node instanceof BukkitCommandNode bukkitCommandNode) {
            return bukkitCommandNode.getBukkitCommand();
        }

        return PaperBrigadier.wrapNode(node);
    }
}

This special map looks up the requested node in the Brigadier dispatcher and usually constructs a new Bukkit Command instance. Since it creates a new Command object each time, setting the permission of the returned object doesn't keep that change for future calls.

willkroboth commented 1 month ago

See https://github.com/PaperMC/Paper/issues/11092. It seems that Paper's stance is that the CommandMap is an old system that should be replaced and isn't necessarily going to be a reliable view of Brigadier commands. If Paper hard-forks from Spigot, it seems possible to me that they would eventually remove the CommandMap entirely. Perhaps the CommandAPI's RegisteredCommand system can be adapted to provide this functionality.