Skip to content

Require authentication on permission policies when role-based authorization is disabled - #5979

Merged
ramonsmits merged 1 commit into
masterfrom
fix-authorization-without-rbac
Oct 9, 2026
Merged

ramonsmits merged 1 commit into
masterfrom
fix-authorization-without-rbac

Conversation

@ramonsmits

@ramonsmits ramonsmits commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Fixes:

Problem

When Authentication.Enabled is true and Authentication.RoleBasedAuthorizationEnabled is left at its default of false, the error, audit and monitoring instances accept requests without a bearer token, with an invalid token or with an expired token on every action that carries a permission policy. The request runs as if a signed-in user had sent it.

PermissionPolicyProvider chose between the role-checking policy and a shared allow-all policy by looking only at the RBAC setting. The allow-all policy has no authenticated-user requirement. The RequireAuthenticatedUser fallback policy does not help, because ASP.NET Core only applies it to endpoints without authorization metadata, and every controller action carries [Authorize(Policy = ...)].

Introduced in 6.18.0 by #5518. When that change broke the Should_reject_requests_* acceptance tests, the fixtures were switched to RBAC enabled, which removed the coverage for the default configuration.

Change

The provider now takes both settings into account:

  • Authentication disabled: allow-all, and RBAC is ignored. This matches the documented master switch.
  • Authentication enabled, RBAC disabled: every permission requires an authenticated user. This restores the behavior from before role-based access control was added, so requests without a valid token get a 401.
  • Authentication and RBAC enabled: unchanged.

Supporting changes:

  • The startup log line for authentication settings now includes the RBAC setting, so operators can see which authorization mode an instance runs in.
  • The monitoring container health check now calls the anonymous root endpoint instead of /connection. The old target requires an authenticated user whenever authentication is enabled and only appeared to work because of this defect. The error and audit images already check anonymous routes.

Constraints

  • Every controller action carries a permission policy, so those policies must enforce authentication themselves. The fallback policy cannot be relied on for them.
  • [AllowAnonymous] routes are unaffected: the root endpoints, /api/configuration, the health endpoints and the audit-count route stay open as before.
  • With RBAC disabled, callers that pass authentication hold every permission, which is what EffectivePermissions already reports to ServicePulse.

Verification

  • New unit tests evaluate every permission through the real AddServiceControlAuthentication and AddServiceControlAuthorization registrations for each combination of the two settings. Against the previous provider, two of them fail, including anonymous callers being allowed every permission with authentication enabled and RBAC disabled.
  • New When_authentication_is_enabled_without_role_based_authorization acceptance fixture on all three instances (2 tests each):
    • A request without a token to every route that is not [AllowAnonymous] gets 401. Against the previous provider, this test fails.
    • A valid token without roles is accepted.
  • The existing When_authentication_is_enabled fixtures (RBAC enabled) also run the every-route test.
  • An invalid, expired or wrong-audience token does not authenticate the caller, so it takes the same path as a request without a token. JWT validation does not depend on the RBAC setting, so the existing token tests run only with RBAC enabled.
  • Checked cross-instance calls: scatter-gather and remote connection details forward the caller's Authorization header, and the remote probes target anonymous routes.

…disabled

PermissionPolicyProvider chose between the role-checking policy and an
allow-all policy by looking only at
Authentication.RoleBasedAuthorizationEnabled. With authentication enabled
and RBAC at its default of false, every permission resolved to allow-all.
Every controller action carries a permission policy, so the
RequireAuthenticatedUser fallback policy never applied. Requests without a
token, or with an invalid or expired token, reached the controllers on the
error, audit and monitoring instances.

The provider now uses both settings:

- Authentication disabled: allow-all. RBAC is ignored.
- Authentication enabled, RBAC disabled: every permission requires an
  authenticated user. This is the behavior from before role-based access
  control was added.
- Authentication and RBAC enabled: unchanged.

Other changes:

- The startup log line for the authentication settings includes the RBAC
  setting.
- The monitoring container health check calls the anonymous root endpoint
  instead of /connection. /connection requires an authenticated user when
  authentication is enabled.

Tests:

- Unit tests evaluate every permission through the real registrations, for
  each combination of the two settings.
- New When_authentication_is_enabled_without_role_based_authorization
  acceptance fixtures on all three instances. When RBAC was added, the
  existing fixtures were switched to RBAC enabled only, which removed the
  coverage for the default configuration.
  - A request without a token to every route that is not [AllowAnonymous]
    gets 401. This test fails without the fix.
  - A valid token without roles is accepted.
- The existing When_authentication_is_enabled fixtures (RBAC enabled) also
  run the every-route test.

Co-authored-by: Ramon Smits <ramon.smits@gmail.com>
@ramonsmits
ramonsmits requested a review from dvdstelt October 9, 2026 13:47
@ramonsmits ramonsmits added this to the 6.21.2 milestone Oct 9, 2026
@ramonsmits ramonsmits added the Bug label Oct 9, 2026
@ramonsmits ramonsmits removed this from the 6.21.2 milestone Oct 9, 2026
@ramonsmits
ramonsmits enabled auto-merge (squash) October 9, 2026 13:48
@ramonsmits
ramonsmits merged commit ff995c9 into master Oct 9, 2026
69 of 70 checks passed
@ramonsmits
ramonsmits deleted the fix-authorization-without-rbac branch October 9, 2026 14:06
ramonsmits added a commit that referenced this pull request Oct 9, 2026
…disabled (#5979) (#5982)

PermissionPolicyProvider chose between the role-checking policy and an
allow-all policy by looking only at
Authentication.RoleBasedAuthorizationEnabled. With authentication enabled
and RBAC at its default of false, every permission resolved to allow-all.
Every controller action carries a permission policy, so the
RequireAuthenticatedUser fallback policy never applied. Requests without a
token, or with an invalid or expired token, reached the controllers on the
error, audit and monitoring instances.

The provider now uses both settings:

- Authentication disabled: allow-all. RBAC is ignored.
- Authentication enabled, RBAC disabled: every permission requires an
  authenticated user. This is the behavior from before role-based access
  control was added.
- Authentication and RBAC enabled: unchanged.

Other changes:

- The startup log line for the authentication settings includes the RBAC
  setting.
- The monitoring container health check calls the anonymous root endpoint
  instead of /connection. /connection requires an authenticated user when
  authentication is enabled.

Tests:

- Unit tests evaluate every permission through the real registrations, for
  each combination of the two settings.
- New When_authentication_is_enabled_without_role_based_authorization
  acceptance fixtures on all three instances. When RBAC was added, the
  existing fixtures were switched to RBAC enabled only, which removed the
  coverage for the default configuration.
  - A request without a token to every route that is not [AllowAnonymous]
    gets 401. This test fails without the fix.
  - A valid token without roles is accepted.
- The existing When_authentication_is_enabled fixtures (RBAC enabled) also
  run the every-route test.


(cherry picked from commit ff995c9)

Co-authored-by: Dennis van der Stelt <dvdstelt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants