From 15472127d64df96e761f0651378d78477fd55335 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 18 Jun 2026 11:32:27 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20feat:=20Audit=20grant?= =?UTF-8?q?=20removals=20from=20the=20role-deletion=20cascade?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the forensic gap Codex flagged (R2-6/R3-4): deleting a role removed its SystemGrants with no audit entries. `deleteGrantsForPrincipal` now returns the removed grants, and the role-deletion handler emits a `grant.removed` audit entry per removed grant (actor = caller, target = role, metadata.capability, request context), matching the explicit revoke endpoint. Fail-open — the role is already deleted, so a failed audit is logged, not propagated; sequential to keep the per-tenant hash chain ordered. Extracted `buildAuditContext` to admin/context.ts (shared by grants + roles). Tests: role-deletion emits one entry per grant / none when no grants; ds 110, api admin 202 green. --- api/server/routes/admin/roles.js | 1 + packages/api/src/admin/context.ts | 21 +++++++ packages/api/src/admin/grants.ts | 20 +----- packages/api/src/admin/roles.spec.ts | 47 +++++++++++++- packages/api/src/admin/roles.ts | 61 +++++++++++++++++-- .../data-schemas/src/methods/systemGrant.ts | 8 ++- 6 files changed, 132 insertions(+), 26 deletions(-) create mode 100644 packages/api/src/admin/context.ts diff --git a/api/server/routes/admin/roles.js b/api/server/routes/admin/roles.js index f2bbd7f7ea..5c6d4e92e8 100644 --- a/api/server/routes/admin/roles.js +++ b/api/server/routes/admin/roles.js @@ -29,6 +29,7 @@ const handlers = createAdminRolesHandlers({ deleteConfig: db.deleteConfig, deleteAclEntries: db.deleteAclEntries, deleteGrantsForPrincipal: db.deleteGrantsForPrincipal, + recordAuditEntry: db.recordAuditEntry, }); router.use(requireJwtAuth, requireAdminAccess); diff --git a/packages/api/src/admin/context.ts b/packages/api/src/admin/context.ts new file mode 100644 index 0000000000..2ab6710753 --- /dev/null +++ b/packages/api/src/admin/context.ts @@ -0,0 +1,21 @@ +import type { AuditContext } from '@librechat/data-schemas'; +import type { ServerRequest } from '~/types/http'; + +/** Normalizes a possibly-repeated header to its first string value. */ +function firstHeaderValue(value: string | string[] | undefined): string | undefined { + if (Array.isArray(value)) return value[0]; + return typeof value === 'string' ? value : undefined; +} + +/** Extracts forensic request context (IP, user agent, correlation id) for an + * audit record. Fields are undefined when the data isn't available. */ +export function buildAuditContext(req: ServerRequest): AuditContext { + const headers = req.headers ?? {}; + const forwarded = firstHeaderValue(headers['x-forwarded-for']); + const forwardedIp = forwarded ? forwarded.split(',')[0]?.trim() : undefined; + return { + ip: req.ip || forwardedIp || req.socket?.remoteAddress || undefined, + userAgent: firstHeaderValue(headers['user-agent']), + requestId: firstHeaderValue(headers['x-request-id'] ?? headers['x-correlation-id']), + }; +} diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index a71dd638c6..bba4495cfb 100644 --- a/packages/api/src/admin/grants.ts +++ b/packages/api/src/admin/grants.ts @@ -18,6 +18,7 @@ import type { Types } from 'mongoose'; import type { ResolvedPrincipal } from '~/types/principal'; import type { ServerRequest } from '~/types/http'; import { parsePagination } from './pagination'; +import { buildAuditContext } from './context'; interface GrantRequestBody { principalType?: string; @@ -94,25 +95,6 @@ export interface AdminGrantsDeps { auditFailClosed?: boolean; } -/** Normalizes a possibly-repeated header to its first string value. */ -function firstHeaderValue(value: string | string[] | undefined): string | undefined { - if (Array.isArray(value)) return value[0]; - return typeof value === 'string' ? value : undefined; -} - -/** Extracts forensic request context (IP, user agent, correlation id) for the - * audit record. Fields are undefined when the data isn't available. */ -function buildAuditContext(req: ServerRequest): AuditContext { - const headers = req.headers ?? {}; - const forwarded = firstHeaderValue(headers['x-forwarded-for']); - const forwardedIp = forwarded ? forwarded.split(',')[0]?.trim() : undefined; - return { - ip: req.ip || forwardedIp || req.socket?.remoteAddress || undefined, - userAgent: firstHeaderValue(headers['user-agent']), - requestId: firstHeaderValue(headers['x-request-id'] ?? headers['x-correlation-id']), - }; -} - /** Currently ROLE-only; Record/Set structure preserved for future principal-type expansion. */ export type GrantPrincipalType = PrincipalType.ROLE; diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index edbd14ba0b..deae5c23c4 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -75,7 +75,7 @@ function createDeps(overrides: Partial = {}): AdminRolesDeps { countUsersByRole: jest.fn().mockResolvedValue(0), deleteConfig: jest.fn().mockResolvedValue(null), deleteAclEntries: jest.fn().mockResolvedValue(undefined), - deleteGrantsForPrincipal: jest.fn().mockResolvedValue(undefined), + deleteGrantsForPrincipal: jest.fn().mockResolvedValue([]), ...overrides, }; } @@ -990,6 +990,51 @@ describe('createAdminRolesHandlers', () => { expect(json).toHaveBeenCalledWith({ success: true }); }); + it('emits a grant.removed audit entry for each grant removed by the cascade', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); + const deps = createDeps({ + deleteGrantsForPrincipal: jest + .fn() + .mockResolvedValue([{ capability: 'manage:users' }, { capability: 'read:users' }]), + recordAuditEntry, + }); + const handlers = createAdminRolesHandlers(deps); + const userId = new Types.ObjectId(); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + user: { _id: userId, role: 'admin', tenantId: 'tenant-1' }, + }); + + await handlers.deleteRole(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(recordAuditEntry).toHaveBeenCalledTimes(2); + expect(recordAuditEntry).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'grant.removed', + actor: expect.objectContaining({ type: 'user', id: userId.toString() }), + target: { type: PrincipalType.ROLE, id: 'editor', name: 'editor' }, + metadata: { capability: 'manage:users' }, + tenantId: 'tenant-1', + }), + ); + }); + + it('does not audit when the deleted role had no grants', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); + const deps = createDeps({ recordAuditEntry }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + user: { _id: new Types.ObjectId(), role: 'admin' }, + }); + + await handlers.deleteRole(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(recordAuditEntry).not.toHaveBeenCalled(); + }); + it('succeeds even when all cascade operations fail', async () => { const deps = createDeps({ deleteConfig: jest.fn().mockRejectedValue(new Error('config cleanup failed')), diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 910e962a5c..4f15658f6e 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -1,10 +1,19 @@ import { PrincipalType, SystemRoles } from 'librechat-data-provider'; import { logger, isValidObjectIdString, RoleConflictError } from '@librechat/data-schemas'; -import type { IRole, IUser, IConfig, AdminMember } from '@librechat/data-schemas'; +import type { + IRole, + IUser, + IConfig, + AdminMember, + ISystemGrant, + RecordAuditEntryInput, + RecordAuditEntryOptions, +} from '@librechat/data-schemas'; import type { FilterQuery, Types } from 'mongoose'; import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; import { parsePagination } from './pagination'; +import { buildAuditContext } from './context'; const systemRoleValues = new Set(Object.values(SystemRoles)); @@ -115,11 +124,18 @@ export interface AdminRolesDeps { principalType: PrincipalType; principalId: string | Types.ObjectId; }) => Promise; - /** Removes all system capability grants held by this principal. */ + /** Removes all system capability grants held by this principal and returns + * the removed grants so each can be audited. */ deleteGrantsForPrincipal: ( principalType: PrincipalType, principalId: string | Types.ObjectId, options?: { tenantId?: string }, + ) => Promise; + /** Optional audit emission for grants removed by the role-deletion cascade. + * Failure is logged but never blocks the deletion (which has already happened). */ + recordAuditEntry?: ( + input: RecordAuditEntryInput, + options?: RecordAuditEntryOptions, ) => Promise; } @@ -152,8 +168,42 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps): { deleteConfig, deleteAclEntries, deleteGrantsForPrincipal, + recordAuditEntry, } = deps; + /** Emits a `grant.removed` audit entry for each grant the role-deletion cascade + * removed, so the cascade leaves the same forensic trail as an explicit revoke. + * Fail-open: the role is already deleted, so a failed audit is logged, not + * propagated. Sequential to keep the per-tenant hash chain ordered. */ + async function emitGrantRemovals( + req: ServerRequest, + roleName: string, + grants: ISystemGrant[], + ): Promise { + if (!recordAuditEntry || grants.length === 0) return; + const user = req.user; + const userId = user?._id?.toString() ?? user?.id; + if (!user || !userId) return; + const actorName = user.name || user.username || user.email || userId; + const context = buildAuditContext(req); + for (const grant of grants) { + try { + await recordAuditEntry({ + action: 'grant.removed', + outcome: 'success', + severity: 'warning', + actor: { type: 'user', id: userId, name: actorName }, + target: { type: PrincipalType.ROLE, id: roleName, name: roleName }, + metadata: { capability: grant.capability }, + context, + tenantId: user.tenantId, + }); + } catch (err) { + logger.error('[adminRoles] grant.removed audit failed during role deletion', err); + } + } + } + async function listRolesHandler(req: ServerRequest, res: Response) { try { const { limit, offset } = parsePagination(req.query); @@ -398,16 +448,19 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps): { } const tenantId = req.user?.tenantId; - const cleanupResults = await Promise.allSettled([ + const [configResult, aclResult, grantsResult] = await Promise.allSettled([ deleteConfig(PrincipalType.ROLE, name), deleteAclEntries({ principalType: PrincipalType.ROLE, principalId: name }), deleteGrantsForPrincipal(PrincipalType.ROLE, name, { tenantId }), ]); - for (const result of cleanupResults) { + for (const result of [configResult, aclResult, grantsResult]) { if (result.status === 'rejected') { logger.error('[adminRoles] cascade cleanup failed for role:', name, result.reason); } } + if (grantsResult.status === 'fulfilled') { + await emitGrantRemovals(req, name, grantsResult.value); + } return res.status(200).json({ success: true }); } catch (error) { diff --git a/packages/data-schemas/src/methods/systemGrant.ts b/packages/data-schemas/src/methods/systemGrant.ts index 833a31f07d..6d82e9cd56 100644 --- a/packages/data-schemas/src/methods/systemGrant.ts +++ b/packages/data-schemas/src/methods/systemGrant.ts @@ -126,7 +126,7 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): { principalType: PrincipalType, principalId: string | Types.ObjectId, options?: { tenantId?: string; session?: ClientSession }, - ) => Promise; + ) => Promise; } { function tenantCondition(tenantId?: string): FilterQuery { return tenantId != null @@ -512,7 +512,7 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): { principalType: PrincipalType, principalId: string | Types.ObjectId, options?: { tenantId?: string; session?: ClientSession }, - ): Promise { + ): Promise { const SystemGrant = mongoose.models.SystemGrant as Model; const normalizedPrincipalId = normalizePrincipalId(principalId, principalType); @@ -522,7 +522,11 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')): { ...(options?.tenantId != null && { tenantId: options.tenantId }), }; const queryOptions = options?.session ? { session: options.session } : {}; + /** Read the matching grants before deleting so cascade callers (e.g. role + * deletion) can emit a `grant.removed` audit entry for each one. */ + const removed = await SystemGrant.find(filter, null, queryOptions).lean(); await SystemGrant.deleteMany(filter, queryOptions); + return removed; } return {