diff --git a/src/__tests__/sharing-surface-guard.test.ts b/src/__tests__/sharing-surface-guard.test.ts index 6d627ac32..e75df50bc 100644 --- a/src/__tests__/sharing-surface-guard.test.ts +++ b/src/__tests__/sharing-surface-guard.test.ts @@ -853,7 +853,7 @@ const DELEGABLE_ROUTES: Record = { }, "app/api/biomarkers/route.ts": { domain: "labs", - why: "The record's biomarker catalogue: the analytes this account tracks and the ranges it tracks them against. Adding a marker is a delegable write; the PUT and DELETE on the sibling route are refused.", + why: "The record's biomarker catalogue: the analytes this account tracks and the ranges it tracks them against. Adding a marker is a delegable write; the PUT and DELETE on the sibling route are manage-level.", }, "app/api/biomarkers/[id]/route.ts": { domain: "labs", @@ -1601,8 +1601,8 @@ const DELEGABLE_MANAGE_ROUTES: Record = { }, "app/api/biomarkers/[id]/route.ts": { domain: "labs", - conditions: ["C4"], - why: "Editing an analyte's reference range, the one admitted edit whose error is silent, which is why C4 puts it in the feed. The DELETE beside it stays refused.", + conditions: ["C3", "C4"], + why: "Editing an analyte's reference range, the one admitted edit whose error is silent, which is why C4 puts it in the feed. The DELETE beside it resolves through the same labs authority since v1.43 instead of a bare requireAuth; it hard-deletes the marker with its readings, so C3 files the name, unit and reading count it took.", }, "app/api/allergies/route.ts": { domain: "profile", @@ -2866,7 +2866,7 @@ describe("(g) the MANAGE route set is frozen", () => { it("keeps the admitted mutation inventory complete and discoverable", () => { expect(ADMITTED_MUTATING_HANDLERS.length).toBeGreaterThan(0); - expect(ADMITTED_MUTATING_HANDLERS.length).toBe(87); + expect(ADMITTED_MUTATING_HANDLERS.length).toBe(88); const expected = ADMITTED_MUTATING_HANDLERS.map( ({ handlerModule, action, level }) => diff --git a/src/app/api/biomarkers/[id]/__tests__/delete-record-auth.test.ts b/src/app/api/biomarkers/[id]/__tests__/delete-record-auth.test.ts new file mode 100644 index 000000000..3b9cb8f7a --- /dev/null +++ b/src/app/api/biomarkers/[id]/__tests__/delete-record-auth.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * `DELETE /api/biomarkers/{id}` takes a marker together with every reading + * filed under it. It must resolve its record through the same record-scoped + * labs authority as the PUT beside it — `requireRecordAuth("manage", "labs")` + * — rather than the bare `requireAuth()` it carried before, so the delete is + * held to the labs domain, the session fence, and the grant level like every + * other labs write. + */ + +const requireAuth = vi.fn(); +const requireRecordAuth = vi.fn(); + +vi.mock("@/lib/api-handler", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + requireAuth: (...args: unknown[]) => requireAuth(...args), + requireRecordAuth: (...args: unknown[]) => requireRecordAuth(...args), + }; +}); + +vi.mock("@/lib/db", () => ({ + prisma: { + biomarker: { findFirst: vi.fn(), delete: vi.fn() }, + labResult: { deleteMany: vi.fn() }, + $transaction: vi.fn(async (ops: Promise[]) => Promise.all(ops)), + }, +})); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserHealthScore: vi.fn(), +})); + +import { DELETE } from "../route"; +import { HttpError } from "@/lib/api-handler"; +import { prisma } from "@/lib/db"; + +const RECORD_USER = { id: "record-1", username: "r", role: "USER" }; + +function delReq(): NextRequest { + return new NextRequest("http://localhost/api/biomarkers/bm-1", { + method: "DELETE", + }); +} +const params = { params: Promise.resolve({ id: "bm-1" }) }; + +beforeEach(() => { + vi.clearAllMocks(); + requireAuth.mockRejectedValue( + new HttpError(403, "Not available for this record"), + ); + requireRecordAuth.mockResolvedValue({ + user: RECORD_USER, + actor: { id: "actor-1" }, + grantId: "grant-1", + authMethod: "cookie", + }); + vi.mocked(prisma.biomarker.findFirst).mockResolvedValue({ + id: "bm-1", + userId: "record-1", + } as never); + vi.mocked(prisma.labResult.deleteMany).mockResolvedValue({ + count: 2, + } as never); + vi.mocked(prisma.biomarker.delete).mockResolvedValue({} as never); +}); + +describe("DELETE /api/biomarkers/[id] record authority", () => { + it("resolves the record through requireRecordAuth('manage', 'labs')", async () => { + const res = await DELETE(delReq(), params); + expect(res.status).toBe(200); + expect(requireRecordAuth).toHaveBeenCalledWith("manage", "labs"); + expect(requireAuth).not.toHaveBeenCalled(); + }); + + it("narrows both delete legs to the resolved record", async () => { + await DELETE(delReq(), params); + expect(prisma.labResult.deleteMany).toHaveBeenCalledWith({ + where: { userId: "record-1", biomarkerId: "bm-1" }, + }); + }); + + it("refuses when the record authority refuses, deleting nothing", async () => { + requireRecordAuth.mockRejectedValue( + new HttpError(403, "Not available for this record"), + ); + const res = await DELETE(delReq(), params); + expect(res.status).toBe(403); + expect(prisma.labResult.deleteMany).not.toHaveBeenCalled(); + expect(prisma.biomarker.delete).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/biomarkers/[id]/route.ts b/src/app/api/biomarkers/[id]/route.ts index 506333ad3..bdfdbae1b 100644 --- a/src/app/api/biomarkers/[id]/route.ts +++ b/src/app/api/biomarkers/[id]/route.ts @@ -1,6 +1,6 @@ import { NextRequest } from "next/server"; -import { apiHandler, requireAuth, requireRecordAuth } from "@/lib/api-handler"; +import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; import { apiError, apiSuccess, @@ -10,7 +10,10 @@ import { sanitiseZodIssues, } from "@/lib/api-response"; import { auditLog } from "@/lib/auth/audit"; -import { overwriteDetails } from "@/lib/sharing/audit-details"; +import { + destroyedDetails, + overwriteDetails, +} from "@/lib/sharing/audit-details"; import { prisma } from "@/lib/db"; import { invalidateUserHealthScore } from "@/lib/cache/invalidate"; import { @@ -90,8 +93,8 @@ export const PUT = apiHandler( // v1.37.0 — MANAGE, and the closest call in this domain: a reference range // recolours every reading under it, so this is the one admitted edit whose // error is silent rather than visible. C4 on the audit row is what puts it - // in the owner's feed. The DELETE below stays refused — it takes the - // readings with it and files an id. + // in the owner's feed. The DELETE below resolves through the same labs + // authority; it takes the readings with it and files the id. const { user } = await requireRecordAuth("manage", "labs"); const { id } = await params; @@ -254,7 +257,10 @@ export const PUT = apiHandler( export const DELETE = apiHandler( async (request: NextRequest, { params }: RouteParams) => { - const { user } = await requireAuth(); + // The same record-scoped labs authority as the PUT above. A bare + // `requireAuth()` here held the one destructive biomarker write to a + // weaker gate than the edit beside it: no labs domain, no grant level. + const { user } = await requireRecordAuth("manage", "labs"); const { id } = await params; const existing = await prisma.biomarker.findFirst({ where: { id } }); @@ -267,7 +273,7 @@ export const DELETE = apiHandler( // readings go with it rather than lingering as orphaned, unlinked rows. // Both legs are userId-narrowed so a cross-user id can never reach another // account's data. - await prisma.$transaction([ + const [removedReadings] = await prisma.$transaction([ prisma.labResult.deleteMany({ where: { userId: user.id, biomarkerId: id }, }), @@ -278,7 +284,22 @@ export const DELETE = apiHandler( await auditLog("biomarker.delete", { userId: user.id, ipAddress: getClientIp(request), - details: { biomarkerId: id }, + // C3: a hard delete files what it destroyed — the marker's name, unit, + // and how many readings went with it — so the owner's feed can say what + // a manager removed, not just an id that now points at nothing. + details: { + biomarkerId: id, + ...destroyedDetails({ + model: "Biomarker", + id, + label: existing.name, + effectiveAt: existing.createdAt, + extra: { + unit: existing.unit, + readingCount: removedReadings?.count ?? 0, + }, + }), + }, }); annotate({ diff --git a/tests/fixtures/v137/sharing-matrix.ts b/tests/fixtures/v137/sharing-matrix.ts index 5d70af0c0..a204608b9 100644 --- a/tests/fixtures/v137/sharing-matrix.ts +++ b/tests/fixtures/v137/sharing-matrix.ts @@ -288,6 +288,14 @@ export const ADMITTED_MUTATING_HANDLERS = [ "manage", "update", ), + handler( + "app/api/biomarkers/[id]/route.ts", + "/api/biomarkers/[id]", + "DELETE", + "labs", + "manage", + "delete", + ), handler( "app/api/allergies/route.ts", "/api/allergies", diff --git a/tests/integration/sharing-manage-handler-matrix.test.ts b/tests/integration/sharing-manage-handler-matrix.test.ts index 6b7d83fce..e373e8131 100644 --- a/tests/integration/sharing-manage-handler-matrix.test.ts +++ b/tests/integration/sharing-manage-handler-matrix.test.ts @@ -282,7 +282,7 @@ const ROUTE_IMPORTS = import.meta.glob>( describe("the complete MANAGE handler matrix", () => { it("starts with a non-empty, exact handler inventory", () => { - expect(ADMITTED_MUTATING_HANDLERS.length).toBe(87); + expect(ADMITTED_MUTATING_HANDLERS.length).toBe(88); }); }); @@ -1097,6 +1097,33 @@ manageContract("PUT /api/biomarkers/[id]", { ?.unit === "ng/mL", }); +manageContract("DELETE /api/biomarkers/[id]", { + contract: ADMITTED_MUTATING_HANDLERS.find( + (entry) => + entry.route === "/api/biomarkers/[id]" && entry.action === "DELETE", + )!, + prepare: async (userId) => + getPrismaClient().biomarker.create({ + data: { userId, name: "Transferrin", unit: "g/L" }, + }), + act: async (row) => { + const { DELETE } = await import("@/app/api/biomarkers/[id]/route"); + return call( + DELETE as Handler, + "DELETE", + `/api/biomarkers/${row.id}`, + undefined, + { id: row.id }, + ); + }, + ok: 200, + auditAction: "biomarker.delete", + applied: async (row) => + (await getPrismaClient().biomarker.findUnique({ + where: { id: row.id }, + })) === null, +}); + manageContract("POST /api/allergies", { contract: ADMITTED_MUTATING_HANDLERS.find( (entry) => entry.route === "/api/allergies" && entry.action === "POST", @@ -2851,13 +2878,13 @@ describe("the conditions the admissions were granted on", () => { describe("the complete MANAGE handler matrix", () => { it("registers one strict actor-and-effect driver for every admission", () => { const expected = ADMITTED_MUTATING_HANDLERS.map(matrixKey).sort(); - expect(STRICT_DRIVER_KEYS.size).toBe(87); + expect(STRICT_DRIVER_KEYS.size).toBe(88); expect([...STRICT_DRIVER_KEYS].sort()).toEqual(expected); }); it("executes every registered driver through its owned effect and actor audit", () => { const expected = ADMITTED_MUTATING_HANDLERS.map(matrixKey).sort(); - expect(REAL_EFFECT_KEYS.size).toBe(87); + expect(REAL_EFFECT_KEYS.size).toBe(88); expect([...REAL_EFFECT_KEYS].sort()).toEqual(expected); }); });