fix: stale cache on rename, extract renameRole helper, shared pagination, cleanup

- Fix updateRoleByName cache bug: invalidate old key and populate new key
  when updates.name differs from roleName (prevents stale cache after rename)
- Extract renameRole helper to eliminate mutable outer-scope state flags
  (isRename, trimmedName, migrationRan) in updateRoleHandler
- Unify system-role protection to 403 for both rename-from and rename-to
- Extract parsePagination to shared admin/pagination.ts; use in both
  roles.ts and groups.ts
- Extract name.trim() to local const in createRoleByName (was called 5×)
- Remove redundant findOne pre-check in deleteRoleByName
- Replace getUserModel closure with local const declarations
- Remove redundant description ?? '' in createRoleHandler (schema default)
- Add doc comment on updateRolePermissionsHandler noting cache dependency
- Add data-layer tests for cache rename behavior (old key null, new key set)
This commit is contained in:
Dustin Healy 2026-03-27 07:46:59 -07:00
parent 153edf6002
commit b9f08e5696
6 changed files with 117 additions and 67 deletions

View file

@ -13,6 +13,7 @@ import type { FilterQuery, ClientSession, DeleteResult } from 'mongoose';
import type { Response } from 'express';
import type { ValidationError } from '~/types/error';
import type { ServerRequest } from '~/types/http';
import { parsePagination } from './pagination';
type GroupListFilter = Pick<GroupFilterOptions, 'source' | 'search'>;
@ -119,8 +120,7 @@ export function createAdminGroupsHandlers(deps: AdminGroupsDeps) {
if (search) {
filter.search = search;
}
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 [groups, total] = await Promise.all([
listGroups({ ...filter, limit, offset }),
countGroups(filter),
@ -348,8 +348,7 @@ export function createAdminGroupsHandlers(deps: AdminGroupsDeps) {
*/
const allMemberIds = [...new Set(group.memberIds || [])];
const total = allMemberIds.length;
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);
if (total === 0 || offset >= total) {
return res.status(200).json({ members: [], total, limit, offset });

View file

@ -0,0 +1,12 @@
export const DEFAULT_PAGE_LIMIT = 50;
export const MAX_PAGE_LIMIT = 200;
export function parsePagination(query: { limit?: string; offset?: string }): {
limit: number;
offset: number;
} {
return {
limit: Math.min(Math.max(Number(query.limit) || DEFAULT_PAGE_LIMIT, 1), MAX_PAGE_LIMIT),
offset: Math.max(Number(query.offset) || 0, 0),
};
}

View file

@ -443,7 +443,7 @@ describe('createAdminRolesHandlers', () => {
expect(deps.getRoleByName).not.toHaveBeenCalled();
});
it('returns 409 when renaming to a system role name', async () => {
it('returns 403 when renaming to a system role name', async () => {
const deps = createDeps();
const handlers = createAdminRolesHandlers(deps);
const { req, res, status, json } = createReqRes({
@ -453,8 +453,8 @@ describe('createAdminRolesHandlers', () => {
await handlers.updateRole(req, res);
expect(status).toHaveBeenCalledWith(409);
expect(json).toHaveBeenCalledWith({ error: 'Cannot rename to a reserved system role name' });
expect(status).toHaveBeenCalledWith(403);
expect(json).toHaveBeenCalledWith({ error: 'Cannot use a reserved system role name' });
});
it('returns 409 when target name already exists', async () => {

View file

@ -4,23 +4,11 @@ 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';
import { parsePagination } from './pagination';
const MAX_NAME_LENGTH = 500;
const MAX_DESCRIPTION_LENGTH = 2000;
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) || DEFAULT_PAGE_LIMIT, 1), MAX_PAGE_LIMIT),
offset: Math.max(Number(query.offset) || 0, 0),
};
}
function validateNameParam(name: string): string | null {
if (!name || typeof name !== 'string') {
return 'name parameter is required';
@ -162,7 +150,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
}
const role = await createRoleByName({
name: (name as string).trim(),
description: description ?? '',
description,
permissions: permissions ?? {},
});
return res.status(201).json({ role });
@ -175,6 +163,33 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
}
}
async function renameRole(
currentName: string,
newName: string,
extraUpdates?: Partial<IRole>,
): Promise<IRole | null> {
await updateUsersByRole(currentName, newName);
try {
const updates: Partial<IRole> = { name: newName, ...extraUpdates };
const role = await updateRoleByName(currentName, updates);
if (!role) {
try {
await updateUsersByRole(newName, currentName);
} catch (rollbackError) {
logger.error('[adminRoles] rollback failed (role not found path):', rollbackError);
}
}
return role;
} catch (error) {
try {
await updateUsersByRole(newName, currentName);
} catch (rollbackError) {
logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError);
}
throw error;
}
}
async function updateRoleHandler(req: ServerRequest, res: Response) {
const { name } = req.params as RoleNameParams;
const paramError = validateNameParam(name);
@ -182,9 +197,6 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
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 {
const nameError = validateRoleName(body.name, false);
if (nameError) {
@ -195,14 +207,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
return res.status(400).json({ error: descError });
}
trimmedName = body.name?.trim() ?? '';
isRename = trimmedName !== '' && trimmedName !== name;
const trimmedName = body.name?.trim() ?? '';
const isRename = trimmedName !== '' && 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' });
return res.status(403).json({ error: 'Cannot use a reserved system role name' });
}
const existing = await getRoleByName(name);
@ -230,31 +242,21 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
}
if (isRename) {
await updateUsersByRole(name, trimmedName);
migrationRan = true;
const descUpdate =
body.description !== undefined ? { description: body.description } : undefined;
const role = await renameRole(name, trimmedName, descUpdate);
if (!role) {
return res.status(404).json({ error: 'Role not found' });
}
return res.status(200).json({ role });
}
const role = await updateRoleByName(name, updates);
if (!role) {
if (migrationRan) {
try {
await updateUsersByRole(trimmedName, name);
} catch (rollbackError) {
logger.error('[adminRoles] rollback failed (role not found path):', rollbackError);
}
}
return res.status(404).json({ error: 'Role not found' });
}
return res.status(200).json({ role });
} catch (error) {
if (migrationRan) {
try {
await updateUsersByRole(trimmedName, name);
} catch (rollbackError) {
logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError);
}
}
if (error instanceof RoleConflictError) {
return res.status(409).json({ error: error.message });
}
@ -263,6 +265,12 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
}
}
/**
* The re-fetch via `getRoleByName` after `updateAccessPermissions` depends on the
* callee having written the updated document to the role cache. If the cache layer
* is refactored to stop writing from within `updateAccessPermissions`, this handler
* must be updated to perform an explicit uncached DB read.
*/
async function updateRolePermissionsHandler(req: ServerRequest, res: Response) {
try {
const { name } = req.params as RoleNameParams;