diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 73b10bc858..309f9e84b4 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -286,11 +286,19 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateRoleByName).toHaveBeenCalledWith('editor', { name: 'trimmed' }); }); - it('migrates users after rename', async () => { + it('migrates users before renaming role', async () => { const role = mockRole({ name: 'new-name' }); + const callOrder: string[] = []; const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), - updateRoleByName: jest.fn().mockResolvedValue(role), + updateUsersByRole: jest.fn().mockImplementation(() => { + callOrder.push('updateUsersByRole'); + return Promise.resolve(); + }), + updateRoleByName: jest.fn().mockImplementation(() => { + callOrder.push('updateRoleByName'); + return Promise.resolve(role); + }), }); const handlers = createAdminRolesHandlers(deps); const { req, res, status } = createReqRes({ @@ -302,6 +310,24 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(200); expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name'); + expect(callOrder).toEqual(['updateUsersByRole', 'updateRoleByName']); + }); + + it('does not rename role when user migration fails', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateUsersByRole: jest.fn().mockRejectedValue(new Error('migration failed')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'new-name' }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(500); + expect(deps.updateRoleByName).not.toHaveBeenCalled(); }); it('does not migrate users when name unchanged', async () => { diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index aa0a2eb928..9efef1a586 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -1,6 +1,6 @@ import { SystemRoles } from 'librechat-data-provider'; import { logger, isValidObjectIdString } from '@librechat/data-schemas'; -import type { IRole, IUser } from '@librechat/data-schemas'; +import type { IRole, IUser, AdminMember } from '@librechat/data-schemas'; import type { FilterQuery } from 'mongoose'; import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; @@ -15,13 +15,6 @@ interface RoleMemberParams extends RoleNameParams { userId: string; } -interface AdminMember { - userId: string; - name: string; - email: string; - avatarUrl?: string; -} - export interface AdminRolesDeps { listRoles: () => Promise; getRoleByName: (name: string, fields?: string | string[] | null) => Promise; @@ -164,15 +157,15 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { updates.description = body.description; } + if (isRename) { + await updateUsersByRole(name, trimmedName); + } + 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); diff --git a/packages/data-schemas/src/methods/role.methods.spec.ts b/packages/data-schemas/src/methods/role.methods.spec.ts index 70341a834e..605c25abc9 100644 --- a/packages/data-schemas/src/methods/role.methods.spec.ts +++ b/packages/data-schemas/src/methods/role.methods.spec.ts @@ -27,7 +27,9 @@ let updateAccessPermissions: ReturnType['updateAccessP let initializeRoles: ReturnType['initializeRoles']; let createRoleByName: ReturnType['createRoleByName']; let deleteRoleByName: ReturnType['deleteRoleByName']; +let updateUsersByRole: ReturnType['updateUsersByRole']; let listUsersByRole: ReturnType['listUsersByRole']; +let countUsersByRole: ReturnType['countUsersByRole']; let mongoServer: MongoMemoryServer; beforeAll(async () => { @@ -43,7 +45,9 @@ beforeAll(async () => { initializeRoles = methods.initializeRoles; createRoleByName = methods.createRoleByName; deleteRoleByName = methods.deleteRoleByName; + updateUsersByRole = methods.updateUsersByRole; listUsersByRole = methods.listUsersByRole; + countUsersByRole = methods.countUsersByRole; }); afterAll(async () => { @@ -670,3 +674,53 @@ describe('listUsersByRole', () => { expect('username' in users[0]).toBe(false); }); }); + +describe('updateUsersByRole', () => { + it('migrates all users from one role to another', async () => { + await User.create([ + { name: 'Alice', email: 'alice@test.com', role: 'editor', username: 'alice' }, + { name: 'Bob', email: 'bob@test.com', role: 'editor', username: 'bob' }, + { name: 'Carol', email: 'carol@test.com', role: SystemRoles.USER, username: 'carol' }, + ]); + + await updateUsersByRole('editor', 'senior-editor'); + + const alice = await User.findOne({ email: 'alice@test.com' }).lean(); + const bob = await User.findOne({ email: 'bob@test.com' }).lean(); + const carol = await User.findOne({ email: 'carol@test.com' }).lean(); + expect(alice!.role).toBe('senior-editor'); + expect(bob!.role).toBe('senior-editor'); + expect(carol!.role).toBe(SystemRoles.USER); + }); + + it('is a no-op when no users have the source role', async () => { + await User.create({ + name: 'Alice', + email: 'alice@test.com', + role: SystemRoles.USER, + username: 'alice', + }); + + await updateUsersByRole('nonexistent', 'new-role'); + + const alice = await User.findOne({ email: 'alice@test.com' }).lean(); + expect(alice!.role).toBe(SystemRoles.USER); + }); +}); + +describe('countUsersByRole', () => { + it('returns the count of users with the given role', async () => { + await User.create([ + { name: 'Alice', email: 'alice@test.com', role: 'editor', username: 'alice' }, + { name: 'Bob', email: 'bob@test.com', role: 'editor', username: 'bob' }, + { name: 'Carol', email: 'carol@test.com', role: SystemRoles.USER, username: 'carol' }, + ]); + + expect(await countUsersByRole('editor')).toBe(2); + expect(await countUsersByRole(SystemRoles.USER)).toBe(1); + }); + + it('returns 0 when no users have the role', async () => { + expect(await countUsersByRole('nonexistent')).toBe(0); + }); +}); diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 53fa13b717..b1aed411da 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -382,6 +382,10 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol throw new Error(`Cannot delete system role: ${roleName}`); } const Role = mongoose.models.Role; + const exists = await Role.findOne({ name: roleName }).lean(); + if (!exists) { + return null; + } const User = mongoose.models.User as Model; await User.updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); const deleted = await Role.findOneAndDelete({ name: roleName }).lean(); @@ -406,6 +410,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol const offset = options?.offset ?? 0; return await User.find({ role: roleName }) .select('_id name email avatar') + .sort({ _id: 1 }) .skip(offset) .limit(limit) .lean(); diff --git a/packages/data-schemas/src/types/admin.ts b/packages/data-schemas/src/types/admin.ts index 99915f659d..9b30cdb98a 100644 --- a/packages/data-schemas/src/types/admin.ts +++ b/packages/data-schemas/src/types/admin.ts @@ -114,7 +114,7 @@ export type AdminMember = { name: string; email: string; avatarUrl?: string; - joinedAt: string; + joinedAt?: string; }; /** Minimal user info returned by user search endpoints. */ diff --git a/packages/data-schemas/src/types/role.ts b/packages/data-schemas/src/types/role.ts index bc85284c34..ad11372dff 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;