fix(security): M2.19 review nits — idle/refresh config guard + adapter tests + dead-var/doc cleanup (#15)
- Add SecurityOptionsValidator (IValidateOptions<SecurityOptions>) enforcing RoleRefreshThresholdMinutes < IdleTimeoutMinutes; registered with ValidateOnStart in AddSecurity — startup FAILS if threshold >= idle, so the invariant cannot be silently misconfigured away. - Update SecurityOptions XML-docs: class-level summary distinguishes JWT Bearer path (JwtSigningKey/JwtExpiryMinutes) from Blazor cookie session path (IdleTimeoutMinutes/ RoleRefreshThresholdMinutes); both time fields document the ~45-min effective idle window and the new cross-field constraint. - Remove dead jwtService variable from /auth/login lambda in AuthEndpoints.cs (resolved but never used since login moved to SessionClaimBuilder). - Extract ApplyValidationResultAsync helper from OnValidatePrincipalAsync (pure decision-application step); add 3 adapter tests covering Reject → RejectPrincipal + SignOutAsync; Replace → ReplacePrincipal + ShouldRenew; Keep → no-op. - Fix inaccurate TryRefreshAsync comment (dropped "OR last-activity needs advancing" — the code only returns non-null when roleRefreshDue). - Add InternalsVisibleTo for Security.Tests in Security.csproj. - Add IsRoleRefreshDue tests: missing claim → due; unparsable claim → due; plus integration test covering the full ValidateAsync path for a principal missing zb:lastrolerefresh (triggers refresh + re-stamps anchor rather than keeping stale principal forever). - Add SecurityOptionsValidatorConfigGuardTests: default succeeds; equal fails; greater fails; boundary (idle-1) succeeds; wiring confirmed via AddSecurity container.
This commit is contained in:
Joseph Doherty
@@ -1,5 +1,8 @@
|
||||
using System.Security.Claims;
|
||||
using Microsoft.AspNetCore.Authentication;
|
||||
using Microsoft.AspNetCore.Authentication.Cookies;
|
||||
using Microsoft.AspNetCore.Http;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Microsoft.Extensions.Options;
|
||||
using ZB.MOM.WW.Auth.Abstractions.Roles;
|
||||
@@ -269,6 +272,81 @@ public class CookieSessionValidatorTests
|
||||
Assert.False(sut.IsRoleRefreshDue(principal, Start.AddMinutes(15)));
|
||||
Assert.True(sut.IsRoleRefreshDue(principal, Start.AddMinutes(15).AddSeconds(1)));
|
||||
}
|
||||
|
||||
// --- Missing/unparsable zb:lastrolerefresh anchor treated as refresh-due ---
|
||||
|
||||
[Fact]
|
||||
public async Task ValidateAsync_MissingLastRoleRefreshClaim_TreatedAsRefreshDue_RefreshesAndRestamps()
|
||||
{
|
||||
// Mirrors the existing MissingLastActivityClaim_TreatedAsIdleTimedOut test but for
|
||||
// the role-refresh anchor. A principal whose zb:lastrolerefresh claim is absent or
|
||||
// unparsable must be treated as "refresh due" (refresh now + re-stamp the anchor),
|
||||
// NOT as "never refresh". This prevents a stale principal from coasting forever
|
||||
// without a role re-check just because the claim was missing.
|
||||
var clock = new TestTimeProvider(Start);
|
||||
// The mapper returns a different mapping than the one encoded in the session, so we
|
||||
// can confirm the refresh actually ran.
|
||||
var mapper = new FakeGroupRoleMapper(new RoleMappingResult([Roles.Viewer], [], false));
|
||||
var sut = CreateValidator(mapper, clock);
|
||||
|
||||
// Build a principal that has a valid LastActivity (not timed out) but NO lastrolerefresh.
|
||||
var identity = new ClaimsIdentity([
|
||||
new Claim(ClaimTypes.Name, "alice"),
|
||||
new Claim(JwtTokenService.UsernameClaimType, "alice"),
|
||||
new Claim(JwtTokenService.DisplayNameClaimType, "Alice"),
|
||||
new Claim(JwtTokenService.RoleClaimType, Roles.Administrator),
|
||||
new Claim(JwtTokenService.LastActivityClaimType, Start.ToString("o")),
|
||||
// Deliberately NO zb:lastrolerefresh claim.
|
||||
], CookieAuthenticationDefaults.AuthenticationScheme,
|
||||
nameType: ClaimTypes.Name,
|
||||
roleType: JwtTokenService.RoleClaimType);
|
||||
var principal = new ClaimsPrincipal(identity);
|
||||
|
||||
var result = await sut.ValidateAsync(principal);
|
||||
|
||||
// Must have triggered a refresh (Replace), NOT Keep.
|
||||
Assert.Equal(SessionValidationAction.Replace, result.Action);
|
||||
Assert.Equal(1, mapper.CallCount);
|
||||
|
||||
// The rebuilt principal carries the new mapping (Viewer, not Administrator).
|
||||
var newRoles = result.Principal!.FindAll(JwtTokenService.RoleClaimType).Select(c => c.Value).ToList();
|
||||
Assert.Contains(Roles.Viewer, newRoles);
|
||||
Assert.DoesNotContain(Roles.Administrator, newRoles);
|
||||
|
||||
// A new lastrolerefresh anchor was stamped.
|
||||
var refreshClaim = result.Principal.FindFirst(JwtTokenService.LastRoleRefreshClaimType);
|
||||
Assert.NotNull(refreshClaim);
|
||||
Assert.True(DateTimeOffset.TryParse(refreshClaim!.Value, out _));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void IsRoleRefreshDue_MissingClaim_ReturnsDue()
|
||||
{
|
||||
// Direct helper test: a principal with no zb:lastrolerefresh claim returns true.
|
||||
var clock = new TestTimeProvider(Start);
|
||||
var sut = CreateValidator(new FakeGroupRoleMapper(new RoleMappingResult([], [], false)), clock);
|
||||
var identity = new ClaimsIdentity([new Claim(ClaimTypes.Name, "alice")],
|
||||
CookieAuthenticationDefaults.AuthenticationScheme);
|
||||
var principal = new ClaimsPrincipal(identity);
|
||||
|
||||
Assert.True(sut.IsRoleRefreshDue(principal, Start));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void IsRoleRefreshDue_UnparsableClaim_ReturnsDue()
|
||||
{
|
||||
// A present but unparsable zb:lastrolerefresh claim is treated as due, not as a
|
||||
// parse error that keeps the session stale forever.
|
||||
var clock = new TestTimeProvider(Start);
|
||||
var sut = CreateValidator(new FakeGroupRoleMapper(new RoleMappingResult([], [], false)), clock);
|
||||
var identity = new ClaimsIdentity([
|
||||
new Claim(ClaimTypes.Name, "alice"),
|
||||
new Claim(JwtTokenService.LastRoleRefreshClaimType, "not-a-date"),
|
||||
], CookieAuthenticationDefaults.AuthenticationScheme);
|
||||
var principal = new ClaimsPrincipal(identity);
|
||||
|
||||
Assert.True(sut.IsRoleRefreshDue(principal, Start));
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -360,3 +438,249 @@ public class SessionClaimBuilderParityTests
|
||||
}
|
||||
|
||||
#endregion
|
||||
|
||||
#region M2.19 (#15): OnValidatePrincipalAsync adapter — translation from SessionValidationResult to cookie context calls
|
||||
|
||||
/// <summary>
|
||||
/// Tests for the <c>ApplyValidationResultAsync</c> helper extracted from
|
||||
/// <see cref="ServiceCollectionExtensions.OnValidatePrincipalAsync"/>: verifies that each
|
||||
/// <see cref="SessionValidationResult"/> case is correctly translated into the
|
||||
/// corresponding <see cref="CookieValidatePrincipalContext"/> mutations.
|
||||
/// The pure decision-application step is tested in isolation — the DI-resolution of
|
||||
/// <see cref="CookieSessionValidator"/> is a separate concern covered by the integration
|
||||
/// wiring tests in <see cref="SecurityReviewRegressionTests"/>.
|
||||
/// </summary>
|
||||
public class OnValidatePrincipalAdapterTests
|
||||
{
|
||||
private static readonly DateTimeOffset AdapterStart = new(2026, 6, 15, 12, 0, 0, TimeSpan.Zero);
|
||||
|
||||
// A minimal IAuthenticationService stub that records SignOut calls so we can assert
|
||||
// the Reject path actually invokes SignOutAsync.
|
||||
private sealed class StubAuthenticationService : IAuthenticationService
|
||||
{
|
||||
public int SignOutCallCount { get; private set; }
|
||||
|
||||
public Task<AuthenticateResult> AuthenticateAsync(HttpContext context, string? scheme) =>
|
||||
Task.FromResult(AuthenticateResult.NoResult());
|
||||
|
||||
public Task ChallengeAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) =>
|
||||
Task.CompletedTask;
|
||||
|
||||
public Task ForbidAsync(HttpContext context, string? scheme, AuthenticationProperties? properties) =>
|
||||
Task.CompletedTask;
|
||||
|
||||
public Task SignInAsync(HttpContext context, string? scheme, ClaimsPrincipal principal, AuthenticationProperties? properties) =>
|
||||
Task.CompletedTask;
|
||||
|
||||
public Task SignOutAsync(HttpContext context, string? scheme, AuthenticationProperties? properties)
|
||||
{
|
||||
SignOutCallCount++;
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
}
|
||||
|
||||
private static (CookieValidatePrincipalContext Context, StubAuthenticationService AuthService)
|
||||
BuildContext(ClaimsPrincipal principal)
|
||||
{
|
||||
var authService = new StubAuthenticationService();
|
||||
|
||||
var services = new ServiceCollection();
|
||||
services.AddSingleton<IAuthenticationService>(authService);
|
||||
services.AddLogging();
|
||||
var serviceProvider = services.BuildServiceProvider();
|
||||
|
||||
var httpContext = new DefaultHttpContext { RequestServices = serviceProvider };
|
||||
var ticket = new AuthenticationTicket(
|
||||
principal,
|
||||
new AuthenticationProperties(),
|
||||
CookieAuthenticationDefaults.AuthenticationScheme);
|
||||
var scheme = new AuthenticationScheme(
|
||||
CookieAuthenticationDefaults.AuthenticationScheme,
|
||||
displayName: null,
|
||||
handlerType: typeof(CookieAuthenticationHandler));
|
||||
|
||||
var context = new CookieValidatePrincipalContext(
|
||||
httpContext,
|
||||
scheme,
|
||||
new CookieAuthenticationOptions(),
|
||||
ticket);
|
||||
|
||||
return (context, authService);
|
||||
}
|
||||
|
||||
private static ClaimsPrincipal AuthenticatedPrincipal() =>
|
||||
SessionClaimBuilder.Build(
|
||||
"alice", "Alice",
|
||||
["SCADA-Admins"],
|
||||
new RoleMappingResult([Roles.Administrator], [], true),
|
||||
AdapterStart);
|
||||
|
||||
[Fact]
|
||||
public async Task ApplyResult_Reject_CallsRejectPrincipalAndSignsOut()
|
||||
{
|
||||
// The Reject result is the idle-timeout path: the adapter must call RejectPrincipal
|
||||
// (sets Principal = null) AND SignOutAsync so the cookie is cleared for the next request.
|
||||
var principal = AuthenticatedPrincipal();
|
||||
var (ctx, authService) = BuildContext(principal);
|
||||
|
||||
await ServiceCollectionExtensions.ApplyValidationResultAsync(ctx, SessionValidationResult.Reject);
|
||||
|
||||
// RejectPrincipal() sets Principal to null.
|
||||
Assert.Null(ctx.Principal);
|
||||
// SignOutAsync must have been invoked exactly once.
|
||||
Assert.Equal(1, authService.SignOutCallCount);
|
||||
// ShouldRenew must NOT be set — we are expiring, not renewing.
|
||||
Assert.False(ctx.ShouldRenew);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ApplyResult_Replace_ReplacesAndSetsRenew()
|
||||
{
|
||||
// The Replace result is the role-refresh path: the adapter must swap in the new
|
||||
// principal and set ShouldRenew = true so the cookie is re-issued with the new claims.
|
||||
var newPrincipal = SessionClaimBuilder.Build(
|
||||
"alice", "Alice",
|
||||
["SCADA-Admins"],
|
||||
new RoleMappingResult([Roles.Viewer], [], false),
|
||||
AdapterStart.AddMinutes(16));
|
||||
var originalPrincipal = AuthenticatedPrincipal();
|
||||
var (ctx, authService) = BuildContext(originalPrincipal);
|
||||
|
||||
await ServiceCollectionExtensions.ApplyValidationResultAsync(ctx, SessionValidationResult.Replace(newPrincipal));
|
||||
|
||||
Assert.Same(newPrincipal, ctx.Principal);
|
||||
Assert.True(ctx.ShouldRenew);
|
||||
Assert.Equal(0, authService.SignOutCallCount);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ApplyResult_Keep_LeavesContextUnchanged()
|
||||
{
|
||||
// The Keep result is the no-op path (no refresh due, or a swallowed refresh fault):
|
||||
// the adapter must leave the principal and ShouldRenew completely untouched.
|
||||
var principal = AuthenticatedPrincipal();
|
||||
var (ctx, authService) = BuildContext(principal);
|
||||
|
||||
await ServiceCollectionExtensions.ApplyValidationResultAsync(ctx, SessionValidationResult.Keep);
|
||||
|
||||
Assert.Same(principal, ctx.Principal);
|
||||
Assert.False(ctx.ShouldRenew);
|
||||
Assert.Equal(0, authService.SignOutCallCount);
|
||||
}
|
||||
}
|
||||
|
||||
#endregion
|
||||
|
||||
#region M2.19 (#15): SecurityOptionsValidator — config-guard fails startup on misconfiguration
|
||||
|
||||
/// <summary>
|
||||
/// Tests for <see cref="SecurityOptionsValidator"/>: the startup validator that prevents
|
||||
/// idle-timeout enforcement from being silently defeated by a misconfigured
|
||||
/// <c>RoleRefreshThresholdMinutes >= IdleTimeoutMinutes</c>.
|
||||
/// </summary>
|
||||
public class SecurityOptionsValidatorConfigGuardTests
|
||||
{
|
||||
private static SecurityOptions ValidOptions() => new()
|
||||
{
|
||||
JwtSigningKey = "this-is-a-test-signing-key-for-hmac-sha256-must-be-long-enough",
|
||||
IdleTimeoutMinutes = 30,
|
||||
RoleRefreshThresholdMinutes = 15,
|
||||
};
|
||||
|
||||
[Fact]
|
||||
public void Validate_DefaultOptions_Succeeds()
|
||||
{
|
||||
var result = new SecurityOptionsValidator().Validate(name: null, ValidOptions());
|
||||
Assert.True(result.Succeeded);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_RefreshEqualsIdle_Fails()
|
||||
{
|
||||
// Equal is invalid: a refresh cycle at t=30 would advance LastActivity to t=30 which
|
||||
// equals the idle timeout — enforcement is defeated.
|
||||
var options = ValidOptions();
|
||||
options.RoleRefreshThresholdMinutes = options.IdleTimeoutMinutes; // 30 == 30
|
||||
|
||||
var result = new SecurityOptionsValidator().Validate(name: null, options);
|
||||
|
||||
Assert.True(result.Failed);
|
||||
Assert.Contains(nameof(SecurityOptions.RoleRefreshThresholdMinutes), result.FailureMessage);
|
||||
Assert.Contains(nameof(SecurityOptions.IdleTimeoutMinutes), result.FailureMessage);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_RefreshGreaterThanIdle_Fails()
|
||||
{
|
||||
// Inverted: refresh threshold LARGER than the idle window is clearly wrong and
|
||||
// must fail loudly at startup.
|
||||
var options = ValidOptions();
|
||||
options.RoleRefreshThresholdMinutes = 60; // 60 > 30
|
||||
|
||||
var result = new SecurityOptionsValidator().Validate(name: null, options);
|
||||
|
||||
Assert.True(result.Failed);
|
||||
Assert.Contains(nameof(SecurityOptions.RoleRefreshThresholdMinutes), result.FailureMessage);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_RefreshOneLessThanIdle_Succeeds()
|
||||
{
|
||||
// Boundary: threshold = idle - 1 is the tightest VALID configuration.
|
||||
var options = ValidOptions();
|
||||
options.IdleTimeoutMinutes = 30;
|
||||
options.RoleRefreshThresholdMinutes = 29;
|
||||
|
||||
var result = new SecurityOptionsValidator().Validate(name: null, options);
|
||||
|
||||
Assert.True(result.Succeeded);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AddSecurity_RegistersSecurityOptionsValidator_WithValidateOnStart()
|
||||
{
|
||||
// End-to-end wiring: AddSecurity registers SecurityOptionsValidator as
|
||||
// IValidateOptions<SecurityOptions>. ValidateOnStart is what makes the DI container
|
||||
// call Validate on startup. We confirm the validator is present in the container.
|
||||
var services = new ServiceCollection();
|
||||
services.AddLogging();
|
||||
services.AddDataProtection();
|
||||
services.AddSecurity();
|
||||
|
||||
using var provider = services.BuildServiceProvider();
|
||||
var validators = provider.GetServices<IValidateOptions<SecurityOptions>>().ToList();
|
||||
Assert.Contains(validators, v => v is SecurityOptionsValidator);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AddSecurity_RegisteredValidator_DetectsMisconfiguration_WhenCalledDirectly()
|
||||
{
|
||||
// Confirm that the SecurityOptionsValidator registered by AddSecurity actually
|
||||
// catches a bad configuration. We resolve the validator from the container and
|
||||
// call it directly — the ValidateOnStart pipeline would fire the same validator
|
||||
// during IHost.StartAsync in a real deployment.
|
||||
var services = new ServiceCollection();
|
||||
services.AddLogging();
|
||||
services.AddDataProtection();
|
||||
services.AddSecurity();
|
||||
|
||||
using var provider = services.BuildServiceProvider();
|
||||
var validator = provider.GetServices<IValidateOptions<SecurityOptions>>()
|
||||
.OfType<SecurityOptionsValidator>()
|
||||
.Single();
|
||||
|
||||
// A configuration where RoleRefreshThreshold == IdleTimeout defeats idle enforcement.
|
||||
var badOptions = new SecurityOptions
|
||||
{
|
||||
JwtSigningKey = "this-is-a-test-signing-key-for-hmac-sha256-must-be-long-enough",
|
||||
IdleTimeoutMinutes = 30,
|
||||
RoleRefreshThresholdMinutes = 30, // == IdleTimeoutMinutes — invalid
|
||||
};
|
||||
var result = validator.Validate(name: null, badOptions);
|
||||
|
||||
Assert.True(result.Failed);
|
||||
Assert.Contains(nameof(SecurityOptions.RoleRefreshThresholdMinutes), result.FailureMessage);
|
||||
}
|
||||
}
|
||||
|
||||
#endregion
|
||||
|
||||
Reference in New Issue
Block a user