Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permissions support #560

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Permissions support #560

wants to merge 21 commits into from

Conversation

zp3x
Copy link
Contributor

@zp3x zp3x commented Feb 18, 2025

Description

Adds support for traditional permissions.

"example.command"

For larger servers, a robust permission system is essential, ideally one that supports existing solutions already using the traditional permissions format.

Changes

  • Allows setting, removing, and retrieving player permissions.
  • Validates permissions upon command execution.

Testing

Please follow our Coding Guidelines

@zp3x
Copy link
Contributor Author

zp3x commented Feb 18, 2025

Just found out that when no commands are registered the Server Crashes on /help

Messed the permission Level and Operator Status on /help.

Fixing that

@vyPal
Copy link
Contributor

vyPal commented Feb 18, 2025

One thing I'd maybe consider implementing is namespaces for these permissions. Specifically for use with plugins so that there aren't any collisions. Each plugin has a plugin name, which has to conform to the Cargo naming system.
I'd recommend using this as either the base node of the permission tree for each plugin (so permissions registered by each plugin would start with plugin_name.some.permission) or as a standard namespace (so each permission registered by a plugin would look more like plugin_name:some.permission)

In this way we can prevent conflicts in permissions (like if two plugins both wanted to register a user.teleport permission and use it for different things)

1 similar comment
@vyPal
Copy link
Contributor

vyPal commented Feb 18, 2025

One thing I'd maybe consider implementing is namespaces for these permissions. Specifically for use with plugins so that there aren't any collisions. Each plugin has a plugin name, which has to conform to the Cargo naming system.
I'd recommend using this as either the base node of the permission tree for each plugin (so permissions registered by each plugin would start with plugin_name.some.permission) or as a standard namespace (so each permission registered by a plugin would look more like plugin_name:some.permission)

In this way we can prevent conflicts in permissions (like if two plugins both wanted to register a user.teleport permission and use it for different things)

@zp3x
Copy link
Contributor Author

zp3x commented Feb 18, 2025

That's actually a very good idea. Will definitely implement and leave a option to enable it

Jakob and others added 3 commits February 18, 2025 14:37
Server does not crash when no commands are registered.
Tab completions do not leak commands that the player is not authorized to
Copy link
Contributor

@DataM0del DataM0del left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_dispatcher's code, is... 🤮

@@ -165,13 +163,32 @@ impl CommandDispatcher {

let Some(permission) = self.permissions.get(key) else {
return Err(GeneralCommandIssue(
"Permission for Command not found".to_string(),
"Permission for Command not found 0".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Permission for Command not found 0".to_string(),
"Permission for command {key} not found".to_string(),

..?

Comment on lines +129 to +292
"pumpkin.playsound",
PermissionLvl::Two,
);
dispatcher.register(
title::init_command_tree(),
"pumpkin.title",
PermissionLvl::Two,
);
dispatcher.register(
summon::init_command_tree(),
"pumpkin.summon",
PermissionLvl::Two,
);
dispatcher.register(
experience::init_command_tree(),
"pumpkin.experience",
PermissionLvl::Two,
);
dispatcher.register(
weather::init_command_tree(),
"pumpkin.weather",
PermissionLvl::Two,
);
dispatcher.register(
particle::init_command_tree(),
"pumpkin.particle",
PermissionLvl::Two,
);
dispatcher.register(
damage::init_command_tree(),
"pumpkin.damage",
PermissionLvl::Two,
);
dispatcher.register(
bossbar::init_command_tree(),
"pumpkin.bossbar",
PermissionLvl::Two,
);
dispatcher.register(say::init_command_tree(), "pumpkin.say", PermissionLvl::Two);
dispatcher.register(
gamemode::init_command_tree(),
"pumpkin.gamemode",
PermissionLvl::Two,
);
dispatcher.register(
transfer::init_command_tree(),
"pumpkin.transfer",
PermissionLvl::Two,
);
dispatcher.register(op::init_command_tree(), "pumpkin.op", PermissionLvl::Three);
dispatcher.register(
deop::init_command_tree(),
"pumpkin.deop",
PermissionLvl::Three,
);
dispatcher.register(
kick::init_command_tree(),
"pumpkin.kick",
PermissionLvl::Three,
);
dispatcher.register(
plugin::init_command_tree(),
"pumpkin.plugin",
PermissionLvl::Three,
);
dispatcher.register(
plugins::init_command_tree(),
"pumpkin.plugins",
PermissionLvl::Three,
);
dispatcher.register(
ban::init_command_tree(),
"pumpkin.ban",
PermissionLvl::Three,
);
dispatcher.register(
banip::init_command_tree(),
"pumpkin.banip",
PermissionLvl::Three,
);
dispatcher.register(
banlist::init_command_tree(),
"pumpkin.banlist",
PermissionLvl::Three,
);
dispatcher.register(
pardon::init_command_tree(),
"pumpkin.pardon",
PermissionLvl::Three,
);
dispatcher.register(
pardonip::init_command_tree(),
"pumpkin.pardonip",
PermissionLvl::Three,
);
dispatcher.register(
stop::init_command_tree(),
"pumpkin.stop",
PermissionLvl::Four,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean this up using a list for each level, containing every command that requires this level?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants