From ecd7bf0d514ea9441f0d9966d48ae7204bcf75b9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 3 Aug 2025 21:53:06 -0400 Subject: [PATCH] WIP: add user role check optimization to user principal check, update type comparisons --- api/app/clients/tools/util/fileSearch.js | 7 +- api/models/File.spec.js | 206 +++++++++++++++++- api/models/Prompt.spec.js | 6 +- .../controllers/PermissionsController.js | 1 + api/server/controllers/agents/v1.js | 5 +- .../canAccessAgentResource.spec.js | 2 +- .../accessResources/canAccessResource.js | 1 + .../middleware/accessResources/fileAccess.js | 7 +- api/server/routes/agents/actions.js | 1 + api/server/routes/files/files.js | 14 +- api/server/routes/files/files.test.js | 13 +- api/server/routes/prompts.js | 3 + api/server/services/Files/Code/process.js | 7 +- api/server/services/Files/permissions.js | 28 ++- api/server/services/PermissionService.js | 15 +- api/server/services/PermissionService.spec.js | 158 ++++++++++++++ .../src/methods/userGroup.roles.spec.ts | 38 +++- .../src/methods/userGroup.spec.ts | 8 +- .../data-schemas/src/methods/userGroup.ts | 32 ++- 19 files changed, 481 insertions(+), 71 deletions(-) diff --git a/api/app/clients/tools/util/fileSearch.js b/api/app/clients/tools/util/fileSearch.js index 7221e9e3f4..c7e7c88336 100644 --- a/api/app/clients/tools/util/fileSearch.js +++ b/api/app/clients/tools/util/fileSearch.js @@ -30,7 +30,12 @@ const primeFiles = async (options) => { // Filter by access if user and agent are provided let dbFiles; if (req?.user?.id && agentId) { - dbFiles = await filterFilesByAgentAccess(allFiles, req.user.id, agentId); + dbFiles = await filterFilesByAgentAccess({ + files: allFiles, + userId: req.user.id, + role: req.user.role, + agentId, + }); } else { dbFiles = allFiles; } diff --git a/api/models/File.spec.js b/api/models/File.spec.js index a07d3721af..dd0981d1fe 100644 --- a/api/models/File.spec.js +++ b/api/models/File.spec.js @@ -2,7 +2,12 @@ const mongoose = require('mongoose'); const { v4: uuidv4 } = require('uuid'); const { createModels } = require('@librechat/data-schemas'); const { MongoMemoryServer } = require('mongodb-memory-server'); -const { AccessRoleIds, ResourceType, PrincipalType } = require('librechat-data-provider'); +const { + SystemRoles, + ResourceType, + AccessRoleIds, + PrincipalType, +} = require('librechat-data-provider'); const { grantPermission } = require('~/server/services/PermissionService'); const { getFiles, createFile } = require('./File'); const { seedDefaultRoles } = require('~/models'); @@ -125,7 +130,12 @@ describe('File Access Control', () => { // Check access for all files const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); - const accessMap = await hasAccessToFilesViaAgent(userId.toString(), fileIds, agentId); + const accessMap = await hasAccessToFilesViaAgent({ + userId: userId, + role: SystemRoles.USER, + fileIds, + agentId: agent.id, // Use agent.id which is the custom UUID + }); // Should have access only to the first two files expect(accessMap.get(fileIds[0])).toBe(true); @@ -163,7 +173,12 @@ describe('File Access Control', () => { // Check access as the author const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); - const accessMap = await hasAccessToFilesViaAgent(authorId.toString(), fileIds, agentId); + const accessMap = await hasAccessToFilesViaAgent({ + userId: authorId, + role: SystemRoles.USER, + fileIds, + agentId, + }); // Author should have access to all files expect(accessMap.get(fileIds[0])).toBe(true); @@ -184,11 +199,12 @@ describe('File Access Control', () => { }); const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); - const accessMap = await hasAccessToFilesViaAgent( - userId.toString(), + const accessMap = await hasAccessToFilesViaAgent({ + userId: userId, + role: SystemRoles.USER, fileIds, - 'non-existent-agent', - ); + agentId: 'non-existent-agent', + }); // Should have no access to any files expect(accessMap.get(fileIds[0])).toBe(false); @@ -242,7 +258,12 @@ describe('File Access Control', () => { // Check access for files const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); - const accessMap = await hasAccessToFilesViaAgent(userId.toString(), fileIds, agentId); + const accessMap = await hasAccessToFilesViaAgent({ + userId: userId, + role: SystemRoles.USER, + fileIds, + agentId, + }); // Should have no access to any files when only VIEW permission expect(accessMap.get(fileIds[0])).toBe(false); @@ -336,7 +357,12 @@ describe('File Access Control', () => { // Then filter by access control const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); - const files = await filterFilesByAgentAccess(allFiles, userId.toString(), agentId); + const files = await filterFilesByAgentAccess({ + files: allFiles, + userId: userId, + role: SystemRoles.USER, + agentId, + }); expect(files).toHaveLength(2); expect(files.map((f) => f.file_id)).toContain(ownedFileId); @@ -371,4 +397,166 @@ describe('File Access Control', () => { expect(files).toHaveLength(2); }); }); + + describe('Role-based file permissions', () => { + it('should optimize permission checks when role is provided', 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', + role: 'ADMIN', // User has ADMIN role + }); + + await User.create({ + _id: authorId, + email: 'author@example.com', + emailVerified: true, + provider: 'local', + }); + + // Create files + for (const fileId of fileIds) { + await createFile({ + file_id: fileId, + user: authorId, + filename: `${fileId}.txt`, + filepath: `/uploads/${fileId}.txt`, + type: 'text/plain', + bytes: 100, + }); + } + + // Create agent with files + const agent = await createAgent({ + id: agentId, + name: 'Test Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + tool_resources: { + file_search: { + file_ids: fileIds, + }, + }, + }); + + // Grant permission to ADMIN role + await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: 'ADMIN', + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: authorId, + }); + + // Check access with role provided (should avoid DB query) + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + const accessMapWithRole = await hasAccessToFilesViaAgent({ + userId: userId, + role: 'ADMIN', + fileIds, + agentId: agent.id, + }); + + // User should have access through their ADMIN role + expect(accessMapWithRole.get(fileIds[0])).toBe(true); + expect(accessMapWithRole.get(fileIds[1])).toBe(true); + + // Check access without role (will query DB to get user's role) + const accessMapWithoutRole = await hasAccessToFilesViaAgent({ + userId: userId, + fileIds, + agentId: agent.id, + }); + + // Should have same result + expect(accessMapWithoutRole.get(fileIds[0])).toBe(true); + expect(accessMapWithoutRole.get(fileIds[1])).toBe(true); + }); + + it('should deny access when user role changes', async () => { + const userId = new mongoose.Types.ObjectId(); + const authorId = new mongoose.Types.ObjectId(); + const agentId = uuidv4(); + const fileId = uuidv4(); + + // Create users + await User.create({ + _id: userId, + email: 'user@example.com', + emailVerified: true, + provider: 'local', + role: 'EDITOR', + }); + + await User.create({ + _id: authorId, + email: 'author@example.com', + emailVerified: true, + provider: 'local', + }); + + // Create file + await createFile({ + file_id: fileId, + user: authorId, + filename: 'test.txt', + filepath: '/uploads/test.txt', + type: 'text/plain', + bytes: 100, + }); + + // Create agent + const agent = await createAgent({ + id: agentId, + name: 'Test Agent', + author: authorId, + model: 'gpt-4', + provider: 'openai', + tool_resources: { + file_search: { + file_ids: [fileId], + }, + }, + }); + + // Grant permission to EDITOR role only + await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: 'EDITOR', + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: authorId, + }); + + const { hasAccessToFilesViaAgent } = require('~/server/services/Files/permissions'); + + // Check with EDITOR role - should have access + const accessAsEditor = await hasAccessToFilesViaAgent({ + userId: userId, + role: 'EDITOR', + fileIds: [fileId], + agentId: agent.id, + }); + expect(accessAsEditor.get(fileId)).toBe(true); + + // Simulate role change to USER - should lose access + const accessAsUser = await hasAccessToFilesViaAgent({ + userId: userId, + role: SystemRoles.USER, + fileIds: [fileId], + agentId: agent.id, + }); + expect(accessAsUser.get(fileId)).toBe(false); + }); + }); }); diff --git a/api/models/Prompt.spec.js b/api/models/Prompt.spec.js index b3265bb340..e00a1a518c 100644 --- a/api/models/Prompt.spec.js +++ b/api/models/Prompt.spec.js @@ -334,10 +334,8 @@ describe('Prompt ACL Permissions', () => { productionId: new ObjectId(), }); - // Add editor to the editors group - await Group.findByIdAndUpdate(testGroups.editors._id, { - $push: { memberIds: testUsers.editor._id }, - }); + const { addUserToGroup } = require('~/models'); + await addUserToGroup(testUsers.editor._id, testGroups.editors._id); const prompt = await promptFns.savePrompt({ author: testUsers.owner._id, diff --git a/api/server/controllers/PermissionsController.js b/api/server/controllers/PermissionsController.js index 219b57e8ec..fab5593963 100644 --- a/api/server/controllers/PermissionsController.js +++ b/api/server/controllers/PermissionsController.js @@ -340,6 +340,7 @@ const getUserEffectivePermissions = async (req, res) => { const permissionBits = await getEffectivePermissions({ userId, + role: req.user.role, resourceType, resourceId, }); diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 7fae5ac78c..df009c0b0c 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -444,6 +444,7 @@ const getListAgentsHandler = async (req, res) => { // Get agent IDs the user has VIEW access to via ACL const accessibleIds = await findAccessibleResources({ userId, + role: req.user.role, resourceType: ResourceType.AGENT, requiredPermissions: requiredPermission, }); @@ -499,7 +500,7 @@ const uploadAgentAvatarHandler = async (req, res) => { return res.status(404).json({ error: 'Agent not found' }); } - const isAuthor = existingAgent.author.toString() === req.user.id; + const isAuthor = existingAgent.author.toString() === req.user.id.toString(); const hasEditPermission = existingAgent.isCollaborative || isAdmin || isAuthor; if (!hasEditPermission) { @@ -607,7 +608,7 @@ const revertAgentVersionHandler = async (req, res) => { return res.status(404).json({ error: 'Agent not found' }); } - const isAuthor = existingAgent.author.toString() === req.user.id; + const isAuthor = existingAgent.author.toString() === req.user.id.toString(); const hasEditPermission = existingAgent.isCollaborative || isAdmin || isAuthor; if (!hasEditPermission) { diff --git a/api/server/middleware/accessResources/canAccessAgentResource.spec.js b/api/server/middleware/accessResources/canAccessAgentResource.spec.js index c7f3efc6bb..e3dca73bd2 100644 --- a/api/server/middleware/accessResources/canAccessAgentResource.spec.js +++ b/api/server/middleware/accessResources/canAccessAgentResource.spec.js @@ -43,7 +43,7 @@ describe('canAccessAgentResource middleware', () => { }); req = { - user: { id: testUser._id.toString(), role: 'test-role' }, + user: { id: testUser._id, role: testUser.role }, params: {}, }; res = { diff --git a/api/server/middleware/accessResources/canAccessResource.js b/api/server/middleware/accessResources/canAccessResource.js index a236d3cffe..c8bd15ffc2 100644 --- a/api/server/middleware/accessResources/canAccessResource.js +++ b/api/server/middleware/accessResources/canAccessResource.js @@ -111,6 +111,7 @@ const canAccessResource = (options) => { // Check permissions using PermissionService with ObjectId const hasPermission = await checkPermission({ userId, + role: req.user.role, resourceType, resourceId, requiredPermission, diff --git a/api/server/middleware/accessResources/fileAccess.js b/api/server/middleware/accessResources/fileAccess.js index d2d084f3a5..2d588e623e 100644 --- a/api/server/middleware/accessResources/fileAccess.js +++ b/api/server/middleware/accessResources/fileAccess.js @@ -8,7 +8,7 @@ const { getFiles } = require('~/models/File'); * Checks if user has access to a file through agent permissions * Files inherit permissions from agents - if you can view the agent, you can access its files */ -const checkAgentBasedFileAccess = async (userId, fileId) => { +const checkAgentBasedFileAccess = async ({ userId, role, fileId }) => { try { // Find agents that have this file in their tool_resources const agentsWithFile = await getAgent({ @@ -35,6 +35,7 @@ const checkAgentBasedFileAccess = async (userId, fileId) => { try { const permissions = await getEffectivePermissions({ userId, + role, resourceType: ResourceType.AGENT, resourceId: agent._id || agent.id, }); @@ -67,7 +68,7 @@ const fileAccess = async (req, res, next) => { try { const fileId = req.params.file_id; const userId = req.user?.id; - + const userRole = req.user?.role; if (!fileId) { return res.status(400).json({ error: 'Bad Request', @@ -98,7 +99,7 @@ const fileAccess = async (req, res, next) => { } // Check agent-based access (file inherits agent permissions) - const hasAgentAccess = await checkAgentBasedFileAccess(userId, fileId); + const hasAgentAccess = await checkAgentBasedFileAccess({ userId, role: userRole, fileId }); if (hasAgentAccess) { req.fileAccess = { file }; return next(); diff --git a/api/server/routes/agents/actions.js b/api/server/routes/agents/actions.js index 8b7bf74e5e..c3a41bc771 100644 --- a/api/server/routes/agents/actions.js +++ b/api/server/routes/agents/actions.js @@ -37,6 +37,7 @@ router.get('/', async (req, res) => { const userId = req.user.id; const editableAgentObjectIds = await findAccessibleResources({ userId, + role: req.user.role, resourceType: ResourceType.AGENT, requiredPermissions: PermissionBits.EDIT, }); diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index a5bda3100d..1708d99536 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -79,6 +79,7 @@ router.get('/agent/:agent_id', async (req, res) => { if (agent.author.toString() !== userId) { const hasEditPermission = await checkPermission({ userId, + role: req.user.role, resourceType: ResourceType.AGENT, resourceId: agent._id, requiredPermission: PermissionBits.EDIT, @@ -152,7 +153,7 @@ router.delete('/', async (req, res) => { const nonOwnedFiles = []; for (const file of dbFiles) { - if (file.user.toString() === req.user.id) { + if (file.user.toString() === req.user.id.toString()) { ownedFiles.push(file); } else { nonOwnedFiles.push(file); @@ -176,11 +177,12 @@ router.delete('/', async (req, res) => { if (req.body.agent_id && nonOwnedFiles.length > 0) { const nonOwnedFileIds = nonOwnedFiles.map((f) => f.file_id); - const accessMap = await hasAccessToFilesViaAgent( - req.user.id, - nonOwnedFileIds, - req.body.agent_id, - ); + const accessMap = await hasAccessToFilesViaAgent({ + userId: req.user.id, + role: req.user.role, + fileIds: nonOwnedFileIds, + agentId: req.body.agent_id, + }); for (const file of nonOwnedFiles) { if (accessMap.get(file.file_id)) { diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index fbbd289595..896542b6f4 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -4,7 +4,12 @@ const mongoose = require('mongoose'); const { v4: uuidv4 } = require('uuid'); const { createMethods } = require('@librechat/data-schemas'); const { MongoMemoryServer } = require('mongodb-memory-server'); -const { AccessRoleIds, ResourceType, PrincipalType } = require('librechat-data-provider'); +const { + SystemRoles, + ResourceType, + AccessRoleIds, + PrincipalType, +} = require('librechat-data-provider'); const { createAgent } = require('~/models/Agent'); const { createFile } = require('~/models/File'); @@ -95,9 +100,11 @@ describe('File Routes - Delete with Agent Access', () => { app = express(); app.use(express.json()); - // Mock authentication middleware app.use((req, res, next) => { - req.user = { id: otherUserId ? otherUserId.toString() : 'default-user' }; + req.user = { + id: otherUserId || 'default-user', + role: SystemRoles.USER, + }; req.app = { locals: {} }; next(); }); diff --git a/api/server/routes/prompts.js b/api/server/routes/prompts.js index b5e5ea5039..6384ad1b1d 100644 --- a/api/server/routes/prompts.js +++ b/api/server/routes/prompts.js @@ -99,6 +99,7 @@ router.get('/all', async (req, res) => { // Get promptGroup IDs the user has VIEW access to via ACL const accessibleIds = await findAccessibleResources({ userId, + role: req.user.role, resourceType: ResourceType.PROMPTGROUP, requiredPermissions: PermissionBits.VIEW, }); @@ -130,6 +131,7 @@ router.get('/groups', async (req, res) => { // Get promptGroup IDs the user has VIEW access to via ACL const accessibleIds = await findAccessibleResources({ userId, + role: req.user.role, resourceType: ResourceType.PROMPTGROUP, requiredPermissions: PermissionBits.VIEW, }); @@ -334,6 +336,7 @@ router.get('/', async (req, res) => { if (groupId) { const permissions = await getEffectivePermissions({ userId: req.user.id, + role: req.user.role, resourceType: ResourceType.PROMPTGROUP, resourceId: groupId, }); diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index f3f5687b53..420451ab6d 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -172,7 +172,12 @@ const primeFiles = async (options, apiKey) => { // Filter by access if user and agent are provided let dbFiles; if (req?.user?.id && agentId) { - dbFiles = await filterFilesByAgentAccess(allFiles, req.user.id, agentId); + dbFiles = await filterFilesByAgentAccess({ + files: allFiles, + userId: req.user.id, + role: req.user.role, + agentId, + }); } else { dbFiles = allFiles; } diff --git a/api/server/services/Files/permissions.js b/api/server/services/Files/permissions.js index 37eaf50cc6..6a7296bbde 100644 --- a/api/server/services/Files/permissions.js +++ b/api/server/services/Files/permissions.js @@ -5,12 +5,14 @@ const { getAgent } = require('~/models/Agent'); /** * Checks if a user has access to multiple files through a shared agent (batch operation) - * @param {string} userId - The user ID to check access for - * @param {string[]} fileIds - Array of file IDs to check - * @param {string} agentId - The agent ID that might grant access + * @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 + * @param {string[]} params.fileIds - Array of file IDs to check + * @param {string} params.agentId - The agent ID that might grant access * @returns {Promise>} Map of fileId to access status */ -const hasAccessToFilesViaAgent = async (userId, fileIds, agentId) => { +const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId }) => { const accessMap = new Map(); // Initialize all files as no access @@ -24,7 +26,7 @@ const hasAccessToFilesViaAgent = async (userId, fileIds, agentId) => { } // Check if user is the author - if so, grant access to all files - if (agent.author.toString() === userId) { + if (agent.author.toString() === userId.toString()) { fileIds.forEach((fileId) => accessMap.set(fileId, true)); return accessMap; } @@ -32,6 +34,7 @@ const hasAccessToFilesViaAgent = async (userId, fileIds, agentId) => { // Check if user has at least VIEW permission on the agent const hasViewPermission = await checkPermission({ userId, + role, resourceType: ResourceType.AGENT, resourceId: agent._id, requiredPermission: PermissionBits.VIEW, @@ -44,6 +47,7 @@ const hasAccessToFilesViaAgent = async (userId, fileIds, agentId) => { // 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, @@ -81,12 +85,14 @@ const hasAccessToFilesViaAgent = async (userId, fileIds, agentId) => { /** * Filter files based on user access through agents - * @param {Array} files - Array of file documents - * @param {string} userId - User ID for access control - * @param {string} agentId - Agent ID that might grant access to files + * @param {Object} params - Parameters object + * @param {Array} params.files - Array of file documents + * @param {string} params.userId - User ID for access control + * @param {string} [params.role] - Optional user role to avoid DB query + * @param {string} params.agentId - Agent ID that might grant access to files * @returns {Promise>} Filtered array of accessible files */ -const filterFilesByAgentAccess = async (files, userId, agentId) => { +const filterFilesByAgentAccess = async ({ files, userId, role, agentId }) => { if (!userId || !agentId || !files || files.length === 0) { return files; } @@ -96,7 +102,7 @@ const filterFilesByAgentAccess = async (files, userId, agentId) => { const ownedFiles = []; for (const file of files) { - if (file.user && file.user.toString() === userId) { + if (file.user && file.user.toString() === userId.toString()) { ownedFiles.push(file); } else { filesToCheck.push(file); @@ -109,7 +115,7 @@ const filterFilesByAgentAccess = async (files, userId, agentId) => { // Batch check access for all non-owned files const fileIds = filesToCheck.map((f) => f.file_id); - const accessMap = await hasAccessToFilesViaAgent(userId, fileIds, agentId); + const accessMap = await hasAccessToFilesViaAgent({ userId, role, fileIds, agentId }); // Filter files based on access const accessibleFiles = filesToCheck.filter((file) => accessMap.get(file.file_id)); diff --git a/api/server/services/PermissionService.js b/api/server/services/PermissionService.js index ef1665949a..240c532b60 100644 --- a/api/server/services/PermissionService.js +++ b/api/server/services/PermissionService.js @@ -126,12 +126,13 @@ const grantPermission = async ({ * Check if a user has specific permission bits on a resource * @param {Object} params - Parameters for checking permissions * @param {string|mongoose.Types.ObjectId} params.userId - The ID of the user + * @param {string} [params.role] - Optional user role (if not provided, will query from DB) * @param {string} params.resourceType - Type of resource (e.g., 'agent') * @param {string|mongoose.Types.ObjectId} params.resourceId - The ID of the resource * @param {number} params.requiredPermissions - The permission bits required (e.g., 1 for VIEW, 3 for VIEW+EDIT) * @returns {Promise} Whether the user has the required permission bits */ -const checkPermission = async ({ userId, resourceType, resourceId, requiredPermission }) => { +const checkPermission = async ({ userId, role, resourceType, resourceId, requiredPermission }) => { try { if (typeof requiredPermission !== 'number' || requiredPermission < 1) { throw new Error('requiredPermission must be a positive number'); @@ -140,7 +141,7 @@ const checkPermission = async ({ userId, resourceType, resourceId, requiredPermi validateResourceType(resourceType); // Get all principals for the user (user + groups + public) - const principals = await getUserPrincipals(userId); + const principals = await getUserPrincipals({ userId, role }); if (principals.length === 0) { return false; @@ -161,16 +162,17 @@ const checkPermission = async ({ userId, resourceType, resourceId, requiredPermi * Get effective permission bitmask for a user on a resource * @param {Object} params - Parameters for getting effective permissions * @param {string|mongoose.Types.ObjectId} params.userId - The ID of the user + * @param {string} [params.role] - Optional user role (if not provided, will query from DB) * @param {string} params.resourceType - Type of resource (e.g., 'agent') * @param {string|mongoose.Types.ObjectId} params.resourceId - The ID of the resource * @returns {Promise} Effective permission bitmask */ -const getEffectivePermissions = async ({ userId, resourceType, resourceId }) => { +const getEffectivePermissions = async ({ userId, role, resourceType, resourceId }) => { try { validateResourceType(resourceType); // Get all principals for the user (user + groups + public) - const principals = await getUserPrincipals(userId); + const principals = await getUserPrincipals({ userId, role }); if (principals.length === 0) { return 0; @@ -186,11 +188,12 @@ const getEffectivePermissions = async ({ userId, resourceType, resourceId }) => * Find all resources of a specific type that a user has access to with specific permission bits * @param {Object} params - Parameters for finding accessible resources * @param {string|mongoose.Types.ObjectId} params.userId - The ID of the user + * @param {string} [params.role] - Optional user role (if not provided, will query from DB) * @param {string} params.resourceType - Type of resource (e.g., 'agent') * @param {number} params.requiredPermissions - The minimum permission bits required (e.g., 1 for VIEW, 3 for VIEW+EDIT) * @returns {Promise} Array of resource IDs */ -const findAccessibleResources = async ({ userId, resourceType, requiredPermissions }) => { +const findAccessibleResources = async ({ userId, role, resourceType, requiredPermissions }) => { try { if (typeof requiredPermissions !== 'number' || requiredPermissions < 1) { throw new Error('requiredPermissions must be a positive number'); @@ -199,7 +202,7 @@ const findAccessibleResources = async ({ userId, resourceType, requiredPermissio validateResourceType(resourceType); // Get all principals for the user (user + groups + public) - const principalsList = await getUserPrincipals(userId); + const principalsList = await getUserPrincipals({ userId, role }); if (principalsList.length === 0) { return []; diff --git a/api/server/services/PermissionService.spec.js b/api/server/services/PermissionService.spec.js index 9b73cd79f0..9836859198 100644 --- a/api/server/services/PermissionService.spec.js +++ b/api/server/services/PermissionService.spec.js @@ -1067,6 +1067,164 @@ describe('PermissionService', () => { expect(hasNoPermission).toBe(false); }); + test('should optimize permission checks when role is provided', async () => { + const testUserId = new mongoose.Types.ObjectId(); + const testResourceId = new mongoose.Types.ObjectId(); + + // Create a user with EDITOR role + const User = mongoose.models.User; + await User.create({ + _id: testUserId, + email: 'editor@test.com', + emailVerified: true, + provider: 'local', + role: 'EDITOR', + }); + + // Grant permission to EDITOR role + await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: 'EDITOR', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: grantedById, + }); + + // Mock getUserPrincipals to return user with EDITOR role when called + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: testUserId }, + { principalType: PrincipalType.ROLE, principalId: 'EDITOR' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + // Test 1: Check permission with role provided (optimization should be used) + const hasPermissionWithRole = await checkPermission({ + userId: testUserId, + role: 'EDITOR', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 1, // VIEW + }); + + expect(hasPermissionWithRole).toBe(true); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'EDITOR' }); + + // Test 2: Check permission without role (should call getUserPrincipals) + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: testUserId }, + { principalType: PrincipalType.ROLE, principalId: 'EDITOR' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const hasPermissionWithoutRole = await checkPermission({ + userId: testUserId, + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 1, // VIEW + }); + + expect(hasPermissionWithoutRole).toBe(true); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: undefined }); + + // Test 3: Verify getEffectivePermissions also uses the optimization + getUserPrincipals.mockClear(); + + const effectiveWithRole = await getEffectivePermissions({ + userId: testUserId, + role: 'EDITOR', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + }); + + expect(effectiveWithRole).toBe(3); // EDITOR = VIEW + EDIT + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'EDITOR' }); + + // Test 4: Verify findAccessibleResources also uses the optimization + getUserPrincipals.mockClear(); + + const accessibleWithRole = await findAccessibleResources({ + userId: testUserId, + role: 'EDITOR', + resourceType: ResourceType.AGENT, + requiredPermissions: 1, // VIEW + }); + + expect(accessibleWithRole.map((id) => id.toString())).toContain(testResourceId.toString()); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'EDITOR' }); + }); + + test('should handle role changes dynamically', async () => { + const testUserId = new mongoose.Types.ObjectId(); + const testResourceId = new mongoose.Types.ObjectId(); + + // Grant permission to ADMIN role only + await grantPermission({ + principalType: PrincipalType.ROLE, + principalId: 'ADMIN', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: grantedById, + }); + + // Test with ADMIN role - should have access + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: testUserId }, + { principalType: PrincipalType.ROLE, principalId: 'ADMIN' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const hasAdminAccess = await checkPermission({ + userId: testUserId, + role: 'ADMIN', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 7, // Full permissions + }); + + expect(hasAdminAccess).toBe(true); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'ADMIN' }); + + // Test with USER role - should NOT have access + getUserPrincipals.mockClear(); + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: testUserId }, + { principalType: PrincipalType.ROLE, principalId: 'USER' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const hasUserAccess = await checkPermission({ + userId: testUserId, + role: 'USER', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 1, // Even VIEW + }); + + expect(hasUserAccess).toBe(false); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'USER' }); + + // Test with EDITOR role - should NOT have access + getUserPrincipals.mockClear(); + getUserPrincipals.mockResolvedValue([ + { principalType: PrincipalType.USER, principalId: testUserId }, + { principalType: PrincipalType.ROLE, principalId: 'EDITOR' }, + { principalType: PrincipalType.PUBLIC }, + ]); + + const hasEditorAccess = await checkPermission({ + userId: testUserId, + role: 'EDITOR', + resourceType: ResourceType.AGENT, + resourceId: testResourceId, + requiredPermission: 1, // VIEW + }); + + expect(hasEditorAccess).toBe(false); + expect(getUserPrincipals).toHaveBeenCalledWith({ userId: testUserId, role: 'EDITOR' }); + }); + test('should work with different resource types', async () => { // Test with promptGroup resources const promptGroupResourceId = new mongoose.Types.ObjectId(); diff --git a/packages/data-schemas/src/methods/userGroup.roles.spec.ts b/packages/data-schemas/src/methods/userGroup.roles.spec.ts index 5307f2dc34..fa50c6f4c4 100644 --- a/packages/data-schemas/src/methods/userGroup.roles.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.roles.spec.ts @@ -53,7 +53,9 @@ describe('Role-based Permissions Integration', () => { role: 'admin', }); - const principals = await methods.getUserPrincipals(adminUser._id as mongoose.Types.ObjectId); + const principals = await methods.getUserPrincipals({ + userId: adminUser._id as mongoose.Types.ObjectId, + }); // Should have user, role, and public principals expect(principals).toHaveLength(3); @@ -84,9 +86,9 @@ describe('Role-based Permissions Integration', () => { role: null, // Explicitly set to null to override default }); - const principals = await methods.getUserPrincipals( - regularUser._id as mongoose.Types.ObjectId, - ); + const principals = await methods.getUserPrincipals({ + userId: regularUser._id as mongoose.Types.ObjectId, + }); // Should only have user and public principals expect(principals).toHaveLength(2); @@ -116,7 +118,9 @@ describe('Role-based Permissions Integration', () => { memberIds: [(user._id as mongoose.Types.ObjectId).toString()], }); - const principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + const principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); // Should have user, role, 2 groups, and public expect(principals).toHaveLength(5); @@ -156,7 +160,9 @@ describe('Role-based Permissions Integration', () => { role: testCase.role, }); - const principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + const principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); const rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); expect(rolePrincipal).toBeDefined(); @@ -292,7 +298,9 @@ describe('Role-based Permissions Integration', () => { }); // Initial principals - let principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + let principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); let rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); expect(rolePrincipal?.principalId).toBe('viewer'); @@ -301,7 +309,9 @@ describe('Role-based Permissions Integration', () => { await user.save(); // Get principals again - principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); expect(rolePrincipal?.principalId).toBe('editor'); }); @@ -315,7 +325,9 @@ describe('Role-based Permissions Integration', () => { }); // Initial check - let principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + let principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); expect(principals).toHaveLength(3); // user, role, public // Remove role @@ -323,7 +335,9 @@ describe('Role-based Permissions Integration', () => { await user.save(); // Check again - principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); expect(principals).toHaveLength(2); // user, public const rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); expect(rolePrincipal).toBeUndefined(); @@ -352,7 +366,9 @@ describe('Role-based Permissions Integration', () => { const user = await User.create(userData); - const principals = await methods.getUserPrincipals(user._id as mongoose.Types.ObjectId); + const principals = await methods.getUserPrincipals({ + userId: user._id as mongoose.Types.ObjectId, + }); const rolePrincipal = principals.find((p) => p.principalType === PrincipalType.ROLE); if (testCase.expected) { diff --git a/packages/data-schemas/src/methods/userGroup.spec.ts b/packages/data-schemas/src/methods/userGroup.spec.ts index bb7514c67e..92a957792a 100644 --- a/packages/data-schemas/src/methods/userGroup.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.spec.ts @@ -325,7 +325,9 @@ describe('User Group Methods Tests', () => { ); /** Get user principals */ - const principals = await methods.getUserPrincipals(testUser1._id as mongoose.Types.ObjectId); + const principals = await methods.getUserPrincipals({ + userId: testUser1._id as mongoose.Types.ObjectId, + }); /** Should include user, role (default USER), group, and public principals */ expect(principals).toHaveLength(4); @@ -349,7 +351,9 @@ describe('User Group Methods Tests', () => { test('should return user and public principals for non-existent user in getUserPrincipals', async () => { const nonExistentId = new mongoose.Types.ObjectId(); - const principals = await methods.getUserPrincipals(nonExistentId); + const principals = await methods.getUserPrincipals({ + userId: nonExistentId, + }); /** Should still return user and public principals even for non-existent user */ expect(principals).toHaveLength(2); diff --git a/packages/data-schemas/src/methods/userGroup.ts b/packages/data-schemas/src/methods/userGroup.ts index dc49e9b8cd..f61469d825 100644 --- a/packages/data-schemas/src/methods/userGroup.ts +++ b/packages/data-schemas/src/methods/userGroup.ts @@ -237,35 +237,45 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { /** * Get a list of all principal identifiers for a user (user ID + group IDs + public) * For use in permission checks - * @param userId - The user ID + * @param params - Parameters object + * @param params.userId - The user ID + * @param params.role - Optional user role (if not provided, will query from DB) * @param session - Optional MongoDB session for transactions * @returns Array of principal objects with type and id */ async function getUserPrincipals( - userId: string | Types.ObjectId, + params: { + userId: string | Types.ObjectId; + role?: string | null; + }, session?: ClientSession, ): Promise> { + const { userId, role } = params; const principals: Array<{ principalType: string; principalId?: string | Types.ObjectId }> = [ { principalType: PrincipalType.USER, principalId: userId }, ]; - // Get user to check their role - const User = mongoose.models.User as Model; - const query = User.findById(userId).select('role'); - if (session) { - query.session(session); + // If role is not provided, query user to get it + let userRole = role; + if (userRole === undefined) { + const User = mongoose.models.User as Model; + const query = User.findById(userId).select('role'); + if (session) { + query.session(session); + } + const user = await query.lean(); + userRole = user?.role; } - const user = await query.lean(); // Add role as a principal if user has one - if (user?.role && user.role.trim()) { - principals.push({ principalType: PrincipalType.ROLE, principalId: user.role }); + if (userRole && userRole.trim()) { + principals.push({ principalType: PrincipalType.ROLE, principalId: userRole }); } const userGroups = await getUserGroups(userId, session); if (userGroups && userGroups.length > 0) { userGroups.forEach((group) => { - principals.push({ principalType: PrincipalType.GROUP, principalId: group._id.toString() }); + principals.push({ principalType: PrincipalType.GROUP, principalId: group._id }); }); }