Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Cache Bukkit Command when wrapping CommandNodes

Open willkroboth opened this issue 1 year ago • 2 comments

Resolves https://github.com/PaperMC/Paper/issues/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>".

For example, the test plugin described in https://github.com/PaperMC/Paper/issues/11378 can do something like this now:

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")...);

            dispatcher.register(Commands.literal("run")...);

            CommandMap commandMap = Bukkit.getCommandMap();
            commandMap.getCommand("test").setPermission(null);
            commandMap.getCommand("run").setPermission(null);
        });
    }
}

When the /test command is ran using Bukkit#dispatchCommand, Bukkit will now see that its permission is null and allow a Player without any permission to run it, which is the case when the same Player runs the command directly.

Without this PR, BukkitBrigForwardingMap creates a new VanillaCommandWrapper each time a CommandNode is requested via the Bukkit CommandMap. This meant that calls to Command#setPermission did not persist between retrievals from the map.

I can squash this into the Brigadier-based-command-API patch file if desired, but I figured it would be easier initially to review the changes when they are in a separate patch.

willkroboth avatar Sep 09 '24 23:09 willkroboth

It's not possible to properly represent a tree of requirements as a permission string. To solve the linked issue properly we need to reimplement dispatchCommand without using the legacy command map. We could also set the permission to null for plugin commands and override hasPermission(Silent) to improve the legacy representation.

jpenilla avatar Sep 10 '24 00:09 jpenilla

To solve the linked issue properly we need to reimplement dispatchCommand without using the legacy command map.

Fair point. I imagine Paper will eventually remove the CommandMap after hard forking from Spigot, so it would be useful to get started on that by changing the command dispatch and suggestions logic available in the API to use Brigadier directly.

We could also set the permission to null for plugin commands and override hasPermission(Silent) to improve the legacy representation.

Note that this is indeed the case for commands registered using Commands#register. For those Brigadier commands which are associated with a specific PluginMeta, Paper uses the PluginCommandNode extends LiteralCommandNode class as the root node for the command. When PaperBrigadier#wrapNode is given one of these CommandNode instances, it returns a PluginVanillaCommandWrapper extends VanillaCommandWrapper, which by default has its permission set to null. That means Bukkit#dispatchCommand always skips its permissions check, which allows the CommandNodes to evaluate their requirements themselves.

#11378 only applies to commands registered through Commands.getDispatcher()#register, which places a LiteralCommandNode directly into the dispatcher. These are treated like the default vanilla commands, and so their VanillaCommandWrappers are given the "minecraft.commands.<name>" permission String, causing #11378. This aligns with Spigot's behavior if one uses NMS and reflection to place a CommandNode directly into the Brigadier dispatcher, which is something Brigadier command frameworks might do before Paper's API in #8235. However, the difference between Spigot and Paper that this PR "resolves" is that on Paper, calling Command#setPermission on a VanillaCommandWrapper retrieved from the CommandMap does not persist since Paper creates a new VanillaCommandWrapper each time.

While not a requirement of the Map interface, it is also neat when map.get(a) == map.get(a) for all a, which isn't necessarily true before this PR.

willkroboth avatar Sep 10 '24 12:09 willkroboth