diff --git a/src/ServiceControl.Hosting/Auth/ClaimsPrinicpalExtensionMethods.cs b/src/ServiceControl.Hosting/Auth/ClaimsPrinicpalExtensionMethods.cs deleted file mode 100644 index 0bb4a2ed3f..0000000000 --- a/src/ServiceControl.Hosting/Auth/ClaimsPrinicpalExtensionMethods.cs +++ /dev/null @@ -1,20 +0,0 @@ -namespace ServiceControl.Hosting.Auth -{ - using System; - using System.Security.Claims; - - public static class ClaimsPrinicpalExtensionMethods - { - public static string RequireClaim(this ClaimsPrincipal user, string claimType, string settingName) - { - var value = user.FindFirst(claimType)?.Value; - if (string.IsNullOrEmpty(value)) - { - throw new InvalidOperationException( - $"Authenticated principal is missing the required '{claimType}' claim configured by {settingName}. " + - "Configure the identity provider to emit this claim, or point the setting at the claim the IdP actually emits."); - } - return value; - } - } -} \ No newline at end of file diff --git a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs index de99974652..d59dd26a7c 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs @@ -6,6 +6,7 @@ namespace ServiceControl.Hosting.Auth; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.Logging; using ServiceControl.Infrastructure; using ServiceControl.Infrastructure.Auth; @@ -22,7 +23,8 @@ namespace ServiceControl.Hosting.Auth; /// public sealed class PermissionVerbHandler( IAuthorizationAuditLog auditLog, - OpenIdConnectSettings oidcSettings) + OpenIdConnectSettings oidcSettings, + ILogger logger) : AuthorizationHandler { protected override Task HandleRequirementAsync( @@ -37,8 +39,25 @@ protected override Task HandleRequirementAsync( return Task.CompletedTask; } - var subjectId = context.User.RequireClaim(oidcSettings.SubjectIdClaim, "Authentication.SubjectIdClaim"); - var subjectName = context.User.RequireClaim(oidcSettings.SubjectNameClaim, "Authentication.SubjectNameClaim"); + var subjectId = context.User.FindFirst(oidcSettings.SubjectIdClaim)?.Value; + var subjectName = context.User.FindFirst(oidcSettings.SubjectNameClaim)?.Value; + + // The audit log needs both values to identify the caller. Without them the request is + // forbidden (403), not an unhandled exception (500). + if (string.IsNullOrEmpty(subjectId) || string.IsNullOrEmpty(subjectName)) + { + var (claimType, settingName) = string.IsNullOrEmpty(subjectId) + ? (oidcSettings.SubjectIdClaim, "Authentication.SubjectIdClaim") + : (oidcSettings.SubjectNameClaim, "Authentication.SubjectNameClaim"); + + logger.LogWarning( + "Access denied: the token has no '{ClaimType}' claim, which is configured by {SettingName}. Configure the identity provider to emit this claim, or change the setting to a claim that the identity provider emits", + claimType, settingName); + + context.Fail(new AuthorizationFailureReason(this, $"The token has no '{claimType}' claim, which is configured by {settingName}")); + return Task.CompletedTask; + } + var roles = context.User.FindAll(ClaimTypes.Role).Select(claim => claim.Value).ToArray(); var permission = requirement.Permission; diff --git a/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs b/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs index c6c6aaeb59..511ef65b52 100644 --- a/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs +++ b/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs @@ -77,6 +77,21 @@ public async Task Admins_are_allowed_every_permission_when_role_based_authorizat Assert.That(await DeniedPermissions(host, Authenticated(RolePermissions.Admin), TestContext.CurrentContext.CancellationToken), Is.Empty); } + // The audit log needs the subject ID and name to identify the caller. A token without them must + // get 403, not an unhandled exception that ASP.NET Core turns into 500. + [TestCase("sub")] + [TestCase("preferred_username")] + public async Task Admins_without_a_subject_claim_are_denied_every_permission(string missingClaim) + { + using var host = BuildHost(authenticationEnabled: true, roleBasedAuthorizationEnabled: true); + + var user = new ClaimsPrincipal(new ClaimsIdentity( + Authenticated(RolePermissions.Admin).Claims.Where(claim => claim.Type != missingClaim), + authenticationType: "test")); + + Assert.That(await AllowedPermissions(host, user, TestContext.CurrentContext.CancellationToken), Is.Empty); + } + static readonly ClaimsPrincipal Anonymous = new(new ClaimsIdentity()); static ClaimsPrincipal Authenticated(params string[] roles) =>