diff --git a/packages/@n8n/backend-services/src/services/__tests__/auth-strategy.registry.test.ts b/packages/@n8n/backend-services/src/services/__tests__/auth-strategy.registry.test.ts index 02ee86772b2..6b22676a9e2 100644 --- a/packages/@n8n/backend-services/src/services/__tests__/auth-strategy.registry.test.ts +++ b/packages/@n8n/backend-services/src/services/__tests__/auth-strategy.registry.test.ts @@ -174,7 +174,7 @@ describe('AuthStrategyRegistry', () => { expect(strategy3.buildTokenGrant).not.toHaveBeenCalled(); }); - it('returns null when all strategies abstain or when a strategy returns false (fail-fast)', async () => { + it('returns null when all strategies abstain', async () => { const emptyRegistry = new AuthStrategyRegistry(); expect(await emptyRegistry.buildContextFromToken('token')).toBeNull(); @@ -182,14 +182,16 @@ describe('AuthStrategyRegistry', () => { abstain.buildTokenGrant.mockResolvedValue(null); registry.register(abstain); expect(await registry.buildContextFromToken('token')).toBeNull(); + }); + it('returns false and stops when a strategy refuses the token (fail-fast)', async () => { const unreached = mock(); const failing = mock(); failing.buildTokenGrant.mockResolvedValue(false); const failRegistry = new AuthStrategyRegistry(); failRegistry.register(failing); failRegistry.register(unreached); - expect(await failRegistry.buildContextFromToken('token')).toBeNull(); + expect(await failRegistry.buildContextFromToken('token')).toBe(false); expect(unreached.buildTokenGrant).not.toHaveBeenCalled(); }); }); diff --git a/packages/@n8n/backend-services/src/services/auth-strategy.registry.ts b/packages/@n8n/backend-services/src/services/auth-strategy.registry.ts index c0113c0c925..2bfbc4e10f3 100644 --- a/packages/@n8n/backend-services/src/services/auth-strategy.registry.ts +++ b/packages/@n8n/backend-services/src/services/auth-strategy.registry.ts @@ -28,22 +28,19 @@ export class AuthStrategyRegistry { this.strategies.push(strategy); } + /** + * Asks each strategy in order. `false`: a strategy recognised the token and refused it. + * `null`: every strategy abstained, so no strategy here owns the token. + */ async buildContextFromToken( token: string, options?: AuthStrategyOptions, - ): Promise { + ): Promise { for (const strategy of this.strategies) { const result = await strategy.buildTokenGrant(token, options); - if (result === false) { - // this strategy was responsible, but the token validation failed - return null; - } - if (result !== null) { - return result; // true = success, false = fail fast - } - // null = this strategy abstained, continue to next + if (result !== null) return result; } - return null; // all strategies abstained = unauthenticated + return null; } async authenticate(req: AuthenticatedRequest): Promise { diff --git a/packages/@n8n/backend-services/src/services/token-purposes.ts b/packages/@n8n/backend-services/src/services/token-purposes.ts index b9f765fbc4a..295f8999295 100644 --- a/packages/@n8n/backend-services/src/services/token-purposes.ts +++ b/packages/@n8n/backend-services/src/services/token-purposes.ts @@ -38,9 +38,10 @@ export type TokenPurpose = keyof typeof TOKEN_PURPOSES; * Known overlap: `mcp-server-api` is also the MCP protected resource's * pre-RFC-8707 audience (`LEGACY_MCP_AUDIENCE` in `mcp-protected-resource.ts`), * so the audience alone does not tell an MCP API key from an OAuth access token - * minted for that resource. What separates them is the signed `meta.isOAuth` - * claim the MCP middleware routes on, and the row each kind must have — in - * `api_key` for one, `oauth_access_token` for the other. + * minted for that resource. What separates them is the `iss` claim the MCP + * middleware routes on — `n8n` for an API key, an http(s) URL for an access + * token — and the row each kind must have — in `api_key` for one, + * `oauth_access_token` for the other. * * Giving API keys an audience of their own would invalidate every key in the * field: they are persisted, handed to clients, and minted without an expiry, diff --git a/packages/@n8n/inbound-auth/src/__tests__/inbound.test.ts b/packages/@n8n/inbound-auth/src/__tests__/inbound.test.ts index ade1e919574..8ce0d89e361 100644 --- a/packages/@n8n/inbound-auth/src/__tests__/inbound.test.ts +++ b/packages/@n8n/inbound-auth/src/__tests__/inbound.test.ts @@ -1,5 +1,5 @@ import type { ProtectedResource } from '../contracts'; -import { extractBearer, inboundFrom, inboundRequestOf } from '../inbound'; +import { extractBearer, inboundFrom, inboundRequestOf, isUrlIssuer } from '../inbound'; import type { Inbound, Verified } from '../pipeline'; const resourceUrl = 'https://n8n.example/mcp-server/http'; @@ -135,6 +135,22 @@ describe('extractBearer', () => { }); }); +describe('isUrlIssuer', () => { + it.each(['https://idp.example', 'http://localhost:5678'])('routes %s as a URL issuer', (iss) => { + expect(isUrlIssuer(iss)).toBe(true); + }); + + it.each([ + ['the API-key issuer', 'n8n'], + ['the token-exchange issuer', 'n8n-token-exchange'], + ['a non-http URL', 'ftp://idp.example'], + ['a number', 42], + ['no issuer', undefined], + ])('does not route %s as a URL issuer', (_label, iss) => { + expect(isUrlIssuer(iss)).toBe(false); + }); +}); + describe('Verified', () => { it('has no field that can hold the token', () => { type HoldsCredential = 'credential' extends keyof Verified ? true : false; diff --git a/packages/@n8n/inbound-auth/src/contracts.ts b/packages/@n8n/inbound-auth/src/contracts.ts index 86a9ee3dd1e..c2434b99f98 100644 --- a/packages/@n8n/inbound-auth/src/contracts.ts +++ b/packages/@n8n/inbound-auth/src/contracts.ts @@ -128,6 +128,9 @@ export type Refused = { export type AdmissionResult = Admitted | Refused; +/** What a challenge needs. A `Refused` fits. Without a reason, the challenge has no error. */ +export type Refusal = Pick & { reason?: RefusalReason }; + /** * What every surface does between taking a credential off the wire and acting as the caller: * stamp the inbound with the resource, run the pipeline, ask the resource to authorize the @@ -150,8 +153,17 @@ export abstract class ResourceAdmission { token: string, ): Promise; - /** The `WWW-Authenticate` value for a refusal: the advertised challenge, then the RFC 6750 error. */ - abstract challenge(refused: Refused): Promise; + /** + * The `WWW-Authenticate` value for a refusal: the advertised challenge, then the RFC 6750 + * error. A refusal without a reason, or for a missing credential, gets no error (RFC 6750 §3.1). + */ + abstract challenge(refusal: Refusal): Promise; + + /** + * Refuses for a reason the surface decides itself, before or after the pipeline, and writes + * the one denial line. The line has no token, no external subject and no email. + */ + abstract refuse(inbound: Inbound, reason: RefusalReason, context?: SecurityContext): Refused; } /** The dispatcher. Owns what every method shares: driver lookup and surface acceptance. */ diff --git a/packages/@n8n/inbound-auth/src/inbound.ts b/packages/@n8n/inbound-auth/src/inbound.ts index 81240047762..fc0d16539c1 100644 --- a/packages/@n8n/inbound-auth/src/inbound.ts +++ b/packages/@n8n/inbound-auth/src/inbound.ts @@ -41,3 +41,11 @@ export function extractBearer(inbound: Inbound): Result { return { ok: true, value: { ...inbound, credential: { kind: 'bearer', token } } }; } + +/** + * Routing, not verification: a token from an OAuth2 trusted source names its issuer with an + * http(s) URL. The instance's own issuer is its base URL, which is http on a local instance. + */ +export function isUrlIssuer(iss: unknown): iss is string { + return typeof iss === 'string' && URL.canParse(iss) && /^https?:$/.test(new URL(iss).protocol); +} diff --git a/packages/cli/src/modules/inbound-auth-core/__tests__/resource-admission.test.ts b/packages/cli/src/modules/inbound-auth-core/__tests__/resource-admission.test.ts index 0437cd10fce..552c6b3a436 100644 --- a/packages/cli/src/modules/inbound-auth-core/__tests__/resource-admission.test.ts +++ b/packages/cli/src/modules/inbound-auth-core/__tests__/resource-admission.test.ts @@ -200,12 +200,61 @@ describe('InboundResourceAdmission', () => { authorizationServers: [], challenges: [], }); - const refused = await admission.admitBearer(resource, REQUEST, ''); + pipeline.run.mockResolvedValue({ ok: false, reason: 'source-not-accepted' }); + const refused = await admission.admitBearer(resource, REQUEST, 'the-token'); if (refused.ok) throw new Error('expected a refusal'); expect(await admission.challenge(refused)).toBe( - 'Bearer, error="invalid_token", error_description="no-credential"', + 'Bearer, error="invalid_token", error_description="source-not-accepted"', ); }); + + it('leaves out the error when the request had no credential (RFC 6750 §3.1)', async () => { + const refused = await admission.admitBearer(resource, REQUEST, ''); + if (refused.ok) throw new Error('expected a refusal'); + + expect(await admission.challenge(refused)).toBe(CHALLENGE); + }); + + it('leaves out the error for a refusal without a reason', async () => { + const challenge = await admission.challenge({ + advertised: { + surface: 'trigger', + resource: { url: RESOURCE_URL, acceptedAudiences: [RESOURCE_URL] }, + }, + }); + + expect(challenge).toBe(CHALLENGE); + }); + }); + + describe('refuse', () => { + it('writes one denial line with the context and returns the refusal', () => { + const inbound = { + surface: 'trigger' as const, + resource: { url: RESOURCE_URL, acceptedAudiences: [RESOURCE_URL] }, + request: { ...REQUEST, headers: { authorization: 'Bearer the-token' } }, + receivedAt: new Date(), + }; + + const refused = admission.refuse(inbound, 'unknown-subject', context); + + expect(refused).toEqual({ + ok: false, + reason: 'unknown-subject', + advertised: { surface: 'trigger', resource: inbound.resource }, + context, + }); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith('Inbound authentication denied', { + surface: 'trigger', + resource: RESOURCE_URL, + sourceId: 'idp-1', + authMethod: 'oauth-access-token', + userId: 'user-1', + reason: 'unknown-subject', + }); + expect(JSON.stringify(refused)).not.toContain('the-token'); + }); }); }); diff --git a/packages/cli/src/modules/inbound-auth-core/public-api-oauth.strategy.ts b/packages/cli/src/modules/inbound-auth-core/public-api-oauth.strategy.ts index 9bfe0cfb01e..9686ac1dc05 100644 --- a/packages/cli/src/modules/inbound-auth-core/public-api-oauth.strategy.ts +++ b/packages/cli/src/modules/inbound-auth-core/public-api-oauth.strategy.ts @@ -1,7 +1,13 @@ import type { AuthenticatedRequest, TokenGrant } from '@n8n/db'; import { UserRepository } from '@n8n/db'; import { Service } from '@n8n/di'; -import { extractBearer, inboundFrom, inboundRequestOf, ResourceAdmission } from '@n8n/inbound-auth'; +import { + extractBearer, + inboundFrom, + inboundRequestOf, + isUrlIssuer, + ResourceAdmission, +} from '@n8n/inbound-auth'; import { getApiKeyScopesForRole, isApiKeyScope, @@ -36,8 +42,7 @@ export class PublicApiOAuthStrategy implements AuthStrategy { } private isOAuthIssuer(token: string): boolean { - const iss: unknown = this.jwtService.decodeUnverified<{ iss?: unknown } | null>(token)?.iss; - return typeof iss === 'string' && URL.canParse(iss) && /^https?:$/.test(new URL(iss).protocol); + return isUrlIssuer(this.jwtService.decodeUnverified<{ iss?: unknown } | null>(token)?.iss); } async authenticate(req: AuthenticatedRequest): Promise { diff --git a/packages/cli/src/modules/inbound-auth-core/resource-admission.ts b/packages/cli/src/modules/inbound-auth-core/resource-admission.ts index 4a2dea461b6..57031337d22 100644 --- a/packages/cli/src/modules/inbound-auth-core/resource-admission.ts +++ b/packages/cli/src/modules/inbound-auth-core/resource-admission.ts @@ -10,6 +10,7 @@ import { type Extracted, type Inbound, type ProtectedResource, + type Refusal, type RefusalReason, type Refused, } from '@n8n/inbound-auth'; @@ -51,17 +52,21 @@ export class InboundResourceAdmission extends ResourceAdmission { return { ok: true, context }; } - async challenge({ advertised, reason }: Refused): Promise { + async challenge({ advertised, reason }: Refusal): Promise { const { challenges } = await this.authenticationService.advertise(advertised); + const challenge = challenges[0] ?? 'Bearer'; + // RFC 6750 §3.1: a request that lacks any authentication information + // SHOULD NOT get an error code. + if (reason === undefined || reason === 'no-credential') return challenge; const error = reason === 'insufficient_scope' ? 'insufficient_scope' : 'invalid_token'; - return `${challenges[0] ?? 'Bearer'}, error="${error}", error_description="${reason}"`; + return `${challenge}, error="${error}", error_description="${reason}"`; } /** * The one log line for a refusal the surface decides itself, before or after the pipeline. * Carries no token, no external subject and no email. */ - private refuse(inbound: Inbound, reason: RefusalReason, context?: SecurityContext): Refused { + refuse(inbound: Inbound, reason: RefusalReason, context?: SecurityContext): Refused { this.logger.warn('Inbound authentication denied', { surface: inbound.surface, resource: inbound.resource.url, diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.integration.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.integration.test.ts index fd18aeb2628..3d4d2f16af7 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.integration.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.integration.test.ts @@ -66,9 +66,10 @@ describe('McpServerApiKeyService.verifyApiKey (integration)', () => { const result = await service.verifyApiKey(apiKey); - expect(result.user?.id).toBe(owner.id); - expect(result.actor).toBeUndefined(); - expect(result.context).toBeUndefined(); + expect(result?.user?.id).toBe(owner.id); + expect(result?.actor).toBeUndefined(); + expect(result?.context).toBeUndefined(); + expect(result?.authMethod).toBe('api-key'); }); it('accepts a scoped JWT and returns the subject when no actor is present', async () => { @@ -76,9 +77,10 @@ describe('McpServerApiKeyService.verifyApiKey (integration)', () => { const result = await service.verifyApiKey(makeScopedJwt(jwtService, subject.id)); - expect(result.user?.id).toBe(subject.id); - expect(result.actor).toBeUndefined(); - expect(result.context).toBeUndefined(); + expect(result?.user?.id).toBe(subject.id); + expect(result?.actor).toBeUndefined(); + expect(result?.context).toBeUndefined(); + expect(result?.authMethod).toBe('scoped-jwt'); }); it('accepts a scoped JWT with delegation and returns the actor as user', async () => { @@ -89,8 +91,18 @@ describe('McpServerApiKeyService.verifyApiKey (integration)', () => { makeScopedJwt(jwtService, subject.id, { actSub: actor.id }), ); - expect(result.user?.id).toBe(actor.id); - expect(result.actor?.id).toBe(actor.id); + expect(result?.user?.id).toBe(actor.id); + expect(result?.actor?.id).toBe(actor.id); + }); + + it('abstains on a valid browser session JWT, so the caller can send it down another path', async () => { + // SessionCookieAuthStrategy is registered into the same registry above, so this proves + // its buildTokenGrant genuinely abstains: the mcp-server-api audience is never satisfied. + const owner = await createOwner(); + + const result = await service.verifyApiKey(Container.get(AuthService).issueJWT(owner, false)); + + expect(result).toBeNull(); }); describe('rejects tokens that fail validation', () => { @@ -143,21 +155,11 @@ describe('McpServerApiKeyService.verifyApiKey (integration)', () => { name: 'scoped JWT whose subject is not in the database', token: () => makeScopedJwt(jwtService, '00000000-0000-0000-0000-000000000000'), }, - { - // SessionCookieAuthStrategy is registered into the same registry above, - // so this proves its buildTokenGrant genuinely abstains rather than the - // audience check merely never encountering the strategy. - name: 'a valid browser session cookie (must not satisfy the mcp-server-api audience)', - token: async () => { - const owner = await createOwner(); - return Container.get(AuthService).issueJWT(owner, false); - }, - }, ])('rejects $name', async ({ token }) => { const result = await service.verifyApiKey(await token()); - expect(result.user).toBeNull(); - expect(result.context?.reason).toBe('invalid_token'); + expect(result?.user).toBeNull(); + expect(result?.context?.reason).toBe('invalid_token'); }); }); @@ -183,7 +185,7 @@ describe('McpServerApiKeyService.verifyApiKey (integration)', () => { const result = await service.verifyApiKey(jwt); - expect(result.user).toBeNull(); - expect(result.context?.reason).toBe('invalid_token'); + expect(result?.user).toBeNull(); + expect(result?.context?.reason).toBe('invalid_token'); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.public-api-disabled.integration.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.public-api-disabled.integration.test.ts index e12679242ae..05b1001810c 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.public-api-disabled.integration.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.public-api-disabled.integration.test.ts @@ -23,7 +23,7 @@ describe('McpServerApiKeyService.verifyApiKey with public API disabled', () => { const result = await service.verifyApiKey(apiKey); - expect(result.user?.id).toBe(owner.id); - expect(result.context).toBeUndefined(); + expect(result?.user?.id).toBe(owner.id); + expect(result?.context).toBeUndefined(); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.test.ts index e42d0ec68d4..dd7a2c532e5 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-api-key.service.test.ts @@ -35,8 +35,8 @@ describe('McpServerApiKeyService', () => { expect(authStrategyRegistry.buildContextFromToken).toHaveBeenCalledWith('any-token', { audience: 'mcp-server-api', }); - expect(result.user).toBe(subject); - expect(result.actor).toBeUndefined(); + expect(result?.user).toBe(subject); + expect(result?.actor).toBeUndefined(); }); it('returns the actor as user when delegation is present', async () => { @@ -50,8 +50,8 @@ describe('McpServerApiKeyService', () => { const result = await service.verifyApiKey('token'); - expect(result.user).toBe(actor); - expect(result.actor).toBe(actor); + expect(result?.user).toBe(actor); + expect(result?.actor).toBe(actor); }); it('reports `api_key` for a delegated token-exchange grant, not a distinct type', async () => { @@ -68,16 +68,38 @@ describe('McpServerApiKeyService', () => { const result = await service.verifyApiKey('scoped-jwt'); - expect(result.caller).toEqual({ authType: 'api_key' }); + expect(result?.caller).toEqual({ authType: 'api_key' }); }); - it('returns a rejection context when the registry returns null', async () => { + it('reports which strategy admitted the caller, for the log line', async () => { + const subject = makeUser('subject-1'); + authStrategyRegistry.buildContextFromToken.mockResolvedValueOnce({ + subject, + scopes: [], + apiKeyId: 'key-1', + } satisfies TokenGrant); + authStrategyRegistry.buildContextFromToken.mockResolvedValueOnce({ + subject, + scopes: [], + } satisfies TokenGrant); + + expect((await service.verifyApiKey('api-key'))?.authMethod).toBe('api-key'); + expect((await service.verifyApiKey('scoped-jwt'))?.authMethod).toBe('scoped-jwt'); + }); + + it('abstains when no strategy knows the token', async () => { authStrategyRegistry.buildContextFromToken.mockResolvedValue(null); + expect(await service.verifyApiKey('unknown-token')).toBeNull(); + }); + + it('returns a rejection context when a strategy refuses the token', async () => { + authStrategyRegistry.buildContextFromToken.mockResolvedValue(false); + const result = await service.verifyApiKey('bad-token'); - expect(result.user).toBeNull(); - expect(result.context).toEqual({ reason: 'invalid_token', auth_type: 'api_key' }); + expect(result?.user).toBeNull(); + expect(result?.context).toEqual({ reason: 'invalid_token', auth_type: 'api_key' }); }); it.each([ @@ -89,8 +111,8 @@ describe('McpServerApiKeyService', () => { const result = await service.verifyApiKey('token'); - expect(result.user).toBeNull(); - expect(result.context).toEqual({ + expect(result?.user).toBeNull(); + expect(result?.context).toEqual({ reason, auth_type: 'api_key', error_details: message, diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-server-middleware.service.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-server-middleware.service.test.ts index 729c277bb7e..8546bde7f92 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-server-middleware.service.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-server-middleware.service.test.ts @@ -1,157 +1,108 @@ import type { Mocked } from 'vitest'; +import type { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; -import type { User } from '@n8n/db'; -import type { Request, Response, NextFunction } from 'express'; +import type { Transaction, User, UserRepository } from '@n8n/db'; +import type { Inbound, RefusalReason, Refused, ResourceAdmission } from '@n8n/inbound-auth'; +import type { Principal, SecurityContext } from '@n8n/permissions'; +import type { Response, NextFunction } from 'express'; import jwt from 'jsonwebtoken'; -import { generateKeyPairSync } from 'node:crypto'; import { mock, mockDeep } from 'vitest-mock-extended'; import type { InstanceSettings } from 'n8n-core'; import { JwtService } from '@n8n/backend-services'; -import { type McpCallerAuth, OAuthTokenVerifierProxy } from '@n8n/backend-services/auth'; import { Telemetry } from '@/telemetry'; import { McpServerApiKeyService } from '../mcp-api-key.service'; import { MCP_CLIENT_INFO_META_KEY, MCP_PROTOCOL_VERSION_META_KEY } from '../mcp.constants'; import { McpProtectedResource } from '../mcp-protected-resource'; import { McpServerMiddlewareService } from '../mcp-server-middleware.service'; +import type { McpAuthenticatedRequest } from '../mcp.types'; -const mockReqWith = (authHeader: string | undefined, body?: any) => { - const req = mockDeep(); +const RESOURCE_URL = 'https://n8n.example.com/mcp-server/http'; +const CHALLENGE = + 'Bearer resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"'; + +const mockReqWith = (authHeader: string | undefined, body?: unknown) => { + const req = mockDeep({ + // `extractBearer` reads the headers object, not `req.header()`. + headers: authHeader === undefined ? {} : { authorization: authHeader }, + method: 'POST', + originalUrl: '/mcp-server/http', + ip: '203.0.113.7', + }); req.header.mockImplementation((name: string) => { if (name === 'authorization') return authHeader; return undefined; }); - req.body = body || {}; + req.body = body ?? {}; return req; }; +const mockRes = () => { + const res = mockDeep(); + res.status.mockReturnThis(); + res.send.mockReturnThis(); + res.header.mockReturnThis(); + return res; +}; + const instanceSettings = mock({ encryptionKey: 'test-key' }); const jwtService = new JwtService(instanceSettings, mock(), mock()); -const signingKey = generateKeyPairSync('ec', { namedCurve: 'P-256' }).privateKey; -/** An access token in the shape n8n mints. The verifier is mocked, so only its claims matter. */ -const signOAuthToken = () => - jwt.sign({ sub: 'user-123', meta: { isOAuth: true } }, signingKey, { - algorithm: 'ES256', - audience: 'https://n8n.example.com/mcp-server/http', - header: { alg: 'ES256', typ: 'at+jwt', kid: 'signing-kid' }, - }); +/** A token from an OAuth2 trusted source. The admission is mocked, so only `iss` matters. */ +const signIssuerToken = (iss: string) => jwt.sign({ iss, sub: 'idp-alice' }, 'any-secret'); + +const context: SecurityContext = { + subject: { id: 'user-123', type: 'human', email: 'alice@example.com' } as Principal, + authMethod: 'oauth-access-token', + subjectClaim: { sourceId: 'idp-1', issuer: 'https://idp.example', subject: 'idp-alice' }, + clientId: 'client-abc', + tokenScopes: ['workflow:read'], +}; let mcpServerApiKeyService: Mocked; -let oauthTokenVerifier: Mocked; let mcpProtectedResource: Mocked; +let admission: Mocked; +let userRepository: Mocked; let telemetry: Mocked; +let logger: Mocked; let service: McpServerMiddlewareService; describe('McpServerMiddlewareService', () => { beforeAll(() => { mcpServerApiKeyService = mockInstance(McpServerApiKeyService) as Mocked; - oauthTokenVerifier = mockInstance(OAuthTokenVerifierProxy) as Mocked; - mcpProtectedResource = mockInstance(McpProtectedResource) as Mocked; + mcpProtectedResource = mockInstance(McpProtectedResource, { + surface: 'instance-mcp', + }) as Mocked; + admission = mock(); + userRepository = mock(); telemetry = mockInstance(Telemetry); + logger = mock(); service = new McpServerMiddlewareService( mcpServerApiKeyService, - oauthTokenVerifier, mcpProtectedResource, + admission, + userRepository, jwtService, telemetry, + logger, ); }); beforeEach(() => { vi.clearAllMocks(); - mcpProtectedResource.getResourceUrl.mockReturnValue('https://n8n.example.com/mcp-server/http'); - mcpProtectedResource.getProtectedResourceMetadataUrl.mockReturnValue( - 'https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http', + mcpProtectedResource.getResourceUrl.mockReturnValue(RESOURCE_URL); + mcpProtectedResource.getAudiences.mockReturnValue([RESOURCE_URL]); + admission.refuse.mockImplementation( + (inbound: Inbound, reason: RefusalReason, ctx?: SecurityContext): Refused => ({ + ok: false, + reason, + advertised: { surface: inbound.surface, resource: inbound.resource }, + ...(ctx && { context: ctx }), + }), ); - }); - - describe('getUserForToken', () => { - it('should return user for valid OAuth token (meta.isOAuth = true)', async () => { - const user = mock({ id: 'user-123' }); - const oauthToken = signOAuthToken(); - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ - user, - caller: { authType: 'oauth', clientId: 'client-abc' }, - }); - - const result = await service.getUserForToken(oauthToken); - - expect(result).toEqual({ user, caller: { authType: 'oauth', clientId: 'client-abc' } }); - expect(oauthTokenVerifier.verifyOAuthAccessToken).toHaveBeenCalledWith( - oauthToken, - 'https://n8n.example.com/mcp-server/http', - ); - expect(mcpServerApiKeyService.verifyApiKey).not.toHaveBeenCalled(); - }); - - it('should return user for valid API key (no meta.isOAuth)', async () => { - const user = mock({ id: 'user-123' }); - const apiKeyToken = jwtService.sign('mcpApiKey', { sub: 'user-123' }); - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ - user, - caller: { authType: 'api_key' }, - }); - - const result = await service.getUserForToken(apiKeyToken); - - expect(result).toEqual({ user, caller: { authType: 'api_key' } }); - expect(mcpServerApiKeyService.verifyApiKey).toHaveBeenCalledWith(apiKeyToken); - expect(oauthTokenVerifier.verifyOAuthAccessToken).not.toHaveBeenCalled(); - }); - - it('should return user for valid API key (meta.isOAuth = false)', async () => { - const user = mock({ id: 'user-123' }); - const apiKeyToken = jwtService.sign('mcpApiKey', { - sub: 'user-123', - meta: { isOAuth: false }, - }); - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ - user, - caller: { authType: 'api_key' }, - }); - - const result = await service.getUserForToken(apiKeyToken); - - expect(result).toEqual({ user, caller: { authType: 'api_key' } }); - expect(mcpServerApiKeyService.verifyApiKey).toHaveBeenCalledWith(apiKeyToken); - expect(oauthTokenVerifier.verifyOAuthAccessToken).not.toHaveBeenCalled(); - }); - - it('should return null for invalid JWT format', async () => { - const invalidToken = 'not-a-jwt-token'; - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ user: null }); - const result = await service.getUserForToken(invalidToken); - - expect(result).toMatchObject({ user: null }); - expect(oauthTokenVerifier.verifyOAuthAccessToken).not.toHaveBeenCalled(); - }); - - it('should return null when OAuth token verification fails', async () => { - const oauthToken = signOAuthToken(); - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ user: null }); - - const result = await service.getUserForToken(oauthToken); - - expect(result).toMatchObject({ user: null }); - }); - - it('should return null when API key verification fails', async () => { - const apiKeyToken = jwtService.sign('mcpApiKey', { sub: 'user-123' }); - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ user: null }); - - const result = await service.getUserForToken(apiKeyToken); - - expect(result).toMatchObject({ user: null }); - }); + admission.challenge.mockResolvedValue(CHALLENGE); }); describe('getEnabledMiddleware', () => { @@ -208,279 +159,352 @@ describe('McpServerMiddlewareService', () => { }); describe('getAuthMiddleware', () => { - it('should return 401 with WWW-Authenticate header when authorization header is missing', async () => { - const req = mockReqWith(undefined); - const res = mockDeep(); - res.status.mockReturnThis(); - res.send.mockReturnThis(); - res.header.mockReturnThis(); + const run = async (authHeader: string | undefined, body?: unknown) => { + const req = mockReqWith(authHeader, body); + const res = mockRes(); const next = vi.fn() as NextFunction; - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ - message: 'Unauthorized: Authorization header not sent', - }); - expect(next).not.toHaveBeenCalled(); - expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { - mcp_connection_status: 'error', - http_status: 401, - error: 'Unauthorized', - client_name: undefined, - client_version: undefined, - auth_type: 'unknown', - error_details: 'Authorization header not sent', - reason: 'missing_authorization_header', - }); - }); - - it('should return 401 with WWW-Authenticate header when authorization header does not start with Bearer', async () => { - const req = mockReqWith('Basic sometoken'); - const res = mockDeep(); - res.status.mockReturnThis(); - const next = vi.fn() as NextFunction; - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ - message: 'Unauthorized: Invalid authorization header format - Missing Bearer prefix', - }); - expect(next).not.toHaveBeenCalled(); - expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { - mcp_connection_status: 'error', - http_status: 401, - error: 'Unauthorized', - client_name: undefined, - client_version: undefined, - auth_type: 'unknown', - error_details: 'Invalid authorization header format - Missing Bearer prefix', - reason: 'invalid_bearer_format', - }); - }); - - it('should return 401 with WWW-Authenticate header when Bearer token is malformed', async () => { - const req = mockReqWith('Bearer'); - const res = mockDeep(); - res.status.mockReturnThis(); - const next = vi.fn() as NextFunction; - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ - message: 'Unauthorized: Invalid authorization header format - Malformed Bearer token', - }); - expect(next).not.toHaveBeenCalled(); - expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { - mcp_connection_status: 'error', - http_status: 401, - error: 'Unauthorized', - client_name: undefined, - client_version: undefined, - auth_type: 'unknown', - error_details: 'Invalid authorization header format - Malformed Bearer token', - reason: 'invalid_bearer_format', - }); - }); - - it('should authenticate with valid OAuth token and call next', async () => { - const user = mock({ id: 'user-123' }); - const oauthToken = signOAuthToken(); - - const req = mockReqWith(`Bearer ${oauthToken}`); - const res = mockDeep(); - const next = vi.fn() as NextFunction; - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ - user, - caller: { authType: 'oauth', clientId: 'client-abc' }, - }); - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - const authenticatedReq = req as Request & { user?: User; mcpCaller?: McpCallerAuth }; - - expect(authenticatedReq.user).toEqual(user); - expect(authenticatedReq.mcpCaller?.authType).toBe('oauth'); - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should attach the OAuth token scopes to the request', async () => { - const user = mock({ id: 'user-123' }); - const oauthToken = signOAuthToken(); - - const req = mockReqWith(`Bearer ${oauthToken}`); - const res = mockDeep(); - const next = vi.fn() as NextFunction; - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ - user, - caller: { authType: 'oauth', clientId: 'client-abc' }, - scopes: ['workflow:read'], - }); - await service.getAuthMiddleware()(req, res, next); - const authenticatedReq = req as Request & { mcpScopes?: string[] }; + return { req, res, next }; + }; - expect(authenticatedReq.mcpScopes).toEqual(['workflow:read']); - expect(next).toHaveBeenCalled(); - }); - - it('should attach the OAuth client the token was issued to', async () => { - const user = mock({ id: 'user-123' }); - const oauthToken = signOAuthToken(); - - const req = mockReqWith(`Bearer ${oauthToken}`); - const res = mockDeep(); - const next = vi.fn() as NextFunction; - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ - user, - caller: { authType: 'oauth', clientId: 'client-abc' }, - }); - - await service.getAuthMiddleware()(req, res, next); - - expect((req as Request & { mcpCaller?: McpCallerAuth }).mcpCaller).toEqual({ - authType: 'oauth', - clientId: 'client-abc', - }); - expect(next).toHaveBeenCalled(); - }); - - it('should authenticate with valid API key and call next', async () => { - const user = mock({ id: 'user-123' }); - const apiKeyToken = jwtService.sign('mcpApiKey', { sub: 'user-123' }); - - const req = mockReqWith(`Bearer ${apiKeyToken}`); - const res = mockDeep(); - const next = vi.fn() as NextFunction; - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ - user, - caller: { authType: 'api_key' }, - }); - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - const authenticatedReq = req as Request & { - user?: User; - mcpCaller?: McpCallerAuth; - mcpScopes?: string[]; - }; - - expect(authenticatedReq.user).toEqual(user); - // An API key is not issued to an OAuth client, so there is none to report - expect(authenticatedReq.mcpCaller).toEqual({ authType: 'api_key' }); - // API keys are not scope-bearing: undefined means full tool access - expect(authenticatedReq.mcpScopes).toBeUndefined(); - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should authenticate with a delegated scoped JWT and set req.user to the actor', async () => { - const actor = mock({ id: 'actor-1' }); - const scopedJwt = jwtService.sign('tokenExchange', { - iss: 'n8n-token-exchange', - sub: 'subject-1', - act: { sub: 'actor-1' }, - jti: 'test-jti', - }); - - const req = mockReqWith(`Bearer ${scopedJwt}`); - const res = mockDeep(); - const next = vi.fn() as NextFunction; - - // verifyApiKey returns the acting principal as `user` and keeps the actor field populated. - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ user: actor, actor }); - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect((req as any).user).toEqual(actor); - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - // Scoped JWTs flow through verifyApiKey (no meta.isOAuth), not the OAuth path. - expect(mcpServerApiKeyService.verifyApiKey).toHaveBeenCalledWith(scopedJwt); - expect(oauthTokenVerifier.verifyOAuthAccessToken).not.toHaveBeenCalled(); - }); - - it('should return 401 with WWW-Authenticate header when token validation fails', async () => { - const invalidToken = signOAuthToken(); - - const req = mockReqWith(`Bearer ${invalidToken}`); - const res = mockDeep(); - res.status.mockReturnThis(); - res.send.mockReturnThis(); - res.header.mockReturnThis(); - const next = vi.fn() as NextFunction; - - oauthTokenVerifier.verifyOAuthAccessToken.mockResolvedValue({ user: null }); - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); + const expectUnauthorized = (res: Mocked, next: NextFunction, message: string) => { + expect(res.header).toHaveBeenCalledWith('WWW-Authenticate', CHALLENGE); expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ message: 'Unauthorized' }); + expect(res.send).toHaveBeenCalledWith({ message }); expect(next).not.toHaveBeenCalled(); + }; + + describe('a header without a bearer', () => { + it('refuses a missing header with no-credential and keeps the telemetry', async () => { + const { res, next } = await run(undefined); + + expect(admission.refuse).toHaveBeenCalledWith( + expect.objectContaining({ surface: 'instance-mcp' }), + 'no-credential', + ); + expect(admission.challenge).toHaveBeenCalledWith( + expect.objectContaining({ reason: 'no-credential' }), + ); + expectUnauthorized(res, next, 'Unauthorized: Authorization header not sent'); + expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { + mcp_connection_status: 'error', + http_status: 401, + error: 'Unauthorized', + client_name: undefined, + client_version: undefined, + auth_type: 'unknown', + error_details: 'Authorization header not sent', + reason: 'missing_authorization_header', + }); + expect(admission.admit).not.toHaveBeenCalled(); + // The shared admission writes the denial line. + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it.each([ + ['Basic sometoken', 'Missing Bearer prefix'], + ['Bearer', 'Malformed Bearer token'], + ['Bearer two parts', 'Malformed Bearer token'], + ])('refuses %j with malformed-credential and today’s message', async (header, problem) => { + const { res, next } = await run(header); + + expect(admission.refuse).toHaveBeenCalledWith(expect.anything(), 'malformed-credential'); + expectUnauthorized( + res, + next, + `Unauthorized: Invalid authorization header format - ${problem}`, + ); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ + reason: 'invalid_bearer_format', + auth_type: 'unknown', + error_details: `Invalid authorization header format - ${problem}`, + }), + ); + }); + }); + + describe('a bearer from a URL issuer', () => { + beforeEach(() => { + admission.admit.mockResolvedValue({ ok: true, context }); + userRepository.findByIdWithRole.mockResolvedValue(mock({ id: 'user-123' })); + }); + + it.each(['https://idp.example', 'http://localhost:5678'])( + 'sends a token from %s to the pipeline, not to the API-key path', + async (iss) => { + const token = signIssuerToken(iss); + + await run(`Bearer ${token}`); + + expect(admission.admit).toHaveBeenCalledWith( + mcpProtectedResource, + expect.objectContaining({ + surface: 'instance-mcp', + credential: { kind: 'bearer', token }, + }), + ); + expect(mcpServerApiKeyService.verifyApiKey).not.toHaveBeenCalled(); + }, + ); + + it('sets the user, the OAuth caller, the token scopes and the security context', async () => { + const user = mock({ id: 'user-123' }); + userRepository.findByIdWithRole.mockResolvedValue(user); + + const { req, res, next } = await run(`Bearer ${signIssuerToken('https://idp.example')}`); + + expect(userRepository.findByIdWithRole).toHaveBeenCalledWith('user-123'); + expect(req.user).toBe(user); + expect(req.mcpCaller).toEqual({ authType: 'oauth', clientId: 'client-abc' }); + expect(req.mcpScopes).toEqual(['workflow:read']); + expect(req.ctx?.security).toBe(context); + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + // The pipeline writes the accept line. + expect(logger.info).not.toHaveBeenCalled(); + }); + + it('keeps what earlier middleware put on the operation context', async () => { + const req = mockReqWith(`Bearer ${signIssuerToken('https://idp.example')}`); + const earlier = { trx: mock() }; + req.ctx = earlier; + + await service.getAuthMiddleware()(req, mockRes(), vi.fn()); + + expect(req.ctx).toEqual({ ...earlier, security: context }); + }); + + it('treats a token without scopes as scope-bearing with no scopes, not full access', async () => { + admission.admit.mockResolvedValue({ + ok: true, + context: { ...context, tokenScopes: undefined }, + }); + + const { req } = await run(`Bearer ${signIssuerToken('https://idp.example')}`); + + expect(req.mcpScopes).toEqual([]); + }); + + it('refuses an admitted token without a client id', async () => { + const withoutClient = { ...context, clientId: undefined }; + admission.admit.mockResolvedValue({ ok: true, context: withoutClient }); + + const { res, next } = await run(`Bearer ${signIssuerToken('https://idp.example')}`); + + expect(admission.refuse).toHaveBeenCalledWith( + expect.anything(), + 'not-an-access-token', + withoutClient, + ); + expectUnauthorized(res, next, 'Unauthorized: not-an-access-token'); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ + reason: 'invalid_token', + auth_type: 'oauth', + error_details: 'not-an-access-token', + }), + ); + }); + + it('refuses an admitted token whose user row is gone', async () => { + userRepository.findByIdWithRole.mockResolvedValue(null); + + const { res, next } = await run(`Bearer ${signIssuerToken('https://idp.example')}`); + + expect(admission.refuse).toHaveBeenCalledWith( + expect.anything(), + 'unknown-subject', + context, + ); + expectUnauthorized(res, next, 'Unauthorized: unknown-subject'); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ reason: 'user_not_found', auth_type: 'oauth' }), + ); + }); + + it.each([ + ['source-not-accepted', 'invalid_token'], + ['signature-invalid', 'invalid_token'], + ['revoked', 'token_not_found_in_db'], + ] as const)( + 'answers a %s refusal with the admission challenge and the reason', + async (reason, telemetryReason) => { + const refused: Refused = { + ok: false, + reason, + advertised: { + surface: 'instance-mcp', + resource: { url: RESOURCE_URL, acceptedAudiences: [RESOURCE_URL] }, + }, + }; + admission.admit.mockResolvedValue(refused); + + const { res, next } = await run(`Bearer ${signIssuerToken('https://idp.example')}`); + + expect(admission.challenge).toHaveBeenCalledWith(refused); + expectUnauthorized(res, next, `Unauthorized: ${reason}`); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ + reason: telemetryReason, + auth_type: 'oauth', + error_details: reason, + }), + ); + // The pipeline writes the reject line. + expect(logger.warn).not.toHaveBeenCalled(); + }, + ); + }); + + describe('a bearer for the API-key path', () => { + it('authenticates an MCP API key without the pipeline and logs the accept', async () => { + const user = mock({ id: 'user-123' }); + const apiKey = jwtService.sign('mcpApiKey', { sub: 'user-123', iss: 'n8n' }); + mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ + user, + caller: { authType: 'api_key' }, + authMethod: 'api-key', + }); + + const { req, res, next } = await run(`Bearer ${apiKey}`); + + expect(mcpServerApiKeyService.verifyApiKey).toHaveBeenCalledWith(apiKey); + expect(admission.admit).not.toHaveBeenCalled(); + expect(req.user).toBe(user); + // An API key is not issued to an OAuth client, so there is none to report + expect(req.mcpCaller).toEqual({ authType: 'api_key' }); + // API keys are not scope-bearing: undefined means full tool access + expect(req.mcpScopes).toBeUndefined(); + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledTimes(1); + expect(logger.info).toHaveBeenCalledWith('Inbound authentication accepted', { + surface: 'instance-mcp', + resource: RESOURCE_URL, + authMethod: 'api-key', + userId: 'user-123', + }); + }); + + it('authenticates a delegated token-exchange JWT and sets req.user to the actor', async () => { + const actor = mock({ id: 'actor-1' }); + const scopedJwt = jwtService.sign('tokenExchange', { + iss: 'n8n-token-exchange', + sub: 'subject-1', + act: { sub: 'actor-1' }, + jti: 'test-jti', + }); + mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ + user: actor, + actor, + caller: { authType: 'api_key' }, + authMethod: 'scoped-jwt', + }); + + const { req, next } = await run(`Bearer ${scopedJwt}`); + + expect(req.user).toBe(actor); + expect(next).toHaveBeenCalled(); + expect(admission.admit).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith( + 'Inbound authentication accepted', + expect.objectContaining({ authMethod: 'scoped-jwt', userId: 'actor-1' }), + ); + }); + + it('answers a rejected API key with the challenge without an error, as before', async () => { + mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ + user: null, + context: { reason: 'invalid_token', auth_type: 'api_key' }, + }); + + const { res, next } = await run('Bearer invalid-token-format'); + + expect(admission.challenge).toHaveBeenCalledWith({ + advertised: { + surface: 'instance-mcp', + resource: { url: RESOURCE_URL, acceptedAudiences: [RESOURCE_URL] }, + }, + }); + expectUnauthorized(res, next, 'Unauthorized'); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ reason: 'invalid_token', auth_type: 'api_key' }), + ); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith('Inbound authentication denied', { + surface: 'instance-mcp', + resource: RESOURCE_URL, + reason: 'invalid_token', + }); + expect(admission.admit).not.toHaveBeenCalled(); + }); + + it('sends a token that no API-key strategy knows to the pipeline', async () => { + mcpServerApiKeyService.verifyApiKey.mockResolvedValue(null); + admission.admit.mockResolvedValue({ + ok: false, + reason: 'unknown-issuer', + advertised: { + surface: 'instance-mcp', + resource: { url: RESOURCE_URL, acceptedAudiences: [RESOURCE_URL] }, + }, + }); + + const { res, next } = await run('Bearer some-session-jwt'); + + expect(admission.admit).toHaveBeenCalled(); + expectUnauthorized(res, next, 'Unauthorized: unknown-issuer'); + expect(telemetry.track).toHaveBeenCalledWith( + 'User connected to MCP server', + expect.objectContaining({ + reason: 'invalid_token', + auth_type: 'unknown', + error_details: 'unknown-issuer', + }), + ); + }); + + it('routes a JWT whose payload is not JSON to the API-key path instead of throwing', async () => { + // `jwt.decode` throws for a token typed `JWT` whose payload does not parse. + const header = Buffer.from(JSON.stringify({ alg: 'HS256', typ: 'JWT' })).toString( + 'base64url', + ); + const token = `${header}.${Buffer.from('not-json').toString('base64url')}.sig`; + mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ user: null }); + + const { res } = await run(`Bearer ${token}`); + + expect(mcpServerApiKeyService.verifyApiKey).toHaveBeenCalledWith(token); + expect(res.status).toHaveBeenCalledWith(401); + }); + + it('keeps the token and the email out of the log lines', async () => { + const apiKey = jwtService.sign('mcpApiKey', { sub: 'user-123', iss: 'n8n' }); + mcpServerApiKeyService.verifyApiKey.mockResolvedValueOnce({ + user: mock({ id: 'user-123', email: 'alice@example.com' }), + caller: { authType: 'api_key' }, + authMethod: 'api-key', + }); + mcpServerApiKeyService.verifyApiKey.mockResolvedValueOnce({ user: null }); + + await run(`Bearer ${apiKey}`); + await run(`Bearer ${apiKey}`); + + const logged = JSON.stringify([logger.info.mock.calls, logger.warn.mock.calls]); + expect(logged).not.toContain(apiKey); + expect(logged).not.toContain('alice@example.com'); + }); }); it('should track telemetry with client info from request body', async () => { - const req = mockReqWith(undefined, { - params: { - clientInfo: { - name: 'test-client', - version: '1.0.0', - }, - }, - }); - const res = mockDeep(); - res.status.mockReturnThis(); - res.send.mockReturnThis(); - res.header.mockReturnThis(); - const next = vi.fn() as NextFunction; + await run(undefined, { params: { clientInfo: { name: 'test-client', version: '1.0.0' } } }); - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ - message: 'Unauthorized: Authorization header not sent', - }); expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { mcp_connection_status: 'error', http_status: 401, @@ -498,7 +522,7 @@ describe('McpServerMiddlewareService', () => { // points the client at the OAuth protected-resource metadata. The client // identity and protocol version it declared in `_meta` are still captured. it('should reject an unauthenticated server/discover probe with 401 and capture _meta', async () => { - const req = mockReqWith(undefined, { + const { res, next } = await run(undefined, { jsonrpc: '2.0', method: 'server/discover', params: { @@ -508,20 +532,8 @@ describe('McpServerMiddlewareService', () => { }, }, }); - const res = mockDeep(); - res.status.mockReturnThis(); - res.send.mockReturnThis(); - res.header.mockReturnThis(); - const next = vi.fn() as NextFunction; - await service.getAuthMiddleware()(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(next).not.toHaveBeenCalled(); + expectUnauthorized(res, next, 'Unauthorized: Authorization header not sent'); expect(telemetry.track).toHaveBeenCalledWith('User connected to MCP server', { mcp_connection_status: 'error', http_status: 401, @@ -534,28 +546,5 @@ describe('McpServerMiddlewareService', () => { reason: 'missing_authorization_header', }); }); - - it('should handle invalid token format gracefully with WWW-Authenticate header', async () => { - const req = mockReqWith('Bearer invalid-token-format'); - const res = mockDeep(); - res.status.mockReturnThis(); - res.send.mockReturnThis(); - res.header.mockReturnThis(); - const next = vi.fn() as NextFunction; - - mcpServerApiKeyService.verifyApiKey.mockResolvedValue({ user: null }); - - const middleware = service.getAuthMiddleware(); - - await middleware(req, res, next); - - expect(res.header).toHaveBeenCalledWith( - 'WWW-Authenticate', - 'Bearer realm="n8n MCP Server", resource_metadata="https://n8n.example.com/.well-known/oauth-protected-resource/mcp-server/http"', - ); - expect(res.status).toHaveBeenCalledWith(401); - expect(res.send).toHaveBeenCalledWith({ message: 'Unauthorized' }); - expect(next).not.toHaveBeenCalled(); - }); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp.controller.api.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp.controller.api.test.ts index 2c3fb44de0e..ac0f6d59718 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp.controller.api.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp.controller.api.test.ts @@ -4,7 +4,11 @@ import { setupTestServer } from '@test-integration/utils'; import { McpSettingsService } from '../mcp.settings.service'; -const testServer = setupTestServer({ modules: ['oauth-server', 'mcp'], endpointGroups: ['mcp'] }); +const testServer = setupTestServer({ + // Production order: the MCP middleware and metadata need the inbound-auth contracts. + modules: ['inbound-auth-core', 'oauth-server', 'mcp'], + endpointGroups: ['mcp'], +}); let mcpSettingsService: McpSettingsService; diff --git a/packages/cli/src/modules/mcp/mcp-api-key.service.ts b/packages/cli/src/modules/mcp/mcp-api-key.service.ts index 584ee91d02f..6b152e2fca1 100644 --- a/packages/cli/src/modules/mcp/mcp-api-key.service.ts +++ b/packages/cli/src/modules/mcp/mcp-api-key.service.ts @@ -1,5 +1,6 @@ import { ApiKey, ApiKeyRepository, User } from '@n8n/db'; import { Service } from '@n8n/di'; +import type { AuthMethod } from '@n8n/permissions'; import { EntityManager } from '@n8n/typeorm'; import { randomUUID } from 'crypto'; import { ensureError } from '@n8n/utils/errors/ensure-error'; @@ -15,6 +16,11 @@ const REDACT_API_KEY_MAX_LENGTH = 10; const API_KEY_LABEL = 'MCP Server API Key'; const REDACT_API_KEY_MIN_HIDDEN_CHARS = 6; +export type McpApiKeyResult = UserWithContext & { + /** Set on success, for the authentication log line. */ + authMethod?: Extract; +}; + /** * Service for managing MCP server API keys, including creation, retrieval, deletion, and authentication middleware. */ @@ -63,29 +69,34 @@ export class McpServerApiKeyService { return apiKey; } - async verifyApiKey(apiKey: string): Promise { + /** `null`: no strategy knows the token, so the caller must send it down another path. */ + async verifyApiKey(apiKey: string): Promise { try { const tokenGrant = await this.authStrategyRegistry.buildContextFromToken(apiKey, { audience: API_KEY_AUDIENCE, }); - if (tokenGrant) { + if (tokenGrant === null) return null; + + if (tokenGrant === false) { return { - user: tokenGrant.actor ?? tokenGrant.subject, - actor: tokenGrant.actor, - // Every non-OAuth bearer token the strategy chain admits is reported - // as `api_key`, token-exchange scoped JWTs included. The distinction - // between them is not surfaced to telemetry or log streaming. - caller: { authType: 'api_key' }, + user: null, + context: { + reason: 'invalid_token', + auth_type: 'api_key', + }, }; } return { - user: null, - context: { - reason: 'invalid_token', - auth_type: 'api_key', - }, + user: tokenGrant.actor ?? tokenGrant.subject, + actor: tokenGrant.actor, + // Every non-OAuth bearer token the strategy chain admits is reported + // as `api_key`, token-exchange scoped JWTs included. The distinction + // between them is not surfaced to telemetry or log streaming. + caller: { authType: 'api_key' }, + // Only the API-key strategy sets `apiKeyId`; a token-exchange grant has none. + authMethod: tokenGrant.apiKeyId === undefined ? 'scoped-jwt' : 'api-key', }; } catch (error) { const errorForSure = ensureError(error); diff --git a/packages/cli/src/modules/mcp/mcp-server-middleware.service.ts b/packages/cli/src/modules/mcp/mcp-server-middleware.service.ts index 5cf253c7dc1..dfd7f194fe0 100644 --- a/packages/cli/src/modules/mcp/mcp-server-middleware.service.ts +++ b/packages/cli/src/modules/mcp/mcp-server-middleware.service.ts @@ -1,18 +1,24 @@ -import { AuthenticatedRequest } from '@n8n/db'; +import { Logger } from '@n8n/backend-common'; +import { UserRepository } from '@n8n/db'; import { Service } from '@n8n/di'; -import { NextFunction, Response, Request } from 'express'; -import { ensureError } from '@n8n/utils/errors/ensure-error'; - -import { AuthError } from '@n8n/errors'; -import { JwtService } from '@n8n/backend-services'; import { - OAuthTokenVerifierProxy, - type TelemetryAuthContext, - type UserWithContext, -} from '@n8n/backend-services/auth'; + extractBearer, + inboundFrom, + inboundRequestOf, + isUrlIssuer, + ResourceAdmission, + type Inbound, + type Refusal, + type RefusalReason, +} from '@n8n/inbound-auth'; +import type { SecurityContext } from '@n8n/permissions'; +import { NextFunction, Response, Request } from 'express'; + +import { JwtService } from '@n8n/backend-services'; +import type { AuthFailureReason, TelemetryAuthContext } from '@n8n/backend-services/auth'; import { Telemetry } from '@/telemetry'; -import { McpServerApiKeyService } from './mcp-api-key.service'; +import { McpServerApiKeyService, type McpApiKeyResult } from './mcp-api-key.service'; import { McpProtectedResource } from './mcp-protected-resource'; import { USER_CONNECTED_TO_MCP_EVENT, @@ -22,48 +28,62 @@ import { import type { McpAuthenticatedRequest } from './mcp.types'; import { getClientInfo, getProtocolVersion, isConnectionHandshake } from './mcp.utils'; +/** Pipeline reasons that have their own value in the MCP connection telemetry. */ +const TELEMETRY_REASONS: Partial> = { + 'no-credential': 'missing_authorization_header', + 'malformed-credential': 'invalid_bearer_format', + 'unknown-subject': 'user_not_found', + insufficient_scope: 'insufficient_scope', + revoked: 'token_not_found_in_db', +}; + +/** The pipeline reason goes into `error_details`, so dashboards keep the detail. */ +const telemetryOf = (reason: RefusalReason, urlIssuer: boolean): TelemetryAuthContext => ({ + reason: TELEMETRY_REASONS[reason] ?? 'invalid_token', + auth_type: urlIssuer ? 'oauth' : 'unknown', + error_details: reason, +}); + +/** + * The telemetry for a request whose header carries no bearer, as it was before the pipeline. + * `extractBearer` reports a missing header as `no-credential` and any other header as malformed. + */ +function headerDenial(header: string | undefined): TelemetryAuthContext { + if (header === undefined) { + return { + reason: 'missing_authorization_header', + auth_type: 'unknown', + error_details: 'Authorization header not sent', + }; + } + const problem = /^bearer(\s|$)/i.test(header) + ? 'Malformed Bearer token' + : 'Missing Bearer prefix'; + return { + reason: 'invalid_bearer_format', + auth_type: 'unknown', + error_details: `Invalid authorization header format - ${problem}`, + }; +} + /** * MCP Server Middleware Service - * Centralizes authentication for MCP server endpoints - * Supports both API key and OAuth token validation + * Centralizes authentication for MCP server endpoints. An MCP API key or a token-exchange JWT + * goes through `McpServerApiKeyService`. An OAuth2 access token from any trusted source that + * accepts the `instance-mcp` surface goes through the inbound-auth pipeline. */ @Service() export class McpServerMiddlewareService { constructor( private readonly mcpServerApiKeyService: McpServerApiKeyService, - private readonly oauthTokenVerifier: OAuthTokenVerifierProxy, private readonly mcpProtectedResource: McpProtectedResource, + private readonly admission: ResourceAdmission, + private readonly userRepository: UserRepository, private readonly jwtService: JwtService, private readonly telemetry: Telemetry, + private readonly logger: Logger, ) {} - /** - * Get user for a given token (API key or OAuth access token) - * Uses JWT metadata to determine token type and route to correct validation - */ - async getUserForToken(token: string): Promise { - let decoded: { meta?: { isOAuth?: boolean } }; - try { - decoded = this.jwtService.decodeUnverified<{ meta?: { isOAuth?: boolean } }>(token); - } catch (error) { - return { - user: null, - context: { - reason: 'jwt_decode_failed', - auth_type: 'unknown', - error_details: ensureError(error).message, - }, - }; - } - - if (decoded?.meta?.isOAuth === true) { - const expectedAudience = this.mcpProtectedResource.getResourceUrl(); - return await this.oauthTokenVerifier.verifyOAuthAccessToken(token, expectedAudience); - } - - return await this.mcpServerApiKeyService.verifyApiKey(token); - } - /** * Express middleware hiding the MCP server while instance MCP access is off. * Answers 404 instead of an authentication challenge, so OAuth-aware clients @@ -87,75 +107,135 @@ export class McpServerMiddlewareService { } /** - * Express middleware for MCP server authentication - * Validates Bearer token (OAuth or API key) and attaches user to request + * Express middleware for MCP server authentication. Routes the bearer on its `iss`: an + * http(s) issuer goes to the pipeline, anything else to the API-key path first. */ getAuthMiddleware() { return async (req: Request, res: Response, next: NextFunction) => { - const authorizationHeader = req.header('authorization'); - - if (!authorizationHeader) { - this.responseWithUnauthorized(res, req, { - reason: 'missing_authorization_header', - auth_type: 'unknown', - error_details: 'Authorization header not sent', - }); + const inbound = inboundFrom(this.mcpProtectedResource, inboundRequestOf(req)); + const extracted = extractBearer(inbound); + if (!extracted.ok) { + const refused = this.admission.refuse(inbound, extracted.reason); + await this.deny(req, res, refused, headerDenial(req.header('authorization'))); return; } - let token: string; - try { - token = this.extractBearerToken(authorizationHeader); - } catch (er) { - const error = ensureError(er); - this.responseWithUnauthorized(res, req, { - reason: 'invalid_bearer_format', - auth_type: 'unknown', - error_details: error.message, - }); - return; + const { token } = extracted.value.credential; + const urlIssuer = isUrlIssuer(this.issuerOf(token)); + if (!urlIssuer) { + const result = await this.mcpServerApiKeyService.verifyApiKey(token); + if (result !== null) { + await this.settleApiKey(req, res, next, inbound, result); + return; + } + // No API-key strategy knows the token. The pipeline refuses it with its own reason. } - const result = await this.getUserForToken(token); - const user = result.user; - - if (!user) { - this.responseWithUnauthorized(res, req, result.context); + const admitted = await this.admission.admit(this.mcpProtectedResource, extracted.value); + if (!admitted.ok) { + await this.deny(req, res, admitted, telemetryOf(admitted.reason, urlIssuer)); return; } - - (req as AuthenticatedRequest).user = user; - const mcpReq = req as McpAuthenticatedRequest; - mcpReq.mcpCaller = result.caller; - // undefined for API keys = not scope-bearing → full tool access - mcpReq.mcpScopes = result.scopes; - - next(); + await this.settleSecurityContext(req, res, next, inbound, admitted.context); }; } - private extractBearerToken(headerValue: string): string { - if (!headerValue.startsWith('Bearer')) { - throw new AuthError('Invalid authorization header format - Missing Bearer prefix'); + /** Routing only, never a decision: the token is not verified yet. */ + private issuerOf(token: string): unknown { + try { + return this.jwtService.decodeUnverified<{ iss?: unknown } | null>(token)?.iss; + } catch { + // `jwt.decode` throws on a JWT-typed token whose payload is not JSON. + return undefined; } - - const tokenMatch = headerValue.match(/^Bearer\s+(.+)$/i); - if (tokenMatch) { - return tokenMatch[1]; - } - - throw new AuthError('Invalid authorization header format - Malformed Bearer token'); } - private responseWithUnauthorized(res: Response, req: Request, context?: TelemetryAuthContext) { - this.trackFailedConnection(req, UNAUTHORIZED_ERROR_MESSAGE, 401, context); - // RFC 6750 Section 3 / RFC 9728 Section 5.1: include the WWW-Authenticate - // header on 401s, advertising the protected-resource metadata URL so - // clients discover it directly instead of guessing the well-known path. - const prmUrl = this.mcpProtectedResource.getProtectedResourceMetadataUrl(); - res.header('WWW-Authenticate', `Bearer realm="n8n MCP Server", resource_metadata="${prmUrl}"`); + private async settleSecurityContext( + req: Request, + res: Response, + next: NextFunction, + inbound: Inbound, + context: SecurityContext, + ) { + // RFC 9068 §2.2 requires `client_id`, and MCP attributes every OAuth call to a client. + if (context.clientId === undefined) { + await this.refuseAdmitted(req, res, inbound, context, 'not-an-access-token'); + return; + } + + // The context names a Principal. The tools need the User entity with its role. + const user = await this.userRepository.findByIdWithRole(context.subject.id); + if (!user) { + await this.refuseAdmitted(req, res, inbound, context, 'unknown-subject'); + return; + } + + const mcpReq = req as McpAuthenticatedRequest; + mcpReq.user = user; + mcpReq.mcpCaller = { authType: 'oauth', clientId: context.clientId }; + // An OAuth caller is always scope-bearing: `undefined` would mean full tool access. + mcpReq.mcpScopes = context.tokenScopes ?? []; + // Keep what earlier middleware put on the operation context. + mcpReq.ctx = { ...mcpReq.ctx, security: context }; + next(); + } + + private async refuseAdmitted( + req: Request, + res: Response, + inbound: Inbound, + context: SecurityContext, + reason: RefusalReason, + ) { + const refused = this.admission.refuse(inbound, reason, context); + await this.deny(req, res, refused, telemetryOf(reason, true)); + } + + /** The API-key path is outside the pipeline's vocabulary, so it writes its own line. */ + private async settleApiKey( + req: Request, + res: Response, + next: NextFunction, + inbound: Inbound, + result: McpApiKeyResult, + ) { + const where = { surface: inbound.surface, resource: inbound.resource.url }; + if (!result.user) { + const telemetry: TelemetryAuthContext = result.context ?? { + reason: 'invalid_token', + auth_type: 'api_key', + }; + this.logger.warn('Inbound authentication denied', { ...where, reason: telemetry.reason }); + // No reason: the challenge carries no error, as before. + const advertised = { surface: inbound.surface, resource: inbound.resource }; + await this.deny(req, res, { advertised }, telemetry); + return; + } + + this.logger.info('Inbound authentication accepted', { + ...where, + authMethod: result.authMethod, + userId: result.user.id, + }); + const mcpReq = req as McpAuthenticatedRequest; + mcpReq.user = result.user; + mcpReq.mcpCaller = result.caller; + // undefined for API keys = not scope-bearing → full tool access + mcpReq.mcpScopes = result.scopes; + next(); + } + + private async deny( + req: Request, + res: Response, + refusal: Refusal, + telemetry: TelemetryAuthContext, + ) { + this.trackFailedConnection(req, UNAUTHORIZED_ERROR_MESSAGE, 401, telemetry); + // RFC 6750 §3 / RFC 9728 §5.1: the challenge names the protected-resource metadata. + res.header('WWW-Authenticate', await this.admission.challenge(refusal)); res.status(401).send({ - message: `${UNAUTHORIZED_ERROR_MESSAGE}${context?.error_details ? ': ' + context.error_details : ''}`, + message: `${UNAUTHORIZED_ERROR_MESSAGE}${telemetry.error_details ? ': ' + telemetry.error_details : ''}`, }); } diff --git a/packages/cli/src/modules/mcp/mcp.module.ts b/packages/cli/src/modules/mcp/mcp.module.ts index 8f8f5c34eda..da9453eccd3 100644 --- a/packages/cli/src/modules/mcp/mcp.module.ts +++ b/packages/cli/src/modules/mcp/mcp.module.ts @@ -6,8 +6,9 @@ import { Container } from '@n8n/di'; /** * Handles instance-level MCP access. * Runs MCP server and exposes endpoints for MCP clients to connect to. - * Requires MCP access to be enabled in settings and a valid API key or an - * OAuth token issued by the shared `oauth-server` module. + * Requires MCP access to be enabled in settings and a valid API key, a + * token-exchange JWT, or an OAuth2 access token from any trusted source that + * accepts the `instance-mcp` surface. */ @BackendModule({ name: 'mcp', instanceTypes: ['main'] }) export class McpModule implements ModuleInterface { diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-access-token-signing.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-access-token-signing.api.test.ts index 6af74e94d9f..962bb4b5041 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-access-token-signing.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-access-token-signing.api.test.ts @@ -48,7 +48,6 @@ describe('OAuth access-token signing', () => { iat: now, exp: now + 60, scope: '', - meta: { isOAuth: true }, }; }; @@ -80,7 +79,6 @@ describe('OAuth access-token signing', () => { iat: expect.any(Number), exp: payload.iat! + OAUTH_ACCESS_TOKEN_TTL_SECONDS, scope: 'tool:read', - meta: { isOAuth: true }, }); }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts index 4f881b0172b..fdf11b6bea0 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-token.service.test.ts @@ -127,7 +127,7 @@ describe('OAuthTokenService', () => { expect(decoded.sub).toBe(userId); expect(decoded.aud).toBe(TEST_RESOURCE_URL); expect(decoded.client_id).toBe(clientId); - expect(decoded.meta.isOAuth).toBe(true); + expect(decoded.meta).toBeUndefined(); expect(decoded.jti).toBeDefined(); expect(decoded.iat).toBeDefined(); expect(decoded.exp).toBeDefined(); @@ -1024,7 +1024,6 @@ describe('OAuthTokenService', () => { iat: now, exp: now + OAUTH_ACCESS_TOKEN_TTL_SECONDS, scope: '', - meta: { isOAuth: true }, }; }; diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts index 577ce4d8a9a..631c599915e 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts @@ -8,6 +8,7 @@ import request from 'supertest'; import { AuthService } from '@/auth/auth.service'; import { AUTH_COOKIE_NAME } from '@/constants'; +import { McpProtectedResource } from '@/modules/mcp/mcp-protected-resource'; import { McpSettingsService } from '@/modules/mcp/mcp.settings.service'; import { UrlService } from '@n8n/backend-services'; import { ProtectedResourceRegistry } from '@n8n/backend-services/auth'; @@ -19,16 +20,24 @@ import { OAuthServerConfig } from '../oauth-server.config'; import type { OAuthController as OAuthControllerClass } from '../oauth.controller'; import { pendingFlowFrom } from './oauth-flow-test-utils'; -const testServer = setupTestServer({ modules: ['oauth-server', 'mcp'], endpointGroups: ['mcp'] }); +const testServer = setupTestServer({ + // Production order: the MCP middleware and metadata need the inbound-auth contracts. + modules: ['inbound-auth-core', 'oauth-server', 'mcp'], + endpointGroups: ['mcp'], +}); let owner: User; let mcpSettingsService: McpSettingsService; +/** Every resource's scopes: the authorization server advertises the union. */ let supportedScopes: string[]; +/** The MCP resource's own scopes, which its protected-resource metadata advertises. */ +let mcpScopes: string[]; beforeAll(async () => { owner = await createOwner(); mcpSettingsService = Container.get(McpSettingsService); supportedScopes = Container.get(ProtectedResourceRegistry).getAllScopes(); + mcpScopes = Container.get(McpProtectedResource).scopes; }); afterEach(async () => { @@ -148,7 +157,7 @@ describe('GET /.well-known/oauth-protected-resource/mcp-server/http', () => { resource: expect.stringContaining('/mcp-server/http'), bearer_methods_supported: ['header'], authorization_servers: [expect.any(String)], - ...(supportedScopes.length > 0 && { scopes_supported: supportedScopes }), + ...(mcpScopes.length > 0 && { scopes_supported: mcpScopes }), }); }); @@ -182,7 +191,7 @@ describe('GET /.well-known/oauth-protected-resource/mcp-server/http', () => { ); expect(response.statusCode).toBe(200); - expect(response.body.scopes_supported).toEqual(supportedScopes); + expect(response.body.scopes_supported).toEqual(mcpScopes); }); test('should be accessible without authentication', async () => { @@ -220,7 +229,7 @@ describe('GET /.well-known/oauth-protected-resource (bare path)', () => { resource: expect.stringContaining('/mcp-server/http'), bearer_methods_supported: ['header'], authorization_servers: [expect.any(String)], - ...(supportedScopes.length > 0 && { scopes_supported: supportedScopes }), + ...(mcpScopes.length > 0 && { scopes_supported: mcpScopes }), }); }); @@ -669,7 +678,7 @@ describe('OAuth Discovery - Cross-validation', () => { expect(protectedResource.authorization_servers).toContain(authServer.issuer); }); - test('should have consistent scopes between authorization server and protected resource', async () => { + test('should advertise every MCP scope on the authorization server', async () => { const authServerResponse = await testServer.restlessAgent.get( '/.well-known/oauth-authorization-server', ); @@ -683,7 +692,8 @@ describe('OAuth Discovery - Cross-validation', () => { const authServerScopes = authServerResponse.body.scopes_supported; const protectedResourceScopes = protectedResourceResponse.body.scopes_supported; - expect(authServerScopes).toEqual(protectedResourceScopes); + // The public API's resource is registered too, so the server lists more than MCP's scopes. + expect(authServerScopes).toEqual(expect.arrayContaining(protectedResourceScopes)); }); }); @@ -736,7 +746,7 @@ describe('Full authorization-code flow (PKCE)', () => { // is withheld at consent here because the community-packages module is // inactive in the test instance. `aiPreference:*` is withheld too: the test // instance has no PostHog, so no user is in the preferences experiment arm. - const grantedScopes = supportedScopes.filter( + const grantedScopes = mcpScopes.filter( (scope) => scope !== 'communityPackage:install' && !scope.startsWith('aiPreference:'), ); const authAgent = testServer.authAgentFor(owner); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts index 1248a57e975..1183fbc0c04 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.test.ts @@ -64,6 +64,10 @@ const publicApiResource: ProtectedResource = { beforeEach(() => { vi.resetAllMocks(); urlService.getInstanceBaseUrl.mockReturnValue('https://n8n.test'); + authenticationService.advertise.mockResolvedValue({ + authorizationServers: ['https://n8n.test'], + challenges: [], + }); controller = new OAuthController(urlService, registry, localServer, authenticationService); }); @@ -121,9 +125,9 @@ describe('OAuthController', () => { expect(registry.getByResourcePath).toHaveBeenCalledWith('/webhook/orders?method=GET'); }); - test('returns the RFC 9728 metadata document for a resolved resource', async () => { + test('returns the RFC 9728 metadata document for a surface outside the pipeline', async () => { const scopes = ['tool:listWorkflows', 'tool:getWorkflowDetails']; - registry.getByResourcePath.mockResolvedValue(resource(scopes)); + registry.getByResourcePath.mockResolvedValue({ ...resource(scopes), surface: 'trigger' }); const res = makeRes(); await controller.protectedResourceMetadata(makeReq(['mcp-server', 'http']), res); @@ -139,6 +143,30 @@ describe('OAuthController', () => { expect(authenticationService.advertise).not.toHaveBeenCalled(); }); + test('lists the issuers of the trusted sources accepted on the instance-mcp surface', async () => { + registry.getByResourcePath.mockResolvedValue(resource(['tool:listWorkflows'])); + authenticationService.advertise.mockResolvedValue({ + authorizationServers: ['https://n8n.test', 'https://idp.example'], + challenges: [], + }); + const res = makeRes(); + + await controller.protectedResourceMetadata(makeReq(['mcp-server', 'http']), res); + + expect(authenticationService.advertise).toHaveBeenCalledWith({ + surface: 'instance-mcp', + resource: { + url: 'https://n8n.test/mcp-server/http', + acceptedAudiences: ['https://n8n.test/mcp-server/http'], + }, + }); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + authorization_servers: ['https://n8n.test', 'https://idp.example'], + }), + ); + }); + test('lists the issuers of the trusted sources accepted on the public-api surface', async () => { registry.getByResourcePath.mockResolvedValue(publicApiResource); authenticationService.advertise.mockResolvedValue({ diff --git a/packages/cli/src/modules/oauth-server/oauth-token.service.ts b/packages/cli/src/modules/oauth-server/oauth-token.service.ts index 26296a01302..fe20a925f3a 100644 --- a/packages/cli/src/modules/oauth-server/oauth-token.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-token.service.ts @@ -93,9 +93,6 @@ export class OAuthTokenService implements OAuthTokenVerifier { // (empty string for scope-less grants), so an absent claim // unambiguously identifies a token minted before scoping shipped. scope: scopes.join(' '), - meta: { - isOAuth: true, - }, }, audience, ); diff --git a/packages/cli/src/modules/oauth-server/oauth.controller.ts b/packages/cli/src/modules/oauth-server/oauth.controller.ts index 6012c91b151..f45f3327046 100644 --- a/packages/cli/src/modules/oauth-server/oauth.controller.ts +++ b/packages/cli/src/modules/oauth-server/oauth.controller.ts @@ -13,7 +13,7 @@ import { StaticRouterMetadata, } from '@n8n/decorators'; import { Container } from '@n8n/di'; -import { AuthenticationService } from '@n8n/inbound-auth'; +import { AuthenticationService, type SurfaceId } from '@n8n/inbound-auth'; import { isRecord } from '@n8n/utils/is-record'; import type { Response, Request, RequestHandler, Router } from 'express'; @@ -32,6 +32,15 @@ const oauthServerConfig = Container.get(OAuthServerConfig); const logger = Container.get(Logger); const urlService = Container.get(UrlService); +/** + * Surfaces that authenticate through the inbound-auth pipeline advertise the trusted sources + * accepted there. The trigger surface joins with its own ticket. + */ +const PIPELINE_SURFACES: ReadonlySet = new Set([ + 'public-api', + 'instance-mcp', +]); + /** * Pre-check guard for the unauthenticated DCR endpoint. Short-circuits with * a structured `server_error` response when the instance is at the @@ -340,18 +349,17 @@ export class OAuthController { private async buildProtectedResourceMetadata( resource: ProtectedResource, ): Promise> { - const authorizationServers = - resource.surface === 'public-api' - ? ( - await this.authenticationService.advertise({ - surface: resource.surface, - resource: { - url: resource.getResourceUrl(), - acceptedAudiences: resource.getAudiences(), - }, - }) - ).authorizationServers - : [this.urlService.getInstanceBaseUrl()]; + const authorizationServers = PIPELINE_SURFACES.has(resource.surface) + ? ( + await this.authenticationService.advertise({ + surface: resource.surface, + resource: { + url: resource.getResourceUrl(), + acceptedAudiences: resource.getAudiences(), + }, + }) + ).authorizationServers + : [this.urlService.getInstanceBaseUrl()]; const metadata: Record = { resource: resource.getResourceUrl(), diff --git a/packages/cli/test/integration/mcp/mcp-trusted-source-auth.test.ts b/packages/cli/test/integration/mcp/mcp-trusted-source-auth.test.ts new file mode 100644 index 00000000000..116b88d1c3a --- /dev/null +++ b/packages/cli/test/integration/mcp/mcp-trusted-source-auth.test.ts @@ -0,0 +1,330 @@ +import { Logger } from '@n8n/backend-common'; +import { CacheService, type OAuthDiscoveryClient, UrlService } from '@n8n/backend-services'; +import { Container } from '@n8n/di'; +import type { User } from '@n8n/db'; +import { + AuthenticationPipeline, + JwkSchema, + LocalAuthorizationServer, + trustedSourceConfigSchemaFor, + type SurfaceId, +} from '@n8n/inbound-auth'; +import { DataSource } from '@n8n/typeorm'; +import { exportJWK, generateKeyPair, SignJWT } from 'jose'; +import type { CryptoKey, JWTPayload } from 'jose'; +import { Cipher } from 'n8n-core'; +import { randomString } from 'n8n-workflow'; +import { mock, mockDeep } from 'vitest-mock-extended'; + +import { TrustedSourceEntity } from '@/modules/inbound-auth-core/database/entities/trusted-source.entity'; +import { TrustedSourceDiscoveryService } from '@/modules/inbound-auth-core/trusted-source-discovery.service'; +import { TrustedSourceDbStore } from '@/modules/inbound-auth-core/trusted-source.store'; +import { McpServerApiKeyService } from '@/modules/mcp/mcp-api-key.service'; +import { McpProtectedResource } from '@/modules/mcp/mcp-protected-resource'; +import { McpService } from '@/modules/mcp/mcp.service'; +import { McpSettingsService } from '@/modules/mcp/mcp.settings.service'; +import { OAuthClientRepository } from '@/modules/oauth-server/database/repositories/oauth-client.repository'; +import { OAuthTokenService } from '@/modules/oauth-server/oauth-token.service'; +import { ScopedJwtStrategy } from '@/modules/token-exchange/services/scoped-jwt.strategy'; +import { TOKEN_EXCHANGE_ISSUER } from '@/modules/token-exchange/token-exchange.types'; +import { AuthStrategyRegistry } from '@n8n/backend-services/auth'; +import { JwtService } from '@n8n/backend-services'; +import { createMember } from '@test-integration/db/users'; +import { setupTestServer } from '@test-integration/utils'; + +/** + * The instance MCP server on the trusted-source pipeline, nothing mocked but the network: the + * pipeline, the admission and the identity service are the real ones. + */ +const testServer = setupTestServer({ + // Production order: the MCP middleware resolves the inbound-auth contracts when it is built. + modules: ['inbound-auth-core', 'oauth-jwe', 'oauth-server', 'mcp'], + endpointGroups: ['mcp', 'jwks'], +}); + +const MCP_ISSUER = 'https://idp-a.example'; +const PUBLIC_API_ISSUER = 'https://idp-b.example'; +const CLIENT_ID = 'external-client'; + +let member: User; +let mcpResource: string; +let metadataUrl: string; +let privateKey: CryptoKey; +let apiKey: string; + +const now = () => Math.floor(Date.now() / 1000); + +/** A token from an external IdP for `member`, linked by its verified email on first use. */ +async function externalToken(issuer: string, claims: JWTPayload = {}) { + const iat = now(); + return await new SignJWT({ + iss: issuer, + sub: 'idp-alice', + aud: mcpResource, + email: member.email, + email_verified: true, + client_id: CLIENT_ID, + scope: 'workflow:read', + iat, + exp: iat + 3600, + ...claims, + }) + .setProtectedHeader({ alg: 'ES256', kid: 'k1', typ: 'at+jwt' }) + .sign(privateKey); +} + +async function seedSource(issuer: string, surface: SurfaceId, keys: unknown[]) { + const config = trustedSourceConfigSchemaFor('admin').parse({ + version: 1, + authentication: { type: 'oauth2' }, + surfaces: { [surface]: {} }, + }); + const rows = Container.get(DataSource).getRepository(TrustedSourceEntity); + await rows.save( + rows.create({ + name: `${surface} source`, + type: 'oauth2', + issuer, + managedBy: 'admin', + status: 'healthy', + lastError: null, + lastCheckedAt: new Date(), + configVersion: 1, + config: await Container.get(Cipher).encryptV2(config), + metadata: JSON.stringify({ + version: 1, + documents: [ + { kind: 'jwks', fetchedAt: new Date().toISOString(), url: `${issuer}/keys`, keys }, + ], + }), + discoveryClaimToken: null, + discoveryClaimedAt: null, + }), + ); +} + +/** An access token as the instance issues it: signed and recorded, so it stays active until revoked. */ +async function internalToken() { + const tokens = Container.get(OAuthTokenService); + const scopes = ['workflow:read']; + const pair = tokens.generateTokenPair(member.id, CLIENT_ID, mcpResource, scopes); + await tokens.saveTokenPair( + pair.accessToken, + pair.refreshToken, + CLIENT_ID, + member.id, + scopes, + pair.audience, + ); + return pair.accessToken; +} + +const probe = async (token?: string) => { + const request = testServer.restlessAgent.get('/mcp-server/http'); + return await (token === undefined ? request : request.set('Authorization', `Bearer ${token}`)); +}; + +/** Every line the authentication edge wrote, from the pipeline, the admission or the middleware. */ +const authLines = (logger: Logger) => + [...vi.mocked(logger.info).mock.calls, ...vi.mocked(logger.warn).mock.calls].filter(([message]) => + message.startsWith('Inbound authentication'), + ); + +const spyOnLogger = () => { + const logger = Container.get(Logger); + vi.spyOn(logger, 'info'); + vi.spyOn(logger, 'warn'); + return logger; +}; + +beforeAll(async () => { + await Container.get(CacheService).init(); + member = await createMember(); + const resource = Container.get(McpProtectedResource); + mcpResource = resource.getResourceUrl(); + metadataUrl = resource.getProtectedResourceMetadataUrl(); + Container.get(AuthStrategyRegistry).register(Container.get(ScopedJwtStrategy)); + apiKey = (await Container.get(McpServerApiKeyService).createMcpServerApiKey(member)).apiKey; + await Container.get(McpSettingsService).setEnabled(true); + // The token rows of the local server need a registered client. + await Container.get(OAuthClientRepository).save({ + id: CLIENT_ID, + name: 'MCP pipeline tests', + redirectUris: ['https://example.com/callback'], + grantTypes: ['authorization_code'], + tokenEndpointAuthMethod: 'none', + }); + + // The module seeded `n8n-internal` at init; discovery normally runs as a system task. Run it + // once here, reading the local authorization server in process, never over HTTP. + const offline = mock(); + const refused = new Error('discovery must not go over HTTP'); + offline.fetchOpenIdConfiguration.mockRejectedValue(refused); + offline.fetchOAuth2ServerMetadata.mockRejectedValue(refused); + offline.fetchJwks.mockRejectedValue(refused); + await new TrustedSourceDiscoveryService( + Container.get(Logger), + Container.get(TrustedSourceDbStore), + offline, + Container.get(LocalAuthorizationServer), + ).refresh('n8n-internal'); + + const pair = await generateKeyPair('ES256'); + privateKey = pair.privateKey; + const jwk = JwkSchema.parse({ + ...(await exportJWK(pair.publicKey)), + kid: 'k1', + alg: 'ES256', + use: 'sig', + }); + await seedSource(MCP_ISSUER, 'instance-mcp', [jwk]); + await seedSource(PUBLIC_API_ISSUER, 'public-api', [jwk]); + await Container.get(CacheService).reset(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('Instance MCP authentication through the trusted-source pipeline', () => { + test('admits an access token from the instance itself', async () => { + const response = await probe(await internalToken()); + + // GET answers 405 once the caller is authenticated. + expect(response.statusCode).toBe(405); + }); + + test('refuses an access token from the instance after it is revoked', async () => { + const token = await internalToken(); + await Container.get(OAuthTokenService).revokeAllTokensForGrant(CLIENT_ID, member.id); + + const response = await probe(token); + + expect(response.statusCode).toBe(401); + expect(response.headers['www-authenticate']).toMatch(/error_description="revoked"$/); + }); + + test('admits a token from a trusted source that accepts instance-mcp, as an OAuth caller', async () => { + const token = await externalToken(MCP_ISSUER); + expect((await probe(token)).statusCode).toBe(405); + // Only what the middleware hands to the server matters here, so stop there. + const mcpService = Container.get(McpService); + vi.spyOn(mcpService, 'resolveFeatureFlags').mockResolvedValue( + mockDeep>>(), + ); + const getServer = vi + .spyOn(mcpService, 'getServer') + .mockRejectedValue(new Error('stop after authentication')); + + await testServer.restlessAgent + .post('/mcp-server/http') + .set('Authorization', `Bearer ${token}`) + .set('Accept', 'application/json, text/event-stream') + .send({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'test-client', version: '1.0.0' }, + }, + }); + + expect(getServer).toHaveBeenCalledWith( + expect.objectContaining({ id: member.id }), + expect.anything(), + expect.anything(), + { caller: { authType: 'oauth', clientId: CLIENT_ID }, grantedScopes: ['workflow:read'] }, + ); + }); + + test('refuses a source that does not accept instance-mcp, with a reason that differs from a bad token', async () => { + const notAccepted = await probe(await externalToken(PUBLIC_API_ISSUER)); + const tampered = await probe(`${(await internalToken()).slice(0, -2)}xx`); + + expect(notAccepted.statusCode).toBe(401); + expect(notAccepted.headers['www-authenticate']).toMatch( + /error="invalid_token", error_description="source-not-accepted"$/, + ); + expect(tampered.statusCode).toBe(401); + expect(tampered.headers['www-authenticate']).toMatch( + /error="invalid_token", error_description="signature-invalid"$/, + ); + }); + + test('keeps an MCP API key and a token-exchange JWT off the pipeline', async () => { + const run = vi.spyOn(Container.get(AuthenticationPipeline), 'run'); + const scopedJwt = Container.get(JwtService).sign('tokenExchange', { + iss: TOKEN_EXCHANGE_ISSUER, + sub: member.id, + iat: now(), + exp: now() + 3600, + jti: `jti-${randomString(8)}`, + }); + + expect((await probe(apiKey)).statusCode).toBe(405); + expect((await probe(scopedJwt)).statusCode).toBe(405); + expect(run).not.toHaveBeenCalled(); + }); + + test('names the protected-resource metadata in every challenge', async () => { + const responses = [ + await probe(), + await probe('not-a-token'), + await probe(await externalToken(PUBLIC_API_ISSUER)), + ]; + + for (const response of responses) { + expect(response.statusCode).toBe(401); + expect(response.headers['www-authenticate']).toContain(`resource_metadata="${metadataUrl}"`); + } + // A request without a credential gets no error code (RFC 6750 §3.1). + expect(responses[0].headers['www-authenticate']).not.toContain('error='); + }); + + test('lists the issuers of the sources that accept instance-mcp in the metadata', async () => { + const metadata = await testServer.restlessAgent + .get('/.well-known/oauth-protected-resource/mcp-server/http') + .expect(200); + + expect(metadata.body.authorization_servers).toEqual( + expect.arrayContaining([Container.get(UrlService).getInstanceBaseUrl(), MCP_ISSUER]), + ); + expect(metadata.body.authorization_servers).not.toContain(PUBLIC_API_ISSUER); + }); + + test('writes one authentication line per outcome, with no token, external subject or email', async () => { + const logger = spyOnLogger(); + const tokens = [ + undefined, + await internalToken(), + await externalToken(MCP_ISSUER), + apiKey, + await externalToken(PUBLIC_API_ISSUER), + ]; + + for (const token of tokens) { + vi.mocked(logger.info).mockClear(); + vi.mocked(logger.warn).mockClear(); + await probe(token); + expect(authLines(logger)).toHaveLength(1); + + const logged = JSON.stringify(authLines(logger)); + if (token !== undefined) expect(logged).not.toContain(token); + expect(logged).not.toContain('idp-alice'); + expect(logged).not.toContain(member.email); + } + }); + + test('refuses a JWT without an issuer with unknown-issuer, in one line', async () => { + const logger = spyOnLogger(); + const token = Container.get(JwtService).sign('mcpApiKey', { sub: member.id }); + + const response = await probe(token); + + expect(response.statusCode).toBe(401); + expect(response.headers['www-authenticate']).toMatch(/error_description="unknown-issuer"$/); + expect(authLines(logger)).toHaveLength(1); + }); +});