🚑️ Require an authenticated user on permission policies when RBAC is disabled (#5979)

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: Dennis van der Stelt <dvdstelt@gmail.com>

Author: ramonsmits

src/ServiceControl.AcceptanceTesting/OpenIdConnect/OpenIdConnectTestConfiguration.cs

@@ -21,7 +21,7 @@ public class OpenIdConnectTestConfiguration(ServiceControlInstanceType instanceT
         /// </summary>
         public OpenIdConnectTestConfiguration WithAuthenticationEnabled()
         {
-            SetEnvironmentVariable("AUTHENTICATION_ENABLED", "true");
+            SetEnvironmentVariable("AUTHENTICATION_ENABLED", bool.TrueString);
             return this;
         }
 
@@ -30,19 +30,19 @@ public OpenIdConnectTestConfiguration WithAuthenticationEnabled()
         /// </summary>
         public OpenIdConnectTestConfiguration WithAuthenticationDisabled()
         {
-            SetEnvironmentVariable("AUTHENTICATION_ENABLED", "false");
+            SetEnvironmentVariable("AUTHENTICATION_ENABLED", bool.FalseString);
             return this;
         }
 
         /// <summary>
         /// Enables role-based authorization. When on, controllers carrying
         /// <c>[Authorize(Policy = Permissions.X)]</c> require the caller's "roles" claim to map to a
-        /// role that grants the permission via <c>RolePermissions</c>. When off, the policy provider
-        /// returns allow-all policies and any authenticated request reaches the controller.
+        /// role that grants the permission via <c>RolePermissions</c>. When off, any authenticated
+        /// request reaches the controller.
         /// </summary>
         public OpenIdConnectTestConfiguration WithRoleBasedAuthorizationEnabled()
         {
-            SetEnvironmentVariable("AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", "true");
+            SetEnvironmentVariable("AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", bool.TrueString);
             return this;
         }
 
@@ -52,7 +52,7 @@ public OpenIdConnectTestConfiguration WithRoleBasedAuthorizationEnabled()
         /// </summary>
         public OpenIdConnectTestConfiguration WithConfigurationValidationDisabled()
         {
-            SetEnvironmentVariable("VALIDATECONFIG", "false");
+            SetEnvironmentVariable("VALIDATECONFIG", bool.FalseString);
             return this;
         }
 

src/ServiceControl.AcceptanceTesting/OpenIdConnect/ProtectedRoutes.cs

@@ -0,0 +1,95 @@
+namespace ServiceControl.AcceptanceTesting.OpenIdConnect;
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Net;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Authorization;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Routing;
+using Microsoft.AspNetCore.Routing.Patterns;
+
+/// <summary>
+/// Finds every route of a running instance that is not marked <c>[AllowAnonymous]</c>, and sends each
+/// one a request without a token. This covers every endpoint, so a new or changed authorization
+/// policy cannot let anonymous callers in without a test failing.
+/// </summary>
+public static class ProtectedRoutes
+{
+    /// <summary>
+    /// Sends a request without a token to every protected route, and returns each route that did not
+    /// answer 401, as "METHOD /path → status".
+    /// </summary>
+    public static async Task<IReadOnlyList<string>> FindRoutesNotRejectingAnonymousRequests(
+        HttpClient client,
+        EndpointDataSource endpointDataSource,
+        CancellationToken cancellationToken = default)
+    {
+        var routes = endpointDataSource.Endpoints
+            .OfType<RouteEndpoint>()
+            .Where(endpoint => endpoint.Metadata.GetMetadata<IAllowAnonymous>() is null)
+            .SelectMany(endpoint => (endpoint.Metadata.GetMetadata<HttpMethodMetadata>()?.HttpMethods ?? [HttpMethods.Get])
+                .Select(method => (Method: method, Path: BuildPath(endpoint.RoutePattern))))
+            .Distinct()
+            .ToList();
+
+        if (routes.Count == 0)
+        {
+            throw new InvalidOperationException("No protected routes found. The endpoint data source is empty or every route allows anonymous access.");
+        }
+
+        var notRejected = new List<string>();
+
+        foreach (var (method, path) in routes)
+        {
+            using var response = await OpenIdConnectAssertions.SendRequestWithoutAuth(client, new HttpMethod(method), path, cancellationToken);
+
+            if (response.StatusCode != HttpStatusCode.Unauthorized)
+            {
+                notRejected.Add($"{method} {path} → {(int)response.StatusCode}");
+            }
+        }
+
+        return notRejected;
+    }
+
+    // Fills each route parameter with a value that satisfies its constraint, so the request matches the
+    // route and reaches the authorization middleware instead of failing with 404.
+    static string BuildPath(RoutePattern pattern)
+    {
+        var segments = pattern.PathSegments.Select(segment => string.Concat(segment.Parts.Select(part => part switch
+        {
+            RoutePatternLiteralPart literal => literal.Content,
+            RoutePatternSeparatorPart separator => separator.Content,
+            RoutePatternParameterPart parameter => SampleValue(parameter),
+            _ => throw new NotSupportedException($"Unsupported route pattern part: {part.GetType().Name}")
+        })));
+
+        return "/" + string.Join('/', segments);
+    }
+
+    static string SampleValue(RoutePatternParameterPart parameter)
+    {
+        var constraints = parameter.ParameterPolicies.Select(policy => policy.Content ?? string.Empty).ToArray();
+
+        if (constraints.Contains("guid", StringComparer.OrdinalIgnoreCase))
+        {
+            return Guid.Empty.ToString();
+        }
+
+        if (constraints.Any(constraint => constraint is "int" or "long"))
+        {
+            return "1";
+        }
+
+        if (constraints.Contains("bool", StringComparer.OrdinalIgnoreCase))
+        {
+            return bool.TrueString;
+        }
+
+        return "x";
+    }
+}

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

@@ -1,10 +1,13 @@
 namespace ServiceControl.AcceptanceTests.Security.OpenIdConnect
 {
+    using System.Collections.Generic;
     using System.Net.Http;
     using System.Security.Claims;
     using System.Threading.Tasks;
     using AcceptanceTesting;
     using AcceptanceTesting.OpenIdConnect;
+    using Microsoft.AspNetCore.Routing;
+    using Microsoft.Extensions.DependencyInjection;
     using NServiceBus.AcceptanceTesting;
     using NUnit.Framework;
 
@@ -235,6 +238,24 @@ public async Task Should_forbid_authenticated_user_lacking_required_permission()
             OpenIdConnectAssertions.AssertForbidden(response);
         }
 
+        [Test]
+        public async Task Should_reject_anonymous_requests_on_every_protected_route()
+        {
+            IReadOnlyList<string> notRejected = null;
+
+            _ = await Define<Context>()
+                .Done(async ctx =>
+                {
+                    notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests(
+                        HttpClient,
+                        ServiceProvider.GetRequiredService<EndpointDataSource>());
+                    return true;
+                })
+                .Run();
+
+            Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token");
+        }
+
         class Context : ScenarioContext;
     }
 }

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

@@ -0,0 +1,90 @@
+namespace ServiceControl.AcceptanceTests.Security.OpenIdConnect
+{
+    using System.Collections.Generic;
+    using System.Net.Http;
+    using System.Threading.Tasks;
+    using AcceptanceTesting;
+    using AcceptanceTesting.OpenIdConnect;
+    using Microsoft.AspNetCore.Routing;
+    using Microsoft.Extensions.DependencyInjection;
+    using NServiceBus.AcceptanceTesting;
+    using NUnit.Framework;
+
+    /// <summary>
+    /// Authentication enabled, role-based authorization disabled (the default). Every route that is not
+    /// [AllowAnonymous] must still require a valid token. Roles are not checked, so a valid token without
+    /// roles is accepted.
+    /// </summary>
+    [NonParallelizable]
+    class When_authentication_is_enabled_without_role_based_authorization : AcceptanceTest
+    {
+        OpenIdConnectTestConfiguration configuration;
+        MockOidcServer mockOidcServer;
+
+        const string TestAudience = "api://test-audience";
+
+        [SetUp]
+        public void ConfigureAuth()
+        {
+            mockOidcServer = new MockOidcServer(audience: TestAudience);
+            mockOidcServer.Start();
+
+            configuration = new OpenIdConnectTestConfiguration(ServiceControlInstanceType.Primary)
+                .WithConfigurationValidationDisabled()
+                .WithAuthenticationEnabled()
+                .WithAuthority(mockOidcServer.Authority)
+                .WithAudience(TestAudience)
+                .WithServicePulseClientId("test-client-id")
+                .WithServicePulseApiScopes("[\"api://test-audience/.default\"]")
+                .WithRequireHttpsMetadata(false);
+        }
+
+        [TearDown]
+        public void CleanupAuth()
+        {
+            configuration?.Dispose();
+            mockOidcServer?.Dispose();
+        }
+
+        [Test]
+        public async Task Should_reject_anonymous_requests_on_every_protected_route()
+        {
+            IReadOnlyList<string> notRejected = null;
+
+            _ = await Define<Context>()
+                .Done(async ctx =>
+                {
+                    notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests(
+                        HttpClient,
+                        ServiceProvider.GetRequiredService<EndpointDataSource>());
+                    return true;
+                })
+                .Run();
+
+            Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token");
+        }
+
+        [Test]
+        public async Task Should_accept_token_without_roles()
+        {
+            HttpResponseMessage response = null;
+
+            _ = await Define<Context>()
+                .Done(async ctx =>
+                {
+                    var tokenWithoutRoles = mockOidcServer.GenerateToken();
+                    response = await OpenIdConnectAssertions.SendRequestWithBearerToken(
+                        HttpClient,
+                        HttpMethod.Get,
+                        "/api/errors",
+                        tokenWithoutRoles);
+                    return response != null;
+                })
+                .Run();
+
+            OpenIdConnectAssertions.AssertAuthenticated(response);
+        }
+
+        class Context : ScenarioContext;
+    }
+}

src/ServiceControl.AcceptanceTests/TestSupport/AcceptanceTest.cs

@@ -23,6 +23,7 @@ abstract class AcceptanceTest : NServiceBusAcceptanceTest, IAcceptanceTestInfras
     {
         public IDomainEvents DomainEvents => serviceControlRunnerBehavior.DomainEvents;
         public HttpClient HttpClient => serviceControlRunnerBehavior.HttpClient;
+        protected IServiceProvider ServiceProvider => serviceControlRunnerBehavior.ServiceProvider;
         public JsonSerializerOptions SerializerOptions => serviceControlRunnerBehavior.SerializerOptions;
         public Settings Settings => serviceControlRunnerBehavior.Settings;
 

src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs

@@ -24,6 +24,7 @@ public ServiceControlComponentBehavior(ITransportIntegration transportToUse, IAc
         }
 
         public HttpClient HttpClient => runner.HttpClient;
+        public IServiceProvider ServiceProvider => runner.ServiceProvider;
         public JsonSerializerOptions SerializerOptions => runner.SerializerOptions;
         public Settings Settings => runner.Settings;
         public IDomainEvents DomainEvents => runner.DomainEvents;

src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs

@@ -44,6 +44,7 @@ public ServiceControlComponentRunner(ITransportIntegration transportToUse, IAcce
         public override string Name { get; } = $"{nameof(ServiceControlComponentRunner)}";
         public Settings Settings { get; private set; }
         public HttpClient HttpClient { get; private set; }
+        public IServiceProvider ServiceProvider { get; private set; }
         public JsonSerializerOptions SerializerOptions => Infrastructure.WebApi.SerializerOptions.Default;
         public IDomainEvents DomainEvents { get; private set; }
 
@@ -152,6 +153,8 @@ async Task InitializeServiceControlCore(ScenarioContext context)
                 await host.StartAsync();
                 DomainEvents = host.Services.GetRequiredService<IDomainEvents>();
                 // Bring this back and look into the base address of the client
+                ServiceProvider = host.Services;
+
                 HttpClient = host.GetTestServer().CreateClient();
             }
         }

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

@@ -1,10 +1,13 @@
 namespace ServiceControl.Audit.AcceptanceTests.Security.OpenIdConnect
 {
+    using System.Collections.Generic;
     using System.Net.Http;
     using System.Security.Claims;
     using System.Threading.Tasks;
     using AcceptanceTesting;
     using AcceptanceTesting.OpenIdConnect;
+    using Microsoft.AspNetCore.Routing;
+    using Microsoft.Extensions.DependencyInjection;
     using NServiceBus.AcceptanceTesting;
     using NUnit.Framework;
 
@@ -174,6 +177,24 @@ public async Task Should_reject_requests_with_wrong_issuer()
             OpenIdConnectAssertions.AssertUnauthorized(response);
         }
 
+        [Test]
+        public async Task Should_reject_anonymous_requests_on_every_protected_route()
+        {
+            IReadOnlyList<string> notRejected = null;
+
+            _ = await Define<Context>()
+                .Done(async ctx =>
+                {
+                    notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests(
+                        HttpClient,
+                        ServiceProvider.GetRequiredService<EndpointDataSource>());
+                    return true;
+                })
+                .Run();
+
+            Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token");
+        }
+
         [Test]
         public async Task Should_allow_anonymous_access_to_root_endpoint()
         {