From 2451bf54cfc44ca54edc9357b916289307f7ac0d Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Tue, 31 Mar 2026 16:25:14 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Restrict=20Syste?= =?UTF-8?q?m=20Grants=20to=20Role=20Principals=20(#12491)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🛡️ 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 | 1 - api/server/routes/admin/users.js | 1 - packages/api/src/admin/grants.spec.ts | 129 +++++++++----------------- packages/api/src/admin/grants.ts | 21 +---- packages/api/src/admin/groups.spec.ts | 9 +- packages/api/src/admin/groups.ts | 10 +- packages/api/src/admin/users.spec.ts | 22 +---- packages/api/src/admin/users.ts | 18 +--- 8 files changed, 54 insertions(+), 157 deletions(-) diff --git a/api/server/routes/admin/groups.js b/api/server/routes/admin/groups.js index 7ca93acaa2..11ed59737e 100644 --- a/api/server/routes/admin/groups.js +++ b/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); diff --git a/api/server/routes/admin/users.js b/api/server/routes/admin/users.js index 46ef16e7c2..20d4eb1797 100644 --- a/api/server/routes/admin/users.js +++ b/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); diff --git a/packages/api/src/admin/grants.spec.ts b/packages/api/src/admin/grants.spec.ts index a11103741f..cd6d1e8d38 100644 --- a/packages/api/src/admin/grants.spec.ts +++ b/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 () => { diff --git a/packages/api/src/admin/grants.ts b/packages/api/src/admin/grants.ts index 6e0b607778..b7205d6c7c 100644 --- a/packages/api/src/admin/grants.ts +++ b/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; } -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 = { [PrincipalType.ROLE]: SystemCapabilities.MANAGE_ROLES, - [PrincipalType.GROUP]: SystemCapabilities.MANAGE_GROUPS, - [PrincipalType.USER]: SystemCapabilities.MANAGE_USERS, }; const READ_CAPABILITY_BY_TYPE: Record = { [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' }); diff --git a/packages/api/src/admin/groups.spec.ts b/packages/api/src/admin/groups.spec.ts index 44371acdf8..d10dfda7e0 100644 --- a/packages/api/src/admin/groups.spec.ts +++ b/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, - }); }); }); diff --git a/packages/api/src/admin/groups.ts b/packages/api/src/admin/groups.ts index 4fc968949f..41dfba6cac 100644 --- a/packages/api/src/admin/groups.ts +++ b/packages/api/src/admin/groups.ts @@ -82,11 +82,6 @@ export interface AdminGroupsDeps { principalType: PrincipalType; principalId: string | Types.ObjectId; }) => Promise; - deleteGrantsForPrincipal: ( - principalType: PrincipalType, - principalId: string | Types.ObjectId, - options?: { tenantId?: string }, - ) => Promise; } 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') { diff --git a/packages/api/src/admin/users.spec.ts b/packages/api/src/admin/users.spec.ts index d4191eafac..1d0e5fbac8 100644 --- a/packages/api/src/admin/users.spec.ts +++ b/packages/api/src/admin/users.spec.ts @@ -58,7 +58,6 @@ function createDeps(overrides: Partial = {}): 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 () => { diff --git a/packages/api/src/admin/users.ts b/packages/api/src/admin/users.ts index 8679b0dbcf..6fc13fd97f 100644 --- a/packages/api/src/admin/users.ts +++ b/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; @@ -40,22 +40,10 @@ export interface AdminUsersDeps { principalType: PrincipalType; principalId: string | Types.ObjectId; }) => Promise; - deleteGrantsForPrincipal: ( - principalType: PrincipalType, - principalId: string | Types.ObjectId, - options?: { tenantId?: string }, - ) => Promise; } 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') {