From 7d776de71a9b7d26ecf19ae126e969f565481040 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 26 Mar 2026 15:30:33 -0700 Subject: [PATCH] fix: address external review findings for admin roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Block renaming system roles (ADMIN/USER) and add user migration on rename - Add input validation: name max-length, trim on update, duplicate name check - Replace fragile String.includes error matching with prefix-based classification - Catch MongoDB 11000 duplicate key in createRoleByName - Add pagination (limit/offset/total) to getRoleMembersHandler - Reverse delete order in deleteRoleByName — reassign users before deletion - Add role existence check in removeRoleMember; drop unused createdAt select - Add Array.isArray guard for permissions input; use consistent ?? coalescing - Fix import ordering per AGENTS.md conventions - Type-cast mongoose.models.User as Model for proper TS inference - Add comprehensive tests: rename guards, pagination, validation, 500 paths --- api/server/routes/admin/roles.js | 2 + packages/api/src/admin/roles.spec.ts | 310 ++++++++++++++++++++-- packages/api/src/admin/roles.ts | 83 ++++-- packages/data-schemas/src/methods/role.ts | 58 ++-- 4 files changed, 403 insertions(+), 50 deletions(-) diff --git a/api/server/routes/admin/roles.js b/api/server/routes/admin/roles.js index 4dbb11bde7..8afdc5abe9 100644 --- a/api/server/routes/admin/roles.js +++ b/api/server/routes/admin/roles.js @@ -20,7 +20,9 @@ const handlers = createAdminRolesHandlers({ deleteRoleByName: db.deleteRoleByName, findUser: db.findUser, updateUser: db.updateUser, + updateUsersByRole: db.updateUsersByRole, listUsersByRole: db.listUsersByRole, + countUsersByRole: db.countUsersByRole, }); router.use(requireJwtAuth, requireAdminAccess); diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 5bf91b0e66..73b10bc858 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -1,16 +1,14 @@ import { Types } from 'mongoose'; import { SystemRoles } from 'librechat-data-provider'; import type { IRole, IUser } from '@librechat/data-schemas'; +import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; import type { AdminRolesDeps } from './roles'; -import type { Response } from 'express'; import { createAdminRolesHandlers } from './roles'; jest.mock('@librechat/data-schemas', () => ({ ...jest.requireActual('@librechat/data-schemas'), - logger: { - error: jest.fn(), - }, + logger: { error: jest.fn() }, })); const validUserId = new Types.ObjectId().toString(); @@ -65,7 +63,9 @@ function createDeps(overrides: Partial = {}): AdminRolesDeps { deleteRoleByName: jest.fn().mockResolvedValue(mockRole()), findUser: jest.fn().mockResolvedValue(null), updateUser: jest.fn().mockResolvedValue(mockUser()), + updateUsersByRole: jest.fn().mockResolvedValue(undefined), listUsersByRole: jest.fn().mockResolvedValue([]), + countUsersByRole: jest.fn().mockResolvedValue(0), ...overrides, }; } @@ -119,6 +119,19 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); }); + + it('returns 500 on error', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockRejectedValue(new Error('db down')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); + + await handlers.getRole(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: 'Failed to get role' }); + }); }); describe('createRole', () => { @@ -164,6 +177,20 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'name is required' }); }); + it('returns 400 when name exceeds max length', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + body: { name: 'a'.repeat(501) }, + }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'name must not exceed 500 characters' }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); + it('returns 409 when role already exists', async () => { const deps = createDeps({ createRoleByName: jest.fn().mockRejectedValue(new Error('Role "editor" already exists')), @@ -206,13 +233,27 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(500); expect(json).toHaveBeenCalledWith({ error: 'db crash' }); }); + + it('does not classify unrelated errors as 409', async () => { + const deps = createDeps({ + createRoleByName: jest + .fn() + .mockRejectedValue(new Error('Disk space reserved for system use')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ body: { name: 'test' } }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(500); + }); }); describe('updateRole', () => { it('updates role and returns 200', async () => { const role = mockRole({ name: 'senior-editor' }); const deps = createDeps({ - getRoleByName: jest.fn().mockResolvedValue(mockRole()), + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), updateRoleByName: jest.fn().mockResolvedValue(role), }); const handlers = createAdminRolesHandlers(deps); @@ -225,6 +266,104 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(200); expect(json).toHaveBeenCalledWith({ role }); + expect(deps.updateRoleByName).toHaveBeenCalledWith('editor', { name: 'senior-editor' }); + }); + + it('trims name before storage', async () => { + const role = mockRole({ name: 'trimmed' }); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateRoleByName: jest.fn().mockResolvedValue(role), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ + params: { name: 'editor' }, + body: { name: ' trimmed ' }, + }); + + await handlers.updateRole(req, res); + + expect(deps.updateRoleByName).toHaveBeenCalledWith('editor', { name: 'trimmed' }); + }); + + it('migrates users after rename', async () => { + const role = mockRole({ name: 'new-name' }); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateRoleByName: jest.fn().mockResolvedValue(role), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'new-name' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name'); + }); + + it('does not migrate users when name unchanged', async () => { + const role = mockRole({ description: 'updated' }); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + updateRoleByName: jest.fn().mockResolvedValue(role), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ + params: { name: 'editor' }, + body: { description: 'updated' }, + }); + + await handlers.updateRole(req, res); + + expect(deps.updateUsersByRole).not.toHaveBeenCalled(); + }); + + it('returns 403 when renaming a system role', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: SystemRoles.ADMIN }, + body: { name: 'custom-admin' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(403); + expect(json).toHaveBeenCalledWith({ error: 'Cannot rename system role' }); + expect(deps.getRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 409 when renaming to a system role name', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { name: SystemRoles.ADMIN }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(409); + expect(json).toHaveBeenCalledWith({ error: 'Cannot rename to a reserved system role name' }); + }); + + it('returns 409 when target name already exists', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'viewer' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(409); + expect(json).toHaveBeenCalledWith({ error: 'Role "viewer" already exists' }); }); it('returns 400 when name is empty string', async () => { @@ -256,18 +395,19 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'name must be a non-empty string' }); }); - it('returns 409 when renaming to a system role', async () => { + it('returns 400 when name exceeds max length', async () => { const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor' }, - body: { name: SystemRoles.ADMIN }, + body: { name: 'a'.repeat(501) }, }); await handlers.updateRole(req, res); - expect(status).toHaveBeenCalledWith(409); - expect(json).toHaveBeenCalledWith({ error: 'Cannot rename to a reserved system role name' }); + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'name must not exceed 500 characters' }); + expect(deps.getRoleByName).not.toHaveBeenCalled(); }); it('returns 404 when role not found', async () => { @@ -284,6 +424,23 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); }); + it('returns 404 when updateRoleByName returns null', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + updateRoleByName: jest.fn().mockResolvedValue(null), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { description: 'updated' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(404); + expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); + }); + it('returns 500 on unexpected error', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), @@ -335,6 +492,20 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'permissions object is required' }); }); + it('returns 400 when permissions is an array', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { permissions: [1, 2, 3] }, + }); + + await handlers.updateRolePermissions(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'permissions object is required' }); + }); + it('returns 404 when role not found', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(null) }); const handlers = createAdminRolesHandlers(deps); @@ -348,6 +519,23 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); }); + + it('returns 500 on error', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + updateAccessPermissions: jest.fn().mockRejectedValue(new Error('db down')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { permissions: { chat: { read: true } } }, + }); + + await handlers.updateRolePermissions(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: 'Failed to update role permissions' }); + }); }); describe('deleteRole', () => { @@ -385,37 +573,89 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); }); + + it('returns 500 on error', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + deleteRoleByName: jest.fn().mockRejectedValue(new Error('db down')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); + + await handlers.deleteRole(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: 'Failed to delete role' }); + }); }); describe('getRoleMembers', () => { - it('returns members with 200', async () => { + it('returns paginated members with 200', async () => { const user = mockUser(); const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), listUsersByRole: jest.fn().mockResolvedValue([user]), + countUsersByRole: jest.fn().mockResolvedValue(1), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); await handlers.getRoleMembers(req, res); - expect(deps.listUsersByRole).toHaveBeenCalledWith('editor'); + expect(deps.listUsersByRole).toHaveBeenCalledWith('editor', { limit: 50, offset: 0 }); + expect(deps.countUsersByRole).toHaveBeenCalledWith('editor'); expect(status).toHaveBeenCalledWith(200); - const members = json.mock.calls[0][0].members; - expect(members).toHaveLength(1); - expect(members[0]).toEqual({ + const response = json.mock.calls[0][0]; + expect(response.members).toHaveLength(1); + expect(response.members[0]).toEqual({ userId: validUserId, name: 'Test User', email: 'test@example.com', avatarUrl: 'https://example.com/avatar.png', }); + expect(response.total).toBe(1); + expect(response.limit).toBe(50); + expect(response.offset).toBe(0); + }); + + it('passes pagination parameters from query', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + countUsersByRole: jest.fn().mockResolvedValue(0), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ + params: { name: 'editor' }, + query: { limit: '10', offset: '20' }, + }); + + await handlers.getRoleMembers(req, res); + + expect(deps.listUsersByRole).toHaveBeenCalledWith('editor', { limit: 10, offset: 20 }); + }); + + it('clamps limit to 200', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + countUsersByRole: jest.fn().mockResolvedValue(0), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res } = createReqRes({ + params: { name: 'editor' }, + query: { limit: '999' }, + }); + + await handlers.getRoleMembers(req, res); + + expect(deps.listUsersByRole).toHaveBeenCalledWith('editor', { limit: 200, offset: 0 }); }); it('does not include joinedAt in response', async () => { - const user = mockUser({ createdAt: new Date() } as Partial); + const user = mockUser(); const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), listUsersByRole: jest.fn().mockResolvedValue([user]), + countUsersByRole: jest.fn().mockResolvedValue(1), }); const handlers = createAdminRolesHandlers(deps); const { req, res, json } = createReqRes({ params: { name: 'editor' } }); @@ -429,7 +669,7 @@ describe('createAdminRolesHandlers', () => { it('returns empty array when no members', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), - listUsersByRole: jest.fn().mockResolvedValue([]), + countUsersByRole: jest.fn().mockResolvedValue(0), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); @@ -437,7 +677,7 @@ describe('createAdminRolesHandlers', () => { await handlers.getRoleMembers(req, res); expect(status).toHaveBeenCalledWith(200); - expect(json).toHaveBeenCalledWith({ members: [] }); + expect(json).toHaveBeenCalledWith({ members: [], total: 0, limit: 50, offset: 0 }); }); it('returns 404 when role not found', async () => { @@ -450,6 +690,20 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); }); + + it('returns 500 on error', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + listUsersByRole: jest.fn().mockRejectedValue(new Error('db down')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); + + await handlers.getRoleMembers(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: 'Failed to get role members' }); + }); }); describe('addRoleMember', () => { @@ -552,6 +806,7 @@ describe('createAdminRolesHandlers', () => { describe('removeRoleMember', () => { it('removes member and returns 200', async () => { const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), findUser: jest.fn().mockResolvedValue(mockUser({ role: 'editor' })), }); const handlers = createAdminRolesHandlers(deps); @@ -580,8 +835,25 @@ describe('createAdminRolesHandlers', () => { expect(deps.findUser).not.toHaveBeenCalled(); }); + it('returns 404 when role not found', async () => { + const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(null) }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'nonexistent', userId: validUserId }, + }); + + await handlers.removeRoleMember(req, res); + + expect(status).toHaveBeenCalledWith(404); + expect(json).toHaveBeenCalledWith({ error: 'Role not found' }); + expect(deps.findUser).not.toHaveBeenCalled(); + }); + it('returns 404 when user not found', async () => { - const deps = createDeps({ findUser: jest.fn().mockResolvedValue(null) }); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + findUser: jest.fn().mockResolvedValue(null), + }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor', userId: validUserId }, @@ -595,6 +867,7 @@ describe('createAdminRolesHandlers', () => { it('returns 400 when user is not a member of the role', async () => { const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), findUser: jest.fn().mockResolvedValue(mockUser({ role: 'other-role' })), }); const handlers = createAdminRolesHandlers(deps); @@ -611,6 +884,7 @@ describe('createAdminRolesHandlers', () => { it('returns 500 on unexpected error', async () => { const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), findUser: jest.fn().mockResolvedValue(mockUser({ role: 'editor' })), updateUser: jest.fn().mockRejectedValue(new Error('timeout')), }); diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 56dedd61cc..aa0a2eb928 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -1,9 +1,11 @@ -import { logger, isValidObjectIdString } from '@librechat/data-schemas'; import { SystemRoles } from 'librechat-data-provider'; +import { logger, isValidObjectIdString } from '@librechat/data-schemas'; import type { IRole, IUser } from '@librechat/data-schemas'; -import type { ServerRequest } from '~/types/http'; import type { FilterQuery } from 'mongoose'; import type { Response } from 'express'; +import type { ServerRequest } from '~/types/http'; + +const MAX_NAME_LENGTH = 500; interface RoleNameParams { name: string; @@ -36,7 +38,12 @@ export interface AdminRolesDeps { fields?: string | string[] | null, ) => Promise; updateUser: (userId: string, data: Partial) => Promise; - listUsersByRole: (roleName: string) => Promise; + updateUsersByRole: (oldRole: string, newRole: string) => Promise; + listUsersByRole: ( + roleName: string, + options?: { limit?: number; offset?: number }, + ) => Promise; + countUsersByRole: (roleName: string) => Promise; } export function createAdminRolesHandlers(deps: AdminRolesDeps) { @@ -49,7 +56,9 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { deleteRoleByName, findUser, updateUser, + updateUsersByRole, listUsersByRole, + countUsersByRole, } = deps; async function listRolesHandler(_req: ServerRequest, res: Response) { @@ -86,17 +95,25 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { if (!name || typeof name !== 'string' || !name.trim()) { return res.status(400).json({ error: 'name is required' }); } + if (name.trim().length > MAX_NAME_LENGTH) { + return res + .status(400) + .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); + } const role = await createRoleByName({ name: name.trim(), description: description ?? '', - permissions: permissions || {}, + permissions: permissions ?? {}, }); return res.status(201).json({ role }); } catch (error) { const message = error instanceof Error ? error.message : 'Failed to create role'; logger.error('[adminRoles] createRole error:', error); - const status = message.includes('already exists') || message.includes('reserved') ? 409 : 500; - return res.status(status).json({ error: message }); + const is409 = + error instanceof Error && + (error.message.startsWith('Role "') || + error.message.startsWith('Cannot create role with reserved')); + return res.status(is409 ? 409 : 500).json({ error: message }); } } @@ -111,11 +128,19 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { ) { return res.status(400).json({ error: 'name must be a non-empty string' }); } - if ( - body.name && - body.name.trim() !== name && - SystemRoles[body.name.trim() as keyof typeof SystemRoles] - ) { + 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` }); + } + + const trimmedName = body.name?.trim(); + const isRename = trimmedName !== undefined && trimmedName !== name; + + if (isRename && SystemRoles[name as keyof typeof SystemRoles]) { + return res.status(403).json({ error: 'Cannot rename system role' }); + } + if (isRename && SystemRoles[trimmedName as keyof typeof SystemRoles]) { return res.status(409).json({ error: 'Cannot rename to a reserved system role name' }); } @@ -124,15 +149,30 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(404).json({ error: 'Role not found' }); } + if (isRename) { + const duplicate = await getRoleByName(trimmedName); + if (duplicate) { + return res.status(409).json({ error: `Role "${trimmedName}" already exists` }); + } + } + const updates: Partial = {}; - if (body.name !== undefined) { - updates.name = body.name; + if (trimmedName !== undefined) { + updates.name = trimmedName; } if (body.description !== undefined) { updates.description = body.description; } const role = await updateRoleByName(name, updates); + if (!role) { + return res.status(404).json({ error: 'Role not found' }); + } + + if (isRename) { + await updateUsersByRole(name, trimmedName); + } + return res.status(200).json({ role }); } catch (error) { logger.error('[adminRoles] updateRole error:', error); @@ -147,7 +187,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { permissions: Record>; }; - if (!permissions || typeof permissions !== 'object') { + if (!permissions || typeof permissions !== 'object' || Array.isArray(permissions)) { return res.status(400).json({ error: 'permissions object is required' }); } @@ -193,14 +233,20 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(404).json({ error: 'Role not found' }); } - const users = await listUsersByRole(name); + const limit = Math.min(Math.max(Number(req.query.limit) || 50, 1), 200); + const offset = Math.max(Number(req.query.offset) || 0, 0); + + const [users, total] = await Promise.all([ + listUsersByRole(name, { limit, offset }), + countUsersByRole(name), + ]); const members: AdminMember[] = users.map((u) => ({ userId: u._id?.toString() ?? '', name: u.name ?? u._id?.toString() ?? '', email: u.email ?? '', avatarUrl: u.avatar, })); - return res.status(200).json({ members }); + return res.status(200).json({ members, total, limit, offset }); } catch (error) { logger.error('[adminRoles] getRoleMembers error:', error); return res.status(500).json({ error: 'Failed to get role members' }); @@ -244,6 +290,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(400).json({ error: 'Invalid user ID format' }); } + const existing = await getRoleByName(name); + if (!existing) { + return res.status(404).json({ error: 'Role not found' }); + } + const user = await findUser({ _id: userId }); if (!user) { return res.status(404).json({ error: 'User not found' }); diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 2c006bbd98..53fa13b717 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -5,6 +5,7 @@ import { permissionsSchema, removeNullishValues, } from 'librechat-data-provider'; +import type { Model } from 'mongoose'; import type { IRole, IUser } from '~/types'; import logger from '~/config/winston'; @@ -356,10 +357,18 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol if (existing) { throw new Error(`Role "${name}" already exists`); } - const role = await new Role({ - ...roleData, - name: name.trim(), - }).save(); + let role; + try { + role = await new Role({ + ...roleData, + name: name.trim(), + }).save(); + } catch (err) { + if (err && typeof err === 'object' && 'code' in err && err.code === 11000) { + throw new Error(`Role "${name.trim()}" already exists`); + } + throw err; + } const cache = deps.getCache?.(CacheKeys.ROLES); if (cache) { await cache.set(role.name, role.toObject()); @@ -373,23 +382,38 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol throw new Error(`Cannot delete system role: ${roleName}`); } const Role = mongoose.models.Role; - const User = mongoose.models.User; + const User = mongoose.models.User as Model; + await User.updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); const deleted = await Role.findOneAndDelete({ name: roleName }).lean(); - if (deleted) { - await User.updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); - const cache = deps.getCache?.(CacheKeys.ROLES); - if (cache) { - await cache.set(roleName, null); - } + const cache = deps.getCache?.(CacheKeys.ROLES); + if (cache) { + await cache.set(roleName, null); } return deleted as IRole | null; } - async function listUsersByRole(roleName: string): Promise { - const User = mongoose.models.User; - return (await User.find({ role: roleName }) - .select('_id name email avatar createdAt') - .lean()) as IUser[]; + async function updateUsersByRole(oldRole: string, newRole: string): Promise { + const User = mongoose.models.User as Model; + await User.updateMany({ role: oldRole }, { $set: { role: newRole } }); + } + + async function listUsersByRole( + roleName: string, + options?: { limit?: number; offset?: number }, + ): Promise { + const User = mongoose.models.User as Model; + const limit = options?.limit ?? 50; + const offset = options?.offset ?? 0; + return await User.find({ role: roleName }) + .select('_id name email avatar') + .skip(offset) + .limit(limit) + .lean(); + } + + async function countUsersByRole(roleName: string): Promise { + const User = mongoose.models.User as Model; + return User.countDocuments({ role: roleName }); } return { @@ -401,7 +425,9 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol migrateRoleSchema, createRoleByName, deleteRoleByName, + updateUsersByRole, listUsersByRole, + countUsersByRole, }; }