Fix RolesClaim default docs and cover the RBAC deny path#5579
Conversation
| /// claim makes the transformation idempotent and returns the same principal on subsequent calls. | ||
| /// </para> | ||
| /// </summary> | ||
| public sealed class RolesClaimsTransformation(OpenIdConnectSettings oidcSettings) : IClaimsTransformation |
There was a problem hiding this comment.
I actually have a feeling this entire class is not needed because we specify the TokenValidationParameters.RoleClaimType value in https://github.com/Particular/ServiceControl/blob/auth/src/ServiceControl.Hosting/Auth/HostApplicationBuilderExtensions.cs#L47
There was a problem hiding this comment.
Nevermind, it is still needed.
There was a problem hiding this comment.
This class is actually the load-bearing part here, and RoleClaimType is the redundant one. TokenValidationParameters.RoleClaimType doesn't create or rename claims to ClaimTypes.Role; it only tells the identity which claim IsInRole() / [Authorize(Roles=…)] should read. Our enforcement doesn't use those, it reads context.User.FindAll(ClaimTypes.Role) in both PermissionVerbHandler and EffectivePermissions. With only RoleClaimType set, that returns nothing:
RoleClaimType = "roles" on the identity
IsInRole("reader") = True
FindAll(ClaimTypes.Role).Count = 0 ← what our handlers see
So dropping RolesClaimsTransformation would make every RBAC check find zero roles and deny everyone. It's also the only thing that can read a nested path like realm_access.roles (Keycloak out-of-box), which RoleClaimType can't express at all since it's a claim name, not a JSON path. The genuinely redundant line is RoleClaimType = oidcSettings.RolesClaim in HostApplicationBuilderExtensions — the transformation already normalizes everything to ClaimTypes.Role, so that setting is dead.
| /// claim makes the transformation idempotent and returns the same principal on subsequent calls. | ||
| /// </para> | ||
| /// </summary> | ||
| public sealed class RolesClaimsTransformation(OpenIdConnectSettings oidcSettings) : IClaimsTransformation |
There was a problem hiding this comment.
Nevermind, it is still needed.
The RolesClaim setting defaults to the flat "roles" claim in code (and in the published docs), but two XML doc comments claimed the default was "realm_access.roles". That is misleading: an operator following the comment for a stock Keycloak instance would expect roles to resolve out-of-box when in fact the flat default yields no roles until RolesClaim is pointed at the nested path. Align the comments with the actual default.
The acceptance suite exercised authentication thoroughly (invalid, expired, wrong audience/issuer, and a reader accepted with 200) but never asserted the core authorization outcome: an authenticated user lacking the required role is forbidden. Add two end-to-end tests against the Primary instance: - a reader (view-only) POSTing to errors/retry/all is forbidden (403), not merely unauthenticated (401) - my/routes projects to exactly what the role permits: the manifest advertises a view route the reader may call and omits a retry route it may not This pins the invariant the design relies on, that the advertised manifest and the enforced policy read the same inputs and cannot drift apart.
RoleClaimType only influences IsInRole() and [Authorize(Roles = ...)], neither of which this platform uses. Authorization reads FindAll(ClaimTypes.Role), and those claims are produced solely by RolesClaimsTransformation, which extracts roles from the configured RolesClaim path (including nested paths such as realm_access.roles) and normalizes them to ClaimTypes.Role. RoleClaimType does not rename or emit claims, so setting it changed nothing. Drop it and document why it stays unset.
dvdstelt
force-pushed
the
fix-rolesclaim-default-and-rbac-deny-tests
branch
from
ddd4c13 to
e3be8f6
Compare
2 participants
Follow-up to the review of #5518. Two issues surfaced there that were not yet addressed.
1.
RolesClaimdefault contradicted its own documentationThe setting defaults to the flat
rolesclaim in code (and in the published docs), but two XML doc comments claimed the default wasrealm_access.roles. That is misleading: an operator following the comment for a stock Keycloak instance would expect roles to resolve out-of-box, when in fact the flat default yields no roles untilRolesClaimis pointed at the nested path.Corrected the comments in
OpenIdConnectSettings.csandRolesClaimsTransformation.csto state the flatrolesdefault and point operators atrealm_access.roles(Keycloak out-of-box) orcognito:groups(AWS Cognito) as dotted-path options. No behavior change.2. The RBAC deny path had no end-to-end coverage
The acceptance suite exercised authentication thoroughly (invalid, expired, wrong audience/issuer, and a reader accepted with 200) but never asserted the core authorization outcome: an authenticated user lacking the required role is forbidden. Three test files even carried a
// would be 403comment for a case no test exercised.Added two end-to-end tests against the Primary instance:
errors/retry/allis forbidden (403), not merely unauthenticated (401)my/routesprojects to exactly what the role permits: the manifest advertises a view route the reader may call (GET /api/errors) and omits a retry route it may not (POST /api/errors/retry/all)This pins the invariant the design relies on: the advertised manifest and the enforced policy read the same inputs and cannot drift apart.