diff --git a/api/models/File.spec.js b/api/models/File.spec.js index 2d4282cff7..ecb2e21b08 100644 --- a/api/models/File.spec.js +++ b/api/models/File.spec.js @@ -152,12 +152,11 @@ describe('File Access Control', () => { expect(accessMap.get(fileIds[3])).toBe(false); }); - it('should grant access to all files when user is the agent author', async () => { + it('should only grant author access to files attached to the agent', async () => { const authorId = new mongoose.Types.ObjectId(); const agentId = uuidv4(); const fileIds = [uuidv4(), uuidv4(), uuidv4()]; - // Create author user await User.create({ _id: authorId, email: 'author@example.com', @@ -165,7 +164,6 @@ describe('File Access Control', () => { provider: 'local', }); - // Create agent await createAgent({ id: agentId, name: 'Test Agent', @@ -174,12 +172,83 @@ describe('File Access Control', () => { provider: 'openai', tool_resources: { file_search: { - file_ids: [fileIds[0]], // Only one file attached + file_ids: [fileIds[0]], + }, + }, + }); + + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + const accessMap = await hasAccessToFilesViaAgent({ + userId: authorId, + role: SystemRoles.USER, + fileIds, + agentId, + }); + + expect(accessMap.get(fileIds[0])).toBe(true); + expect(accessMap.get(fileIds[1])).toBe(false); + expect(accessMap.get(fileIds[2])).toBe(false); + }); + + it('should deny all access when agent has no tool_resources', async () => { + const authorId = new mongoose.Types.ObjectId(); + const agentId = uuidv4(); + const fileId = uuidv4(); + + await User.create({ + _id: authorId, + email: 'author-no-resources@example.com', + emailVerified: true, + provider: 'local', + }); + + await createAgent({ + id: agentId, + name: 'Bare Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + }); + + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + const accessMap = await hasAccessToFilesViaAgent({ + userId: authorId, + role: SystemRoles.USER, + fileIds: [fileId], + agentId, + }); + + expect(accessMap.get(fileId)).toBe(false); + }); + + it('should grant access to files across multiple resource types', async () => { + const authorId = new mongoose.Types.ObjectId(); + const agentId = uuidv4(); + const fileIds = [uuidv4(), uuidv4(), uuidv4()]; + + await User.create({ + _id: authorId, + email: 'author-multi@example.com', + emailVerified: true, + provider: 'local', + }); + + await createAgent({ + id: agentId, + name: 'Multi Resource Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + tool_resources: { + file_search: { + file_ids: [fileIds[0]], + }, + execute_code: { + file_ids: [fileIds[1]], }, }, }); - // Check access as the author const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); const accessMap = await hasAccessToFilesViaAgent({ userId: authorId, @@ -188,10 +257,48 @@ describe('File Access Control', () => { agentId, }); - // Author should have access to all files expect(accessMap.get(fileIds[0])).toBe(true); expect(accessMap.get(fileIds[1])).toBe(true); - expect(accessMap.get(fileIds[2])).toBe(true); + expect(accessMap.get(fileIds[2])).toBe(false); + }); + + it('should grant author access to attached files when isDelete is true', async () => { + const authorId = new mongoose.Types.ObjectId(); + const agentId = uuidv4(); + const attachedFileId = uuidv4(); + const unattachedFileId = uuidv4(); + + await User.create({ + _id: authorId, + email: 'author-delete@example.com', + emailVerified: true, + provider: 'local', + }); + + await createAgent({ + id: agentId, + name: 'Delete Test Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + tool_resources: { + file_search: { + file_ids: [attachedFileId], + }, + }, + }); + + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + const accessMap = await hasAccessToFilesViaAgent({ + userId: authorId, + role: SystemRoles.USER, + fileIds: [attachedFileId, unattachedFileId], + agentId, + isDelete: true, + }); + + expect(accessMap.get(attachedFileId)).toBe(true); + expect(accessMap.get(unattachedFileId)).toBe(false); }); it('should handle non-existent agent gracefully', async () => { diff --git a/api/server/services/Files/permissions.js b/api/server/services/Files/permissions.js index d909afe25a..df484f7c29 100644 --- a/api/server/services/Files/permissions.js +++ b/api/server/services/Files/permissions.js @@ -4,7 +4,26 @@ const { checkPermission } = require('~/server/services/PermissionService'); const { getAgent } = require('~/models/Agent'); /** - * Checks if a user has access to multiple files through a shared agent (batch operation) + * @param {Object} agent - The agent document (lean) + * @returns {Set} All file IDs attached across all resource types + */ +function getAttachedFileIds(agent) { + const attachedFileIds = new Set(); + if (agent.tool_resources) { + for (const resource of Object.values(agent.tool_resources)) { + if (resource?.file_ids && Array.isArray(resource.file_ids)) { + for (const fileId of resource.file_ids) { + attachedFileIds.add(fileId); + } + } + } + } + return attachedFileIds; +} + +/** + * Checks if a user has access to multiple files through a shared agent (batch operation). + * Access is always scoped to files actually attached to the agent's tool_resources. * @param {Object} params - Parameters object * @param {string} params.userId - The user ID to check access for * @param {string} [params.role] - Optional user role to avoid DB query @@ -16,7 +35,6 @@ const { getAgent } = require('~/models/Agent'); const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDelete }) => { const accessMap = new Map(); - // Initialize all files as no access fileIds.forEach((fileId) => accessMap.set(fileId, false)); try { @@ -26,13 +44,17 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDele return accessMap; } - // Check if user is the author - if so, grant access to all files + const attachedFileIds = getAttachedFileIds(agent); + if (agent.author.toString() === userId.toString()) { - fileIds.forEach((fileId) => accessMap.set(fileId, true)); + fileIds.forEach((fileId) => { + if (attachedFileIds.has(fileId)) { + accessMap.set(fileId, true); + } + }); return accessMap; } - // Check if user has at least VIEW permission on the agent const hasViewPermission = await checkPermission({ userId, role, @@ -46,7 +68,6 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDele } if (isDelete) { - // Check if user has EDIT permission (which would indicate collaborative access) const hasEditPermission = await checkPermission({ userId, role, @@ -55,23 +76,11 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDele requiredPermission: PermissionBits.EDIT, }); - // If user only has VIEW permission, they can't access files - // Only users with EDIT permission or higher can access agent files if (!hasEditPermission) { return accessMap; } } - const attachedFileIds = new Set(); - if (agent.tool_resources) { - for (const [_resourceType, resource] of Object.entries(agent.tool_resources)) { - if (resource?.file_ids && Array.isArray(resource.file_ids)) { - resource.file_ids.forEach((fileId) => attachedFileIds.add(fileId)); - } - } - } - - // Grant access only to files that are attached to this agent fileIds.forEach((fileId) => { if (attachedFileIds.has(fileId)) { accessMap.set(fileId, true);