[WIP] Authorization#5518
Draft
ramonsmits wants to merge 17 commits into
Draft
Conversation
Add Authorize attribute to all controller APIs
Adds Authentication.RolesClaim (default "realm_access.roles") and a RolesClaimsTransformation that normalises per-IdP role claim shapes — Keycloak's nested realm_access.roles, flat repeated claims (Entra app roles, Keycloak with a User Realm Role mapper, Cognito groups), and JSON-array-as-string values — into canonical "roles" claims that PermissionVerbHandler reads unchanged. The parsing lives in a pure static RolesClaimExtractor in Infrastructure so it can be unit-tested without an ASP.NET host; the IClaimsTransformation implementation in Hosting is a thin wrapper that adds idempotency via a sentinel claim. Removes the TODO on PermissionVerbHandler that the transformation now resolves.
Without RequireAuthenticatedUser() in the policy, an unauthenticated request reaches PermissionVerbHandler, finds no roles, and fails the requirement — which ASP.NET classifies as FailedRequirements and turns into a 403 Forbid. The OIDC acceptance tests expect 401 Challenge (Should_reject_requests_without_bearer_token, ..._with_invalid/expired /wrong_audience/wrong_issuer). Adding the auth requirement first ensures the failure is classified as FailedAuthentication when no valid principal is present.
The Primary, Audit, and Monitoring acceptance-test runners called AddServiceControlAuthentication but not AddServiceControlAuthorization, while production RunCommand wires both. Without the authorization provider, ASP.NET cannot resolve the policy names emitted by the Permissions catalogue and any request to a controller carrying [Authorize(Policy = Permissions.X)] throws "AuthorizationPolicy named '...' was not found" — which breaks the test hosts and times out the audit acceptance tests that exercise authorized endpoints.
Should_accept_requests_with_valid_bearer_token previously passed in CI by accident: AddServiceControlAuthorization was missing from the test runners, so the policy provider could not resolve the [Authorize] policy name and threw, returning 500 — which the assertion "not 401 and not 403" accepted. With the policy provider now wired up, a valid token without any role claim correctly fails the permission requirement and returns 403. The test must therefore supply the canonical "roles" claim with a value that maps to a role granting the endpoint's permission. "reader" grants every *:*:view permission, which covers the endpoints the three acceptance-test variants exercise (error:messages:view, audit:message:view, monitoring:endpoint:view).
The PlatformSampleSettings approval tests in SC, Audit, and Monitoring unit-test projects serialise the settings object and diff it against an approved JSON snapshot. The earlier Authentication.RolesClaim addition needs the snapshots updated to include the new property.
The recent refactor moved Authentication.RoleBasedAuthorizationEnabled to a separate master switch (default false) and made the entire authorization registration short-circuit when it was off. That left the permission policy provider unregistered in every default deployment, so ASP.NET could not resolve the policy names emitted by [Authorize(Policy = Permissions.X)] attributes — every annotated endpoint returned 500 with "AuthorizationPolicy named '...' was not found", which is what was timing out the audit acceptance tests at 90s each. PermissionPolicyProvider already returns allow-all policies for every known permission when its oidcEnabled flag is false. Registering it unconditionally and passing RoleBasedAuthorizationEnabled to that flag gives the right behaviour in all three combinations: RBAC off → allow-all (controllers reachable, no permission check), RBAC on → require auth + the permission requirement evaluated by PermissionVerbHandler. The handler itself remains gated on RoleBasedAuthorizationEnabled since it has nothing to evaluate when RBAC is off.
Authentication.RoleBasedAuthorizationEnabled defaults to false, so without an explicit opt-in the policy provider returns allow-all and unauthenticated requests reach the controller — breaking every Should_reject_requests_* test in the three When_authentication_is_enabled classes. Add WithRoleBasedAuthorizationEnabled() to the test configuration helper and call it alongside WithAuthenticationEnabled() in all three OIDC enabled fixtures.
Two settings shifted in the OpenIdConnectSettings serialisation: RolesClaim moved after the ServicePulse block and its default changed from "realm_access.roles" to "roles", and the new RoleBasedAuthorizationEnabled property (default false) was added. Sync the three approved snapshots so the approval tests reflect the current shape.
5 tasks
Make the name of the claim for roles configurable
* ✨ Audit log for every authorization decision PermissionVerbHandler now resolves the calling principal's subject id (sub claim), display name (preferred_username, falling back to name then sub), and roles, and emits a structured "allow" or "deny" entry through the new IAuthorizationAuditLog for every verb-level check. Both outcomes are captured — denies alone are insufficient for most compliance use cases — and the reason embeds the matched role(s) for allow and the candidate role(s) for deny. AuthorizationAuditLog writes on the stable category "ServiceControl.Audit" via a source-generated structured log method so any ILogger-compatible sink (Seq, OTLP, file, in-memory test double, …) can collect or filter the trail without coupling to the concrete type name. The audit log is registered alongside the verb handler — only when OIDC is enabled and the handler has decisions to make. Unauthenticated requests are skipped at the top of HandleRequirementAsync so the audit log only records identified principals; the framework challenges with 401 via the policy's RequireAuthenticatedUser anyway. Ported from the keycloak-rbac-poc spike (with the namespace flattened from Infrastructure.Auth.Rbac to Infrastructure.Auth to match the real branch) along with a RecordingLoggerProvider test helper colocated with the unit tests. * ✨ Configurable required subject claims for the audit log PermissionVerbHandler now reads the subject id and subject name from configurable claim keys (Authentication.SubjectIdClaim, default "sub"; Authentication.SubjectNameClaim, default "preferred_username") and throws InvalidOperationException when an authenticated principal lacks either claim or carries an empty value — both are required for the audit log to be meaningful and a missing value indicates an IdP misconfiguration the operator needs to fix. The settings are passed through AddServiceControlAuthorization, which now takes the full OpenIdConnectSettings (the existing bool-only overload is removed; the six callers — three RunCommand entry points and three acceptance-test runners — pass the settings object). The MockOidcServer test helper defaults preferred_username to the subject value so existing OIDC acceptance tests don't have to repeat the boilerplate. * Separate allow/deny log templates, log deny as Warning * Add TODO for instance-specific audit categories in AuthorizationAuditLog * IDE0055 * Refactor dependencies on settings to inject `OpenIdConnectSettings` * ✨ Route ServiceControl.Audit category to a structured JSON log target Add a dedicated NLog target and a final logging rule that emit the authorization audit trail as structured JSON on the ServiceControl.Audit category, separate from the plain-text operational log, so it can be shipped to a SIEM without the two streams polluting each other. Audit events are captured from Info upward (allow = Information, deny = Warning) independent of the operational LogLevel, so lowering operational verbosity never drops entries from the audit trail. The audit rule is registered before the catch-all operational rules and marked Final so audit events are not duplicated into the operational log. Extracts BuildConfiguration from ConfigureNLog, registers the targets explicitly so the configuration is fully formed before it is installed, and exposes AuthorizationAuditLog.AuditCategory so the routing is unit-testable against a single source of truth for the category name. Tests assert the routing structure and that a real decision renders as one valid JSON object per line. * 🐛 Align AuthorizationAuditLog tests with the Allow:/Deny: templates The audit log message templates were changed to a capitalised "Allow:"/"Deny:" prefix (and deny moved to Warning) when the allow/deny templates were split, but these tests still asserted the old lowercase "allow"/"deny" substrings and an Information level for deny — so they failed on CI (Linux-Default, Windows-Default). Update the assertions to match the current output: "Allow:"/"Deny:" and deny at Warning level. * ✨ Emit the authorization audit event as an Elastic Common Schema (ECS) document AuthorizationAuditLog now serialises each decision as an ECS-shaped JSON document (@timestamp, event.kind/category/type/action/outcome, user.id/name, and the app-specific servicecontrol.* namespace) so it ingests into Elastic/Kibana — and most SIEMs — with no custom mapping. The schema is owned in the domain class; the NLog audit target now writes the pre-rendered document verbatim (one object per line) instead of assembling JSON in logging configuration. Allow/deny is carried by event.type (["allowed"]/["denied"]) and event.outcome (success/failure); the log level still differs (Information/Warning) so sinks can alert on denies without parsing the payload. Relaxed JSON escaping keeps the output readable for log sinks. Only fields available at the verb-level check are populated today; user.roles, user.email and resource scope follow as that data reaches the decision point. * Remove outdated TODO in `AuthorizationAuditLog` comment regarding instance-specific categories * Apply suggestions from code review Co-authored-by: WilliamBZA <WilliamBZA@users.noreply.github.com> * cleanup --------- Co-authored-by: WilliamBZA <WilliamBZA@users.noreply.github.com>
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
3 participants
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.
No description provided.