LibreChat/api/server/controllers/__tests__/deleteUserMcpServers.spec.js
Danny Avila 1ecff83b20
🪦 fix: ACL-Safe User Account Deletion for Agents, Prompts, and MCP Servers (#12314)
* fix: use ACL ownership for prompt group cleanup on user deletion

deleteUserPrompts previously called getAllPromptGroups with only an
author filter, which defaults to searchShared=true and drops the
author filter for shared/global project entries. This caused any user
deleting their account to strip shared prompt group associations and
ACL entries for other users.

Replace the author-based query with ACL-based ownership lookup:
- Find prompt groups where the user has OWNER permission (DELETE bit)
- Only delete groups where the user is the sole owner
- Preserve multi-owned groups and their ACL entries for other owners

* fix: use ACL ownership for agent cleanup on user deletion

deleteUserAgents used the deprecated author field to find and delete
agents, then unconditionally removed all ACL entries for those agents.
This could destroy ACL entries for agents shared with or co-owned by
other users.

Replace the author-based query with ACL-based ownership lookup:
- Find agents where the user has OWNER permission (DELETE bit)
- Only delete agents where the user is the sole owner
- Preserve multi-owned agents and their ACL entries for other owners
- Also clean up handoff edges referencing deleted agents

* fix: add MCP server cleanup on user deletion

User deletion had no cleanup for MCP servers, leaving solely-owned
servers orphaned in the database with dangling ACL entries for other
users.

Add deleteUserMcpServers that follows the same ACL ownership pattern
as prompt groups and agents: find servers with OWNER permission,
check for sole ownership, and only delete those with no other owners.

* style: fix prettier formatting in Prompt.spec.js

* refactor: extract getSoleOwnedResourceIds to PermissionService

The ACL sole-ownership detection algorithm was duplicated across
deleteUserPrompts, deleteUserAgents, and deleteUserMcpServers.
Centralizes the three-step pattern (find owned entries, find other
owners, compute sole-owned set) into a single reusable utility.

* refactor: use getSoleOwnedResourceIds in all deletion functions

- Replace inline ACL queries with the centralized utility
- Remove vestigial _req parameter from deleteUserPrompts
- Use Promise.all for parallel project removal instead of sequential awaits
- Disconnect live MCP sessions and invalidate tool cache before deleting
  sole-owned MCP server documents
- Export deleteUserMcpServers for testability

* test: improve deletion test coverage and quality

- Move deleteUserPrompts call to beforeAll to eliminate execution-order
  dependency between tests
- Standardize on test() instead of it() for consistency in Prompt.spec.js
- Add assertion for deleting user's own ACL entry preservation on
  multi-owned agents
- Add deleteUserMcpServers integration test suite with 6 tests covering
  sole-owner deletion, multi-owner preservation, session disconnect,
  cache invalidation, model-not-registered guard, and missing MCPManager
- Add PermissionService mock to existing deleteUser.spec.js to fix
  import chain

* fix: add legacy author-based fallback for unmigrated resources

Resources created before the ACL system have author set but no AclEntry
records. The sole-ownership detection returns empty for these, causing
deleteUserPrompts, deleteUserAgents, and deleteUserMcpServers to silently
skip them — permanently orphaning data on user deletion.

Add a fallback that identifies author-owned resources with zero ACL
entries (truly unmigrated) and includes them in the deletion set. This
preserves the multi-owner safety of the ACL path while ensuring pre-ACL
resources are still cleaned up regardless of migration status.

* style: fix prettier formatting across all changed files

* test: add resource type coverage guard for user deletion

Ensures every ResourceType in the ACL system has a corresponding cleanup
handler wired into deleteUserController. When a new ResourceType is added
(e.g. WORKFLOW), this test fails immediately, preventing silent data
orphaning on user account deletion.

* style: fix import order in PermissionService destructure

* test: add opt-out set and fix test lifecycle in coverage guard

Add NO_USER_CLEANUP_NEEDED set for resource types that legitimately
require no per-user deletion. Move fs.readFileSync into beforeAll so
path errors surface as clean test failures instead of unhandled crashes.
2026-03-19 17:46:14 -04:00

319 lines
9.7 KiB
JavaScript

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();
});
});