From 25ab34c77d1f6771e4cabd63af4cec2c43be28e4 Mon Sep 17 00:00:00 2001 From: StuartFerguson Date: Wed, 7 Oct 2026 18:58:58 +0100 Subject: [PATCH 1/2] Add missing security regression tests --- .../DataProtectionPersistenceTests.cs | 34 ++++++++ .../ClientRequestHandlerTests.cs | 82 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs diff --git a/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs b/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs new file mode 100644 index 00000000..c4ae9b27 --- /dev/null +++ b/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs @@ -0,0 +1,34 @@ +using Microsoft.AspNetCore.DataProtection; +using Shouldly; + +namespace SecurityService.UnitTests.Configuration; + +public sealed class DataProtectionPersistenceTests : IDisposable +{ + private readonly string _temporaryDirectory = Path.Combine( + Path.GetTempPath(), + $"securityservice-keys-{Guid.NewGuid():N}"); + + [Fact] + public void PersistedKeys_CanUnprotectDataCreatedByAnotherProviderInstance() + { + Directory.CreateDirectory(this._temporaryDirectory); + + var firstProvider = DataProtectionProvider.Create(new DirectoryInfo(this._temporaryDirectory)); + var firstProtector = firstProvider.CreateProtector("SecurityService.Tests"); + var protectedValue = firstProtector.Protect("sensitive-value"); + + var secondProvider = DataProtectionProvider.Create(new DirectoryInfo(this._temporaryDirectory)); + var secondProtector = secondProvider.CreateProtector("SecurityService.Tests"); + + secondProtector.Unprotect(protectedValue).ShouldBe("sensitive-value"); + } + + public void Dispose() + { + if (Directory.Exists(this._temporaryDirectory)) + { + Directory.Delete(this._temporaryDirectory, recursive: true); + } + } +} diff --git a/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs b/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs index de5d48ed..9c47c387 100644 --- a/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs +++ b/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs @@ -128,6 +128,88 @@ public async Task CreateClient_WhenGrantTypeIsUnsupported_ReturnsInvalid() result.Message.ShouldContain("Unsupported grant types"); } + [Fact] + public async Task CreateClient_WithLegacyGrantType_IsRejectedUnlessClientIsConfigured() + { + using var provider = TestServiceProviderFactory.Create( + nameof(this.CreateClient_WithLegacyGrantType_IsRejectedUnlessClientIsConfigured), + configureOptions: options => + { + options.OAuth.EnableLegacyGrantTypes = true; + options.OAuth.LegacyGrantTypeClients[OpenIddictConstants.GrantTypes.Password] = ["legacy-client"]; + }); + var mediator = provider.GetRequiredService(); + + var rejected = await mediator.Send(new SecurityServiceCommands.CreateClientCommand( + "other-client", + "secret", + "Other Client", + null, + [], + [OpenIddictConstants.GrantTypes.Password], + null, + [], + [], + false, + false)); + + rejected.IsFailed.ShouldBeTrue(); + rejected.Status.ShouldBe(ResultStatus.Invalid); + + var accepted = await mediator.Send(new SecurityServiceCommands.CreateClientCommand( + "legacy-client", + "secret", + "Legacy Client", + null, + [], + [OpenIddictConstants.GrantTypes.Password], + null, + [], + [], + false, + false)); + + accepted.IsSuccess.ShouldBeTrue(); + } + + [Fact] + public async Task CreateClient_WithPasswordGrant_PersistsOnlyPasswordGrantPermission() + { + using var provider = TestServiceProviderFactory.Create( + nameof(this.CreateClient_WithPasswordGrant_PersistsOnlyPasswordGrantPermission), + configureOptions: options => + { + options.OAuth.EnableLegacyGrantTypes = true; + options.OAuth.LegacyGrantTypeClients[OpenIddictConstants.GrantTypes.Password] = ["legacy-client"]; + }); + var mediator = provider.GetRequiredService(); + var applicationManager = provider.GetRequiredService(); + + var result = await mediator.Send(new SecurityServiceCommands.CreateClientCommand( + "legacy-client", + "secret", + "Legacy Client", + null, + [], + [OpenIddictConstants.GrantTypes.Password], + null, + [], + [], + false, + false)); + + result.IsSuccess.ShouldBeTrue(); + + object application = (await applicationManager.FindByClientIdAsync("legacy-client"))!; + var permissions = (await applicationManager.GetPermissionsAsync(application)).ToHashSet(StringComparer.Ordinal); + + permissions.ShouldContain(OpenIddictConstants.Permissions.Endpoints.Token); + permissions.ShouldContain(OpenIddictConstants.Permissions.GrantTypes.Password); + permissions.ShouldNotContain(OpenIddictConstants.Permissions.GrantTypes.ClientCredentials); + permissions.ShouldNotContain(OpenIddictConstants.Permissions.GrantTypes.RefreshToken); + permissions.ShouldNotContain(OpenIddictConstants.Permissions.Endpoints.Authorization); + } + [Fact] public async Task CreateClient_WithInvalidRedirectUri_ReturnsInvalidWithoutPersisting() { From 74122c74f482f968213ce3a4c25c4fae8fd6888a Mon Sep 17 00:00:00 2001 From: StuartFerguson Date: Wed, 7 Oct 2026 19:07:28 +0100 Subject: [PATCH 2/2] Address PR test coverage feedback --- .../DataProtectionPersistenceTests.cs | 18 ++++++++++++++++-- .../ClientRequestHandlerTests.cs | 11 ++++++----- .../DataProtectionRegistration.cs | 18 ++++++++++++++++++ SecurityService/Program.cs | 7 +------ 4 files changed, 41 insertions(+), 13 deletions(-) create mode 100644 SecurityService/Configuration/DataProtectionRegistration.cs diff --git a/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs b/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs index c4ae9b27..7a7a284e 100644 --- a/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs +++ b/SecurityService.UnitTests/Configuration/DataProtectionPersistenceTests.cs @@ -1,4 +1,7 @@ using Microsoft.AspNetCore.DataProtection; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using SecurityService.Configuration; using Shouldly; namespace SecurityService.UnitTests.Configuration; @@ -14,11 +17,22 @@ public void PersistedKeys_CanUnprotectDataCreatedByAnotherProviderInstance() { Directory.CreateDirectory(this._temporaryDirectory); - var firstProvider = DataProtectionProvider.Create(new DirectoryInfo(this._temporaryDirectory)); + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["ServiceOptions:DataProtectionKeyDirectory"] = this._temporaryDirectory + }) + .Build(); + + var firstServices = new ServiceCollection(); + DataProtectionRegistration.Add(firstServices, configuration); + var firstProvider = firstServices.BuildServiceProvider().GetRequiredService(); var firstProtector = firstProvider.CreateProtector("SecurityService.Tests"); var protectedValue = firstProtector.Protect("sensitive-value"); - var secondProvider = DataProtectionProvider.Create(new DirectoryInfo(this._temporaryDirectory)); + var secondServices = new ServiceCollection(); + DataProtectionRegistration.Add(secondServices, configuration); + var secondProvider = secondServices.BuildServiceProvider().GetRequiredService(); var secondProtector = secondProvider.CreateProtector("SecurityService.Tests"); secondProtector.Unprotect(protectedValue).ShouldBe("sensitive-value"); diff --git a/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs b/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs index 9c47c387..4c773c11 100644 --- a/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs +++ b/SecurityService.UnitTests/RequestHandlers/ClientRequestHandlerTests.cs @@ -202,12 +202,13 @@ public async Task CreateClient_WithPasswordGrant_PersistsOnlyPasswordGrantPermis object application = (await applicationManager.FindByClientIdAsync("legacy-client"))!; var permissions = (await applicationManager.GetPermissionsAsync(application)).ToHashSet(StringComparer.Ordinal); + var expectedPermissions = new HashSet(StringComparer.Ordinal) + { + OpenIddictConstants.Permissions.Endpoints.Token, + OpenIddictConstants.Permissions.GrantTypes.Password + }; - permissions.ShouldContain(OpenIddictConstants.Permissions.Endpoints.Token); - permissions.ShouldContain(OpenIddictConstants.Permissions.GrantTypes.Password); - permissions.ShouldNotContain(OpenIddictConstants.Permissions.GrantTypes.ClientCredentials); - permissions.ShouldNotContain(OpenIddictConstants.Permissions.GrantTypes.RefreshToken); - permissions.ShouldNotContain(OpenIddictConstants.Permissions.Endpoints.Authorization); + permissions.SetEquals(expectedPermissions).ShouldBeTrue(); } [Fact] diff --git a/SecurityService/Configuration/DataProtectionRegistration.cs b/SecurityService/Configuration/DataProtectionRegistration.cs new file mode 100644 index 00000000..67aa5055 --- /dev/null +++ b/SecurityService/Configuration/DataProtectionRegistration.cs @@ -0,0 +1,18 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.AspNetCore.DataProtection; + +namespace SecurityService.Configuration; + +public static class DataProtectionRegistration +{ + public static void Add(IServiceCollection services, IConfiguration configuration) + { + var dataProtectionKeyDirectory = configuration["ServiceOptions:DataProtectionKeyDirectory"]; + var dataProtectionBuilder = services.AddDataProtection(); + if (string.IsNullOrWhiteSpace(dataProtectionKeyDirectory) == false) + { + dataProtectionBuilder.PersistKeysToFileSystem(new DirectoryInfo(dataProtectionKeyDirectory)); + } + } +} diff --git a/SecurityService/Program.cs b/SecurityService/Program.cs index 56d59d6b..33ea3105 100644 --- a/SecurityService/Program.cs +++ b/SecurityService/Program.cs @@ -207,12 +207,7 @@ }); builder.Services.AddHttpContextAccessor(); -var dataProtectionKeyDirectory = builder.Configuration["ServiceOptions:DataProtectionKeyDirectory"]; -var dataProtectionBuilder = builder.Services.AddDataProtection(); -if (string.IsNullOrWhiteSpace(dataProtectionKeyDirectory) == false) -{ - dataProtectionBuilder.PersistKeysToFileSystem(new DirectoryInfo(dataProtectionKeyDirectory)); -} +DataProtectionRegistration.Add(builder.Services, builder.Configuration); builder.Services.AddSingleton(serviceProvider => new ConsentTransactionProtector( serviceProvider.GetRequiredService().CreateProtector("SecurityService.Consent"))); builder.Services.AddScoped();