feat: fine-grained selector permission#31
feat: fine-grained selector permission#31DrexHD merged 20 commits intoDrexHD:mainfrom qwertycxz:feat-selector
Conversation
1. Control one can or cannot select self, nonself players and non-player entities 2. Selecting amount limit 3. Selecting weight: entity with no weight can always select or be selected. If both have weight, then one cannot select th other who has bigger weight. The above permission settings could be applied to specified command and selectors. For Example, `/teleport` has two selectors: `targets` and `destination`. You could separated control them.
1. Update & format README.md 2. Update & format CHANGELOG.md
src/main/java/me/drex/vanillapermissions/mixin/selector/EntityArgumentMixin.java
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
|
Thank you very much, looks like you have some cool/useful ideas. |
1. Add support for `GameProfileArgument`, `ScoreHolderArgument` and `WaypointArgument` 2. Refactor directory structure 3. Improve performance by early return and async functions 4. Check `selectedWeight` only for players.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
I have already PRed a fix to fabric-permission-api to fix the NPE. I have also just pushed a commit to fix/improve the command name resolution. Some more "problems" I see from a second quick look:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
src/main/java/me/drex/vanillapermissions/util/ArgumentPermission.java
Outdated
Show resolved
Hide resolved
This commit has no functional changes. 1. Use static imports as many as possible 2. Rename `ArgumentPermission.check` to `ArgumentPermission.validate` 3. `Iterables.getLast(context.getNodes())` -> `context.getNodes().getLast()` 4. Use parallelStream instead of for-loop. Fix potential `NullPointerError` as well as improve performance 5. fix logger format bug 6. fabric_permissions_version for 1.21.6: 0.4.0 -> 0.4.1
This commit has no functional changes.
qwertycxz
left a comment
There was a problem hiding this comment.
About some refactors.
|
|
||
| String[] parts; | ||
| if (context.getRootNode() instanceof RootCommandNode) { | ||
| parts = context.getNodes().parallelStream().map(node -> node.getNode().getName()).toArray(String[]::new); |
There was a problem hiding this comment.
If the root node is THE root node (i.e. not alias), the infomation stored in context is enough to get name from it.
src/main/java/me/drex/vanillapermissions/util/ArgumentPermission.java
Outdated
Show resolved
Hide resolved
|
|
||
| var sourcePlayer = source.getPlayer().getGameProfile(); | ||
| try { | ||
| allOf(selected.parallelStream().mapMulti((object, consumer) -> { |
There was a problem hiding this comment.
parallelStream instead of for-loop. No more NPE and more efficient!
|
Do you have some examples of use cases (of the specific selector permissions) that you have in mind with this implementation and what permission setup would be required to "implement them". I am still a bit skeptical that this will be a rather complex system that barely anyone will ever use, because it may be quite complicated and (maybe?) isn't very practical for real use cases. (I am not sure, that's why I am asking if you could maybe explain more of your vision on that) For example: In the readme teleport example you deny
I am not sure about using a separate mod for that, because it would lack the context. Because I don't think this approach of recursively looking up meta data will be correct for all meta keys from other mods, which wont expect this behavior. |
This comment was marked as resolved.
This comment was marked as resolved.
…nto DrexHD-main
Migrate to kotlin gradle scripts
This comment was marked as resolved.
This comment was marked as resolved.
I had a try in here. It was a mess. So I turn to a new idea. A compromised idea. The compromised idea:
It can cover 100% usage. Everyone will be happy. :) Although the name is very long, MW4fpa could help. |
I want to keep the maintenance burden for this feature minimal, because I don't think this (super fine selector control) is something that many server admins need.
Did you have an answer to this? This is part of my skepticism about the usefulness of this PR. Permissions just aren't a great configuration system for something complex like this. |
|
Just don't deny Is that means users have to deny all the three permissions? No, only need to deny one or two of them. If you want to deny all selectors in
Great minds think alike! 😀 |
with readme update
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
I am pretty sure that |
|
You shouldn't use #number to reference things, because github will interpret that as an issue / pr reference. |
|
How about this? README.md
Implicitly Allow by default:
minecraft.selector.* #0
Allow:
minecraft.command.teleport # /teleport
minecraft.selector # All selectors
minecraft.selector.player.teleport.destination.destination #1
minecraft.selector.entity.teleport.destination.destination #2
minecraft.selector.entity.teleport.targets.targets.destination #3
minecraft.selector.player.teleport.facingEntity.* #4
Deny:
minecraft.selector.player.teleport.* #6
minecraft.selector.entity.* #5
minecraft.selector.self.teleport.facingEntity.* #7Command Behavior:
|
|
Maybe I don't quite understand what you want to say with the Another thing that I just thought about is that luckperms |
Mainly because I want to highlight the default. I have already moved it.
Actually, no. The example works whether But you remind me. The readme of MW4fpa should have mentioned that there is no |
|
MW4fpa is finally published 😀. Now there is just one more thing before merge this PR.
@DrexHD What do you think? |
|
It should be reasonably easy to manually patch the broken versions with a mixin in VanillaPermission. I don't think lucko is interested in a backport all the way to 1.20.1! |
|
Hey @DrexHD. I have some bad news: As far as I know, The latest version of LuckPerms in 1.20.1 does not support offline metadata. That means we have to drop support for Selection Weight for offline players in 1.20.1. The mixin of farbic-permissions-api is somehow tricky, though. I can do it. However, since we are dropping the support of 1.20.1, perhaps it is not that bad to drop the support of 1.21.1, 1.21.4 and 1.21.5.
What do you think? |
|
In that case drop it and clarify it in the README. |
|
Sorry, I made a mistake. Here's a correction: StatusThe following list shows which selectors can use fine-grained permissions:
The above text has already added to the README.md. A relative patch is finished as well. I'd say that this PR is ready. 😃 |
| /*if (object instanceof Player onlinePlayer) { | ||
| var selectedWeight = get(onlinePlayer, weight, Integer::parseInt); | ||
| if (selectedWeight.isPresent() && selectedWeight.get() > sourceWeightValue) { | ||
| consumer.accept(failedFuture(new CompletionException(ERROR_SELECTORS_NOT_ALLOWED.create()))); |
There was a problem hiding this comment.
For < 1.21.6, we only check online player's selected weight.
|
Thanks, I should be able to take a look and fully review this next week! |
|
Thanks! I have now reviewed this and pushed some code style changes. The only actual code changes were:
Let me know if these seem reasonable to me. I installed Metadata Wildcard for fabric-permissions-api and tried to use meta wild keys, but it didn't work as expected. If the meta problem is resolved (explained what I did wrong or fixed) I think this should be ready to merged and released as a beta 0.3 version! |
|
Thank you for your review!
This is because MW4fpa works like when I have already mentioned that in README of MW4fpa. But if you want, I could change the behavior from |
|
Thank you for your work! |
EntityArgumentto implement fine-grained selector permission.README.md.README.mdandCHANGELOG.md.Details, also available in
README.mdPermissions
minecraft.selector.entity.<command>.<selector>selectorwithincommandminecraft.selector.player.<command>.<selector>selectorwithincommandminecraft.selector.self.<command>.<selector>selectorwithincommandMeta
Also sometimes referred to as "options" or "variables".
Incorrect types are considered undefined values.
minecraft.selector.limit.<command>.<selector>Integerselectorwithincommandminecraft.selector.weight.<command>.<selector>IntegerSelectors
Command blocks and datapacks bypass all selector permission checks.
By default, granting
minecraft.selectorallows players to use any selector in commands they have access to.Fine-grained permission control operates as follows. Note that this mod restricts based on selection results, not
raw selector syntax. Using player names, UUIDs, or selectors like
@eare equivalent if they produce identicalresults.
Value of
selectorDefined in the Arguments section of each command's Minecraft Wiki page.
For example, the
/teleportcommand uses<targets>and<destination>as selectors.Scope Control
Use these permissions to define selector scope:
minecraft.selector.entity.<command>.<selector>minecraft.selector.player.<command>.<selector>minecraft.selector.self.<command>.<selector>Commands fail if a player attempts to select unauthorized entities.
Example
Players may teleport themselves (
selffortargets) or non-player entities (entityfortargets) to themselves(
selffordestination) or nonself players (playerfordestination).Entity Limit
Set meta
minecraft.selector.limit.<command>.<selector>to restrict the maximum number of entities selectable via agiven selector.
No limit is applied if this meta is unset.
Selection Weight
Controlled by meta
minecraft.selector.weight.<command>.<selector>.Entities without weight settings can always select any target and be selected by any selector. When both entities have
weight values, a selector can only select targets whose weight is
less than or equalto its own.Example
No weight restriction for Player4