From b9f08e5696785c5ecf7c1dc0a78dd778adb8bb2c Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 27 Mar 2026 07:46:59 -0700 Subject: [PATCH] fix: stale cache on rename, extract renameRole helper, shared pagination, cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- packages/api/src/admin/groups.ts | 7 +- packages/api/src/admin/pagination.ts | 12 +++ packages/api/src/admin/roles.spec.ts | 6 +- packages/api/src/admin/roles.ts | 82 ++++++++++--------- .../src/methods/role.methods.spec.ts | 38 ++++++++- packages/data-schemas/src/methods/role.ts | 39 +++++---- 6 files changed, 117 insertions(+), 67 deletions(-) create mode 100644 packages/api/src/admin/pagination.ts diff --git a/packages/api/src/admin/groups.ts b/packages/api/src/admin/groups.ts index 58ff4d9782..ab4490e05f 100644 --- a/packages/api/src/admin/groups.ts +++ b/packages/api/src/admin/groups.ts @@ -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; @@ -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 }); diff --git a/packages/api/src/admin/pagination.ts b/packages/api/src/admin/pagination.ts new file mode 100644 index 0000000000..836ee9e700 --- /dev/null +++ b/packages/api/src/admin/pagination.ts @@ -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), + }; +} diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 125c2a3723..da0ff18a63 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -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 () => { diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 76b51eacb0..6b704041ac 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -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, + ): Promise { + await updateUsersByRole(currentName, newName); + try { + const updates: Partial = { 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; diff --git a/packages/data-schemas/src/methods/role.methods.spec.ts b/packages/data-schemas/src/methods/role.methods.spec.ts index a9468994e0..f8a66bef5d 100644 --- a/packages/data-schemas/src/methods/role.methods.spec.ts +++ b/packages/data-schemas/src/methods/role.methods.spec.ts @@ -30,6 +30,7 @@ let deleteRoleByName: ReturnType['deleteRoleByName']; let updateUsersByRole: ReturnType['updateUsersByRole']; let listUsersByRole: ReturnType['listUsersByRole']; let countUsersByRole: ReturnType['countUsersByRole']; +let updateRoleByName: ReturnType['updateRoleByName']; let listRoles: ReturnType['listRoles']; let countRoles: ReturnType['countRoles']; let mongoServer: MongoMemoryServer; @@ -47,6 +48,7 @@ beforeAll(async () => { initializeRoles = methods.initializeRoles; createRoleByName = methods.createRoleByName; deleteRoleByName = methods.deleteRoleByName; + updateRoleByName = methods.updateRoleByName; updateUsersByRole = methods.updateUsersByRole; listUsersByRole = methods.listUsersByRole; countUsersByRole = methods.countUsersByRole; @@ -630,12 +632,42 @@ describe('deleteRoleByName', () => { expect(mockCache.set).toHaveBeenCalledWith('editor', null); }); - it('does not touch cache when role does not exist', async () => { + it('returns null and invalidates cache when role does not exist', async () => { mockCache.set.mockClear(); - await deleteRoleByName('nonexistent'); + const result = await deleteRoleByName('nonexistent'); - expect(mockCache.set).not.toHaveBeenCalled(); + expect(result).toBeNull(); + expect(mockCache.set).toHaveBeenCalledWith('nonexistent', null); + }); +}); + +describe('updateRoleByName - cache on rename', () => { + it('invalidates old key and populates new key on rename', async () => { + await createRoleByName({ name: 'editor', description: 'Can edit' }); + mockCache.set.mockClear(); + + const updated = await updateRoleByName('editor', { name: 'senior-editor' }); + + expect(updated.name).toBe('senior-editor'); + expect(mockCache.set).toHaveBeenCalledWith('editor', null); + expect(mockCache.set).toHaveBeenCalledWith( + 'senior-editor', + expect.objectContaining({ name: 'senior-editor' }), + ); + }); + + it('writes same key when name unchanged', async () => { + await createRoleByName({ name: 'editor' }); + mockCache.set.mockClear(); + + await updateRoleByName('editor', { description: 'Updated desc' }); + + expect(mockCache.set).toHaveBeenCalledWith( + 'editor', + expect.objectContaining({ name: 'editor', description: 'Updated desc' }), + ); + expect(mockCache.set).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index b9365e0e0e..7ac3778739 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -128,7 +128,11 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol .lean() .exec(); if (cache) { - await cache.set(roleName, role); + if (updates.name && updates.name !== roleName) { + await Promise.all([cache.set(roleName, null), cache.set(updates.name, role)]); + } else { + await cache.set(roleName, role); + } } return role as unknown as IRole; } catch (error) { @@ -375,20 +379,18 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol if (!name || typeof name !== 'string' || !name.trim()) { throw new Error('Role name is required'); } - if (SystemRoles[name.trim() as keyof typeof SystemRoles]) { + const trimmed = name.trim(); + if (SystemRoles[trimmed as keyof typeof SystemRoles]) { throw new RoleConflictError(`Cannot create role with reserved system name: ${name}`); } const Role = mongoose.models.Role; - const existing = await Role.findOne({ name: name.trim() }).lean(); + const existing = await Role.findOne({ name: trimmed }).lean(); if (existing) { - throw new RoleConflictError(`Role "${name.trim()}" already exists`); + throw new RoleConflictError(`Role "${trimmed}" already exists`); } let role; try { - role = await new Role({ - ...roleData, - name: name.trim(), - }).save(); + role = await new Role({ ...roleData, name: trimmed }).save(); } catch (err) { /** * The compound unique index `{ name: 1, tenantId: 1 }` on the role schema @@ -397,7 +399,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol * the same user-facing message as the application-level duplicate check. */ if (err && typeof err === 'object' && 'code' in err && err.code === 11000) { - throw new RoleConflictError(`Role "${name.trim()}" already exists`); + throw new RoleConflictError(`Role "${trimmed}" already exists`); } throw err; } @@ -412,19 +414,14 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol return role.toObject() as IRole; } - const getUserModel = () => mongoose.models.User as Model; - /** Guards against deleting system roles. Reassigns affected users back to USER. */ async function deleteRoleByName(roleName: string): Promise { if (SystemRoles[roleName as keyof typeof SystemRoles]) { 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; - } - await getUserModel().updateMany({ role: roleName }, { $set: { role: SystemRoles.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(); try { const cache = deps.getCache?.(CacheKeys.ROLES); @@ -438,17 +435,18 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } async function updateUsersByRole(oldRole: string, newRole: string): Promise { - await getUserModel().updateMany({ role: oldRole }, { $set: { role: newRole } }); + 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 getUserModel() - .find({ role: roleName }) + return await User.find({ role: roleName }) .select('_id name email avatar') .sort({ _id: 1 }) .skip(offset) @@ -457,7 +455,8 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } async function countUsersByRole(roleName: string): Promise { - return await getUserModel().countDocuments({ role: roleName }); + const User = mongoose.models.User as Model; + return await User.countDocuments({ role: roleName }); } return {