Pass-through impermissible commands.

[CoreyShupe]

The Problem Currently if you have a setup such as:

proxy A declares command `/hello <string list>` under permission `example`
server A declares command `/hello [<literal x>] <string list>` under no permission

player without permission `example` joins proxy A and gets forwarded to server A

Expected:
the tab completion for `/hello` along with command execution should forward to the backend server

Actual:
Parsing fails at the location of `hello <==` and it refuses to pass anything including command execution along.

Soltuion: We should be checking permissibility before running the dispatcher parse/executor - if we know the user doesn't have permission we can simply pass through the request. This also goes for command result executors and parsers.

[hugmanrique]

No, we document (and API users expect) that all the children of an alias node registered by the proxy must come from the proxy, see https://github.com/PaperMC/Velocity/blob/fee292bcc9b88638af94151286877296c1948f56/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java#L61. Thus, the backend server's nodes are discarded when constructing the command graph.

This change opens up lots of edge cases around redirects (I can elaborate in a following comment if you want) and does something (constructing context objects for permission checking) I specifically discouraged in https://github.com/PaperMC/Velocity/blob/fee292bcc9b88638af94151286877296c1948f56/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java#L107. This is for various reasons: Brigadier should be the only one creating these objects (during parsing), since it is the only one in charge of handling them. Maintaining the reader state along with the parsed node list is quite complicated. Furthermore, the current code doesn't check the context-aware permission predicate, used by the InvocableCommand implementations and API users that add their own BrigadierCommands (i.e. this breaks API). Even if the current logic is correct, constructing the objects needed for the context-aware predicate is a greater hack and requires "inventing" a command execution by the player (I know because I've tried :P). Lastly, one of the main goals of the command system overhaul in the Velocity 1.1->2.0 transition was to avoid duplicate permission checking calls to plugins. This re-introduces the problem.

Some notes on the implementation, even if I disagree overall: these changes should go in CommandGraphInjector to avoid adding public API (not needed) and to leave the packet-handling code untouched (avoiding further entangling the networking and command-related portions of Velocity). Minor change, the methods in CommandManager need to lock to ensure thread-safety. Also, tests are needed for each command subinterface.

Ultimately proxy commands have their own unique aliases, and must not be shared with the backend server's aliases. If the same alias absolutely needs to be shared, the proxy plugin can register all involved nodes and forward execution to the backend servers by returning BrigadierCommand.FORWARD from its Command. This behavior is both supported by BrigadierCommands, and invocable commands (via the hinting system).

A similar issue was discussed in #555.

The command system is somewhat involved, and these changes need to be discussed before code gets written. I'm sorry for the wasted time, but I hope the explanation above serves as an introduction to further improve this system safely.

[CoreyShupe]

Ultimately proxy commands have their own unique aliases, and must not be shared with the backend server's aliases. If the same alias absolutely needs to be shared, the proxy plugin can register all involved nodes and forward execution to the backend servers by returning BrigadierCommand.FORWARD from its Command. This behavior is both supported by BrigadierCommands, and invocable commands (via the hinting system).

Not wasted time, we'll be running a version of this PR in production in my server so it's not necessarily something I'm worried about getting out fast.

We can discuss this thoroughly before it gets placed in the upstream though. I'm happy to do that.

I'd like this to be the PR where it's solved though - it's not proper behavior even if it's intended.