fix(auth): atomic last-admin guards on role-change and delete

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);
+  });
 });

src/data/users.ts

@@ -25,7 +25,9 @@ export interface UserStore {
   verifyPassword(plain: string, hash: string): Promise<boolean>;
   changePassword(userId: string, newPassword: string): Promise<void>;
   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();
+    },
   };
 }

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();
   });