diff --git a/docs/ops/backup-restore.md b/docs/ops/backup-restore.md index 94bd55a96..8779d947a 100644 --- a/docs/ops/backup-restore.md +++ b/docs/ops/backup-restore.md @@ -103,7 +103,12 @@ exists, and the console then asks where the records should go: password or email, as an ordinary account with the user role, and nobody can sign into it yet. Before handing it over, set its first password with **Reset password** in Admin → Users; the person can change it, add an email - and enrol a passkey from their settings. + and enrol a passkey from their settings. The account is made before the + file is read, with an `admin.user.create` audit row (`outcome: "created"`), + and removed again when the file is refused, with an `admin.user.delete` row + (`reason: "backup_upload_refused"`). When that row says `outcome: "failed"`, + the removal did not happen and the account is left over, holding nothing: + delete it in Admin → Users. - **An existing account**, for someone who has signed up again. Pick the account and type its username to confirm. The account must hold no records yet: a restore replaces an account's records and does not merge them, so an diff --git a/src/__tests__/baselines/source-function-lines.json b/src/__tests__/baselines/source-function-lines.json index 1cf1294f5..a9e9c0346 100644 --- a/src/__tests__/baselines/source-function-lines.json +++ b/src/__tests__/baselines/source-function-lines.json @@ -1,10 +1,10 @@ { "$comment": "Production functions over 200 lines, keyed \"file :: name\", and their length. Maintained by src/__tests__/source-size-ratchet.test.ts.", "count": 345, - "total": 111940, + "total": 111929, "entries": { "app/achievements/page.tsx :: AchievementsPage": 206, - "app/api/admin/backups/upload/route.ts :: apiHandler(arg)": 286, + "app/api/admin/backups/upload/route.ts :: apiHandler(arg)": 275, "app/api/admin/notifications/reminder-check/route.ts :: apiHandler(arg)": 206, "app/api/admin/settings/route.ts :: apiHandler(arg) #2": 211, "app/api/auth/login/route.ts :: apiHandler(arg)": 262, diff --git a/src/app/api/admin/backups/upload/route.ts b/src/app/api/admin/backups/upload/route.ts index 7708fae2c..386062a8d 100644 --- a/src/app/api/admin/backups/upload/route.ts +++ b/src/app/api/admin/backups/upload/route.ts @@ -295,17 +295,42 @@ async function checkTargetBeforeRead( return null; } +/** Who made an account for a copy, for the trail. */ +interface UploadActor { + adminId: string; + ipAddress: string | null; +} + /** * The account made for a copy. Created before the file is read, because the * file is rewritten to its id on the way in, and deleted again when nothing * is stored. No password, no email: nobody can sign into it until an admin - * sets its first password (Admin, Users, Reset password). + * sets its first password (Admin, Users, Reset password). Its audit row + * commits with it, so an account this route made always has a trace, even + * when the upload fails and removing it again fails too. */ -async function createTargetAccount(username: string): Promise { +async function createTargetAccount( + username: string, + actor: UploadActor, +): Promise { try { - const account = await prisma.user.create({ - data: { username, role: "USER" }, - select: { id: true, username: true }, + const account = await prisma.$transaction(async (tx) => { + const row = await tx.user.create({ + data: { username, role: "USER" }, + select: { id: true, username: true }, + }); + await auditLog("admin.user.create", { + userId: actor.adminId, + ipAddress: actor.ipAddress, + client: tx, + details: { + targetUserId: row.id, + username: row.username, + reason: "backup_restore", + outcome: "created", + }, + }); + return row; }); return { ...account, created: true }; } catch (err) { @@ -355,13 +380,20 @@ async function ownerOfRetargetedFile( /** * Delete the account an upload created for its copy, when nothing was stored. * It holds nothing and nobody can have signed into it (no password, no email), - * so the row is all there is. A failure is noted, never thrown over the + * so the row is all there is. Either way the trail says what became of it: a + * removal that failed leaves an account to delete by hand + * (`docs/ops/backup-restore.md`). A failure is noted, never thrown over the * refusal the operator needs to see. */ -async function discardCreatedAccount(userId: string): Promise { +async function discardCreatedAccount( + account: ChosenTarget, + actor: UploadActor, +): Promise { + let outcome: "deleted" | "failed" = "deleted"; try { - await prisma.user.delete({ where: { id: userId } }); + await prisma.user.delete({ where: { id: account.id } }); } catch (err) { + outcome = "failed"; annotate({ meta: { backup_upload_account_discard_failed: true, @@ -370,6 +402,20 @@ async function discardCreatedAccount(userId: string): Promise { }, }); } + try { + await auditLog("admin.user.delete", { + userId: actor.adminId, + ipAddress: actor.ipAddress, + details: { + targetUserId: account.id, + username: account.username, + reason: "backup_upload_refused", + outcome, + }, + }); + } catch { + annotate({ meta: { backup_upload_account_discard_unaudited: true } }); + } } export const POST = apiHandler(async (request: NextRequest) => { @@ -379,6 +425,7 @@ export const POST = apiHandler(async (request: NextRequest) => { await assertRecentCookieProof(admin, session.id); annotate({ action: { name: "admin.backups.upload" } }); const ipAddress = getClientIp(request); + const actor: UploadActor = { adminId: admin.id, ipAddress }; // Rate-limit identical to the manual-run endpoint. Three is plenty for a // deliberate restore-prep workflow. @@ -494,7 +541,7 @@ export const POST = apiHandler(async (request: NextRequest) => { let rewrite: BackupOwnerRewrite | null = null; if (choice?.into === "new") { try { - target = await createTargetAccount(choice.username); + target = await createTargetAccount(choice.username, actor); } catch (err) { if (err instanceof UploadRefused) return refuse(err); throw err; @@ -583,7 +630,7 @@ export const POST = apiHandler(async (request: NextRequest) => { ); } catch (err) { // Nothing was stored, so the account made for the copy goes again. - if (chosen?.created) await discardCreatedAccount(chosen.id); + if (chosen?.created) await discardCreatedAccount(chosen, actor); if (err instanceof UploadRefused) return refuse(err); // The file was fine; its encrypted copy is larger than the stored-copy // limit (`BACKUP_MAX_STORED_MB`). That is the operator's to act on, with @@ -609,18 +656,6 @@ export const POST = apiHandler(async (request: NextRequest) => { }; const storedOwner = owner as { id: string; username: string } | null; - if (chosen?.created) { - await auditLog("admin.user.create", { - userId: admin.id, - ipAddress, - details: { - targetUserId: chosen.id, - username: chosen.username, - reason: "backup_restore", - backupId: created.id, - }, - }); - } await auditLog("admin.backups.upload", { userId: admin.id, ipAddress, diff --git a/tests/integration/backup-erased-account-restore.test.ts b/tests/integration/backup-erased-account-restore.test.ts index f35b59c7c..9ab37b93a 100644 --- a/tests/integration/backup-erased-account-restore.test.ts +++ b/tests/integration/backup-erased-account-restore.test.ts @@ -677,17 +677,20 @@ describe("restoring the backup of an erased account into a chosen account", () = passwordHash: null, }); + // Traced when it is made, before the file is read; the upload row + // links it to the copy. expect(await auditDetails(prisma, "admin.user.create")).toEqual([ { targetUserId: account.id, username: "returning-person", reason: "backup_restore", - backupId, + outcome: "created", }, ]); expect(await auditDetails(prisma, "admin.backups.upload")).toEqual([ - expect.objectContaining({ ownerId: account.id, into: "new" }), + expect.objectContaining({ backupId, ownerId: account.id, into: "new" }), ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([]); expect(await cellsNaming(prisma, OWNER_ID)).toEqual([]); // How the person gets in: the admin sets a first password with the @@ -861,7 +864,24 @@ describe("restoring the backup of an erased account into a chosen account", () = expect( await prisma.user.count({ where: { username: "would-be-copy" } }), ).toBe(0); - expect(await auditDetails(prisma, "admin.user.create")).toEqual([]); + // The account was made, traced, and removed again, traced as well. + const made = await auditDetails(prisma, "admin.user.create"); + expect(made).toEqual([ + { + targetUserId: expect.any(String), + username: "would-be-copy", + reason: "backup_restore", + outcome: "created", + }, + ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([ + { + targetUserId: (made[0] as { targetUserId: string }).targetUserId, + username: "would-be-copy", + reason: "backup_upload_refused", + outcome: "deleted", + }, + ]); expect(await prisma.dataBackup.count()).toBe(0); // Without a target, the same file goes to its own account as before. @@ -879,6 +899,50 @@ describe("restoring the backup of an erased account into a chosen account", () = SLOW, ); + it( + "leaves a trace of an account made for a refused file when it cannot be removed again", + async () => { + const prisma = getPrismaClient(); + const { json } = await eraseAfterExport(prisma); + await createOwner(prisma); + const { prisma: appPrisma } = await import("@/lib/db"); + const remove = vi + .spyOn(appPrisma.user, "delete") + .mockRejectedValueOnce(new Error("connection reset")); + + const refused = await uploadInto(json, { + into: "new", + username: "left-behind", + }); + remove.mockRestore(); + // The operator still sees why the file was refused. + expect(refused.status, JSON.stringify(refused.body)).toBe(409); + expect(refused.body.meta?.errorCode).toBe("backup.owner.exists"); + + const account = await prisma.user.findUniqueOrThrow({ + where: { username: "left-behind" }, + }); + expect(account).toMatchObject({ passwordHash: null, email: null }); + expect(await auditDetails(prisma, "admin.user.create")).toEqual([ + { + targetUserId: account.id, + username: "left-behind", + reason: "backup_restore", + outcome: "created", + }, + ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([ + { + targetUserId: account.id, + username: "left-behind", + reason: "backup_upload_refused", + outcome: "failed", + }, + ]); + }, + SLOW, + ); + it( "refuses a non-admin session and an admin's Bearer token, and creates nothing", async () => {