fix(secrets-ui): preserve metadata on rotate + audit unexpected mutation/reveal failures

2026-07-15 17:31:34 -04:00
parent 0ab276dac0
commit 294f53b49a
8 changed files with 306 additions and 37 deletions
@@ -3,15 +3,17 @@ namespace ZB.MOM.WW.Secrets.Abstractions;
 /// <summary>
 /// Resolves the current principal (the ZB.MOM.WW.Auth identity) for audit attribution when a
 /// secret is resolved. A consumer wires this to whatever surfaces the ambient caller — an HTTP
-/// context accessor, an actor context, a CLI identity, etc. A <c>null</c> <see cref="CurrentActor"/>
-/// (or no accessor at all) is treated as the <c>"system"</c> actor by the resolver, so keyless /
-/// background resolutions are still attributed rather than left blank.
+/// context accessor, an actor context, a CLI identity, etc. When <see cref="CurrentActor"/> is
+/// <c>null</c> (or no accessor is registered), the fallback actor string is consumer-defined so an
+/// action is still attributed rather than left blank — e.g. <c>"system"</c> for background/keyless
+/// resolution, or <c>"unknown"</c> for an unauthenticated UI action (the value the UI's
+/// <c>SecretActorResolver</c> uses).
 /// </summary>
 public interface ISecretActorAccessor
 {
    /// <summary>
    /// The current principal to record as the audit <c>Actor</c>, or <c>null</c> when no
-    /// principal is available (treated as <c>"system"</c>).
+    /// principal is available (the resolver then substitutes its consumer-defined fallback actor).
    /// </summary>
    string? CurrentActor { get; }
 }
@@ -57,12 +57,53 @@
    {
        _error = null;
        _busy = true;
+        bool deleted = false;
        try
        {
            string actor = await SecretActorResolver.ResolveAsync(Services, AuthState);

-            await Store.DeleteAsync(Name, actor, _cts.Token);
+            try
+            {
+                await Store.DeleteAsync(Name, actor, _cts.Token);

+                await Audit.WriteAsync(
+                    new AuditEvent
+                    {
+                        EventId = Guid.NewGuid(),
+                        OccurredAtUtc = DateTimeOffset.UtcNow,
+                        Actor = actor,
+                        Action = "secret.delete",
+                        Outcome = AuditOutcome.Success,
+                        Category = "Secrets",
+                        Target = Name.Value,
+                    },
+                    _cts.Token);
+
+                deleted = true;
+            }
+            catch (Exception)
+            {
+                // An unexpected store fault must still be audited as a Failure and surfaced to the
+                // operator instead of tearing down the circuit.
+                _error = "Unable to delete the secret.";
+                await WriteFailureAuditAsync(actor);
+            }
+        }
+        finally
+        {
+            _busy = false;
+        }
+
+        if (deleted)
+        {
+            await OnDeleted.InvokeAsync();
+        }
+    }
+
+    private async Task WriteFailureAuditAsync(string actor)
+    {
+        try
+        {
            await Audit.WriteAsync(
                new AuditEvent
                {
@@ -70,17 +111,16 @@
                    OccurredAtUtc = DateTimeOffset.UtcNow,
                    Actor = actor,
                    Action = "secret.delete",
-                    Outcome = AuditOutcome.Success,
+                    Outcome = AuditOutcome.Failure,
                    Category = "Secrets",
                    Target = Name.Value,
+                    DetailsJson = "{\"reason\":\"error\"}",
                },
                _cts.Token);
-
-            await OnDeleted.InvokeAsync();
        }
-        finally
+        catch
        {
-            _busy = false;
+            // Best-effort audit of a failure; never let audit itself surface into the event pipeline.
        }
    }

@@ -73,6 +73,15 @@ else
            _error = "Unable to decrypt secret.";
            outcome = AuditOutcome.Failure;
        }
+        catch (Exception)
+        {
+            // Any other unexpected store/cipher fault (e.g. master key unavailable, timeout) must
+            // still be audited as a Failure and surfaced to the user, not left to tear down the
+            // circuit. The value is never shown.
+            _value = null;
+            _error = "Unable to reveal secret.";
+            outcome = AuditOutcome.Failure;
+        }
        finally
        {
            _busy = false;
@@ -80,18 +89,26 @@ else
        }

        string actor = await SecretActorResolver.ResolveAsync(Services, AuthState);
-        await Audit.WriteAsync(
-            new AuditEvent
-            {
-                EventId = Guid.NewGuid(),
-                OccurredAtUtc = DateTimeOffset.UtcNow,
-                Actor = actor,
-                Action = "secret.reveal",
-                Outcome = outcome,
-                Category = "Secrets",
-                Target = Name.Value,
-            },
-            _cts.Token);
+        try
+        {
+            await Audit.WriteAsync(
+                new AuditEvent
+                {
+                    EventId = Guid.NewGuid(),
+                    OccurredAtUtc = DateTimeOffset.UtcNow,
+                    Actor = actor,
+                    Action = "secret.reveal",
+                    Outcome = outcome,
+                    Category = "Secrets",
+                    Target = Name.Value,
+                    DetailsJson = outcome == AuditOutcome.Failure ? "{\"reason\":\"error\"}" : null,
+                },
+                _cts.Token);
+        }
+        catch
+        {
+            // Best-effort audit; never let an audit-write fault surface into the event pipeline.
+        }
    }

    private void Hide()
@@ -82,13 +82,26 @@
    [CascadingParameter] private Task<AuthenticationState>? AuthState { get; set; }

    private bool IsRotate => ExistingName is not null;
+    private bool _metadataLoaded;

    /// <inheritdoc />
-    protected override void OnParametersSet()
+    protected override async Task OnParametersSetAsync()
    {
        if (IsRotate)
        {
            _name = ExistingName!.Value.Value;
+
+            // Rotate must preserve the existing row's metadata: the store's upsert overwrites the
+            // description and content type from the incoming row, so pre-fill both from the current
+            // row (metadata only — no plaintext is read or decrypted). Load once so a re-render does
+            // not clobber operator edits. The operator still supplies the NEW value.
+            if (!_metadataLoaded)
+            {
+                _metadataLoaded = true;
+                StoredSecret? row = await Store.GetAsync(ExistingName.Value, _cts.Token);
+                _description = row?.Description ?? string.Empty;
+                _contentType = row?.ContentType ?? SecretContentType.Text;
+            }
        }
    }

@@ -108,37 +121,79 @@
        }

        _busy = true;
+        bool saved = false;
        try
        {
            string actor = await SecretActorResolver.ResolveAsync(Services, AuthState);

-            StoredSecret row = Cipher.Encrypt(name, _value, _contentType);
-            row = row with
+            try
            {
-                Description = string.IsNullOrWhiteSpace(_description) ? null : _description,
-                CreatedBy = actor,
-                UpdatedBy = actor,
-            };
-            await Store.UpsertAsync(row, _cts.Token);
+                StoredSecret row = Cipher.Encrypt(name, _value, _contentType);
+                row = row with
+                {
+                    Description = string.IsNullOrWhiteSpace(_description) ? null : _description,
+                    CreatedBy = actor,
+                    UpdatedBy = actor,
+                };
+                await Store.UpsertAsync(row, _cts.Token);

+                await Audit.WriteAsync(
+                    new AuditEvent
+                    {
+                        EventId = Guid.NewGuid(),
+                        OccurredAtUtc = DateTimeOffset.UtcNow,
+                        Actor = actor,
+                        Action = IsRotate ? "secret.rotate" : "secret.add",
+                        Outcome = AuditOutcome.Success,
+                        Category = "Secrets",
+                        Target = name.Value,
+                    },
+                    _cts.Token);
+
+                saved = true;
+            }
+            catch (Exception)
+            {
+                // An unexpected store/cipher fault (e.g. master key unavailable, timeout) must still
+                // be audited as a Failure and surfaced to the operator — never the value — instead of
+                // tearing down the circuit. The detail carries no plaintext.
+                _error = IsRotate ? "Unable to rotate the secret." : "Unable to add the secret.";
+                await WriteFailureAuditAsync(
+                    actor, IsRotate ? "secret.rotate" : "secret.add", name.Value);
+            }
+        }
+        finally
+        {
+            _busy = false;
+        }
+
+        if (saved)
+        {
+            await OnSaved.InvokeAsync();
+        }
+    }
+
+    private async Task WriteFailureAuditAsync(string actor, string action, string target)
+    {
+        try
+        {
            await Audit.WriteAsync(
                new AuditEvent
                {
                    EventId = Guid.NewGuid(),
                    OccurredAtUtc = DateTimeOffset.UtcNow,
                    Actor = actor,
-                    Action = IsRotate ? "secret.rotate" : "secret.add",
-                    Outcome = AuditOutcome.Success,
+                    Action = action,
+                    Outcome = AuditOutcome.Failure,
                    Category = "Secrets",
-                    Target = name.Value,
+                    Target = target,
+                    DetailsJson = "{\"reason\":\"error\"}",
                },
                _cts.Token);
-
-            await OnSaved.InvokeAsync();
        }
-        finally
+        catch
        {
-            _busy = false;
+            // Best-effort audit of a failure; never let audit itself surface into the event pipeline.
        }
    }

@@ -80,6 +80,44 @@ public class ConfirmDeleteModalTests : TestContext
        Assert.Empty(audit.Events);
    }

+    [Fact]
+    public void ConfirmDeleteModal_StoreFault_AuditsFailure_ShowsError_DoesNotThrow()
+    {
+        var store = new RecordingSecretStore
+        {
+            DeleteFault = new MasterKeyUnavailableException("KEK unavailable."),
+        };
+        var audit = new CapturingAuditWriter();
+        Services.AddSingleton<ISecretStore>(store);
+        Services.AddSingleton<IAuditWriter>(audit);
+
+        var name = new SecretName("db/primary");
+        var auth = this.AddTestAuthorization();
+        auth.SetAuthorized("carol");
+        auth.SetPolicies(SecretsAuthorization.ManagePolicy);
+
+        bool deleted = false;
+        var cut = RenderComponent<ConfirmDeleteModal>(p => p
+            .Add(c => c.Visible, true)
+            .Add(c => c.Name, name)
+            .Add(c => c.OnDeleted, () => deleted = true));
+
+        // The click must not throw into the Blazor event pipeline.
+        cut.Find("[data-testid=confirm-delete]").Click();
+
+        // OnDeleted not raised; a Failure audit recorded; an error surfaced.
+        Assert.False(deleted);
+        Assert.Single(audit.Events);
+        AuditEvent evt = audit.Events[0];
+        Assert.Equal("secret.delete", evt.Action);
+        Assert.Equal(AuditOutcome.Failure, evt.Outcome);
+        Assert.Equal("Secrets", evt.Category);
+        Assert.Equal("db/primary", evt.Target);
+        Assert.Equal("carol", evt.Actor);
+
+        Assert.NotEmpty(cut.FindAll("[data-testid=delete-error]"));
+    }
+
    [Fact]
    public void ConfirmDeleteModal_NotVisible_RendersNothing()
    {
@@ -20,6 +20,12 @@ public sealed class RecordingSecretStore : ISecretStore
    /// <summary>Delete calls (name + recorded actor), in call order.</summary>
    public IReadOnlyList<(SecretName Name, string? Actor)> Deletes => _deletes.ToArray();

+    /// <summary>When set, <see cref="UpsertAsync"/> throws this to drive the mutation-failure path.</summary>
+    public Exception? UpsertFault { get; set; }
+
+    /// <summary>When set, <see cref="DeleteAsync"/> throws this to drive the mutation-failure path.</summary>
+    public Exception? DeleteFault { get; set; }
+
    /// <summary>Seeds a row so <see cref="GetAsync"/> and <see cref="ListAsync"/> can return it.</summary>
    /// <param name="row">The row to seed.</param>
    public void Seed(StoredSecret row) => _rows[row.Name.Value] = row;
@@ -31,6 +37,11 @@ public sealed class RecordingSecretStore : ISecretStore
    /// <inheritdoc />
    public Task UpsertAsync(StoredSecret row, CancellationToken ct)
    {
+        if (UpsertFault is not null)
+        {
+            throw UpsertFault;
+        }
+
        _rows[row.Name.Value] = row;
        _upserts.Enqueue(row);
        return Task.CompletedTask;
@@ -39,6 +50,11 @@ public sealed class RecordingSecretStore : ISecretStore
    /// <inheritdoc />
    public Task<bool> DeleteAsync(SecretName name, string? actor, CancellationToken ct)
    {
+        if (DeleteFault is not null)
+        {
+            throw DeleteFault;
+        }
+
        _deletes.Enqueue((name, actor));
        return Task.FromResult(_rows.TryRemove(name.Value, out _));
    }
@@ -85,6 +85,35 @@ public class RevealButtonTests : TestContext
        Assert.Equal("api/missing", evt.Target);
    }

+    [Fact]
+    public void RevealButton_DecryptFault_AuditsFailure_ShowsError_NoValue()
+    {
+        var (store, audit, cipher) = RegisterServices();
+        var name = new SecretName("api/token");
+        store.Seed(Row(name, PlainValue));
+        cipher.FailDecrypt = true;
+
+        var auth = this.AddTestAuthorization();
+        auth.SetAuthorized("carol");
+        auth.SetPolicies(SecretsAuthorization.RevealPolicy);
+
+        var cut = RenderComponent<RevealButton>(p => p.Add(c => c.Name, name));
+
+        // The click must not throw into the Blazor event pipeline.
+        cut.Find("button.reveal-secret").Click();
+
+        // No value shown, an error surfaced, and a Failure reveal audit recorded (no value leak).
+        Assert.DoesNotContain(PlainValue, cut.Markup);
+        Assert.NotEmpty(cut.FindAll("[data-testid=reveal-error]"));
+        Assert.Single(audit.Events);
+        AuditEvent evt = audit.Events[0];
+        Assert.Equal("secret.reveal", evt.Action);
+        Assert.Equal(AuditOutcome.Failure, evt.Outcome);
+        Assert.Equal("api/token", evt.Target);
+        Assert.Equal("carol", evt.Actor);
+        AssertNoValueLeak(evt);
+    }
+
    private static void AssertNoValueLeak(AuditEvent evt)
    {
        foreach (string? field in new[] { evt.Action, evt.Actor, evt.Category, evt.Target, evt.DetailsJson, evt.ToString() })
@@ -99,6 +99,78 @@ public class SecretEditorTests : TestContext
        AssertNoValueLeak(audit.Events[0], "rotated-value");
    }

+    [Fact]
+    public void SecretEditor_Rotate_PreservesDescriptionAndContentType()
+    {
+        var (store, _, cipher) = RegisterServices();
+        var name = new SecretName("db/primary");
+
+        // Seed an existing Json secret with a description; rotating must NOT blank either.
+        StoredSecret existing = cipher.Encrypt(name, "old-value", SecretContentType.Json) with
+        {
+            Description = "db creds",
+            Revision = 1,
+            CreatedUtc = DateTimeOffset.UnixEpoch,
+            UpdatedUtc = DateTimeOffset.UnixEpoch,
+        };
+        store.Seed(existing);
+
+        var auth = this.AddTestAuthorization();
+        auth.SetAuthorized("dave");
+        auth.SetPolicies(SecretsAuthorization.ManagePolicy);
+
+        var cut = RenderComponent<SecretEditor>(p => p
+            .Add(c => c.ExistingName, name)
+            .Add(c => c.OnSaved, () => { }));
+
+        // The editor pre-filled the existing metadata (no plaintext read).
+        Assert.Equal("db creds", cut.Find("[data-testid=secret-description]").GetAttribute("value"));
+
+        // The operator supplies ONLY a new value.
+        cut.Find("[data-testid=secret-value]").Change("rotated-value");
+        cut.Find("[data-testid=editor-submit]").Click();
+
+        Assert.Single(store.Upserts);
+        StoredSecret row = store.Upserts[0];
+        Assert.Equal("db/primary", row.Name.Value);
+        Assert.Equal("rotated-value", cipher.Decrypt(row));
+        // Metadata preserved — content type not reset to Text, description not blanked.
+        Assert.Equal(SecretContentType.Json, row.ContentType);
+        Assert.Equal("db creds", row.Description);
+    }
+
+    [Fact]
+    public void SecretEditor_Add_StoreFault_AuditsFailure_ShowsError_DoesNotThrow()
+    {
+        var (store, audit, _) = RegisterServices();
+        store.UpsertFault = new MasterKeyUnavailableException("KEK unavailable.");
+
+        var auth = this.AddTestAuthorization();
+        auth.SetAuthorized("carol");
+        auth.SetPolicies(SecretsAuthorization.ManagePolicy);
+
+        var cut = RenderComponent<SecretEditor>(p => p
+            .Add(c => c.OnSaved, () => { }));
+
+        cut.Find("[data-testid=secret-name]").Change("app/thing");
+        cut.Find("[data-testid=secret-value]").Change(EnteredValue);
+
+        // The click must not throw into the Blazor event pipeline.
+        cut.Find("[data-testid=editor-submit]").Click();
+
+        // Nothing persisted, a Failure audit recorded, an error surfaced, and no value leak.
+        Assert.Empty(store.Upserts);
+        Assert.Single(audit.Events);
+        AuditEvent evt = audit.Events[0];
+        Assert.Equal("secret.add", evt.Action);
+        Assert.Equal(AuditOutcome.Failure, evt.Outcome);
+        Assert.Equal("app/thing", evt.Target);
+        Assert.Equal("carol", evt.Actor);
+        AssertNoValueLeak(evt);
+
+        Assert.NotEmpty(cut.FindAll("[data-testid=editor-error]"));
+    }
+
    private static void AssertNoValueLeak(AuditEvent evt, string value = EnteredValue)
    {
        foreach (string? field in new[] { evt.Action, evt.Actor, evt.Category, evt.Target, evt.DetailsJson, evt.ToString() })