Skip to content

Commit 81b47d6

Browse files
committed
Unify MCP authorization classification (#5596)
- use the shared OAuth-required classifier in aggregate discovery - keep server summaries consistent for redirect-required auth - cover both sibling paths with focused regressions
1 parent 246f145 commit 81b47d6

2 files changed

Lines changed: 43 additions & 8 deletions

File tree

apps/sim/lib/mcp/service.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,47 @@ describe('McpService.discoverTools per-server caching', () => {
392392
expect(mockListTools).toHaveBeenCalledTimes(1)
393393
})
394394

395+
it('treats an OAuth redirect during aggregate discovery as authorization pending', async () => {
396+
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })])
397+
mockConnect.mockRejectedValueOnce(
398+
new McpOauthRedirectRequired('https://mcp-a.example.com/authorize')
399+
)
400+
401+
await expect(mcpService.discoverTools(USER_ID, WORKSPACE_ID)).resolves.toEqual([])
402+
expect(mockDbUpdateSet).toHaveBeenCalledTimes(1)
403+
expect(mockDbUpdateSet).toHaveBeenCalledWith({
404+
connectionStatus: 'disconnected',
405+
lastError: null,
406+
updatedAt: expect.any(Date),
407+
})
408+
409+
mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')])
410+
const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID)
411+
412+
expect(tools.map((item) => item.name)).toEqual(['a1'])
413+
expect(mockListTools).toHaveBeenCalledTimes(1)
414+
})
415+
416+
it('reports an OAuth redirect as a disconnected server summary without a hard error', async () => {
417+
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })])
418+
mockConnect.mockRejectedValueOnce(
419+
new McpOauthRedirectRequired('https://mcp-a.example.com/authorize')
420+
)
421+
422+
await expect(mcpService.getServerSummaries(USER_ID, WORKSPACE_ID)).resolves.toEqual([
423+
{
424+
id: 'mcp-a',
425+
name: 'A',
426+
url: 'https://mcp-a.example.com/mcp',
427+
transport: 'streamable-http',
428+
status: 'disconnected',
429+
toolCount: 0,
430+
lastSeen: undefined,
431+
error: undefined,
432+
},
433+
])
434+
})
435+
395436
it('verifies a server with a force-refreshed live tools handshake', async () => {
396437
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
397438
mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a'), tool('a2', 'mcp-a')])

apps/sim/lib/mcp/service.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -541,10 +541,7 @@ class McpService {
541541
await client.disconnect()
542542
}
543543
} catch (error) {
544-
if (
545-
error instanceof McpOauthAuthorizationRequiredError ||
546-
error instanceof UnauthorizedError
547-
) {
544+
if (isMcpAuthorizationRequired(error)) {
548545
return { kind: 'oauth-pending' }
549546
}
550547
return {
@@ -809,10 +806,7 @@ class McpService {
809806
error: undefined,
810807
})
811808
} catch (error) {
812-
if (
813-
error instanceof McpOauthAuthorizationRequiredError ||
814-
error instanceof UnauthorizedError
815-
) {
809+
if (isMcpAuthorizationRequired(error)) {
816810
summaries.push({
817811
id: config.id,
818812
name: config.name,

0 commit comments

Comments
 (0)