diff --git a/workspaces/orchestrator/plugins/orchestrator-backend/src/__testUtils__/mcpTestUtils.ts b/workspaces/orchestrator/plugins/orchestrator-backend/src/__testUtils__/mcpTestUtils.ts index d3abeb3d6d4..6f98130cfab 100644 --- a/workspaces/orchestrator/plugins/orchestrator-backend/src/__testUtils__/mcpTestUtils.ts +++ b/workspaces/orchestrator/plugins/orchestrator-backend/src/__testUtils__/mcpTestUtils.ts @@ -28,20 +28,13 @@ import { import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; +import { ORCHESTRATOR_WORKFLOW_RESOURCE_TYPE } from '@red-hat-developer-hub/backstage-plugin-orchestrator-common'; + import type { Server } from 'node:http'; -// Must be imported after the `jest.mock` calls above so that `orchestratorPlugin`'s -// transitive imports resolve to the mocked service classes. -// eslint-disable-next-line import/first +// eslint-disable-next-line import/first -- must follow jest.mock setup above import { orchestratorPlugin } from '../plugin'; -// Orchestrator has no official test double for SonataFlowService / -// DataIndexService / WorkflowCacheService / OrchestratorService (unlike e.g. -// Scorecard's `catalogServiceMock`). The only established convention for -// stubbing them in this package is `service/router.test.ts`'s same-package, -// relative-path `jest.mock(...)` calls, reused here verbatim so the real -// `orchestratorPlugin` boots through `startTestBackend` without touching a -// real SonataFlow/DataIndex service. jest.mock('../service/DataIndexService', () => ({ DataIndexService: jest.fn().mockImplementation(() => ({})), })); @@ -73,9 +66,11 @@ jest.mock('../service/OrchestratorService', () => ({ .mockImplementation(() => mockOrchestratorService), })); -export type BackendPermissionMode = 'allow-all' | 'deny-all'; +export type BackendPermissionMode = + 'allow-all' | 'deny-all' | 'conditional-workflow1-only'; export type StartMcpBackendOptions = { + pluginSources?: string[]; permissionMode?: BackendPermissionMode; }; @@ -87,19 +82,48 @@ function getServerPort(server: Server): number { return address.port; } +function conditionalWorkflowDecision(workflowIds: string[]): PolicyDecision { + return { + result: AuthorizeResult.CONDITIONAL, + pluginId: 'orchestrator', + resourceType: ORCHESTRATOR_WORKFLOW_RESOURCE_TYPE, + conditions: { + anyOf: [ + { + rule: 'IS_ALLOWED_WORKFLOW_ID', + resourceType: ORCHESTRATOR_WORKFLOW_RESOURCE_TYPE, + params: { workflowIds }, + }, + ], + }, + } as PolicyDecision; +} + function createPermissionsFactory(mode: BackendPermissionMode) { - const decision: PolicyDecision = - mode === 'deny-all' - ? { result: AuthorizeResult.DENY } - : { result: AuthorizeResult.ALLOW }; + if (mode === 'deny-all') { + return mockServices.permissions.mock({ + authorize: async () => [{ result: AuthorizeResult.DENY }], + authorizeConditional: async () => [{ result: AuthorizeResult.DENY }], + }).factory; + } + + if (mode === 'conditional-workflow1-only') { + return mockServices.permissions.mock({ + authorize: async () => [{ result: AuthorizeResult.DENY }], + authorizeConditional: async () => [ + conditionalWorkflowDecision(['workflow1']), + ], + }).factory; + } return mockServices.permissions.mock({ - authorize: async () => [decision], - authorizeConditional: async () => [decision], + authorize: async () => [{ result: AuthorizeResult.ALLOW }], + authorizeConditional: async () => [{ result: AuthorizeResult.ALLOW }], }).factory; } export async function startMcpBackend({ + pluginSources = ['orchestrator'], permissionMode = 'allow-all', }: StartMcpBackendOptions = {}) { return startTestBackend({ @@ -110,7 +134,7 @@ export async function startMcpBackend({ data: { backend: { baseUrl: 'http://localhost:7007', - actions: { pluginSources: ['orchestrator'] }, + actions: { pluginSources }, }, orchestrator: { dataIndexService: { url: 'http://localhost:8080' }, diff --git a/workspaces/orchestrator/plugins/orchestrator-backend/src/mcp-tools.integration.test.ts b/workspaces/orchestrator/plugins/orchestrator-backend/src/mcp-tools.integration.test.ts index c9e95b697c2..53f50ed8a4d 100644 --- a/workspaces/orchestrator/plugins/orchestrator-backend/src/mcp-tools.integration.test.ts +++ b/workspaces/orchestrator/plugins/orchestrator-backend/src/mcp-tools.integration.test.ts @@ -27,17 +27,6 @@ import { withMcpClient, } from './__testUtils__/mcpTestUtils'; -/** - * Pyramid invariant (RHIDP-14041): the per-action unit tests - * (`actions/*.test.ts`) already prove each action's branching/mapping logic - * against mocked services. This suite proves the remaining, un-mocked - * concern - that all 5 actions are actually *wired*: registered by the real - * `orchestratorPlugin`, reachable through the real `@backstage/plugin-mcp-actions-backend` - * plugin, and callable end-to-end by a real `@modelcontextprotocol/sdk` - * client - not just invokable directly against `ActionsRegistryService` - * mocks. Mirrors Scorecard PR #3332's `mcp-tools.integration.test.ts`. - */ - type McpTestBackend = Awaited>; const ORCHESTRATOR_TOOL_NAMES = [ @@ -74,10 +63,14 @@ function rawInstance(overrides: Record = {}) { describe('Orchestrator MCP tools integration', () => { let allowedBackend: McpTestBackend; let deniedBackend: McpTestBackend; + let conditionalBackend: McpTestBackend; beforeAll(async () => { allowedBackend = await startMcpBackend({ permissionMode: 'allow-all' }); deniedBackend = await startMcpBackend({ permissionMode: 'deny-all' }); + conditionalBackend = await startMcpBackend({ + permissionMode: 'conditional-workflow1-only', + }); }); beforeEach(() => { @@ -98,7 +91,7 @@ describe('Orchestrator MCP tools integration', () => { }); }); - it('marks the 4 read actions read-only and execute-workflow not read-only', async () => { + it('marks read tools read-only and execute-workflow destructive', async () => { await withMcpClient(allowedBackend.server, async client => { const result = await client.request( { method: 'tools/list' }, @@ -111,16 +104,41 @@ describe('Orchestrator MCP tools integration', () => { for (const toolName of READ_ONLY_TOOL_NAMES) { expect(toolsByName[toolName]?.annotations?.readOnlyHint).toBe(true); + expect(typeof toolsByName[toolName]?.annotations?.destructiveHint).toBe( + 'boolean', + ); } + expect( toolsByName['orchestrator.execute-workflow']?.annotations?.readOnlyHint, ).toBe(false); + expect( + toolsByName['orchestrator.execute-workflow']?.annotations + ?.destructiveHint, + ).toBe(true); + }); + }); + + it('hides orchestrator tools when orchestrator is not in pluginSources', async () => { + const filteredBackend = await startMcpBackend({ pluginSources: [] }); + + await withMcpClient(filteredBackend.server, async client => { + const result = await client.request( + { method: 'tools/list' }, + ListToolsResultSchema, + ); + + const toolNames = result.tools.map(tool => tool.name); + for (const toolName of ORCHESTRATOR_TOOL_NAMES) { + expect(toolNames).not.toContain(toolName); + } }); }); it('calls orchestrator.list-workflows and returns configured workflows', async () => { mockOrchestratorService.fetchWorkflowOverviews.mockResolvedValue([ { workflowId: 'workflow1', name: 'Onboard Employee' }, + { workflowId: 'workflow2', name: 'Offboard Employee' }, ]); await withMcpClient(allowedBackend.server, async client => { @@ -134,7 +152,42 @@ describe('Orchestrator MCP tools integration', () => { }; expect(result.isError).not.toBe(true); - expect(output.workflows.map(w => w.workflowId)).toContain('workflow1'); + expect(output.workflows.map(w => w.workflowId)).toEqual( + expect.arrayContaining(['workflow1', 'workflow2']), + ); + }); + }); + + it('filters orchestrator.list-workflows for conditional workflow permissions', async () => { + mockOrchestratorService.fetchWorkflowOverviews.mockResolvedValue([ + { workflowId: 'workflow1', name: 'Onboard Employee' }, + { workflowId: 'workflow2', name: 'Offboard Employee' }, + ]); + + await withMcpClient(conditionalBackend.server, async client => { + const result = await client.callTool( + { name: 'orchestrator.list-workflows', arguments: {} }, + CallToolResultSchema, + ); + + const output = parseCallToolOutput(result) as { + workflows: Array<{ workflowId: string }>; + }; + + expect(result.isError).not.toBe(true); + expect(output.workflows.map(w => w.workflowId)).toEqual(['workflow1']); + }); + }); + + it('returns a tool error when orchestrator.list-workflows access is denied', async () => { + await withMcpClient(deniedBackend.server, async client => { + const result = await client.callTool( + { name: 'orchestrator.list-workflows', arguments: {} }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('denied'); }); }); @@ -191,6 +244,36 @@ describe('Orchestrator MCP tools integration', () => { }); }); + it('returns validation error when get-workflow-schema workflowId is missing', async () => { + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.get-workflow-schema', + arguments: {}, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('workflowId'); + }); + }); + + it('returns validation error when get-workflow-schema workflowId has invalid type', async () => { + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.get-workflow-schema', + arguments: { workflowId: 12345 }, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('workflowId'); + }); + }); + it('calls orchestrator.execute-workflow and returns the new instance id and status', async () => { mockOrchestratorService.fetchWorkflowInfo.mockResolvedValue({ id: 'workflow1', @@ -223,6 +306,69 @@ describe('Orchestrator MCP tools integration', () => { }); }); + it('returns a tool error when execute-workflow inputs fail schema validation', async () => { + mockOrchestratorService.fetchWorkflowInfo.mockResolvedValue({ + id: 'workflow1', + serviceUrl: 'http://localhost:8080', + }); + mockOrchestratorService.fetchWorkflowDefinition.mockResolvedValue({ + id: 'workflow1', + dataInputSchema: 'schema.json', + }); + mockOrchestratorService.fetchWorkflowInfoOnService.mockResolvedValue({ + id: 'workflow1', + inputSchema: { + type: 'object', + properties: { name: { type: 'string' } }, + required: ['name'], + }, + }); + + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.execute-workflow', + arguments: { workflowId: 'workflow1', inputs: {} }, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('Invalid inputs'); + expect(parseCallToolError(result)).toContain('workflow1'); + }); + }); + + it('returns a tool error when orchestrator.execute-workflow access is denied', async () => { + await withMcpClient(deniedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.execute-workflow', + arguments: { workflowId: 'workflow1', inputs: {} }, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('denied'); + }); + }); + + it('returns validation error when execute-workflow workflowId is missing', async () => { + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.execute-workflow', + arguments: { inputs: {} }, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('workflowId'); + }); + }); + it('calls orchestrator.list-instances and returns configured instances', async () => { mockOrchestratorService.getWorkflowIds.mockReturnValue(['workflow1']); mockOrchestratorService.fetchInstances.mockResolvedValue([rawInstance()]); @@ -261,15 +407,50 @@ describe('Orchestrator MCP tools integration', () => { }); }); - it('returns a tool error when access is denied', async () => { - await withMcpClient(deniedBackend.server, async client => { + it('returns a tool error when orchestrator.get-instance targets a missing instance', async () => { + mockOrchestratorService.fetchInstance.mockResolvedValue(undefined); + + await withMcpClient(allowedBackend.server, async client => { const result = await client.callTool( - { name: 'orchestrator.list-workflows', arguments: {} }, + { + name: 'orchestrator.get-instance', + arguments: { instanceId: 'missing-instance' }, + }, CallToolResultSchema, ); expect(result.isError).toBe(true); - expect(parseCallToolError(result)).toContain('denied'); + expect(parseCallToolError(result)).toContain('missing-instance'); + }); + }); + + it('returns validation error when get-instance instanceId is missing', async () => { + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.get-instance', + arguments: {}, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('instanceId'); + }); + }); + + it('returns validation error when get-instance instanceId has invalid type', async () => { + await withMcpClient(allowedBackend.server, async client => { + const result = await client.callTool( + { + name: 'orchestrator.get-instance', + arguments: { instanceId: 12345 }, + }, + CallToolResultSchema, + ); + + expect(result.isError).toBe(true); + expect(parseCallToolError(result)).toContain('instanceId'); }); }); });