From b37e4f5df75c441c02368adb879fe4f797577359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 17:02:32 +0100 Subject: [PATCH] fix(admin): the last-admin rule counts only admins who can sign in Demoting an admin counted every admin, pending deletion included, so after one admin deleted their account the other could step down and leave the instance with no admin who can sign in or undo the deletion. Both the demotion and an admin's own deletion now count the other admins not pending deletion, inside the transaction that makes the change, under one roster lock, so two at the same moment cannot each count the other. --- .../admin/users/[id]/__tests__/route.test.ts | 38 +++++++-- src/app/api/admin/users/[id]/route.ts | 80 +++++++++++-------- src/app/api/settings/account/route.ts | 10 +++ src/lib/auth/admin-roster.ts | 37 +++++++++ 4 files changed, 126 insertions(+), 39 deletions(-) create mode 100644 src/lib/auth/admin-roster.ts diff --git a/src/app/api/admin/users/[id]/__tests__/route.test.ts b/src/app/api/admin/users/[id]/__tests__/route.test.ts index 7ef8e1828..e5219f1be 100644 --- a/src/app/api/admin/users/[id]/__tests__/route.test.ts +++ b/src/app/api/admin/users/[id]/__tests__/route.test.ts @@ -1,15 +1,21 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { NextRequest } from "next/server"; -vi.mock("@/lib/db", () => ({ - prisma: { +vi.mock("@/lib/db", () => { + const prisma = { user: { findUnique: vi.fn(), count: vi.fn(), update: vi.fn(), }, - }, -})); + $queryRaw: vi.fn(async () => [{ locked: 1 }]), + $transaction: vi.fn(), + }; + prisma.$transaction.mockImplementation( + async (fn: (tx: typeof prisma) => unknown) => fn(prisma), + ); + return { prisma }; +}); vi.mock("@/lib/api-handler", async () => { const actual = @@ -111,7 +117,8 @@ describe("PUT /api/admin/users/[id]", () => { id: "u1", role: "ADMIN", } as never); - vi.mocked(prisma.user.count).mockResolvedValue(1); + // No other admin who can sign in. + vi.mocked(prisma.user.count).mockResolvedValue(0); const res = await PUT(jsonReq({ role: "USER" }), params("u1")); expect(res.status).toBe(400); const body = (await res.json()) as { error: string }; @@ -119,12 +126,31 @@ describe("PUT /api/admin/users/[id]", () => { expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled(); }); + // An admin pending deletion cannot sign in and is erased when the window + // ends; counted, they let the last usable admin step down. + it("counts only the other admins who are not pending deletion, under the roster lock", async () => { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + id: "u1", + role: "ADMIN", + } as never); + vi.mocked(prisma.user.count).mockResolvedValue(0); + await PUT(jsonReq({ role: "USER" }), params("u1")); + expect(vi.mocked(prisma.user.count).mock.calls[0]?.[0]).toEqual({ + where: { + role: "ADMIN", + id: { not: "u1" }, + deletionRequestedAt: null, + }, + }); + expect(prisma.$queryRaw).toHaveBeenCalled(); + }); + it("allows demoting an admin while another admin exists", async () => { vi.mocked(prisma.user.findUnique).mockResolvedValue({ id: "u1", role: "ADMIN", } as never); - vi.mocked(prisma.user.count).mockResolvedValue(2); + vi.mocked(prisma.user.count).mockResolvedValue(1); vi.mocked(prisma.user.update).mockResolvedValue({ id: "u1", username: "alice", diff --git a/src/app/api/admin/users/[id]/route.ts b/src/app/api/admin/users/[id]/route.ts index 95349dc68..687edeaf2 100644 --- a/src/app/api/admin/users/[id]/route.ts +++ b/src/app/api/admin/users/[id]/route.ts @@ -1,4 +1,8 @@ import { prisma } from "@/lib/db"; +import { + assertAnotherAdminStays, + LastAdminError, +} from "@/lib/auth/admin-roster"; import { apiHandler, requireAdmin } from "@/lib/api-handler"; import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, getClientIp } from "@/lib/api-response"; @@ -55,43 +59,53 @@ export const PUT = apiHandler( }); if (!target) return apiError("User not found", 404); - // Prevent removing the last admin - if (parsed.data.role === "USER" && target.role === "ADMIN") { - const adminCount = await prisma.user.count({ where: { role: "ADMIN" } }); - if (adminCount <= 1) { + const demoting = parsed.data.role === "USER" && target.role === "ADMIN"; + + let updatedUser; + try { + updatedUser = await prisma.$transaction(async (tx) => { + // Another admin who can sign in must stay: one pending deletion does + // not count, and the count holds the roster lock an admin's own + // deletion takes too. + if (demoting) await assertAnotherAdminStays(tx, id); + return tx.user.update({ + where: { id }, + data: { + ...(parsed.data.username !== undefined && { + username: parsed.data.username, + }), + ...(parsed.data.email !== undefined && { + email: parsed.data.email, + }), + ...(parsed.data.role !== undefined && { role: parsed.data.role }), + ...(parsed.data.mfaEnforced !== undefined && { + mfaEnforced: parsed.data.mfaEnforced, + }), + ...(parsed.data.documentQuotaBytes !== undefined && { + documentQuotaBytes: + parsed.data.documentQuotaBytes === null + ? null + : BigInt(parsed.data.documentQuotaBytes), + }), + }, + select: { + id: true, + username: true, + email: true, + role: true, + createdAt: true, + mfaEnforced: true, + documentQuotaBytes: true, + }, + }); + }); + } catch (error) { + if (error instanceof LastAdminError) { return apiError("The last admin cannot be demoted", 400); } + throw error; } - const updatedUser = await prisma.user.update({ - where: { id }, - data: { - ...(parsed.data.username !== undefined && { - username: parsed.data.username, - }), - ...(parsed.data.email !== undefined && { email: parsed.data.email }), - ...(parsed.data.role !== undefined && { role: parsed.data.role }), - ...(parsed.data.mfaEnforced !== undefined && { - mfaEnforced: parsed.data.mfaEnforced, - }), - ...(parsed.data.documentQuotaBytes !== undefined && { - documentQuotaBytes: - parsed.data.documentQuotaBytes === null - ? null - : BigInt(parsed.data.documentQuotaBytes), - }), - }, - select: { - id: true, - username: true, - email: true, - role: true, - createdAt: true, - mfaEnforced: true, - documentQuotaBytes: true, - }, - }); - await auditLog("admin.user.update", { userId: user.id, ipAddress: getClientIp(request), diff --git a/src/app/api/settings/account/route.ts b/src/app/api/settings/account/route.ts index 1c1a1f290..2378f2aee 100644 --- a/src/app/api/settings/account/route.ts +++ b/src/app/api/settings/account/route.ts @@ -22,6 +22,10 @@ import { lockAccountForDeletion, } from "@/lib/account-deletion/lifecycle"; import { ACTIVE_ACCOUNT } from "@/lib/account-deletion/active-account"; +import { + assertAnotherAdminStays, + LastAdminError, +} from "@/lib/auth/admin-roster"; export const dynamic = "force-dynamic"; @@ -110,6 +114,9 @@ export const DELETE = apiHandler(async (request: NextRequest) => { await deleteGuardianAccountWithLifecycle( userId, async (tx) => { + // Checked again under the roster lock: two admins deleting at the + // same moment must not each count the other. + if (user.role === "ADMIN") await assertAnotherAdminStays(tx, userId); // The invariant has now passed, so this success event cannot survive // a refusal. On an immediate erasure it is purged with the rest of the // account's audit history; during a grace window it is the account's @@ -145,6 +152,9 @@ export const DELETE = apiHandler(async (request: NextRequest) => { { erasing: purgeAt === null }, ); } catch (error) { + if (error instanceof LastAdminError) { + return apiError("The last admin account cannot be deleted", 400); + } if (error instanceof LastManagedGuardianError) { return apiError( "Add another Guardian or delete the managed profile first", diff --git a/src/lib/auth/admin-roster.ts b/src/lib/auth/admin-roster.ts new file mode 100644 index 000000000..925e17169 --- /dev/null +++ b/src/lib/auth/admin-roster.ts @@ -0,0 +1,37 @@ +/** + * The rule that an instance keeps an admin who can sign in. + * + * Two changes can take the last one away: an admin demoted, and an admin + * deleting their own account. Both count the other admins who are not pending + * deletion (one pending deletion cannot sign in and is erased when the window + * ends) under one transaction-scoped lock, so two of them at the same moment + * cannot each count the other and leave nobody. + */ +import type { Prisma } from "@/generated/prisma/client"; +import { ACTIVE_ACCOUNT } from "@/lib/account-deletion/active-account"; + +/** Thrown inside the transaction to roll the change back. */ +export class LastAdminError extends Error { + constructor() { + super("the instance would be left without an admin who can sign in"); + this.name = "LastAdminError"; + } +} + +/** + * Hold the admin roster for the rest of `tx`, then throw `LastAdminError` + * unless an admin other than `leavingId` stays, not pending deletion. + */ +export async function assertAnotherAdminStays( + tx: Prisma.TransactionClient, + leavingId: string, +): Promise { + await tx.$queryRaw` + SELECT 1 AS locked + FROM pg_advisory_xact_lock(hashtextextended('admin-roster', 0)) + `; + const others = await tx.user.count({ + where: { role: "ADMIN", id: { not: leavingId }, ...ACTIVE_ACCOUNT }, + }); + if (others === 0) throw new LastAdminError(); +}