From 8f85cce298a77ae22d4080ffbc3a129265038cec Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 03:28:11 -0400 Subject: [PATCH] fix(ui): schema-library delete-audit name + busy guard + edit-row guard + sanitized create-race test (#260) --- .../Pages/Design/SchemaLibrary.razor | 32 ++++++--- .../Services/SchemaLibraryQueryService.cs | 7 +- .../ManagementActor.cs | 7 +- .../Design/SchemaLibraryPageTests.cs | 20 ++++++ .../SchemaLibraryHandlerTests.cs | 66 +++++++++++++++++++ 5 files changed, 121 insertions(+), 11 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SchemaLibrary.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SchemaLibrary.razor index 0b61535a..aaecdb6e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SchemaLibrary.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SchemaLibrary.razor @@ -93,10 +93,13 @@ @(string.IsNullOrWhiteSpace(s.Scope) ? "—" : s.Scope) lib:@s.Name + @* Row actions are disabled while the editor is open so the + row under edit (and its siblings) can't be deleted out from + under the form, and while a delete is in flight (_busy). *@ + @onclick="() => BeginEdit(s)" disabled="@(_editing || _busy)">Edit + @onclick="() => DeleteSchema(s)" disabled="@(_editing || _busy)">Delete } @@ -208,21 +211,34 @@ private async Task DeleteSchema(SharedSchema schema) { + // In-flight guard: an editor-open row action is already disabled in the markup, + // but the _busy gate is the authoritative guard against a double-invoked delete + // (and mirrors the Save path's guard). + if (_busy) return; + var confirmed = await Dialog.ConfirmAsync( "Delete Schema", $"Delete library schema '{schema.Name}'? References to lib:{schema.Name} will no longer resolve.", danger: true); if (!confirmed) return; - var result = await SchemaLibraryService.DeleteAsync(schema.Id); - if (result.Success) + _busy = true; + try { - _toast.ShowSuccess($"Schema '{schema.Name}' deleted."); - await LoadAsync(); + var result = await SchemaLibraryService.DeleteAsync(schema.Id); + if (result.Success) + { + _toast.ShowSuccess($"Schema '{schema.Name}' deleted."); + await LoadAsync(); + } + else + { + _toast.ShowError(result.Error ?? "Delete failed."); + } } - else + finally { - _toast.ShowError(result.Error ?? "Delete failed."); + _busy = false; } } } diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/SchemaLibraryQueryService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/SchemaLibraryQueryService.cs index 66bb1345..2f3d9677 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/SchemaLibraryQueryService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/SchemaLibraryQueryService.cs @@ -31,8 +31,11 @@ public sealed class SchemaLibraryQueryService : ISchemaLibraryQueryService var repo = scope.ServiceProvider.GetRequiredService(); var all = await repo.ListAsync(cancellationToken); - // Ordinal-keyed to match the lib:Name resolver's exact-name lookup. Last-wins on - // the (DB-unique) name guards against a transient duplicate read. + // Ordinal-keyed to match the lib:Name resolver's exact-name lookup. Name is + // DB-unique, so a list yields at most one row per name and no real collision + // occurs; the indexer assignment is defensive only — should two rows ever share + // a name (e.g. a mid-write transient read), the later one in enumeration order + // overwrites the earlier rather than throwing. var map = new Dictionary(StringComparer.Ordinal); foreach (var schema in all) { diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index afcc58f0..3366f84b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -2323,8 +2323,13 @@ public class ManagementActor : ReceiveActor private static async Task HandleDeleteSharedSchema(IServiceProvider sp, DeleteSharedSchemaCommand cmd, string user) { var repo = sp.GetRequiredService(); + // Pre-fetch the human-readable name before the row is gone so the audit + // EntityName records "Address" rather than the numeric id — mirroring the + // Site delete handler. Falls back to the id when the row is already absent. + var schema = await repo.GetByIdAsync(cmd.SharedSchemaId); await repo.DeleteAsync(cmd.SharedSchemaId); - await AuditAsync(sp, user, "Delete", "SharedSchema", cmd.SharedSchemaId.ToString(), cmd.SharedSchemaId.ToString(), null); + await AuditAsync(sp, user, "Delete", "SharedSchema", cmd.SharedSchemaId.ToString(), + schema?.Name ?? cmd.SharedSchemaId.ToString(), null); return true; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs index 6c44d6f4..48ea76bf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs @@ -187,6 +187,26 @@ public class SchemaLibraryPageTests : BunitContext 9, "Address", Arg.Any(), Arg.Any(), Arg.Any())); } + [Fact] + public void Editing_DisablesRowActions_SoTheRowUnderEditCannotBeDeleted() + { + // #260: while the edit form is open the list still shows the row, but its + // Edit/Delete actions must be disabled so the row under edit (or a sibling) + // can't be deleted out from under the form. + SeedSchemas(new SharedSchema { Id = 9, Name = "Address", Scope = "us", SchemaJson = "{\"type\":\"object\"}" }); + + var cut = Render(); + cut.WaitForState(() => cut.Markup.Contains("Address")); + + cut.FindAll("tbody tr button").First(b => b.TextContent.Contains("Edit")).Click(); + Assert.Contains("Edit Schema: Address", cut.Markup); + + // Every row action button is now disabled while the editor is open. + var rowButtons = cut.FindAll("tbody tr button"); + Assert.NotEmpty(rowButtons); + Assert.All(rowButtons, b => Assert.True(b.HasAttribute("disabled"))); + } + /// A dialog service that auto-confirms, so the delete path runs end-to-end. private sealed class AlwaysConfirmDialogService : IDialogService { diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SchemaLibraryHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SchemaLibraryHandlerTests.cs index aae2152d..e9ec984c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SchemaLibraryHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SchemaLibraryHandlerTests.cs @@ -302,6 +302,28 @@ public class SchemaLibraryHandlerTests : TestKit, IDisposable _schemaRepo.Received(1).DeleteAsync(5, Arg.Any()); } + [Fact] + public void DeleteSharedSchema_RecordsHumanReadableNameInAudit() + { + // #260: the delete handler pre-fetches the row name BEFORE deleting so the + // audit EntityName records "Addr" rather than the numeric id (mirroring the + // Site delete handler). + _schemaRepo.GetByIdAsync(5, Arg.Any()) + .Returns(new SharedSchema { Id = 5, Name = "Addr", SchemaJson = ValidSchema }); + + var actor = CreateActor(); + var envelope = Envelope(new DeleteSharedSchemaCommand(5), "Designer"); + + actor.Tell(envelope); + + ExpectMsg(TimeSpan.FromSeconds(5)); + // LogAsync(user, action, entityType, entityId, entityName, afterState, ct): + // entityId is the id ("5"), entityName is the resolved name ("Addr"). + _auditService.Received(1).LogAsync( + Arg.Any(), "Delete", "SharedSchema", "5", "Addr", + Arg.Any(), Arg.Any()); + } + [Fact] public void ListSharedSchemas_WhenRepoThrows_ReturnsError() { @@ -317,4 +339,48 @@ public class SchemaLibraryHandlerTests : TestKit, IDisposable Assert.Equal("COMMAND_FAILED", response.ErrorCode); Assert.DoesNotContain("db down secret detail", response.Error); } + + // ── Concurrent-create race (#260) ─────────────────────────────────────── + + [Fact] + public void CreateSharedSchema_WhenInsertRacesUniqueViolation_SurfacesSanitizedError() + { + // Race: GetByNameAsync sees no row, but a concurrent create commits the + // same unique Name before our AddAsync, so the INSERT hits the DB unique + // constraint. EF surfaces this as a DbUpdateException whose message carries + // raw DB internals (server/constraint/SQL detail). The handler does NOT + // catch this as a curated ManagementCommandException, so the actor's + // top-level failure path must sanitize it to a generic, correlation-tagged + // message — never leaking the raw SQL/DB text to the caller. + // + // Modeled with a SqlException-shaped fault rather than a real EF + // DbUpdateException so the ManagementService.Tests project takes no new + // EntityFrameworkCore package reference; the actor's sanitization keys on + // the exception NOT being a ManagementCommandException, which is identical + // for both types — so this exercises the exact same code path. + const string rawDbDetail = + "Cannot insert duplicate key row in object 'dbo.SharedSchemas' with unique " + + "index 'IX_SharedSchemas_Name'. The duplicate key value is (Addr). " + + "INSERT INTO [SharedSchemas] ..."; + + _schemaRepo.GetByNameAsync("Addr", Arg.Any()) + .Returns((SharedSchema?)null); + _schemaRepo.AddAsync(Arg.Any(), Arg.Any()) + .ThrowsAsync(new InvalidOperationException(rawDbDetail)); + + var actor = CreateActor(); + var envelope = Envelope( + new CreateSharedSchemaCommand("Addr", null, ValidSchema), "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + // Sanitized: the generic internal-error message, NOT the raw DB detail. + Assert.Contains("internal error", response.Error, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("SharedSchemas", response.Error); + Assert.DoesNotContain("IX_SharedSchemas_Name", response.Error); + Assert.DoesNotContain("INSERT INTO", response.Error); + Assert.DoesNotContain("duplicate key", response.Error); + } }