fix(housekeeping): require reasons for destructive changes

This commit is contained in:
Simo committed 2026-09-02 22:12:58 +02:00
1 parent 50598472ad
commit 93367a239f
16 files changed
+323 -15

No files matched your search

@@ -174,6 +174,7 @@ export function createContentCommands(
{ {
correlationId: context.correlationId, correlationId: context.correlationId,
expectedActorId: context.capability.actor.id, expectedActorId: context.capability.actor.id,
...(context.reason === undefined ? {} : { reason: context.reason }),
}, },
operation, operation,
input, input,
@@ -1,6 +1,7 @@
"use client"; "use client";
import { useActionState, useId } from "react"; import { useActionState, useId } from "react";
import { requiresHousekeepingReason } from "../../../foundation/commands/reason-policy";
import type { HousekeepingResult } from "../../../foundation/contracts"; import type { HousekeepingResult } from "../../../foundation/contracts";
export interface ContentCommandField { export interface ContentCommandField {
@@ -135,11 +136,11 @@ export async function submitContentCommandForm(
formData: FormData, formData: FormData,
): Promise<ContentCommandFormState> { ): Promise<ContentCommandFormState> {
const fields = configuration.fields ?? []; const fields = configuration.fields ?? [];
const submittedValues = readSubmittedValues( const reasonRequired = requiresHousekeepingReason(
fields,
configuration.requiresReason ?? false, configuration.requiresReason ?? false,
formData, configuration.input,
); );
const submittedValues = readSubmittedValues(fields, reasonRequired, formData);
const submittedFields = fields.flatMap((field) => { const submittedFields = fields.flatMap((field) => {
const value = parseField(field, formData); const value = parseField(field, formData);
return value === OMIT_FIELD ? [] : [[field.name, value] as const]; return value === OMIT_FIELD ? [] : [[field.name, value] as const];
@@ -158,7 +159,7 @@ export async function submitContentCommandForm(
const result = await executeHousekeepingCommand({ const result = await executeHousekeepingCommand({
commandId: configuration.commandId, commandId: configuration.commandId,
input, input,
...(configuration.requiresReason ? { reason } : {}), ...(reasonRequired ? { reason } : {}),
}); });
return { result, values: result.ok ? {} : submittedValues }; return { result, values: result.ok ? {} : submittedValues };
} }
@@ -190,12 +191,13 @@ export function ContentCommandForm({
requiresReason = false, requiresReason = false,
}: ContentCommandFormProps) { }: ContentCommandFormProps) {
const formId = useId(); const formId = useId();
const reasonRequired = requiresHousekeepingReason(requiresReason, input);
const [state, submit, pending] = useActionState( const [state, submit, pending] = useActionState(
submitContentCommandForm.bind(null, { submitContentCommandForm.bind(null, {
commandId, commandId,
input, input,
fields, fields,
requiresReason, requiresReason: reasonRequired,
}), }),
initialState, initialState,
); );
@@ -342,7 +344,7 @@ export function ContentCommandForm({
</label> </label>
); );
})} })}
{requiresReason ? ( {reasonRequired ? (
<label htmlFor={`${formId}-reason`} className="block text-sm"> <label htmlFor={`${formId}-reason`} className="block text-sm">
Reason Reason
<textarea <textarea
@@ -283,6 +283,36 @@ describe("Content actionable form wiring", () => {
}); });
}); });
it("automatically exposes and submits a required reason for bound delete actions", async () => {
const html = renderToStaticMarkup(
<ContentCommandForm
commandId="content.engagement.event-prize.change"
buttonLabel="Remove prize"
input={{ action: "delete", id: "7" }}
/>,
);
expect(html).toMatch(/<textarea[^>]*name="reason"[^>]*required/u);
vi.mocked(executeHousekeepingCommand).mockResolvedValue(
ok({ before: { id: "7" }, after: null }, "delete-prize"),
);
const formData = new FormData();
formData.set("reason", " Duplicate prize ");
await submitContentCommandForm(
{
commandId: "content.engagement.event-prize.change",
input: { action: "delete", id: "7" },
},
null,
formData,
);
expect(executeHousekeepingCommand).toHaveBeenCalledWith({
commandId: "content.engagement.event-prize.change",
input: { action: "delete", id: "7" },
reason: "Duplicate prize",
});
});
it("clears retained values after a successful command", async () => { it("clears retained values after a successful command", async () => {
vi.mocked(executeHousekeepingCommand).mockResolvedValue( vi.mocked(executeHousekeepingCommand).mockResolvedValue(
ok({ before: null, after: { id: "41" } }, "poll-success"), ok({ before: null, after: { id: "41" } }, "poll-success"),
@@ -80,6 +80,54 @@ describe("Content mutation service authority", () => {
expect(execute).toHaveBeenCalledTimes(CONTENT_MUTATION_OPERATIONS.length); expect(execute).toHaveBeenCalledTimes(CONTENT_MUTATION_OPERATIONS.length);
}); });
it("requires a reason at the direct service boundary for every generic runtime delete branch", async () => {
const deleteCapableOperations = [
"article.change",
"ad.change",
"banner.change",
"event-prize.change",
"tag.change",
"prefix.change",
"help-question.change",
"writeable-box.change",
"email-template.change",
] as const;
const execute = vi.fn(async () => ({ before: { id: "7" }, after: null }));
const service = createContentMutationService({ execute }, async () =>
context(Object.values(PERMS)),
);
for (const operation of deleteCapableOperations) {
const denied = await service.execute(
{ correlationId: `missing-${operation}`, expectedActorId: 42 },
operation,
{ action: "delete", id: "7" },
);
expect(denied, operation).toMatchObject({
ok: false,
error: {
code: "VALIDATION",
fieldErrors: { reason: ["errors.validation.required"] },
},
});
}
expect(execute).not.toHaveBeenCalled();
for (const operation of deleteCapableOperations) {
const allowed = await service.execute(
{
correlationId: `allowed-${operation}`,
expectedActorId: 42,
reason: "Remove obsolete resource",
},
operation,
{ action: "delete", id: "7" },
);
expect(allowed.ok, operation).toBe(true);
}
expect(execute).toHaveBeenCalledTimes(deleteCapableOperations.length);
});
it("maps adapter failures without exposing storage or template payloads", async () => { it("maps adapter failures without exposing storage or template payloads", async () => {
const service = createContentMutationService( const service = createContentMutationService(
{ {
@@ -1,5 +1,9 @@
import { PERMS } from "@/lib/permission-slugs"; import { PERMS } from "@/lib/permission-slugs";
import { satisfiesCapability } from "../../../foundation/capability-context"; import { satisfiesCapability } from "../../../foundation/capability-context";
import {
normalizeHousekeepingReason,
requiresHousekeepingReason,
} from "../../../foundation/commands/reason-policy";
import { import {
anyCapability, anyCapability,
fail, fail,
@@ -56,6 +60,7 @@ export interface ContentMutationInvocation {
readonly correlationId: string; readonly correlationId: string;
readonly expectedActorId: number; readonly expectedActorId: number;
readonly legacy?: boolean; readonly legacy?: boolean;
readonly reason?: string;
} }
export interface ContentMutationContext { export interface ContentMutationContext {
@@ -153,6 +158,17 @@ export function createContentMutationService(
invocation.correlationId, invocation.correlationId,
); );
} }
if (
requiresHousekeepingReason(false, input) &&
!normalizeHousekeepingReason(invocation.reason)
) {
return fail(
"VALIDATION",
"errors.housekeeping.validation",
invocation.correlationId,
{ reason: ["errors.validation.required"] },
);
}
try { try {
const snapshot = await adapter.execute(operation, input, { const snapshot = await adapter.execute(operation, input, {
capability, capability,
@@ -134,6 +134,7 @@ export function createEconomyCommands(
{ {
correlationId: context.correlationId, correlationId: context.correlationId,
expectedActorId: context.capability.actor.id, expectedActorId: context.capability.actor.id,
...(context.reason === undefined ? {} : { reason: context.reason }),
}, },
operation, operation,
input, input,
@@ -1,6 +1,7 @@
"use client"; "use client";
import { useActionState } from "react"; import { useActionState } from "react";
import { requiresHousekeepingReason } from "../../../foundation/commands/reason-policy";
import type { HousekeepingResult } from "../../../foundation/contracts"; import type { HousekeepingResult } from "../../../foundation/contracts";
export interface EconomyCommandField { export interface EconomyCommandField {
@@ -69,6 +70,10 @@ export async function submitEconomyCommandForm(
_previous: HousekeepingResult<unknown> | null, _previous: HousekeepingResult<unknown> | null,
formData: FormData, formData: FormData,
): Promise<HousekeepingResult<unknown>> { ): Promise<HousekeepingResult<unknown>> {
const reasonRequired = requiresHousekeepingReason(
configuration.requiresReason ?? false,
configuration.input,
);
const submitted = (configuration.fields ?? []).flatMap((field) => { const submitted = (configuration.fields ?? []).flatMap((field) => {
const value = parseField(field, formData); const value = parseField(field, formData);
return value === OMIT_FIELD ? [] : [[field.name, value] as const]; return value === OMIT_FIELD ? [] : [[field.name, value] as const];
@@ -83,7 +88,7 @@ export async function submitEconomyCommandForm(
return executeHousekeepingCommand({ return executeHousekeepingCommand({
commandId: configuration.commandId, commandId: configuration.commandId,
input: { ...configuration.input, ...Object.fromEntries(submitted) }, input: { ...configuration.input, ...Object.fromEntries(submitted) },
...(configuration.requiresReason ? { reason } : {}), ...(reasonRequired ? { reason } : {}),
}); });
} }
@@ -94,12 +99,13 @@ export function EconomyCommandForm({
fields = [], fields = [],
requiresReason = false, requiresReason = false,
}: EconomyCommandFormProps) { }: EconomyCommandFormProps) {
const reasonRequired = requiresHousekeepingReason(requiresReason, input);
const [result, submit, pending] = useActionState( const [result, submit, pending] = useActionState(
submitEconomyCommandForm.bind(null, { submitEconomyCommandForm.bind(null, {
commandId, commandId,
input, input,
fields, fields,
requiresReason, requiresReason: reasonRequired,
}), }),
initialState, initialState,
); );
@@ -172,7 +178,7 @@ export function EconomyCommandForm({
</label> </label>
); );
})} })}
{requiresReason ? ( {reasonRequired ? (
<label <label
htmlFor={`economy-${commandId}-reason`} htmlFor={`economy-${commandId}-reason`}
className="block text-sm" className="block text-sm"
@@ -1,5 +1,6 @@
import { renderToStaticMarkup } from "react-dom/server"; import { renderToStaticMarkup } from "react-dom/server";
import { describe, expect, it } from "vitest"; import { describe, expect, it, vi } from "vitest";
import { executeHousekeepingCommand } from "@/actions/housekeeping-command";
import { PERMS } from "@/lib/permission-slugs"; import { PERMS } from "@/lib/permission-slugs";
import { import {
fail, fail,
@@ -8,11 +9,19 @@ import {
} from "../../../foundation/contracts"; } from "../../../foundation/contracts";
import { EconomyCatalogPage } from "./catalog"; import { EconomyCatalogPage } from "./catalog";
import { EconomyCommercePage } from "./commerce"; import { EconomyCommercePage } from "./commerce";
import {
EconomyCommandForm,
submitEconomyCommandForm,
} from "./economy-command-form";
import { EconomyHistoryPage } from "./history"; import { EconomyHistoryPage } from "./history";
import { EconomyItemsPage } from "./items"; import { EconomyItemsPage } from "./items";
import { EconomyRewardsPage } from "./rewards"; import { EconomyRewardsPage } from "./rewards";
import { EconomyValuePage } from "./value"; import { EconomyValuePage } from "./value";
vi.mock("@/actions/housekeeping-command", () => ({
executeHousekeepingCommand: vi.fn(),
}));
function context(granted: readonly string[]): HousekeepingCapabilityContext { function context(granted: readonly string[]): HousekeepingCapabilityContext {
const permissions = new Set(granted); const permissions = new Set(granted);
return { return {
@@ -77,6 +86,36 @@ describe.each(cases)("Economy %s page", (kind, Component) => {
}); });
describe("Economy mutation forms", () => { describe("Economy mutation forms", () => {
it("automatically exposes and submits a required reason for bound delete actions", async () => {
const html = renderToStaticMarkup(
<EconomyCommandForm
commandId="economy.commerce.voucher.change"
buttonLabel="Delete voucher"
input={{ action: "delete", id: "7" }}
/>,
);
expect(html).toMatch(/<textarea[^>]*name="reason"[^>]*required/u);
vi.mocked(executeHousekeepingCommand).mockResolvedValue(
ok({ before: { id: "7" }, after: null }, "delete-voucher"),
);
const formData = new FormData();
formData.set("reason", " Expired duplicate ");
await submitEconomyCommandForm(
{
commandId: "economy.commerce.voucher.change",
input: { action: "delete", id: "7" },
},
null,
formData,
);
expect(executeHousekeepingCommand).toHaveBeenCalledWith({
commandId: "economy.commerce.voucher.change",
input: { action: "delete", id: "7" },
reason: "Expired duplicate",
});
});
it("renders edit controls only with the exact mutation capability", () => { it("renders edit controls only with the exact mutation capability", () => {
const result = ok( const result = ok(
{ {
@@ -72,6 +72,54 @@ describe("Economy mutation service", () => {
}); });
}); });
it("requires a reason at the direct service boundary for every generic runtime delete branch", async () => {
const deleteCapableOperations = [
"catalog-page.change",
"bc-page.change",
"bc-item.change",
"catalog-item.change",
"shop-article.change",
"voucher.change",
"rare-category.change",
"rare-value.change",
"soundtrack.change",
] as const;
const execute = vi.fn(async () => ({ before: { id: "7" }, after: null }));
const service = createEconomyMutationService({ execute }, async () =>
capability(),
);
for (const operation of deleteCapableOperations) {
const denied = await service.execute(
{ correlationId: `missing-${operation}`, expectedActorId: 42 },
operation,
{ action: "delete", id: "7" },
);
expect(denied, operation).toMatchObject({
ok: false,
error: {
code: "VALIDATION",
fieldErrors: { reason: ["errors.validation.required"] },
},
});
}
expect(execute).not.toHaveBeenCalled();
for (const operation of deleteCapableOperations) {
const allowed = await service.execute(
{
correlationId: `allowed-${operation}`,
expectedActorId: 42,
reason: "Remove obsolete resource",
},
operation,
{ action: "delete", id: "7" },
);
expect(allowed.ok, operation).toBe(true);
}
expect(execute).toHaveBeenCalledTimes(deleteCapableOperations.length);
});
it("returns snapshots and partial completion from the adapter", async () => { it("returns snapshots and partial completion from the adapter", async () => {
const service = createEconomyMutationService( const service = createEconomyMutationService(
{ {
@@ -1,5 +1,9 @@
import { PERMS } from "@/lib/permission-slugs"; import { PERMS } from "@/lib/permission-slugs";
import { satisfiesCapability } from "../../../foundation/capability-context"; import { satisfiesCapability } from "../../../foundation/capability-context";
import {
normalizeHousekeepingReason,
requiresHousekeepingReason,
} from "../../../foundation/commands/reason-policy";
import { import {
anyCapability, anyCapability,
fail, fail,
@@ -53,6 +57,7 @@ export interface EconomyMutationInvocation {
readonly correlationId: string; readonly correlationId: string;
readonly expectedActorId: number; readonly expectedActorId: number;
readonly legacy?: boolean; readonly legacy?: boolean;
readonly reason?: string;
} }
export interface EconomyMutationContext { export interface EconomyMutationContext {
readonly capability: HousekeepingCapabilityContext; readonly capability: HousekeepingCapabilityContext;
@@ -129,6 +134,17 @@ export function createEconomyMutationService(
invocation.correlationId, invocation.correlationId,
); );
} }
if (
requiresHousekeepingReason(false, input) &&
!normalizeHousekeepingReason(invocation.reason)
) {
return fail(
"VALIDATION",
"errors.housekeeping.validation",
invocation.correlationId,
{ reason: ["errors.validation.required"] },
);
}
try { try {
const snapshot = await adapter.execute(operation, input, { const snapshot = await adapter.execute(operation, input, {
capability, capability,
@@ -7,13 +7,13 @@ import type { HousekeepingCommand } from "./registry";
function command( function command(
requiresReason: boolean, requiresReason: boolean,
risk: "safe" | "sensitive", risk: "safe" | "sensitive",
): HousekeepingCommand<Record<string, never>, null> { ): HousekeepingCommand<Record<string, unknown>, null> {
return { return {
id: "system.confirmation.test", id: "system.confirmation.test",
owner: "system", owner: "system",
risk, risk,
capability: anyCapability("admin.settings.edit"), capability: anyCapability("admin.settings.edit"),
input: z.object({}), input: z.object({}).catchall(z.unknown()),
requiresReason, requiresReason,
rateLimit: { attempts: 2, windowMs: 60_000 }, rateLimit: { attempts: 2, windowMs: 60_000 },
execute: async (context) => ok(null, context.correlationId), execute: async (context) => ok(null, context.correlationId),
@@ -75,4 +75,23 @@ describe("housekeeping command confirmation", () => {
correlationId: "corr-safe", correlationId: "corr-safe",
}); });
}); });
it("requires a reason dynamically when parsed input binds a delete action", () => {
expect(
confirmHousekeepingCommand(
command(false, "sensitive"),
" \t ",
"corr-delete-reason",
{ action: "delete", id: "7" },
),
).toEqual({
ok: false,
error: {
code: "VALIDATION",
messageKey: "errors.housekeeping.validation",
fieldErrors: { reason: ["errors.validation.required"] },
},
correlationId: "corr-delete-reason",
});
});
}); });
@@ -1,4 +1,8 @@
import { fail, type HousekeepingResult, ok } from "../contracts"; import { fail, type HousekeepingResult, ok } from "../contracts";
import {
normalizeHousekeepingReason,
requiresHousekeepingReason,
} from "./reason-policy";
import type { HousekeepingCommand } from "./registry"; import type { HousekeepingCommand } from "./registry";
export interface HousekeepingCommandConfirmation { export interface HousekeepingCommandConfirmation {
@@ -12,9 +16,14 @@ export function confirmHousekeepingCommand<I, O>(
command: HousekeepingCommand<I, O>, command: HousekeepingCommand<I, O>,
reason: string | undefined, reason: string | undefined,
correlationId: string, correlationId: string,
input?: I,
): HousekeepingResult<HousekeepingCommandConfirmation> { ): HousekeepingResult<HousekeepingCommandConfirmation> {
const normalizedReason = reason?.normalize("NFC").trim() || undefined; const normalizedReason = normalizeHousekeepingReason(reason);
if (command.requiresReason && !normalizedReason) { const reasonRequired = requiresHousekeepingReason(
command.requiresReason,
input,
);
if (reasonRequired && !normalizedReason) {
return fail("VALIDATION", "errors.housekeeping.validation", correlationId, { return fail("VALIDATION", "errors.housekeeping.validation", correlationId, {
reason: ["errors.validation.required"], reason: ["errors.validation.required"],
}); });
@@ -24,7 +33,7 @@ export function confirmHousekeepingCommand<I, O>(
{ {
commandId: command.id, commandId: command.id,
risk: command.risk, risk: command.risk,
requiresReason: command.requiresReason, requiresReason: reasonRequired,
...(normalizedReason === undefined ? {} : { reason: normalizedReason }), ...(normalizedReason === undefined ? {} : { reason: normalizedReason }),
}, },
correlationId, correlationId,
@@ -374,6 +374,53 @@ describe("dispatchHousekeepingCommand", () => {
expect(audit.entries.map((entry) => entry.outcome)).toEqual(["denied"]); expect(audit.entries.map((entry) => entry.outcome)).toEqual(["denied"]);
}); });
it("requires and forwards a normalized reason for a dynamically destructive input", async () => {
let executionReason: string | undefined;
let executions = 0;
register(
baseCommand("system.dispatch.dynamic-delete-reason", {
risk: "sensitive",
input: z.object({
action: z.enum(["update", "delete"]),
id: z.string(),
}),
execute: async (context) => {
executions += 1;
executionReason = context.reason;
return ok(null, context.correlationId);
},
}),
);
const denied = await dispatchHousekeepingCommand(
{
commandId: "system.dispatch.dynamic-delete-reason",
input: { action: "delete", id: "7" },
},
dependencies(),
);
expect(denied).toMatchObject({
ok: false,
error: {
code: "VALIDATION",
fieldErrors: { reason: ["errors.validation.required"] },
},
});
expect(executions).toBe(0);
const allowed = await dispatchHousekeepingCommand(
{
commandId: "system.dispatch.dynamic-delete-reason",
input: { action: "delete", id: "7" },
reason: " Remove duplicate ",
},
dependencies(),
);
expect(allowed).toMatchObject({ ok: true });
expect(executions).toBe(1);
expect(executionReason).toBe("Remove duplicate");
});
it("uses actor, IP, command ID, and the approved command limit", async () => { it("uses actor, IP, command ID, and the approved command limit", async () => {
const calls: Array<[string, number, number]> = []; const calls: Array<[string, number, number]> = [];
let executed = false; let executed = false;
@@ -223,6 +223,7 @@ export async function dispatchHousekeepingCommand(
command, command,
commandRequest.reason || undefined, commandRequest.reason || undefined,
correlationId, correlationId,
parsedInput.data,
); );
} catch (error) { } catch (error) {
return persistReturnedOutcome( return persistReturnedOutcome(
@@ -277,6 +278,7 @@ export async function dispatchHousekeepingCommand(
capability: dependencies.context, capability: dependencies.context,
correlationId, correlationId,
ipAddress, ipAddress,
...(commandRequest.reason ? { reason: commandRequest.reason } : {}),
}; };
let correlatedResult: HousekeepingResult<unknown>; let correlatedResult: HousekeepingResult<unknown>;
try { try {
@@ -0,0 +1,23 @@
export function normalizeHousekeepingReason(
reason: string | undefined,
): string | undefined {
return reason?.normalize("NFC").trim() || undefined;
}
export function isHousekeepingDeleteInput(input: unknown): boolean {
if (typeof input !== "object" || input === null || Array.isArray(input)) {
return false;
}
try {
return Reflect.get(input, "action") === "delete";
} catch {
return false;
}
}
export function requiresHousekeepingReason(
commandRequiresReason: boolean,
input: unknown,
): boolean {
return commandRequiresReason || isHousekeepingDeleteInput(input);
}
@@ -14,6 +14,7 @@ export interface HousekeepingCommandContext {
readonly capability: HousekeepingCapabilityContext; readonly capability: HousekeepingCapabilityContext;
readonly correlationId: string; readonly correlationId: string;
readonly ipAddress: string; readonly ipAddress: string;
readonly reason?: string;
} }
export interface HousekeepingCommand<I, O> { export interface HousekeepingCommand<I, O> {