fix(permissions): stop wildcards from granting negative permissions#13
Draft
hbrombeer wants to merge 1 commit into
Draft
fix(permissions): stop wildcards from granting negative permissions#13hbrombeer wants to merge 1 commit into
hbrombeer wants to merge 1 commit into
Conversation
A permission whose meaning is inverted — holding it takes something away — was matched by `*` like any other. The admin role holds `ALLOW *`, so every admin also held `grounds.chat.muted` and was silently muted: /msg answered "Du bist gemutet." and their global chat was dropped without a word. A manifest entry can now declare `negative: true`. Negative permissions are only ever matched by an exact grant, never by `*` or `foo.*`, so a wildcard keeps meaning "give this player everything" instead of also meaning "mute them". Muting still works — by name. The loaded plugins' own manifests are the authority: only they can be asked about a permission in this JVM, so the checker is built from the collected manifests before it answers its first question.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
/msganswered "Du bist gemutet." for every admin, and their global chat was dropped silently.Ground truth from the dev cluster:
grounds.chat.mutedis an inverted permission: holding it takes chat away.ALLOW *means "give this player everything" — and the matcher dutifully handed them the mute too. Every admin was muted by their own admin role.It is not specific to chat. Any negative permission anyone adds to the catalog gets handed to every wildcard holder on the next snapshot.
The fix
A manifest entry can declare
negative: true. A negative permission is then only ever matched by an exact grant — never by*orfoo.*. Muting still works, it just has to be done by name.The decision is made here, not in
service-permissions: the service only serves snapshots of patterns, and nothing server-side callsPolicyEngine.hasPermissionin production. Every real check runs in this plugin, in the JVM that loaded the manifests — which makes those manifests the complete authority on what is negative.Follow-up (not in this PR)
service-permissionscarries a second copy of the same matcher (PolicyEngine/PermissionPattern). It has no production caller today, but it would reintroduce this bug the day it gets one.negativecolumn, so the Portal permission panel cannot warn that tickinggrounds.chat.mutedmutes somebody.Verified
*andgrounds.chat.*no longer grantgrounds.chat.mutedwhile still grantinggrounds.chat.staff; an exact grant still mutes; DENY still beats an exact grant../gradlew buildgreen.Needs
plugin-chatto mark the key negative (separate PR) before it changes anything at runtime.