diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index a494231..4c0c34c 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -222,10 +222,14 @@ adminRouter.post('/members', zValidator('json', createMemberSchema, noEchoHook), // - parsePositiveIntParam: rejects malformed ids (T-20-03) // --------------------------------------------------------------------------- -const updateMemberSchema = z.object({ - displayName: z.string().min(1).max(256).optional(), - isAdmin: z.boolean().optional(), -}); +const updateMemberSchema = z + .object({ + displayName: z.string().min(1).max(256).optional(), + isAdmin: z.boolean().optional(), + }) + .refine((data) => data.displayName !== undefined || data.isAdmin !== undefined, { + message: 'At least one field must be provided', + }); adminRouter.patch('/members/:id', zValidator('json', updateMemberSchema, noEchoHook), async (c) => { const targetId = parsePositiveIntParam(c.req.param('id')); @@ -236,35 +240,58 @@ adminRouter.patch('/members/:id', zValidator('json', updateMemberSchema, noEchoH const { displayName, isAdmin } = c.req.valid('json'); // T-20-04: NEVER log request body - // Verify the target user exists (404 if not) - const [target] = await db - .select({ id: users.id, isAdmin: users.isAdmin }) - .from(users) - .where(eq(users.id, targetId)) - .limit(1); - - if (!target) { - return c.json({ error: 'Member not found' }, 404); - } - - // D-03 last-admin guard: reject demotion of the only remaining admin (T-20-02) - if (isAdmin === false && target.isAdmin) { - const [{ count }] = await db - .select({ count: sql`COUNT(*)` }) - .from(users) - .where(eq(users.isAdmin, true)); - if (Number(count) <= 1) { - return c.json({ error: 'Cannot remove the last admin' }, 409); - } - } - - // Build a partial set() from whichever fields are present + // Build a partial set() from whichever fields are present — validated non-empty by schema const updates: { displayName?: string; isAdmin?: boolean } = {}; if (displayName !== undefined) updates.displayName = displayName; if (isAdmin !== undefined) updates.isAdmin = isAdmin; try { - await db.update(users).set(updates).where(eq(users.id, targetId)); + // CR-01: wrap the guard + update in a transaction so the last-admin check and + // the UPDATE are atomic. Without a transaction, two concurrent demotions both + // read count=2, both pass the guard, and both commit — leaving zero admins. + // The locking read (FOR UPDATE via raw SQL suffix) serialises concurrent + // demotions: the second PATCH blocks until the first commits and then re-reads + // a count of 1, triggering the 'last-admin' error correctly. + let lastAdminViolation = false; + let notFound = false; + + await db.transaction(async (tx) => { + // Re-read the target inside the transaction so we see the committed state + const [target] = await tx + .select({ id: users.id, isAdmin: users.isAdmin }) + .from(users) + .where(eq(users.id, targetId)) + .limit(1); + + if (!target) { + notFound = true; + return; + } + + // D-03 last-admin guard: reject demotion of the only remaining admin (T-20-02). + // Use a raw locking read to serialise concurrent demotions. Drizzle 0.45.x does + // not expose a first-class .for('update') on select; appending FOR UPDATE via a + // raw sql suffix achieves the same serialisation in InnoDB. + if (isAdmin === false && target.isAdmin) { + const [[{ count }]] = (await tx.execute( + sql`SELECT COUNT(*) AS count FROM ${users} WHERE ${users.isAdmin} = true FOR UPDATE`, + )) as unknown as [{ count: number | string }[], unknown]; + if (Number(count) <= 1) { + lastAdminViolation = true; + return; + } + } + + await tx.update(users).set(updates).where(eq(users.id, targetId)); + }); + + if (notFound) { + return c.json({ error: 'Member not found' }, 404); + } + if (lastAdminViolation) { + return c.json({ error: 'Cannot remove the last admin' }, 409); + } + return c.json({ ok: true }, 200); } catch (err) { console.error( diff --git a/apps/api/tests/routes/admin.test.ts b/apps/api/tests/routes/admin.test.ts index 62aa3d0..21bc7ee 100644 --- a/apps/api/tests/routes/admin.test.ts +++ b/apps/api/tests/routes/admin.test.ts @@ -1226,6 +1226,21 @@ describe('PATCH /api/admin/members/:id', () => { ); expect(res.status).toBe(404); }); + + // Test H (WR-06): empty {} body must return 400, not crash Drizzle with a 503 + it('Test H (WR-06 empty body): PATCH with {} returns 400 { error: "Invalid request" }', async () => { + const adminId = await seedUser('admin-patch-empty', true); + const memberId = await seedUser('member-patch-empty', false); + currentDevUserId = adminId; + const app = await getApp(); + + const res = await app.fetch( + jsonRequest('PATCH', `/api/admin/members/${memberId}`, {}), + ); + expect(res.status).toBe(400); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe('Invalid request'); + }); }); // ===========================================================================