diff --git a/src/DfE.CoreLibs.Security/Authorization/ApiOboTokenService.cs b/src/DfE.CoreLibs.Security/Authorization/ApiOboTokenService.cs index 8bc9cfc..36b80ca 100644 --- a/src/DfE.CoreLibs.Security/Authorization/ApiOboTokenService.cs +++ b/src/DfE.CoreLibs.Security/Authorization/ApiOboTokenService.cs @@ -40,7 +40,9 @@ public async Task GetApiOboTokenAsync(string? authenticationScheme = nul // Retrieve user roles var userRoles = user.FindAll(ClaimTypes.Role).Select(c => c.Value).ToArray(); +#pragma warning disable S2589 if (userRoles == null || !userRoles.Any()) +#pragma warning restore S2589 { throw new UnauthorizedAccessException("User does not have any roles assigned."); } diff --git a/src/DfE.CoreLibs.Security/Authorization/Events/RejectSessionCookieWhenAccountNotInCacheEvents.cs b/src/DfE.CoreLibs.Security/Authorization/Events/RejectSessionCookieWhenAccountNotInCacheEvents.cs index ee26ce9..c1116f6 100644 --- a/src/DfE.CoreLibs.Security/Authorization/Events/RejectSessionCookieWhenAccountNotInCacheEvents.cs +++ b/src/DfE.CoreLibs.Security/Authorization/Events/RejectSessionCookieWhenAccountNotInCacheEvents.cs @@ -53,13 +53,6 @@ await tokenAcquisition.GetAccessTokenForUserAsync( // Reject the principal to sign out the user context.RejectPrincipal(); } - catch (Exception ex) - { - var logger = context.HttpContext.RequestServices.GetRequiredService>(); - logger.LogError(ex, "An unexpected error occurred in ValidatePrincipal."); - - throw; - } } /// diff --git a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ApiOboTokenServiceTests.cs b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ApiOboTokenServiceTests.cs index 8f51eb2..858a315 100644 --- a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ApiOboTokenServiceTests.cs +++ b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ApiOboTokenServiceTests.cs @@ -2,21 +2,13 @@ using DfE.CoreLibs.Security.Configurations; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Options; using Microsoft.Identity.Web; +using Moq; using NSubstitute; -using System; -using System.Collections.Generic; -using System.Linq; using System.Security.Claims; -using System.Text; -using System.Threading.Tasks; -using Microsoft.Extensions.Configuration; -using Microsoft.AspNetCore.Authentication; -using Castle.Core.Configuration; using IConfiguration = Microsoft.Extensions.Configuration.IConfiguration; -using System.Diagnostics; -using Moq; namespace DfE.CoreLibs.Security.Tests.AuthorizationTests { @@ -27,7 +19,6 @@ public class ApiOboTokenServiceTests private readonly IConfiguration _configuration; private readonly IMemoryCache _memoryCacheMock; private readonly IOptions _tokenSettingsMock; - private readonly TokenSettings _tokenSettings; private readonly ClaimsPrincipal _testUser; public ApiOboTokenServiceTests() @@ -42,9 +33,9 @@ public ApiOboTokenServiceTests() .Build(); // Configure TokenSettings from appsettings - _tokenSettings = _configuration.GetSection("Authorization:TokenSettings").Get(); + var tokenSettings = _configuration.GetSection("Authorization:TokenSettings").Get(); _tokenSettingsMock = Substitute.For>(); - _tokenSettingsMock.Value.Returns(_tokenSettings); + _tokenSettingsMock.Value.Returns(tokenSettings); // Setup test user _testUser = new ClaimsPrincipal(new ClaimsIdentity(new[] @@ -58,7 +49,7 @@ public ApiOboTokenServiceTests() public async Task GetApiOboTokenAsync_UserNotAuthenticated_ThrowsUnauthorizedAccessException() { // Arrange - _httpContextAccessorMock.HttpContext.Returns((HttpContext)null); + _httpContextAccessorMock.HttpContext.Returns((HttpContext)null!); var service = new ApiOboTokenService( _tokenAcquisitionMock, @@ -103,7 +94,7 @@ public async Task GetApiOboTokenAsync_ApiClientIdMissing_ThrowsInvalidOperationE IConfiguration _configurationMock = Substitute.For(); _httpContextAccessorMock.HttpContext.Returns(new DefaultHttpContext { User = _testUser }); - _configurationMock["Authorization:ApiSettings:ApiClientId"].Returns((string)null); + _configurationMock["Authorization:ApiSettings:ApiClientId"].Returns((string)null!); var service = new ApiOboTokenService( _tokenAcquisitionMock, @@ -127,7 +118,7 @@ public async Task GetApiOboTokenAsync_TokenCached_ReturnsCachedToken() var cacheKey = "ApiOboToken_test-user-id,Scope1"; // Simulate retrieving a cached token - _memoryCacheMock.TryGetValue(cacheKey, out Arg.Any()) + _memoryCacheMock.TryGetValue(cacheKey, out Arg.Any()!) .Returns(call => { call[1] = "cached-token"; @@ -167,9 +158,9 @@ public async Task GetApiOboTokenAsync_AcquiresNewToken_AndCachesIt_WithInMemoryC var cacheKey = "ApiOboToken_test-user-id_Scope1"; // Setup TryGetValue to simulate a cache miss - object cachedValue = null; + object cachedValue = null!; memoryCacheMock - .Setup(mc => mc.TryGetValue(It.Is(key => key == cacheKey), out cachedValue)) + .Setup(mc => mc.TryGetValue(It.Is(key => key == cacheKey), out cachedValue!)) .Returns(false); // Mock CreateEntry for caching behavior @@ -317,9 +308,11 @@ public async Task GetApiOboTokenAsync_UsesDefaultScope_WhenNoApiScopes() })) }); + var defaultScopes = new[] { "api://test-api-client-id/default-scope" }; + _tokenAcquisitionMock .GetAccessTokenForUserAsync( - Arg.Is>(scopes => scopes.SequenceEqual(new[] { "api://test-api-client-id/default-scope" })), + Arg.Is>(scopes => scopes.SequenceEqual(defaultScopes)), Arg.Is(scheme => scheme == null), Arg.Is(_ => true), Arg.Is(_ => true), @@ -333,7 +326,7 @@ public async Task GetApiOboTokenAsync_UsesDefaultScope_WhenNoApiScopes() // Assert Assert.Equal("mock-token", token); await _tokenAcquisitionMock.Received(1).GetAccessTokenForUserAsync( - Arg.Is>(scopes => scopes.SequenceEqual(new[] { "api://test-api-client-id/default-scope" })), + Arg.Is>(scopes => scopes.SequenceEqual(defaultScopes)), Arg.Any(), Arg.Any(), Arg.Any(), @@ -349,7 +342,6 @@ public async Task GetApiOboTokenAsync_AcquiresNewToken_AndCachesIt() _httpContextAccessorMock.HttpContext.Returns(new DefaultHttpContext { User = _testUser }); var formattedScopes = new[] { "api://test-api-client-id/Scope1" }; - var cacheKey = "ApiOboToken_test-user-id_Scope1"; // Simulate no cached token _memoryCacheMock.TryGetValue(Arg.Any(), out Arg.Any()!) diff --git a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/AuthorizationExtensionsTests.cs b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/AuthorizationExtensionsTests.cs index 9a18b6e..12b4fb5 100644 --- a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/AuthorizationExtensionsTests.cs +++ b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/AuthorizationExtensionsTests.cs @@ -10,52 +10,31 @@ namespace DfE.CoreLibs.Security.Tests.AuthorizationTests { - /// - /// Custom authorization requirement for testing purposes. - /// - public class CustomRequirement : IAuthorizationRequirement { } - - /// - /// Unit tests for the AddApplicationAuthorization extension method. - /// public class AuthorizationExtensionsTests { private readonly IConfiguration _configuration; private readonly ServiceProvider _serviceProvider; - /// - /// Initializes a new instance of the class. - /// Sets up the configuration and service provider for testing. - /// public AuthorizationExtensionsTests() { - // Build configuration from the test project's appsettings.json _configuration = new ConfigurationBuilder() .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true) .Build(); - // Set up the service collection var services = new ServiceCollection(); - // Register logging services services.AddLogging(); - // Add application authorization services.AddApplicationAuthorization(_configuration); - // Add Authorization services services.AddAuthorization(); - // Register a dummy IClaimsTransformation if necessary services.AddTransient(); - // Register the AuthorizationPolicyProvider services.AddSingleton(); - // Register the IAuthorizationService services.AddSingleton(); - // Build the service provider _serviceProvider = services.BuildServiceProvider(); } @@ -68,9 +47,6 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user with required scopes and roles - // Note: As per your logic, a user should have either scopes or roles, not both. - // This test ensures that policies are correctly loaded, not the logic itself. var userWithScopes = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim("http://schemas.microsoft.com/identity/claims/scope", "SCOPE.API.Read SCOPE.API.Write") @@ -83,28 +59,22 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio }, "TestAuthentication")); // Act & Assert - - // Test "CanRead" policy with user having scopes var canReadResultScopes = await authorizationService.AuthorizeAsync(userWithScopes, null, "CanRead"); Assert.True(canReadResultScopes.Succeeded, "User with scopes should be authorized for CanRead policy."); - // Test "CanRead" policy with user having roles var canReadResultRoles = await authorizationService.AuthorizeAsync(userWithRoles, null, "CanRead"); Assert.True(canReadResultRoles.Succeeded, "User with roles should be authorized for CanRead policy."); - // Test "CanReadWrite" policy with user having scopes var canReadWriteResultScopes = await authorizationService.AuthorizeAsync(userWithScopes, null, "CanReadWrite"); Assert.True(canReadWriteResultScopes.Succeeded, "User with scopes should be authorized for CanReadWrite policy."); - // Test "CanReadWrite" policy with user having roles var canReadWriteResultRoles = await authorizationService.AuthorizeAsync(userWithRoles, null, "CanReadWrite"); Assert.True(canReadWriteResultRoles.Succeeded, "User with roles should be authorized for CanReadWrite policy."); - // Test "CanReadWritePlus" policy with user having scopes and required claims var userWithScopesAndClaims = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim("http://schemas.microsoft.com/identity/claims/scope", "SCOPE.API.Read SCOPE.API.Write"), @@ -116,7 +86,6 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio Assert.True(canReadWritePlusResultScopes.Succeeded, "User with scopes and required claims should be authorized for CanReadWritePlus policy."); - // Test "CanReadWritePlus" policy with user having roles and required claims var userWithRolesAndClaims = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim(ClaimTypes.Role, "API.Read"), @@ -130,16 +99,12 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio "User with roles and required claims should be authorized for CanReadWritePlus policy."); } - /// - /// Tests that authorization succeeds when the user has all required scopes, even if some roles are missing. - /// [Fact] public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllScopesEvenIfMissingSomeRoles() { // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user with all required scopes but missing some roles var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim("http://schemas.microsoft.com/identity/claims/scope", "SCOPE.API.Read SCOPE.API.Write") @@ -154,16 +119,12 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllScope "User should be authorized for CanReadWrite policy because they have all required scopes."); } - /// - /// Tests that authorization succeeds when the user has all required roles, even if some scopes are missing. - /// [Fact] public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllRolesEvenIfMissingSomeScopes() { // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user with all required roles but missing some scopes var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim(ClaimTypes.Role, "API.Read"), @@ -179,16 +140,12 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllRoles "User should be authorized for CanReadWrite policy because they have all required roles."); } - /// - /// Tests that authorization fails when the user lacks both required scopes and roles. - /// [Fact] public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredScopesAndRoles() { // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user missing all required scopes and roles var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { // No scope claims @@ -203,20 +160,16 @@ public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredSco "User should not be authorized for CanReadWrite policy when missing required scopes and roles."); } - /// - /// Tests that authorization fails when the user lacks required scopes. - /// [Fact] public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredScopes() { // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user with some scopes but missing one required scope var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] { new Claim("http://schemas.microsoft.com/identity/claims/scope", - "SCOPE.API.Read") // Missing "SCOPE.API.Write" + "SCOPE.API.Read") // No role claims }, "TestAuthentication")); @@ -228,21 +181,16 @@ public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredSco "User should not be authorized for CanReadWrite policy when missing required scopes."); } - /// - /// Tests that authorization fails when the user lacks required roles. - /// [Fact] public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredRoles() { // Arrange var authorizationService = _serviceProvider.GetRequiredService(); - // Define a test user with some roles but missing one required role - var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] - { + var user = new ClaimsPrincipal(new ClaimsIdentity([ new Claim(ClaimTypes.Role, "API.Read") // Missing "API.Write" // No scope claims - }, "TestAuthentication")); + ], "TestAuthentication")); // Act var canReadWriteResult = await authorizationService.AuthorizeAsync(user, null, "CanReadWrite"); @@ -269,7 +217,7 @@ public void AddCustomClaimProvider_ShouldRegisterClaimProvider() } [Fact] - public void AddApplicationAuthorization_ShouldApplyPolicyCustomizations() + public async Task AddApplicationAuthorization_ShouldApplyPolicyCustomizations() { // Arrange var services = new ServiceCollection(); @@ -296,7 +244,7 @@ public void AddApplicationAuthorization_ShouldApplyPolicyCustomizations() var options = provider.GetRequiredService(); // Assert - var policy = options.GetPolicyAsync("CustomPolicy").Result; + var policy = await options.GetPolicyAsync("CustomPolicy"); Assert.NotNull(policy); Assert.Contains(policy.Requirements, r => r is ClaimsAuthorizationRequirement claimReq && claimReq.ClaimType == "CustomClaim" && @@ -373,7 +321,7 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenPolicyOperatorI [Fact] - public void AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations() + public async Task AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations() { // Arrange var services = new ServiceCollection(); @@ -392,7 +340,7 @@ public void AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations() var options = provider.GetRequiredService(); // Assert - var policy = options.GetPolicyAsync("PolicyWithoutCustomizations").Result; + var policy = await options.GetPolicyAsync("PolicyWithoutCustomizations"); Assert.NotNull(policy); } diff --git a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ServiceCollectionExtensionsTests.cs b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ServiceCollectionExtensionsTests.cs index f3777a9..d4004f9 100644 --- a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ServiceCollectionExtensionsTests.cs +++ b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ServiceCollectionExtensionsTests.cs @@ -109,7 +109,7 @@ public void AddCustomJwtAuthentication_ShouldThrowException_WhenTokenSettingsMis _services.AddLogging(); var invalidConfiguration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary()) + .AddInMemoryCollection(new Dictionary()!) .Build(); var authenticationBuilder = new AuthenticationBuilder(_services); diff --git a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/UserTokenServiceTests.cs b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/UserTokenServiceTests.cs index d5729dc..5bb9d69 100644 --- a/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/UserTokenServiceTests.cs +++ b/src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/UserTokenServiceTests.cs @@ -15,8 +15,7 @@ public class UserTokenServiceTests private readonly IMemoryCache _memoryCacheMock; private readonly ILogger _loggerMock; private readonly IOptions _tokenSettingsMock; - private readonly TokenSettings _tokenSettings; - private readonly IConfiguration _configuration; + private readonly TokenSettings? _tokenSettings; private readonly ClaimsPrincipal _testUser; public UserTokenServiceTests() @@ -24,14 +23,12 @@ public UserTokenServiceTests() _memoryCacheMock = Substitute.For(); _loggerMock = Substitute.For>(); - // Load configuration from appsettings.json - _configuration = new ConfigurationBuilder() - .SetBasePath(AppContext.BaseDirectory) // Ensure the path to the test directory + IConfiguration configuration = new ConfigurationBuilder() + .SetBasePath(AppContext.BaseDirectory) .AddJsonFile("appsettings.json", optional: false, reloadOnChange: false) .Build(); - // Configure TokenSettings from appsettings - _tokenSettings = _configuration.GetSection("Authorization:TokenSettings").Get(); + _tokenSettings = configuration.GetSection("Authorization:TokenSettings").Get(); _tokenSettingsMock = Substitute.For>(); _tokenSettingsMock.Value.Returns(_tokenSettings); @@ -107,7 +104,7 @@ public async Task GetUserTokenAsync_ThrowsException_WhenUserIsNull() var service = new UserTokenService(_tokenSettingsMock, _memoryCacheMock, _loggerMock); // Act & Assert - var exception = await Assert.ThrowsAsync(() => service.GetUserTokenAsync(null)); + var exception = await Assert.ThrowsAsync(() => service.GetUserTokenAsync(null!)); Assert.Equal("user", exception.ParamName); } @@ -139,8 +136,8 @@ public void GenerateToken_CreatesValidJwtToken() Assert.NotNull(token); var handler = new JwtSecurityTokenHandler(); var jwtToken = handler.ReadJwtToken(token); - Assert.Equal(_tokenSettings.Issuer, jwtToken.Issuer); - Assert.Equal(_tokenSettings.Audience, jwtToken.Audiences.First()); + Assert.Equal(_tokenSettings?.Issuer, jwtToken.Issuer); + Assert.Equal(_tokenSettings?.Audience, jwtToken.Audiences.First()); Assert.Contains(jwtToken.Claims, c => c.Type == ClaimTypes.Name && c.Value == "TestUser"); Assert.Contains(jwtToken.Claims, c => c.Type == ClaimTypes.Role && c.Value == "Admin"); }