Phase 20: Admin Member Editor & Form Declutter #25
@@ -222,10 +222,14 @@ adminRouter.post('/members', zValidator('json', createMemberSchema, noEchoHook),
|
||||
// - parsePositiveIntParam: rejects malformed ids (T-20-03)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const updateMemberSchema = z.object({
|
||||
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
|
||||
// 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 {
|
||||
// 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);
|
||||
}
|
||||
|
||||
// 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<number>`COUNT(*)` })
|
||||
.from(users)
|
||||
.where(eq(users.isAdmin, true));
|
||||
if (Number(count) <= 1) {
|
||||
if (lastAdminViolation) {
|
||||
return c.json({ error: 'Cannot remove the last admin' }, 409);
|
||||
}
|
||||
}
|
||||
|
||||
// Build a partial set() from whichever fields are present
|
||||
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));
|
||||
return c.json({ ok: true }, 200);
|
||||
} catch (err) {
|
||||
console.error(
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
// ===========================================================================
|
||||
|
||||
Reference in New Issue
Block a user