Add configurable claim-validation gates#4
Conversation
Introduce an opt-in, path-agnostic gate package that can reject a valid token whose claims do not meet an authorization requirement: - AudienceGate: token aud must contain an accepted audience (membership test) - RoleGate: token must carry one of the required roles (any-of), read from a configurable nested claim path (e.g. resource_access.<client>.roles) Gates operate on the structured JWTClaimsSet (not the lossy flattened authInfo map), throw a dedicated ClaimGateException (not BadCredentialsException), and toggle independently per path (idToken/bearer) defaulting off. ClaimGateFactory resolves config with per-path override -> shared fallback. Wired into the interactive ID-token path in OpenIdConnectFilter, mapping a gate failure to BadCredentialsException (its existing redirect handling). The infrastructure supports the bearer path for the upcoming bearer filter. Documented the new properties in README and CHANGELOG. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| ## <a name="1.5.0"></a>OpenID Authentication Plugin Version 1.5.x Release Notes | ||
| **BREAKING CHANGE:** 1.5.0 is compiled in Java21 and has dependency updates that require XNAT 1.10.0. | ||
|
|
||
| ### <a name="1.5.1"></a>Version 1.5.1 |
There was a problem hiding this comment.
This seems like a new feature, why not 1.6.0?
There was a problem hiding this comment.
The reason it's 1.5.1 now is that the develop branch is already set up to build 1.5.1-SNAPSHOT and I didn't adjust that. But you’re right that this and the upcoming bearer token PR seem substantive enough that a 1.6.0 could be justified.
|
|
||
| /** True if {@code openid.{p}.{path}.{gate}.enabled} is set to {@code true}. */ | ||
| private boolean isEnabled(final String providerId, final AuthPath path, final String gate) { | ||
| return Boolean.parseBoolean(plugin.getProperty(providerId, path.prefix() + "." + gate + ".enabled")); |
There was a problem hiding this comment.
The PR description says "the gates can be turned on globally", but this looks like path must always be specified
There was a problem hiding this comment.
That was an incorrect phrasing in the PR description. The implementation does follow the design, which is that you set globally what to check—what audience and roles you allow in the tokens—but can set for each token path whether to check those things or not. The reason being that aud is more or less useless on the idToken path since we literally just redirected to the IdP to get that token minted for us specifically, but is very important on the bearer path since we need to know the token coming in is for us. The role difference is about backwards compatibility, as I've mentioned in another comment.
The description I wrote was wrong about the design. The README says it better than the PR description did:
Configuration has two layers: what to check (defined once per provider) and whether to check it, per path.
But I'm open to discussing whether this is the right design or not. We could definitely have the same global/per-path override on enabled that we do on the claim settings.
There was a problem hiding this comment.
I have modified the implementation to match the PR description: The enabled settings are now per-path with a non-path-specific fallback, just like the other settings.
| Comma-delimited list of roles. The role gate passes if the token's roles contain at least one of | ||
| these (any-of). | ||
|
|
||
| #### openid.`providerId`.idToken.audCheck.enabled / openid.`providerId`.idToken.roleCheck.enabled |
There was a problem hiding this comment.
I was going to ask: why require a separate enabled flag, vs just detecting that requiredRoles/acceptedAudiences is not empty, but I guess because you want to control these separately for idToken vs bearer. Is that a real requirement (to handle those two differently)?
There was a problem hiding this comment.
Is that a real requirement (to handle [
idTokenvsbearer] differently)?
I think it is if only to maintain backwards compatibility. The existing idToken path doesn't validate the audience or check roles now, so to enable that by default would be a behavior change that consumers might not expect. But when I add bearer token handling, 1. that's new so we can set the default config however we want, and 2. the gates become much more critical, IMO, since that token could come from anywhere and should really be checked before being accepted; so I would want to set these to enabled=true by default for the bearer path but enabled=false by default for the idToken path (with a recommendation to enable them).
Move the per-path/shared property resolution out of ClaimGateFactory's private helpers into a GateConfig value object bound to a (provider, path). Callers now select resolution semantics by method name (value() for "what to check" fields with shared fallback, enabled() for per-path toggles) instead of hand-building property name fragments, removing a footgun for the upcoming bearer filter. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the gate enable toggles resolve the same way as the "what to check"
fields: a shared per-provider toggle (openid.{p}.audCheck.enabled) applies
to all paths, and a path-scoped toggle (openid.{p}.idToken.audCheck.enabled)
overrides it for that path. This removes the prior asymmetry where the
toggles were read per-path-only while every other field fell back to the
shared value — now every gate property is read identically, which is easier
to reason about and extend.
Because the path-scoped value always wins, a path-scoped enabled=false
overrides a shared enabled=true, letting one path opt out of a gate enabled
provider-wide. Defaults are unchanged (absent = false).
GateConfig.enabled() now delegates to the same resolve() helper as value().
Adds tests for the shared toggle and the path-scoped override; updates
README, CHANGELOG, and javadoc.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Add configurable, opt-in token gates that can reject an otherwise valid token whose claims do not meet some authorization requirement:
AudienceGate: tokenaudmust contain an accepted audience (for instance,aud=xnat). The purpose is to only accept tokens that were minted specifically for XNAT and reject any other tokens that are valid and signed by the issuer, but are intended for a different service (i.e. they have a differentaud).RoleGate: token must carry one of the required roles (any-of), read from a configurable nested claim path (e.g.resource_access.<client>.roles). The purpose is to be able to set some property likexnat-userorxnat-adminon a user's account in the upstream IdP, which then writes these roles into the token at the given path. When validating login we read the claim path and check for the presence of any of the accepted roles, and reject the login if none are present. This lets us do basic access control for XNAT at the IdP, but it is not intended for finer-grained project-level access controls, just "can they log in to XNAT or not?".The gates are written so that new ones can be added fairly easily in the future if there are other claims in the token that need to gate login.
By default all the gates are disabled, meaning any token that was accepted before will still be accepted. To opt in the gates can be turned on globally (
openid.<provider>.{audCheck,roleCheck}.enabled=true) or per token path (openid.<provider>.<path>.{audCheck,roleCheck}.enabled=true), with the per-path setting overriding the global setting for that path if they conflict. Right now the only implemented path isidToken, but I also added hooks and settings for a bearer token path which will be in a forthcoming PR.I added documentation for these settings to the
README.