mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 22:37:20 +02:00
fix: validate permissions in create, RoleConflictError, rollback safety, cache consistency
- Add permissions type/array validation in createRoleHandler
- Introduce RoleConflictError class replacing fragile string-prefix matching
- Wrap rollback in !role null path with try/catch for correct 404 response
- Wrap deleteRoleByName cache.set in try/catch matching createRoleByName
- Narrow updateRoleHandler body type to { name?, description? }
- Add tests: non-string description in create, rollback failure logging,
permissions array rejection, description max-length assertion fix
This commit is contained in:
parent
16678c0ece
commit
b9e0fa48c6
5 changed files with 95 additions and 21 deletions
|
|
@ -6,6 +6,8 @@ import type { ServerRequest } from '~/types/http';
|
|||
import type { AdminRolesDeps } from './roles';
|
||||
import { createAdminRolesHandlers } from './roles';
|
||||
|
||||
const { RoleConflictError } = jest.requireActual('@librechat/data-schemas');
|
||||
|
||||
jest.mock('@librechat/data-schemas', () => ({
|
||||
...jest.requireActual('@librechat/data-schemas'),
|
||||
logger: { error: jest.fn() },
|
||||
|
|
@ -209,7 +211,9 @@ describe('createAdminRolesHandlers', () => {
|
|||
|
||||
it('returns 409 when role already exists', async () => {
|
||||
const deps = createDeps({
|
||||
createRoleByName: jest.fn().mockRejectedValue(new Error('Role "editor" already exists')),
|
||||
createRoleByName: jest
|
||||
.fn()
|
||||
.mockRejectedValue(new RoleConflictError('Role "editor" already exists')),
|
||||
});
|
||||
const handlers = createAdminRolesHandlers(deps);
|
||||
const { req, res, status, json } = createReqRes({ body: { name: 'editor' } });
|
||||
|
|
@ -224,7 +228,9 @@ describe('createAdminRolesHandlers', () => {
|
|||
const deps = createDeps({
|
||||
createRoleByName: jest
|
||||
.fn()
|
||||
.mockRejectedValue(new Error('Cannot create role with reserved system name: ADMIN')),
|
||||
.mockRejectedValue(
|
||||
new RoleConflictError('Cannot create role with reserved system name: ADMIN'),
|
||||
),
|
||||
});
|
||||
const handlers = createAdminRolesHandlers(deps);
|
||||
const { req, res, status, json } = createReqRes({ body: { name: 'ADMIN' } });
|
||||
|
|
@ -263,6 +269,34 @@ describe('createAdminRolesHandlers', () => {
|
|||
|
||||
expect(status).toHaveBeenCalledWith(500);
|
||||
});
|
||||
|
||||
it('returns 400 when description is not a string', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminRolesHandlers(deps);
|
||||
const { req, res, status, json } = createReqRes({
|
||||
body: { name: 'editor', description: 123 },
|
||||
});
|
||||
|
||||
await handlers.createRole(req, res);
|
||||
|
||||
expect(status).toHaveBeenCalledWith(400);
|
||||
expect(json).toHaveBeenCalledWith({ error: 'description must be a string' });
|
||||
expect(deps.createRoleByName).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns 400 when permissions is an array', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminRolesHandlers(deps);
|
||||
const { req, res, status, json } = createReqRes({
|
||||
body: { name: 'editor', permissions: [1, 2, 3] },
|
||||
});
|
||||
|
||||
await handlers.createRole(req, res);
|
||||
|
||||
expect(status).toHaveBeenCalledWith(400);
|
||||
expect(json).toHaveBeenCalledWith({ error: 'permissions must be an object' });
|
||||
expect(deps.createRoleByName).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateRole', () => {
|
||||
|
|
@ -522,11 +556,30 @@ describe('createAdminRolesHandlers', () => {
|
|||
expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor');
|
||||
});
|
||||
|
||||
it('returns 400 when description exceeds max length', async () => {
|
||||
it('logs rollback failure and still returns 500', async () => {
|
||||
const deps = createDeps({
|
||||
getRoleByName: jest.fn().mockResolvedValue(mockRole()),
|
||||
getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
|
||||
updateUsersByRole: jest
|
||||
.fn()
|
||||
.mockResolvedValueOnce(undefined)
|
||||
.mockRejectedValueOnce(new Error('rollback failed')),
|
||||
updateRoleByName: jest.fn().mockRejectedValue(new Error('rename 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.updateUsersByRole).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('returns 400 when description exceeds max length', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminRolesHandlers(deps);
|
||||
const { req, res, status, json } = createReqRes({
|
||||
params: { name: 'editor' },
|
||||
body: { description: 'a'.repeat(2001) },
|
||||
|
|
@ -538,7 +591,7 @@ describe('createAdminRolesHandlers', () => {
|
|||
expect(json).toHaveBeenCalledWith({
|
||||
error: 'description must not exceed 2000 characters',
|
||||
});
|
||||
expect(deps.updateRoleByName).not.toHaveBeenCalled();
|
||||
expect(deps.getRoleByName).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns 500 on unexpected error', async () => {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { SystemRoles } from 'librechat-data-provider';
|
||||
import { logger, isValidObjectIdString } from '@librechat/data-schemas';
|
||||
import { logger, isValidObjectIdString, RoleConflictError } from '@librechat/data-schemas';
|
||||
import type { IRole, IUser, AdminMember } from '@librechat/data-schemas';
|
||||
import type { FilterQuery } from 'mongoose';
|
||||
import type { Response } from 'express';
|
||||
|
|
@ -102,6 +102,12 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
|
|||
.status(400)
|
||||
.json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` });
|
||||
}
|
||||
if (
|
||||
permissions !== undefined &&
|
||||
(typeof permissions !== 'object' || Array.isArray(permissions))
|
||||
) {
|
||||
return res.status(400).json({ error: 'permissions must be an object' });
|
||||
}
|
||||
const role = await createRoleByName({
|
||||
name: name.trim(),
|
||||
description: description ?? '',
|
||||
|
|
@ -110,18 +116,16 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
|
|||
return res.status(201).json({ role });
|
||||
} catch (error) {
|
||||
logger.error('[adminRoles] createRole error:', error);
|
||||
const is409 =
|
||||
error instanceof Error &&
|
||||
(error.message.startsWith('Role "') ||
|
||||
error.message.startsWith('Cannot create role with reserved'));
|
||||
const message = is409 && error instanceof Error ? error.message : 'Failed to create role';
|
||||
return res.status(is409 ? 409 : 500).json({ error: message });
|
||||
if (error instanceof RoleConflictError) {
|
||||
return res.status(409).json({ error: error.message });
|
||||
}
|
||||
return res.status(500).json({ error: 'Failed to create role' });
|
||||
}
|
||||
}
|
||||
|
||||
async function updateRoleHandler(req: ServerRequest, res: Response) {
|
||||
const { name } = req.params as RoleNameParams;
|
||||
const body = req.body as Partial<IRole>;
|
||||
const body = req.body as { name?: string; description?: string };
|
||||
let isRename = false;
|
||||
let trimmedName = '';
|
||||
let migrationRan = false;
|
||||
|
|
@ -188,7 +192,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
|
|||
const role = await updateRoleByName(name, updates);
|
||||
if (!role) {
|
||||
if (migrationRan) {
|
||||
await updateUsersByRole(trimmedName, name);
|
||||
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' });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ export * from './utils';
|
|||
export { createModels } from './models';
|
||||
export {
|
||||
createMethods,
|
||||
RoleConflictError,
|
||||
DEFAULT_REFRESH_TOKEN_EXPIRY,
|
||||
DEFAULT_SESSION_EXPIRY,
|
||||
tokenValues,
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { createSessionMethods, DEFAULT_REFRESH_TOKEN_EXPIRY, type SessionMethods } from './session';
|
||||
import { createTokenMethods, type TokenMethods } from './token';
|
||||
import { createRoleMethods, type RoleMethods, type RoleDeps } from './role';
|
||||
import { createRoleMethods, RoleConflictError, type RoleMethods, type RoleDeps } from './role';
|
||||
export { RoleConflictError };
|
||||
import { createUserMethods, DEFAULT_SESSION_EXPIRY, type UserMethods } from './user';
|
||||
|
||||
export { DEFAULT_REFRESH_TOKEN_EXPIRY, DEFAULT_SESSION_EXPIRY };
|
||||
|
|
|
|||
|
|
@ -9,6 +9,13 @@ import type { Model } from 'mongoose';
|
|||
import type { IRole, IUser } from '~/types';
|
||||
import logger from '~/config/winston';
|
||||
|
||||
export class RoleConflictError extends Error {
|
||||
constructor(message: string) {
|
||||
super(message);
|
||||
this.name = 'RoleConflictError';
|
||||
}
|
||||
}
|
||||
|
||||
export interface RoleDeps {
|
||||
/** Returns a cache store for the given key. Injected from getLogStores. */
|
||||
getCache?: (key: string) => {
|
||||
|
|
@ -350,12 +357,12 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
|
|||
throw new Error('Role name is required');
|
||||
}
|
||||
if (SystemRoles[name.trim() as keyof typeof SystemRoles]) {
|
||||
throw new Error(`Cannot create role with reserved system name: ${name}`);
|
||||
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();
|
||||
if (existing) {
|
||||
throw new Error(`Role "${name.trim()}" already exists`);
|
||||
throw new RoleConflictError(`Role "${name.trim()}" already exists`);
|
||||
}
|
||||
let role;
|
||||
try {
|
||||
|
|
@ -371,7 +378,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 Error(`Role "${name.trim()}" already exists`);
|
||||
throw new RoleConflictError(`Role "${name.trim()}" already exists`);
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
|
|
@ -400,9 +407,13 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
|
|||
}
|
||||
await getUserModel().updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } });
|
||||
const deleted = await Role.findOneAndDelete({ name: roleName }).lean();
|
||||
const cache = deps.getCache?.(CacheKeys.ROLES);
|
||||
if (cache) {
|
||||
await cache.set(roleName, null);
|
||||
try {
|
||||
const cache = deps.getCache?.(CacheKeys.ROLES);
|
||||
if (cache) {
|
||||
await cache.set(roleName, null);
|
||||
}
|
||||
} catch (cacheError) {
|
||||
logger.error(`[deleteRoleByName] cache invalidation failed for "${roleName}":`, cacheError);
|
||||
}
|
||||
return deleted as IRole | null;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue