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

Target permissions #575

Merged
3 commits merged into from
May 9, 2022
Merged

Target permissions #575

3 commits merged into from
May 9, 2022

Conversation

Birdulon
Copy link
Member

@Birdulon Birdulon commented May 6, 2022

image

Adds permissionTargeted to all applicable commands. (i.e. every command which targets a player and isn't sendmail)

The player.perm and server.perm distinction seems a bit ill-defined, things like drop seem like they should be player.drop instead of server.drop (ideally I think the true server.perm commands wouldn't have a target.perm permission at all since they would imply usage on anyone and everyone), but that might be something to leave for one big permissions rework down the track since any changes would involve db migration procedures for server operators. I think some wildcard matching would be good at some point, so admins can set things like player.* and *.spawn for convenience.

@ghost ghost requested review from KingRainbow44 and memetrollsXD May 6, 2022 08:21
@jie65535
Copy link
Contributor

jie65535 commented May 6, 2022

My suggestion is to use @ as default permission

String permissionTargeted() default "@";

@memetrollsXD
Copy link
Member

How about a new subpermission? player.clear.others for all commands

@memetrollsXD
Copy link
Member

It's basically just another syntax of what you have here, looking for peoples opinions

@KingRainbow44 KingRainbow44 removed their request for review May 7, 2022 01:42
@lhhxxxxx
Copy link
Contributor

lhhxxxxx commented May 7, 2022

Account.hasPermission() should also be updated.

@Birdulon
Copy link
Member Author

Birdulon commented May 7, 2022

Account.hasPermission() should also be updated.

Good catch, I didn't see that it already has wildcards

@lhhxxxxx
Copy link
Contributor

lhhxxxxx commented May 7, 2022

Account.hasPermission() should also be updated.

Good catch, I didn't see that it already has wildcards

I don't consider subpermission to be a good idea, because "player.*" seems to cover all the subpermissions under the player permission, but in fact we don't want those with player permission to have @ permission

@lhhxxxxx
Copy link
Contributor

lhhxxxxx commented May 7, 2022

public boolean hasPermission(String permission) {
		return this.permissions.contains(permission) ||
                this.permissions.contains("*") ||
                (this.permissions.contains("player") || this.permissions.contains("player.*")) && permission.startsWith("player.") ||
                (this.permissions.contains("server") || this.permissions.contains("server.*")) && permission.startsWith("server.") ||
                (this.permissions.contains("target") || this.permissions.contains("target.*") || this.permissions.contains("@")) && permission.startsWith("target.");
	}

This could be a solution

@4Benj
Copy link
Member

4Benj commented May 7, 2022

The permission system will be getting a rewrite soon to make it more expandable and to fix this sort of thing. Current plan is for * to be a first layer wildcard and ** to be an all layer wildcard

@Birdulon
Copy link
Member Author

Birdulon commented May 7, 2022

Added this

Current plan is for * to be a first layer wildcard and ** to be an all layer wildcard

EDIT: Hacky unit test over at https://github.com/Birdulon/Grasscutter/tree/TargetPermsUnitTest
image

@ghost
Copy link

ghost commented May 7, 2022

The permission system will be getting a rewrite soon to make it more expandable and to fix this sort of thing. Current plan is for * to be a first layer wildcard and ** to be an all layer wildcard

Note that player.*.self and player.*.target might be a good alternative and will be simpler to implement than what you propose. Examples include:

  • player.* - anything that starts with player.
  • player.*.target - anything that matches player.*.target, where * is a wildcard
  • player.*.target.* - anything that starts with player.*.target., where * is a wildcard
  • player.coop.self.* - anything that starts with player.coop.self., where * is a wildcard

Implementation of wildcard can be done as regular expression, so internally it becomes:

  • ^player(?:\..*)?$
  • ^player\..*\.target$
  • ^player\..*\.target(?:\..*)?$
  • ^player\.coop\.self(?:\..*)?$

Internally, we would store list of permissions account has, then escape expression and replace \* with .*, add ^ and $ and then test it against full permission name in code. Note that last instance of \.\* should be replaced with (?:\..*)? instead so that it encompasses both player.coop.target as well as player.coop.target.join.

@ghost ghost requested a review from 4Benj May 8, 2022 05:48
@Birdulon
Copy link
Member Author

Birdulon commented May 9, 2022

Fixed merge conflicts.

@ghost ghost merged commit 993fe31 into Grasscutters:development May 9, 2022
@Birdulon Birdulon deleted the TargetPerms branch May 9, 2022 10:18
This pull request was closed.
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.

None yet

5 participants