Skip to content

Commit

Permalink
General refactoring
Browse files Browse the repository at this point in the history
  • Loading branch information
Farshad DASHTI authored and Farshad DASHTI committed Nov 22, 2024
1 parent 58cf07d commit a67719f
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 98 deletions.
2 changes: 2 additions & 0 deletions src/DfE.CoreLibs.Security/Authorization/ApiOboTokenService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ public async Task<string> 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.");
}
Expand Down
7 changes: 0 additions & 7 deletions ....CoreLibs.Security/Authorization/Events/RejectSessionCookieWhenAccountNotInCacheEvents.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ILogger<RejectSessionCookieWhenAccountNotInCacheEvents>>();
logger.LogError(ex, "An unexpected error occurred in ValidatePrincipal.");

throw;
}
}

/// <summary>
Expand Down
34 changes: 13 additions & 21 deletions src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ApiOboTokenServiceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -27,7 +19,6 @@ public class ApiOboTokenServiceTests
private readonly IConfiguration _configuration;
private readonly IMemoryCache _memoryCacheMock;
private readonly IOptions<TokenSettings> _tokenSettingsMock;
private readonly TokenSettings _tokenSettings;
private readonly ClaimsPrincipal _testUser;

public ApiOboTokenServiceTests()
Expand All @@ -42,9 +33,9 @@ public ApiOboTokenServiceTests()
.Build();

// Configure TokenSettings from appsettings
_tokenSettings = _configuration.GetSection("Authorization:TokenSettings").Get<TokenSettings>();
var tokenSettings = _configuration.GetSection("Authorization:TokenSettings").Get<TokenSettings>();
_tokenSettingsMock = Substitute.For<IOptions<TokenSettings>>();
_tokenSettingsMock.Value.Returns(_tokenSettings);
_tokenSettingsMock.Value.Returns(tokenSettings);

// Setup test user
_testUser = new ClaimsPrincipal(new ClaimsIdentity(new[]
Expand All @@ -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,
Expand Down Expand Up @@ -103,7 +94,7 @@ public async Task GetApiOboTokenAsync_ApiClientIdMissing_ThrowsInvalidOperationE
IConfiguration _configurationMock = Substitute.For<IConfiguration>();

_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,
Expand All @@ -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<object>())
_memoryCacheMock.TryGetValue(cacheKey, out Arg.Any<object>()!)
.Returns(call =>
{
call[1] = "cached-token";
Expand Down Expand Up @@ -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<string>(key => key == cacheKey), out cachedValue))
.Setup(mc => mc.TryGetValue(It.Is<string>(key => key == cacheKey), out cachedValue!))
.Returns(false);

// Mock CreateEntry for caching behavior
Expand Down Expand Up @@ -317,9 +308,11 @@ public async Task GetApiOboTokenAsync_UsesDefaultScope_WhenNoApiScopes()
}))
});

var defaultScopes = new[] { "api://test-api-client-id/default-scope" };

_tokenAcquisitionMock
.GetAccessTokenForUserAsync(
Arg.Is<IEnumerable<string>>(scopes => scopes.SequenceEqual(new[] { "api://test-api-client-id/default-scope" })),
Arg.Is<IEnumerable<string>>(scopes => scopes.SequenceEqual(defaultScopes)),
Arg.Is<string>(scheme => scheme == null),
Arg.Is<string>(_ => true),
Arg.Is<string>(_ => true),
Expand All @@ -333,7 +326,7 @@ public async Task GetApiOboTokenAsync_UsesDefaultScope_WhenNoApiScopes()
// Assert
Assert.Equal("mock-token", token);
await _tokenAcquisitionMock.Received(1).GetAccessTokenForUserAsync(
Arg.Is<IEnumerable<string>>(scopes => scopes.SequenceEqual(new[] { "api://test-api-client-id/default-scope" })),
Arg.Is<IEnumerable<string>>(scopes => scopes.SequenceEqual(defaultScopes)),
Arg.Any<string>(),
Arg.Any<string>(),
Arg.Any<string>(),
Expand All @@ -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<string>(), out Arg.Any<object>()!)
Expand Down
66 changes: 7 additions & 59 deletions src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/AuthorizationExtensionsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,52 +10,31 @@

namespace DfE.CoreLibs.Security.Tests.AuthorizationTests
{
/// <summary>
/// Custom authorization requirement for testing purposes.
/// </summary>
public class CustomRequirement : IAuthorizationRequirement { }

/// <summary>
/// Unit tests for the AddApplicationAuthorization extension method.
/// </summary>
public class AuthorizationExtensionsTests
{
private readonly IConfiguration _configuration;
private readonly ServiceProvider _serviceProvider;

/// <summary>
/// Initializes a new instance of the <see cref="AuthorizationExtensionsTests"/> class.
/// Sets up the configuration and service provider for testing.
/// </summary>
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<IClaimsTransformation, DummyClaimsTransformation>();

// Register the AuthorizationPolicyProvider
services.AddSingleton<IAuthorizationPolicyProvider, DefaultAuthorizationPolicyProvider>();

// Register the IAuthorizationService
services.AddSingleton<IAuthorizationService, DefaultAuthorizationService>();

// Build the service provider
_serviceProvider = services.BuildServiceProvider();
}

Expand All @@ -68,9 +47,6 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// 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")
Expand All @@ -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"),
Expand All @@ -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"),
Expand All @@ -130,16 +99,12 @@ public async Task AddApplicationAuthorization_ShouldLoadPoliciesFromConfiguratio
"User with roles and required claims should be authorized for CanReadWritePlus policy.");
}

/// <summary>
/// Tests that authorization succeeds when the user has all required scopes, even if some roles are missing.
/// </summary>
[Fact]
public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllScopesEvenIfMissingSomeRoles()
{
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// 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")
Expand All @@ -154,16 +119,12 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllScope
"User should be authorized for CanReadWrite policy because they have all required scopes.");
}

/// <summary>
/// Tests that authorization succeeds when the user has all required roles, even if some scopes are missing.
/// </summary>
[Fact]
public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllRolesEvenIfMissingSomeScopes()
{
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// 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"),
Expand All @@ -179,16 +140,12 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenUserHasAllRoles
"User should be authorized for CanReadWrite policy because they have all required roles.");
}

/// <summary>
/// Tests that authorization fails when the user lacks both required scopes and roles.
/// </summary>
[Fact]
public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredScopesAndRoles()
{
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// Define a test user missing all required scopes and roles
var user = new ClaimsPrincipal(new ClaimsIdentity(new Claim[]
{
// No scope claims
Expand All @@ -203,20 +160,16 @@ public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredSco
"User should not be authorized for CanReadWrite policy when missing required scopes and roles.");
}

/// <summary>
/// Tests that authorization fails when the user lacks required scopes.
/// </summary>
[Fact]
public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredScopes()
{
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// 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"));

Expand All @@ -228,21 +181,16 @@ public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredSco
"User should not be authorized for CanReadWrite policy when missing required scopes.");
}

/// <summary>
/// Tests that authorization fails when the user lacks required roles.
/// </summary>
[Fact]
public async Task AddApplicationAuthorization_ShouldFailWhenUserLacksRequiredRoles()
{
// Arrange
var authorizationService = _serviceProvider.GetRequiredService<IAuthorizationService>();

// 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");
Expand All @@ -269,7 +217,7 @@ public void AddCustomClaimProvider_ShouldRegisterClaimProvider()
}

[Fact]
public void AddApplicationAuthorization_ShouldApplyPolicyCustomizations()
public async Task AddApplicationAuthorization_ShouldApplyPolicyCustomizations()
{
// Arrange
var services = new ServiceCollection();
Expand All @@ -296,7 +244,7 @@ public void AddApplicationAuthorization_ShouldApplyPolicyCustomizations()
var options = provider.GetRequiredService<IAuthorizationPolicyProvider>();

// 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" &&
Expand Down Expand Up @@ -373,7 +321,7 @@ public async Task AddApplicationAuthorization_ShouldAuthorizeWhenPolicyOperatorI


[Fact]
public void AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations()
public async Task AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations()
{
// Arrange
var services = new ServiceCollection();
Expand All @@ -392,7 +340,7 @@ public void AddApplicationAuthorization_ShouldWorkWithoutPolicyCustomizations()
var options = provider.GetRequiredService<IAuthorizationPolicyProvider>();

// Assert
var policy = options.GetPolicyAsync("PolicyWithoutCustomizations").Result;
var policy = await options.GetPolicyAsync("PolicyWithoutCustomizations");
Assert.NotNull(policy);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Tests/DfE.CoreLibs.Security.Tests/AuthorizationTests/ServiceCollectionExtensionsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void AddCustomJwtAuthentication_ShouldThrowException_WhenTokenSettingsMis
_services.AddLogging();

var invalidConfiguration = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string>())
.AddInMemoryCollection(new Dictionary<string, string>()!)
.Build();

var authenticationBuilder = new AuthenticationBuilder(_services);
Expand Down
Loading

0 comments on commit a67719f

Please sign in to comment.