fix(admin): resolve Medium code-review finding (Admin-007)
NewCluster.razor and ClusterDetail.razor now resolve ClaimTypes.Name / NameIdentifier from the cascaded AuthenticationState instead of hardcoding "admin-ui" as the createdBy audit field. The operator principal is now attributed correctly on every cluster-create and draft-create write path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -123,13 +123,13 @@
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Design-document adherence |
|
| Category | Design-document adherence |
|
||||||
| Location | `Components/Pages/Clusters/NewCluster.razor:91,95-96` |
|
| Location | `Components/Pages/Clusters/NewCluster.razor:91,95-96` |
|
||||||
| Status | Open |
|
| Status | Resolved |
|
||||||
|
|
||||||
**Description:** `NewCluster.CreateAsync` hardcodes `CreatedBy = "admin-ui"` (both on the `ServerCluster` row and the draft generation) instead of the signed-in operator principal name. `admin-ui.md` section "Audit" requires "the operator principal" be recorded on every write. The audit trail therefore cannot attribute cluster creation to a person. The same literal would apply to any anonymous creation that Admin-001/002 currently permit.
|
**Description:** `NewCluster.CreateAsync` hardcodes `CreatedBy = "admin-ui"` (both on the `ServerCluster` row and the draft generation) instead of the signed-in operator principal name. `admin-ui.md` section "Audit" requires "the operator principal" be recorded on every write. The audit trail therefore cannot attribute cluster creation to a person. The same literal would apply to any anonymous creation that Admin-001/002 currently permit.
|
||||||
|
|
||||||
**Recommendation:** Pass the authenticated user identity (`ClaimTypes.Name` / `NameIdentifier` from the cascaded `AuthenticationState`) as `createdBy`. Apply the same pattern to every other Admin write path that records a `CreatedBy`/`PublishedBy`/`ReleasedBy` field.
|
**Recommendation:** Pass the authenticated user identity (`ClaimTypes.Name` / `NameIdentifier` from the cascaded `AuthenticationState`) as `createdBy`. Apply the same pattern to every other Admin write path that records a `CreatedBy`/`PublishedBy`/`ReleasedBy` field.
|
||||||
|
|
||||||
**Resolution:** _(open)_
|
**Resolution:** Resolved 2026-05-22 — `NewCluster.razor` and `ClusterDetail.razor` (the two pages that call `ClusterService.CreateAsync` / `GenerationService.CreateDraftAsync` with a hardcoded literal) now resolve `ClaimTypes.Name` / `ClaimTypes.NameIdentifier` from the cascaded `AuthenticationState` and pass the operator principal name as `createdBy`; the fallback is `"unknown"` (defensive, should never occur on an `[Authorize]`-gated page).
|
||||||
|
|
||||||
### Admin-008
|
### Admin-008
|
||||||
|
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
@page "/clusters/{ClusterId}"
|
@page "/clusters/{ClusterId}"
|
||||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
@attribute [Microsoft.AspNetCore.Authorization.Authorize]
|
||||||
|
@using System.Security.Claims
|
||||||
@using Microsoft.AspNetCore.Components.Web
|
@using Microsoft.AspNetCore.Components.Web
|
||||||
@using Microsoft.AspNetCore.SignalR.Client
|
@using Microsoft.AspNetCore.SignalR.Client
|
||||||
@using ZB.MOM.WW.OtOpcUa.Admin.Hubs
|
@using ZB.MOM.WW.OtOpcUa.Admin.Hubs
|
||||||
@@ -200,7 +201,12 @@ else
|
|||||||
_busy = true;
|
_busy = true;
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
var draft = await GenerationSvc.CreateDraftAsync(ClusterId, createdBy: "admin-ui", CancellationToken.None);
|
// Admin-007: record the authenticated operator's name, not a static literal.
|
||||||
|
var user = AuthState is not null ? (await AuthState).User : null;
|
||||||
|
var operatorName = user?.FindFirstValue(ClaimTypes.Name)
|
||||||
|
?? user?.FindFirstValue(ClaimTypes.NameIdentifier)
|
||||||
|
?? "unknown";
|
||||||
|
var draft = await GenerationSvc.CreateDraftAsync(ClusterId, createdBy: operatorName, CancellationToken.None);
|
||||||
Nav.NavigateTo($"/clusters/{ClusterId}/draft/{draft.GenerationId}");
|
Nav.NavigateTo($"/clusters/{ClusterId}/draft/{draft.GenerationId}");
|
||||||
}
|
}
|
||||||
finally { _busy = false; }
|
finally { _busy = false; }
|
||||||
|
|||||||
@@ -4,6 +4,8 @@
|
|||||||
and its CreateAsync write path exploitable by any caller. *@
|
and its CreateAsync write path exploitable by any caller. *@
|
||||||
@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "CanPublish")]
|
@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "CanPublish")]
|
||||||
@using System.ComponentModel.DataAnnotations
|
@using System.ComponentModel.DataAnnotations
|
||||||
|
@using System.Security.Claims
|
||||||
|
@using Microsoft.AspNetCore.Components.Authorization
|
||||||
@using Microsoft.AspNetCore.Components.Web
|
@using Microsoft.AspNetCore.Components.Web
|
||||||
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
@using ZB.MOM.WW.OtOpcUa.Admin.Services
|
||||||
@using ZB.MOM.WW.OtOpcUa.Configuration.Entities
|
@using ZB.MOM.WW.OtOpcUa.Configuration.Entities
|
||||||
@@ -73,6 +75,10 @@
|
|||||||
public RedundancyMode RedundancyMode { get; set; } = RedundancyMode.None;
|
public RedundancyMode RedundancyMode { get; set; } = RedundancyMode.None;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Admin-007: record the authenticated operator's identity on every write path, not
|
||||||
|
// the static literal "admin-ui" which produced an unattributable audit trail.
|
||||||
|
[CascadingParameter] private Task<AuthenticationState>? AuthState { get; set; }
|
||||||
|
|
||||||
private Input _input = new();
|
private Input _input = new();
|
||||||
private bool _submitting;
|
private bool _submitting;
|
||||||
private string? _error;
|
private string? _error;
|
||||||
@@ -84,6 +90,14 @@
|
|||||||
|
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
// Resolve the signed-in principal name. The page is [Authorize(Policy="CanPublish")]
|
||||||
|
// so AuthState will always be available with an authenticated user here; 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";
|
||||||
|
|
||||||
var cluster = new ServerCluster
|
var cluster = new ServerCluster
|
||||||
{
|
{
|
||||||
ClusterId = _input.ClusterId,
|
ClusterId = _input.ClusterId,
|
||||||
@@ -93,11 +107,11 @@
|
|||||||
RedundancyMode = _input.RedundancyMode,
|
RedundancyMode = _input.RedundancyMode,
|
||||||
NodeCount = _input.RedundancyMode == RedundancyMode.None ? (byte)1 : (byte)2,
|
NodeCount = _input.RedundancyMode == RedundancyMode.None ? (byte)1 : (byte)2,
|
||||||
Enabled = true,
|
Enabled = true,
|
||||||
CreatedBy = "admin-ui",
|
CreatedBy = operatorName,
|
||||||
};
|
};
|
||||||
|
|
||||||
await ClusterSvc.CreateAsync(cluster, createdBy: "admin-ui", CancellationToken.None);
|
await ClusterSvc.CreateAsync(cluster, createdBy: operatorName, CancellationToken.None);
|
||||||
await GenerationSvc.CreateDraftAsync(cluster.ClusterId, createdBy: "admin-ui", CancellationToken.None);
|
await GenerationSvc.CreateDraftAsync(cluster.ClusterId, createdBy: operatorName, CancellationToken.None);
|
||||||
|
|
||||||
Nav.NavigateTo($"/clusters/{cluster.ClusterId}");
|
Nav.NavigateTo($"/clusters/{cluster.ClusterId}");
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user