Files
API/test/admin/users.admin.router.test.ts
Paddy 33489585a0 Model permissions as (app, role) and name passkeys from their AAGUID
Two changes to the admin module, both made now because it is not deployed yet
and neither is free later.

Permissions were "an app", with a `role` column reserved for a future
fine-grained model. Reviewing whether that reservation was enough found three
problems:

- Every row was written with role = 'admin', hardcoded, and the column
  defaulted to it. On a `tickets` row that reads as "tickets administrator"
  when it only ever meant "has access", and once real roles existed there
  would have been no way to tell an old plain grant from a deliberate one.
- The role never left the database. /admin/me, the user list, the user detail
  and both write endpoints all spoke apps: AppName[]. Adding roles would have
  been a breaking change to /admin/me - and after the cutover that endpoint
  has two more consumers, turning a local edit into a coordinated deploy of
  three apps.
- The key (user_id, app) allowed one role per app, i.e. a tier rather than a
  set of capabilities. Choosing later means an ALTER on a live table.

So: the key is now (user_id, app, role), the role is `access`, and APP_ROLES
in admin.schema.ts is the contract - a role not listed there is rejected with
400 rather than written. permissions: [{app, role}] is on the wire alongside
the derived apps: AppName[], which is kept because the three frontends only
ever ask "may I show this app?". Both write endpoints accept either shape, and
the invitation column (now `permissions`) is parsed leniently: invitations live
seven days, so a deploy that changes the shape has in-flight rows in the old
one. requireAppAccess(app, role?) takes an optional role; nothing passes one
yet.

countActiveAdminsForUpdate now counts DISTINCT users rather than rows. With
several roles per app, counting rows would make a single admin holding two
roles look like two admins and defeat the last-admin guard at exactly the
moment it matters.

Separately, passkey registration now fills `name` from the authenticator's
AAGUID via registration.afterVerification and better-auth's own
getAuthenticatorName, yielding "1Password", "iCloud Keychain", "Windows Hello".
Without it the column stayed NULL and the account page could only label every
passkey "Passkey" - useless when someone has to remove the one on the device
they just lost. A client-supplied name still wins; an unknown AAGUID still
leaves it blank.

148 unit tests (up from 131, including the new admin.schema.test.ts) and 41
integration tests pass. The integration suite applies sql/admin/001_init.sql,
so the new key is exercised rather than trusted.

No production migration is needed - the module is not deployed. An existing dev
database needs three statements: set role = 'access', drop and re-add the
primary key, rename invitations.apps to permissions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-06 11:23:08 +02:00

200 lines
6.7 KiB
TypeScript

import {vi, describe, it, expect, beforeEach, type Mock} from 'vitest';
import express from 'express';
import request from 'supertest';
vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({
listUsers: vi.fn(),
getUserDetail: vi.fn(),
loadAccess: vi.fn(),
setPermissions: vi.fn(),
setPermissionsGuarded: vi.fn(),
disableUser: vi.fn(),
disableUserGuarded: vi.fn(),
enableUser: vi.fn(),
revokeSession: vi.fn(),
countActiveAdmins: vi.fn(),
userExists: vi.fn()
}));
import * as UsersService from '../../src/models/admin/users/users.admin.service.js';
import {usersAdminRouter} from '../../src/models/admin/users/users.admin.router.js';
const service = UsersService as unknown as Record<string, Mock>;
// The router always runs behind requireAppAccess('admin'), which is what puts
// res.locals.admin there; this stands in for it.
const makeApp = (callerId = 'me') => {
const app = express();
app.use(express.json());
app.use((req, res, next) => {
res.locals.admin = {id: callerId, email: 'me@nachklang.art', displayName: 'Me', apps: ['admin']};
next();
});
app.use('/admin/users', usersAdminRouter);
return app;
};
beforeEach(() => {
for (const fn of Object.values(service)) {
if (typeof fn?.mockReset === 'function') {
fn.mockReset();
}
}
service.getUserDetail.mockResolvedValue({id: 'other', apps: []});
service.userExists.mockResolvedValue(true);
service.setPermissionsGuarded.mockResolvedValue('ok');
service.disableUserGuarded.mockResolvedValue('ok');
});
describe('PUT /admin/users/:id/permissions', () => {
it('rejects an unknown app name', async () => {
const res = await request(makeApp()).put('/admin/users/other/permissions').send({apps: ['calendar', 'nope']});
expect(res.status).toBe(400);
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
it('rejects a non-array body', async () => {
const res = await request(makeApp()).put('/admin/users/other/permissions').send({apps: 'admin'});
expect(res.status).toBe(400);
});
it('404s for an unknown user', async () => {
service.userExists.mockResolvedValue(false);
const res = await request(makeApp()).put('/admin/users/ghost/permissions').send({apps: []});
expect(res.status).toBe(404);
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
it('refuses to remove the caller\'s own admin permission', async () => {
service.loadAccess.mockResolvedValue({id: 'me', disabled: false, apps: ['admin']});
service.countActiveAdmins.mockResolvedValue(5);
const res = await request(makeApp('me')).put('/admin/users/me/permissions').send({apps: ['feedback']});
expect(res.status).toBe(409);
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
// The last-admin decision is made inside the write transaction (so two
// admins acting at once cannot both pass a check-then-act); the router's
// job is only to turn that verdict into a 409.
it('answers 409 when the service reports the last admin would be removed', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
service.setPermissionsGuarded.mockResolvedValue('last-admin');
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: []});
expect(res.status).toBe(409);
});
it('allows removing an admin while another active admin remains', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['tickets']});
expect(res.status).toBe(200);
expect(service.setPermissionsGuarded).toHaveBeenCalledWith(
'other',
[{app: 'tickets', role: 'access'}],
'me'
);
});
it('accepts the richer {permissions} body', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: []});
const res = await request(makeApp('me'))
.put('/admin/users/other/permissions')
.send({permissions: [{app: 'tickets', role: 'access'}]});
expect(res.status).toBe(200);
expect(service.setPermissionsGuarded).toHaveBeenCalledWith(
'other',
[{app: 'tickets', role: 'access'}],
'me'
);
});
it('rejects a role that does not exist', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: []});
const res = await request(makeApp('me'))
.put('/admin/users/other/permissions')
.send({permissions: [{app: 'tickets', role: 'refund'}]});
expect(res.status).toBe(400);
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
it('allows granting permissions to someone who has none', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: []});
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['feedback', 'tickets']});
expect(res.status).toBe(200);
expect(service.setPermissionsGuarded).toHaveBeenCalledWith(
'other',
[{app: 'feedback', role: 'access'}, {app: 'tickets', role: 'access'}],
'me'
);
});
});
describe('POST /admin/users/:id/disable', () => {
it('refuses to disable the caller', async () => {
const res = await request(makeApp('me')).post('/admin/users/me/disable');
expect(res.status).toBe(409);
expect(service.disableUserGuarded).not.toHaveBeenCalled();
});
it('answers 409 when the service reports the last active admin would be disabled', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
service.disableUserGuarded.mockResolvedValue('last-admin');
const res = await request(makeApp('me')).post('/admin/users/other/disable');
expect(res.status).toBe(409);
});
it('disables a non-admin user', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['feedback']});
const res = await request(makeApp('me')).post('/admin/users/other/disable');
expect(res.status).toBe(200);
expect(service.disableUserGuarded).toHaveBeenCalledWith('other');
});
it('404s for an unknown user', async () => {
service.loadAccess.mockResolvedValue(null);
const res = await request(makeApp('me')).post('/admin/users/ghost/disable');
expect(res.status).toBe(404);
});
});
describe('DELETE /admin/users/:id/sessions/:sid', () => {
it('404s when the session does not belong to that user', async () => {
service.revokeSession.mockResolvedValue(false);
const res = await request(makeApp()).delete('/admin/users/other/sessions/s1');
expect(res.status).toBe(404);
});
it('204s on a successful revoke', async () => {
service.revokeSession.mockResolvedValue(true);
const res = await request(makeApp()).delete('/admin/users/other/sessions/s1');
expect(res.status).toBe(204);
expect(service.revokeSession).toHaveBeenCalledWith('other', 's1');
});
});