From 86fe79c37d148a08c3ff2cda6b4aaa6dea57c16b Mon Sep 17 00:00:00 2001 From: Atef Bellaaj Date: Wed, 3 Jun 2026 20:17:17 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=97=20feat:=20Add=20Granular=20Access?= =?UTF-8?q?=20Control=20to=20Shared=20Links=20via=20ACL=20System=20(#13051?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: Add granular access control to shared links via ACL system * fix(shared-links): preserve isPublic on failed migration grants Transient ACL failures during auto-migration permanently stranded links — $unset ran unconditionally, removing the legacy flag that triggers retry. Now only $unset isPublic after all grants succeed. * fix(config): skip isPublic unset for failed ACL grants Bulk migration unconditionally removed isPublic from all links, even those whose ACL writes failed. Failed links then lost the legacy marker needed for auto-migration retry. Now tracks failed link IDs per-batch and excludes them from the $unset step. Also adds sharedLink to AccessRole resourceType schema enum — was missing, only worked because seedDefaultRoles uses findOneAndUpdate which bypasses validation. * ci(config): add jest config and PR workflow for migration tests config/__tests__/ specs depend on api/jest.config.js module mappings but had no dedicated runner. Adds config/jest.config.js extending api config with absolutized paths, npm test:config script, and a GitHub Actions workflow triggered by changes to config/, api/models/, api/db/, or packages/ ACL code. * fix(permissions): honor boolean sharedLinks config SHARED_LINKS has no USE permission, so boolean config produced an empty update payload — gate conditions only matched object form, making `sharedLinks: false` a no-op on existing perms. * fix(share): resolve role before creating shared link Role lookup between create and grant left an orphaned link without ACL entries if getRoleByName threw — retry then hit "Share already exists" with no recovery path. * fix: Restore Public ACL Access Checks * fix: Type Public ACL Lookup * fix: Preserve Private Legacy Shared Links * chore: Promote Shared Link Permission Migration * fix: Address Shared Link Review Findings * fix: Repair Shared Link CI Follow-Up * fix: Narrow Shared Link Mongoose Test Mock * fix: Address Shared Link Review Follow-Ups * fix: Close Shared Link Review Gaps * fix: Guard Missing Shared Link Permission Backfill * test: Add Shared Link Mock E2E * test: Stabilize Shared Link Mock E2E --------- Co-authored-by: Danny Avila --- .github/workflows/config-review.yml | 88 +++++ .../controllers/PermissionsController.js | 6 +- api/server/controllers/UserController.js | 3 +- .../controllers/__tests__/deleteUser.spec.js | 3 + .../deleteUserResourceCoverage.spec.js | 1 + api/server/middleware/canAccessSharedLink.js | 6 + .../__test-utils__/convos-route-mocks.js | 2 + api/server/routes/__tests__/convos.spec.js | 99 +++-- api/server/routes/__tests__/share.spec.js | 67 +++- api/server/routes/accessPermissions.js | 66 +++- .../accessPermissions.sharePolicy.test.js | 109 ++++++ api/server/routes/convos.js | 6 +- api/server/routes/share.js | 71 ++-- .../components/Chat/ExportAndShareMenu.tsx | 11 +- .../ConvoOptions/ConvoOptions.tsx | 14 +- .../ConvoOptions/ShareButton.tsx | 7 +- .../ConvoOptions/SharedLinkButton.tsx | 46 ++- .../Nav/SettingsTabs/Data/SharedLinks.tsx | 1 - client/src/components/Share/MessagesView.tsx | 2 +- .../components/Sharing/AccessRolesPicker.tsx | 10 +- .../Sharing/GenericGrantAccessDialog.tsx | 8 +- .../PeoplePicker/SelectedPrincipalsList.tsx | 48 ++- .../Sharing/PeoplePickerAdminSettings.tsx | 2 +- .../Sharing/PublicSharingToggle.tsx | 3 +- client/src/data-provider/queries.ts | 5 +- client/src/hooks/Sharing/useCanSharePublic.ts | 3 +- client/src/locales/en/translation.json | 2 + client/src/utils/resources.ts | 10 + client/src/utils/roles.ts | 9 + .../migrate-shared-link-permissions.spec.js | 218 +++++++++++ config/jest.config.js | 18 + config/migrate-shared-link-permissions.js | 357 +++++++++++++++++ e2e/specs/mock/shared-links.spec.ts | 187 +++++++++ librechat.example.yaml | 6 + package.json | 4 + .../api/src/acl/accessControlService.spec.ts | 60 +++ packages/api/src/acl/accessControlService.ts | 40 +- packages/api/src/app/permissions.spec.ts | 144 +++++++ packages/api/src/app/permissions.ts | 60 +++ packages/api/src/index.ts | 3 + packages/api/src/middleware/share.spec.ts | 40 +- packages/api/src/middleware/share.ts | 13 +- packages/api/src/shared-links/access.test.ts | 325 +++++++++++++++ packages/api/src/shared-links/access.ts | 137 +++++++ packages/api/src/shared-links/service.test.ts | 369 ++++++++++++++++++ packages/api/src/shared-links/service.ts | 324 +++++++++++++++ .../data-provider/src/accessPermissions.ts | 9 +- packages/data-provider/src/api-endpoints.ts | 3 +- packages/data-provider/src/config.ts | 15 + packages/data-provider/src/data-service.ts | 6 +- packages/data-provider/src/permissions.ts | 13 + packages/data-provider/src/roles.spec.ts | 3 +- packages/data-provider/src/roles.ts | 16 + packages/data-provider/src/schemas.ts | 1 - packages/data-provider/src/types.ts | 4 +- packages/data-provider/src/types/queries.ts | 2 - .../data-schemas/src/admin/capabilities.ts | 6 + packages/data-schemas/src/app/interface.ts | 1 + .../src/methods/accessRole.spec.ts | 2 + .../data-schemas/src/methods/accessRole.ts | 14 + packages/data-schemas/src/methods/aclEntry.ts | 6 +- .../data-schemas/src/methods/share.test.ts | 132 +------ packages/data-schemas/src/methods/share.ts | 60 +-- .../data-schemas/src/schema/accessRole.ts | 11 +- packages/data-schemas/src/schema/aclEntry.ts | 4 + packages/data-schemas/src/schema/role.ts | 5 + packages/data-schemas/src/schema/share.ts | 5 - packages/data-schemas/src/types/aclEntry.ts | 2 + packages/data-schemas/src/types/role.ts | 5 + packages/data-schemas/src/types/share.ts | 7 +- 70 files changed, 3057 insertions(+), 288 deletions(-) create mode 100644 .github/workflows/config-review.yml create mode 100644 api/server/middleware/canAccessSharedLink.js create mode 100644 config/__tests__/migrate-shared-link-permissions.spec.js create mode 100644 config/jest.config.js create mode 100644 config/migrate-shared-link-permissions.js create mode 100644 e2e/specs/mock/shared-links.spec.ts create mode 100644 packages/api/src/shared-links/access.test.ts create mode 100644 packages/api/src/shared-links/access.ts create mode 100644 packages/api/src/shared-links/service.test.ts create mode 100644 packages/api/src/shared-links/service.ts diff --git a/.github/workflows/config-review.yml b/.github/workflows/config-review.yml new file mode 100644 index 0000000000..e08662aa1c --- /dev/null +++ b/.github/workflows/config-review.yml @@ -0,0 +1,88 @@ +name: Config Migration Tests +on: + pull_request: + paths: + - 'config/**' + - 'api/models/**' + - 'api/db/**' + - 'packages/data-schemas/src/**' + - 'packages/data-provider/src/**' + - 'packages/api/src/acl/**' + - 'packages/api/src/shared-links/**' + +env: + NODE_ENV: CI + NODE_OPTIONS: '--max-old-space-size=${{ secrets.NODE_MAX_OLD_SPACE_SIZE || 6144 }}' + +jobs: + test-config: + name: 'Tests: config migrations' + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + + - name: Use Node.js 20.19 + uses: actions/setup-node@v4 + with: + node-version: '20.19' + + - name: Restore node_modules cache + id: cache-node-modules + uses: actions/cache@v4 + with: + path: | + node_modules + api/node_modules + packages/api/node_modules + packages/data-provider/node_modules + packages/data-schemas/node_modules + key: node-modules-backend-${{ runner.os }}-20.19-${{ hashFiles('package-lock.json') }} + + - name: Install dependencies + if: steps.cache-node-modules.outputs.cache-hit != 'true' + run: npm ci + + - name: Restore data-provider build cache + id: cache-data-provider + uses: actions/cache@v4 + with: + path: packages/data-provider/dist + key: build-data-provider-${{ runner.os }}-${{ hashFiles('packages/data-provider/src/**', 'packages/data-provider/tsconfig*.json', 'packages/data-provider/rollup.config.js', 'packages/data-provider/package.json') }} + + - name: Build data-provider + if: steps.cache-data-provider.outputs.cache-hit != 'true' + run: npm run build:data-provider + + - name: Restore data-schemas build cache + id: cache-data-schemas + uses: actions/cache@v4 + with: + path: packages/data-schemas/dist + key: build-data-schemas-${{ runner.os }}-${{ hashFiles('packages/data-schemas/src/**', 'packages/data-schemas/tsconfig*.json', 'packages/data-schemas/rollup.config.js', 'packages/data-schemas/package.json', 'packages/data-provider/src/**', 'packages/data-provider/tsconfig*.json', 'packages/data-provider/rollup.config.js', 'packages/data-provider/package.json') }} + + - name: Build data-schemas + if: steps.cache-data-schemas.outputs.cache-hit != 'true' + run: npm run build:data-schemas + + - name: Restore api build cache + id: cache-api + uses: actions/cache@v4 + with: + path: packages/api/dist + key: build-api-${{ runner.os }}-${{ hashFiles('packages/api/src/**', 'packages/api/tsconfig*.json', 'packages/api/server-rollup.config.js', 'packages/api/package.json', 'packages/data-provider/src/**', 'packages/data-provider/tsconfig*.json', 'packages/data-provider/rollup.config.js', 'packages/data-provider/package.json', 'packages/data-schemas/src/**', 'packages/data-schemas/tsconfig*.json', 'packages/data-schemas/rollup.config.js', 'packages/data-schemas/package.json') }} + + - name: Build api + if: steps.cache-api.outputs.cache-hit != 'true' + run: npm run build:api + + - name: Create empty auth.json file + run: | + mkdir -p api/data + echo '{}' > api/data/auth.json + + - name: Prepare .env.test file + run: cp api/test/.env.test.example api/test/.env.test + + - name: Run config migration tests + run: npm run test:config diff --git a/api/server/controllers/PermissionsController.js b/api/server/controllers/PermissionsController.js index ffe159a82c..000bba7671 100644 --- a/api/server/controllers/PermissionsController.js +++ b/api/server/controllers/PermissionsController.js @@ -134,8 +134,8 @@ const updateResourcePermissions = async (req, res) => { revokedPrincipals.push(...removed); } - // If public is disabled, add public to revoked list - if (!isPublic) { + // If public is explicitly disabled, add public to revoked list + if (isPublic === false) { revokedPrincipals.push({ type: PrincipalType.PUBLIC, id: null, @@ -167,7 +167,7 @@ const updateResourcePermissions = async (req, res) => { message: 'Permissions updated successfully', results: { principals: results.granted, - public: isPublic || false, + ...(isPublic !== undefined ? { public: isPublic } : {}), publicAccessRoleId: isPublic ? publicAccessRoleId : undefined, }, }; diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index ca560389e6..7884d0a611 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -7,6 +7,7 @@ const { MCPTokenStorage, normalizeHttpError, extractWebSearchEnvVars, + deleteAllSharedLinksWithCleanup, } = require('@librechat/api'); const { Tools, @@ -359,7 +360,7 @@ const deleteUserController = async (req, res) => { } await deleteUserPluginAuth(user.id, null, true); await db.deleteUserById(user.id); - await db.deleteAllSharedLinks(user.id); + await deleteAllSharedLinksWithCleanup(user.id); await deleteUserFiles(req); await db.deleteFiles(null, user.id); await db.deleteToolCalls(user.id); diff --git a/api/server/controllers/__tests__/deleteUser.spec.js b/api/server/controllers/__tests__/deleteUser.spec.js index 1d7c852153..6198122bd0 100644 --- a/api/server/controllers/__tests__/deleteUser.spec.js +++ b/api/server/controllers/__tests__/deleteUser.spec.js @@ -3,6 +3,7 @@ const mockDeleteMessages = jest.fn(); const mockDeleteAllUserSessions = jest.fn(); const mockDeleteUserById = jest.fn(); const mockDeleteAllSharedLinks = jest.fn(); +const mockDeleteAllSharedLinksWithCleanup = jest.fn(); const mockDeletePresets = jest.fn(); const mockDeleteUserKey = jest.fn(); const mockDeleteConvos = jest.fn(); @@ -38,6 +39,7 @@ jest.mock('@librechat/api', () => ({ extractWebSearchEnvVars: jest.fn(), needsRefresh: jest.fn(), getNewS3URL: jest.fn(), + deleteAllSharedLinksWithCleanup: (...args) => mockDeleteAllSharedLinksWithCleanup(...args), })); jest.mock('~/models', () => ({ @@ -126,6 +128,7 @@ function stubDeletionMocks() { mockDeleteUserPluginAuth.mockResolvedValue(); mockDeleteUserById.mockResolvedValue(); mockDeleteAllSharedLinks.mockResolvedValue(); + mockDeleteAllSharedLinksWithCleanup.mockResolvedValue({ deletedCount: 0 }); mockGetFiles.mockResolvedValue([]); mockProcessDeleteRequest.mockResolvedValue({ deletedFileIds: [], failedFileIds: [] }); mockDeleteFiles.mockResolvedValue(); diff --git a/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js index 78fcfa16b0..1bd5b2efaa 100644 --- a/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js +++ b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js @@ -16,6 +16,7 @@ const HANDLED_RESOURCE_TYPES = { [ResourceType.PROMPTGROUP]: 'deleteUserPrompts', [ResourceType.MCPSERVER]: 'deleteUserMcpServers', [ResourceType.SKILL]: 'deleteUserSkills', + [ResourceType.SHARED_LINK]: 'deleteAllSharedLinksWithCleanup', }; /** diff --git a/api/server/middleware/canAccessSharedLink.js b/api/server/middleware/canAccessSharedLink.js new file mode 100644 index 0000000000..79fd93e486 --- /dev/null +++ b/api/server/middleware/canAccessSharedLink.js @@ -0,0 +1,6 @@ +const mongoose = require('mongoose'); +const { createSharedLinkAccessMiddleware } = require('@librechat/api'); + +const canAccessSharedLink = createSharedLinkAccessMiddleware({ mongoose }); + +module.exports = canAccessSharedLink; diff --git a/api/server/routes/__test-utils__/convos-route-mocks.js b/api/server/routes/__test-utils__/convos-route-mocks.js index a3718addff..a0eb6fe312 100644 --- a/api/server/routes/__test-utils__/convos-route-mocks.js +++ b/api/server/routes/__test-utils__/convos-route-mocks.js @@ -12,6 +12,8 @@ module.exports = { })), logAxiosError: jest.fn(), restoreTenantContextFromReq: jest.fn((req, res, next) => next()), + deleteConvoSharedLinksWithCleanup: jest.fn(), + deleteAllSharedLinksWithCleanup: jest.fn(), ...overrides, }), diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index 23978f28e9..2f76669460 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/api/server/routes/__tests__/convos.spec.js @@ -21,13 +21,11 @@ jest.mock('~/server/services/Endpoints/assistants', () => require(MOCKS).assista describe('Convos Routes', () => { let app; let convosRouter; + const { deleteToolCalls, deleteConvos, saveConvo } = require('~/models'); const { - deleteAllSharedLinks, - deleteConvoSharedLink, - deleteToolCalls, - deleteConvos, - saveConvo, - } = require('~/models'); + deleteAllSharedLinksWithCleanup, + deleteConvoSharedLinksWithCleanup, + } = require('@librechat/api'); beforeAll(() => { convosRouter = require('../convos'); @@ -57,7 +55,7 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue(mockDbResponse); deleteToolCalls.mockResolvedValue({ deletedCount: 10 }); - deleteAllSharedLinks.mockResolvedValue({ + deleteAllSharedLinksWithCleanup.mockResolvedValue({ message: 'All shared links deleted successfully', deletedCount: 3, }); @@ -75,12 +73,12 @@ describe('Convos Routes', () => { expect(deleteToolCalls).toHaveBeenCalledWith('test-user-123'); expect(deleteToolCalls).toHaveBeenCalledTimes(1); - /** Verify deleteAllSharedLinks was called with correct userId */ - expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); - expect(deleteAllSharedLinks).toHaveBeenCalledTimes(1); + /** Verify deleteAllSharedLinksWithCleanup was called with correct userId */ + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledWith('test-user-123'); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledTimes(1); }); - it('should call deleteAllSharedLinks even when no conversations exist', async () => { + it('should call deleteAllSharedLinksWithCleanup even when no conversations exist', async () => { const mockDbResponse = { deletedCount: 0, message: 'No conversations to delete', @@ -88,7 +86,7 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue(mockDbResponse); deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); - deleteAllSharedLinks.mockResolvedValue({ + deleteAllSharedLinksWithCleanup.mockResolvedValue({ message: 'All shared links deleted successfully', deletedCount: 0, }); @@ -96,7 +94,7 @@ describe('Convos Routes', () => { const response = await request(app).delete('/api/convos/all'); expect(response.status).toBe(201); - expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledWith('test-user-123'); }); it('should return 500 if deleteConvos fails', async () => { @@ -123,10 +121,10 @@ describe('Convos Routes', () => { expect(response.text).toBe('Error clearing conversations'); }); - it('should return 500 if deleteAllSharedLinks fails', async () => { + it('should return 500 if deleteAllSharedLinksWithCleanup fails', async () => { deleteConvos.mockResolvedValue({ deletedCount: 5 }); deleteToolCalls.mockResolvedValue({ deletedCount: 10 }); - deleteAllSharedLinks.mockRejectedValue(new Error('Shared links deletion failed')); + deleteAllSharedLinksWithCleanup.mockRejectedValue(new Error('Shared links deletion failed')); const response = await request(app).delete('/api/convos/all'); @@ -138,12 +136,12 @@ describe('Convos Routes', () => { /** First user */ deleteConvos.mockResolvedValue({ deletedCount: 3 }); deleteToolCalls.mockResolvedValue({ deletedCount: 5 }); - deleteAllSharedLinks.mockResolvedValue({ deletedCount: 2 }); + deleteAllSharedLinksWithCleanup.mockResolvedValue({ deletedCount: 2 }); let response = await request(app).delete('/api/convos/all'); expect(response.status).toBe(201); - expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledWith('test-user-123'); jest.clearAllMocks(); @@ -158,12 +156,12 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue({ deletedCount: 7 }); deleteToolCalls.mockResolvedValue({ deletedCount: 12 }); - deleteAllSharedLinks.mockResolvedValue({ deletedCount: 4 }); + deleteAllSharedLinksWithCleanup.mockResolvedValue({ deletedCount: 4 }); response = await request(app2).delete('/api/convos/all'); expect(response.status).toBe(201); - expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-456'); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledWith('test-user-456'); }); it('should execute deletions in correct sequence', async () => { @@ -179,15 +177,19 @@ describe('Convos Routes', () => { return Promise.resolve({ deletedCount: 10 }); }); - deleteAllSharedLinks.mockImplementation(() => { - executionOrder.push('deleteAllSharedLinks'); + deleteAllSharedLinksWithCleanup.mockImplementation(() => { + executionOrder.push('deleteAllSharedLinksWithCleanup'); return Promise.resolve({ deletedCount: 3 }); }); await request(app).delete('/api/convos/all'); /** Verify all three functions were called */ - expect(executionOrder).toEqual(['deleteConvos', 'deleteToolCalls', 'deleteAllSharedLinks']); + expect(executionOrder).toEqual([ + 'deleteConvos', + 'deleteToolCalls', + 'deleteAllSharedLinksWithCleanup', + ]); }); it('should maintain data integrity by cleaning up shared links when conversations are deleted', async () => { @@ -201,17 +203,17 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue(mockConvosDeleted); deleteToolCalls.mockResolvedValue(mockToolCallsDeleted); - deleteAllSharedLinks.mockResolvedValue(mockSharedLinksDeleted); + deleteAllSharedLinksWithCleanup.mockResolvedValue(mockSharedLinksDeleted); const response = await request(app).delete('/api/convos/all'); expect(response.status).toBe(201); /** Verify that shared links cleanup was called for the same user */ - expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledWith('test-user-123'); /** Verify no shared links remain for deleted conversations */ - expect(deleteAllSharedLinks).toHaveBeenCalledAfter(deleteConvos); + expect(deleteAllSharedLinksWithCleanup).toHaveBeenCalledAfter(deleteConvos); }); }); @@ -225,7 +227,7 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue(mockDbResponse); deleteToolCalls.mockResolvedValue({ deletedCount: 3 }); - deleteConvoSharedLink.mockResolvedValue({ + deleteConvoSharedLinksWithCleanup.mockResolvedValue({ message: 'Shared links deleted successfully', deletedCount: 1, }); @@ -249,11 +251,14 @@ describe('Convos Routes', () => { /** Verify deleteToolCalls was called */ expect(deleteToolCalls).toHaveBeenCalledWith('test-user-123', mockConversationId); - /** Verify deleteConvoSharedLink was called */ - expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + /** Verify deleteConvoSharedLinksWithCleanup was called */ + expect(deleteConvoSharedLinksWithCleanup).toHaveBeenCalledWith( + 'test-user-123', + mockConversationId, + ); }); - it('should not call deleteConvoSharedLink when no conversationId provided', async () => { + it('should not call deleteConvoSharedLinksWithCleanup when no conversationId provided', async () => { deleteConvos.mockResolvedValue({ deletedCount: 0 }); deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); @@ -266,7 +271,7 @@ describe('Convos Routes', () => { }); expect(response.status).toBe(200); - expect(deleteConvoSharedLink).not.toHaveBeenCalled(); + expect(deleteConvoSharedLinksWithCleanup).not.toHaveBeenCalled(); }); it('should handle deletion of conversation without shared links', async () => { @@ -274,7 +279,7 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue({ deletedCount: 1 }); deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); - deleteConvoSharedLink.mockResolvedValue({ + deleteConvoSharedLinksWithCleanup.mockResolvedValue({ message: 'Shared links deleted successfully', deletedCount: 0, }); @@ -288,7 +293,10 @@ describe('Convos Routes', () => { }); expect(response.status).toBe(201); - expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + expect(deleteConvoSharedLinksWithCleanup).toHaveBeenCalledWith( + 'test-user-123', + mockConversationId, + ); }); it('should return 400 when no parameters provided', async () => { @@ -299,7 +307,7 @@ describe('Convos Routes', () => { expect(response.status).toBe(400); expect(response.body).toEqual({ error: 'no parameters provided' }); expect(deleteConvos).not.toHaveBeenCalled(); - expect(deleteConvoSharedLink).not.toHaveBeenCalled(); + expect(deleteConvoSharedLinksWithCleanup).not.toHaveBeenCalled(); }); it('should return 400 when request body is empty (DoS prevention)', async () => { @@ -336,12 +344,14 @@ describe('Convos Routes', () => { expect(deleteConvos).not.toHaveBeenCalled(); }); - it('should return 500 if deleteConvoSharedLink fails', async () => { + it('should return 500 if deleteConvoSharedLinksWithCleanup fails', async () => { const mockConversationId = 'conv-error'; deleteConvos.mockResolvedValue({ deletedCount: 1 }); deleteToolCalls.mockResolvedValue({ deletedCount: 2 }); - deleteConvoSharedLink.mockRejectedValue(new Error('Failed to delete shared links')); + deleteConvoSharedLinksWithCleanup.mockRejectedValue( + new Error('Failed to delete shared links'), + ); const response = await request(app) .delete('/api/convos') @@ -369,8 +379,8 @@ describe('Convos Routes', () => { return Promise.resolve({ deletedCount: 2 }); }); - deleteConvoSharedLink.mockImplementation(() => { - executionOrder.push('deleteConvoSharedLink'); + deleteConvoSharedLinksWithCleanup.mockImplementation(() => { + executionOrder.push('deleteConvoSharedLinksWithCleanup'); return Promise.resolve({ deletedCount: 1 }); }); @@ -382,7 +392,11 @@ describe('Convos Routes', () => { }, }); - expect(executionOrder).toEqual(['deleteConvos', 'deleteToolCalls', 'deleteConvoSharedLink']); + expect(executionOrder).toEqual([ + 'deleteConvos', + 'deleteToolCalls', + 'deleteConvoSharedLinksWithCleanup', + ]); }); it('should prevent orphaned shared links when deleting single conversation', async () => { @@ -390,7 +404,7 @@ describe('Convos Routes', () => { deleteConvos.mockResolvedValue({ deletedCount: 1 }); deleteToolCalls.mockResolvedValue({ deletedCount: 4 }); - deleteConvoSharedLink.mockResolvedValue({ + deleteConvoSharedLinksWithCleanup.mockResolvedValue({ message: 'Shared links deleted successfully', deletedCount: 2, }); @@ -406,10 +420,13 @@ describe('Convos Routes', () => { expect(response.status).toBe(201); /** Verify shared links were deleted for the specific conversation */ - expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + expect(deleteConvoSharedLinksWithCleanup).toHaveBeenCalledWith( + 'test-user-123', + mockConversationId, + ); /** Verify it was called after the conversation was deleted */ - expect(deleteConvoSharedLink).toHaveBeenCalledAfter(deleteConvos); + expect(deleteConvoSharedLinksWithCleanup).toHaveBeenCalledAfter(deleteConvos); }); }); diff --git a/api/server/routes/__tests__/share.spec.js b/api/server/routes/__tests__/share.spec.js index 541ae451c6..02941c8453 100644 --- a/api/server/routes/__tests__/share.spec.js +++ b/api/server/routes/__tests__/share.spec.js @@ -3,9 +3,18 @@ const request = require('supertest'); const mongoose = require('mongoose'); const mockGetSharedLinkExpiration = jest.fn(); +const mockGrantCreationPermissions = jest.fn(); +const mockUpdateSharedLinkPermissionsExpiration = jest.fn(); +const mockSharedLinksAccess = jest.fn((_req, _res, next) => next()); jest.mock('@librechat/api', () => ({ isEnabled: jest.fn(() => true), + generateCheckAccess: jest.fn(() => mockSharedLinksAccess), + grantCreationPermissions: (...args) => mockGrantCreationPermissions(...args), + updateSharedLinkPermissionsExpiration: (...args) => + mockUpdateSharedLinkPermissionsExpiration(...args), + ensureLinkPermissions: jest.fn(), + deleteSharedLinkWithCleanup: jest.fn(), getSharedLinkExpiration: (...args) => mockGetSharedLinkExpiration(...args), isActiveExpirationDate: jest.fn((expiredAt) => expiredAt > new Date()), })); @@ -16,6 +25,13 @@ jest.mock('@librechat/data-schemas', () => ({ })); jest.mock('librechat-data-provider', () => ({ + PermissionTypes: { + SHARED_LINKS: 'SHARED_LINKS', + }, + Permissions: { + CREATE: 'CREATE', + SHARE_PUBLIC: 'SHARE_PUBLIC', + }, RetentionMode: { ALL: 'all', TEMPORARY: 'temporary', @@ -40,13 +56,17 @@ jest.mock('~/models', () => ({ deleteSharedLink: jest.fn(), getSharedLinks: jest.fn(), getSharedLink: jest.fn(), + getRoleByName: jest.fn(), })); +jest.mock('~/server/middleware/canAccessSharedLink', () => (_req, _res, next) => next()); +jest.mock('~/server/middleware/optionalJwtAuth', () => (req, _res, next) => next()); jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next()); const { RetentionMode } = require('librechat-data-provider'); const { createTempChatExpirationDate, logger } = require('@librechat/data-schemas'); -const { createSharedLink, updateSharedLink } = require('~/models'); +const { deleteSharedLinkWithCleanup } = require('@librechat/api'); +const { createSharedLink, updateSharedLink, getRoleByName } = require('~/models'); const shareRouter = require('../share'); const activeExpiration = new Date('2030-01-01T00:00:00.000Z'); @@ -71,11 +91,19 @@ const buildApp = ({ retentionMode = RetentionMode.TEMPORARY } = {}) => { describe('share routes retention', () => { beforeEach(() => { jest.clearAllMocks(); + getRoleByName.mockResolvedValue({ + permissions: { + SHARED_LINKS: { + SHARE_PUBLIC: true, + }, + }, + }); + mockGrantCreationPermissions.mockResolvedValue(undefined); }); it('expires new shares for retained non-temporary conversations', async () => { mockGetSharedLinkExpiration.mockResolvedValue(activeExpiration); - createSharedLink.mockResolvedValue({ shareId: 'share-123' }); + createSharedLink.mockResolvedValue({ _id: 'link-123', shareId: 'share-123' }); const response = await request(buildApp()) .post('/api/share/convo-123') @@ -106,11 +134,18 @@ describe('share routes retention', () => { 'msg-123', new Date('2030-01-01T00:00:00.000Z'), ); + expect(mockGrantCreationPermissions).toHaveBeenCalledWith( + 'link-123', + 'user-123', + true, + new Date('2030-01-01T00:00:00.000Z'), + ); + expect(mockSharedLinksAccess).toHaveBeenCalled(); }); it('rejects new shares when the retained conversation expired', async () => { mockGetSharedLinkExpiration.mockResolvedValue(expiredExpiration); - createSharedLink.mockResolvedValue({ shareId: 'share-123' }); + createSharedLink.mockResolvedValue({ _id: 'link-123', shareId: 'share-123' }); const response = await request(buildApp()) .post('/api/share/convo-123') @@ -122,7 +157,7 @@ describe('share routes retention', () => { it('rejects new shares for expired conversations in all retention mode', async () => { mockGetSharedLinkExpiration.mockResolvedValue(expiredExpiration); - createSharedLink.mockResolvedValue({ shareId: 'share-123' }); + createSharedLink.mockResolvedValue({ _id: 'link-123', shareId: 'share-123' }); const response = await request(buildApp({ retentionMode: RetentionMode.ALL })) .post('/api/share/convo-123') @@ -135,7 +170,7 @@ describe('share routes retention', () => { it('expires updated shares for retained non-temporary conversations', async () => { mongoose.models.SharedLink.findOne.mockReturnValue(lean({ conversationId: 'convo-123' })); mockGetSharedLinkExpiration.mockResolvedValue(activeExpiration); - updateSharedLink.mockResolvedValue({ shareId: 'share-456' }); + updateSharedLink.mockResolvedValue({ _id: 'link-456', shareId: 'share-456' }); const response = await request(buildApp()).patch('/api/share/share-123'); @@ -162,6 +197,10 @@ describe('share routes retention', () => { undefined, new Date('2030-01-01T00:00:00.000Z'), ); + expect(mockUpdateSharedLinkPermissionsExpiration).toHaveBeenCalledWith( + 'link-456', + new Date('2030-01-01T00:00:00.000Z'), + ); }); it('rejects updated shares when the retained conversation expired', async () => { @@ -195,12 +234,14 @@ describe('share routes retention', () => { it('clears updated share expiration when the conversation is no longer retained', async () => { mongoose.models.SharedLink.findOne.mockReturnValue(lean({ conversationId: 'convo-123' })); mockGetSharedLinkExpiration.mockResolvedValue(null); - updateSharedLink.mockResolvedValue({ shareId: 'share-456' }); + updateSharedLink.mockResolvedValue({ _id: 'link-456', shareId: 'share-456' }); const response = await request(buildApp()).patch('/api/share/share-123'); expect(response.status).toBe(200); expect(updateSharedLink).toHaveBeenCalledWith('user-123', 'share-123', undefined, null); + expect(mockUpdateSharedLinkPermissionsExpiration).toHaveBeenCalledWith('link-456', null); + expect(mockSharedLinksAccess).not.toHaveBeenCalled(); }); it('preserves updated share expiration when the conversation cannot be found', async () => { @@ -212,6 +253,7 @@ describe('share routes retention', () => { expect(response.status).toBe(200); expect(updateSharedLink).toHaveBeenCalledWith('user-123', 'share-123', undefined, undefined); + expect(mockUpdateSharedLinkPermissionsExpiration).not.toHaveBeenCalled(); }); it('clears updated share expiration when creating a new expiration throws', async () => { @@ -221,7 +263,7 @@ describe('share routes retention', () => { dependencies.logger.error('[getSharedLinkExpiration] Error creating expiration date:', error); return null; }); - updateSharedLink.mockResolvedValue({ shareId: 'share-456' }); + updateSharedLink.mockResolvedValue({ _id: 'link-456', shareId: 'share-456' }); const response = await request(buildApp()).patch('/api/share/share-123'); @@ -231,6 +273,7 @@ describe('share routes retention', () => { error, ); expect(updateSharedLink).toHaveBeenCalledWith('user-123', 'share-123', undefined, null); + expect(mockUpdateSharedLinkPermissionsExpiration).toHaveBeenCalledWith('link-456', null); }); it('updates share target message while applying retention expiration', async () => { @@ -259,4 +302,14 @@ describe('share routes retention', () => { expect(response.status).toBe(400); expect(updateSharedLink).not.toHaveBeenCalled(); }); + + it('allows deleting existing shares without CREATE permission gate', async () => { + deleteSharedLinkWithCleanup.mockResolvedValue({ shareId: 'share-123' }); + + const response = await request(buildApp()).delete('/api/share/share-123'); + + expect(response.status).toBe(200); + expect(mockSharedLinksAccess).not.toHaveBeenCalled(); + expect(deleteSharedLinkWithCleanup).toHaveBeenCalledWith('user-123', 'share-123'); + }); }); diff --git a/api/server/routes/accessPermissions.js b/api/server/routes/accessPermissions.js index e53d0ef1a7..6ef731daba 100644 --- a/api/server/routes/accessPermissions.js +++ b/api/server/routes/accessPermissions.js @@ -1,5 +1,11 @@ +const mongoose = require('mongoose'); const express = require('express'); -const { ResourceType, PermissionBits } = require('librechat-data-provider'); +const { + AccessRoleIds, + PrincipalType, + ResourceType, + PermissionBits, +} = require('librechat-data-provider'); const { getUserEffectivePermissions, getAllEffectivePermissions, @@ -82,6 +88,12 @@ const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) = resourceIdParam: 'resourceId', idResolver: getSkillById, }); + } else if (resourceType === ResourceType.SHARED_LINK) { + middleware = canAccessResource({ + resourceType: ResourceType.SHARED_LINK, + requiredPermission, + resourceIdParam: 'resourceId', + }); } else { return res.status(400).json({ error: 'Bad Request', @@ -93,6 +105,57 @@ const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) = middleware(req, res, next); }; +const rejectSharedLinkOwnerPermissionChanges = async (req, res, next) => { + if (req.params.resourceType !== ResourceType.SHARED_LINK) { + return next(); + } + + const updated = Array.isArray(req.body?.updated) ? req.body.updated : []; + const removed = Array.isArray(req.body?.removed) ? req.body.removed : []; + const grantsOwner = updated.some( + (principal) => principal?.accessRoleId === AccessRoleIds.SHARED_LINK_OWNER, + ); + const grantsPublicOwner = req.body?.publicAccessRoleId === AccessRoleIds.SHARED_LINK_OWNER; + + if (grantsOwner || grantsPublicOwner) { + return res.status(400).json({ + error: 'Bad Request', + message: 'Shared link owner permissions cannot be changed', + }); + } + + const userMutations = [...updated, ...removed].filter( + (principal) => principal?.type === PrincipalType.USER && principal?.id, + ); + + if (userMutations.length === 0) { + return next(); + } + + try { + const SharedLink = mongoose.models.SharedLink; + const link = await SharedLink.findById(req.params.resourceId, 'user').lean(); + const ownerId = link?.user?.toString(); + const touchesOwner = ownerId + ? userMutations.some((principal) => principal.id?.toString() === ownerId) + : false; + + if (touchesOwner) { + return res.status(400).json({ + error: 'Bad Request', + message: 'Shared link owner permissions cannot be changed', + }); + } + } catch (_error) { + return res.status(500).json({ + error: 'Internal Server Error', + message: 'Failed to validate shared link owner permissions', + }); + } + + return next(); +}; + /** * GET /api/permissions/{resourceType}/{resourceId} * Get all permissions for a specific resource @@ -115,6 +178,7 @@ router.put( checkResourcePermissionAccess(PermissionBits.SHARE), checkShareAccess, checkSharePublicAccess, + rejectSharedLinkOwnerPermissionChanges, updateResourcePermissions, ); diff --git a/api/server/routes/accessPermissions.sharePolicy.test.js b/api/server/routes/accessPermissions.sharePolicy.test.js index 0fc7a90dea..ed17a04452 100644 --- a/api/server/routes/accessPermissions.sharePolicy.test.js +++ b/api/server/routes/accessPermissions.sharePolicy.test.js @@ -30,6 +30,7 @@ jest.mock('~/server/controllers/PermissionsController', () => ({ const express = require('express'); const request = require('supertest'); +const mongoose = require('mongoose'); const { SystemRoles, ResourceType, @@ -48,6 +49,8 @@ const { getRoleByName } = require('~/models'); describe('Access permissions share policy', () => { let app; + const mockSharedLinkFindById = jest.fn(); + const originalSharedLinkModel = mongoose.models.SharedLink; const resourceId = '507f1f77bcf86cd799439011'; const sharePolicyCases = [ @@ -116,8 +119,30 @@ describe('Access permissions share policy', () => { accessRoleId, }); + const allowSharedLinkSharing = () => { + getRoleByName.mockResolvedValue({ + permissions: { + [PermissionTypes.SHARED_LINKS]: { + [Permissions.SHARE]: true, + [Permissions.SHARE_PUBLIC]: true, + }, + }, + }); + }; + + const mockSharedLinkOwner = (ownerId = 'owner-user') => { + mockSharedLinkFindById.mockReturnValue({ + lean: jest.fn().mockResolvedValue({ user: ownerId }), + }); + }; + beforeEach(() => { jest.clearAllMocks(); + if (mongoose.models.SharedLink) { + mongoose.models.SharedLink.findById = mockSharedLinkFindById; + } else { + mongoose.models.SharedLink = { findById: mockSharedLinkFindById }; + } hasCapability.mockResolvedValue(false); app = express(); @@ -129,6 +154,14 @@ describe('Access permissions share policy', () => { app.use('/api/permissions', accessPermissionsRouter); }); + afterAll(() => { + if (originalSharedLinkModel) { + mongoose.models.SharedLink = originalSharedLinkModel; + } else { + delete mongoose.models.SharedLink; + } + }); + it.each(sharePolicyCases)( 'blocks non-public $label sharing when ACL SHARE passes but role SHARE is disabled', async ({ resourceType, permissionType, accessRoleId, middlewareOptions }) => { @@ -208,4 +241,80 @@ describe('Access permissions share policy', () => { }); expect(updateResourcePermissions).not.toHaveBeenCalled(); }); + + it('blocks granting shared-link owner through generic permission updates', async () => { + allowSharedLinkSharing(); + mockSharedLinkOwner(); + + const response = await request(app) + .put(`/api/permissions/${ResourceType.SHARED_LINK}/${resourceId}`) + .send({ + updated: [ + { + type: PrincipalType.USER, + id: 'target-user', + accessRoleId: AccessRoleIds.SHARED_LINK_OWNER, + }, + ], + public: false, + }); + + expect(response.status).toBe(400); + expect(response.body.message).toBe('Shared link owner permissions cannot be changed'); + expect(updateResourcePermissions).not.toHaveBeenCalled(); + }); + + it('blocks granting shared-link owner to the public principal', async () => { + allowSharedLinkSharing(); + + const response = await request(app) + .put(`/api/permissions/${ResourceType.SHARED_LINK}/${resourceId}`) + .send({ + public: true, + publicAccessRoleId: AccessRoleIds.SHARED_LINK_OWNER, + }); + + expect(response.status).toBe(400); + expect(response.body.message).toBe('Shared link owner permissions cannot be changed'); + expect(updateResourcePermissions).not.toHaveBeenCalled(); + expect(mockSharedLinkFindById).not.toHaveBeenCalled(); + }); + + it('blocks removing the canonical shared-link owner', async () => { + allowSharedLinkSharing(); + mockSharedLinkOwner('owner-user'); + + const response = await request(app) + .put(`/api/permissions/${ResourceType.SHARED_LINK}/${resourceId}`) + .send({ + updated: [], + removed: [{ type: PrincipalType.USER, id: 'owner-user' }], + public: false, + }); + + expect(response.status).toBe(400); + expect(response.body.message).toBe('Shared link owner permissions cannot be changed'); + expect(updateResourcePermissions).not.toHaveBeenCalled(); + }); + + it('allows viewer grants for non-owner shared-link users', async () => { + allowSharedLinkSharing(); + mockSharedLinkOwner('owner-user'); + + const response = await request(app) + .put(`/api/permissions/${ResourceType.SHARED_LINK}/${resourceId}`) + .send({ + updated: [ + { + type: PrincipalType.USER, + id: 'target-user', + accessRoleId: AccessRoleIds.SHARED_LINK_VIEWER, + }, + ], + public: false, + }); + + expect(response.status).toBe(200); + expect(updateResourcePermissions).toHaveBeenCalledTimes(1); + }); }); diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index dc59482afa..8dfe8621ef 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -5,6 +5,8 @@ const { isEnabled, resolveImportMaxFileSize, restoreTenantContextFromReq, + deleteAllSharedLinksWithCleanup, + deleteConvoSharedLinksWithCleanup, } = require('@librechat/api'); const { logger } = require('@librechat/data-schemas'); const { CacheKeys, EModelEndpoint } = require('librechat-data-provider'); @@ -133,7 +135,7 @@ router.delete('/', async (req, res) => { const dbResponse = await db.deleteConvos(req.user.id, filter); if (filter.conversationId) { await db.deleteToolCalls(req.user.id, filter.conversationId); - await db.deleteConvoSharedLink(req.user.id, filter.conversationId); + await deleteConvoSharedLinksWithCleanup(req.user.id, filter.conversationId); } res.status(201).json(dbResponse); } catch (error) { @@ -146,7 +148,7 @@ router.delete('/all', async (req, res) => { try { const dbResponse = await db.deleteConvos(req.user.id, {}); await db.deleteToolCalls(req.user.id); - await db.deleteAllSharedLinks(req.user.id); + await deleteAllSharedLinksWithCleanup(req.user.id); res.status(201).json(dbResponse); } catch (error) { logger.error('Error clearing conversations', error); diff --git a/api/server/routes/share.js b/api/server/routes/share.js index ce4dee1a1f..19c760007a 100644 --- a/api/server/routes/share.js +++ b/api/server/routes/share.js @@ -1,18 +1,36 @@ const mongoose = require('mongoose'); const express = require('express'); -const { isEnabled, isActiveExpirationDate, getSharedLinkExpiration } = require('@librechat/api'); +const { + isEnabled, + generateCheckAccess, + grantCreationPermissions, + ensureLinkPermissions, + deleteSharedLinkWithCleanup, + updateSharedLinkPermissionsExpiration, + isActiveExpirationDate, + getSharedLinkExpiration, +} = require('@librechat/api'); const { logger, createTempChatExpirationDate } = require('@librechat/data-schemas'); +const { PermissionTypes, Permissions } = require('librechat-data-provider'); const { getSharedMessages, createSharedLink, updateSharedLink, - deleteSharedLink, getSharedLinks, getSharedLink, + getRoleByName, } = require('~/models'); +const canAccessSharedLink = require('~/server/middleware/canAccessSharedLink'); +const optionalJwtAuth = require('~/server/middleware/optionalJwtAuth'); const requireJwtAuth = require('~/server/middleware/requireJwtAuth'); const router = express.Router(); +const checkSharedLinksAccess = generateCheckAccess({ + permissionType: PermissionTypes.SHARED_LINKS, + permissions: [Permissions.CREATE], + getRoleByName, +}); + const resolveSharedLinkExpiration = (req, conversationId) => getSharedLinkExpiration( { req, conversationId }, @@ -36,25 +54,19 @@ const allowSharedLinks = process.env.ALLOW_SHARED_LINKS === undefined || isEnabled(process.env.ALLOW_SHARED_LINKS); if (allowSharedLinks) { - const allowSharedLinksPublic = isEnabled(process.env.ALLOW_SHARED_LINKS_PUBLIC); - router.get( - '/:shareId', - allowSharedLinksPublic ? (req, res, next) => next() : requireJwtAuth, - async (req, res) => { - try { - const share = await getSharedMessages(req.params.shareId); - - if (share) { - res.status(200).json(share); - } else { - res.status(404).end(); - } - } catch (error) { - logger.error('Error getting shared messages:', error); - res.status(500).json({ message: 'Error getting shared messages' }); + router.get('/:shareId', optionalJwtAuth, canAccessSharedLink, async (req, res) => { + try { + const share = await getSharedMessages(req.params.shareId, req.shareResourceId); + if (share) { + res.status(200).json(share); + } else { + res.status(404).end(); } - }, - ); + } catch (error) { + logger.error('Error getting shared messages:', error); + res.status(500).json({ message: 'Error getting shared messages' }); + } + }); } /** @@ -65,7 +77,6 @@ router.get('/', requireJwtAuth, async (req, res) => { const params = { pageParam: req.query.cursor, pageSize: Math.max(1, parseInt(req.query.pageSize) || 10), - isPublic: isEnabled(req.query.isPublic), sortBy: ['createdAt', 'title'].includes(req.query.sortBy) ? req.query.sortBy : 'createdAt', sortDirection: ['asc', 'desc'].includes(req.query.sortDirection) ? req.query.sortDirection @@ -77,7 +88,6 @@ router.get('/', requireJwtAuth, async (req, res) => { req.user.id, params.pageParam, params.pageSize, - params.isPublic, params.sortBy, params.sortDirection, params.search, @@ -101,7 +111,12 @@ router.get('/link/:conversationId', requireJwtAuth, async (req, res) => { try { const share = await getSharedLink(req.user.id, req.params.conversationId); + if (share._id && share.success) { + await ensureLinkPermissions(share._id, req.user.id); + } + return res.status(200).json({ + _id: share._id, success: share.success, shareId: share.shareId, targetMessageId: share.targetMessageId, @@ -113,7 +128,7 @@ router.get('/link/:conversationId', requireJwtAuth, async (req, res) => { } }); -router.post('/:conversationId', requireJwtAuth, async (req, res) => { +router.post('/:conversationId', requireJwtAuth, checkSharedLinksAccess, async (req, res) => { try { const { targetMessageId } = req.body; const expiredAt = await resolveSharedLinkExpiration(req, req.params.conversationId); @@ -121,6 +136,10 @@ router.post('/:conversationId', requireJwtAuth, async (req, res) => { return res.status(404).end(); } + const role = await getRoleByName(req.user.role); + const sharedLinksPerms = role?.permissions?.[PermissionTypes.SHARED_LINKS] || {}; + const grantPublic = sharedLinksPerms[Permissions.SHARE_PUBLIC] === true; + const created = await createSharedLink( req.user.id, req.params.conversationId, @@ -128,6 +147,7 @@ router.post('/:conversationId', requireJwtAuth, async (req, res) => { expiredAt, ); if (created) { + await grantCreationPermissions(created._id, req.user.id, grantPublic, expiredAt); res.status(200).json(created); } else { res.status(404).end(); @@ -165,6 +185,9 @@ router.patch('/:shareId', requireJwtAuth, async (req, res) => { expiredAt, ); if (updatedShare) { + if (updatedShare._id && expiredAt !== undefined) { + await updateSharedLinkPermissionsExpiration(updatedShare._id, expiredAt); + } res.status(200).json(updatedShare); } else { res.status(404).end(); @@ -177,7 +200,7 @@ router.patch('/:shareId', requireJwtAuth, async (req, res) => { router.delete('/:shareId', requireJwtAuth, async (req, res) => { try { - const result = await deleteSharedLink(req.user.id, req.params.shareId); + const result = await deleteSharedLinkWithCleanup(req.user.id, req.params.shareId); if (!result) { return res.status(404).json({ message: 'Share not found' }); diff --git a/client/src/components/Chat/ExportAndShareMenu.tsx b/client/src/components/Chat/ExportAndShareMenu.tsx index 739f2c497b..5dcbe9ea13 100644 --- a/client/src/components/Chat/ExportAndShareMenu.tsx +++ b/client/src/components/Chat/ExportAndShareMenu.tsx @@ -2,11 +2,12 @@ import { useState, useId, useRef } from 'react'; import { useRecoilValue } from 'recoil'; import * as Ariakit from '@ariakit/react'; import { Upload, Share2 } from 'lucide-react'; +import { PermissionTypes, Permissions } from 'librechat-data-provider'; import { DropdownPopup, TooltipAnchor, useMediaQuery } from '@librechat/client'; import type * as t from '~/common'; import ExportModal from '~/components/Nav/ExportConversation/ExportModal'; import { ShareButton } from '~/components/Conversations/ConvoOptions'; -import { useLocalize } from '~/hooks'; +import { useHasAccess, useLocalize } from '~/hooks'; import store from '~/store'; export default function ExportAndShareMenu({ @@ -22,6 +23,10 @@ export default function ExportAndShareMenu({ const menuId = useId(); const shareButtonRef = useRef(null); const exportButtonRef = useRef(null); + const canCreateSharedLinks = useHasAccess({ + permissionType: PermissionTypes.SHARED_LINKS, + permission: Permissions.CREATE, + }); const isSmallScreen = useMediaQuery('(max-width: 768px)'); const conversation = useRecoilValue(store.conversationByIndex(0)); @@ -48,11 +53,11 @@ export default function ExportAndShareMenu({ label: localize('com_ui_share'), onClick: shareHandler, icon: , - show: isSharedButtonEnabled, + show: isSharedButtonEnabled && canCreateSharedLinks, /** NOTE: THE FOLLOWING PROPS ARE REQUIRED FOR MENU ITEMS THAT OPEN DIALOGS */ hideOnClick: false, ref: shareButtonRef, - render: (props) => )} /> + + {canManageAccess && ( + + ( + + )} + /> + + )} )} (-1); return ( -
+
role.accessRoleId !== ownerRoleId) + : accessRoles || []; + const selectedRole = accessRoles?.find((role) => role.accessRoleId === selectedRoleId); const selectedRoleInfo = selectedRole ? getLocalizedRoleInfo(selectedRole.accessRoleId) : null; @@ -44,7 +50,7 @@ export default function AccessRolesPicker({ return ; } - const dropdownItems: t.MenuItemProps[] = accessRoles.map((role: AccessRole) => { + const dropdownItems: t.MenuItemProps[] = filteredRoles.map((role: AccessRole) => { const localizedInfo = getLocalizedRoleInfo(role.accessRoleId); return { id: role.accessRoleId, diff --git a/client/src/components/Sharing/GenericGrantAccessDialog.tsx b/client/src/components/Sharing/GenericGrantAccessDialog.tsx index ce7fdbdc14..24e9d1ae9b 100644 --- a/client/src/components/Sharing/GenericGrantAccessDialog.tsx +++ b/client/src/components/Sharing/GenericGrantAccessDialog.tsx @@ -180,14 +180,18 @@ export default function GenericGrantAccessDialog({ return !allSharesMap.has(key); }); + const publicChanged = isPublic !== currentIsPublic; + const publicRoleChanged = isPublic && publicRole !== currentPublicRole; + const sendPublicUpdate = publicChanged || publicRoleChanged; + await updatePermissionsMutation.mutateAsync({ resourceType, resourceId: resourceDbId, data: { updated, removed, - public: isPublic, - publicAccessRoleId: isPublic ? publicRole : undefined, + ...(sendPublicUpdate ? { public: isPublic } : {}), + ...(sendPublicUpdate && isPublic ? { publicAccessRoleId: publicRole } : {}), }, }); diff --git a/client/src/components/Sharing/PeoplePicker/SelectedPrincipalsList.tsx b/client/src/components/Sharing/PeoplePicker/SelectedPrincipalsList.tsx index b74b212669..05ee3704a5 100644 --- a/client/src/components/Sharing/PeoplePicker/SelectedPrincipalsList.tsx +++ b/client/src/components/Sharing/PeoplePicker/SelectedPrincipalsList.tsx @@ -5,6 +5,7 @@ import { ResourceType } from 'librechat-data-provider'; import type { TPrincipal, AccessRoleIds } from 'librechat-data-provider'; import AccessRolesPicker from '~/components/Sharing/AccessRolesPicker'; import PrincipalAvatar from '~/components/Sharing/PrincipalAvatar'; +import { RESOURCE_CONFIGS } from '~/utils/resources'; import { useLocalize } from '~/hooks'; interface SelectedPrincipalsListProps { @@ -50,6 +51,10 @@ export default function SelectedPrincipalsList({
{principles.map((share) => { const { displayName, subtitle } = getPrincipalDisplayInfo(share); + const ownerRoleId = RESOURCE_CONFIGS[resourceType]?.defaultOwnerRoleId; + const isOwner = share.accessRoleId === ownerRoleId; + const isSharedLink = resourceType === ResourceType.SHARED_LINK; + const lockOwner = isSharedLink && isOwner; return (
- {!!share.accessRoleId && !!onRoleChange && ( - { - onRoleChange?.(share.idOnTheSource!, newRole); - }} - className="min-w-0" - /> + {lockOwner ? ( + + {localize('com_ui_role_owner')} + + ) : ( + !!share.accessRoleId && + !!onRoleChange && ( + { + onRoleChange?.(share.idOnTheSource!, newRole); + }} + className="min-w-0" + /> + ) + )} + {!lockOwner && ( + )} -
); diff --git a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx index 5f334f3764..ab3f9f5517 100644 --- a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx +++ b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx @@ -140,7 +140,7 @@ const PeoplePickerAdminSettings = () => {