diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 76f76be..d0caf76 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -138,13 +138,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `Services/ReservationService.cs:28-37` | -| Status | Open | +| Status | Resolved | **Description:** `ReservationService.ReleaseAsync` calls `sp_ReleaseExternalIdReservation` with only `@Kind`, `@Value`, `@ReleaseReason`. `admin-ui.md` section "Release an external-ID reservation" specifies the proc sets `ReleasedBy` to the FleetAdmin who performed the release, and the action is the only path that allows ZTag/SAPID reuse and "requires explicit FleetAdmin action with a documented reason." The service does not capture or pass the operator principal, so the compliance audit trail for a release records no actor (unless the proc derives it from the DB session login, which would be the shared service account, not the operator). **Recommendation:** Add an operator-principal parameter to `ReleaseAsync`, pass it to the stored proc as `@ReleasedBy`, and have callers supply the signed-in user. Confirm the proc signature accepts it. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — a new EF migration (`20260522000001_AddReleasedByToReleaseExternalIdReservation`) adds `@ReleasedBy nvarchar(128)` to `sp_ReleaseExternalIdReservation` and uses it for both `ExternalIdReservation.ReleasedBy` and `ConfigAuditLog.Principal` (replacing `SUSER_SNAME()`); `ReservationService.ReleaseAsync` gains a `releasedBy` parameter with a guard; `Reservations.razor` resolves `ClaimTypes.Name` / `ClaimTypes.NameIdentifier` from the cascaded `AuthenticationState` and passes the operator principal to the service. ### Admin-009 diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260522000001_AddReleasedByToReleaseExternalIdReservation.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260522000001_AddReleasedByToReleaseExternalIdReservation.cs new file mode 100644 index 0000000..c337481 --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260522000001_AddReleasedByToReleaseExternalIdReservation.cs @@ -0,0 +1,120 @@ +using Microsoft.EntityFrameworkCore.Migrations; + +#nullable disable + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Migrations +{ + /// + /// Admin-008: adds @ReleasedBy parameter to + /// dbo.sp_ReleaseExternalIdReservation so the operator principal name (the LDAP + /// sign-in) is recorded in ExternalIdReservation.ReleasedBy and the + /// ConfigAuditLog.Principal column. + /// + /// Prior to this migration the proc used SUSER_SNAME() for both columns, which + /// recorded the shared SQL service account rather than the Admin-UI operator who performed + /// the release — making the audit trail useless for attribution. The stored proc now + /// accepts @ReleasedBy nvarchar(128) and uses it for both columns; validation + /// rejects a null/empty value the same way @ReleaseReason is validated. + /// + /// + public partial class AddReleasedByToReleaseExternalIdReservation : Migration + { + /// + protected override void Up(MigrationBuilder migrationBuilder) + { + migrationBuilder.Sql(Procs.ReleaseExternalIdReservationV2); + } + + /// + protected override void Down(MigrationBuilder migrationBuilder) + { + migrationBuilder.Sql(Procs.ReleaseExternalIdReservationV1); + } + + private static class Procs + { + /// V2 — accepts @ReleasedBy for proper operator attribution. + public const string ReleaseExternalIdReservationV2 = @" +CREATE OR ALTER PROCEDURE dbo.sp_ReleaseExternalIdReservation + @Kind nvarchar(16), + @Value nvarchar(64), + @ReleaseReason nvarchar(512), + @ReleasedBy nvarchar(128) +AS +BEGIN + SET NOCOUNT ON; + SET XACT_ABORT ON; + + IF @ReleaseReason IS NULL OR LEN(@ReleaseReason) = 0 + BEGIN + RAISERROR('ReleaseReason is required', 16, 1); + RETURN; + END + + IF @ReleasedBy IS NULL OR LEN(@ReleasedBy) = 0 + BEGIN + RAISERROR('ReleasedBy is required', 16, 1); + RETURN; + END + + UPDATE dbo.ExternalIdReservation + SET ReleasedAt = SYSUTCDATETIME(), + ReleasedBy = @ReleasedBy, + ReleaseReason = @ReleaseReason + WHERE Kind = @Kind AND Value = @Value AND ReleasedAt IS NULL; + + IF @@ROWCOUNT = 0 + BEGIN + RAISERROR('No active reservation found for (%s, %s)', 16, 1, @Kind, @Value); + RETURN; + END + + -- Escape all caller-supplied values via STRING_ESCAPE so quotes/backslashes cannot break the + -- JSON document or inject additional structure into the audit record. + INSERT dbo.ConfigAuditLog (Principal, EventType, DetailsJson) + VALUES (@ReleasedBy, 'ExternalIdReleased', + CONCAT('{""kind"":""', STRING_ESCAPE(@Kind, 'json'), + '"",""value"":""', STRING_ESCAPE(@Value, 'json'), '""}')); +END +"; + + /// V1 — original proc (uses SUSER_SNAME() for attribution). Restored on Down(). + public const string ReleaseExternalIdReservationV1 = @" +CREATE OR ALTER PROCEDURE dbo.sp_ReleaseExternalIdReservation + @Kind nvarchar(16), + @Value nvarchar(64), + @ReleaseReason nvarchar(512) +AS +BEGIN + SET NOCOUNT ON; + SET XACT_ABORT ON; + + IF @ReleaseReason IS NULL OR LEN(@ReleaseReason) = 0 + BEGIN + RAISERROR('ReleaseReason is required', 16, 1); + RETURN; + END + + UPDATE dbo.ExternalIdReservation + SET ReleasedAt = SYSUTCDATETIME(), + ReleasedBy = SUSER_SNAME(), + ReleaseReason = @ReleaseReason + WHERE Kind = @Kind AND Value = @Value AND ReleasedAt IS NULL; + + IF @@ROWCOUNT = 0 + BEGIN + RAISERROR('No active reservation found for (%s, %s)', 16, 1, @Kind, @Value); + RETURN; + END + + -- Escape both caller-supplied values via STRING_ESCAPE so quotes/backslashes cannot break the + -- JSON document or inject additional structure into the audit record. + INSERT dbo.ConfigAuditLog (Principal, EventType, DetailsJson) + VALUES (SUSER_SNAME(), 'ExternalIdReleased', + CONCAT('{""kind"":""', STRING_ESCAPE(@Kind, 'json'), + '"",""value"":""', STRING_ESCAPE(@Value, 'json'), '""}')); +END +"; + } + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Reservations.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Reservations.razor index 35e6f89..0676367 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Reservations.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Reservations.razor @@ -1,5 +1,7 @@ @page "/reservations" +@using System.Security.Claims @using Microsoft.AspNetCore.Authorization +@using Microsoft.AspNetCore.Components.Authorization @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Admin.Services @using ZB.MOM.WW.OtOpcUa.Configuration.Entities @@ -86,6 +88,10 @@ } @code { + // Admin-008: capture the signed-in operator so the release is attributed correctly in the + // ExternalIdReservation.ReleasedBy column and the ConfigAuditLog.Principal column. + [CascadingParameter] private Task? AuthState { get; set; } + private List? _active; private List? _released; private ExternalIdReservation? _releasing; @@ -111,10 +117,20 @@ private async Task ReleaseAsync() { if (_releasing is null || string.IsNullOrWhiteSpace(_reason)) { _error = "Reason is required"; return; } + + // Resolve the operator principal. The page is [Authorize(Policy="CanPublish")] so + // AuthState will be available with an authenticated user; fall back to "unknown" only + // as a defensive last resort (should never happen in practice). + var user = AuthState is not null ? (await AuthState).User : null; + var operatorName = user?.FindFirstValue(ClaimTypes.Name) + ?? user?.FindFirstValue(ClaimTypes.NameIdentifier) + ?? "unknown"; + _busy = true; try { - await ReservationSvc.ReleaseAsync(_releasing.Kind.ToString(), _releasing.Value, _reason, CancellationToken.None); + await ReservationSvc.ReleaseAsync( + _releasing.Kind.ToString(), _releasing.Value, _reason, operatorName, CancellationToken.None); _releasing = null; await ReloadAsync(); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Services/ReservationService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Services/ReservationService.cs index 955dde2..a08d105 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Services/ReservationService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Services/ReservationService.cs @@ -25,14 +25,24 @@ public sealed class ReservationService(OtOpcUaConfigDbContext db) .Take(100) .ToListAsync(ct); - public async Task ReleaseAsync(string kind, string value, string reason, CancellationToken ct) + /// + /// Releases an active reservation, recording (the signed-in + /// Admin-UI operator) in ExternalIdReservation.ReleasedBy and the + /// ConfigAuditLog.Principal column. + /// + /// Both and are required audit + /// fields — the stored proc validates them and raises an error if either is null/empty. + /// + public async Task ReleaseAsync(string kind, string value, string reason, string releasedBy, CancellationToken ct) { if (string.IsNullOrWhiteSpace(reason)) throw new ArgumentException("ReleaseReason is required (audit invariant)", nameof(reason)); + if (string.IsNullOrWhiteSpace(releasedBy)) + throw new ArgumentException("ReleasedBy is required (audit invariant)", nameof(releasedBy)); await db.Database.ExecuteSqlRawAsync( - "EXEC dbo.sp_ReleaseExternalIdReservation @Kind = {0}, @Value = {1}, @ReleaseReason = {2}", - [kind, value, reason], + "EXEC dbo.sp_ReleaseExternalIdReservation @Kind = {0}, @Value = {1}, @ReleaseReason = {2}, @ReleasedBy = {3}", + [kind, value, reason, releasedBy], ct); } }