diff --git a/api/models/Agent.js b/api/models/Agent.js index 663285183a..53098888d6 100644 --- a/api/models/Agent.js +++ b/api/models/Agent.js @@ -17,7 +17,10 @@ const { removeAgentIdsFromProject, addAgentIdsToProject, } = require('./Project'); -const { removeAllPermissions } = require('~/server/services/PermissionService'); +const { + getSoleOwnedResourceIds, + removeAllPermissions, +} = require('~/server/services/PermissionService'); const { getMCPServerTools } = require('~/server/services/Config'); const { Agent, AclEntry, User } = require('~/db/models'); const { getActions } = require('./Action'); @@ -617,30 +620,70 @@ const deleteAgent = async (searchParameter) => { }; /** - * Deletes all agents created by a specific user. + * Deletes agents solely owned by the user and cleans up their ACLs/project references. + * Agents with other owners are left intact; the caller is responsible for + * removing the user's own ACL principal entries separately. + * + * Also handles legacy (pre-ACL) agents that only have the author field set, + * ensuring they are not orphaned if no permission migration has been run. * @param {string} userId - The ID of the user whose agents should be deleted. - * @returns {Promise} A promise that resolves when all user agents have been deleted. + * @returns {Promise} */ const deleteUserAgents = async (userId) => { try { - const userAgents = await getAgents({ author: userId }); + const userObjectId = new mongoose.Types.ObjectId(userId); + const soleOwnedObjectIds = await getSoleOwnedResourceIds(userObjectId, [ + ResourceType.AGENT, + ResourceType.REMOTE_AGENT, + ]); - if (userAgents.length === 0) { + const authoredAgents = await Agent.find({ author: userObjectId }).select('id _id').lean(); + + const migratedEntries = + authoredAgents.length > 0 + ? await AclEntry.find({ + resourceType: { $in: [ResourceType.AGENT, ResourceType.REMOTE_AGENT] }, + resourceId: { $in: authoredAgents.map((a) => a._id) }, + }) + .select('resourceId') + .lean() + : []; + const migratedIds = new Set(migratedEntries.map((e) => e.resourceId.toString())); + const legacyAgents = authoredAgents.filter((a) => !migratedIds.has(a._id.toString())); + + /** resourceId is the MongoDB _id; agent.id is the string identifier for project/edge queries */ + const soleOwnedAgents = + soleOwnedObjectIds.length > 0 + ? await Agent.find({ _id: { $in: soleOwnedObjectIds } }) + .select('id _id') + .lean() + : []; + + const allAgents = [...soleOwnedAgents, ...legacyAgents]; + + if (allAgents.length === 0) { return; } - const agentIds = userAgents.map((agent) => agent.id); - const agentObjectIds = userAgents.map((agent) => agent._id); + const agentIds = allAgents.map((agent) => agent.id); + const agentObjectIds = allAgents.map((agent) => agent._id); - for (const agentId of agentIds) { - await removeAgentFromAllProjects(agentId); - } + await Promise.all(agentIds.map((id) => removeAgentFromAllProjects(id))); await AclEntry.deleteMany({ resourceType: { $in: [ResourceType.AGENT, ResourceType.REMOTE_AGENT] }, resourceId: { $in: agentObjectIds }, }); + try { + await Agent.updateMany( + { 'edges.to': { $in: agentIds } }, + { $pull: { edges: { to: { $in: agentIds } } } }, + ); + } catch (error) { + logger.error('[deleteUserAgents] Error removing agents from handoff edges', error); + } + try { await User.updateMany( { 'favorites.agentId': { $in: agentIds } }, @@ -650,7 +693,7 @@ const deleteUserAgents = async (userId) => { logger.error('[deleteUserAgents] Error removing agents from user favorites', error); } - await Agent.deleteMany({ author: userId }); + await Agent.deleteMany({ _id: { $in: agentObjectIds } }); } catch (error) { logger.error('[deleteUserAgents] General error:', error); } diff --git a/api/models/Agent.spec.js b/api/models/Agent.spec.js index baceb3e8f3..b2597872ab 100644 --- a/api/models/Agent.spec.js +++ b/api/models/Agent.spec.js @@ -15,7 +15,12 @@ const mongoose = require('mongoose'); const { v4: uuidv4 } = require('uuid'); const { agentSchema } = require('@librechat/data-schemas'); const { MongoMemoryServer } = require('mongodb-memory-server'); -const { AccessRoleIds, ResourceType, PrincipalType } = require('librechat-data-provider'); +const { + ResourceType, + AccessRoleIds, + PrincipalType, + PermissionBits, +} = require('librechat-data-provider'); const { getAgent, loadAgent, @@ -442,6 +447,7 @@ describe('models/Agent', () => { beforeEach(async () => { await Agent.deleteMany({}); + await AclEntry.deleteMany({}); }); test('should create and get an agent', async () => { @@ -838,8 +844,7 @@ describe('models/Agent', () => { const agent2Id = `agent_${uuidv4()}`; const otherAuthorAgentId = `agent_${uuidv4()}`; - // Create agents by the author to be deleted - await createAgent({ + const agent1 = await createAgent({ id: agent1Id, name: 'Author Agent 1', provider: 'test', @@ -847,7 +852,7 @@ describe('models/Agent', () => { author: authorId, }); - await createAgent({ + const agent2 = await createAgent({ id: agent2Id, name: 'Author Agent 2', provider: 'test', @@ -855,7 +860,6 @@ describe('models/Agent', () => { author: authorId, }); - // Create agent by different author (should not be deleted) await createAgent({ id: otherAuthorAgentId, name: 'Other Author Agent', @@ -864,7 +868,23 @@ describe('models/Agent', () => { author: otherAuthorId, }); - // Create user with all agents in favorites + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent1._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent2._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await User.create({ _id: userId, name: 'Test User', @@ -878,21 +898,16 @@ describe('models/Agent', () => { ], }); - // Verify user has all favorites const userBefore = await User.findById(userId); expect(userBefore.favorites).toHaveLength(4); - // Delete all agents by the author await deleteUserAgents(authorId.toString()); - // Verify author's agents are deleted from database expect(await getAgent({ id: agent1Id })).toBeNull(); expect(await getAgent({ id: agent2Id })).toBeNull(); - // Verify other author's agent still exists expect(await getAgent({ id: otherAuthorAgentId })).not.toBeNull(); - // Verify user favorites: author's agents removed, others remain const userAfter = await User.findById(userId); expect(userAfter.favorites).toHaveLength(2); expect(userAfter.favorites.some((f) => f.agentId === agent1Id)).toBe(false); @@ -911,8 +926,7 @@ describe('models/Agent', () => { const agent2Id = `agent_${uuidv4()}`; const unrelatedAgentId = `agent_${uuidv4()}`; - // Create agents by the author - await createAgent({ + const agent1 = await createAgent({ id: agent1Id, name: 'Author Agent 1', provider: 'test', @@ -920,7 +934,7 @@ describe('models/Agent', () => { author: authorId, }); - await createAgent({ + const agent2 = await createAgent({ id: agent2Id, name: 'Author Agent 2', provider: 'test', @@ -928,7 +942,23 @@ describe('models/Agent', () => { author: authorId, }); - // Create users with various favorites configurations + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent1._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent2._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await User.create({ _id: user1Id, name: 'User 1', @@ -953,10 +983,8 @@ describe('models/Agent', () => { favorites: [{ agentId: unrelatedAgentId }, { model: 'gpt-4', endpoint: 'openAI' }], }); - // Delete all agents by the author await deleteUserAgents(authorId.toString()); - // Verify all users' favorites are correctly updated const user1After = await User.findById(user1Id); expect(user1After.favorites).toHaveLength(0); @@ -965,7 +993,6 @@ describe('models/Agent', () => { expect(user2After.favorites.some((f) => f.agentId === agent1Id)).toBe(false); expect(user2After.favorites.some((f) => f.model === 'claude-3')).toBe(true); - // User 3 should be completely unaffected const user3After = await User.findById(user3Id); expect(user3After.favorites).toHaveLength(2); expect(user3After.favorites.some((f) => f.agentId === unrelatedAgentId)).toBe(true); @@ -979,8 +1006,7 @@ describe('models/Agent', () => { const existingAgentId = `agent_${uuidv4()}`; - // Create agent by different author - await createAgent({ + const existingAgent = await createAgent({ id: existingAgentId, name: 'Existing Agent', provider: 'test', @@ -988,7 +1014,15 @@ describe('models/Agent', () => { author: otherAuthorId, }); - // Create user with favorites + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherAuthorId, + resourceType: ResourceType.AGENT, + resourceId: existingAgent._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: otherAuthorId, + }); + await User.create({ _id: userId, name: 'Test User', @@ -997,13 +1031,10 @@ describe('models/Agent', () => { favorites: [{ agentId: existingAgentId }, { model: 'gpt-4', endpoint: 'openAI' }], }); - // Delete agents for user with no agents (should be a no-op) await deleteUserAgents(authorWithNoAgentsId.toString()); - // Verify existing agent still exists expect(await getAgent({ id: existingAgentId })).not.toBeNull(); - // Verify user favorites are unchanged const userAfter = await User.findById(userId); expect(userAfter.favorites).toHaveLength(2); expect(userAfter.favorites.some((f) => f.agentId === existingAgentId)).toBe(true); @@ -1017,8 +1048,7 @@ describe('models/Agent', () => { const agent1Id = `agent_${uuidv4()}`; const agent2Id = `agent_${uuidv4()}`; - // Create agents by the author - await createAgent({ + const agent1 = await createAgent({ id: agent1Id, name: 'Agent 1', provider: 'test', @@ -1026,7 +1056,7 @@ describe('models/Agent', () => { author: authorId, }); - await createAgent({ + const agent2 = await createAgent({ id: agent2Id, name: 'Agent 2', provider: 'test', @@ -1034,7 +1064,23 @@ describe('models/Agent', () => { author: authorId, }); - // Create user with favorites that don't include these agents + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent1._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: authorId, + resourceType: ResourceType.AGENT, + resourceId: agent2._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: authorId, + }); + await User.create({ _id: userId, name: 'Test User', @@ -1043,23 +1089,112 @@ describe('models/Agent', () => { favorites: [{ model: 'gpt-4', endpoint: 'openAI' }], }); - // Verify agents exist expect(await getAgent({ id: agent1Id })).not.toBeNull(); expect(await getAgent({ id: agent2Id })).not.toBeNull(); - // Delete all agents by the author await deleteUserAgents(authorId.toString()); - // Verify agents are deleted expect(await getAgent({ id: agent1Id })).toBeNull(); expect(await getAgent({ id: agent2Id })).toBeNull(); - // Verify user favorites are unchanged const userAfter = await User.findById(userId); expect(userAfter.favorites).toHaveLength(1); expect(userAfter.favorites.some((f) => f.model === 'gpt-4')).toBe(true); }); + test('should preserve multi-owned agents when deleteUserAgents is called', async () => { + const deletingUserId = new mongoose.Types.ObjectId(); + const otherOwnerId = new mongoose.Types.ObjectId(); + + const soleOwnedId = `agent_${uuidv4()}`; + const multiOwnedId = `agent_${uuidv4()}`; + + const soleAgent = await createAgent({ + id: soleOwnedId, + name: 'Sole Owned Agent', + provider: 'test', + model: 'test-model', + author: deletingUserId, + }); + + const multiAgent = await createAgent({ + id: multiOwnedId, + name: 'Multi Owned Agent', + provider: 'test', + model: 'test-model', + author: deletingUserId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUserId, + resourceType: ResourceType.AGENT, + resourceId: soleAgent._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: deletingUserId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUserId, + resourceType: ResourceType.AGENT, + resourceId: multiAgent._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: deletingUserId, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherOwnerId, + resourceType: ResourceType.AGENT, + resourceId: multiAgent._id, + accessRoleId: AccessRoleIds.AGENT_OWNER, + grantedBy: otherOwnerId, + }); + + await deleteUserAgents(deletingUserId.toString()); + + expect(await getAgent({ id: soleOwnedId })).toBeNull(); + expect(await getAgent({ id: multiOwnedId })).not.toBeNull(); + + const soleAcl = await AclEntry.find({ + resourceType: ResourceType.AGENT, + resourceId: soleAgent._id, + }); + expect(soleAcl).toHaveLength(0); + + const multiAcl = await AclEntry.find({ + resourceType: ResourceType.AGENT, + resourceId: multiAgent._id, + principalId: otherOwnerId, + }); + expect(multiAcl).toHaveLength(1); + expect(multiAcl[0].permBits & PermissionBits.DELETE).toBeTruthy(); + + const deletingUserMultiAcl = await AclEntry.find({ + resourceType: ResourceType.AGENT, + resourceId: multiAgent._id, + principalId: deletingUserId, + }); + expect(deletingUserMultiAcl).toHaveLength(1); + }); + + test('should delete legacy agents that have author but no ACL entries', async () => { + const legacyUserId = new mongoose.Types.ObjectId(); + const legacyAgentId = `agent_${uuidv4()}`; + + await createAgent({ + id: legacyAgentId, + name: 'Legacy Agent (no ACL)', + provider: 'test', + model: 'test-model', + author: legacyUserId, + }); + + await deleteUserAgents(legacyUserId.toString()); + + expect(await getAgent({ id: legacyAgentId })).toBeNull(); + }); + test('should update agent projects', async () => { const agentId = `agent_${uuidv4()}`; const authorId = new mongoose.Types.ObjectId(); diff --git a/api/models/Prompt.js b/api/models/Prompt.js index bde911b23a..4b14edbc74 100644 --- a/api/models/Prompt.js +++ b/api/models/Prompt.js @@ -13,7 +13,10 @@ const { addGroupIdsToProject, getProjectByName, } = require('./Project'); -const { removeAllPermissions } = require('~/server/services/PermissionService'); +const { + getSoleOwnedResourceIds, + removeAllPermissions, +} = require('~/server/services/PermissionService'); const { PromptGroup, Prompt, AclEntry } = require('~/db/models'); /** @@ -592,31 +595,49 @@ module.exports = { } }, /** - * Delete all prompts and prompt groups created by a specific user. - * @param {ServerRequest} req - The server request object. + * Delete prompt groups solely owned by the user and clean up their prompts/ACLs. + * Groups with other owners are left intact; the caller is responsible for + * removing the user's own ACL principal entries separately. + * + * Also handles legacy (pre-ACL) prompt groups that only have the author field set, + * ensuring they are not orphaned if the permission migration has not been run. * @param {string} userId - The ID of the user whose prompts and prompt groups are to be deleted. */ - deleteUserPrompts: async (req, userId) => { + deleteUserPrompts: async (userId) => { try { - const promptGroups = await getAllPromptGroups(req, { author: new ObjectId(userId) }); + const userObjectId = new ObjectId(userId); + const soleOwnedIds = await getSoleOwnedResourceIds(userObjectId, ResourceType.PROMPTGROUP); - if (promptGroups.length === 0) { + const authoredGroups = await PromptGroup.find({ author: userObjectId }).select('_id').lean(); + const authoredGroupIds = authoredGroups.map((g) => g._id); + + const migratedEntries = + authoredGroupIds.length > 0 + ? await AclEntry.find({ + resourceType: ResourceType.PROMPTGROUP, + resourceId: { $in: authoredGroupIds }, + }) + .select('resourceId') + .lean() + : []; + const migratedIds = new Set(migratedEntries.map((e) => e.resourceId.toString())); + const legacyGroupIds = authoredGroupIds.filter((id) => !migratedIds.has(id.toString())); + + const allGroupIdsToDelete = [...soleOwnedIds, ...legacyGroupIds]; + + if (allGroupIdsToDelete.length === 0) { return; } - const groupIds = promptGroups.map((group) => group._id); - - for (const groupId of groupIds) { - await removeGroupFromAllProjects(groupId); - } + await Promise.all(allGroupIdsToDelete.map((id) => removeGroupFromAllProjects(id))); await AclEntry.deleteMany({ resourceType: ResourceType.PROMPTGROUP, - resourceId: { $in: groupIds }, + resourceId: { $in: allGroupIdsToDelete }, }); - await PromptGroup.deleteMany({ author: new ObjectId(userId) }); - await Prompt.deleteMany({ author: new ObjectId(userId) }); + await PromptGroup.deleteMany({ _id: { $in: allGroupIdsToDelete } }); + await Prompt.deleteMany({ groupId: { $in: allGroupIdsToDelete } }); } catch (error) { logger.error('[deleteUserPrompts] General error:', error); } diff --git a/api/models/Prompt.spec.js b/api/models/Prompt.spec.js index e00a1a518c..a2063e6cfc 100644 --- a/api/models/Prompt.spec.js +++ b/api/models/Prompt.spec.js @@ -561,4 +561,231 @@ describe('Prompt ACL Permissions', () => { expect(prompt._id.toString()).toBe(legacyPrompt._id.toString()); }); }); + + describe('deleteUserPrompts', () => { + let deletingUser; + let otherUser; + let soleOwnedGroup; + let multiOwnedGroup; + let sharedGroup; + let soleOwnedPrompt; + let multiOwnedPrompt; + let sharedPrompt; + + beforeAll(async () => { + deletingUser = await User.create({ + name: 'Deleting User', + email: 'deleting@example.com', + role: SystemRoles.USER, + }); + otherUser = await User.create({ + name: 'Other User', + email: 'other@example.com', + role: SystemRoles.USER, + }); + + const soleProductionId = new ObjectId(); + soleOwnedGroup = await PromptGroup.create({ + name: 'Sole Owned Group', + author: deletingUser._id, + authorName: deletingUser.name, + productionId: soleProductionId, + }); + soleOwnedPrompt = await Prompt.create({ + prompt: 'Sole owned prompt', + author: deletingUser._id, + groupId: soleOwnedGroup._id, + type: 'text', + }); + await PromptGroup.updateOne( + { _id: soleOwnedGroup._id }, + { productionId: soleOwnedPrompt._id }, + ); + + const multiProductionId = new ObjectId(); + multiOwnedGroup = await PromptGroup.create({ + name: 'Multi Owned Group', + author: deletingUser._id, + authorName: deletingUser.name, + productionId: multiProductionId, + }); + multiOwnedPrompt = await Prompt.create({ + prompt: 'Multi owned prompt', + author: deletingUser._id, + groupId: multiOwnedGroup._id, + type: 'text', + }); + await PromptGroup.updateOne( + { _id: multiOwnedGroup._id }, + { productionId: multiOwnedPrompt._id }, + ); + + const sharedProductionId = new ObjectId(); + sharedGroup = await PromptGroup.create({ + name: 'Shared Group (other user owns)', + author: otherUser._id, + authorName: otherUser.name, + productionId: sharedProductionId, + }); + sharedPrompt = await Prompt.create({ + prompt: 'Shared prompt', + author: otherUser._id, + groupId: sharedGroup._id, + type: 'text', + }); + await PromptGroup.updateOne({ _id: sharedGroup._id }, { productionId: sharedPrompt._id }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUser._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: soleOwnedGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_OWNER, + grantedBy: deletingUser._id, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUser._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: multiOwnedGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_OWNER, + grantedBy: deletingUser._id, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUser._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: multiOwnedGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_OWNER, + grantedBy: otherUser._id, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUser._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: sharedGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_OWNER, + grantedBy: otherUser._id, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUser._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: sharedGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_VIEWER, + grantedBy: otherUser._id, + }); + + const globalProject = await Project.findOne({ name: 'Global' }); + await Project.updateOne( + { _id: globalProject._id }, + { + $addToSet: { + promptGroupIds: { + $each: [soleOwnedGroup._id, multiOwnedGroup._id, sharedGroup._id], + }, + }, + }, + ); + + await promptFns.deleteUserPrompts(deletingUser._id.toString()); + }); + + test('should delete solely-owned prompt groups and their prompts', async () => { + expect(await PromptGroup.findById(soleOwnedGroup._id)).toBeNull(); + expect(await Prompt.findById(soleOwnedPrompt._id)).toBeNull(); + }); + + test('should remove solely-owned groups from projects', async () => { + const globalProject = await Project.findOne({ name: 'Global' }); + const projectGroupIds = globalProject.promptGroupIds.map((id) => id.toString()); + expect(projectGroupIds).not.toContain(soleOwnedGroup._id.toString()); + }); + + test('should remove all ACL entries for solely-owned groups', async () => { + const aclEntries = await AclEntry.find({ + resourceType: ResourceType.PROMPTGROUP, + resourceId: soleOwnedGroup._id, + }); + expect(aclEntries).toHaveLength(0); + }); + + test('should preserve multi-owned prompt groups', async () => { + expect(await PromptGroup.findById(multiOwnedGroup._id)).not.toBeNull(); + expect(await Prompt.findById(multiOwnedPrompt._id)).not.toBeNull(); + }); + + test('should preserve ACL entries of other owners on multi-owned groups', async () => { + const otherOwnerAcl = await AclEntry.findOne({ + resourceType: ResourceType.PROMPTGROUP, + resourceId: multiOwnedGroup._id, + principalId: otherUser._id, + }); + expect(otherOwnerAcl).not.toBeNull(); + expect(otherOwnerAcl.permBits & PermissionBits.DELETE).toBeTruthy(); + }); + + test('should preserve groups owned by other users', async () => { + expect(await PromptGroup.findById(sharedGroup._id)).not.toBeNull(); + expect(await Prompt.findById(sharedPrompt._id)).not.toBeNull(); + }); + + test('should preserve project membership of non-deleted groups', async () => { + const globalProject = await Project.findOne({ name: 'Global' }); + const projectGroupIds = globalProject.promptGroupIds.map((id) => id.toString()); + expect(projectGroupIds).toContain(multiOwnedGroup._id.toString()); + expect(projectGroupIds).toContain(sharedGroup._id.toString()); + }); + + test('should preserve ACL entries for shared group owned by other user', async () => { + const ownerAcl = await AclEntry.findOne({ + resourceType: ResourceType.PROMPTGROUP, + resourceId: sharedGroup._id, + principalId: otherUser._id, + }); + expect(ownerAcl).not.toBeNull(); + }); + + test('should be a no-op when user has no owned prompt groups', async () => { + const unrelatedUser = await User.create({ + name: 'Unrelated User', + email: 'unrelated@example.com', + role: SystemRoles.USER, + }); + + const beforeCount = await PromptGroup.countDocuments(); + await promptFns.deleteUserPrompts(unrelatedUser._id.toString()); + const afterCount = await PromptGroup.countDocuments(); + + expect(afterCount).toBe(beforeCount); + }); + + test('should delete legacy prompt groups that have author but no ACL entries', async () => { + const legacyUser = await User.create({ + name: 'Legacy User', + email: 'legacy-prompt@example.com', + role: SystemRoles.USER, + }); + + const legacyGroup = await PromptGroup.create({ + name: 'Legacy Group (no ACL)', + author: legacyUser._id, + authorName: legacyUser.name, + productionId: new ObjectId(), + }); + const legacyPrompt = await Prompt.create({ + prompt: 'Legacy prompt text', + author: legacyUser._id, + groupId: legacyGroup._id, + type: 'text', + }); + + await promptFns.deleteUserPrompts(legacyUser._id.toString()); + + expect(await PromptGroup.findById(legacyGroup._id)).toBeNull(); + expect(await Prompt.findById(legacyPrompt._id)).toBeNull(); + }); + }); }); diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index 6d5df0ac8d..51f6d218ec 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -1,11 +1,18 @@ +const mongoose = require('mongoose'); const { logger, webSearchKeys } = require('@librechat/data-schemas'); -const { Tools, CacheKeys, Constants, FileSources } = require('librechat-data-provider'); const { MCPOAuthHandler, MCPTokenStorage, normalizeHttpError, extractWebSearchEnvVars, } = require('@librechat/api'); +const { + Tools, + CacheKeys, + Constants, + FileSources, + ResourceType, +} = require('librechat-data-provider'); const { deleteAllUserSessions, deleteAllSharedLinks, @@ -45,6 +52,7 @@ const { getAppConfig } = require('~/server/services/Config'); const { deleteToolCalls } = require('~/models/ToolCall'); const { deleteUserPrompts } = require('~/models/Prompt'); const { deleteUserAgents } = require('~/models/Agent'); +const { getSoleOwnedResourceIds } = require('~/server/services/PermissionService'); const { getLogStores } = require('~/cache'); const getUserController = async (req, res) => { @@ -113,6 +121,78 @@ const deleteUserFiles = async (req) => { } }; +/** + * Deletes MCP servers solely owned by the user and cleans up their ACLs. + * Disconnects live sessions for deleted servers before removing DB records. + * Servers with other owners are left intact; the caller is responsible for + * removing the user's own ACL principal entries separately. + * + * Also handles legacy (pre-ACL) MCP servers that only have the author field set, + * ensuring they are not orphaned if no permission migration has been run. + * @param {string} userId - The ID of the user. + */ +const deleteUserMcpServers = async (userId) => { + try { + const MCPServer = mongoose.models.MCPServer; + if (!MCPServer) { + return; + } + + const userObjectId = new mongoose.Types.ObjectId(userId); + const soleOwnedIds = await getSoleOwnedResourceIds(userObjectId, ResourceType.MCPSERVER); + + const authoredServers = await MCPServer.find({ author: userObjectId }) + .select('_id serverName') + .lean(); + + const migratedEntries = + authoredServers.length > 0 + ? await AclEntry.find({ + resourceType: ResourceType.MCPSERVER, + resourceId: { $in: authoredServers.map((s) => s._id) }, + }) + .select('resourceId') + .lean() + : []; + const migratedIds = new Set(migratedEntries.map((e) => e.resourceId.toString())); + const legacyServers = authoredServers.filter((s) => !migratedIds.has(s._id.toString())); + const legacyServerIds = legacyServers.map((s) => s._id); + + const allServerIdsToDelete = [...soleOwnedIds, ...legacyServerIds]; + + if (allServerIdsToDelete.length === 0) { + return; + } + + const aclOwnedServers = + soleOwnedIds.length > 0 + ? await MCPServer.find({ _id: { $in: soleOwnedIds } }) + .select('serverName') + .lean() + : []; + const allServersToDelete = [...aclOwnedServers, ...legacyServers]; + + const mcpManager = getMCPManager(); + if (mcpManager) { + await Promise.all( + allServersToDelete.map(async (s) => { + await mcpManager.disconnectUserConnection(userId, s.serverName); + await invalidateCachedTools({ userId, serverName: s.serverName }); + }), + ); + } + + await AclEntry.deleteMany({ + resourceType: ResourceType.MCPSERVER, + resourceId: { $in: allServerIdsToDelete }, + }); + + await MCPServer.deleteMany({ _id: { $in: allServerIdsToDelete } }); + } catch (error) { + logger.error('[deleteUserMcpServers] General error:', error); + } +}; + const updateUserPluginsController = async (req, res) => { const appConfig = await getAppConfig({ role: req.user?.role }); const { user } = req; @@ -281,7 +361,8 @@ const deleteUserController = async (req, res) => { await Assistant.deleteMany({ user: user.id }); // delete user assistants await ConversationTag.deleteMany({ user: user.id }); // delete user conversation tags await MemoryEntry.deleteMany({ userId: user.id }); // delete user memory entries - await deleteUserPrompts(req, user.id); // delete user prompts + await deleteUserPrompts(user.id); // delete user prompts + await deleteUserMcpServers(user.id); // delete user MCP servers await Action.deleteMany({ user: user.id }); // delete user actions await Token.deleteMany({ userId: user.id }); // delete user OAuth tokens await Group.updateMany( @@ -439,4 +520,5 @@ module.exports = { verifyEmailController, updateUserPluginsController, resendVerificationController, + deleteUserMcpServers, }; diff --git a/api/server/controllers/__tests__/deleteUser.spec.js b/api/server/controllers/__tests__/deleteUser.spec.js index d0f54a046f..6382cd1d8e 100644 --- a/api/server/controllers/__tests__/deleteUser.spec.js +++ b/api/server/controllers/__tests__/deleteUser.spec.js @@ -104,6 +104,10 @@ jest.mock('~/server/services/Config', () => ({ getAppConfig: jest.fn(), })); +jest.mock('~/server/services/PermissionService', () => ({ + getSoleOwnedResourceIds: jest.fn().mockResolvedValue([]), +})); + jest.mock('~/models/ToolCall', () => ({ deleteToolCalls: (...args) => mockDeleteToolCalls(...args), })); diff --git a/api/server/controllers/__tests__/deleteUserMcpServers.spec.js b/api/server/controllers/__tests__/deleteUserMcpServers.spec.js new file mode 100644 index 0000000000..fcb3211f24 --- /dev/null +++ b/api/server/controllers/__tests__/deleteUserMcpServers.spec.js @@ -0,0 +1,319 @@ +const mockGetMCPManager = jest.fn(); +const mockInvalidateCachedTools = jest.fn(); + +jest.mock('~/config', () => ({ + getMCPManager: (...args) => mockGetMCPManager(...args), + getFlowStateManager: jest.fn(), + getMCPServersRegistry: jest.fn(), +})); + +jest.mock('~/server/services/Config/getCachedTools', () => ({ + invalidateCachedTools: (...args) => mockInvalidateCachedTools(...args), +})); + +jest.mock('~/server/services/Config', () => ({ + getAppConfig: jest.fn(), + getMCPServerTools: jest.fn(), +})); + +const mongoose = require('mongoose'); +const { mcpServerSchema } = require('@librechat/data-schemas'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { + ResourceType, + AccessRoleIds, + PrincipalType, + PermissionBits, +} = require('librechat-data-provider'); +const permissionService = require('~/server/services/PermissionService'); +const { deleteUserMcpServers } = require('~/server/controllers/UserController'); +const { AclEntry, AccessRole } = require('~/db/models'); + +let MCPServer; + +describe('deleteUserMcpServers', () => { + let mongoServer; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + const mongoUri = mongoServer.getUri(); + MCPServer = mongoose.models.MCPServer || mongoose.model('MCPServer', mcpServerSchema); + await mongoose.connect(mongoUri); + + await AccessRole.create({ + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + name: 'MCP Server Owner', + resourceType: ResourceType.MCPSERVER, + permBits: + PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE, + }); + + await AccessRole.create({ + accessRoleId: AccessRoleIds.MCPSERVER_VIEWER, + name: 'MCP Server Viewer', + resourceType: ResourceType.MCPSERVER, + permBits: PermissionBits.VIEW, + }); + }, 20000); + + afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); + }); + + beforeEach(async () => { + await MCPServer.deleteMany({}); + await AclEntry.deleteMany({}); + jest.clearAllMocks(); + }); + + test('should delete solely-owned MCP servers and their ACL entries', async () => { + const userId = new mongoose.Types.ObjectId(); + + const server = await MCPServer.create({ + serverName: 'sole-owned-server', + config: { title: 'Test Server' }, + author: userId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: userId, + resourceType: ResourceType.MCPSERVER, + resourceId: server._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: userId, + }); + + mockGetMCPManager.mockReturnValue({ + disconnectUserConnection: jest.fn().mockResolvedValue(undefined), + }); + + await deleteUserMcpServers(userId.toString()); + + expect(await MCPServer.findById(server._id)).toBeNull(); + + const aclEntries = await AclEntry.find({ + resourceType: ResourceType.MCPSERVER, + resourceId: server._id, + }); + expect(aclEntries).toHaveLength(0); + }); + + test('should disconnect MCP sessions and invalidate tool cache before deletion', async () => { + const userId = new mongoose.Types.ObjectId(); + const mockDisconnect = jest.fn().mockResolvedValue(undefined); + + const server = await MCPServer.create({ + serverName: 'session-server', + config: { title: 'Session Server' }, + author: userId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: userId, + resourceType: ResourceType.MCPSERVER, + resourceId: server._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: userId, + }); + + mockGetMCPManager.mockReturnValue({ disconnectUserConnection: mockDisconnect }); + + await deleteUserMcpServers(userId.toString()); + + expect(mockDisconnect).toHaveBeenCalledWith(userId.toString(), 'session-server'); + expect(mockInvalidateCachedTools).toHaveBeenCalledWith({ + userId: userId.toString(), + serverName: 'session-server', + }); + }); + + test('should preserve multi-owned MCP servers', async () => { + const deletingUserId = new mongoose.Types.ObjectId(); + const otherOwnerId = new mongoose.Types.ObjectId(); + + const soleServer = await MCPServer.create({ + serverName: 'sole-server', + config: { title: 'Sole Server' }, + author: deletingUserId, + }); + + const multiServer = await MCPServer.create({ + serverName: 'multi-server', + config: { title: 'Multi Server' }, + author: deletingUserId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUserId, + resourceType: ResourceType.MCPSERVER, + resourceId: soleServer._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: deletingUserId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: deletingUserId, + resourceType: ResourceType.MCPSERVER, + resourceId: multiServer._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: deletingUserId, + }); + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherOwnerId, + resourceType: ResourceType.MCPSERVER, + resourceId: multiServer._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: otherOwnerId, + }); + + mockGetMCPManager.mockReturnValue({ + disconnectUserConnection: jest.fn().mockResolvedValue(undefined), + }); + + await deleteUserMcpServers(deletingUserId.toString()); + + expect(await MCPServer.findById(soleServer._id)).toBeNull(); + expect(await MCPServer.findById(multiServer._id)).not.toBeNull(); + + const soleAcl = await AclEntry.find({ + resourceType: ResourceType.MCPSERVER, + resourceId: soleServer._id, + }); + expect(soleAcl).toHaveLength(0); + + const multiAclOther = await AclEntry.find({ + resourceType: ResourceType.MCPSERVER, + resourceId: multiServer._id, + principalId: otherOwnerId, + }); + expect(multiAclOther).toHaveLength(1); + expect(multiAclOther[0].permBits & PermissionBits.DELETE).toBeTruthy(); + + const multiAclDeleting = await AclEntry.find({ + resourceType: ResourceType.MCPSERVER, + resourceId: multiServer._id, + principalId: deletingUserId, + }); + expect(multiAclDeleting).toHaveLength(1); + }); + + test('should be a no-op when user has no owned MCP servers', async () => { + const userId = new mongoose.Types.ObjectId(); + + const otherUserId = new mongoose.Types.ObjectId(); + const server = await MCPServer.create({ + serverName: 'other-server', + config: { title: 'Other Server' }, + author: otherUserId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.MCPSERVER, + resourceId: server._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: otherUserId, + }); + + await deleteUserMcpServers(userId.toString()); + + expect(await MCPServer.findById(server._id)).not.toBeNull(); + expect(mockGetMCPManager).not.toHaveBeenCalled(); + }); + + test('should handle gracefully when MCPServer model is not registered', async () => { + const originalModel = mongoose.models.MCPServer; + delete mongoose.models.MCPServer; + + try { + const userId = new mongoose.Types.ObjectId(); + await expect(deleteUserMcpServers(userId.toString())).resolves.toBeUndefined(); + } finally { + mongoose.models.MCPServer = originalModel; + } + }); + + test('should handle gracefully when MCPManager is not available', async () => { + const userId = new mongoose.Types.ObjectId(); + + const server = await MCPServer.create({ + serverName: 'no-manager-server', + config: { title: 'No Manager Server' }, + author: userId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: userId, + resourceType: ResourceType.MCPSERVER, + resourceId: server._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: userId, + }); + + mockGetMCPManager.mockReturnValue(null); + + await deleteUserMcpServers(userId.toString()); + + expect(await MCPServer.findById(server._id)).toBeNull(); + }); + + test('should delete legacy MCP servers that have author but no ACL entries', async () => { + const legacyUserId = new mongoose.Types.ObjectId(); + + const legacyServer = await MCPServer.create({ + serverName: 'legacy-server', + config: { title: 'Legacy Server' }, + author: legacyUserId, + }); + + mockGetMCPManager.mockReturnValue({ + disconnectUserConnection: jest.fn().mockResolvedValue(undefined), + }); + + await deleteUserMcpServers(legacyUserId.toString()); + + expect(await MCPServer.findById(legacyServer._id)).toBeNull(); + }); + + test('should delete both ACL-owned and legacy servers in one call', async () => { + const userId = new mongoose.Types.ObjectId(); + + const aclServer = await MCPServer.create({ + serverName: 'acl-server', + config: { title: 'ACL Server' }, + author: userId, + }); + + await permissionService.grantPermission({ + principalType: PrincipalType.USER, + principalId: userId, + resourceType: ResourceType.MCPSERVER, + resourceId: aclServer._id, + accessRoleId: AccessRoleIds.MCPSERVER_OWNER, + grantedBy: userId, + }); + + const legacyServer = await MCPServer.create({ + serverName: 'legacy-mixed-server', + config: { title: 'Legacy Mixed' }, + author: userId, + }); + + mockGetMCPManager.mockReturnValue({ + disconnectUserConnection: jest.fn().mockResolvedValue(undefined), + }); + + await deleteUserMcpServers(userId.toString()); + + expect(await MCPServer.findById(aclServer._id)).toBeNull(); + expect(await MCPServer.findById(legacyServer._id)).toBeNull(); + }); +}); diff --git a/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js new file mode 100644 index 0000000000..b08e502800 --- /dev/null +++ b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js @@ -0,0 +1,53 @@ +const fs = require('fs'); +const path = require('path'); +const { ResourceType } = require('librechat-data-provider'); + +/** + * Maps each ResourceType to the cleanup function name that must appear in + * deleteUserController's source to prove it is handled during user deletion. + * + * When a new ResourceType is added, this test will fail until a corresponding + * entry is added here (or to NO_USER_CLEANUP_NEEDED) AND the actual cleanup + * logic is implemented. + */ +const HANDLED_RESOURCE_TYPES = { + [ResourceType.AGENT]: 'deleteUserAgents', + [ResourceType.REMOTE_AGENT]: 'deleteUserAgents', + [ResourceType.PROMPTGROUP]: 'deleteUserPrompts', + [ResourceType.MCPSERVER]: 'deleteUserMcpServers', +}; + +/** + * ResourceTypes that are ACL-tracked but have no per-user deletion semantics + * (e.g., system resources, public-only). Must be explicitly listed here with + * a justification to prevent silent omissions. + */ +const NO_USER_CLEANUP_NEEDED = new Set([ + // Example: ResourceType.SYSTEM_TEMPLATE — public/system; not user-owned +]); + +describe('deleteUserController - resource type coverage guard', () => { + let controllerSource; + + beforeAll(() => { + controllerSource = fs.readFileSync(path.resolve(__dirname, '../UserController.js'), 'utf-8'); + }); + + test('every ResourceType must have a documented cleanup handler or explicit exclusion', () => { + const allTypes = Object.values(ResourceType); + const handledTypes = Object.keys(HANDLED_RESOURCE_TYPES); + const unhandledTypes = allTypes.filter( + (t) => !handledTypes.includes(t) && !NO_USER_CLEANUP_NEEDED.has(t), + ); + + expect(unhandledTypes).toEqual([]); + }); + + test('every cleanup handler referenced in HANDLED_RESOURCE_TYPES must appear in the controller source', () => { + const uniqueHandlers = [...new Set(Object.values(HANDLED_RESOURCE_TYPES))]; + + for (const handler of uniqueHandlers) { + expect(controllerSource).toContain(handler); + } + }); +}); diff --git a/api/server/services/PermissionService.js b/api/server/services/PermissionService.js index a843f48f6f..ba1ef68032 100644 --- a/api/server/services/PermissionService.js +++ b/api/server/services/PermissionService.js @@ -1,7 +1,12 @@ const mongoose = require('mongoose'); const { isEnabled } = require('@librechat/api'); const { getTransactionSupport, logger } = require('@librechat/data-schemas'); -const { ResourceType, PrincipalType, PrincipalModel } = require('librechat-data-provider'); +const { + ResourceType, + PrincipalType, + PrincipalModel, + PermissionBits, +} = require('librechat-data-provider'); const { entraIdPrincipalFeatureEnabled, getUserOwnedEntraGroups, @@ -799,6 +804,49 @@ const bulkUpdateResourcePermissions = async ({ } }; +/** + * Returns resource IDs where the given user is the sole owner + * (no other principal holds the DELETE bit on the same resource). + * @param {mongoose.Types.ObjectId} userObjectId + * @param {string|string[]} resourceTypes - One or more ResourceType values. + * @returns {Promise} + */ +const getSoleOwnedResourceIds = async (userObjectId, resourceTypes) => { + const types = Array.isArray(resourceTypes) ? resourceTypes : [resourceTypes]; + const ownedEntries = await AclEntry.find({ + principalType: PrincipalType.USER, + principalId: userObjectId, + resourceType: { $in: types }, + permBits: { $bitsAllSet: PermissionBits.DELETE }, + }) + .select('resourceId') + .lean(); + + if (ownedEntries.length === 0) { + return []; + } + + const ownedIds = ownedEntries.map((e) => e.resourceId); + + const otherOwners = await AclEntry.aggregate([ + { + $match: { + resourceType: { $in: types }, + resourceId: { $in: ownedIds }, + permBits: { $bitsAllSet: PermissionBits.DELETE }, + $or: [ + { principalId: { $ne: userObjectId } }, + { principalType: { $ne: PrincipalType.USER } }, + ], + }, + }, + { $group: { _id: '$resourceId' } }, + ]); + + const multiOwnerIds = new Set(otherOwners.map((doc) => doc._id.toString())); + return ownedIds.filter((id) => !multiOwnerIds.has(id.toString())); +}; + /** * Remove all permissions for a resource (cleanup when resource is deleted) * @param {Object} params - Parameters for removing all permissions @@ -839,5 +887,6 @@ module.exports = { ensurePrincipalExists, ensureGroupPrincipalExists, syncUserEntraGroupMemberships, + getSoleOwnedResourceIds, removeAllPermissions, };