diff --git a/packages/api/src/acl/accessControlService.ts b/packages/api/src/acl/accessControlService.ts index 361bb70098..fbd41f0210 100644 --- a/packages/api/src/acl/accessControlService.ts +++ b/packages/api/src/acl/accessControlService.ts @@ -351,7 +351,7 @@ export class AccessControlService { requiredPermission, }: { userId: string; - role?: string; + role?: string | null; resourceType: ResourceType; resourceId: string | Types.ObjectId; requiredPermission: number; diff --git a/packages/api/src/shared-links/access.test.ts b/packages/api/src/shared-links/access.test.ts index f76284227e..a1176e5137 100644 --- a/packages/api/src/shared-links/access.test.ts +++ b/packages/api/src/shared-links/access.test.ts @@ -6,7 +6,13 @@ jest.mock('@librechat/data-schemas', () => ({ import mongoose, { Types, Model } from 'mongoose'; import { MongoMemoryServer } from 'mongodb-memory-server'; import { createModels, createMethods, tenantStorage } from '@librechat/data-schemas'; -import { ResourceType, PrincipalType, AccessRoleIds } from 'librechat-data-provider'; +import { + ResourceType, + PrincipalType, + PrincipalModel, + AccessRoleIds, + PermissionBits, +} from 'librechat-data-provider'; import type { Request, Response, NextFunction } from 'express'; import type { IAclEntry, ISharedLink } from '@librechat/data-schemas'; import { AccessControlService } from '~/acl/accessControlService'; @@ -105,6 +111,33 @@ async function grantUserViewer(resourceId: Types.ObjectId, uid: Types.ObjectId) }); } +/** + * Inserts a ROLE VIEW grant directly (bypassing the role-name lookup, which is + * tenant-scoped and would miss the globally seeded roles under a tenant context). + * Inherits the caller's tenant context so the entry is tenant-scoped on save. + */ +async function grantRoleViewer(resourceId: Types.ObjectId, role: string) { + await new AclEntry({ + principalType: PrincipalType.ROLE, + principalModel: PrincipalModel.ROLE, + principalId: role, + resourceType: ResourceType.SHARED_LINK, + resourceId, + permBits: PermissionBits.VIEW, + grantedBy: userId, + }).save(); +} + +/** Mirrors getUserPrincipals: a ROLE principal is only added when a role is supplied. */ +function mockPrincipalsWithRole(viewer: Types.ObjectId) { + mockGetUserPrincipals.mockImplementation(({ role }: { role?: string | null }) => + Promise.resolve([ + { principalType: PrincipalType.USER, principalId: viewer }, + ...(role ? [{ principalType: PrincipalType.ROLE, principalId: role }] : []), + ]), + ); +} + describe('canAccessSharedLink', () => { describe('input validation', () => { test('returns 400 when shareId is missing', async () => { @@ -241,6 +274,98 @@ describe('canAccessSharedLink', () => { }); describe('cross-tenant lookup', () => { + test('does not let a cross-tenant viewer role satisfy a ROLE ACL from another tenant', async () => { + // Share owned by tenant-a, granted VIEW to role USER. A tenant-b viewer whose + // own role is also USER must not inherit that grant: role principals are just + // name strings, so the middleware suppresses the viewer's role for cross-tenant + // views. getUserPrincipals is called with role: null, so no ROLE principal is + // built and the tenant-a ROLE:USER grant never matches → 403. + const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + const tenantLink = await createTestLink(); + await grantRoleViewer(tenantLink._id, 'USER'); + return tenantLink; + }); + const viewer = new Types.ObjectId(); + mockPrincipalsWithRole(viewer); + + const req = createReq({ + params: { shareId: link.shareId }, + user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-b' }, + }); + const res = createRes(); + const next = jest.fn(); + await tenantStorage.run({ tenantId: 'tenant-b' }, () => + canAccessSharedLink(req, res, next as unknown as NextFunction), + ); + + expect(next).not.toHaveBeenCalled(); + expect(res._status).toBe(403); + expect(mockGetUserPrincipals).toHaveBeenCalledWith({ + userId: viewer.toString(), + role: null, + }); + }); + + test('still honors a ROLE ACL for a same-tenant viewer with that role', async () => { + // Same-tenant view: the viewer's role is trusted, so the tenant-a ROLE:USER + // grant matches and access is allowed. Confirms the cross-tenant fix does not + // break legitimate role-based sharing within a tenant. + const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + const tenantLink = await createTestLink(); + await grantRoleViewer(tenantLink._id, 'USER'); + return tenantLink; + }); + const viewer = new Types.ObjectId(); + mockPrincipalsWithRole(viewer); + + const req = createReq({ + params: { shareId: link.shareId }, + user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-a' }, + }); + const res = createRes(); + const next = jest.fn(); + await tenantStorage.run({ tenantId: 'tenant-a' }, () => + canAccessSharedLink(req, res, next as unknown as NextFunction), + ); + + expect(next).toHaveBeenCalled(); + expect((req as unknown as Record).shareResourceId).toBe(link._id.toString()); + expect(mockGetUserPrincipals).toHaveBeenCalledWith({ + userId: viewer.toString(), + role: 'USER', + }); + }); + + test('honors a same-tenant ROLE grant when no ALS tenant context is set (cookie-auth file request)', async () => { + // Share file routes (/download) authenticate via optionalShareFileAuth, + // which sets req.user from the refresh cookie but establishes no tenant ALS + // context, so getTenantId() is undefined here. The comparison must use the + // authenticated user's own tenantId, else a same-tenant viewer is wrongly denied + // their ROLE grant. No tenantStorage.run wrapper mirrors the file route. + const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + const tenantLink = await createTestLink(); + await grantRoleViewer(tenantLink._id, 'USER'); + return tenantLink; + }); + const viewer = new Types.ObjectId(); + mockPrincipalsWithRole(viewer); + + const req = createReq({ + params: { shareId: link.shareId }, + user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-a' }, + }); + const res = createRes(); + const next = jest.fn(); + await canAccessSharedLink(req, res, next as unknown as NextFunction); + + expect(next).toHaveBeenCalled(); + expect((req as unknown as Record).shareResourceId).toBe(link._id.toString()); + expect(mockGetUserPrincipals).toHaveBeenCalledWith({ + userId: viewer.toString(), + role: 'USER', + }); + }); + test('resolves a share owned by another tenant via system fallback, then enforces the ACL', async () => { // Share created under tenant-a; an authenticated viewer from tenant-b would // miss the tenant-scoped lookup and previously get a 404 before access could diff --git a/packages/api/src/shared-links/access.ts b/packages/api/src/shared-links/access.ts index 5e1d23a32f..1743abba31 100644 --- a/packages/api/src/shared-links/access.ts +++ b/packages/api/src/shared-links/access.ts @@ -56,6 +56,7 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) { return; } + const viewerTenantId = getTenantId(); const SharedLink = mg.models.SharedLink as Model; const findShare = async () => (await SharedLink.findOne({ @@ -68,8 +69,8 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) { // different tenant — still resolves. Access remains gated by the ACL check // below, which runs under the share's own tenant, so this only broadens the // lookup, never the authorization. - let rawShare = getTenantId() ? await findShare() : await runAsSystem(findShare); - if (!rawShare && getTenantId()) { + let rawShare = viewerTenantId ? await findShare() : await runAsSystem(findShare); + if (!rawShare && viewerTenantId) { rawShare = await runAsSystem(findShare); } @@ -136,7 +137,10 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) { const hasAccess = await aclService.checkPermission({ userId, - role: user.role, + // Trust the viewer's role only for a same-tenant view, comparing the share + // tenant to the user's own tenantId (the ALS context is absent on cookie-auth + // file requests). null suppresses the ROLE principal for cross-tenant views. + role: rawShare.tenantId === user.tenantId ? user.role : null, resourceType: ResourceType.SHARED_LINK, resourceId, requiredPermission: PermissionBits.VIEW,