From 8772b04d1d505988d3e25adbe3c373f45c306d4f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 28 Aug 2025 21:16:23 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=83=EF=B8=8F=20refactor:=20File=20Acce?= =?UTF-8?q?ss=20via=20Agent;=20Deny=20Deletion=20if=20not=20Editor,=20Allo?= =?UTF-8?q?w=20Viewer=20(#9357)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/models/File.spec.js | 67 ++++++++++++++++++++++-- api/server/routes/files/files.js | 1 + api/server/services/Files/permissions.js | 30 ++++++----- 3 files changed, 80 insertions(+), 18 deletions(-) diff --git a/api/models/File.spec.js b/api/models/File.spec.js index dd0981d1f..c92224ea3 100644 --- a/api/models/File.spec.js +++ b/api/models/File.spec.js @@ -211,7 +211,67 @@ describe('File Access Control', () => { expect(accessMap.get(fileIds[1])).toBe(false); }); - it('should deny access when user only has VIEW permission', async () => { + it('should deny access when user only has VIEW permission and needs access for deletion', async () => { + const userId = new mongoose.Types.ObjectId(); + const authorId = new mongoose.Types.ObjectId(); + const agentId = uuidv4(); + const fileIds = [uuidv4(), uuidv4()]; + + // Create users + await User.create({ + _id: userId, + email: 'user@example.com', + emailVerified: true, + provider: 'local', + }); + + await User.create({ + _id: authorId, + email: 'author@example.com', + emailVerified: true, + provider: 'local', + }); + + // Create agent with files + const agent = await createAgent({ + id: agentId, + name: 'View-Only Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + tool_resources: { + file_search: { + file_ids: fileIds, + }, + }, + }); + + // Grant only VIEW permission to user on the agent + await grantPermission({ + principalType: PrincipalType.USER, + principalId: userId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: authorId, + }); + + // Check access for files + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + const accessMap = await hasAccessToFilesViaAgent({ + userId: userId, + role: SystemRoles.USER, + fileIds, + agentId, + isDelete: true, + }); + + // Should have no access to any files when only VIEW permission + expect(accessMap.get(fileIds[0])).toBe(false); + expect(accessMap.get(fileIds[1])).toBe(false); + }); + + it('should grant access when user has VIEW permission', async () => { const userId = new mongoose.Types.ObjectId(); const authorId = new mongoose.Types.ObjectId(); const agentId = uuidv4(); @@ -265,9 +325,8 @@ describe('File Access Control', () => { agentId, }); - // Should have no access to any files when only VIEW permission - expect(accessMap.get(fileIds[0])).toBe(false); - expect(accessMap.get(fileIds[1])).toBe(false); + expect(accessMap.get(fileIds[0])).toBe(true); + expect(accessMap.get(fileIds[1])).toBe(true); }); }); diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 09e229c96..5d6e72ff2 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -185,6 +185,7 @@ router.delete('/', async (req, res) => { role: req.user.role, fileIds: nonOwnedFileIds, agentId: req.body.agent_id, + isDelete: true, }); for (const file of nonOwnedFiles) { diff --git a/api/server/services/Files/permissions.js b/api/server/services/Files/permissions.js index 6a7296bbd..d909afe25 100644 --- a/api/server/services/Files/permissions.js +++ b/api/server/services/Files/permissions.js @@ -10,9 +10,10 @@ const { getAgent } = require('~/models/Agent'); * @param {string} [params.role] - Optional user role to avoid DB query * @param {string[]} params.fileIds - Array of file IDs to check * @param {string} params.agentId - The agent ID that might grant access + * @param {boolean} [params.isDelete] - Whether the operation is a delete operation * @returns {Promise>} Map of fileId to access status */ -const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId }) => { +const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDelete }) => { const accessMap = new Map(); // Initialize all files as no access @@ -44,22 +45,23 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId }) => { return accessMap; } - // Check if user has EDIT permission (which would indicate collaborative access) - const hasEditPermission = await checkPermission({ - userId, - role, - resourceType: ResourceType.AGENT, - resourceId: agent._id, - requiredPermission: PermissionBits.EDIT, - }); + if (isDelete) { + // Check if user has EDIT permission (which would indicate collaborative access) + const hasEditPermission = await checkPermission({ + userId, + role, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + 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; + // 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; + } } - // User has edit permissions - check which files are actually attached const attachedFileIds = new Set(); if (agent.tool_resources) { for (const [_resourceType, resource] of Object.entries(agent.tool_resources)) {