SimpleCommandMap: Enforce command permissions by default

[dktapps]

trafficstars

Introduction

this resolves many security issues, as well as removing a ton of boilerplate code. It may be desirable to react to permission denied; this can be done by overriding Command->testPermission(), or by using setPermissionMessage() to set a custom permission denied message.

Note that it's still currently necessary to use testPermission() manually in some cases, such as in the case of /whitelist, where a union of permissions is used for the initial gate, but specific permissions are required for each subcommand.

Relevant issues

Changes

Behavioural changes

Command permissions are now checked prior to Command->execute(), assuming that a permission was set.

Note: This will only take effect when a permission has been set using setPermission(). If no permission has been set, the behaviour will remain as before.

Backwards compatibility

This is backwards compatible, but has some potential to cause behavioural changes in plugins due to usage of setPermission() in combination with non-standard methods of checking permissions.

Follow-up

In PM5, we probably ought to mandate a permission be set for all commands. Lack of a permission is dangerous and usually a sign of a bug, and it's easy to create a permission which is granted to everyone anyway.

[SOF3]

This PR is behaviourally BC breaking if a plugin intentionally doesn't call testPermission normally, e.g. for the following use cases:

• The plugin wants to implement custom permission failure handlers, e.g. report the event to console
• The plugin only uses testPermissionSilent/setPermission to hide the command from /help, but does not intend them to be checked during commands execution
• The plugin simply forgot to check, but server owners rely on this behavior and don't want the plugin to suddenly start checking permissions

[dktapps]

I don't think "forgot to check" counts as "intentionally". The rest of the points I acknowledge.

[dktapps]

To be clear, I'm unconcerned about breaking broken use cases; forgetting to check a permission is highly likely to be a security vulnerability and server owners can easily grant appropriate permissions using permission plugins.

I'm doubtful that hiding commands that remain executable is a use case that makes sense. This community typically abuses PlayerCommandPreProcessEvent for that purpose anyway.

Failure handlers can be implemented by overriding testPermission(), which remains unchanged. However, I concede that this does break existing cases which didn't use testPermission() (which is most of them).

[dktapps]

Retargeting this to next-major out of an abundance of caution.