📉 perf: Skip Redundant Permission Queries on MCP Servers List (#14077)

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

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

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

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

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) {

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',