fix: defensive rollback in removeRoleMember, type/style cleanup, test coverage

- Wrap removeRoleMember post-write admin rollback in try/catch so a
  transient DB failure cannot leave the system with zero administrators
- Replace double `as unknown[] as IRole[]` cast with `.lean<IRole[]>()`
- Type parsePagination param explicitly; extract DEFAULT/MAX page constants
- Preserve original error cause in updateRoleByName re-throw
- Add test for rollback failure path in removeRoleMember (returns 400)
- Add test for pre-existing roles missing description field (.lean())
This commit is contained in:
Dustin Healy 2026-03-26 17:55:58 -07:00
parent ad47919ecd
commit e55db4bfb4
4 changed files with 48 additions and 7 deletions

View file

@ -1196,6 +1196,28 @@ describe('createAdminRolesHandlers', () => {
});
});
it('returns 400 even when rollback updateUser throws', async () => {
const deps = createDeps({
getRoleByName: jest.fn().mockResolvedValue(mockRole({ name: SystemRoles.ADMIN })),
findUser: jest.fn().mockResolvedValue(mockUser({ role: SystemRoles.ADMIN })),
countUsersByRole: jest.fn().mockResolvedValueOnce(2).mockResolvedValueOnce(0),
updateUser: jest
.fn()
.mockResolvedValueOnce(mockUser({ role: SystemRoles.USER }))
.mockRejectedValueOnce(new Error('rollback failed')),
});
const handlers = createAdminRolesHandlers(deps);
const { req, res, status, json } = createReqRes({
params: { name: SystemRoles.ADMIN, userId: validUserId },
});
await handlers.removeRoleMember(req, res);
expect(status).toHaveBeenCalledWith(400);
expect(json).toHaveBeenCalledWith({ error: 'Cannot remove the last admin user' });
expect(deps.updateUser).toHaveBeenCalledTimes(2);
});
it('returns 500 on unexpected error', async () => {
const deps = createDeps({
getRoleByName: jest.fn().mockResolvedValue(mockRole()),

View file

@ -8,9 +8,15 @@ import type { ServerRequest } from '~/types/http';
const MAX_NAME_LENGTH = 500;
const MAX_DESCRIPTION_LENGTH = 2000;
function parsePagination(query: Record<string, unknown>): { limit: number; offset: number } {
const DEFAULT_PAGE_LIMIT = 50;
const MAX_PAGE_LIMIT = 200;
function parsePagination(query: { limit?: string; offset?: string }): {
limit: number;
offset: number;
} {
return {
limit: Math.min(Math.max(Number(query.limit) || 50, 1), 200),
limit: Math.min(Math.max(Number(query.limit) || DEFAULT_PAGE_LIMIT, 1), MAX_PAGE_LIMIT),
offset: Math.max(Number(query.offset) || 0, 0),
};
}
@ -417,7 +423,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
if (name === SystemRoles.ADMIN) {
const postCount = await countUsersByRole(SystemRoles.ADMIN);
if (postCount === 0) {
await updateUser(userId, { role: SystemRoles.ADMIN });
try {
await updateUser(userId, { role: SystemRoles.ADMIN });
} catch (rollbackError) {
logger.error('[adminRoles] CRITICAL: admin rollback failed:', rollbackError);
}
return res.status(400).json({ error: 'Cannot remove the last admin user' });
}
}

View file

@ -817,6 +817,16 @@ describe('listRoles', () => {
const roles = await listRoles();
expect(roles).toEqual([]);
});
it('returns undefined description for pre-existing roles without the field', async () => {
await Role.collection.insertOne({ name: 'legacy', permissions: {} });
const roles = await listRoles();
expect(roles).toHaveLength(1);
expect(roles[0].name).toBe('legacy');
expect(roles[0].description).toBeUndefined();
});
});
describe('countRoles', () => {

View file

@ -62,13 +62,12 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
const Role = mongoose.models.Role;
const limit = options?.limit ?? 50;
const offset = options?.offset ?? 0;
const results = await Role.find({})
return await Role.find({})
.select('name description')
.sort({ name: 1 })
.skip(offset)
.limit(limit)
.lean();
return results as unknown[] as IRole[];
.lean<IRole[]>();
}
async function countRoles(): Promise<number> {
@ -137,7 +136,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
const targetName = updates.name ?? roleName;
throw new RoleConflictError(`Role "${targetName}" already exists`);
}
throw new Error(`Failed to update role: ${(error as Error).message}`);
throw new Error(`Failed to update role: ${(error as Error).message}`, { cause: error });
}
}