diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/IAuditActorAccessor.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/IAuditActorAccessor.cs new file mode 100644 index 00000000..46ffb98f --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/IAuditActorAccessor.cs @@ -0,0 +1,31 @@ +namespace ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; + +/// +/// Resolves the Actor for an audit row from the current authenticated +/// principal (Phase 3 of the audit re-architecture). User-facing emit sites +/// (the inbound API middleware on a cookie/LDAP-authenticated request) read +/// so the canonical AuditEvent.Actor records +/// the real authenticated user, rather than a generic system/identity fallback. +/// +/// +/// The seam is deliberately ASP.NET-free (a plain string?) so it can +/// live in Commons and be consumed by any project without pulling an HTTP +/// dependency. The HTTP-backed implementation +/// (ZB.MOM.WW.ScadaBridge.Security.HttpAuditActorAccessor) reads the +/// authenticated principal off IHttpContextAccessor.HttpContext?.User. +/// This seam is for the authenticated, interactive actor only. +/// System-originated emitters (script/notification/db-outbound) keep their own +/// system actor/fallback and do NOT consult this accessor — there is no +/// interactive principal to read in those flows. +/// +public interface IAuditActorAccessor +{ + /// + /// The actor string for the currently authenticated principal, or + /// null when there is no authenticated interactive user (no ambient + /// request, or an unauthenticated / auth-failure request). A null result + /// signals the caller to fall back to its existing actor (API-key name, + /// "system", etc.) — an unauthenticated principal is never echoed back. + /// + string? CurrentActor { get; } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs index 53feef8d..3fce05b9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs @@ -33,14 +33,18 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Middleware; /// /// /// -/// Actor resolution. Inbound API auth runs inside the endpoint handler +/// Actor resolution. The API-key auth path runs inside the endpoint handler /// (no UseAuthentication-backed scheme populates -/// for X-API-Key callers), so the handler stashes the resolved API key name on +/// for Bearer API-key callers), so the handler stashes the resolved API key name on /// under after /// IApiKeyVerifier.VerifyAsync succeeds. The middleware reads it in -/// its finally block — on auth failures the key remains absent and -/// stays null (we never echo back an -/// unauthenticated principal). +/// its finally block. Phase 3: when no API-key name is stashed, the actor is +/// sourced from the authenticated interactive principal via +/// (a cookie/LDAP-authenticated inbound user, +/// keyed off the canonical username claim). On auth failures (401/403) the actor is +/// forced null before resolution runs, and the accessor itself returns null for an +/// unauthenticated principal — so stays null and we +/// never echo back an unauthenticated principal. /// /// /// @@ -90,6 +94,7 @@ public sealed class AuditWriteMiddleware private readonly ICentralAuditWriter _auditWriter; private readonly ILogger _logger; private readonly IOptionsMonitor _options; + private readonly IAuditActorAccessor? _actorAccessor; /// /// Initializes the middleware with its required dependencies. @@ -98,16 +103,25 @@ public sealed class AuditWriteMiddleware /// Central audit writer used to persist inbound API audit events. /// Logger for this middleware. /// Live-reloadable audit log options, read per-request. + /// + /// Phase 3 (optional): resolves the audit from the + /// authenticated principal on a cookie/LDAP-authenticated inbound request. Optional + /// so existing tests (and any composition without the accessor registered) still + /// construct the middleware; when absent, actor resolution falls back to the + /// stashed API-key name only. + /// public AuditWriteMiddleware( RequestDelegate next, ICentralAuditWriter auditWriter, ILogger logger, - IOptionsMonitor options) + IOptionsMonitor options, + IAuditActorAccessor? actorAccessor = null) { _next = next ?? throw new ArgumentNullException(nameof(next)); _auditWriter = auditWriter ?? throw new ArgumentNullException(nameof(auditWriter)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _options = options ?? throw new ArgumentNullException(nameof(options)); + _actorAccessor = actorAccessor; } /// @@ -472,13 +486,24 @@ public sealed class AuditWriteMiddleware } /// - /// Reads the API key name the endpoint handler stashed on - /// after successful auth. Falls back to - /// the authenticated user name when an ASP.NET scheme has populated - /// (defensive — currently unused for inbound - /// API but cheap and forward-compatible). + /// Resolves the audit for a non-auth-failure + /// inbound request: + /// + /// the API key name the endpoint handler stashed on + /// after successful key auth (the + /// key-authenticated path — the canonical identity of an API-key caller); + /// otherwise the authenticated interactive principal + /// resolved through (Phase 3 — a + /// cookie/LDAP-authenticated inbound user, sourced from the canonical username + /// claim). The accessor reads the ambient , so the + /// fall-through here only fires when no API-key name was stashed; + /// otherwise null — never echo an unauthenticated + /// principal back as an actor. + /// + /// The accessor is optional (constructor default null); when absent only + /// the stashed API-key name is consulted, preserving the pre-Phase-3 behaviour. /// - private static string? ResolveActor(HttpContext ctx) + private string? ResolveActor(HttpContext ctx) { if (ctx.Items.TryGetValue(AuditActorItemKey, out var stashed) && stashed is string name @@ -487,13 +512,11 @@ public sealed class AuditWriteMiddleware return name; } - var user = ctx.User; - if (user?.Identity is { IsAuthenticated: true, Name: { Length: > 0 } userName }) - { - return userName; - } - - return null; + // Phase 3: an interactive cookie/LDAP-authenticated inbound user records + // their real identity as Actor. Returns null for the key-authenticated + // and auth-failure paths (no authenticated interactive principal), so the + // existing API-key/auth-failure behaviour is preserved. + return _actorAccessor?.CurrentActor; } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/HttpAuditActorAccessor.cs b/src/ZB.MOM.WW.ScadaBridge.Security/HttpAuditActorAccessor.cs new file mode 100644 index 00000000..31cf1f15 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Security/HttpAuditActorAccessor.cs @@ -0,0 +1,65 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using ZB.MOM.WW.Auth.AspNetCore; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; + +namespace ZB.MOM.WW.ScadaBridge.Security; + +/// +/// HTTP-backed (Phase 3): resolves the audit +/// Actor from the authenticated principal on the ambient +/// . Used by the user-facing +/// inbound API audit path so a cookie/LDAP-authenticated request records the +/// real user as AuditEvent.Actor. +/// +/// +/// The username is sourced from the canonical +/// claim (= , +/// minted by and by the cookie login path), falling +/// back to (which +/// pins to via its +/// token-validation NameClaimType). +/// When there is no ambient request, the principal is unauthenticated, or no +/// usable name claim is present, returns null so +/// the caller keeps its existing actor/fallback — an unauthenticated principal is +/// never echoed back as an actor. +/// +public sealed class HttpAuditActorAccessor : IAuditActorAccessor +{ + private readonly IHttpContextAccessor _httpContextAccessor; + + /// Initializes a new instance of . + /// Accessor for the ambient HTTP context. + public HttpAuditActorAccessor(IHttpContextAccessor httpContextAccessor) + { + _httpContextAccessor = httpContextAccessor + ?? throw new ArgumentNullException(nameof(httpContextAccessor)); + } + + /// + public string? CurrentActor + { + get + { + var user = _httpContextAccessor.HttpContext?.User; + if (user?.Identity is not { IsAuthenticated: true }) + { + // No ambient request, or the principal is unauthenticated — never + // echo an unauthenticated identity back as an actor. + return null; + } + + // Prefer the canonical username claim (the value JwtTokenService and + // the cookie login path mint); fall back to Identity.Name (pinned to + // ZbClaimTypes.Name by JwtTokenService.NameClaimType). + var username = user.FindFirst(JwtTokenService.UsernameClaimType)?.Value; + if (!string.IsNullOrWhiteSpace(username)) + { + return username; + } + + var name = user.Identity?.Name; + return string.IsNullOrWhiteSpace(name) ? null : name; + } + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs index 3e157093..faa006b1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs @@ -4,6 +4,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.Roles; using ZB.MOM.WW.Auth.AspNetCore; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; namespace ZB.MOM.WW.ScadaBridge.Security; @@ -49,6 +50,18 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddScoped(); + // Audit Actor wiring (Phase 3): the user-facing inbound API audit path + // sources AuditEvent.Actor from the authenticated principal via this + // seam. HttpAuditActorAccessor reads IHttpContextAccessor.HttpContext?.User + // (canonical username claim, Identity.Name fallback) and returns null when + // there is no authenticated interactive user — so the caller keeps its + // existing actor/fallback (API-key name, "system"). Registered as a + // singleton (it is stateless and only dereferences the ambient request); + // AddHttpContextAccessor is idempotent (TryAdd-based) so calling it here + // is safe even though the Host's AddCentralUI also registers it. + services.AddHttpContextAccessor(); + services.AddSingleton(); + // Auth-adoption Task 1.1: register the shared IGroupRoleMapper // seam additively, wrapping RoleMapper to reuse its DB-backed mapping + // site-scope union semantics. Scoped to match RoleMapper's lifetime (it diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj b/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj index 5708eeb2..c6e75280 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj @@ -8,10 +8,15 @@ - - - - + + diff --git a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Audit/ScadaBridgeAuditEventFactoryActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Audit/ScadaBridgeAuditEventFactoryActorTests.cs new file mode 100644 index 00000000..70b55bf2 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Audit/ScadaBridgeAuditEventFactoryActorTests.cs @@ -0,0 +1,64 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; + +namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Types.Audit; + +/// +/// Phase 3 (wire audit Actor from the Auth principal): the +/// is the single construction point and +/// records the actor the CALLER passes in — it never injects a principal of +/// its own. These tests pin that contract so the per-emit-site decision holds: +/// authenticated emit sites pass the principal's actor (sourced via +/// IAuditActorAccessor at the inbound middleware), while system-originated +/// emitters (notification / script DB-outbound) keep passing their own system/script +/// actor unchanged. The factory does not blur the two. +/// +public class ScadaBridgeAuditEventFactoryActorTests +{ + [Theory] + // Mirrors the literal system actors the outbound emitters pass: + // NotificationOutboxActor → "system"; AuditingDbCommand → the source script. + [InlineData("system")] + [InlineData("order-sync.caspx")] + public void SystemOriginatedEmit_PreservesCallerActor_Verbatim(string systemActor) + { + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.Notification, + kind: AuditKind.NotifyDeliver, + status: AuditStatus.Delivered, + actor: systemActor); + + // The system emit keeps its system/script actor — the factory does not + // overwrite it with any authenticated principal. + Assert.Equal(systemActor, evt.Actor); + } + + [Fact] + public void AuthenticatedEmit_PreservesCallerActor_Verbatim() + { + // An authenticated emit site (e.g. the inbound middleware) passes the + // principal's actor; the factory records it as-is. + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiInbound, + kind: AuditKind.InboundRequest, + status: AuditStatus.Delivered, + actor: "alice"); + + Assert.Equal("alice", evt.Actor); + } + + [Fact] + public void NullActor_MapsToEmptyString_OnCanonicalRecord() + { + // The canonical AuditEvent.Actor is a non-null string; a null actor (no + // authenticated principal AND no system fallback supplied) maps to empty. + // AuditRowProjection then surfaces empty as null at the row boundary. + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiInbound, + kind: AuditKind.InboundAuthFailure, + status: AuditStatus.Failed, + actor: null); + + Assert.Equal(string.Empty, evt.Actor); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/Middleware/AuditWriteMiddlewareTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/Middleware/AuditWriteMiddlewareTests.cs index 10577cd4..bf16ce10 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/Middleware/AuditWriteMiddlewareTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/Middleware/AuditWriteMiddlewareTests.cs @@ -85,12 +85,26 @@ public class AuditWriteMiddlewareTests private static AuditWriteMiddleware CreateMiddleware( RequestDelegate next, ICentralAuditWriter writer, - AuditLogOptions? options = null) => + AuditLogOptions? options = null, + IAuditActorAccessor? actorAccessor = null) => new( next, writer, NullLogger.Instance, - new StaticAuditLogOptionsMonitor(options ?? new AuditLogOptions())); + new StaticAuditLogOptionsMonitor(options ?? new AuditLogOptions()), + actorAccessor); + + /// + /// File-local test double returning a fixed + /// actor string — stands in for the HTTP-backed accessor that reads the + /// authenticated principal off the ambient request (Phase 3). + /// + private sealed class StubAuditActorAccessor : IAuditActorAccessor + { + public StubAuditActorAccessor(string? currentActor) => CurrentActor = currentActor; + + public string? CurrentActor { get; } + } /// /// File-local test double — returns the @@ -278,6 +292,105 @@ public class AuditWriteMiddlewareTests Assert.Equal("integration-svc", evt.Actor); } + // --------------------------------------------------------------------- + // 5b. Phase 3 — Actor from the authenticated principal. When no API-key + // name was stashed, the actor is sourced from IAuditActorAccessor + // (the authenticated interactive cookie/LDAP user). The API-key stash + // still takes precedence, and auth-failure / no-principal paths stay + // null — never echo an unauthenticated principal back. + // --------------------------------------------------------------------- + + [Fact] + public async Task AuthenticatedUser_FromAccessor_RecordedAsActor_WhenNoApiKeyStash() + { + var writer = new RecordingAuditWriter(); + var ctx = BuildContext(); + var mw = CreateMiddleware( + _ => + { + // No API-key name stashed — this is an interactive cookie/LDAP + // authenticated inbound user, surfaced via the accessor. + ctx.Response.StatusCode = 200; + return Task.CompletedTask; + }, + writer, + actorAccessor: new StubAuditActorAccessor("alice")); + + await mw.InvokeAsync(ctx); + + var evt = Assert.Single(writer.Events); + Assert.Equal("alice", evt.Actor); + } + + [Fact] + public async Task ApiKeyStash_TakesPrecedence_OverAuthenticatedPrincipal() + { + // A key-authenticated caller: the endpoint handler stashed the API key + // name. Even if an accessor would resolve a principal, the API-key + // identity is the canonical actor for the key-authenticated path. + var writer = new RecordingAuditWriter(); + var ctx = BuildContext(); + var mw = CreateMiddleware( + _ => + { + ctx.Items[AuditWriteMiddleware.AuditActorItemKey] = "integration-svc"; + ctx.Response.StatusCode = 200; + return Task.CompletedTask; + }, + writer, + actorAccessor: new StubAuditActorAccessor("should-not-win")); + + await mw.InvokeAsync(ctx); + + var evt = Assert.Single(writer.Events); + Assert.Equal("integration-svc", evt.Actor); + } + + [Fact] + public async Task AuthFailure_KeepsActorNull_EvenWhenAccessorResolvesPrincipal() + { + // 401/403 force the actor null BEFORE resolution — an auth failure must + // never echo a principal back, even one the accessor could produce. + var writer = new RecordingAuditWriter(); + var ctx = BuildContext(); + var mw = CreateMiddleware( + _ => + { + ctx.Response.StatusCode = 401; + return Task.CompletedTask; + }, + writer, + actorAccessor: new StubAuditActorAccessor("attacker")); + + await mw.InvokeAsync(ctx); + + var evt = Assert.Single(writer.Events); + Assert.Equal(AuditKind.InboundAuthFailure, evt.Kind); + Assert.Null(evt.Actor); + } + + [Fact] + public async Task NoApiKey_NoAuthenticatedPrincipal_LeavesActorNull() + { + // Accessor present but returns null (no authenticated interactive user) + // and no API-key stash — the actor stays null rather than empty/echoed. + var writer = new RecordingAuditWriter(); + var ctx = BuildContext(); + var mw = CreateMiddleware( + _ => + { + ctx.Response.StatusCode = 200; + return Task.CompletedTask; + }, + writer, + actorAccessor: new StubAuditActorAccessor(currentActor: null)); + + await mw.InvokeAsync(ctx); + + var evt = Assert.Single(writer.Events); + Assert.Null(evt.Actor); + } + // --------------------------------------------------------------------- // 6. Writer failure must NEVER alter the HTTP response // --------------------------------------------------------------------- diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/HttpAuditActorAccessorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/HttpAuditActorAccessorTests.cs new file mode 100644 index 00000000..3a470157 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/HttpAuditActorAccessorTests.cs @@ -0,0 +1,114 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using ZB.MOM.WW.Auth.AspNetCore; +using ZB.MOM.WW.ScadaBridge.Security; + +namespace ZB.MOM.WW.ScadaBridge.Security.Tests; + +/// +/// Phase 3 (wire audit Actor from the Auth principal): unit tests for +/// . The accessor resolves the audit +/// Actor from the authenticated principal on the ambient +/// — the canonical username claim +/// with an fallback — and +/// returns null whenever there is no authenticated interactive user, so the +/// caller keeps its existing actor/fallback rather than echoing an unauthenticated +/// principal. +/// +public class HttpAuditActorAccessorTests +{ + /// + /// Minimal test double returning a fixed + /// (possibly null) . + /// + private sealed class StubHttpContextAccessor : IHttpContextAccessor + { + public HttpContext? HttpContext { get; set; } + } + + private static HttpContext AuthenticatedContext(params Claim[] claims) + { + var ctx = new DefaultHttpContext + { + User = new ClaimsPrincipal(new ClaimsIdentity(claims, authenticationType: "TestAuth")), + }; + return ctx; + } + + [Fact] + public void CurrentActor_Authenticated_ReturnsUsernameClaim() + { + var ctx = AuthenticatedContext( + new Claim(JwtTokenService.UsernameClaimType, "alice"), + // A different Identity.Name proves the username claim is preferred. + new Claim(ClaimTypes.Name, "Alice Liddell")); + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = ctx }); + + Assert.Equal("alice", accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_Authenticated_NoUsernameClaim_FallsBackToIdentityName() + { + // No canonical username claim; Identity.Name (pinned to ZbClaimTypes.Name) + // is the documented fallback. DefaultHttpContext maps the ClaimTypes.Name + // claim onto Identity.Name. + var ctx = AuthenticatedContext(new Claim(ClaimTypes.Name, "bob")); + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = ctx }); + + Assert.Equal("bob", accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_Authenticated_PrefersUsernameOverZbName() + { + // Both the canonical username and the canonical name claim present — the + // username claim wins. + var ctx = AuthenticatedContext( + new Claim(JwtTokenService.UsernameClaimType, "svc-user"), + new Claim(ZbClaimTypes.Name, "Service User")); + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = ctx }); + + Assert.Equal("svc-user", accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_Unauthenticated_ReturnsNull() + { + // An anonymous identity (no authenticationType) is NOT authenticated — + // never echo it back as an actor even if a name claim is somehow present. + var ctx = new DefaultHttpContext + { + User = new ClaimsPrincipal( + new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "ghost") })), + }; + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = ctx }); + + Assert.Null(accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_NoAmbientHttpContext_ReturnsNull() + { + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = null }); + + Assert.Null(accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_AuthenticatedButNoUsableName_ReturnsNull() + { + // Authenticated identity carrying only an unrelated claim (no username, + // no name) — there is nothing usable to record, so fall back to null. + var ctx = AuthenticatedContext(new Claim(ZbClaimTypes.Role, "Administrator")); + var accessor = new HttpAuditActorAccessor( + new StubHttpContextAccessor { HttpContext = ctx }); + + Assert.Null(accessor.CurrentActor); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj index 26459727..ba5db655 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj @@ -8,14 +8,19 @@ false + + + + + - - - - - +