From 53b3e166d8800e26767c6f9740cc2dec495dd1bc Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 2 Jul 2026 11:45:23 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=89=20perf:=20Skip=20Redundant=20Permi?= =?UTF-8?q?ssion=20Queries=20on=20MCP=20Servers=20List=20(#14077)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../controllers/__tests__/mcp.servers.spec.js | 258 ++++++++++++++++++ api/server/controllers/mcp.js | 36 ++- api/strategies/jwtStrategy.js | 2 + api/strategies/jwtStrategy.spec.js | 75 +++++ api/strategies/openIdJwtStrategy.js | 2 + api/strategies/openIdJwtStrategy.spec.js | 45 +++ 6 files changed, 405 insertions(+), 13 deletions(-) create mode 100644 api/server/controllers/__tests__/mcp.servers.spec.js create mode 100644 api/strategies/jwtStrategy.spec.js diff --git a/api/server/controllers/__tests__/mcp.servers.spec.js b/api/server/controllers/__tests__/mcp.servers.spec.js new file mode 100644 index 0000000000..e8f3f6abda --- /dev/null +++ b/api/server/controllers/__tests__/mcp.servers.spec.js @@ -0,0 +1,258 @@ +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { SystemCapabilities } = require('@librechat/data-schemas'); +const { + SystemRoles, + ResourceType, + AccessRoleIds, + PrincipalType, +} = require('librechat-data-provider'); + +jest.mock('@librechat/data-schemas', () => ({ + ...jest.requireActual('@librechat/data-schemas'), + getTransactionSupport: jest.fn().mockResolvedValue(false), +})); + +jest.mock('~/server/services/GraphApiService', () => ({ + entraIdPrincipalFeatureEnabled: jest.fn().mockReturnValue(false), + getUserOwnedEntraGroups: jest.fn().mockResolvedValue([]), + getUserEntraGroups: jest.fn().mockResolvedValue([]), + getEntraGroupDetailsBatch: jest.fn().mockResolvedValue([]), + getGroupMembers: jest.fn().mockResolvedValue([]), + getGroupOwners: jest.fn().mockResolvedValue([]), +})); + +const mockRegistryInstance = { + getServerConfig: jest.fn(), +}; + +jest.mock('~/config', () => ({ + logger: { debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn() }, + getMCPManager: jest.fn(), + getMCPServersRegistry: jest.fn(() => mockRegistryInstance), +})); + +const mockResolveAllMcpConfigs = jest.fn(); +jest.mock('~/server/services/MCP', () => ({ + resolveConfigServers: jest.fn().mockResolvedValue({}), + resolveMcpConfigNames: jest.fn().mockResolvedValue([]), + resolveAllMcpConfigs: (...args) => mockResolveAllMcpConfigs(...args), +})); + +jest.mock('~/server/services/Config', () => ({ + cacheMCPServerTools: jest.fn(), + getMCPServerTools: jest.fn(), +})); + +const { getMCPServersList, getMCPServerById } = require('~/server/controllers/mcp'); +const { grantPermission } = require('~/server/services/PermissionService'); +const { seedDefaultRoles } = require('~/models'); + +let mongoServer; +let SystemGrant; +let AclEntry; +let User; + +const yamlConfig = { + type: 'streamable-http', + url: 'https://internal.example.com/mcp', + title: 'YAML Server', + source: 'yaml', + oauth: { + client_id: 'client-id', + authorization_url: 'https://internal.example.com/auth', + token_url: 'https://internal.example.com/token', + }, +}; + +const createRes = () => { + const res = {}; + res.status = jest.fn(() => res); + res.json = jest.fn(() => res); + return res; +}; + +const createDbConfig = (dbId) => ({ + type: 'streamable-http', + url: 'https://user.example.com/mcp', + title: 'DB Server', + source: 'user', + dbId: String(dbId), +}); + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + + const { createModels } = jest.requireActual('@librechat/data-schemas'); + createModels(mongoose); + const dbModels = require('~/db/models'); + Object.assign(mongoose.models, dbModels); + SystemGrant = dbModels.SystemGrant; + AclEntry = dbModels.AclEntry; + User = dbModels.User; + + await seedDefaultRoles(); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +let existsSpy; + +beforeEach(async () => { + await SystemGrant.deleteMany({}); + await AclEntry.deleteMany({}); + await User.deleteMany({}); + mockResolveAllMcpConfigs.mockReset(); + mockRegistryInstance.getServerConfig.mockReset(); + existsSpy = jest.spyOn(SystemGrant, 'exists'); +}); + +afterEach(() => { + existsSpy.mockRestore(); +}); + +const seedManageMcpGrant = async (role = SystemRoles.ADMIN) => { + await SystemGrant.create({ + principalType: PrincipalType.ROLE, + principalId: role, + capability: SystemCapabilities.MANAGE_MCP_SERVERS, + grantedAt: new Date(), + }); +}; + +const createUser = async (role = SystemRoles.USER) => { + const user = await User.create({ + name: 'Test User', + email: `user-${new mongoose.Types.ObjectId().toString()}@example.com`, + provider: 'local', + role, + }); + return { id: user._id.toString(), role, idOnTheSource: null }; +}; + +describe('getMCPServersList', () => { + it('skips the capability probe when no server is DB-backed', async () => { + await seedManageMcpGrant(); + const reqUser = await createUser(SystemRoles.ADMIN); + mockResolveAllMcpConfigs.mockResolvedValue({ yamlServer: { ...yamlConfig } }); + + const res = createRes(); + await getMCPServersList({ user: reqUser }, res); + + expect(existsSpy).not.toHaveBeenCalled(); + const payload = res.json.mock.calls[0][0]; + expect(payload.yamlServer.title).toBe('YAML Server'); + expect(payload.yamlServer.url).toBeUndefined(); + expect(payload.yamlServer.oauth.authorization_url).toBeUndefined(); + }); + + it('skips the probe entirely for an empty server map', async () => { + const reqUser = await createUser(); + mockResolveAllMcpConfigs.mockResolvedValue({}); + + const res = createRes(); + await getMCPServersList({ user: reqUser }, res); + + expect(existsSpy).not.toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith({}); + }); + + it('applies the capability bypass to all servers when a DB-backed server is present', async () => { + await seedManageMcpGrant(); + const reqUser = await createUser(SystemRoles.ADMIN); + const dbId = new mongoose.Types.ObjectId(); + mockResolveAllMcpConfigs.mockResolvedValue({ + dbServer: createDbConfig(dbId), + yamlServer: { ...yamlConfig }, + }); + + const res = createRes(); + await getMCPServersList({ user: reqUser }, res); + + expect(existsSpy).toHaveBeenCalledTimes(1); + const payload = res.json.mock.calls[0][0]; + expect(payload.dbServer.url).toBe('https://user.example.com/mcp'); + expect(payload.yamlServer.url).toBe('https://internal.example.com/mcp'); + }); + + it('falls back to ACL EDIT for DB-backed servers without the capability', async () => { + const reqUser = await createUser(); + const dbId = new mongoose.Types.ObjectId(); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: reqUser.id, + resourceType: ResourceType.MCPSERVER, + resourceId: dbId, + accessRoleId: AccessRoleIds.MCPSERVER_EDITOR, + grantedBy: reqUser.id, + }); + mockResolveAllMcpConfigs.mockResolvedValue({ + dbServer: createDbConfig(dbId), + yamlServer: { ...yamlConfig }, + }); + + const res = createRes(); + await getMCPServersList({ user: reqUser }, res); + + expect(existsSpy).toHaveBeenCalledTimes(1); + const payload = res.json.mock.calls[0][0]; + expect(payload.dbServer.url).toBe('https://user.example.com/mcp'); + expect(payload.yamlServer.url).toBeUndefined(); + }); + + it('leaves DB-backed servers redacted for viewer-only ACL', async () => { + const reqUser = await createUser(); + const dbId = new mongoose.Types.ObjectId(); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: reqUser.id, + resourceType: ResourceType.MCPSERVER, + resourceId: dbId, + accessRoleId: AccessRoleIds.MCPSERVER_VIEWER, + grantedBy: reqUser.id, + }); + mockResolveAllMcpConfigs.mockResolvedValue({ dbServer: createDbConfig(dbId) }); + + const res = createRes(); + await getMCPServersList({ user: reqUser }, res); + + expect(existsSpy).toHaveBeenCalledTimes(1); + const payload = res.json.mock.calls[0][0]; + expect(payload.dbServer.title).toBe('DB Server'); + expect(payload.dbServer.url).toBeUndefined(); + }); +}); + +describe('getMCPServerById', () => { + it('still runs the capability probe for YAML servers on the detail route', async () => { + await seedManageMcpGrant(); + const reqUser = await createUser(SystemRoles.ADMIN); + mockRegistryInstance.getServerConfig.mockResolvedValue({ ...yamlConfig }); + + const res = createRes(); + await getMCPServerById({ user: reqUser, params: { serverName: 'yamlServer' } }, res); + + expect(existsSpy).toHaveBeenCalledTimes(1); + expect(res.status).toHaveBeenCalledWith(200); + const payload = res.json.mock.calls[0][0]; + expect(payload.url).toBe('https://internal.example.com/mcp'); + expect(payload.oauth.authorization_url).toBe('https://internal.example.com/auth'); + }); + + it('redacts YAML server details for users without the capability', async () => { + const reqUser = await createUser(); + mockRegistryInstance.getServerConfig.mockResolvedValue({ ...yamlConfig }); + + const res = createRes(); + await getMCPServerById({ user: reqUser, params: { serverName: 'yamlServer' } }, res); + + expect(existsSpy).toHaveBeenCalledTimes(1); + const payload = res.json.mock.calls[0][0]; + expect(payload.url).toBeUndefined(); + expect(payload.oauth.authorization_url).toBeUndefined(); + }); +}); diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 85b840891d..c3d3fc46df 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -201,9 +201,27 @@ const getMCPTools = async (req, res) => { res.status(500).json({ message: error.message }); } }; -/** Mirrors canAccessResource's capability bypass plus per-resource ACL EDIT check. */ -async function computeCanEditByServer(req, serverConfigs) { +/** + * Mirrors canAccessResource's capability bypass plus per-resource ACL EDIT check. + * `skipCapabilityWithoutDbIds` lets the list path skip the MANAGE_MCP_SERVERS probe + * when no DB-backed server is present; no list consumer reads the edit-gated fields + * the bypass would disclose. The detail route must not set it. + */ +async function computeCanEditByServer(req, serverConfigs, { skipCapabilityWithoutDbIds } = {}) { const canEditByServer = new Map(); + const dbIdsToCheck = []; + const dbIdToServerName = new Map(); + for (const [name, config] of Object.entries(serverConfigs)) { + if (config.dbId) { + dbIdsToCheck.push(config.dbId); + dbIdToServerName.set(String(config.dbId), name); + continue; + } + canEditByServer.set(name, isUserSourced(config)); + } + if (skipCapabilityWithoutDbIds === true && dbIdsToCheck.length === 0) { + return canEditByServer; + } let bypass = false; try { bypass = await hasCapability(req.user, SystemCapabilities.MANAGE_MCP_SERVERS); @@ -216,16 +234,6 @@ async function computeCanEditByServer(req, serverConfigs) { } return canEditByServer; } - const dbIdsToCheck = []; - const dbIdToServerName = new Map(); - for (const [name, config] of Object.entries(serverConfigs)) { - if (config.dbId) { - dbIdsToCheck.push(config.dbId); - dbIdToServerName.set(String(config.dbId), name); - continue; - } - canEditByServer.set(name, isUserSourced(config)); - } if (dbIdsToCheck.length > 0) { try { const permsMap = await getResourcePermissionsMap({ @@ -262,7 +270,9 @@ const getMCPServersList = async (req, res) => { } const serverConfigs = await resolveAllMcpConfigs(userId, req.user); - const canEditByServer = await computeCanEditByServer(req, serverConfigs); + const canEditByServer = await computeCanEditByServer(req, serverConfigs, { + skipCapabilityWithoutDbIds: true, + }); return res.json(redactAllServerSecrets(serverConfigs, { canEditByServer })); } catch (error) { logger.error('[getMCPServersList]', error); diff --git a/api/strategies/jwtStrategy.js b/api/strategies/jwtStrategy.js index 386b7bafa1..7a41f553e2 100644 --- a/api/strategies/jwtStrategy.js +++ b/api/strategies/jwtStrategy.js @@ -15,6 +15,8 @@ const jwtLogin = () => const user = await getUserById(payload?.id, '-password -__v -totpSecret -backupCodes'); if (user) { user.id = user._id.toString(); + /** Absent on the full doc means local user; null skips getUserPrincipals' fallback lookup */ + user.idOnTheSource ??= null; if (!user.role) { user.role = SystemRoles.USER; await updateUser(user.id, { role: user.role }); diff --git a/api/strategies/jwtStrategy.spec.js b/api/strategies/jwtStrategy.spec.js new file mode 100644 index 0000000000..3a8b83812d --- /dev/null +++ b/api/strategies/jwtStrategy.spec.js @@ -0,0 +1,75 @@ +const { SystemRoles } = require('librechat-data-provider'); + +let capturedVerifyCallback; +jest.mock('passport-jwt', () => ({ + Strategy: jest.fn((opts, verifyCallback) => { + capturedVerifyCallback = verifyCallback; + return { name: 'jwt' }; + }), + ExtractJwt: { + fromAuthHeaderAsBearerToken: jest.fn(() => 'mock-extractor'), + }, +})); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { info: jest.fn(), warn: jest.fn(), debug: jest.fn(), error: jest.fn() }, +})); + +jest.mock('~/models', () => ({ + getUserById: jest.fn(), + updateUser: jest.fn(), +})); + +const jwtLogin = require('./jwtStrategy'); +const { getUserById, updateUser } = require('~/models'); + +function invokeVerify(payload) { + return new Promise((resolve, reject) => { + capturedVerifyCallback(payload, (err, user, info) => { + if (err) { + return reject(err); + } + resolve({ user, info }); + }); + }); +} + +describe('jwtStrategy', () => { + beforeEach(() => { + jest.clearAllMocks(); + updateUser.mockResolvedValue({}); + jwtLogin(); + }); + + it('coerces missing idOnTheSource to null for local users', async () => { + getUserById.mockResolvedValue({ + _id: { toString: () => 'user-1' }, + role: SystemRoles.USER, + }); + + const { user } = await invokeVerify({ id: 'user-1' }); + + expect(user.id).toBe('user-1'); + expect(user.idOnTheSource).toBeNull(); + }); + + it('preserves a stored idOnTheSource for federated users', async () => { + getUserById.mockResolvedValue({ + _id: { toString: () => 'user-2' }, + role: SystemRoles.USER, + idOnTheSource: 'entra-oid-123', + }); + + const { user } = await invokeVerify({ id: 'user-2' }); + + expect(user.idOnTheSource).toBe('entra-oid-123'); + }); + + it('returns false when no user is found', async () => { + getUserById.mockResolvedValue(null); + + const { user } = await invokeVerify({ id: 'missing' }); + + expect(user).toBe(false); + }); +}); diff --git a/api/strategies/openIdJwtStrategy.js b/api/strategies/openIdJwtStrategy.js index 14f50f3f04..9b9c9644f2 100644 --- a/api/strategies/openIdJwtStrategy.js +++ b/api/strategies/openIdJwtStrategy.js @@ -117,6 +117,8 @@ const openIdJwtLogin = (openIdConfig) => { if (user) { user.id = user._id.toString(); + /** Absent on the full doc means local user; null skips getUserPrincipals' fallback lookup */ + user.idOnTheSource ??= null; const updateData = {}; if (migration) { diff --git a/api/strategies/openIdJwtStrategy.spec.js b/api/strategies/openIdJwtStrategy.spec.js index 5b4bc86c49..4121d64def 100644 --- a/api/strategies/openIdJwtStrategy.spec.js +++ b/api/strategies/openIdJwtStrategy.spec.js @@ -348,6 +348,51 @@ describe('openIdJwtStrategy – token source handling', () => { }); }); +describe('openIdJwtStrategy – idOnTheSource boundary coercion', () => { + const payload = { + sub: 'oidc-123', + email: 'test@example.com', + iss: 'https://issuer.example.com', + exp: 9999999999, + }; + const req = { headers: { authorization: 'Bearer tok' }, session: {} }; + + beforeEach(() => { + jest.clearAllMocks(); + updateUser.mockResolvedValue({}); + openIdJwtLogin(mockOpenIdConfig); + }); + + it('coerces missing idOnTheSource to null', async () => { + findOpenIDUser.mockResolvedValue({ + user: { _id: { toString: () => 'user-abc' }, role: SystemRoles.USER, provider: 'openid' }, + error: null, + migration: false, + }); + + const { user } = await invokeVerify(req, payload); + + expect(user.idOnTheSource).toBeNull(); + }); + + it('preserves a stored idOnTheSource', async () => { + findOpenIDUser.mockResolvedValue({ + user: { + _id: { toString: () => 'user-abc' }, + role: SystemRoles.USER, + provider: 'openid', + idOnTheSource: 'entra-oid-123', + }, + error: null, + migration: false, + }); + + const { user } = await invokeVerify(req, payload); + + expect(user.idOnTheSource).toBe('entra-oid-123'); + }); +}); + describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => { const payload = { sub: 'oidc-123',