fix(staff): add revoke button to super user rows + serialize guardrail in transaction

Frontend:
- Super users now see a "Revoke" button (disabled when last super user)
  alongside the ★ badge on super-user rows in the Staff table.
  Non-super-user rows show the existing "+ Grant" button.

Backend (race condition fix):
- PATCH /api/staff/:id (isSuperUser=false or active=false): count check +
  update now wrapped in a db.transaction() with FOR UPDATE lock on the
  target row, preventing a race where two concurrent revokes could both
  pass the guard and leave zero super users.
- DELETE /api/staff/🆔 same transaction + FOR UPDATE guard applied.

GRO-206 CTO review feedback

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
groombook-ci[bot]
2026-03-29 12:11:53 +00:00
parent e3c5ebb337
commit c76a37b15c
2 changed files with 90 additions and 56 deletions
+75 -53
View File
@@ -65,42 +65,61 @@ staffRouter.patch("/:id", zValidator("json", updateStaffSchema), async (c) => {
return c.json({ error: "Forbidden: super user privileges required to modify super user status" }, 403); return c.json({ error: "Forbidden: super user privileges required to modify super user status" }, 403);
} }
// Before revoking super user, check if this is the last one // Before revoking or deactivating the last super user, serialize access with a
if (body.isSuperUser === false) { // transaction + FOR UPDATE to prevent a race where two concurrent requests both
const superUserCount = await db // pass the count check and leave zero super users.
.select({ id: staff.id }) const needsSuperUserGuard = body.isSuperUser === false || body.active === false;
.from(staff) if (needsSuperUserGuard) {
.where(and(eq(staff.isSuperUser, true), eq(staff.active, true))) const [guardError, row] = await db.transaction(async (tx) => {
.limit(2); // Lock the target row so no other request can modify it concurrently
// If only 1 or 0 active super users and the target is one of them, block revoke const [target] = await tx
if (superUserCount.length <= 1) { .select({ isSuperUser: staff.isSuperUser })
return c.json( .from(staff)
{ error: "Cannot revoke the last super user. Assign another super user first." }, .where(eq(staff.id, targetId))
400 .limit(1)
); .for("update");
}
}
// When deactivating a super user, also check last-super-user guardrail if (!target) return ["Not found", null as (typeof staff.$inferSelect | null)];
if (body.active === false) {
const [target] = await db // Only enforce guard if the target is actually a super user
.select({ isSuperUser: staff.isSuperUser }) const isRevokingSuperUser = body.isSuperUser === false && target.isSuperUser;
.from(staff) const isDeactivatingSuperUser = body.active === false && target.isSuperUser;
.where(eq(staff.id, targetId)) if (!isRevokingSuperUser && !isDeactivatingSuperUser) {
.limit(1); const [updated] = await tx
if (target?.isSuperUser) { .update(staff)
const superUserCount = await db .set({ ...body, updatedAt: new Date() })
.where(eq(staff.id, targetId))
.returning();
return [null, updated];
}
// Count active super users (excluding target — it will be changed)
const superUserCount = await tx
.select({ id: staff.id }) .select({ id: staff.id })
.from(staff) .from(staff)
.where(and(eq(staff.isSuperUser, true), eq(staff.active, true))) .where(and(eq(staff.isSuperUser, true), eq(staff.active, true), ne(staff.id, targetId)))
.limit(2); .limit(2);
if (superUserCount.length <= 1) { if (superUserCount.length <= 1) {
return c.json( return [
{ error: "Cannot deactivate the last super user. Assign another super user first." }, body.isSuperUser === false
400 ? "Cannot revoke the last super user. Assign another super user first."
); : "Cannot deactivate the last super user. Assign another super user first.",
null,
];
} }
}
const [updated] = await tx
.update(staff)
.set({ ...body, updatedAt: new Date() })
.where(eq(staff.id, targetId))
.returning();
return [null, updated];
});
if (guardError) return c.json({ error: guardError }, guardError === "Not found" ? 404 : 400);
if (!row) return c.json({ error: "Not found" }, 404);
return c.json(row);
} }
const [row] = await db const [row] = await db
@@ -138,31 +157,34 @@ staffRouter.delete("/:id", async (c) => {
); );
} }
// Prevent deleting the last super user // Prevent deleting the last super user — use transaction to avoid race
const [targetStaff] = await db const [guardError, deleted] = await db.transaction(async (tx) => {
.select({ isSuperUser: staff.isSuperUser }) const [targetStaff] = await tx
.from(staff) .select({ isSuperUser: staff.isSuperUser })
.where(eq(staff.id, id))
.limit(1);
if (targetStaff?.isSuperUser) {
const superUserCount = await db
.select({ id: staff.id })
.from(staff) .from(staff)
.where(and(eq(staff.isSuperUser, true), eq(staff.active, true))) .where(eq(staff.id, id))
.limit(2); .limit(1)
if (superUserCount.length <= 1) { .for("update");
return c.json(
{ error: "Cannot delete the last super user. Assign another super user first." },
400
);
}
}
const [row] = await db if (!targetStaff) return ["Not found", null];
.delete(staff)
.where(eq(staff.id, id)) if (targetStaff.isSuperUser) {
.returning(); const superUserCount = await tx
if (!row) return c.json({ error: "Not found" }, 404); .select({ id: staff.id })
.from(staff)
.where(and(eq(staff.isSuperUser, true), eq(staff.active, true), ne(staff.id, id)))
.limit(2);
if (superUserCount.length <= 1) {
return ["Cannot delete the last super user. Assign another super user first.", null];
}
}
const [row] = await tx.delete(staff).where(eq(staff.id, id)).returning();
return [null, row];
});
if (guardError === "Not found") return c.json({ error: "Not found" }, 404);
if (guardError) return c.json({ error: guardError }, 400);
return c.json({ ok: true }); return c.json({ ok: true });
}); });
+15 -3
View File
@@ -143,9 +143,21 @@ export function StaffPage() {
<td style={tdStyle}><span style={{ textTransform: "capitalize" }}>{s.role}</span></td> <td style={tdStyle}><span style={{ textTransform: "capitalize" }}>{s.role}</span></td>
<td style={tdStyle}> <td style={tdStyle}>
{s.isSuperUser ? ( {s.isSuperUser ? (
<span style={{ display: "inline-flex", alignItems: "center", gap: 4, padding: "2px 8px", borderRadius: 12, fontSize: 11, fontWeight: 600, background: "#ede9fe", color: "#5b21b6" }}> <>
Super User <span style={{ display: "inline-flex", alignItems: "center", gap: 4, padding: "2px 8px", borderRadius: 12, fontSize: 11, fontWeight: 600, background: "#ede9fe", color: "#5b21b6" }}>
</span> Super User
</span>
{isCurrentUserSuperUser && s.id !== me?.id && (
<button
onClick={() => toggleSuperUser(s)}
disabled={togglingSuperUser === s.id || activeSuperUserCount <= 1}
title={activeSuperUserCount <= 1 ? "Cannot revoke: last super user" : undefined}
style={{ ...btnStyle, padding: "0.2rem 0.6rem", fontSize: 11, background: "#f3f4f6", color: "#374151", borderColor: "#d1d5db", marginLeft: 4 }}
>
{togglingSuperUser === s.id ? "…" : "Revoke"}
</button>
)}
</>
) : isCurrentUserSuperUser ? ( ) : isCurrentUserSuperUser ? (
<button <button
onClick={() => toggleSuperUser(s)} onClick={() => toggleSuperUser(s)}