fix: address re-review findings for admin roles

- Gate deleteRoleByName on existence check — skip user reassignment and
  cache invalidation when role doesn't exist (fixes test mismatch)
- Reverse rename order: migrate users before renaming role so a migration
  failure leaves the system in a consistent state
- Add .sort({ _id: 1 }) to listUsersByRole for deterministic pagination
- Import shared AdminMember type from data-schemas instead of local copy;
  make joinedAt optional since neither groups nor roles populate it
- Change IRole.description from optional to required to match schema default
- Add data-layer tests for updateUsersByRole and countUsersByRole
- Add handler test verifying users-first rename ordering and migration
  failure safety
This commit is contained in:
Dustin Healy 2026-03-26 15:54:14 -07:00
parent 7d776de71a
commit 94fdb3cd93
6 changed files with 94 additions and 16 deletions

View file

@ -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 () => {

View file

@ -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<IRole[]>;
getRoleByName: (name: string, fields?: string | string[] | null) => Promise<IRole | null>;
@ -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);