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

Added default permission "console" #6501

Closed
wants to merge 7 commits into from
Closed

Conversation

fsetinh
Copy link

@fsetinh fsetinh commented Nov 13, 2024

Added a new default permissions value console to make sensitive commands restricted to console only.

Related to this #6499

Related issues & PRs

Fixes #6499

Changes

Behavioural changes

Any command with default: console permission added in its plugin.yml will only work and show in console.

@fsetinh fsetinh requested a review from a team as a code owner November 13, 2024 18:29

public const DEFAULT_CONSOLE = "console";

public const DEFAULT_NOT_CONSOLE = "notconsole";
Copy link
Member

Choose a reason for hiding this comment

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

Don't bother with this. You haven't implemented support for it and no one will use it. (Trust me, no one understands what notop is supposed to be used for either.)

Copy link
Contributor

Choose a reason for hiding this comment

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

The "notconsole" may be useful for commands that are not supported to run in cli mode. For example, a command that opens a form for the sender

Copy link
Member

Choose a reason for hiding this comment

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

Inverted permissions aren't well-defined.

I also don't think denying permission is the right way to address that, since it'll give confusing messages.

Copy link
Member

Choose a reason for hiding this comment

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

You'd also still end up needing to check instanceof of command sender within the execute() anyway to ensure that you're not letting in other types of command senders, or console senders which have had this permission overridden to true.

@@ -41,13 +45,17 @@ class PermissionParser{
"isoperator" => self::DEFAULT_OP,
"admin" => self::DEFAULT_OP,
"isadmin" => self::DEFAULT_OP,
"console" => self::DEFAULT_CONSOLE,
"isconsole" => self::DEFAULT_CONSOLE,
Copy link
Member

Choose a reason for hiding this comment

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

Don't add stuff that doesn't exist.

@jasonw4331 jasonw4331 added Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Nov 13, 2024
@dktapps dktapps changed the title Added default permission "console". Added default permission "console" Nov 14, 2024
@ShockedPlot7560
Copy link
Member

I don't see the real purpose of the PR apart from adding an extra permission that may never be used and that won't solve the problem of having to check the CommandSender instance.

@dktapps
Copy link
Member

dktapps commented Nov 14, 2024

I don't see the real purpose of the PR apart from adding an extra permission that may never be used and that won't solve the problem of having to check the CommandSender instance.

It's actually used internally already for /dumpmemory. There's definitely a use case for restricting access to terminal-only.

@dktapps dktapps added the Easy task Probably really easy to do, good task for first-time contributors label Dec 1, 2024
@fsetinh fsetinh closed this by deleting the head repository Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Easy task Probably really easy to do, good task for first-time contributors Status: Waiting on Author Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants