🛡️ fix: Restrict System Grants to Role Principals (#12491)

* 🛡️ fix: restrict system grants to role principals only

Narrows GrantPrincipalType to PrincipalType.ROLE, rejecting GROUP and
USER with 400. Removes grant cascade cleanup from group/user deletion
handlers and their route wiring since only roles can hold grants.

* 🛡️ fix: address review findings for grants roles-only restriction

Add missing GROUP rejection test for revokeGrant (symmetric with
getPrincipalGrants and assignGrant coverage), add extensibility comment
to GrantPrincipalType, and document the checkRoleExists guard.

api/server/routes/admin/groups.js

@@ -24,7 +24,6 @@ const handlers = createAdminGroupsHandlers({
   findUsers: db.findUsers,
   deleteConfig: db.deleteConfig,
   deleteAclEntries: db.deleteAclEntries,
-  deleteGrantsForPrincipal: db.deleteGrantsForPrincipal,
 });
 
 router.use(requireJwtAuth, requireAdminAccess);

api/server/routes/admin/users.js

@@ -17,7 +17,6 @@ const handlers = createAdminUsersHandlers({
   deleteUserById: db.deleteUserById,
   deleteConfig: db.deleteConfig,
   deleteAclEntries: db.deleteAclEntries,
-  deleteGrantsForPrincipal: db.deleteGrantsForPrincipal,
 });
 
 router.use(requireJwtAuth, requireAdminAccess);

packages/api/src/admin/grants.spec.ts

@@ -456,10 +456,8 @@ describe('createAdminGrantsHandlers', () => {
       expect(json).toHaveBeenCalledWith({ grants });
     });
 
-    it('returns grants for a group principal', async () => {
-      const deps = createDeps({
-        getCapabilitiesForPrincipal: jest.fn().mockResolvedValue([]),
-      });
+    it('returns 400 for group principal type', async () => {
+      const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
       const { req, res, status, json } = createReqRes({
         params: { principalType: PrincipalType.GROUP, principalId: validObjectId },
@@ -467,15 +465,12 @@ describe('createAdminGrantsHandlers', () => {
 
       await handlers.getPrincipalGrants(req, res);
 
-      expect(status).toHaveBeenCalledWith(200);
-      expect(json).toHaveBeenCalledWith({ grants: [] });
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
     });
 
-    it('returns grants for a user principal', async () => {
-      const grants = [mockGrant({ principalType: PrincipalType.USER, principalId: validObjectId })];
-      const deps = createDeps({
-        getCapabilitiesForPrincipal: jest.fn().mockResolvedValue(grants),
-      });
+    it('returns 400 for user principal type', async () => {
+      const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
       const { req, res, status, json } = createReqRes({
         params: { principalType: PrincipalType.USER, principalId: validObjectId },
@@ -483,17 +478,8 @@ describe('createAdminGrantsHandlers', () => {
 
       await handlers.getPrincipalGrants(req, res);
 
-      expect(deps.hasCapabilityForPrincipals).toHaveBeenCalledWith(
-        expect.objectContaining({ capability: SystemCapabilities.READ_USERS }),
-      );
-      expect(deps.getCapabilitiesForPrincipal).toHaveBeenCalledWith(
-        expect.objectContaining({
-          principalType: PrincipalType.USER,
-          principalId: validObjectId,
-        }),
-      );
-      expect(status).toHaveBeenCalledWith(200);
-      expect(json).toHaveBeenCalledWith({ grants });
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
     });
 
     it('passes tenantId to dep calls', async () => {
@@ -542,33 +528,7 @@ describe('createAdminGrantsHandlers', () => {
       expect(json).toHaveBeenCalledWith({ error: 'Principal ID is required' });
     });
 
-    it('returns 400 for non-ObjectId group principalId', async () => {
-      const deps = createDeps();
-      const handlers = createAdminGrantsHandlers(deps);
-      const { req, res, status, json } = createReqRes({
-        params: { principalType: PrincipalType.GROUP, principalId: 'not-an-objectid' },
-      });
-
-      await handlers.getPrincipalGrants(req, res);
-
-      expect(status).toHaveBeenCalledWith(400);
-      expect(json).toHaveBeenCalledWith({ error: 'Invalid principalId format' });
-    });
-
-    it('returns 400 for non-ObjectId user principalId', async () => {
-      const deps = createDeps();
-      const handlers = createAdminGrantsHandlers(deps);
-      const { req, res, status, json } = createReqRes({
-        params: { principalType: PrincipalType.USER, principalId: 'not-an-objectid' },
-      });
-
-      await handlers.getPrincipalGrants(req, res);
-
-      expect(status).toHaveBeenCalledWith(400);
-      expect(json).toHaveBeenCalledWith({ error: 'Invalid principalId format' });
-    });
-
-    it('accepts string principalId for role type without ObjectId validation', async () => {
+    it('accepts string principalId for role type', async () => {
       const deps = createDeps({
         getCapabilitiesForPrincipal: jest.fn().mockResolvedValue([]),
       });
@@ -792,13 +752,13 @@ describe('createAdminGrantsHandlers', () => {
       expect(json).toHaveBeenCalledWith({ error: 'Invalid capability' });
     });
 
-    it('returns 400 for invalid ObjectId on group principalId', async () => {
+    it('returns 400 for group principal type', async () => {
       const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
       const { req, res, status, json } = createReqRes({
         body: {
           principalType: PrincipalType.GROUP,
-          principalId: 'not-an-objectid',
+          principalId: validObjectId,
           capability: SystemCapabilities.READ_USERS,
         },
       });
@@ -806,7 +766,7 @@ describe('createAdminGrantsHandlers', () => {
       await handlers.assignGrant(req, res);
 
       expect(status).toHaveBeenCalledWith(400);
-      expect(json).toHaveBeenCalledWith({ error: 'Invalid principalId format' });
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
     });
 
     it('returns 401 when user is not authenticated', async () => {
@@ -899,16 +859,10 @@ describe('createAdminGrantsHandlers', () => {
       );
     });
 
-    it('checks MANAGE_GROUPS for group principal type', async () => {
-      const deps = createDeps({
-        getHeldCapabilities: jest
-          .fn()
-          .mockResolvedValue(
-            new Set([SystemCapabilities.MANAGE_GROUPS, SystemCapabilities.READ_USERS]),
-          ),
-      });
+    it('rejects group principal type before checking capabilities', async () => {
+      const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
-      const { req, res } = createReqRes({
+      const { req, res, status, json } = createReqRes({
         body: {
           principalType: PrincipalType.GROUP,
           principalId: validObjectId,
@@ -918,23 +872,15 @@ describe('createAdminGrantsHandlers', () => {
 
       await handlers.assignGrant(req, res);
 
-      expect(deps.getHeldCapabilities).toHaveBeenCalledWith(
-        expect.objectContaining({
-          capabilities: expect.arrayContaining([SystemCapabilities.MANAGE_GROUPS]),
-        }),
-      );
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
+      expect(deps.getHeldCapabilities).not.toHaveBeenCalled();
     });
 
-    it('checks MANAGE_USERS for user principal type', async () => {
-      const deps = createDeps({
-        getHeldCapabilities: jest
-          .fn()
-          .mockResolvedValue(
-            new Set([SystemCapabilities.MANAGE_USERS, SystemCapabilities.READ_USERS]),
-          ),
-      });
+    it('rejects user principal type before checking capabilities', async () => {
+      const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
-      const { req, res } = createReqRes({
+      const { req, res, status, json } = createReqRes({
         body: {
           principalType: PrincipalType.USER,
           principalId: validObjectId,
@@ -944,11 +890,9 @@ describe('createAdminGrantsHandlers', () => {
 
       await handlers.assignGrant(req, res);
 
-      expect(deps.getHeldCapabilities).toHaveBeenCalledWith(
-        expect.objectContaining({
-          capabilities: expect.arrayContaining([SystemCapabilities.MANAGE_USERS]),
-        }),
-      );
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
+      expect(deps.getHeldCapabilities).not.toHaveBeenCalled();
     });
 
     it('returns 400 when role does not exist', async () => {
@@ -1174,13 +1118,13 @@ describe('createAdminGrantsHandlers', () => {
       expect(deps.revokeCapability).not.toHaveBeenCalled();
     });
 
-    it('returns 400 for invalid user ObjectId', async () => {
+    it('returns 400 for group principal type', async () => {
       const deps = createDeps();
       const handlers = createAdminGrantsHandlers(deps);
       const { req, res, status, json } = createReqRes({
         params: {
-          principalType: PrincipalType.USER,
-          principalId: 'bad-id',
+          principalType: PrincipalType.GROUP,
+          principalId: validObjectId,
           capability: SystemCapabilities.READ_USERS,
         },
       });
@@ -1188,7 +1132,24 @@ describe('createAdminGrantsHandlers', () => {
       await handlers.revokeGrant(req, res);
 
       expect(status).toHaveBeenCalledWith(400);
-      expect(json).toHaveBeenCalledWith({ error: 'Invalid principalId format' });
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
+    });
+
+    it('returns 400 for user principal type', async () => {
+      const deps = createDeps();
+      const handlers = createAdminGrantsHandlers(deps);
+      const { req, res, status, json } = createReqRes({
+        params: {
+          principalType: PrincipalType.USER,
+          principalId: validObjectId,
+          capability: SystemCapabilities.READ_USERS,
+        },
+      });
+
+      await handlers.revokeGrant(req, res);
+
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'Invalid principal type' });
     });
 
     it('returns 500 on unexpected error', async () => {

packages/api/src/admin/grants.ts

@@ -2,7 +2,6 @@ import { PrincipalType } from 'librechat-data-provider';
 import {
   logger,
   isValidCapability,
-  isValidObjectIdString,
   SystemCapabilities,
   expandImplications,
 } from '@librechat/data-schemas';
@@ -75,7 +74,8 @@ export interface AdminGrantsDeps {
   checkRoleExists?: (roleId: string) => Promise<boolean>;
 }
 
-export type GrantPrincipalType = PrincipalType.ROLE | PrincipalType.GROUP | PrincipalType.USER;
+/** Currently ROLE-only; Record/Set structure preserved for future principal-type expansion. */
+export type GrantPrincipalType = PrincipalType.ROLE;
 
 /** Creates admin grant handlers with dependency injection for the /api/admin/grants routes. */
 export function createAdminGrantsHandlers(deps: AdminGrantsDeps) {
@@ -95,14 +95,10 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps) {
 
   const MANAGE_CAPABILITY_BY_TYPE: Record<GrantPrincipalType, SystemCapability> = {
     [PrincipalType.ROLE]: SystemCapabilities.MANAGE_ROLES,
-    [PrincipalType.GROUP]: SystemCapabilities.MANAGE_GROUPS,
-    [PrincipalType.USER]: SystemCapabilities.MANAGE_USERS,
   };
 
   const READ_CAPABILITY_BY_TYPE: Record<GrantPrincipalType, SystemCapability> = {
     [PrincipalType.ROLE]: SystemCapabilities.READ_ROLES,
-    [PrincipalType.GROUP]: SystemCapabilities.READ_GROUPS,
-    [PrincipalType.USER]: SystemCapabilities.READ_USERS,
   };
 
   const VALID_PRINCIPAL_TYPES = new Set(
@@ -148,9 +144,6 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps) {
     if (!principalId) {
       return 'Principal ID is required';
     }
-    if (principalType !== PrincipalType.ROLE && !isValidObjectIdString(principalId)) {
-      return 'Invalid principalId format';
-    }
     return null;
   }
 
@@ -335,14 +328,8 @@ export function createAdminGrantsHandlers(deps: AdminGrantsDeps) {
         return res.status(403).json({ error: 'Cannot grant a capability you do not possess' });
       }
 
-      /*
-       * Role existence is validated when checkRoleExists is provided.
-       * GROUP and USER principals are ObjectId-validated by validatePrincipal
-       * but not existence-checked — orphan grants for deleted principals are
-       * accepted as a trade-off. Cascade cleanup on group/user deletion
-       * (deleteGrantsForPrincipal) handles the removal path.
-       */
-      if (principalType === PrincipalType.ROLE && checkRoleExists) {
+      /** Reject grants targeting non-existent roles when the dep is provided. */
+      if (checkRoleExists) {
         const exists = await checkRoleExists(principalId);
         if (!exists) {
           return res.status(400).json({ error: 'Role not found' });

packages/api/src/admin/groups.spec.ts

@@ -76,7 +76,6 @@ describe('createAdminGroupsHandlers', () => {
       findUsers: jest.fn().mockResolvedValue([]),
       deleteConfig: jest.fn().mockResolvedValue(null),
       deleteAclEntries: jest.fn().mockResolvedValue({ deletedCount: 0 }),
-      deleteGrantsForPrincipal: jest.fn().mockResolvedValue(undefined),
       ...overrides,
     };
   }
@@ -809,12 +808,9 @@ describe('createAdminGroupsHandlers', () => {
         principalType: PrincipalType.GROUP,
         principalId: new Types.ObjectId(validId),
       });
-      expect(deps.deleteGrantsForPrincipal).toHaveBeenCalledWith(PrincipalType.GROUP, validId, {
-        tenantId: undefined,
-      });
     });
 
-    it('cleans up Config, AclEntry, and SystemGrant on group delete', async () => {
+    it('cleans up Config and AclEntry on group delete', async () => {
       const deps = createDeps({ deleteGroup: jest.fn().mockResolvedValue(mockGroup()) });
       const handlers = createAdminGroupsHandlers(deps);
       const { req, res, status } = createReqRes({ params: { id: validId } });
@@ -827,9 +823,6 @@ describe('createAdminGroupsHandlers', () => {
         principalType: PrincipalType.GROUP,
         principalId: new Types.ObjectId(validId),
       });
-      expect(deps.deleteGrantsForPrincipal).toHaveBeenCalledWith(PrincipalType.GROUP, validId, {
-        tenantId: undefined,
-      });
     });
   });
 

packages/api/src/admin/groups.ts

@@ -82,11 +82,6 @@ export interface AdminGroupsDeps {
     principalType: PrincipalType;
     principalId: string | Types.ObjectId;
   }) => Promise<DeleteResult>;
-  deleteGrantsForPrincipal: (
-    principalType: PrincipalType,
-    principalId: string | Types.ObjectId,
-    options?: { tenantId?: string },
-  ) => Promise<void>;
 }
 
 export function createAdminGroupsHandlers(deps: AdminGroupsDeps) {
@@ -103,7 +98,6 @@ export function createAdminGroupsHandlers(deps: AdminGroupsDeps) {
     findUsers,
     deleteConfig,
     deleteAclEntries,
-    deleteGrantsForPrincipal,
   } = deps;
 
   async function listGroupsHandler(req: ServerRequest, res: Response) {
@@ -309,16 +303,14 @@ export function createAdminGroupsHandlers(deps: AdminGroupsDeps) {
       /**
        * deleteAclEntries is a raw deleteMany wrapper with no type casting.
        * grantPermission stores group principalId as ObjectId, so we must
-       * cast here. deleteConfig and deleteGrantsForPrincipal normalize internally.
+       * cast here. deleteConfig normalizes internally.
        */
-      const tenantId = req.user?.tenantId;
       const cleanupResults = await Promise.allSettled([
         deleteConfig(PrincipalType.GROUP, id),
         deleteAclEntries({
           principalType: PrincipalType.GROUP,
           principalId: new Types.ObjectId(id),
         }),
-        deleteGrantsForPrincipal(PrincipalType.GROUP, id, { tenantId }),
       ]);
       for (const result of cleanupResults) {
         if (result.status === 'rejected') {

packages/api/src/admin/users.spec.ts

@@ -58,7 +58,6 @@ function createDeps(overrides: Partial<AdminUsersDeps> = {}): AdminUsersDeps {
       .mockResolvedValue({ deletedCount: 1, message: 'User was deleted successfully.' }),
     deleteConfig: jest.fn().mockResolvedValue(null),
     deleteAclEntries: jest.fn().mockResolvedValue(undefined),
-    deleteGrantsForPrincipal: jest.fn().mockResolvedValue(undefined),
     ...overrides,
   };
 }
@@ -417,7 +416,7 @@ describe('createAdminUsersHandlers', () => {
       expect(deps.countUsers).not.toHaveBeenCalled();
     });
 
-    it('cascades cleanup of Config, AclEntries, and SystemGrants', async () => {
+    it('cascades cleanup of Config and AclEntries', async () => {
       const result: UserDeleteResult = {
         deletedCount: 1,
         message: 'User was deleted successfully.',
@@ -434,24 +433,6 @@ describe('createAdminUsersHandlers', () => {
         principalType: PrincipalType.USER,
         principalId: expect.any(Types.ObjectId),
       });
-      expect(deps.deleteGrantsForPrincipal).toHaveBeenCalledWith(PrincipalType.USER, validUserId, {
-        tenantId: undefined,
-      });
-    });
-
-    it('scopes grant cleanup to the caller tenantId', async () => {
-      const deps = createDeps();
-      const handlers = createAdminUsersHandlers(deps);
-      const { req, res } = createReqRes({
-        params: { id: validUserId },
-        user: { _id: new Types.ObjectId(), role: 'admin', tenantId: 'tenant-xyz' },
-      });
-
-      await handlers.deleteUser(req, res);
-
-      expect(deps.deleteGrantsForPrincipal).toHaveBeenCalledWith(PrincipalType.USER, validUserId, {
-        tenantId: 'tenant-xyz',
-      });
     });
 
     it('returns success even when cascade cleanup partially fails', async () => {
@@ -483,7 +464,6 @@ describe('createAdminUsersHandlers', () => {
       expect(status).toHaveBeenCalledWith(404);
       expect(deps.deleteConfig).not.toHaveBeenCalled();
       expect(deps.deleteAclEntries).not.toHaveBeenCalled();
-      expect(deps.deleteGrantsForPrincipal).not.toHaveBeenCalled();
     });
 
     it('returns 400 for invalid ObjectId', async () => {

packages/api/src/admin/users.ts

@@ -28,7 +28,7 @@ export interface AdminUsersDeps {
    * Thin data-layer delete — removes the User document only.
    * Full cascade of user-owned resources (conversations, messages, files, tokens, etc.)
    * is handled by `UserController.deleteUserController` in the self-delete flow.
-   * This admin endpoint currently cascades Config, AclEntries, and SystemGrants.
+   * This admin endpoint currently cascades Config and AclEntries.
    * A future iteration should consolidate the full cascade into a shared service function.
    */
   deleteUserById: (userId: string) => Promise<UserDeleteResult>;
@@ -40,22 +40,10 @@ export interface AdminUsersDeps {
     principalType: PrincipalType;
     principalId: string | Types.ObjectId;
   }) => Promise<void>;
-  deleteGrantsForPrincipal: (
-    principalType: PrincipalType,
-    principalId: string | Types.ObjectId,
-    options?: { tenantId?: string },
-  ) => Promise<void>;
 }
 
 export function createAdminUsersHandlers(deps: AdminUsersDeps) {
-  const {
-    findUsers,
-    countUsers,
-    deleteUserById,
-    deleteConfig,
-    deleteAclEntries,
-    deleteGrantsForPrincipal,
-  } = deps;
+  const { findUsers, countUsers, deleteUserById, deleteConfig, deleteAclEntries } = deps;
 
   async function listUsersHandler(req: ServerRequest, res: Response) {
     try {
@@ -171,11 +159,9 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps) {
       }
 
       const objectId = new Types.ObjectId(id);
-      const tenantId = req.user?.tenantId;
       const cleanupResults = await Promise.allSettled([
         deleteConfig(PrincipalType.USER, id),
         deleteAclEntries({ principalType: PrincipalType.USER, principalId: objectId }),
-        deleteGrantsForPrincipal(PrincipalType.USER, id, { tenantId }),
       ]);
       for (const r of cleanupResults) {
         if (r.status === 'rejected') {