search

pmmp / PocketMine-MP

A server software for Minecraft: Bedrock Edition in PHP
https://pmmp.io
GNU Lesser General Public License v3.0
3.28k stars 1.55k forks source link

Testing for nonexisting permissions produces unexpected behaviour #2862

Open dktapps opened 5 years ago

dktapps commented 5 years ago

Description

This issue proposes that Permissible->hasPermission() and related methods should throw exceptions when used with nonexisting permissions.

Justification

Thanks to yet more stupidity inherited from Bukkit, testing for a permission that doesn't exist will silently yield whether the given Permissible has the Permission::$DEFAULT_PERMISSION. This is infuriating because a simple typo can therefore be difficult to find, and cause unexpected results.

This is particularly a problem because nonexisting child permission nodes will not respect overrides of their expected parents, without apparent reason.

For example, testing for the nonexisting permission pocketmine.command.giive will silently yield whether the player is an operator, irrespective of whether the pocketmine.command root node is set for that player.

Case in point: #2849

dktapps commented 5 years ago

For the sake of being able to statically analyse permission usage, it may also be desirable to have a generated registry interface like VanillaBlocks or similar. This would make it obvious to a static analyser when a non-existent permission is used.

dktapps commented 3 years ago

I today committed this, which replaces all hardcoded permission strings with statically analysable constants: https://github.com/pmmp/PocketMine-MP/commit/bf7d69b69edf17c51f87f9fdaae0f4323f6faa8c

I also did this, which eliminates the obscure behaviour of undefined permissions and makes them always return false: https://github.com/pmmp/PocketMine-MP/commit/6d8833ccd36ad3ddfc26bb78355905e5c2c64293

dktapps commented 1 year ago

Due to the performance impact of adding permission validation at every permission check, I think it's acceptable to leave the current behaviour (always returning false for invalid permissions) as is, and just make sure that we have validation at the point when the users of permissions (e.g. commands) are set up (already done), or when permissions are applied (not yet done, and potentially problematic).