Skip to content

Commit f667da3

Browse files
fix(mcp): list servers without Credential Group access
1 parent 17e415d commit f667da3

4 files changed

Lines changed: 219 additions & 21 deletions

File tree

‎apps/sim/hooks/queries/mcp.test.tsx‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
useAllowedMcpDomains,
2828
useForceRefreshMcpTools,
2929
useMcpServers,
30+
useMcpToolServers,
3031
useMcpToolsQuery,
3132
useStoredMcpTools,
3233
} from '@/hooks/queries/mcp'
@@ -118,6 +119,75 @@ class FakeEventSource {
118119
close(): void {}
119120
}
120121

122+
describe('useMcpToolServers', () => {
123+
beforeEach(() => {
124+
vi.clearAllMocks()
125+
})
126+
127+
it('lists ordinary workspace servers when no managed connections are available', async () => {
128+
const sharedServer = server('shared-server')
129+
mockServers([
130+
sharedServer,
131+
server('managed-canonical-server', { credentialGroupId: 'group-1' }),
132+
])
133+
134+
const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID))
135+
await flush()
136+
137+
expect(hook.getResult()).toEqual({ data: [sharedServer], isLoading: false, error: null })
138+
hook.unmount()
139+
})
140+
141+
it('includes allowed managed connections alongside ordinary servers', async () => {
142+
const sharedServer = server('shared-server')
143+
const managedServer = server('mcp-cg-123456789012345678901', {
144+
name: 'Fireflies — person@example.com',
145+
managedConnectorId: 'fireflies',
146+
authType: 'oauth',
147+
url: undefined,
148+
})
149+
mockRequestJson.mockImplementation(async (contract) => {
150+
if (contract === listMcpServersContract) {
151+
return { success: true, data: { servers: [sharedServer] } }
152+
}
153+
if (contract === listManagedMcpCatalogContract) return { servers: [managedServer], tools: [] }
154+
throw new Error('Unexpected MCP request')
155+
})
156+
157+
const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID))
158+
await flush()
159+
160+
expect(hook.getResult()).toEqual({
161+
data: [sharedServer, managedServer],
162+
isLoading: false,
163+
error: null,
164+
})
165+
hook.unmount()
166+
})
167+
168+
it.each([
169+
{ name: 'shared servers', failingContract: listMcpServersContract },
170+
{ name: 'managed catalog', failingContract: listManagedMcpCatalogContract },
171+
])('keeps unrelated $name errors visible', async ({ failingContract }) => {
172+
const error = new Error('MCP request failed')
173+
mockRequestJson.mockImplementation(async (contract) => {
174+
if (contract === failingContract) throw error
175+
if (contract === listMcpServersContract) {
176+
return { success: true, data: { servers: [server('shared-server')] } }
177+
}
178+
if (contract === listManagedMcpCatalogContract) return { servers: [], tools: [] }
179+
throw new Error('Unexpected MCP request')
180+
})
181+
182+
const hook = renderHookWithClient(() => useMcpToolServers(WORKSPACE_ID))
183+
await flush()
184+
185+
expect(hook.getResult().error).toBe(error)
186+
expect(hook.getResult().isLoading).toBe(false)
187+
hook.unmount()
188+
})
189+
})
190+
121191
describe('useMcpToolsQuery', () => {
122192
beforeEach(() => {
123193
vi.clearAllMocks()

‎apps/sim/lib/credential-groups/application/authorization.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,37 @@ describe('requireCredentialGroupCredentialAccess', () => {
276276
await expect(requireAccess(executorPrincipal())).rejects.toMatchObject({ code: 'not_found' })
277277
})
278278

279+
it('requires a live connector grant to execute a managed MCP credential', async () => {
280+
mocks.loadBinding.mockResolvedValue(null)
281+
const managedContext = { ...context, credentialType: 'mcp:fireflies' as const }
282+
const principal = executorPrincipal()
283+
const requireManagedAccess = () =>
284+
requireCredentialGroupCredentialAccess(
285+
principal,
286+
managedContext,
287+
credentialOperations.useManagedMcp.resourcePolicy
288+
)
289+
290+
await expect(requireManagedAccess()).resolves.toBeUndefined()
291+
292+
mocks.requirePolicy.mockResolvedValue(storedPolicy([]))
293+
await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'forbidden' })
294+
295+
mocks.requirePolicy.mockResolvedValue({
296+
document: buildOrganizationAccountAccessPolicy('group-1', [
297+
{
298+
workspaceId: context.workspaceId,
299+
access: { mode: 'selected', credentialTypes: ['oauth:gmail'] },
300+
},
301+
]),
302+
})
303+
await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'forbidden' })
304+
305+
mocks.requirePolicy.mockResolvedValue(storedPolicy())
306+
mocks.isAvailable.mockResolvedValue(false)
307+
await expect(requireManagedAccess()).rejects.toMatchObject({ code: 'not_found' })
308+
})
309+
279310
it('rejects inconsistent Sim and external subject assertions before loading policy', async () => {
280311
const simPrincipal = executorPrincipal()
281312
simPrincipal.subjectUserId = 'user-2'

‎apps/sim/lib/mcp/application/managed-connections.test.ts‎

Lines changed: 103 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/** @vitest-environment node */
22
import type { SessionPrincipal } from '@sim/auth/principal'
33
import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
4-
import { eq } from 'drizzle-orm'
4+
import { eq, inArray } from 'drizzle-orm'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66

77
const mocks = vi.hoisted(() => ({
@@ -10,7 +10,8 @@ const mocks = vi.hoisted(() => ({
1010
group: vi.fn(),
1111
workspace: vi.fn(),
1212
permission: vi.fn(),
13-
requireAccess: vi.fn(),
13+
scopedAvailable: vi.fn(),
14+
policy: vi.fn(),
1415
}))
1516
vi.mock('@/lib/billing/core/workspace-access', () => ({
1617
getWorkspaceOwnerSubscriptionAccess: mocks.billing,
@@ -21,16 +22,22 @@ vi.mock('@/lib/credential-groups/availability', () => ({
2122
vi.mock('@/lib/credential-groups/credentials', () => ({
2223
loadScopedAccountsCredentialListContext: mocks.group,
2324
}))
24-
vi.mock('@/lib/credential-groups/application/organization-workspace-access', () => ({
25-
requireOrganizationAccountsWorkspaceAccess: mocks.requireAccess,
25+
vi.mock('@/lib/credential-groups/scoped-availability', () => ({
26+
isScopedCredentialGroupsAvailable: mocks.scopedAvailable,
27+
}))
28+
vi.mock('@/lib/resource-policies/repository', () => ({
29+
requireResourcePolicy: mocks.policy,
2630
}))
2731
vi.mock('@/lib/mcp/application/context', () => ({ resolveMcpWorkspaceContext: mocks.workspace }))
2832
vi.mock('@sim/platform-authz/workspace', () => ({
2933
permissionSatisfies: (permission: string | null) => permission !== null,
3034
resolveEffectiveWorkspacePermission: mocks.permission,
3135
}))
3236

33-
import { buildOrganizationAccountAccessPolicy } from '@/lib/credential-groups/application/workspace-access-policy'
37+
import {
38+
buildOrganizationAccountAccessPolicy,
39+
organizationAccountAccessPolicyCodec,
40+
} from '@/lib/credential-groups/application/workspace-access-policy'
3441
import { listManagedMcpConnectionsUseCase } from '@/lib/mcp/application/managed-connections'
3542

3643
const principal: SessionPrincipal = { kind: 'session', userId: 'user-1', sessionId: 'session-1' }
@@ -53,6 +60,7 @@ describe('managed MCP connection catalog', () => {
5360
resetDbChainMock()
5461
mocks.billing.mockResolvedValue({ organizationId: 'org-1' })
5562
mocks.available.mockResolvedValue(true)
63+
mocks.scopedAvailable.mockResolvedValue(true)
5664
mocks.group.mockResolvedValue({ credentialGroupId: 'group-1' })
5765
mocks.workspace.mockResolvedValue({
5866
workspaceId: 'workspace-1',
@@ -61,11 +69,11 @@ describe('managed MCP connection catalog', () => {
6169
billedAccountUserId: 'owner-1',
6270
})
6371
mocks.permission.mockResolvedValue('read')
64-
mocks.requireAccess.mockResolvedValue(
65-
buildOrganizationAccountAccessPolicy('group-1', [
72+
mocks.policy.mockResolvedValue({
73+
document: buildOrganizationAccountAccessPolicy('group-1', [
6674
{ workspaceId: 'workspace-1', access: { mode: 'all' } },
67-
])
68-
)
75+
]),
76+
})
6977
})
7078

7179
it('uses organization ownership and workspace access before exposing credential operations', async () => {
@@ -78,11 +86,12 @@ describe('managed MCP connection catalog', () => {
7886
])
7987
const result = await listManagedMcpConnectionsUseCase.execute({ principal, input })
8088
expect(mocks.group).toHaveBeenCalledWith({ kind: 'organization', organizationId: 'org-1' })
81-
expect(mocks.requireAccess).toHaveBeenCalledWith(
89+
expect(mocks.policy).toHaveBeenCalledWith(
8290
expect.objectContaining({
8391
organizationId: 'org-1',
84-
credentialGroupId: 'group-1',
85-
workspaceId: 'workspace-1',
92+
resourceType: 'credential_group',
93+
resourceId: 'group-1',
94+
codec: organizationAccountAccessPolicyCodec,
8695
})
8796
)
8897
expect(eq).toHaveBeenCalledWith(schemaMock.credential.organizationId, 'org-1')
@@ -95,14 +104,93 @@ describe('managed MCP connection catalog', () => {
95104
})
96105
})
97106

98-
it('denies revoked workspace access before reading credentials', async () => {
99-
mocks.requireAccess.mockRejectedValue(new Error('Workspace access revoked'))
107+
it('returns an empty catalog when organization connected accounts are not configured', async () => {
108+
mocks.group.mockResolvedValue(null)
109+
await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual({
110+
servers: [],
111+
tools: [],
112+
})
113+
expect(mocks.policy).not.toHaveBeenCalled()
114+
expect(dbChainMockFns.from).not.toHaveBeenCalled()
115+
})
116+
117+
it.each(['workspace', 'organization'])(
118+
'returns an empty catalog when %s availability is disabled',
119+
async (scope) => {
120+
const available = scope === 'workspace' ? mocks.available : mocks.scopedAvailable
121+
available.mockResolvedValue(false)
122+
await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual(
123+
{
124+
servers: [],
125+
tools: [],
126+
}
127+
)
128+
expect(mocks.policy).not.toHaveBeenCalled()
129+
expect(dbChainMockFns.from).not.toHaveBeenCalled()
130+
}
131+
)
132+
133+
it.each([
134+
{ name: 'no grants', grants: [] },
135+
{
136+
name: 'another workspace only',
137+
grants: [{ workspaceId: 'other-workspace', access: { mode: 'all' as const } }],
138+
},
139+
{
140+
name: 'OAuth only',
141+
grants: [
142+
{
143+
workspaceId: input.workspaceId,
144+
access: { mode: 'selected' as const, credentialTypes: ['oauth:gmail' as const] },
145+
},
146+
],
147+
},
148+
])('returns an empty catalog without an MCP workspace grant: $name', async ({ grants }) => {
149+
mocks.policy.mockResolvedValue({
150+
document: buildOrganizationAccountAccessPolicy('group-1', grants),
151+
})
152+
await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).resolves.toEqual({
153+
servers: [],
154+
tools: [],
155+
})
156+
expect(dbChainMockFns.from).not.toHaveBeenCalled()
157+
})
158+
159+
it('only queries connectors granted to this workspace', async () => {
160+
mocks.policy.mockResolvedValue({
161+
document: buildOrganizationAccountAccessPolicy('group-1', [
162+
{
163+
workspaceId: input.workspaceId,
164+
access: { mode: 'selected', credentialTypes: ['mcp:fireflies'] },
165+
},
166+
]),
167+
})
168+
await listManagedMcpConnectionsUseCase.execute({ principal, input })
169+
expect(inArray).toHaveBeenCalledWith(schemaMock.mcpServers.managedConnectorId, ['fireflies'])
170+
})
171+
172+
it('rejects callers without workspace access before checking catalog availability', async () => {
173+
mocks.permission.mockResolvedValue(null)
100174
await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).rejects.toThrow(
101-
'revoked'
175+
'Insufficient workspace permissions'
102176
)
177+
expect(mocks.billing).not.toHaveBeenCalled()
178+
expect(mocks.policy).not.toHaveBeenCalled()
103179
expect(dbChainMockFns.from).not.toHaveBeenCalled()
104180
})
105181

182+
it.each(['scopedAvailable', 'policy'] as const)(
183+
'propagates %s failures before reading credentials',
184+
async (dependency) => {
185+
const error = new Error('Database unavailable')
186+
mocks[dependency].mockRejectedValue(error)
187+
await expect(listManagedMcpConnectionsUseCase.execute({ principal, input })).rejects.toBe(
188+
error
189+
)
190+
expect(dbChainMockFns.from).not.toHaveBeenCalled()
191+
}
192+
)
193+
106194
it.each([
107195
[Array.from({ length: 501 }, () => metadata), 'connection limit'],
108196
[[{ ...metadata, toolSnapshotBytes: 6 * 1024 * 1024 }], 'metadata limit'],

‎apps/sim/lib/mcp/application/managed-connections.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,21 @@ import { credential, credentialGroup, credentialGroupEnrollment, mcpServers } fr
33
import { and, asc, eq, inArray, isNotNull, isNull, or, sql } from 'drizzle-orm'
44
import { getWorkspaceOwnerSubscriptionAccess } from '@/lib/billing/core/workspace-access'
55
import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
6-
import { requireOrganizationAccountsWorkspaceAccess } from '@/lib/credential-groups/application/organization-workspace-access'
7-
import { organizationAccountPolicyAllowsWorkspace } from '@/lib/credential-groups/application/workspace-access-policy'
6+
import {
7+
organizationAccountAccessPolicyCodec,
8+
organizationAccountPolicyAllowsWorkspace,
9+
} from '@/lib/credential-groups/application/workspace-access-policy'
810
import { isCredentialGroupsAvailable } from '@/lib/credential-groups/availability'
911
import { loadScopedAccountsCredentialListContext } from '@/lib/credential-groups/credentials'
1012
import {
1113
getManagedMcpConnector,
1214
MANAGED_MCP_CONNECTOR_IDS,
1315
} from '@/lib/credential-groups/managed-mcp-connectors'
16+
import { isScopedCredentialGroupsAvailable } from '@/lib/credential-groups/scoped-availability'
1417
import { resolveMcpWorkspaceContext } from '@/lib/mcp/application/context'
1518
import { mcpServerOperations } from '@/lib/mcp/application/operations'
1619
import type { McpToolSchema } from '@/lib/mcp/types'
20+
import { requireResourcePolicy } from '@/lib/resource-policies/repository'
1721

1822
const MAX_MANAGED_MCP_CONNECTIONS = 500
1923
const MAX_MANAGED_MCP_CATALOG_BYTES = 5 * 1024 * 1024
@@ -52,13 +56,18 @@ export const listManagedMcpConnectionsUseCase = defineAuthorizedWorkspaceUseCase
5256
organizationId,
5357
})
5458
if (!group) return { servers: [], tools: [] }
55-
const policy = await requireOrganizationAccountsWorkspaceAccess({
56-
...context,
59+
if (!(await isScopedCredentialGroupsAvailable({ kind: 'organization', organizationId }))) {
60+
return { servers: [], tools: [] }
61+
}
62+
const policy = await requireResourcePolicy({
5763
organizationId,
58-
credentialGroupId: group.credentialGroupId,
64+
resourceType: 'credential_group',
65+
resourceId: group.credentialGroupId,
66+
codec: organizationAccountAccessPolicyCodec,
5967
})
68+
/** Catalogs omit unavailable credentials; execution still requires explicit workspace access. */
6069
const allowedConnectorIds = MANAGED_MCP_CONNECTOR_IDS.filter((id) =>
61-
organizationAccountPolicyAllowsWorkspace(policy, context.workspaceId, `mcp:${id}`)
70+
organizationAccountPolicyAllowsWorkspace(policy.document, context.workspaceId, `mcp:${id}`)
6271
)
6372
if (!allowedConnectorIds.length) return { servers: [], tools: [] }
6473
const managedCatalogScope = () =>

0 commit comments

Comments
 (0)