fix(rbac): enforce admin seat cap on promotion and harden last-admin and audit paths (#1266)

Promoting a user to admin now respects the per-tier admin seat limit the same
way user creation does, closing a path that let an operator exceed the cap by
creating an account and then editing its role to admin.

The last-admin guard for demote and delete now runs the admin-count re-check
and the write in a single transaction, so two concurrent admin changes can no
longer race the admin count to zero and lock everyone out.

Admin two-factor resets are now recorded once with their own audit summary
instead of being mislabeled as a user creation by the audit middleware.

backend/src/__tests__/audit-log.test.ts

@@ -126,6 +126,19 @@ describe('getAuditSummary()', () => {
     expect(getAuditSummary('DELETE', '/nodes/5')).toBe('Deleted node: 5');
     expect(getAuditSummary('DELETE', 'nodes/5')).toBe('Deleted node: 5');
   });
+
+  it('matches user management routes', () => {
+    expect(getAuditSummary('POST', '/users')).toBe('Created user');
+    expect(getAuditSummary('PUT', '/users/42')).toBe('Updated user: 42');
+    expect(getAuditSummary('DELETE', '/users/42')).toBe('Deleted user: 42');
+    expect(getAuditSummary('POST', '/users/42/roles')).toBe('Assigned role: 42');
+    expect(getAuditSummary('DELETE', '/users/42/roles/7')).toBe('Removed role assignment: 42');
+  });
+
+  it('labels an MFA reset distinctly and never as user creation', () => {
+    expect(getAuditSummary('POST', '/users/42/mfa/reset')).toBe('Reset two-factor authentication: 42');
+    expect(getAuditSummary('POST', '/users/42/mfa/reset')).not.toBe('Created user: 42');
+  });
 });
 
 // ---- DatabaseService audit methods ----

backend/src/__tests__/mfa.test.ts

@@ -555,6 +555,21 @@ describe('POST /api/users/:id/mfa/reset', () => {
     expect(db.getUserMfa(userId)).toBeUndefined();
     expect(db.getUser(userId)!.token_version).toBeGreaterThan(before);
   });
+
+  it('writes exactly one audit row, labeled as a reset and never as user creation', async () => {
+    const db = DatabaseService.getInstance();
+    const { userId } = await seedMfaUser('victim3', 'victim3pass123');
+    const res = await request(app)
+      .post(`/api/users/${userId}/mfa/reset`)
+      .set('Authorization', `Bearer ${adminToken()}`);
+    expect(res.status).toBe(200);
+
+    // The route no longer writes its own audit row; the middleware writes one.
+    const { entries } = db.getAuditLogs({ search: `/users/${userId}/mfa/reset` });
+    const resetRows = entries.filter((e) => e.path === `/api/users/${userId}/mfa/reset`);
+    expect(resetRows).toHaveLength(1);
+    expect(resetRows[0].summary).toBe(`Reset two-factor authentication: ${userId}`);
+  });
 });
 
 // ─── SSO bypass toggle ────────────────────────────────────────────────────────

backend/src/__tests__/users-rbac.test.ts

@@ -563,6 +563,85 @@ describe('Last-admin protection', () => {
   });
 });
 
+// ---- Seat Limit Enforcement On Promotion ----
+
+describe('Seat limit enforcement on role promotion', () => {
+  it('rejects promoting a viewer to admin when the admin seat limit is reached', async () => {
+    const db = DatabaseService.getInstance();
+    const hash = await bcrypt.hash('password123', 1);
+    const viewerId = db.addUser({ username: 'promoteme', password_hash: hash, role: 'viewer' });
+
+    const { LicenseService } = await import('../services/LicenseService');
+    vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: 1, maxViewers: null });
+
+    const res = await request(app)
+      .put(`/api/users/${viewerId}`)
+      .set('Authorization', `Bearer ${adminToken()}`)
+      .send({ role: 'admin' });
+    expect(res.status).toBe(403);
+    expect(res.body.error).toContain('maximum');
+    // The role must remain unchanged when the cap blocks the promotion.
+    expect(db.getUser(viewerId)!.role).toBe('viewer');
+
+    vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: null, maxViewers: null });
+    db.deleteUser(viewerId);
+  });
+
+  it('allows promoting a viewer to admin when admin seats are unlimited', async () => {
+    const db = DatabaseService.getInstance();
+    const hash = await bcrypt.hash('password123', 1);
+    const viewerId = db.addUser({ username: 'promoteok', password_hash: hash, role: 'viewer' });
+    // Global beforeAll mock already returns unlimited seats; the gate must not over-block.
+    const res = await request(app)
+      .put(`/api/users/${viewerId}`)
+      .set('Authorization', `Bearer ${adminToken()}`)
+      .send({ role: 'admin' });
+    expect(res.status).toBe(200);
+    expect(db.getUser(viewerId)!.role).toBe('admin');
+    db.deleteUser(viewerId);
+  });
+});
+
+// ---- Atomic Last-Admin Guard (TOCTOU protection) ----
+
+describe('Atomic last-admin guard', () => {
+  // These lock the guard contract: the admin-count re-check and the mutation run
+  // in one transaction, so a refusal writes nothing (no partial state) and the
+  // count is unchanged. That re-check inside the transaction is what closes the
+  // TOCTOU window a route-level pre-check left open.
+  it('updateUserIfNotLastAdmin refuses to demote the sole admin and applies otherwise', async () => {
+    const db = DatabaseService.getInstance();
+    expect(db.getAdminCount()).toBe(1);
+    const sole = db.getUserByUsername(TEST_USERNAME)!;
+    expect(db.updateUserIfNotLastAdmin(sole.id, { role: 'viewer' })).toBe(false);
+    // Refusal is side-effect free: role intact and count unchanged.
+    expect(db.getUser(sole.id)!.role).toBe('admin');
+    expect(db.getAdminCount()).toBe(1);
+
+    const hash = await bcrypt.hash('password123', 1);
+    const extra = db.addUser({ username: 'raceadmin', password_hash: hash, role: 'admin' });
+    expect(db.updateUserIfNotLastAdmin(extra, { role: 'viewer' })).toBe(true);
+    expect(db.getUser(extra)!.role).toBe('viewer');
+    expect(db.getAdminCount()).toBe(1);
+    db.deleteUser(extra);
+  });
+
+  it('deleteUserIfNotLastAdmin refuses to delete the sole admin and applies otherwise', async () => {
+    const db = DatabaseService.getInstance();
+    expect(db.getAdminCount()).toBe(1);
+    const sole = db.getUserByUsername(TEST_USERNAME)!;
+    expect(db.deleteUserIfNotLastAdmin(sole.id)).toBe(false);
+    // Refusal is side-effect free: row intact and count unchanged.
+    expect(db.getUser(sole.id)).toBeTruthy();
+    expect(db.getAdminCount()).toBe(1);
+
+    const hash = await bcrypt.hash('password123', 1);
+    const extra = db.addUser({ username: 'raceadmin2', password_hash: hash, role: 'admin' });
+    expect(db.deleteUserIfNotLastAdmin(extra)).toBe(true);
+    expect(db.getAdminCount()).toBe(1);
+  });
+});
+
 // ---- Orphaned Role Assignment Cleanup ----
 
 describe('Orphaned role assignment cleanup', () => {

backend/src/routes/users.ts

@@ -30,6 +30,24 @@ function validateUsername(value: unknown): string | null {
   return null;
 }
 
+// Returns a seat-limit error message if adding an account of `role` would
+// exceed the current license seat caps, or null when within limits. Counts are
+// read at call time so the check reflects live state. Used by both user
+// creation and admin promotion so the cap cannot be bypassed via role change.
+// Seat caps gate new seat acquisition only (creation, and promotion to admin);
+// reducing privilege by demoting an admin is never blocked on the viewer cap.
+function seatLimitError(role: UserRole, db: DatabaseService): string | null {
+  const seatLimits = LicenseService.getInstance().getSeatLimits();
+  if (role === 'admin') {
+    if (seatLimits.maxAdmins !== null && db.getAdminCount() >= seatLimits.maxAdmins) {
+      return `Your license allows a maximum of ${seatLimits.maxAdmins} admin account${seatLimits.maxAdmins === 1 ? '' : 's'}. Upgrade to Admiral for unlimited accounts.`;
+    }
+  } else if (seatLimits.maxViewers !== null && db.getNonAdminCount() >= seatLimits.maxViewers) {
+    return `Your license allows a maximum of ${seatLimits.maxViewers} viewer account${seatLimits.maxViewers === 1 ? '' : 's'}. Upgrade to Admiral for unlimited accounts.`;
+  }
+  return null;
+}
+
 export const usersRouter = Router();
 
 usersRouter.get('/', authMiddleware, async (req: Request, res: Response): Promise<void> => {
@@ -84,13 +102,9 @@ usersRouter.post('/', authMiddleware, async (req: Request, res: Response): Promi
     }
 
     // Enforce seat limits based on license variant.
-    const seatLimits = LicenseService.getInstance().getSeatLimits();
-    if (role === 'admin' && seatLimits.maxAdmins !== null && db.getAdminCount() >= seatLimits.maxAdmins) {
-      res.status(403).json({ error: `Your license allows a maximum of ${seatLimits.maxAdmins} admin account${seatLimits.maxAdmins === 1 ? '' : 's'}. Upgrade to Admiral for unlimited accounts.` });
-      return;
-    }
-    if (role !== 'admin' && seatLimits.maxViewers !== null && db.getNonAdminCount() >= seatLimits.maxViewers) {
-      res.status(403).json({ error: `Your license allows a maximum of ${seatLimits.maxViewers} viewer account${seatLimits.maxViewers === 1 ? '' : 's'}. Upgrade to Admiral for unlimited accounts.` });
+    const seatError = seatLimitError(role, db);
+    if (seatError) {
+      res.status(403).json({ error: seatError });
       return;
     }
 
@@ -145,9 +159,17 @@ usersRouter.put('/:id', authMiddleware, async (req: Request, res: Response): Pro
         res.status(400).json({ error: 'Cannot change your own role' });
         return;
       }
-      if (user.role === 'admin' && role !== 'admin' && db.getAdminCount() <= 1) {
-        res.status(400).json({ error: 'Cannot demote the only admin user' });
-        return;
+      // Promoting a non-admin to admin consumes an admin seat; enforce the cap
+      // here the same way user creation does, so a role change cannot exceed it.
+      if (role === 'admin' && user.role !== 'admin') {
+        const seatError = seatLimitError('admin', db);
+        if (isDebugEnabled()) {
+          console.log('[Users:diag] admin-promotion id=', id, 'blocked=', seatError !== null, 'actor=', sanitizeForLog(req.user!.username));
+        }
+        if (seatError) {
+          res.status(403).json({ error: seatError });
+          return;
+        }
       }
       updates.role = role;
     }
@@ -166,7 +188,13 @@ usersRouter.put('/:id', authMiddleware, async (req: Request, res: Response): Pro
       updates.password_hash = await bcrypt.hash(password, BCRYPT_SALT_ROUNDS);
     }
 
-    db.updateUser(id, updates);
+    // updateUserIfNotLastAdmin returns false only when this update would demote
+    // the last remaining admin; map that single case to the guard message.
+    const applied = db.updateUserIfNotLastAdmin(id, updates);
+    if (!applied) {
+      res.status(400).json({ error: 'Cannot demote the only admin user' });
+      return;
+    }
     // Invalidate the user's active sessions when their role or password changes.
     if (updates.role || updates.password_hash) {
       db.bumpTokenVersion(id);
@@ -196,12 +224,11 @@ usersRouter.delete('/:id', authMiddleware, async (req: Request, res: Response):
       return;
     }
 
-    if (user.role === 'admin' && db.getAdminCount() <= 1) {
+    const deleted = db.deleteUserIfNotLastAdmin(id);
+    if (!deleted) {
       res.status(400).json({ error: 'Cannot delete the only admin user' });
       return;
     }
-
-    db.deleteUser(id);
     console.log('[Users] Deleted:', user.username, '(id:', id, ') by:', req.user!.username);
     res.json({ success: true });
   } catch (error) {
@@ -230,20 +257,9 @@ usersRouter.post('/:id/mfa/reset', authMiddleware, (req: Request, res: Response)
     }
     db.deleteUserMfa(id);
     db.bumpTokenVersion(id);
-    try {
-      db.insertAuditLog({
-        timestamp: Date.now(),
-        username: req.user!.username,
-        method: 'POST',
-        path: req.originalUrl,
-        status_code: 200,
-        node_id: null,
-        ip_address: req.ip || 'unknown',
-        summary: `Admin reset two-factor authentication for ${target.username}`,
-      });
-    } catch (err) {
-      console.warn('[MFA] Admin reset audit log write failed:', getErrorMessage(err, 'unknown'));
-    }
+    // The audit-log middleware records this POST automatically (summary
+    // "Reset two-factor authentication: <id>", keyed on the target user id);
+    // no explicit write is needed here.
     console.log('[MFA] Admin reset: target=', target.username, 'by=', req.user!.username);
     if (isDebugEnabled()) {
       console.log('[MFA:diag] admin-reset target=', target.username, 'actor=', req.user!.username);

backend/src/services/DatabaseService.ts

@@ -2622,6 +2622,38 @@ export class DatabaseService {
         this.db.prepare('DELETE FROM users WHERE id = ?').run(id);
     }
 
+    /**
+     * Atomically apply `updates` unless doing so would demote the last remaining
+     * admin. Returns false (nothing written) when the change would leave zero
+     * admins, true otherwise. The current-role read, the admin count, and the
+     * write run in a single transaction so a concurrent demote or delete of the
+     * other admin cannot race the count to zero.
+     */
+    public updateUserIfNotLastAdmin(id: number, updates: Partial<{ username: string; password_hash: string; role: string; email: string }>): boolean {
+        return this.transaction(() => {
+            if (updates.role !== undefined && updates.role !== 'admin') {
+                const current = this.db.prepare('SELECT role FROM users WHERE id = ?').get(id) as { role: string } | undefined;
+                if (current?.role === 'admin' && this.getAdminCount() <= 1) return false;
+            }
+            this.updateUser(id, updates);
+            return true;
+        });
+    }
+
+    /**
+     * Atomically delete the user unless it is the last remaining admin. Returns
+     * false (nothing deleted) in that case, true otherwise. Same single-
+     * transaction guard as {@link updateUserIfNotLastAdmin}.
+     */
+    public deleteUserIfNotLastAdmin(id: number): boolean {
+        return this.transaction(() => {
+            const current = this.db.prepare('SELECT role FROM users WHERE id = ?').get(id) as { role: string } | undefined;
+            if (current?.role === 'admin' && this.getAdminCount() <= 1) return false;
+            this.deleteUser(id);
+            return true;
+        });
+    }
+
     public getUserCount(): number {
         return (this.db.prepare('SELECT COUNT(*) as count FROM users').get() as { count: number })?.count || 0;
     }

backend/src/utils/audit-summaries.ts

@@ -53,6 +53,7 @@ export const AUDIT_ROUTE_SUMMARIES: Record<string, string> = {
   'POST /users': 'Created user',
   'DELETE /users': 'Deleted user',
   'PUT /users': 'Updated user',
+  'POST /users/*/mfa/reset': 'Reset two-factor authentication',
   'POST /users/*/roles': 'Assigned role',
   'DELETE /users/*/roles': 'Removed role assignment',
 

docs/features/rbac.mdx

@@ -105,7 +105,7 @@ The password fields change subtly in edit mode:
 - The **Password** label becomes **New Password (optional)** with the placeholder `Leave blank to keep`. Submit without filling them in and the current password is preserved.
 - If the user was provisioned via SSO, the password fields are replaced with an inline line that reads `Password is managed by the identity provider (<provider>).` Sencho never stores or rotates passwords for SSO accounts.
 
-Click **Update user** to save. Changing the role takes effect on the next API request from any of that user's active sessions; see [Session security](#session-security) below.
+Click **Update user** to save. Changing the role takes effect on the next API request from any of that user's active sessions; see [Session security](#session-security) below. Promoting a user to **Admin** consumes an admin seat, so it is subject to the same per-tier limit as creating an admin: at your cap, the change is rejected until you free a seat or upgrade.
 
 ## Scoped permissions
 
@@ -224,8 +224,8 @@ Entries include the acting user, IP address, HTTP method and path, response stat
   <Accordion title="The role I want is greyed out in the role combobox">
     The combobox only shows roles available on your tier. On Skipper, the combobox lists Admin and Viewer only. **Deployer**, **Node Admin**, and **Auditor** are Admiral-only roles and do not appear on Skipper. Upgrade to Admiral, or use scoped permissions equivalents once you do.
   </Accordion>
-  <Accordion title="Creating a user fails with `Your license allows a maximum of N account(s)`">
-    You have hit the seat limit for your tier. Skipper allows one admin and three non-admin users; Admiral has no cap. Either delete an unused account or upgrade. The exact remaining capacity is visible on the OPERATORS counter in the panel header.
+  <Accordion title="Creating a user or promoting one to Admin fails with `Your license allows a maximum of N account(s)`">
+    You have hit the seat limit for your tier. Skipper allows one admin and three non-admin users; Admiral has no cap. Promoting an existing user to Admin counts against the admin limit the same way creating one does. Either delete an unused account or upgrade. The exact remaining capacity is visible on the OPERATORS counter in the panel header.
   </Accordion>
   <Accordion title="A user complains they were signed out unexpectedly">
     Token-version bumps invalidate sessions. Two events do this: an admin changed the user's password, or an admin reset their 2FA. Both rotate the user's token version, so every JWT issued before the rotation is rejected on the next request. The user can sign in again with their (possibly new) password. Role changes do **not** sign the user out; they take effect on the next request without rotating the token version.