From f667da36d816b4b687171b295091ee1fbb57debf Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Tue, 22 Sep 2026 10:32:41 -0700 Subject: [PATCH] fix(mcp): list servers without Credential Group access --- apps/sim/hooks/queries/mcp.test.tsx | 70 +++++++++++ .../application/authorization.test.ts | 31 +++++ .../application/managed-connections.test.ts | 118 +++++++++++++++--- .../mcp/application/managed-connections.ts | 21 +++- 4 files changed, 219 insertions(+), 21 deletions(-) diff --git a/apps/sim/hooks/queries/mcp.test.tsx b/apps/sim/hooks/queries/mcp.test.tsx index 167c94e51a3..637a8bbae41 100644 --- a/apps/sim/hooks/queries/mcp.test.tsx +++ b/apps/sim/hooks/queries/mcp.test.tsx @@ -27,6 +27,7 @@ import { useAllowedMcpDomains, useForceRefreshMcpTools, useMcpServers, + useMcpToolServers, useMcpToolsQuery, useStoredMcpTools, } from '@/hooks/queries/mcp' @@ -118,6 +119,75 @@ class FakeEventSource { close(): void {} } +describe('useMcpToolServers', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('lists ordinary workspace servers when no managed connections are available', async () => { + const sharedServer = server('shared-server') + mockServers([ + sharedServer, + server('managed-canonical-server', { credentialGroupId: 'group-1' }), + ]) + + const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID)) + await flush() + + expect(hook.getResult()).toEqual({ data: [sharedServer], isLoading: false, error: null }) + hook.unmount() + }) + + it('includes allowed managed connections alongside ordinary servers', async () => { + const sharedServer = server('shared-server') + const managedServer = server('mcp-cg-123456789012345678901', { + name: 'Fireflies — person@example.com', + managedConnectorId: 'fireflies', + authType: 'oauth', + url: undefined, + }) + mockRequestJson.mockImplementation(async (contract) => { + if (contract === listMcpServersContract) { + return { success: true, data: { servers: [sharedServer] } } + } + if (contract === listManagedMcpCatalogContract) return { servers: [managedServer], tools: [] } + throw new Error('Unexpected MCP request') + }) + + const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID)) + await flush() + + expect(hook.getResult()).toEqual({ + data: [sharedServer, managedServer], + isLoading: false, + error: null, + }) + hook.unmount() + }) + + it.each([ + { name: 'shared servers', failingContract: listMcpServersContract }, + { name: 'managed catalog', failingContract: listManagedMcpCatalogContract }, + ])('keeps unrelated $name errors visible', async ({ failingContract }) => { + const error = new Error('MCP request failed') + mockRequestJson.mockImplementation(async (contract) => { + if (contract === failingContract) throw error + if (contract === listMcpServersContract) { + return { success: true, data: { servers: [server('shared-server')] } } + } + if (contract === listManagedMcpCatalogContract) return { servers: [], tools: [] } + throw new Error('Unexpected MCP request') + }) + + const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID)) + await flush() + + expect(hook.getResult().error).toBe(error) + expect(hook.getResult().isLoading).toBe(false) + hook.unmount() + }) +}) + describe('useMcpToolsQuery', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/apps/sim/lib/credential-groups/application/authorization.test.ts b/apps/sim/lib/credential-groups/application/authorization.test.ts index 8638bed8885..081e9a7dbe0 100644 --- a/apps/sim/lib/credential-groups/application/authorization.test.ts +++ b/apps/sim/lib/credential-groups/application/authorization.test.ts @@ -276,6 +276,37 @@ describe('requireCredentialGroupCredentialAccess', () => { await expect(requireAccess(executorPrincipal())).rejects.toMatchObject({ code: 'not_found' }) }) + it('requires a live connector grant to execute a managed MCP credential', async () => { + mocks.loadBinding.mockResolvedValue(null) + const managedContext = { ...context, credentialType: 'mcp:fireflies' as const } + const principal = executorPrincipal() + const requireManagedAccess = () => + requireCredentialGroupCredentialAccess( + principal, + managedContext, + credentialOperations.useManagedMcp.resourcePolicy + ) + + await expect(requireManagedAccess()).resolves.toBeUndefined() + + mocks.requirePolicy.mockResolvedValue(storedPolicy([])) + await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'forbidden' }) + + mocks.requirePolicy.mockResolvedValue({ + document: buildOrganizationAccountAccessPolicy('group-1', [ + { + workspaceId: context.workspaceId, + access: { mode: 'selected', credentialTypes: ['oauth:gmail'] }, + }, + ]), + }) + await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'forbidden' }) + + mocks.requirePolicy.mockResolvedValue(storedPolicy()) + mocks.isAvailable.mockResolvedValue(false) + await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'not_found' }) + }) + it('rejects inconsistent Sim and external subject assertions before loading policy', async () => { const simPrincipal = executorPrincipal() simPrincipal.subjectUserId = 'user-2' diff --git a/apps/sim/lib/mcp/application/managed-connections.test.ts b/apps/sim/lib/mcp/application/managed-connections.test.ts index 9a85b92e116..c88c162c115 100644 --- a/apps/sim/lib/mcp/application/managed-connections.test.ts +++ b/apps/sim/lib/mcp/application/managed-connections.test.ts @@ -1,7 +1,7 @@ /** @vitest-environment node */ import type { SessionPrincipal } from '@sim/auth/principal' import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' -import { eq } from 'drizzle-orm' +import { eq, inArray } from 'drizzle-orm' import { beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ @@ -10,7 +10,8 @@ const mocks = vi.hoisted(() => ({ group: vi.fn(), workspace: vi.fn(), permission: vi.fn(), - requireAccess: vi.fn(), + scopedAvailable: vi.fn(), + policy: vi.fn(), })) vi.mock('@/lib/billing/core/workspace-access', () => ({ getWorkspaceOwnerSubscriptionAccess: mocks.billing, @@ -21,8 +22,11 @@ vi.mock('@/lib/credential-groups/availability', () => ({ vi.mock('@/lib/credential-groups/credentials', () => ({ loadScopedAccountsCredentialListContext: mocks.group, })) -vi.mock('@/lib/credential-groups/application/organization-workspace-access', () => ({ - requireOrganizationAccountsWorkspaceAccess: mocks.requireAccess, +vi.mock('@/lib/credential-groups/scoped-availability', () => ({ + isScopedCredentialGroupsAvailable: mocks.scopedAvailable, +})) +vi.mock('@/lib/resource-policies/repository', () => ({ + requireResourcePolicy: mocks.policy, })) vi.mock('@/lib/mcp/application/context', () => ({ resolveMcpWorkspaceContext: mocks.workspace })) vi.mock('@sim/platform-authz/workspace', () => ({ @@ -30,7 +34,10 @@ vi.mock('@sim/platform-authz/workspace', () => ({ resolveEffectiveWorkspacePermission: mocks.permission, })) -import { buildOrganizationAccountAccessPolicy } from '@/lib/credential-groups/application/workspace-access-policy' +import { + buildOrganizationAccountAccessPolicy, + organizationAccountAccessPolicyCodec, +} from '@/lib/credential-groups/application/workspace-access-policy' import { listManagedMcpConnectionsUseCase } from '@/lib/mcp/application/managed-connections' const principal: SessionPrincipal = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } @@ -53,6 +60,7 @@ describe('managed MCP connection catalog', () => { resetDbChainMock() mocks.billing.mockResolvedValue({ organizationId: 'org-1' }) mocks.available.mockResolvedValue(true) + mocks.scopedAvailable.mockResolvedValue(true) mocks.group.mockResolvedValue({ credentialGroupId: 'group-1' }) mocks.workspace.mockResolvedValue({ workspaceId: 'workspace-1', @@ -61,11 +69,11 @@ describe('managed MCP connection catalog', () => { billedAccountUserId: 'owner-1', }) mocks.permission.mockResolvedValue('read') - mocks.requireAccess.mockResolvedValue( - buildOrganizationAccountAccessPolicy('group-1', [ + mocks.policy.mockResolvedValue({ + document: buildOrganizationAccountAccessPolicy('group-1', [ { workspaceId: 'workspace-1', access: { mode: 'all' } }, - ]) - ) + ]), + }) }) it('uses organization ownership and workspace access before exposing credential operations', async () => { @@ -78,11 +86,12 @@ describe('managed MCP connection catalog', () => { ]) const result = await listManagedMcpConnectionsUseCase.execute({ principal, input }) expect(mocks.group).toHaveBeenCalledWith({ kind: 'organization', organizationId: 'org-1' }) - expect(mocks.requireAccess).toHaveBeenCalledWith( + expect(mocks.policy).toHaveBeenCalledWith( expect.objectContaining({ organizationId: 'org-1', - credentialGroupId: 'group-1', - workspaceId: 'workspace-1', + resourceType: 'credential_group', + resourceId: 'group-1', + codec: organizationAccountAccessPolicyCodec, }) ) expect(eq).toHaveBeenCalledWith(schemaMock.credential.organizationId, 'org-1') @@ -95,14 +104,93 @@ describe('managed MCP connection catalog', () => { }) }) - it('denies revoked workspace access before reading credentials', async () => { - mocks.requireAccess.mockRejectedValue(new Error('Workspace access revoked')) + it('returns an empty catalog when organization connected accounts are not configured', async () => { + mocks.group.mockResolvedValue(null) + await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual({ + servers: [], + tools: [], + }) + expect(mocks.policy).not.toHaveBeenCalled() + expect(dbChainMockFns.from).not.toHaveBeenCalled() + }) + + it.each(['workspace', 'organization'])( + 'returns an empty catalog when %s availability is disabled', + async (scope) => { + const available = scope === 'workspace' ? mocks.available : mocks.scopedAvailable + available.mockResolvedValue(false) + await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual( + { + servers: [], + tools: [], + } + ) + expect(mocks.policy).not.toHaveBeenCalled() + expect(dbChainMockFns.from).not.toHaveBeenCalled() + } + ) + + it.each([ + { name: 'no grants', grants: [] }, + { + name: 'another workspace only', + grants: [{ workspaceId: 'other-workspace', access: { mode: 'all' as const } }], + }, + { + name: 'OAuth only', + grants: [ + { + workspaceId: input.workspaceId, + access: { mode: 'selected' as const, credentialTypes: ['oauth:gmail' as const] }, + }, + ], + }, + ])('returns an empty catalog without an MCP workspace grant: $name', async ({ grants }) => { + mocks.policy.mockResolvedValue({ + document: buildOrganizationAccountAccessPolicy('group-1', grants), + }) + await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual({ + servers: [], + tools: [], + }) + expect(dbChainMockFns.from).not.toHaveBeenCalled() + }) + + it('only queries connectors granted to this workspace', async () => { + mocks.policy.mockResolvedValue({ + document: buildOrganizationAccountAccessPolicy('group-1', [ + { + workspaceId: input.workspaceId, + access: { mode: 'selected', credentialTypes: ['mcp:fireflies'] }, + }, + ]), + }) + await listManagedMcpConnectionsUseCase.execute({ principal, input }) + expect(inArray).toHaveBeenCalledWith(schemaMock.mcpServers.managedConnectorId, ['fireflies']) + }) + + it('rejects callers without workspace access before checking catalog availability', async () => { + mocks.permission.mockResolvedValue(null) await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).rejects.toThrow( - 'revoked' + 'Insufficient workspace permissions' ) + expect(mocks.billing).not.toHaveBeenCalled() + expect(mocks.policy).not.toHaveBeenCalled() expect(dbChainMockFns.from).not.toHaveBeenCalled() }) + it.each(['scopedAvailable', 'policy'] as const)( + 'propagates %s failures before reading credentials', + async (dependency) => { + const error = new Error('Database unavailable') + mocks[dependency].mockRejectedValue(error) + await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).rejects.toBe( + error + ) + expect(dbChainMockFns.from).not.toHaveBeenCalled() + } + ) + it.each([ [Array.from({ length: 501 }, () => metadata), 'connection limit'], [[{ ...metadata, toolSnapshotBytes: 6 * 1024 * 1024 }], 'metadata limit'], diff --git a/apps/sim/lib/mcp/application/managed-connections.ts b/apps/sim/lib/mcp/application/managed-connections.ts index c3e26b82102..853bc1f5895 100644 --- a/apps/sim/lib/mcp/application/managed-connections.ts +++ b/apps/sim/lib/mcp/application/managed-connections.ts @@ -3,17 +3,21 @@ import { credential, credentialGroup, credentialGroupEnrollment, mcpServers } fr import { and, asc, eq, inArray, isNotNull, isNull, or, sql } from 'drizzle-orm' import { getWorkspaceOwnerSubscriptionAccess } from '@/lib/billing/core/workspace-access' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' -import { requireOrganizationAccountsWorkspaceAccess } from '@/lib/credential-groups/application/organization-workspace-access' -import { organizationAccountPolicyAllowsWorkspace } from '@/lib/credential-groups/application/workspace-access-policy' +import { + organizationAccountAccessPolicyCodec, + organizationAccountPolicyAllowsWorkspace, +} from '@/lib/credential-groups/application/workspace-access-policy' import { isCredentialGroupsAvailable } from '@/lib/credential-groups/availability' import { loadScopedAccountsCredentialListContext } from '@/lib/credential-groups/credentials' import { getManagedMcpConnector, MANAGED_MCP_CONNECTOR_IDS, } from '@/lib/credential-groups/managed-mcp-connectors' +import { isScopedCredentialGroupsAvailable } from '@/lib/credential-groups/scoped-availability' import { resolveMcpWorkspaceContext } from '@/lib/mcp/application/context' import { mcpServerOperations } from '@/lib/mcp/application/operations' import type { McpToolSchema } from '@/lib/mcp/types' +import { requireResourcePolicy } from '@/lib/resource-policies/repository' const MAX_MANAGED_MCP_CONNECTIONS = 500 const MAX_MANAGED_MCP_CATALOG_BYTES = 5 * 1024 * 1024 @@ -52,13 +56,18 @@ export const listManagedMcpConnectionsUseCase = defineAuthorizedWorkspaceUseCase organizationId, }) if (!group) return { servers: [], tools: [] } - const policy = await requireOrganizationAccountsWorkspaceAccess({ - ...context, + if (!(await isScopedCredentialGroupsAvailable({ kind: 'organization', organizationId }))) { + return { servers: [], tools: [] } + } + const policy = await requireResourcePolicy({ organizationId, - credentialGroupId: group.credentialGroupId, + resourceType: 'credential_group', + resourceId: group.credentialGroupId, + codec: organizationAccountAccessPolicyCodec, }) + /** Catalogs omit unavailable credentials; execution still requires explicit workspace access. */ const allowedConnectorIds = MANAGED_MCP_CONNECTOR_IDS.filter((id) => - organizationAccountPolicyAllowsWorkspace(policy, context.workspaceId, `mcp:${id}`) + organizationAccountPolicyAllowsWorkspace(policy.document, context.workspaceId, `mcp:${id}`) ) if (!allowedConnectorIds.length) return { servers: [], tools: [] } const managedCatalogScope = () =>