✅ test: Cover audit emission, scope emitAudit to today's ROLE-only surface

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

@@ -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);

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);
+    });
+  });
 });

packages/api/src/admin/grants.ts

@@ -79,16 +79,6 @@ export interface AdminGrantsDeps {
   checkRoleExists?: (roleId: string) => Promise<boolean>;
   /** Optional audit emission. Failure is logged but does not roll back the grant. */
   recordAuditEntry?: (input: RecordAuditEntryInput) => Promise<void>;
-  /**
-   * 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<string | null>;
 }
 
 /** 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<void> {
     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,
       });