diff --git a/src/data/users.test.ts b/src/data/users.test.ts index c365903..66f7ff4 100644 --- a/src/data/users.test.ts +++ b/src/data/users.test.ts @@ -122,4 +122,65 @@ describe("UserStore", () => { expect(alice.role).toBe("admin"); expect(bob.role).toBe("member"); }); + + it("setRoleIfNotLastAdmin returns 'would_orphan' for the only admin being demoted", async () => { + const alice = await users.createUser("alice", "pw-alice", "admin"); + expect(users.setRoleIfNotLastAdmin(alice.id, "member")).toBe("would_orphan"); + expect(users.findById(alice.id)!.role).toBe("admin"); // unchanged + }); + + it("setRoleIfNotLastAdmin allows demotion when another admin exists", async () => { + const alice = await users.createUser("alice", "pw-alice", "admin"); + await users.createUser("bob", "pw-bob-bob", "admin"); + expect(users.setRoleIfNotLastAdmin(alice.id, "member")).toBe("ok"); + expect(users.findById(alice.id)!.role).toBe("member"); + }); + + it("setRoleIfNotLastAdmin returns 'not_found' for unknown id", () => { + expect(users.setRoleIfNotLastAdmin("not-a-real-id", "member")).toBe("not_found"); + }); + + it("setRoleIfNotLastAdmin: concurrent demotions of two admins keep one admin", async () => { + const alice = await users.createUser("alice", "pw-alice", "admin"); + const bob = await users.createUser("bob", "pw-bob-bob", "admin"); + // Concurrent demotion of both + const [r1, r2] = await Promise.all([ + Promise.resolve(users.setRoleIfNotLastAdmin(alice.id, "member")), + Promise.resolve(users.setRoleIfNotLastAdmin(bob.id, "member")), + ]); + // Exactly one should succeed; the other gets "would_orphan" + const oks = [r1, r2].filter((r) => r === "ok").length; + const orphans = [r1, r2].filter((r) => r === "would_orphan").length; + expect(oks).toBe(1); + expect(orphans).toBe(1); + // System retains at least one admin + expect(users.countAdmins()).toBe(1); + }); + + it("deleteUserIfNotLastAdmin returns 'would_orphan' for the only admin", async () => { + const alice = await users.createUser("alice", "pw-alice", "admin"); + expect(users.deleteUserIfNotLastAdmin(alice.id)).toBe("would_orphan"); + expect(users.findById(alice.id)).not.toBeNull(); + }); + + it("deleteUserIfNotLastAdmin allows deleting a member at any count", async () => { + await users.createUser("alice", "pw-alice", "admin"); + const bob = await users.createUser("bob", "pw-bob-bob", "member"); + expect(users.deleteUserIfNotLastAdmin(bob.id)).toBe("ok"); + expect(users.findById(bob.id)).toBeNull(); + }); + + it("deleteUserIfNotLastAdmin: concurrent deletes of two admins keep one admin", async () => { + const alice = await users.createUser("alice", "pw-alice", "admin"); + const bob = await users.createUser("bob", "pw-bob-bob", "admin"); + const [r1, r2] = await Promise.all([ + Promise.resolve(users.deleteUserIfNotLastAdmin(alice.id)), + Promise.resolve(users.deleteUserIfNotLastAdmin(bob.id)), + ]); + const oks = [r1, r2].filter((r) => r === "ok").length; + const orphans = [r1, r2].filter((r) => r === "would_orphan").length; + expect(oks).toBe(1); + expect(orphans).toBe(1); + expect(users.countAdmins()).toBe(1); + }); }); diff --git a/src/data/users.ts b/src/data/users.ts index 1ad4728..432887d 100644 --- a/src/data/users.ts +++ b/src/data/users.ts @@ -25,7 +25,9 @@ export interface UserStore { verifyPassword(plain: string, hash: string): Promise; changePassword(userId: string, newPassword: string): Promise; setRole(userId: string, role: UserRole): boolean; + setRoleIfNotLastAdmin(id: string, newRole: UserRole): "ok" | "not_found" | "would_orphan"; deleteUser(id: string): boolean; + deleteUserIfNotLastAdmin(id: string): "ok" | "not_found" | "would_orphan"; listUsers(): Array<{ id: string; username: string; createdAt: number; role: UserRole }>; } @@ -125,6 +127,21 @@ export function createUserStore(db: Database.Database): UserStore { return result.changes > 0; }, + setRoleIfNotLastAdmin(id, newRole) { + const tx = db.transaction(() => { + const row = findByIdStmt.get(id) as UserRow | undefined; + if (!row) return "not_found" as const; + if (row.role === newRole) return "ok" as const; // no-op + if (row.role === "admin" && newRole === "member") { + const adminCount = (countAdminsStmt.get() as { n: number }).n; + if (adminCount <= 1) return "would_orphan" as const; + } + updateRoleStmt.run(newRole, Date.now(), id); + return "ok" as const; + }); + return tx(); + }, + listUsers() { return listUsersStmt.all() as Array<{ id: string; username: string; createdAt: number; role: UserRole }>; }, @@ -133,5 +150,19 @@ export function createUserStore(db: Database.Database): UserStore { const result = deleteUserStmt.run(id); return result.changes > 0; }, + + deleteUserIfNotLastAdmin(id) { + const tx = db.transaction(() => { + const row = findByIdStmt.get(id) as UserRow | undefined; + if (!row) return "not_found" as const; + if (row.role === "admin") { + const adminCount = (countAdminsStmt.get() as { n: number }).n; + if (adminCount <= 1) return "would_orphan" as const; + } + deleteUserStmt.run(id); + return "ok" as const; + }); + return tx(); + }, }; } diff --git a/src/web/api/users.ts b/src/web/api/users.ts index b4f54b6..0517654 100644 --- a/src/web/api/users.ts +++ b/src/web/api/users.ts @@ -58,6 +58,7 @@ export function createUsersRouter( router.delete("/:id", (req, res) => { const targetId = req.params.id; + // Snapshot target's username BEFORE deletion for audit const target = users.findById(targetId); if (!target) { res.status(404).json({ error: "not found" }); @@ -67,15 +68,16 @@ export function createUsersRouter( res.status(400).json({ error: "cannot delete self" }); return; } - if (target.role === "admin" && users.countAdmins() <= 1) { - res.status(400).json({ error: "cannot delete last admin" }); - return; - } - const deleted = users.deleteUser(targetId); - if (!deleted) { + const result = users.deleteUserIfNotLastAdmin(targetId); + if (result === "not_found") { res.status(404).json({ error: "not found" }); return; } + if (result === "would_orphan") { + res.status(400).json({ error: "cannot delete last admin" }); + return; + } + // FK CASCADE removes sessions; explicit call is belt-and-suspenders sessions.deleteAllForUser(targetId); try { audit.record({ @@ -128,34 +130,35 @@ export function createUsersRouter( res.status(400).json({ error: "invalid role" }); return; } - const target = users.findById(targetId); - if (!target) { + // Snapshot the target's old role and username for audit (BEFORE the atomic update, + // so we record what actually changed; if the user is gone we'll skip audit). + const targetBefore = users.findById(targetId); + if (!targetBefore) { res.status(404).json({ error: "not found" }); return; } - if (target.role === newRole) { - res.status(204).end(); + const result = users.setRoleIfNotLastAdmin(targetId, newRole); + if (result === "not_found") { + res.status(404).json({ error: "not found" }); return; } - if (target.role === "admin" && newRole === "member" && users.countAdmins() <= 1) { + if (result === "would_orphan") { res.status(400).json({ error: "cannot demote last admin" }); return; } - const changed = users.setRole(targetId, newRole); - if (!changed) { - res.status(404).json({ error: "not found" }); - return; + // Only audit when the role actually changed + if (targetBefore.role !== newRole) { + try { + audit.record({ + actorId: req.user!.id, actorUsername: req.user!.username, + targetUserId: targetBefore.id, targetUsername: targetBefore.username, + action: "user.role_changed", + }); + } catch (auditErr) { + logger.warn({ err: auditErr, action: "user.role_changed" }, "audit insert failed"); + } + logger.info({ actorId: req.user!.id, targetId, newRole }, "User role changed"); } - try { - audit.record({ - actorId: req.user!.id, actorUsername: req.user!.username, - targetUserId: target.id, targetUsername: target.username, - action: "user.role_changed", - }); - } catch (auditErr) { - logger.warn({ err: auditErr, action: "user.role_changed" }, "audit insert failed"); - } - logger.info({ actorId: req.user!.id, targetId, newRole }, "User role changed"); res.status(204).end(); });