From d55a014af72648fa46dc48ba3af6573c9f573ee7 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 14 May 2026 14:55:59 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=85=20test:=20Cover=20audit=20emission,?= =?UTF-8?q?=20scope=20emitAudit=20to=20today's=20ROLE-only=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a misleading test that claimed to verify "idempotent even if the grant does not exist" while mocking deletedCount: 1 (the grant DID exist). Replaces it with the actual no-op scenario (deletedCount: 0) and adds an assertion that recordAuditEntry is NOT called, since the whole point of the deletedCount > 0 gate is to avoid fictitious revocation rows. Adds a dedicated audit emission describe block covering: grant_assigned emission with the actor name resolved from req.user, grant_removed emission when deletedCount is positive, and the no-emission fallback when recordAuditEntry is not configured. The actor-name assertions exercise the name / username / email fallback chain in resolveUser. The previous commit also added a `resolveTargetName` dep and an emitAudit branch for USER/GROUP targets. The grants surface is ROLE-only today (MANAGE_CAPABILITY_BY_TYPE has only PrincipalType.ROLE), so that code path is unreachable from the handler. Removed the dep and the branch; the audit row uses principalId as the target name, which is the human-readable role name for ROLE principals. A comment in emitAudit flags where to plumb resolveTargetName back in once USER and GROUP grants are enabled. --- api/server/routes/admin/grants.js | 16 ----- packages/api/src/admin/grants.spec.ts | 86 ++++++++++++++++++++++++++- packages/api/src/admin/grants.ts | 35 +++-------- 3 files changed, 91 insertions(+), 46 deletions(-) diff --git a/api/server/routes/admin/grants.js b/api/server/routes/admin/grants.js index 201d627199..baa1325d9f 100644 --- a/api/server/routes/admin/grants.js +++ b/api/server/routes/admin/grants.js @@ -1,6 +1,5 @@ const express = require('express'); const { createAdminGrantsHandlers, getCachedPrincipals } = require('@librechat/api'); -const { PrincipalType } = require('librechat-data-provider'); const { SystemCapabilities } = require('@librechat/data-schemas'); const { requireCapability } = require('~/server/middleware/roles/capabilities'); const { requireJwtAuth } = require('~/server/middleware'); @@ -23,21 +22,6 @@ const handlers = createAdminGrantsHandlers({ getCachedPrincipals, checkRoleExists: async (name) => (await db.getRoleByName(name)) != null, recordAuditEntry: db.recordAuditEntry, - resolveTargetName: async (principalType, principalId) => { - try { - if (principalType === PrincipalType.USER) { - const user = await db.getUserById(principalId, 'name username email'); - return user?.name || user?.username || user?.email || null; - } - if (principalType === PrincipalType.GROUP) { - const group = await db.findGroupById(principalId, { name: 1 }); - return group?.name || null; - } - return null; - } catch { - return null; - } - }, }); router.use(requireJwtAuth, requireAdminAccess); diff --git a/packages/api/src/admin/grants.spec.ts b/packages/api/src/admin/grants.spec.ts index 575c406426..0399df98a1 100644 --- a/packages/api/src/admin/grants.spec.ts +++ b/packages/api/src/admin/grants.spec.ts @@ -967,9 +967,11 @@ describe('createAdminGrantsHandlers', () => { capability: SystemCapabilities.READ_USERS, }; - it('returns 200 idempotently even if the grant does not exist', async () => { + it('returns 200 idempotently and skips audit emission when the grant does not exist', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); const deps = createDeps({ - revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 1 }), + revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 0 }), + recordAuditEntry, }); const handlers = createAdminGrantsHandlers(deps); const { req, res, status, json } = createReqRes({ params: validParams }); @@ -978,6 +980,9 @@ describe('createAdminGrantsHandlers', () => { expect(status).toHaveBeenCalledWith(200); expect(json).toHaveBeenCalledWith({ success: true }); + /** A no-op revoke must not produce an audit row. The whole point of the + * `deletedCount > 0` gate is to avoid logging fictitious revocations. */ + expect(recordAuditEntry).not.toHaveBeenCalled(); }); it('revokes a grant and returns 200', async () => { @@ -1165,4 +1170,81 @@ describe('createAdminGrantsHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'Failed to revoke grant' }); }); }); + + describe('audit emission', () => { + const assignBody = { + principalType: PrincipalType.ROLE, + principalId: 'editor', + capability: SystemCapabilities.READ_USERS, + }; + + function reqUser(overrides: { name?: string; username?: string; email?: string } = {}) { + return { + _id: new Types.ObjectId(), + role: 'admin', + tenantId: 'tenant-1', + ...overrides, + } as { _id: Types.ObjectId; role: string; tenantId?: string }; + } + + it('emits a grant_assigned entry after a successful assignment', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); + const deps = createDeps({ recordAuditEntry }); + const handlers = createAdminGrantsHandlers(deps); + const user = reqUser({ name: 'Alice Admin' }); + const { req, res } = createReqRes({ body: assignBody, user }); + + await handlers.assignGrant(req, res); + + expect(recordAuditEntry).toHaveBeenCalledTimes(1); + expect(recordAuditEntry).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'grant_assigned', + actorId: user._id.toString(), + actorName: 'Alice Admin', + targetPrincipalType: PrincipalType.ROLE, + targetPrincipalId: 'editor', + /** ROLE targets carry the human-readable name in `principalId`, so the audit + * row stores it directly. */ + targetName: 'editor', + capability: SystemCapabilities.READ_USERS, + tenantId: 'tenant-1', + }), + ); + }); + + it('emits a grant_removed entry when deletedCount > 0', async () => { + const recordAuditEntry = jest.fn().mockResolvedValue(undefined); + const deps = createDeps({ + revokeCapability: jest.fn().mockResolvedValue({ deletedCount: 1 }), + recordAuditEntry, + }); + const handlers = createAdminGrantsHandlers(deps); + const user = reqUser({ username: 'alice' }); + const { req, res } = createReqRes({ params: assignBody, user }); + + await handlers.revokeGrant(req, res); + + expect(recordAuditEntry).toHaveBeenCalledTimes(1); + expect(recordAuditEntry).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'grant_removed', + actorName: 'alice', + targetPrincipalType: PrincipalType.ROLE, + targetPrincipalId: 'editor', + targetName: 'editor', + }), + ); + }); + + it('does not emit when recordAuditEntry is not configured', async () => { + const deps = createDeps(); + const handlers = createAdminGrantsHandlers(deps); + const { req, res, status } = createReqRes({ body: assignBody, user: reqUser() }); + + await handlers.assignGrant(req, res); + + expect(status).toHaveBeenCalledWith(201); + }); + }); }); diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index 5da7873889..b85cf200eb 100644 --- a/packages/api/src/admin/grants.ts +++ b/packages/api/src/admin/grants.ts @@ -79,16 +79,6 @@ export interface AdminGrantsDeps { checkRoleExists?: (roleId: string) => Promise; /** Optional audit emission. Failure is logged but does not roll back the grant. */ recordAuditEntry?: (input: RecordAuditEntryInput) => Promise; - /** - * Resolves the human-readable target name for USER/GROUP principals (ROLE - * principals already carry their name in `principalId`). Returning `null` - * falls back to the principalId string, so callers do not need to throw on - * a missing lookup. - */ - resolveTargetName?: ( - principalType: PrincipalType, - principalId: string, - ) => Promise; } /** Currently ROLE-only; Record/Set structure preserved for future principal-type expansion. */ @@ -115,7 +105,6 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { getCachedPrincipals, checkRoleExists, recordAuditEntry, - resolveTargetName, } = deps; async function emitAudit(args: { @@ -126,30 +115,20 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { capability: SystemCapability; }): Promise { if (!recordAuditEntry) return; - /** - * For ROLE principals, `principalId` is the role name (the SystemGrant - * model stores names, not ObjectIds, for ROLE). For USER and GROUP, it is - * an ObjectId hex string; resolving a display name requires a lookup, and - * the dep is responsible for it. Falling back to `principalId` keeps the - * audit row intelligible (just less friendly) when the lookup misses. - */ - let targetName = args.principalId; - if (args.principalType !== PrincipalType.ROLE && resolveTargetName) { - try { - const resolved = await resolveTargetName(args.principalType, args.principalId); - if (resolved) targetName = resolved; - } catch (err) { - logger.error('[adminGrants] target name resolution failed', err); - } - } try { + /** The grants surface is ROLE-only today (see `MANAGE_CAPABILITY_BY_TYPE`), + * and SystemGrant stores the human-readable role name in `principalId` for + * ROLE principals, so the audit row's `targetName` is just `principalId`. + * When USER and GROUP grants are enabled, a `resolveTargetName` dep should + * be added here to look up the display name; until then it would be dead + * code. */ await recordAuditEntry({ action: args.action, actorId: args.caller.userId, actorName: args.caller.actorName, targetPrincipalType: args.principalType, targetPrincipalId: args.principalId, - targetName, + targetName: args.principalId, capability: args.capability, tenantId: args.caller.tenantId, });