fix: name which term of the delegation intersection came out empty - #356
Open
KunalJavelin wants to merge 2 commits into
Open
KunalJavelin wants to merge 2 commits into
KunalJavelin wants to merge 2 commits into
Conversation
A delegated grant is a three-way intersection:
requested ∩ the subject token's own scopes ∩ the actor's ceiling
When it came out empty, tokenExchange returned one string for all three
causes — and the three need OPPOSITE repairs. The caller either names the
scopes, widens the DELEGATOR, or widens the SUB-AGENT. One message for all
three sends people to widen the identity that was never the constraint;
highflame-authn#181 reports losing time to two of them in a row.
All three sets are already in hand at the denial site, so naming the empty
term costs no extra lookup.
omitted -> ...: no scopes were requested, and this grant has no default
— name the scopes to delegate
subject -> ...: the subject token does not hold [data:read]
actor -> ...: the actor identity is not registered for [data:read]
both -> ...: the subject token does not hold [order:write]; the actor
identity is not registered for [data:read]
Design notes:
- The error code stays invalid_scope and the original sentence stays as a
prefix, so neither the wire contract nor an existing log grep changes.
Nothing in the repo asserted the old string.
- An actor with NO ceiling is never blamed. An empty ceiling means "no
restriction from this layer", so blaming it would send the caller to widen
a registration that was never the constraint.
- Each scope is blamed once. A scope the subject cannot delegate is reported
under the subject even when the actor also lacks it; listing it twice would
read as two separate repairs.
- The omitted-scope case is called out explicitly because token_exchange is
the ONE grant with no RFC 6749 §3.3 default — every other grant teaches the
caller that omitting scope means "grant the full ceiling".
Tests: six unit tests cover delegationScopeDenial in isolation; two
integration subtests cover that tokenExchange hands it the right three sets.
All three integration assertions fail against the old single message and pass
with this change. Full suite green — unit plus 39s of integration.
Fixes #302
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first pass always said "the actor identity is not registered for [x]". effectiveAllowedScopes is either/or, not layered: when the actor's credential policy sets scopes, the identity's own allowed_scopes is never read. So the ceiling frequently came from the POLICY while the message pointed at the REGISTRATION — the wrong-repair hint this denial exists to remove. The existing integration subtest proved it. delegationPolicy() sets allowed_scopes, so that test's ceiling came from the policy, and the old assertion passed on a message that named the wrong field. policy ceiling -> the actor's credential policy does not permit [tools:read] registration -> the actor identity is not registered for [tools:read] effectiveAllowedScopesWithSource returns the ceiling AND its source, and effectiveAllowedScopes is now a thin wrapper over it. One decision point: a caller that re-derived "which layer won" separately would go stale the moment that precedence changes — and it is due to change, since #300's fix makes the resolution layered rather than either/or. Behaviour of effectiveAllowedScopes is unchanged. It returned a nil-or-empty identity slice before and returns nil now; every caller gates on len(). Tests: one new unit test per source, plus TestEffectiveAllowedScopesWithSource pinning that the reported source never disagrees with the scopes returned. The integration subtest for a policy ceiling joins the one for a registration ceiling, so both branches are covered end to end. Forcing the old single wording fails the policy subtest: "...the actor identity is not registered for [tools:read]" does not contain "the actor's credential policy does not permit [tools:read]" Full suite green — unit plus 37s of integration. go vet and gofmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saucam
approved these changes
Sep 16, 2026
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
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.
Fixes #302. Also closes ask 3 of highflame-ai/highflame-authn#181.
The problem
A delegated grant is a three-way intersection:
When it came out empty,
tokenExchangereturned one string for all three causes:The three need opposite repairs:
allowed_scopesOne message for all three sends people to widen the identity that was never the constraint. highflame-ai/highflame-authn#181 reports losing time to two of them in a row.
What the caller sees now
The actor term splits in two because
effectiveAllowedScopesis either/or, not layered: when the actor's credential policy sets scopes, the identity's ownallowed_scopesis never read. Naming the registration when the ceiling came from the policy sends the caller to edit a field that was not the constraint — the exact wrong-repair hint this PR exists to remove.effectiveAllowedScopesWithSourcereturns the ceiling and its source, andeffectiveAllowedScopesis now a thin wrapper over it. One decision point, so a caller cannot re-derive "which layer won" and go stale — and it is due to change, since #300's fix makes the resolution layered rather than either/or.Behaviour of
effectiveAllowedScopesis unchanged. It returned a nil-or-empty identity slice before and returns nil now; every caller gates onlen().All three sets were already in hand at the denial site, so naming the empty term costs no extra lookup.
Four design decisions
The wire contract does not change. The code stays
invalid_scope, and the original sentence stays as a prefix, so an existing log grep keeps matching. Nothing in the repo asserted the old string.An actor with no ceiling is never blamed. An empty ceiling means "no restriction from this layer". Blaming it would send the caller to widen a registration that was never the constraint.
Each scope is blamed once. A scope the subject cannot delegate is reported under the subject even when the actor also lacks it. Listing it twice would read as two separate repairs.
The omitted-scope case is called out explicitly.
token_exchangeis the one grant with no RFC 6749 §3.3 default. Every other grant teaches the caller that omittingscopemeans "grant the full ceiling", so the failure is genuinely surprising.Tests
delegationScopeDenialin isolation, including the two cases that are easy to get wrong: an unrestricted actor, and a scope both parties lack.tokenExchangehands the helper the right sets, including one per actor-ceiling source. The unit tests alone would not have caught a wiring mistake.TestEffectiveAllowedScopesWithSourcepins that the reported source never disagrees with the scopes returned.TestSubagentDelegation_EmptyRequestGrantsNothinggains a description assertion.All three integration assertions were run against the old single message and fail:
Forcing the old single actor wording fails the policy subtest too:
That subtest is how the imprecision was found:
delegationPolicy()setsallowed_scopes, so its ceiling came from the policy, and the first pass asserted a message naming the wrong field.Full suite green: unit, plus 39s of integration.
go vetandgofmtclean.Not in scope
The self-mint denial from #341 and #345.
requireGrantableScopeanswersapi_key,client_credentials,jwt_bearerandauthorization_codewith one string —"requested scopes are not permitted for this identity"— for a chain of up to four ceilings (key.scopes, the key's policy, the identity's policy, the deprecated identity list). Same defect class as this PR, different grant, and naming the layer there means threading the individual ceilings into that helper. Worth its own issue.The scope-ceiling semantics. #300 (the tenant default policy sets no scope ceiling) and #301 (an empty ceiling means "no restriction") are unchanged here. This PR only improves the message once the intersection is already empty.
🤖 Generated with Claude Code