Merge pull request #5579 from Particular/fix-rolesclaim-default-and-rbac-deny-tests

Fix RolesClaim default docs and cover the RBAC deny path

Author: dvdstelt

src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs

@@ -205,6 +205,31 @@ public async Task Should_reject_requests_with_wrong_issuer()
             OpenIdConnectAssertions.AssertUnauthorized(response);
         }
 
+        [Test]
+        public async Task Should_forbid_authenticated_user_lacking_required_permission()
+        {
+            HttpResponseMessage response = null;
+
+            _ = await Define<Context>()
+                .Done(async ctx =>
+                {
+                    // The "reader" role grants every :view permission but none of the operate ones.
+                    // Retrying messages requires error:messages:retry (writer/admin only), so an
+                    // authenticated reader must be forbidden (403), not merely unauthenticated (401).
+                    var readerToken = mockOidcServer.GenerateToken(
+                        additionalClaims: new[] { new Claim("roles", "reader") });
+                    response = await OpenIdConnectAssertions.SendRequestWithBearerToken(
+                        HttpClient,
+                        HttpMethod.Post,
+                        "/api/errors/retry/all",
+                        readerToken);
+                    return response != null;
+                })
+                .Run();
+
+            OpenIdConnectAssertions.AssertForbidden(response);
+        }
+
         class Context : ScenarioContext;
     }
 }

src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_my_routes_are_requested.cs

@@ -63,5 +63,41 @@ public async Task Should_reject_requests_without_bearer_token()
         OpenIdConnectAssertions.AssertUnauthorized(response);
     }
 
+    [Test]
+    public async Task Should_return_only_the_routes_the_callers_role_permits()
+    {
+        HttpResponseMessage response = null;
+
+        _ = await Define<Context>()
+            .Done(async ctx =>
+            {
+                // A reader holds every :view permission but none of the operate ones. The manifest is
+                // the projection of exactly what the server enforces, so it must advertise a view route
+                // the reader may call and omit a retry route it may not.
+                var readerToken = mockOidcServer.GenerateToken(
+                    additionalClaims: new[] { new Claim("roles", "reader") });
+                response = await OpenIdConnectAssertions.SendRequestWithBearerToken(
+                    HttpClient, HttpMethod.Get, "/api/my/routes", readerToken);
+                return response != null;
+            })
+            .Run();
+
+        OpenIdConnectAssertions.AssertAuthenticated(response);
+
+        var root = JsonDocument.Parse(await response.Content.ReadAsStringAsync()).RootElement;
+
+        var roles = root.GetProperty("roles").EnumerateArray().Select(role => role.GetString());
+        Assert.That(roles, Does.Contain("reader"));
+
+        var routes = root.GetProperty("routes").EnumerateArray()
+            .Select(entry => $"{entry.GetProperty("method").GetString()} {entry.GetProperty("url_template").GetString()}")
+            .ToArray();
+
+        Assert.That(routes, Does.Contain("GET /api/errors"),
+            "Reader holds error:messages:view, so the errors list route must be advertised.");
+        Assert.That(routes, Does.Not.Contain("POST /api/errors/retry/all"),
+            "Reader lacks error:messages:retry, so the retry route must be filtered out.");
+    }
+
     class Context : ScenarioContext;
 }

src/ServiceControl.Hosting/Auth/HostApplicationBuilderExtensions.cs

@@ -43,12 +43,15 @@ public static void AddServiceControlAuthentication(this IHostApplicationBuilder
                     ValidateLifetime = oidcSettings.ValidateLifetime,
                     ValidateIssuerSigningKey = oidcSettings.ValidateIssuerSigningKey,
                     ValidAudience = oidcSettings.Audience,
-                    ClockSkew = TimeSpan.FromMinutes(5), // Allow 5 minutes clock skew
-                    RoleClaimType = oidcSettings.RolesClaim
+                    ClockSkew = TimeSpan.FromMinutes(5) // Allow 5 minutes clock skew
                 };
                 options.RequireHttpsMetadata = oidcSettings.RequireHttpsMetadata;
                 // Don't map inbound claims to legacy Microsoft claim types
                 options.MapInboundClaims = false;
+                // No RoleClaimType is set: it only influences IsInRole()/[Authorize(Roles = ...)],
+                // which this platform does not use. Roles are read from RolesClaim (which may be a
+                // nested path such as realm_access.roles) and normalized to ClaimTypes.Role by
+                // RolesClaimsTransformation, which is what the authorization handlers actually read.
 
                 // Custom error response handling for better client experience
                 options.Events = new JwtBearerEvents

src/ServiceControl.Hosting/Auth/RolesClaimsTransformation.cs

@@ -11,9 +11,10 @@ namespace ServiceControl.Hosting.Auth;
 /// <summary>
 /// Normalises per-IdP role claim shapes into a flat set of <c>roles</c> claims that
 /// <see cref="PermissionVerbHandler"/> can read directly. The source path is configured via
-/// <c>Authentication.RolesClaim</c> (default <c>realm_access.roles</c> — the Keycloak out-of-box
-/// shape). Flat claim names work too (<c>roles</c> for Keycloak with a "User Realm Role" mapper or
-/// Microsoft Entra ID app roles, <c>cognito:groups</c> for AWS Cognito).
+/// <c>Authentication.RolesClaim</c> (default <c>roles</c> — a flat top-level claim, as emitted by
+/// Microsoft Entra ID app roles or Keycloak with a "User Realm Role" mapper). A dotted path reaches
+/// into a nested JSON object claim: <c>realm_access.roles</c> for Keycloak's out-of-box shape, or
+/// <c>cognito:groups</c> for AWS Cognito.
 /// <para>
 /// ASP.NET may invoke <see cref="TransformAsync"/> multiple times for the same principal; a sentinel
 /// claim makes the transformation idempotent and returns the same principal on subsequent calls.

src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs

@@ -139,11 +139,10 @@ public OpenIdConnectSettings(SettingsRootNamespace rootNamespace, bool validateC
     public string ServicePulseApiScopes { get; }
 
     /// <summary>
-    /// Path within the JWT where the user's role values live. Defaults to <c>realm_access.roles</c>
-    /// to match Keycloak's out-of-box token shape. A flat claim name like <c>roles</c> is used when
-    /// the identity provider emits role values as top-level claims (Keycloak with a "User Realm Role"
-    /// mapper, Microsoft Entra ID app roles, AWS Cognito groups, etc.). The dotted form navigates
-    /// into a nested JSON object claim.
+    /// Path within the JWT where the user's role values live. Defaults to the flat <c>roles</c>
+    /// claim, as emitted by Microsoft Entra ID app roles or Keycloak with a "User Realm Role" mapper.
+    /// A dotted path navigates into a nested JSON object claim: set it to <c>realm_access.roles</c>
+    /// for Keycloak's out-of-box token shape, or <c>cognito:groups</c> for AWS Cognito.
     /// </summary>
     public string RolesClaim { get; }