diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 74bdee085f..a66ad38ee4 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -703,7 +703,7 @@ describe('createAdminRolesHandlers', () => { }); const handlers = createAdminRolesHandlers(deps); const perms = { chat: { read: true, write: true } }; - const { req, res, status } = createReqRes({ + const { req, res, status, json } = createReqRes({ params: { name: 'editor' }, body: { permissions: perms }, }); @@ -712,6 +712,7 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateAccessPermissions).toHaveBeenCalledWith('editor', perms, role); expect(status).toHaveBeenCalledWith(200); + expect(json).toHaveBeenCalledWith({ role: expect.objectContaining({ name: 'editor' }) }); }); it('returns 400 when permissions is missing', async () => { @@ -945,7 +946,7 @@ describe('createAdminRolesHandlers', () => { it('adds member and returns 200', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), - findUser: jest.fn().mockResolvedValue(mockUser()), + findUser: jest.fn().mockResolvedValue(mockUser({ role: 'viewer' })), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ @@ -960,6 +961,24 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ success: true }); }); + it('skips DB write when user already has the target role', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + findUser: jest.fn().mockResolvedValue(mockUser({ role: 'editor' })), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { userId: validUserId }, + }); + + await handlers.addRoleMember(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(json).toHaveBeenCalledWith({ success: true }); + expect(deps.updateUser).not.toHaveBeenCalled(); + }); + it('returns 400 when userId is missing', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); @@ -1022,7 +1041,7 @@ describe('createAdminRolesHandlers', () => { it('returns 500 on unexpected error', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), - findUser: jest.fn().mockResolvedValue(mockUser()), + findUser: jest.fn().mockResolvedValue(mockUser({ role: 'viewer' })), updateUser: jest.fn().mockRejectedValue(new Error('timeout')), }); const handlers = createAdminRolesHandlers(deps); @@ -1153,6 +1172,30 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateUser).toHaveBeenCalledWith(validUserId, { role: SystemRoles.USER }); }); + it('rolls back removal when post-write check finds zero admins', 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), + }); + 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); + expect(deps.updateUser).toHaveBeenNthCalledWith(1, validUserId, { + role: SystemRoles.USER, + }); + expect(deps.updateUser).toHaveBeenNthCalledWith(2, validUserId, { + role: SystemRoles.ADMIN, + }); + }); + it('returns 500 on unexpected error', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 62689f9c62..9ed1582aee 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -8,6 +8,53 @@ import type { ServerRequest } from '~/types/http'; const MAX_NAME_LENGTH = 500; const MAX_DESCRIPTION_LENGTH = 2000; +function parsePagination(query: Record): { limit: number; offset: number } { + return { + limit: Math.min(Math.max(Number(query.limit) || 50, 1), 200), + offset: Math.max(Number(query.offset) || 0, 0), + }; +} + +function validateNameParam(name: string): string | null { + if (!name || typeof name !== 'string') { + return 'name parameter is required'; + } + if (name.length > MAX_NAME_LENGTH) { + return `name must not exceed ${MAX_NAME_LENGTH} characters`; + } + return null; +} + +function validateRoleName(name: unknown, required: boolean): string | null { + if (required) { + if (!name || typeof name !== 'string' || !(name as string).trim()) { + return 'name is required'; + } + } else if (name !== undefined) { + if (!name || typeof name !== 'string' || !(name as string).trim()) { + return 'name must be a non-empty string'; + } + } + if (name !== undefined && typeof name === 'string' && name.trim().length > MAX_NAME_LENGTH) { + return `name must not exceed ${MAX_NAME_LENGTH} characters`; + } + return null; +} + +function validateDescription(description: unknown): string | null { + if (description !== undefined && typeof description !== 'string') { + return 'description must be a string'; + } + if ( + description !== undefined && + typeof description === 'string' && + description.length > MAX_DESCRIPTION_LENGTH + ) { + return `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters`; + } + return null; +} + interface RoleNameParams { name: string; } @@ -59,8 +106,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function listRolesHandler(req: ServerRequest, res: Response) { try { - const limit = Math.min(Math.max(Number(req.query.limit) || 50, 1), 200); - const offset = Math.max(Number(req.query.offset) || 0, 0); + const { limit, offset } = parsePagination(req.query); const [roles, total] = await Promise.all([listRoles({ limit, offset }), countRoles()]); return res.status(200).json({ roles, total, limit, offset }); } catch (error) { @@ -72,6 +118,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function getRoleHandler(req: ServerRequest, res: Response) { try { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } const role = await getRoleByName(name); if (!role) { return res.status(404).json({ error: 'Role not found' }); @@ -90,21 +140,13 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { description?: string; permissions?: IRole['permissions']; }; - if (!name || typeof name !== 'string' || !name.trim()) { - return res.status(400).json({ error: 'name is required' }); + const nameError = validateRoleName(name, true); + if (nameError) { + return res.status(400).json({ error: nameError }); } - if (name.trim().length > MAX_NAME_LENGTH) { - return res - .status(400) - .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); - } - if (description !== undefined && typeof description !== 'string') { - return res.status(400).json({ error: 'description must be a string' }); - } - if (description && description.length > MAX_DESCRIPTION_LENGTH) { - return res - .status(400) - .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` }); + const descError = validateDescription(description); + if (descError) { + return res.status(400).json({ error: descError }); } if ( permissions !== undefined && @@ -113,7 +155,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(400).json({ error: 'permissions must be an object' }); } const role = await createRoleByName({ - name: name.trim(), + name: (name as string).trim(), description: description ?? '', permissions: permissions ?? {}, }); @@ -129,29 +171,22 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function updateRoleHandler(req: ServerRequest, res: Response) { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } const body = req.body as { name?: string; description?: string }; let isRename = false; let trimmedName = ''; let migrationRan = false; try { - if ( - body.name !== undefined && - (!body.name || typeof body.name !== 'string' || !body.name.trim()) - ) { - return res.status(400).json({ error: 'name must be a non-empty string' }); + const nameError = validateRoleName(body.name, false); + if (nameError) { + return res.status(400).json({ error: nameError }); } - if (body.name !== undefined && body.name.trim().length > MAX_NAME_LENGTH) { - return res - .status(400) - .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); - } - if (body.description !== undefined && typeof body.description !== 'string') { - return res.status(400).json({ error: 'description must be a string' }); - } - if (body.description && body.description.length > MAX_DESCRIPTION_LENGTH) { - return res - .status(400) - .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` }); + const descError = validateDescription(body.description); + if (descError) { + return res.status(400).json({ error: descError }); } trimmedName = body.name?.trim() ?? ''; @@ -214,6 +249,9 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError); } } + if (error instanceof RoleConflictError) { + return res.status(409).json({ error: error.message }); + } logger.error('[adminRoles] updateRole error:', error); return res.status(500).json({ error: 'Failed to update role' }); } @@ -222,6 +260,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function updateRolePermissionsHandler(req: ServerRequest, res: Response) { try { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } const { permissions } = req.body as { permissions: Record>; }; @@ -250,6 +292,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function deleteRoleHandler(req: ServerRequest, res: Response) { try { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } if (SystemRoles[name as keyof typeof SystemRoles]) { return res.status(403).json({ error: 'Cannot delete system role' }); } @@ -268,13 +314,16 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function getRoleMembersHandler(req: ServerRequest, res: Response) { try { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } const existing = await getRoleByName(name); if (!existing) { return res.status(404).json({ error: 'Role not found' }); } - const limit = Math.min(Math.max(Number(req.query.limit) || 50, 1), 200); - const offset = Math.max(Number(req.query.offset) || 0, 0); + const { limit, offset } = parsePagination(req.query); const [users, total] = await Promise.all([ listUsersByRole(name, { limit, offset }), @@ -296,6 +345,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function addRoleMemberHandler(req: ServerRequest, res: Response) { try { const { name } = req.params as RoleNameParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } const { userId } = req.body as { userId: string }; if (!userId || typeof userId !== 'string') { @@ -315,6 +368,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(404).json({ error: 'User not found' }); } + if (user.role === name) { + return res.status(200).json({ success: true }); + } + await updateUser(userId, { role: name }); return res.status(200).json({ success: true }); } catch (error) { @@ -326,6 +383,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { async function removeRoleMemberHandler(req: ServerRequest, res: Response) { try { const { name, userId } = req.params as RoleMemberParams; + const paramError = validateNameParam(name); + if (paramError) { + return res.status(400).json({ error: paramError }); + } if (!isValidObjectIdString(userId)) { return res.status(400).json({ error: 'Invalid user ID format' }); } @@ -352,6 +413,15 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { } await updateUser(userId, { role: SystemRoles.USER }); + + if (name === SystemRoles.ADMIN) { + const postCount = await countUsersByRole(SystemRoles.ADMIN); + if (postCount === 0) { + await updateUser(userId, { role: SystemRoles.ADMIN }); + return res.status(400).json({ error: 'Cannot remove the last admin user' }); + } + } + return res.status(200).json({ success: true }); } catch (error) { logger.error('[adminRoles] removeRoleMember error:', error); diff --git a/packages/data-schemas/src/methods/role.methods.spec.ts b/packages/data-schemas/src/methods/role.methods.spec.ts index 1c19165f6c..08f0a0262c 100644 --- a/packages/data-schemas/src/methods/role.methods.spec.ts +++ b/packages/data-schemas/src/methods/role.methods.spec.ts @@ -30,6 +30,8 @@ let deleteRoleByName: ReturnType['deleteRoleByName']; let updateUsersByRole: ReturnType['updateUsersByRole']; let listUsersByRole: ReturnType['listUsersByRole']; let countUsersByRole: ReturnType['countUsersByRole']; +let listRoles: ReturnType['listRoles']; +let countRoles: ReturnType['countRoles']; let mongoServer: MongoMemoryServer; beforeAll(async () => { @@ -48,6 +50,8 @@ beforeAll(async () => { updateUsersByRole = methods.updateUsersByRole; listUsersByRole = methods.listUsersByRole; countUsersByRole = methods.countUsersByRole; + listRoles = methods.listRoles; + countRoles = methods.countRoles; }); afterAll(async () => { @@ -745,3 +749,112 @@ describe('countUsersByRole', () => { expect(await countUsersByRole('nonexistent')).toBe(0); }); }); + +describe('listRoles', () => { + beforeEach(async () => { + await Role.deleteMany({}); + }); + + it('returns roles sorted alphabetically by name', async () => { + await Role.create([ + { name: 'zebra', permissions: {} }, + { name: 'alpha', permissions: {} }, + { name: 'middle', permissions: {} }, + ]); + + const roles = await listRoles(); + + expect(roles.map((r) => r.name)).toEqual(['alpha', 'middle', 'zebra']); + }); + + it('respects limit and offset for pagination', async () => { + await Role.create([ + { name: 'a-role', permissions: {} }, + { name: 'b-role', permissions: {} }, + { name: 'c-role', permissions: {} }, + { name: 'd-role', permissions: {} }, + { name: 'e-role', permissions: {} }, + ]); + + const page1 = await listRoles({ limit: 2, offset: 0 }); + const page2 = await listRoles({ limit: 2, offset: 2 }); + const page3 = await listRoles({ limit: 2, offset: 4 }); + + expect(page1).toHaveLength(2); + expect(page1.map((r) => r.name)).toEqual(['a-role', 'b-role']); + expect(page2).toHaveLength(2); + expect(page2.map((r) => r.name)).toEqual(['c-role', 'd-role']); + expect(page3).toHaveLength(1); + expect(page3.map((r) => r.name)).toEqual(['e-role']); + }); + + it('defaults to limit 50 and offset 0', async () => { + await Role.create({ name: 'only-role', permissions: {} }); + + const roles = await listRoles(); + + expect(roles).toHaveLength(1); + expect(roles[0].name).toBe('only-role'); + }); + + it('returns only name and description fields', async () => { + await Role.create({ + name: 'editor', + description: 'Can edit', + permissions: { PROMPTS: { USE: true } }, + }); + + const roles = await listRoles(); + + expect(roles).toHaveLength(1); + expect(roles[0].name).toBe('editor'); + expect(roles[0].description).toBe('Can edit'); + expect(roles[0]._id).toBeDefined(); + expect('permissions' in roles[0]).toBe(false); + }); + + it('returns empty array when no roles exist', async () => { + const roles = await listRoles(); + expect(roles).toEqual([]); + }); +}); + +describe('countRoles', () => { + beforeEach(async () => { + await Role.deleteMany({}); + }); + + it('returns the total number of roles', async () => { + await Role.create([ + { name: 'a', permissions: {} }, + { name: 'b', permissions: {} }, + { name: 'c', permissions: {} }, + ]); + + expect(await countRoles()).toBe(3); + }); + + it('returns 0 when no roles exist', async () => { + expect(await countRoles()).toBe(0); + }); +}); + +describe('createRoleByName - duplicate key race', () => { + beforeEach(async () => { + await Role.deleteMany({}); + }); + + it('throws RoleConflictError on concurrent insert (11000)', async () => { + await createRoleByName({ name: 'editor' }); + + const insertSpy = jest.spyOn(Role.prototype, 'save').mockImplementationOnce(() => { + const err = new Error('E11000 duplicate key error') as Error & { code: number }; + err.code = 11000; + throw err; + }); + + await expect(createRoleByName({ name: 'editor2' })).rejects.toThrow(/already exists/); + + insertSpy.mockRestore(); + }); +}); diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 07f53b97b3..69e88d347d 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -38,8 +38,11 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol const defaultPerms = roleDefaults[roleName].permissions; if (!role) { - role = new Role(roleDefaults[roleName]); + role = new Role({ ...roleDefaults[roleName], description: '' }); } else { + if (role.description == null) { + role.description = ''; + } const permissions = role.toObject()?.permissions ?? {}; role.permissions = role.permissions || {}; for (const permType of Object.keys(defaultPerms)) { @@ -59,12 +62,13 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol const Role = mongoose.models.Role; const limit = options?.limit ?? 50; const offset = options?.offset ?? 0; - return await Role.find({}) + const results = await Role.find({}) .select('name description') .sort({ name: 1 }) .skip(offset) .limit(limit) .lean(); + return results as unknown[] as IRole[]; } async function countRoles(): Promise { @@ -129,6 +133,10 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } return role as unknown as IRole; } catch (error) { + if (error && typeof error === 'object' && 'code' in error && error.code === 11000) { + const targetName = updates.name ?? roleName; + throw new RoleConflictError(`Role "${targetName}" already exists`); + } throw new Error(`Failed to update role: ${(error as Error).message}`); } } diff --git a/packages/data-schemas/src/types/role.ts b/packages/data-schemas/src/types/role.ts index ad11372dff..bc85284c34 100644 --- a/packages/data-schemas/src/types/role.ts +++ b/packages/data-schemas/src/types/role.ts @@ -5,7 +5,7 @@ import { CursorPaginationParams } from '~/common'; export interface IRole extends Document { name: string; - description: string; + description?: string; permissions: { [PermissionTypes.BOOKMARKS]?: { [Permissions.USE]?: boolean;