From cb398c2671628ac47d160453402138cff1fe359e Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 14 May 2026 14:51:49 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20feat:=20Resolve=20audit?= =?UTF-8?q?=20names=20without=20extra=20DB=20round-trips?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For the actor name, JWT-authenticated `req.user` already carries `name`, `username`, and `email`. `resolveUser` now derives the actor display name from `req.user` directly and threads it through the caller context, so every grant assign and revoke no longer triggers a separate `getUserById` lookup. For the target name, replaces the previous always-store-the-principalId behavior (which buried opaque ObjectId strings in immutable audit rows for USER and GROUP targets) with a `resolveTargetName` dep. ROLE principals continue to use `principalId` directly because the SystemGrant model stores role names there. USER and GROUP principals route through the new dep, which in `api/server/routes/admin/grants.js` calls `db.getUserById` or `db.findGroupById` respectively and falls back to the principalId on miss or error so the audit row stays intelligible. Drops the misleading "display name lookup happens in a later iteration" comment. --- api/server/routes/admin/grants.js | 14 +++++++-- packages/api/src/admin/grants.ts | 49 ++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/api/server/routes/admin/grants.js b/api/server/routes/admin/grants.js index 9aa01e036f..201d627199 100644 --- a/api/server/routes/admin/grants.js +++ b/api/server/routes/admin/grants.js @@ -1,5 +1,6 @@ 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'); @@ -22,10 +23,17 @@ const handlers = createAdminGrantsHandlers({ getCachedPrincipals, checkRoleExists: async (name) => (await db.getRoleByName(name)) != null, recordAuditEntry: db.recordAuditEntry, - resolveUserName: async (userId) => { + resolveTargetName: async (principalType, principalId) => { try { - const user = await db.getUserById(userId, 'name username email'); - return user?.name || user?.username || user?.email || null; + 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; } diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index 1209769ac4..5da7873889 100644 --- a/packages/api/src/admin/grants.ts +++ b/packages/api/src/admin/grants.ts @@ -79,8 +79,16 @@ 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 a User document's display name from its id (for audit actor name). */ - resolveUserName?: (userId: string | Types.ObjectId) => 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. */ @@ -107,28 +115,38 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { getCachedPrincipals, checkRoleExists, recordAuditEntry, - resolveUserName, + resolveTargetName, } = deps; async function emitAudit(args: { action: AuditAction; - caller: { userId: string; role: string; tenantId?: string }; + caller: { userId: string; actorName: string; role: string; tenantId?: string }; principalType: PrincipalType; principalId: string; 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 { - const actorName = resolveUserName - ? ((await resolveUserName(args.caller.userId)) ?? args.caller.userId) - : args.caller.userId; - // For ROLE principals the principalId IS the human-readable name; for USER/GROUP - // the same string is the id, and the display name lookup happens in a later iteration. - const targetName = args.principalId; await recordAuditEntry({ action: args.action, actorId: args.caller.userId, - actorName, + actorName: args.caller.actorName, targetPrincipalType: args.principalType, targetPrincipalId: args.principalId, targetName, @@ -136,7 +154,7 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { tenantId: args.caller.tenantId, }); } catch (err) { - // Audit failure must not roll back the grant — log and move on. + /** Audit failure must not roll back the grant: log and move on. */ logger.error('[adminGrants] audit write failed', err); } } @@ -155,7 +173,7 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { function resolveUser( req: ServerRequest, - ): { userId: string; role: string; tenantId?: string } | null { + ): { userId: string; role: string; actorName: string; tenantId?: string } | null { const user = req.user; if (!user) { return null; @@ -164,7 +182,10 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps): { if (!userId || !user.role) { return null; } - return { userId, role: user.role, tenantId: user.tenantId }; + /** JWT-loaded `req.user` already carries name/username/email, so the actor + * display name is available without a database round-trip. */ + const actorName = user.name || user.username || user.email || userId; + return { userId, role: user.role, actorName, tenantId: user.tenantId }; } async function resolvePrincipals(user: {