From 315248bc8b10b8745067702c9f2961febd6d77d6 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 09:43:49 -0800 Subject: [PATCH 1/8] Add logging Signed-off-by: Victor Chang --- src/Authentication/Logging.cs | 8 +++++++- .../Middleware/EndpointAuthorizationMiddleware.cs | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Authentication/Logging.cs b/src/Authentication/Logging.cs index ab0f411..aba4fc4 100644 --- a/src/Authentication/Logging.cs +++ b/src/Authentication/Logging.cs @@ -20,7 +20,13 @@ namespace Monai.Deploy.WorkflowManager.Logging { public static partial class Log { - [LoggerMessage(EventId = 500000, Level = LogLevel.Information, Message = "BYpass authentication.")] + [LoggerMessage(EventId = 500000, Level = LogLevel.Information, Message = "Bypass authentication.")] public static partial void BypassAuthentication(this ILogger logger); + + [LoggerMessage(EventId = 500001, Level = LogLevel.Debug, Message = "User '{user}' attempting to access controller '{controller}'.")] + public static partial void UserAccessingController(this ILogger logger, string? user, string controller); + + [LoggerMessage(EventId = 500002, Level = LogLevel.Debug, Message = "User '{user}' access denied due to allowed permissions: '{permissions}'.")] + public static partial void UserAccessDenied(this ILogger logger, string? user, string? permissions); } } diff --git a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs index b9f2de7..7faeed4 100644 --- a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs +++ b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs @@ -21,6 +21,7 @@ using Microsoft.Extensions.Options; using Monai.Deploy.Security.Authentication.Configurations; using Monai.Deploy.Security.Authentication.Extensions; +using Monai.Deploy.WorkflowManager.Logging; namespace Monai.Deploy.Security.Authentication.Middleware { @@ -54,11 +55,13 @@ public async Task InvokeAsync(HttpContext httpcontext) { if (httpcontext.GetRouteValue("controller") is string controller) { + _logger.UserAccessingController(httpcontext.User.Identity.Name, controller); var validEndpoints = httpcontext.GetValidEndpoints(_options.Value.OpenId!.Claims!.RequiredAdminClaims!, _options.Value.OpenId!.Claims!.RequiredUserClaims!); var result = validEndpoints.Any(e => e.Equals(controller, StringComparison.InvariantCultureIgnoreCase)) || validEndpoints.Contains("all"); if (result is false) { + _logger.UserAccessDenied(httpcontext.User.Identity.Name, string.Join(',', validEndpoints)); httpcontext.Response.StatusCode = (int)HttpStatusCode.Forbidden; await httpcontext.Response.CompleteAsync().ConfigureAwait(false); From 291fa199f4f564d49ed3e23ac6efba0e15395309 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 11:19:24 -0800 Subject: [PATCH 2/8] Remove AddPolicy Signed-off-by: Victor Chang --- .../MonaiAuthenticationExtensions.cs | 24 +------------------ .../EndpointAuthorizationMiddleware.cs | 2 +- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs index fa6fdf3..5646ba2 100644 --- a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs +++ b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs @@ -72,30 +72,8 @@ public static IServiceCollection AddMonaiAuthentication( }; }); - services.AddAuthorization(options => - { - if (configurations.Value.OpenId!.Claims!.RequiredAdminClaims!.Any()) - { - AddPolicy(options, configurations.Value.OpenId!.Claims!.RequiredAdminClaims!, AuthKeys.AdminPolicyName); - } - - if (configurations.Value.OpenId!.Claims!.RequiredUserClaims!.Any()) - { - AddPolicy(options, configurations.Value.OpenId!.Claims!.RequiredUserClaims!, AuthKeys.UserPolicyName); - } - }); - + services.AddAuthorization(); return services; } - - private static void AddPolicy(AuthorizationOptions options, List claims, string policyName) - { - foreach (var dict in claims) - { - options.AddPolicy(policyName, policy => policy - .RequireAuthenticatedUser() - .RequireClaim("user_roles", dict.UserRoles!)); - } - } } } diff --git a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs index 7faeed4..1f74e08 100644 --- a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs +++ b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs @@ -45,7 +45,7 @@ public async Task InvokeAsync(HttpContext httpcontext) { if (_options.Value.BypassAuth(_logger)) { - await _next(httpcontext); + await _next(httpcontext).ConfigureAwait(false); return; } From efe5bd80618b3f2e7ef0dc060343bc8687b28e48 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 13:55:25 -0800 Subject: [PATCH 3/8] Change configuration structure Signed-off-by: Victor Chang --- .../Configurations/AuthenticationOptions.cs | 2 +- .../{Claims.cs => ClaimMappings.cs} | 21 +++++++++++-------- .../Configurations/OpenIdOptions.cs | 4 ++-- src/Authentication/Extensions/AuthKeys.cs | 1 - .../Extensions/HttpContextExtension.cs | 6 +++--- .../EndpointAuthorizationMiddleware.cs | 2 +- .../Tests/MockJwtTokenHandler.cs | 5 ++--- src/Authentication/Tests/test.auth.json | 17 +++++++++------ src/Authentication/example.json | 16 ++++++++------ 9 files changed, 42 insertions(+), 32 deletions(-) rename src/Authentication/Configurations/{Claims.cs => ClaimMappings.cs} (59%) diff --git a/src/Authentication/Configurations/AuthenticationOptions.cs b/src/Authentication/Configurations/AuthenticationOptions.cs index 6aee72c..ff8b555 100644 --- a/src/Authentication/Configurations/AuthenticationOptions.cs +++ b/src/Authentication/Configurations/AuthenticationOptions.cs @@ -44,7 +44,7 @@ public bool BypassAuth(ILogger logger) { throw new InvalidOperationException("OpenId configuration is invalid."); } - if (OpenId.Claims is null || OpenId.Claims.RequiredUserClaims!.IsNullOrEmpty() || OpenId.Claims.RequiredAdminClaims!.IsNullOrEmpty()) + if (OpenId.Claims is null || OpenId.Claims.UserClaims!.IsNullOrEmpty() || OpenId.Claims.AdminClaims!.IsNullOrEmpty()) { throw new InvalidOperationException("No claims defined for OpenId."); } diff --git a/src/Authentication/Configurations/Claims.cs b/src/Authentication/Configurations/ClaimMappings.cs similarity index 59% rename from src/Authentication/Configurations/Claims.cs rename to src/Authentication/Configurations/ClaimMappings.cs index 057ec06..d63e881 100644 --- a/src/Authentication/Configurations/Claims.cs +++ b/src/Authentication/Configurations/ClaimMappings.cs @@ -18,21 +18,24 @@ namespace Monai.Deploy.Security.Authentication.Configurations { - public class Claims + public class ClaimMappings { - [ConfigurationKeyName("RequiredUserClaims")] - public List? RequiredUserClaims { get; set; } + [ConfigurationKeyName("UserClaims")] + public List? UserClaims { get; set; } - [ConfigurationKeyName("RequiredAdminClaims")] - public List? RequiredAdminClaims { get; set; } + [ConfigurationKeyName("AdminClaims")] + public List? AdminClaims { get; set; } } - public class Claim + public class ClaimMapping { - [ConfigurationKeyName("user_roles")] - public string? UserRoles { get; set; } + [ConfigurationKeyName("claim")] + public string Claim { get; set; } = string.Empty; + + [ConfigurationKeyName("role")] + public string Role { get; set; } = string.Empty; [ConfigurationKeyName("endpoints")] - public List? Endpoints { get; set; } + public List? Endpoints { get; set; } = default; } } diff --git a/src/Authentication/Configurations/OpenIdOptions.cs b/src/Authentication/Configurations/OpenIdOptions.cs index df77d49..f35358c 100644 --- a/src/Authentication/Configurations/OpenIdOptions.cs +++ b/src/Authentication/Configurations/OpenIdOptions.cs @@ -29,8 +29,8 @@ public class OpenIdOptions [ConfigurationKeyName("ClientId")] public string? ClientId { get; set; } - [ConfigurationKeyName("Claims")] - public Claims? Claims { get; set; } + [ConfigurationKeyName("ClaimMappings")] + public ClaimMappings? Claims { get; set; } [ConfigurationKeyName("Audiences")] public IList? Audiences { get; set; } diff --git a/src/Authentication/Extensions/AuthKeys.cs b/src/Authentication/Extensions/AuthKeys.cs index 799dbdc..65af5df 100644 --- a/src/Authentication/Extensions/AuthKeys.cs +++ b/src/Authentication/Extensions/AuthKeys.cs @@ -25,6 +25,5 @@ public static class AuthKeys // Configuration Keys public const string OpenId = "OpenId"; - public const string UserRoles = "user_roles"; } } diff --git a/src/Authentication/Extensions/HttpContextExtension.cs b/src/Authentication/Extensions/HttpContextExtension.cs index 610bd8e..4407e81 100644 --- a/src/Authentication/Extensions/HttpContextExtension.cs +++ b/src/Authentication/Extensions/HttpContextExtension.cs @@ -27,14 +27,14 @@ public static class HttpContextExtension /// /// /// - public static List GetValidEndpoints(this HttpContext httpcontext, List adminClaims, List userClaims) + public static List GetValidEndpoints(this HttpContext httpcontext, List adminClaims, List userClaims) { Guard.Against.Null(adminClaims); Guard.Against.Null(userClaims); foreach (var claim in adminClaims!) { - if (httpcontext.User.HasClaim(AuthKeys.UserRoles, claim.UserRoles!)) + if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) { return new List { "all" }; } @@ -42,7 +42,7 @@ public static List GetValidEndpoints(this HttpContext httpcontext, List< foreach (var claim in userClaims!) { - if (httpcontext.User.HasClaim(AuthKeys.UserRoles, claim.UserRoles!)) + if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) { return claim.Endpoints!; } diff --git a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs index 1f74e08..35390e9 100644 --- a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs +++ b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs @@ -56,7 +56,7 @@ public async Task InvokeAsync(HttpContext httpcontext) if (httpcontext.GetRouteValue("controller") is string controller) { _logger.UserAccessingController(httpcontext.User.Identity.Name, controller); - var validEndpoints = httpcontext.GetValidEndpoints(_options.Value.OpenId!.Claims!.RequiredAdminClaims!, _options.Value.OpenId!.Claims!.RequiredUserClaims!); + var validEndpoints = httpcontext.GetValidEndpoints(_options.Value.OpenId!.Claims!.AdminClaims!, _options.Value.OpenId!.Claims!.UserClaims!); var result = validEndpoints.Any(e => e.Equals(controller, StringComparison.InvariantCultureIgnoreCase)) || validEndpoints.Contains("all"); if (result is false) diff --git a/src/Authentication/Tests/MockJwtTokenHandler.cs b/src/Authentication/Tests/MockJwtTokenHandler.cs index ece844c..30901ea 100644 --- a/src/Authentication/Tests/MockJwtTokenHandler.cs +++ b/src/Authentication/Tests/MockJwtTokenHandler.cs @@ -17,7 +17,6 @@ using System.IdentityModel.Tokens.Jwt; using System.Text; using Microsoft.IdentityModel.Tokens; -using Monai.Deploy.Security.Authentication.Extensions; using Claim = System.Security.Claims.Claim; namespace Monai.Deploy.Security.Authentication.Tests @@ -30,7 +29,7 @@ public static class MockJwtTokenHandler public static SecurityKey SecurityKey { get; } public static SigningCredentials SigningCredentials { get; } - private static readonly JwtSecurityTokenHandler TokenHandler = new JwtSecurityTokenHandler(); + private static readonly JwtSecurityTokenHandler TokenHandler = new(); static MockJwtTokenHandler() { @@ -40,7 +39,7 @@ static MockJwtTokenHandler() public static string GenerateJwtToken(string role) { - var claims = new[] { new Claim(AuthKeys.UserRoles, role) }; + var claims = new[] { new Claim("user_roles", role) }; return TokenHandler.WriteToken(new JwtSecurityToken(Issuer, "monai-app", claims, null, DateTime.UtcNow.AddMinutes(20), SigningCredentials)); } } diff --git a/src/Authentication/Tests/test.auth.json b/src/Authentication/Tests/test.auth.json index 3fb1c0b..630a0f5 100644 --- a/src/Authentication/Tests/test.auth.json +++ b/src/Authentication/Tests/test.auth.json @@ -6,19 +6,24 @@ "ServerRealmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", "Audiences": [ "monai-app" ], "ClientId": "monai-app-test", - "Claims": { - "RequiredUserClaims": [ + "ClaimMappings": { + "UserClaims": [ { - "user_roles": "role-with-test", + "claim": "user_roles", + "role": "role-with-test", "endpoints": [ "test" ] }, { - "user_roles": "role-without-test", + "claim": "user_roles", + "roles": "role-without-test", "endpoints": [ "no-test" ] } ], - "RequiredAdminClaims": [ - { "user_roles": "monai-role-admin" } + "AdminClaims": [ + { + "claim": "user_roles", + "role": "monai-role-admin" + } ] } } diff --git a/src/Authentication/example.json b/src/Authentication/example.json index aed3e39..8696fc1 100644 --- a/src/Authentication/example.json +++ b/src/Authentication/example.json @@ -6,19 +6,23 @@ "ServerRealmKey": "EncryptionKey", "ClientId": "monai-app", "Audiences": [ "monai-deploy" ], - "Claims": { - "RequiredUserClaims": [ + "ClaimMappings": { + "UserClaims": [ { - "user_roles": "monai-deploy-users", - "endpoints": [ "payloads", "workflows", "workflowinstances", "tasks" ] + "claim": "user_roles", + "role": "monai-deploy-user", + "endpoints": [ "test" ] }, { "user_roles": "pacs-admins", "endpoints": [ "config" ] } ], - "RequiredAdminClaims": [ - { "user_roles": "monai-role-admin" } + "AdminClaims": [ + { + "claim": "user_roles", + "role": "monai-role-admin" + } ] } } From 5da612500f404c12f147a5c301ca6642f04cdd07 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 14:21:04 -0800 Subject: [PATCH 4/8] Add test case with expired token Signed-off-by: Victor Chang --- .../Extensions/HttpContextExtension.cs | 1 - .../Extensions/MonaiAuthenticationExtensions.cs | 1 - .../EndpointAuthorizationMiddlewareTest.cs | 17 +++++++++++++++++ src/Authentication/Tests/MockJwtTokenHandler.cs | 4 ++-- src/Authentication/Tests/test.auth.json | 2 +- src/Authentication/Tests/test.bypassd.json | 2 +- src/Authentication/Tests/test.emptyopenid.json | 2 +- src/Authentication/Tests/test.noauth.json | 2 +- src/Authentication/example.json | 2 +- 9 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/Authentication/Extensions/HttpContextExtension.cs b/src/Authentication/Extensions/HttpContextExtension.cs index 4407e81..f82c775 100644 --- a/src/Authentication/Extensions/HttpContextExtension.cs +++ b/src/Authentication/Extensions/HttpContextExtension.cs @@ -46,7 +46,6 @@ public static List GetValidEndpoints(this HttpContext httpcontext, List< { return claim.Endpoints!; } - } return new List(); diff --git a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs index 5646ba2..495f093 100644 --- a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs +++ b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs @@ -17,7 +17,6 @@ using System.Text; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.JwtBearer; -using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; diff --git a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs index 518ddd4..c8a3c3a 100644 --- a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs +++ b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs @@ -109,6 +109,23 @@ public async Task GivenConfigurationFileWithOpenIdConfigured_WhenUserIsAuthentic Assert.Equal(HttpStatusCode.Forbidden, responseMessage.StatusCode); } + [Theory] + [InlineData("role-with-test")] + public async Task GivenConfigurationFileWithOpenIdConfigured_WhenUserProvidesAnExpiredToken_ExpectToDenyRequest(string role) + { + using var host = await new HostBuilder().ConfigureWebHost(SetupWebServer("test.auth.json")).StartAsync().ConfigureAwait(false); + + var server = host.GetTestServer(); + server.BaseAddress = new Uri("https://example.com/"); + + var token = MockJwtTokenHandler.GenerateJwtToken(role, -5); + + var client = server.CreateClient(); + client.DefaultRequestHeaders.Add("Authorization", $"{JwtBearerDefaults.AuthenticationScheme} {token}"); + var responseMessage = await client.GetAsync("api/Test").ConfigureAwait(false); + + Assert.Equal(HttpStatusCode.Unauthorized, responseMessage.StatusCode); + } private static Action SetupWebServer(string configFile) => webBuilder => { diff --git a/src/Authentication/Tests/MockJwtTokenHandler.cs b/src/Authentication/Tests/MockJwtTokenHandler.cs index 30901ea..5719ea5 100644 --- a/src/Authentication/Tests/MockJwtTokenHandler.cs +++ b/src/Authentication/Tests/MockJwtTokenHandler.cs @@ -37,10 +37,10 @@ static MockJwtTokenHandler() SigningCredentials = new SigningCredentials(SecurityKey, SecurityAlgorithms.HmacSha256Signature); } - public static string GenerateJwtToken(string role) + public static string GenerateJwtToken(string role, int expiresInMinutes = 5) { var claims = new[] { new Claim("user_roles", role) }; - return TokenHandler.WriteToken(new JwtSecurityToken(Issuer, "monai-app", claims, null, DateTime.UtcNow.AddMinutes(20), SigningCredentials)); + return TokenHandler.WriteToken(new JwtSecurityToken(Issuer, "monai-app", claims, null, DateTime.UtcNow.AddMinutes(expiresInMinutes), SigningCredentials)); } } } diff --git a/src/Authentication/Tests/test.auth.json b/src/Authentication/Tests/test.auth.json index 630a0f5..5c0885b 100644 --- a/src/Authentication/Tests/test.auth.json +++ b/src/Authentication/Tests/test.auth.json @@ -28,4 +28,4 @@ } } } -} +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.bypassd.json b/src/Authentication/Tests/test.bypassd.json index a1d2b3f..577ca21 100644 --- a/src/Authentication/Tests/test.bypassd.json +++ b/src/Authentication/Tests/test.bypassd.json @@ -2,4 +2,4 @@ "MonaiDeployAuthentication": { "BypassAuthentication": true } -} +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.emptyopenid.json b/src/Authentication/Tests/test.emptyopenid.json index 59817be..2265f8c 100644 --- a/src/Authentication/Tests/test.emptyopenid.json +++ b/src/Authentication/Tests/test.emptyopenid.json @@ -3,4 +3,4 @@ "BypassAuthentication": false, "OpenId": {} } -} +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.noauth.json b/src/Authentication/Tests/test.noauth.json index 0f530c1..8593c62 100644 --- a/src/Authentication/Tests/test.noauth.json +++ b/src/Authentication/Tests/test.noauth.json @@ -1,2 +1,2 @@ { -} +} \ No newline at end of file diff --git a/src/Authentication/example.json b/src/Authentication/example.json index 8696fc1..d5d7e4e 100644 --- a/src/Authentication/example.json +++ b/src/Authentication/example.json @@ -27,4 +27,4 @@ } } } -} +} \ No newline at end of file From 32e203b21610400c28b1d615029db21414537e9f Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 14:44:49 -0800 Subject: [PATCH 5/8] Log user claims Signed-off-by: Victor Chang --- .../Extensions/HttpContextExtension.cs | 7 ++++- src/Authentication/Logging.cs | 5 +++- .../EndpointAuthorizationMiddleware.cs | 26 +++++++++---------- src/Authentication/example.json | 2 +- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/Authentication/Extensions/HttpContextExtension.cs b/src/Authentication/Extensions/HttpContextExtension.cs index f82c775..4fba559 100644 --- a/src/Authentication/Extensions/HttpContextExtension.cs +++ b/src/Authentication/Extensions/HttpContextExtension.cs @@ -16,6 +16,9 @@ using Ardalis.GuardClauses; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; +using Monai.Deploy.Security.Authentication.Middleware; +using Monai.Deploy.WorkflowManager.Logging; namespace Monai.Deploy.Security.Authentication.Extensions { @@ -27,7 +30,7 @@ public static class HttpContextExtension /// /// /// - public static List GetValidEndpoints(this HttpContext httpcontext, List adminClaims, List userClaims) + public static List GetValidEndpoints(this HttpContext httpcontext, ILogger logger, List adminClaims, List userClaims) { Guard.Against.Null(adminClaims); Guard.Against.Null(userClaims); @@ -36,6 +39,7 @@ public static List GetValidEndpoints(this HttpContext httpcontext, List< { if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) { + logger.UserClaimFound(claim.Claim, claim.Role); return new List { "all" }; } } @@ -44,6 +48,7 @@ public static List GetValidEndpoints(this HttpContext httpcontext, List< { if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) { + logger.UserClaimFound(claim.Claim, claim.Role); return claim.Endpoints!; } } diff --git a/src/Authentication/Logging.cs b/src/Authentication/Logging.cs index aba4fc4..0d52a3f 100644 --- a/src/Authentication/Logging.cs +++ b/src/Authentication/Logging.cs @@ -26,7 +26,10 @@ public static partial class Log [LoggerMessage(EventId = 500001, Level = LogLevel.Debug, Message = "User '{user}' attempting to access controller '{controller}'.")] public static partial void UserAccessingController(this ILogger logger, string? user, string controller); - [LoggerMessage(EventId = 500002, Level = LogLevel.Debug, Message = "User '{user}' access denied due to allowed permissions: '{permissions}'.")] + [LoggerMessage(EventId = 500002, Level = LogLevel.Debug, Message = "User '{user}' access denied due to limited permissions: '{permissions}'.")] public static partial void UserAccessDenied(this ILogger logger, string? user, string? permissions); + + [LoggerMessage(EventId = 500003, Level = LogLevel.Debug, Message = "User claim {claim}={value}.")] + public static partial void UserClaimFound(this ILogger logger, string? claim, string? value); } } diff --git a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs index 35390e9..34c46ac 100644 --- a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs +++ b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs @@ -41,39 +41,39 @@ public EndpointAuthorizationMiddleware(RequestDelegate next, IOptions e.Equals(controller, StringComparison.InvariantCultureIgnoreCase)) || validEndpoints.Contains("all"); if (result is false) { - _logger.UserAccessDenied(httpcontext.User.Identity.Name, string.Join(',', validEndpoints)); - httpcontext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + _logger.UserAccessDenied(httpContext.User.Identity.Name, string.Join(',', validEndpoints)); + httpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; - await httpcontext.Response.CompleteAsync().ConfigureAwait(false); + await httpContext.Response.CompleteAsync().ConfigureAwait(false); return; } } - await _next(httpcontext).ConfigureAwait(false); + await _next(httpContext).ConfigureAwait(false); } else { - httpcontext.Response.StatusCode = (int)HttpStatusCode.Unauthorized; + httpContext.Response.StatusCode = (int)HttpStatusCode.Unauthorized; } } } diff --git a/src/Authentication/example.json b/src/Authentication/example.json index d5d7e4e..8696fc1 100644 --- a/src/Authentication/example.json +++ b/src/Authentication/example.json @@ -27,4 +27,4 @@ } } } -} \ No newline at end of file +} From 7bbdcef5e914fc15ff887d34136ac4586cfefa7b Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 15:00:00 -0800 Subject: [PATCH 6/8] Log user claims Signed-off-by: Victor Chang --- .../Configurations/AuthenticationOptions.cs | 4 +-- .../Configurations/ClaimMappings.cs | 8 ++--- .../Configurations/OpenIdOptions.cs | 10 +++--- .../Extensions/HttpContextExtension.cs | 31 +++++++++++++------ src/Authentication/Logging.cs | 5 ++- .../EndpointAuthorizationMiddleware.cs | 2 +- src/Authentication/Tests/test.auth.json | 24 +++++++------- src/Authentication/example.json | 7 +++-- 8 files changed, 54 insertions(+), 37 deletions(-) diff --git a/src/Authentication/Configurations/AuthenticationOptions.cs b/src/Authentication/Configurations/AuthenticationOptions.cs index ff8b555..ac8d9b9 100644 --- a/src/Authentication/Configurations/AuthenticationOptions.cs +++ b/src/Authentication/Configurations/AuthenticationOptions.cs @@ -24,10 +24,10 @@ namespace Monai.Deploy.Security.Authentication.Configurations { public class AuthenticationOptions { - [ConfigurationKeyName("BypassAuthentication")] + [ConfigurationKeyName("bypassAuthentication")] public bool? BypassAuthentication { get; set; } - [ConfigurationKeyName("OpenId")] + [ConfigurationKeyName("openId")] public OpenIdOptions? OpenId { get; set; } public bool BypassAuth(ILogger logger) diff --git a/src/Authentication/Configurations/ClaimMappings.cs b/src/Authentication/Configurations/ClaimMappings.cs index d63e881..684db57 100644 --- a/src/Authentication/Configurations/ClaimMappings.cs +++ b/src/Authentication/Configurations/ClaimMappings.cs @@ -20,10 +20,10 @@ namespace Monai.Deploy.Security.Authentication.Configurations { public class ClaimMappings { - [ConfigurationKeyName("UserClaims")] + [ConfigurationKeyName("userClaims")] public List? UserClaims { get; set; } - [ConfigurationKeyName("AdminClaims")] + [ConfigurationKeyName("adminClaims")] public List? AdminClaims { get; set; } } @@ -32,8 +32,8 @@ public class ClaimMapping [ConfigurationKeyName("claim")] public string Claim { get; set; } = string.Empty; - [ConfigurationKeyName("role")] - public string Role { get; set; } = string.Empty; + [ConfigurationKeyName("roles")] + public List Roles { get; set; } = new List(); [ConfigurationKeyName("endpoints")] public List? Endpoints { get; set; } = default; diff --git a/src/Authentication/Configurations/OpenIdOptions.cs b/src/Authentication/Configurations/OpenIdOptions.cs index f35358c..081a9d0 100644 --- a/src/Authentication/Configurations/OpenIdOptions.cs +++ b/src/Authentication/Configurations/OpenIdOptions.cs @@ -20,19 +20,19 @@ namespace Monai.Deploy.Security.Authentication.Configurations { public class OpenIdOptions { - [ConfigurationKeyName("ServerRealm")] + [ConfigurationKeyName("realm")] public string? ServerRealm { get; set; } - [ConfigurationKeyName("ServerRealmKey")] + [ConfigurationKeyName("realmKey")] public string? ServerRealmKey { get; set; } - [ConfigurationKeyName("ClientId")] + [ConfigurationKeyName("clientId")] public string? ClientId { get; set; } - [ConfigurationKeyName("ClaimMappings")] + [ConfigurationKeyName("claimMappings")] public ClaimMappings? Claims { get; set; } - [ConfigurationKeyName("Audiences")] + [ConfigurationKeyName("audiences")] public IList? Audiences { get; set; } } } diff --git a/src/Authentication/Extensions/HttpContextExtension.cs b/src/Authentication/Extensions/HttpContextExtension.cs index 4fba559..9ae3dec 100644 --- a/src/Authentication/Extensions/HttpContextExtension.cs +++ b/src/Authentication/Extensions/HttpContextExtension.cs @@ -27,33 +27,46 @@ public static class HttpContextExtension /// /// Gets endpoints specified in config for roles in claims. /// - /// + /// /// /// - public static List GetValidEndpoints(this HttpContext httpcontext, ILogger logger, List adminClaims, List userClaims) + public static List GetValidEndpoints(this HttpContext httpContext, ILogger logger, List adminClaims, List userClaims) { Guard.Against.Null(adminClaims); Guard.Against.Null(userClaims); + foreach (var claim in httpContext.User.Claims) + { + logger.UserClaimFound(claim.Type, claim.Value); + + } + foreach (var claim in adminClaims!) { - if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) + foreach (var role in claim.Roles) { - logger.UserClaimFound(claim.Claim, claim.Role); - return new List { "all" }; + logger.CheckingUserClaim(claim.Claim, role); + if (httpContext.User.HasClaim(claim.Claim, role)) + { + return new List { "*" }; + } } } + var endpoints = new List(); foreach (var claim in userClaims!) { - if (httpcontext.User.HasClaim(claim.Claim, claim.Role)) + foreach (var role in claim.Roles) { - logger.UserClaimFound(claim.Claim, claim.Role); - return claim.Endpoints!; + logger.CheckingUserClaim(claim.Claim, role); + if (httpContext.User.HasClaim(claim.Claim, role)) + { + endpoints.AddRange(claim.Endpoints!); + } } } - return new List(); + return endpoints.Distinct().ToList(); } } } diff --git a/src/Authentication/Logging.cs b/src/Authentication/Logging.cs index 0d52a3f..2766669 100644 --- a/src/Authentication/Logging.cs +++ b/src/Authentication/Logging.cs @@ -29,7 +29,10 @@ public static partial class Log [LoggerMessage(EventId = 500002, Level = LogLevel.Debug, Message = "User '{user}' access denied due to limited permissions: '{permissions}'.")] public static partial void UserAccessDenied(this ILogger logger, string? user, string? permissions); - [LoggerMessage(EventId = 500003, Level = LogLevel.Debug, Message = "User claim {claim}={value}.")] + [LoggerMessage(EventId = 500003, Level = LogLevel.Trace, Message = "User claim {claim}={value}.")] public static partial void UserClaimFound(this ILogger logger, string? claim, string? value); + + [LoggerMessage(EventId = 500004, Level = LogLevel.Trace, Message = "Checking user claim {claim}={value}.")] + public static partial void CheckingUserClaim(this ILogger logger, string? claim, string? value); } } diff --git a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs index 34c46ac..9b2fc1e 100644 --- a/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs +++ b/src/Authentication/Middleware/EndpointAuthorizationMiddleware.cs @@ -57,7 +57,7 @@ public async Task InvokeAsync(HttpContext httpContext) { _logger.UserAccessingController(httpContext.User.Identity.Name, controller); var validEndpoints = httpContext.GetValidEndpoints(_logger, _options.Value.OpenId!.Claims!.AdminClaims!, _options.Value.OpenId!.Claims!.UserClaims!); - var result = validEndpoints.Any(e => e.Equals(controller, StringComparison.InvariantCultureIgnoreCase)) || validEndpoints.Contains("all"); + var result = validEndpoints.Any(e => e.Equals(controller, StringComparison.InvariantCultureIgnoreCase)) || validEndpoints.Contains("*"); if (result is false) { diff --git a/src/Authentication/Tests/test.auth.json b/src/Authentication/Tests/test.auth.json index 5c0885b..c395e87 100644 --- a/src/Authentication/Tests/test.auth.json +++ b/src/Authentication/Tests/test.auth.json @@ -1,28 +1,28 @@ { "MonaiDeployAuthentication": { - "BypassAuthentication": false, - "OpenId": { - "ServerRealm": "TEST-REALM", - "ServerRealmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", - "Audiences": [ "monai-app" ], - "ClientId": "monai-app-test", - "ClaimMappings": { - "UserClaims": [ + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ { "claim": "user_roles", - "role": "role-with-test", + "roles": [ "role-with-test" ], "endpoints": [ "test" ] }, { "claim": "user_roles", - "roles": "role-without-test", + "roles": [ "role-without-test" ], "endpoints": [ "no-test" ] } ], - "AdminClaims": [ + "adminClaims": [ { "claim": "user_roles", - "role": "monai-role-admin" + "roles": [ "monai-role-admin" ] } ] } diff --git a/src/Authentication/example.json b/src/Authentication/example.json index 8696fc1..feb13c9 100644 --- a/src/Authentication/example.json +++ b/src/Authentication/example.json @@ -10,18 +10,19 @@ "UserClaims": [ { "claim": "user_roles", - "role": "monai-deploy-user", + "roles": [ "monai-deploy-user" ], "endpoints": [ "test" ] }, { - "user_roles": "pacs-admins", + "user_roles": "user_roles", + "roles": [ "pacs-admins" ], "endpoints": [ "config" ] } ], "AdminClaims": [ { "claim": "user_roles", - "role": "monai-role-admin" + "role": [ "monai-role-admin" ] } ] } From afaca88429257912bc3391ebd341952e098f0583 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 16:07:58 -0800 Subject: [PATCH 7/8] Set RoleClaimType override Signed-off-by: Victor Chang --- .../Configurations/AuthenticationOptions.cs | 14 ++++----- .../Configurations/ClaimMappings.cs | 8 ++--- .../Configurations/OpenIdOptions.cs | 7 +++-- .../Extensions/HttpContextExtension.cs | 12 +++---- .../MonaiAuthenticationExtensions.cs | 9 +++--- .../EndpointAuthorizationMiddlewareTest.cs | 4 +++ .../Tests/test.auth-noclaims.json | 12 +++++++ .../Tests/test.auth-noclientid.json | 31 +++++++++++++++++++ .../Tests/test.auth-norealm.json | 31 +++++++++++++++++++ .../Tests/test.auth-norealmkey.json | 31 +++++++++++++++++++ src/Authentication/Tests/test.auth.json | 13 ++++---- src/Authentication/example.json | 12 +++---- 12 files changed, 149 insertions(+), 35 deletions(-) create mode 100644 src/Authentication/Tests/test.auth-noclaims.json create mode 100644 src/Authentication/Tests/test.auth-noclientid.json create mode 100644 src/Authentication/Tests/test.auth-norealm.json create mode 100644 src/Authentication/Tests/test.auth-norealmkey.json diff --git a/src/Authentication/Configurations/AuthenticationOptions.cs b/src/Authentication/Configurations/AuthenticationOptions.cs index ac8d9b9..428188a 100644 --- a/src/Authentication/Configurations/AuthenticationOptions.cs +++ b/src/Authentication/Configurations/AuthenticationOptions.cs @@ -42,23 +42,23 @@ public bool BypassAuth(ILogger logger) if (OpenId is null) { - throw new InvalidOperationException("OpenId configuration is invalid."); + throw new InvalidOperationException("openId configuration is invalid."); } if (OpenId.Claims is null || OpenId.Claims.UserClaims!.IsNullOrEmpty() || OpenId.Claims.AdminClaims!.IsNullOrEmpty()) { - throw new InvalidOperationException("No claims defined for OpenId."); + throw new InvalidOperationException("No claimMappings defined for OpenId."); } if (string.IsNullOrWhiteSpace(OpenId.ClientId)) { - throw new InvalidOperationException("No ClientId defined for OpenId."); + throw new InvalidOperationException("No clientId defined for OpenId."); } - if (string.IsNullOrWhiteSpace(OpenId.ServerRealmKey)) + if (string.IsNullOrWhiteSpace(OpenId.RealmKey)) { - throw new InvalidOperationException("No ServerRealmKey defined for OpenId."); + throw new InvalidOperationException("No realmKey defined for OpenId."); } - if (string.IsNullOrWhiteSpace(OpenId.ServerRealm)) + if (string.IsNullOrWhiteSpace(OpenId.Realm)) { - throw new InvalidOperationException("No ServerRealm defined for OpenId."); + throw new InvalidOperationException("No realm defined for OpenId."); } return false; diff --git a/src/Authentication/Configurations/ClaimMappings.cs b/src/Authentication/Configurations/ClaimMappings.cs index 684db57..9e4b94b 100644 --- a/src/Authentication/Configurations/ClaimMappings.cs +++ b/src/Authentication/Configurations/ClaimMappings.cs @@ -29,11 +29,11 @@ public class ClaimMappings public class ClaimMapping { - [ConfigurationKeyName("claim")] - public string Claim { get; set; } = string.Empty; + [ConfigurationKeyName("claimType")] + public string ClaimType { get; set; } = string.Empty; - [ConfigurationKeyName("roles")] - public List Roles { get; set; } = new List(); + [ConfigurationKeyName("claimValues")] + public List ClaimValues { get; set; } = new List(); [ConfigurationKeyName("endpoints")] public List? Endpoints { get; set; } = default; diff --git a/src/Authentication/Configurations/OpenIdOptions.cs b/src/Authentication/Configurations/OpenIdOptions.cs index 081a9d0..695dd0f 100644 --- a/src/Authentication/Configurations/OpenIdOptions.cs +++ b/src/Authentication/Configurations/OpenIdOptions.cs @@ -21,10 +21,10 @@ namespace Monai.Deploy.Security.Authentication.Configurations public class OpenIdOptions { [ConfigurationKeyName("realm")] - public string? ServerRealm { get; set; } + public string? Realm { get; set; } [ConfigurationKeyName("realmKey")] - public string? ServerRealmKey { get; set; } + public string? RealmKey { get; set; } [ConfigurationKeyName("clientId")] public string? ClientId { get; set; } @@ -34,5 +34,8 @@ public class OpenIdOptions [ConfigurationKeyName("audiences")] public IList? Audiences { get; set; } + + [ConfigurationKeyName("roleClaimType")] + public string RoleClaimType { get; set; } = "roles"; } } diff --git a/src/Authentication/Extensions/HttpContextExtension.cs b/src/Authentication/Extensions/HttpContextExtension.cs index 9ae3dec..adc6188 100644 --- a/src/Authentication/Extensions/HttpContextExtension.cs +++ b/src/Authentication/Extensions/HttpContextExtension.cs @@ -43,10 +43,10 @@ public static List GetValidEndpoints(this HttpContext httpContext, ILogg foreach (var claim in adminClaims!) { - foreach (var role in claim.Roles) + foreach (var role in claim.ClaimValues) { - logger.CheckingUserClaim(claim.Claim, role); - if (httpContext.User.HasClaim(claim.Claim, role)) + logger.CheckingUserClaim(claim.ClaimType, role); + if (httpContext.User.HasClaim(claim.ClaimType, role)) { return new List { "*" }; } @@ -56,10 +56,10 @@ public static List GetValidEndpoints(this HttpContext httpContext, ILogg var endpoints = new List(); foreach (var claim in userClaims!) { - foreach (var role in claim.Roles) + foreach (var role in claim.ClaimValues) { - logger.CheckingUserClaim(claim.Claim, role); - if (httpContext.User.HasClaim(claim.Claim, role)) + logger.CheckingUserClaim(claim.ClaimType, role); + if (httpContext.User.HasClaim(claim.ClaimType, role)) { endpoints.AddRange(claim.Endpoints!); } diff --git a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs index 495f093..85c0474 100644 --- a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs +++ b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs @@ -55,14 +55,15 @@ public static IServiceCollection AddMonaiAuthentication( }) .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, AuthKeys.OpenId, options => { - options.Authority = configurations.Value.OpenId!.ServerRealm; - options.Audience = configurations.Value.OpenId!.ServerRealm; + options.Authority = configurations.Value.OpenId!.Realm; + options.Audience = configurations.Value.OpenId!.Realm; options.RequireHttpsMetadata = false; options.TokenValidationParameters = new TokenValidationParameters { - IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(configurations.Value.OpenId!.ServerRealmKey!)), - ValidIssuer = configurations.Value.OpenId.ServerRealm, + IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(configurations.Value.OpenId!.RealmKey!)), + RoleClaimType = configurations.Value.OpenId.RoleClaimType, + ValidIssuer = configurations.Value.OpenId.Realm, ValidAudiences = configurations.Value.OpenId.Audiences, ValidateIssuerSigningKey = true, ValidateIssuer = true, diff --git a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs index c8a3c3a..c2233c7 100644 --- a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs +++ b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs @@ -28,6 +28,10 @@ public partial class EndpointAuthorizationMiddlewareTest [Theory] [InlineData("test.noauth.json")] [InlineData("test.emptyopenid.json")] + [InlineData("test.auth-noclaims.json")] + [InlineData("test.auth-noclientid.json")] + [InlineData("test.auth-norealm.json")] + [InlineData("test.auth-norealmkey.json")] public async Task GivenConfigurationFilesIsBad_ExpectExceptionToBeThrown(string configFile) { await Assert.ThrowsAsync(async () => diff --git a/src/Authentication/Tests/test.auth-noclaims.json b/src/Authentication/Tests/test.auth-noclaims.json new file mode 100644 index 0000000..65c04da --- /dev/null +++ b/src/Authentication/Tests/test.auth-noclaims.json @@ -0,0 +1,12 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test" + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-noclientid.json b/src/Authentication/Tests/test.auth-noclientid.json new file mode 100644 index 0000000..72903b0 --- /dev/null +++ b/src/Authentication/Tests/test.auth-noclientid.json @@ -0,0 +1,31 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "role-with-test" ], + "endpoints": [ "test" ] + }, + { + "claimType": "user_roles", + "claimValues": [ "role-without-test" ], + "endpoints": [ "no-test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-norealm.json b/src/Authentication/Tests/test.auth-norealm.json new file mode 100644 index 0000000..9e4a226 --- /dev/null +++ b/src/Authentication/Tests/test.auth-norealm.json @@ -0,0 +1,31 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "role-with-test" ], + "endpoints": [ "test" ] + }, + { + "claimType": "user_roles", + "claimValues": [ "role-without-test" ], + "endpoints": [ "no-test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-norealmkey.json b/src/Authentication/Tests/test.auth-norealmkey.json new file mode 100644 index 0000000..5a6e233 --- /dev/null +++ b/src/Authentication/Tests/test.auth-norealmkey.json @@ -0,0 +1,31 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "role-with-test" ], + "endpoints": [ "test" ] + }, + { + "claimType": "user_roles", + "claimValues": [ "role-without-test" ], + "endpoints": [ "no-test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth.json b/src/Authentication/Tests/test.auth.json index c395e87..0c5fa81 100644 --- a/src/Authentication/Tests/test.auth.json +++ b/src/Authentication/Tests/test.auth.json @@ -5,24 +5,25 @@ "realm": "TEST-REALM", "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", "audiences": [ "monai-app" ], + "roleClaimType": "roles", "clientId": "monai-app-test", "claimMappings": { "userClaims": [ { - "claim": "user_roles", - "roles": [ "role-with-test" ], + "claimType": "user_roles", + "claimValues": [ "role-with-test" ], "endpoints": [ "test" ] }, { - "claim": "user_roles", - "roles": [ "role-without-test" ], + "claimType": "user_roles", + "claimValues": [ "role-without-test" ], "endpoints": [ "no-test" ] } ], "adminClaims": [ { - "claim": "user_roles", - "roles": [ "monai-role-admin" ] + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] } ] } diff --git a/src/Authentication/example.json b/src/Authentication/example.json index feb13c9..b21ca74 100644 --- a/src/Authentication/example.json +++ b/src/Authentication/example.json @@ -9,20 +9,20 @@ "ClaimMappings": { "UserClaims": [ { - "claim": "user_roles", - "roles": [ "monai-deploy-user" ], + "claimType": "user_roles", + "claimValues": [ "monai-deploy-user" ], "endpoints": [ "test" ] }, { - "user_roles": "user_roles", - "roles": [ "pacs-admins" ], + "claimType": "user_roles", + "claimValues": [ "pacs-admins" ], "endpoints": [ "config" ] } ], "AdminClaims": [ { - "claim": "user_roles", - "role": [ "monai-role-admin" ] + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] } ] } From d6a5d06243a18a89986a31586c90f0ad1e1afe4d Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Thu, 1 Dec 2022 17:03:40 -0800 Subject: [PATCH 8/8] Add test cases Signed-off-by: Victor Chang --- .../Configurations/AuthenticationOptions.cs | 40 ++++++++++++++-- .../Configurations/OpenIdOptions.cs | 6 ++- .../MonaiAuthenticationExtensions.cs | 48 +++++++++++-------- .../EndpointAuthorizationMiddlewareTest.cs | 4 ++ .../Tests/test.auth-noclaimtype.json | 27 +++++++++++ .../Tests/test.auth-noclaimvalues.json | 25 ++++++++++ .../Tests/test.auth-noendpoints.json | 26 ++++++++++ .../test.auth-whitespaceclaimvalues.json | 27 +++++++++++ 8 files changed, 178 insertions(+), 25 deletions(-) create mode 100644 src/Authentication/Tests/test.auth-noclaimtype.json create mode 100644 src/Authentication/Tests/test.auth-noclaimvalues.json create mode 100644 src/Authentication/Tests/test.auth-noendpoints.json create mode 100644 src/Authentication/Tests/test.auth-whitespaceclaimvalues.json diff --git a/src/Authentication/Configurations/AuthenticationOptions.cs b/src/Authentication/Configurations/AuthenticationOptions.cs index 428188a..32fba93 100644 --- a/src/Authentication/Configurations/AuthenticationOptions.cs +++ b/src/Authentication/Configurations/AuthenticationOptions.cs @@ -44,10 +44,6 @@ public bool BypassAuth(ILogger logger) { throw new InvalidOperationException("openId configuration is invalid."); } - if (OpenId.Claims is null || OpenId.Claims.UserClaims!.IsNullOrEmpty() || OpenId.Claims.AdminClaims!.IsNullOrEmpty()) - { - throw new InvalidOperationException("No claimMappings defined for OpenId."); - } if (string.IsNullOrWhiteSpace(OpenId.ClientId)) { throw new InvalidOperationException("No clientId defined for OpenId."); @@ -60,8 +56,44 @@ public bool BypassAuth(ILogger logger) { throw new InvalidOperationException("No realm defined for OpenId."); } + if (OpenId.Claims is null || OpenId.Claims.UserClaims!.IsNullOrEmpty() || OpenId.Claims.AdminClaims!.IsNullOrEmpty()) + { + throw new InvalidOperationException("No claimMappings defined for OpenId."); + } + + ValidateClaims(OpenId.Claims.UserClaims!, true); + ValidateClaims(OpenId.Claims.AdminClaims!, false); return false; } + + private void ValidateClaims(List claims, bool validateEndpoints) + { + foreach (var claim in claims) + { + if (string.IsNullOrWhiteSpace(claim.ClaimType)) + { + throw new InvalidOperationException("Value for claimType is invalid."); + } + + if (claim.ClaimValues.IsNullOrEmpty()) + { + throw new InvalidOperationException("Value for claimType is invalid."); + } + + foreach (var claimValue in claim.ClaimValues) + { + if (string.IsNullOrWhiteSpace(claimValue)) + { + throw new InvalidOperationException($"Invalid claimValue for {claim.ClaimType}."); + } + } + + if (validateEndpoints && claim.Endpoints.IsNullOrEmpty()) + { + throw new InvalidOperationException("Value for claimType is invalid."); + } + } + } } } diff --git a/src/Authentication/Configurations/OpenIdOptions.cs b/src/Authentication/Configurations/OpenIdOptions.cs index 695dd0f..1dcb088 100644 --- a/src/Authentication/Configurations/OpenIdOptions.cs +++ b/src/Authentication/Configurations/OpenIdOptions.cs @@ -14,6 +14,7 @@ * limitations under the License. */ +using System.Security.Claims; using Microsoft.Extensions.Configuration; namespace Monai.Deploy.Security.Authentication.Configurations @@ -36,6 +37,9 @@ public class OpenIdOptions public IList? Audiences { get; set; } [ConfigurationKeyName("roleClaimType")] - public string RoleClaimType { get; set; } = "roles"; + public string RoleClaimType { get; set; } = ClaimTypes.Role; + + [ConfigurationKeyName("clearDefaultRoleMappigns")] + public bool ClearDefaultRoleMappigns { get; set; } = true; } } diff --git a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs index 85c0474..7a9d25e 100644 --- a/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs +++ b/src/Authentication/Extensions/MonaiAuthenticationExtensions.cs @@ -14,6 +14,7 @@ * limitations under the License. */ +using System.IdentityModel.Tokens.Jwt; using System.Text; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.JwtBearer; @@ -48,29 +49,36 @@ public static IServiceCollection AddMonaiAuthentication( return services; } - services.AddAuthentication(options => + if (configurations.Value.OpenId?.ClearDefaultRoleMappigns ?? false) { - options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme; - options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; - }) - .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, AuthKeys.OpenId, options => - { - options.Authority = configurations.Value.OpenId!.Realm; - options.Audience = configurations.Value.OpenId!.Realm; - options.RequireHttpsMetadata = false; + JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Remove("role"); + JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Remove("roles"); + } - options.TokenValidationParameters = new TokenValidationParameters + services + .AddAuthentication(options => { - IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(configurations.Value.OpenId!.RealmKey!)), - RoleClaimType = configurations.Value.OpenId.RoleClaimType, - ValidIssuer = configurations.Value.OpenId.Realm, - ValidAudiences = configurations.Value.OpenId.Audiences, - ValidateIssuerSigningKey = true, - ValidateIssuer = true, - ValidateLifetime = true, - ValidateAudience = true, - }; - }); + options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme; + options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; + }) + .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, AuthKeys.OpenId, options => + { + options.Authority = configurations.Value.OpenId!.Realm; + options.Audience = configurations.Value.OpenId!.Realm; + options.RequireHttpsMetadata = false; + + options.TokenValidationParameters = new TokenValidationParameters + { + IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(configurations.Value.OpenId!.RealmKey!)), + RoleClaimType = configurations.Value.OpenId.RoleClaimType, + ValidIssuer = configurations.Value.OpenId.Realm, + ValidAudiences = configurations.Value.OpenId.Audiences, + ValidateIssuerSigningKey = true, + ValidateIssuer = true, + ValidateLifetime = true, + ValidateAudience = true, + }; + }); services.AddAuthorization(); return services; diff --git a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs index c2233c7..9668f20 100644 --- a/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs +++ b/src/Authentication/Tests/EndpointAuthorizationMiddlewareTest.cs @@ -32,6 +32,10 @@ public partial class EndpointAuthorizationMiddlewareTest [InlineData("test.auth-noclientid.json")] [InlineData("test.auth-norealm.json")] [InlineData("test.auth-norealmkey.json")] + [InlineData("test.auth-noclaimtype.json")] + [InlineData("test.auth-noclaimvalues.json")] + [InlineData("test.auth-whitespaceclaimvalues.json")] + [InlineData("test.auth-noendpoints.json")] public async Task GivenConfigurationFilesIsBad_ExpectExceptionToBeThrown(string configFile) { await Assert.ThrowsAsync(async () => diff --git a/src/Authentication/Tests/test.auth-noclaimtype.json b/src/Authentication/Tests/test.auth-noclaimtype.json new file mode 100644 index 0000000..129c67d --- /dev/null +++ b/src/Authentication/Tests/test.auth-noclaimtype.json @@ -0,0 +1,27 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "", + "claimValues": [ "role-with-test" ], + "endpoints": [ "test" ] + } + ], + "adminClaims": [ + { + "claimType": "", + "claimValues": [ "monai-role-admin" ] + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-noclaimvalues.json b/src/Authentication/Tests/test.auth-noclaimvalues.json new file mode 100644 index 0000000..b749bef --- /dev/null +++ b/src/Authentication/Tests/test.auth-noclaimvalues.json @@ -0,0 +1,25 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "endpoints": [ "test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-noendpoints.json b/src/Authentication/Tests/test.auth-noendpoints.json new file mode 100644 index 0000000..53d0e3c --- /dev/null +++ b/src/Authentication/Tests/test.auth-noendpoints.json @@ -0,0 +1,26 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "role-with-test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + "claimValues": [ "monai-role-admin" ] + } + ] + } + } + } +} \ No newline at end of file diff --git a/src/Authentication/Tests/test.auth-whitespaceclaimvalues.json b/src/Authentication/Tests/test.auth-whitespaceclaimvalues.json new file mode 100644 index 0000000..c4187fe --- /dev/null +++ b/src/Authentication/Tests/test.auth-whitespaceclaimvalues.json @@ -0,0 +1,27 @@ +{ + "MonaiDeployAuthentication": { + "bypassAuthentication": false, + "openId": { + "realm": "TEST-REALM", + "realmKey": "l9ZRlbMQBt9k1klUUrlWFuke8WbqnEde", + "audiences": [ "monai-app" ], + "roleClaimType": "roles", + "clientId": "monai-app-test", + "claimMappings": { + "userClaims": [ + { + "claimType": "user_roles", + "claimValues": [ " " ], + "endpoints": [ "test" ] + } + ], + "adminClaims": [ + { + "claimType": "user_roles", + "claimValues": [ " " ] + } + ] + } + } + } +} \ No newline at end of file