dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(admin): resolve Medium code-review finding (Admin-008)

Browse Source 
Add @ReleasedBy parameter to sp_ReleaseExternalIdReservation via a new EF
migration so the operator principal (not the shared SQL account) is recorded
in ExternalIdReservation.ReleasedBy and ConfigAuditLog.Principal.
ReservationService.ReleaseAsync gains a releasedBy parameter; Reservations.razor
resolves the signed-in user from AuthenticationState and passes it through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 07:29:54 -04:00
parent 71f91aa57c
commit 328ab1e614
4 changed files with 152 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
code-reviews/Admin/findings.md
View File

@@ -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

120
src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260522000001_AddReleasedByToReleaseExternalIdReservation.cs Normal file
View File

@@ -0,0 +1,120 @@
using Microsoft.EntityFrameworkCore.Migrations;
#nullable disable
namespace ZB.MOM.WW.OtOpcUa.Configuration.Migrations
{
/// <summary>
/// Admin-008: adds <c>@ReleasedBy</c> parameter to
/// <c>dbo.sp_ReleaseExternalIdReservation</c> so the operator principal name (the LDAP
/// sign-in) is recorded in <c>ExternalIdReservation.ReleasedBy</c> and the
/// <c>ConfigAuditLog.Principal</c> column.
///
/// Prior to this migration the proc used <c>SUSER_SNAME()</c> 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 <c>@ReleasedBy nvarchar(128)</c> and uses it for both columns; validation
/// rejects a null/empty value the same way <c>@ReleaseReason</c> is validated.
/// </summary>
/// <inheritdoc />
public partial class AddReleasedByToReleaseExternalIdReservation : Migration
{
/// <inheritdoc />
protected override void Up(MigrationBuilder migrationBuilder)
{
migrationBuilder.Sql(Procs.ReleaseExternalIdReservationV2);
}
/// <inheritdoc />
protected override void Down(MigrationBuilder migrationBuilder)
{
migrationBuilder.Sql(Procs.ReleaseExternalIdReservationV1);
}
private static class Procs
{
/// <summary>V2 — accepts <c>@ReleasedBy</c> for proper operator attribution.</summary>
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
";
/// <summary>V1 — original proc (uses SUSER_SNAME() for attribution). Restored on Down().</summary>
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
";
}
}
}

18
src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Reservations.razor
View File

@@ -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<AuthenticationState>? AuthState { get; set; }
private List<ExternalIdReservation>? _active;
private List<ExternalIdReservation>? _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();
}

16
src/Server/ZB.MOM.WW.OtOpcUa.Admin/Services/ReservationService.cs
View File

@@ -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)
/// <summary>
/// Releases an active reservation, recording <paramref name="releasedBy"/> (the signed-in
/// Admin-UI operator) in <c>ExternalIdReservation.ReleasedBy</c> and the
/// <c>ConfigAuditLog.Principal</c> column.
///
/// Both <paramref name="reason"/> and <paramref name="releasedBy"/> are required audit
/// fields — the stored proc validates them and raises an error if either is null/empty.
/// </summary>
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);
}
}