From 5926ebf0940d3483665f06cdd0e0ee2465ff53fb Mon Sep 17 00:00:00 2001 From: Stephen Wright Date: Fri, 5 Dec 2025 15:16:10 +0000 Subject: [PATCH] feat: Make underlying oauth abstractions more generic (#22741) --- .../abstract-oauth.controller.test.ts | 47 - .../oauth1-credential.controller.test.ts | 230 +--- .../oauth2-credential.controller.test.ts | 983 +++++++----------- .../oauth/oauth1-credential.controller.ts | 47 +- .../oauth/oauth2-credential.controller.ts | 59 +- .../src/oauth/__tests__/oauth.service.test.ts | 693 ++++++++++++ .../oauth.service.ts} | 82 +- .../controllers/oauth/oauth2.api.test.ts | 14 +- 8 files changed, 1233 insertions(+), 922 deletions(-) delete mode 100644 packages/cli/src/controllers/oauth/__tests__/abstract-oauth.controller.test.ts create mode 100644 packages/cli/src/oauth/__tests__/oauth.service.test.ts rename packages/cli/src/{controllers/oauth/abstract-oauth.controller.ts => oauth/oauth.service.ts} (77%) diff --git a/packages/cli/src/controllers/oauth/__tests__/abstract-oauth.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/abstract-oauth.controller.test.ts deleted file mode 100644 index 01f4633340d..00000000000 --- a/packages/cli/src/controllers/oauth/__tests__/abstract-oauth.controller.test.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { shouldSkipAuthOnOAuthCallback } from '../abstract-oauth.controller'; - -describe('shouldSkipAuthOnOAuthCallback', () => { - const originalEnv = process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK; - - afterEach(() => { - // Restore original environment variable after each test - if (originalEnv === undefined) { - delete process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK; - } else { - process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK = originalEnv; - } - }); - - describe('when N8N_SKIP_AUTH_ON_OAUTH_CALLBACK is not set', () => { - beforeEach(() => { - delete process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK; - }); - - it('should return true', () => { - expect(shouldSkipAuthOnOAuthCallback()).toBe(false); - }); - }); - - describe('with various environment variable values', () => { - const testCases = [ - { value: 'true', expected: true }, - { value: 'TRUE', expected: true }, - { value: 'True', expected: true }, - { value: 'false', expected: false }, - { value: 'FALSE', expected: false }, - { value: 'False', expected: false }, - { value: '', expected: false }, - { value: '1', expected: false }, - { value: 'yes', expected: false }, - { value: 'on', expected: false }, - { value: 'enabled', expected: false }, - { value: ' ', expected: false }, - { value: ' true ', expected: false }, - ] as const; - - test.each(testCases)('"%s" value should return %s', ({ value, expected }) => { - process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK = value; - expect(shouldSkipAuthOnOAuthCallback()).toBe(expected); - }); - }); -}); diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts index d87459b5276..2f48de84687 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts @@ -1,57 +1,22 @@ import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; -import { Time } from '@n8n/constants'; -import type { CredentialsEntity, User } from '@n8n/db'; -import { CredentialsRepository, GLOBAL_OWNER_ROLE } from '@n8n/db'; +import { type CredentialsEntity, type User } from '@n8n/db'; import { Container } from '@n8n/di'; -import Csrf from 'csrf'; +import { mock } from 'jest-mock-extended'; +import axios from 'axios'; import type { Response } from 'express'; -import { captor, mock } from 'jest-mock-extended'; -import { Cipher, type InstanceSettings, ExternalSecretsProxy } from 'n8n-core'; -import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; -import nock from 'nock'; - import { OAuth1CredentialController } from '@/controllers/oauth/oauth1-credential.controller'; -import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; -import { CredentialsHelper } from '@/credentials-helper'; -import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; -import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { NotFoundError } from '@/errors/response-errors/not-found.error'; -import { ExternalHooks } from '@/external-hooks'; import type { OAuthRequest } from '@/requests'; -import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; +import { OauthService } from '@/oauth/oauth.service'; +import { ExternalHooks } from '@/external-hooks'; -jest.mock('@/workflow-execute-additional-data'); +jest.mock('axios'); describe('OAuth1CredentialController', () => { + const oauthService = mockInstance(OauthService); + mockInstance(Logger); mockInstance(ExternalHooks); - mockInstance(ExternalSecretsProxy); - mockInstance(VariablesService, { - getAllCached: async () => [], - }); - const additionalData = mock(); - (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); - - const cipher = new Cipher(mock({ encryptionKey: 'password' })); - Container.set(Cipher, cipher); - const credentialsHelper = mockInstance(CredentialsHelper); - const credentialsRepository = mockInstance(CredentialsRepository); - const credentialsFinderService = mockInstance(CredentialsFinderService); - - const csrfSecret = 'csrf-secret'; - const user = mock({ - id: '123', - password: 'password', - authIdentities: [], - role: GLOBAL_OWNER_ROLE, - }); - const credential = mock({ - id: '1', - name: 'Test Credential', - type: 'oAuth1Api', - data: cipher.encrypt({}), - }); const controller = Container.get(OAuth1CredentialController); @@ -64,64 +29,26 @@ describe('OAuth1CredentialController', () => { }); describe('getAuthUri', () => { - it('should throw a BadRequestError when credentialId is missing in the query', async () => { - const req = mock({ query: { id: '' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - new BadRequestError('Required credential ID is missing'), - ); - }); - - it('should throw a NotFoundError when no matching credential is found for the user', async () => { - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(null); - - const req = mock({ user, query: { id: '1' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - new NotFoundError('Credential not found'), - ); - }); - it('should return a valid auth URI', async () => { - jest.spyOn(Csrf.prototype, 'secretSync').mockReturnValueOnce(csrfSecret); - jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token'); - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValueOnce({ + const mockResolvedCredential = mock({ id: '1' }); + oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.createCsrfState.mockReturnValueOnce(['csrf-secret', 'state']); + oauthService.getOAuthCredentials.mockResolvedValueOnce({ requestTokenUrl: 'https://example.domain/oauth/request_token', authUrl: 'https://example.domain/oauth/authorize', accessTokenUrl: 'https://example.domain/oauth/access_token', - signatureMethod: 'HMAC-SHA1', + signatureMethod: 'HMAC-SHA1' as const, + }); + jest.mocked(axios).request.mockResolvedValueOnce({ data: { oauth_token: 'random-token' } }); + const req = mock({ + user: mock({ id: '123' }), + query: { id: '1' }, }); - nock('https://example.domain') - .post('/oauth/request_token', { - oauth_callback: - 'http://localhost:5678/rest/oauth1-credential/callback?state=eyJ0b2tlbiI6InRva2VuIiwiY2lkIjoiMSIsImNyZWF0ZWRBdCI6MTcwNjc1MDYyNTY3OCwidXNlcklkIjoiMTIzIn0=', - }) - .once() - .reply(200, { oauth_token: 'random-token' }); - - const req = mock({ user, query: { id: '1' } }); const authUri = await controller.getAuthUri(req); expect(authUri).toEqual('https://example.domain/oauth/authorize?oauth_token=random-token'); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', - expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth1Api', - }), - ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ csrfSecret: 'csrf-secret' }), - ); - expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( - additionalData, - credential, - credential.type, - 'internal', - undefined, - false, + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ csrfSecret: 'csrf-secret' }), ); }); }); @@ -149,113 +76,40 @@ describe('OAuth1CredentialController', () => { invalidReq.query = { state: 'test' } as OAuthRequest.OAuth1Credential.Callback['query']; await controller.handleCallback(invalidReq, res); - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Insufficient parameters for OAuth1 callback.', - reason: 'Received following query parameters: {"state":"test"}', - }, - }); - expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); - }); - - it('should render the error page when `state` query param is invalid', async () => { - const invalidReq = mock({ - query: { - oauth_verifier: 'verifier', - oauth_token: 'token', - state: 'test', - }, - }); - await controller.handleCallback(invalidReq, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Invalid state format', - }, - }); - expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); - }); - - it('should render the error page when credential is not found in DB', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(null); - - await controller.handleCallback(req, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'OAuth callback failed because of insufficient permissions', - }, - }); - expect(credentialsRepository.findOneBy).toHaveBeenCalledTimes(1); - expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ id: '1' }); - }); - - it('should render the error page when state differs from the stored state in the credential', async () => { - credentialsRepository.findOneBy.mockResolvedValue(credential); - credentialsHelper.getDecrypted.mockResolvedValue({ csrfSecret: 'invalid' }); - - await controller.handleCallback(req, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'The OAuth callback state is invalid!', - }, - }); - }); - - it('should render the error page when state is older than 5 minutes', async () => { - credentialsRepository.findOneBy.mockResolvedValue(credential); - credentialsHelper.getDecrypted.mockResolvedValue({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - - jest.advanceTimersByTime(10 * Time.minutes.toMilliseconds); - - await controller.handleCallback(req, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'The OAuth callback state is invalid!', - }, - }); + expect(oauthService.renderCallbackError).toHaveBeenCalledWith( + res, + 'Insufficient parameters for OAuth1 callback.', + 'Received following query parameters: {"state":"test"}', + ); }); it('should exchange the code for a valid token, and save it to DB', async () => { - credentialsRepository.findOneBy.mockResolvedValue(credential); - credentialsHelper.getDecrypted.mockResolvedValue({ csrfSecret }); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValueOnce({ + const mockResolvedCredential = mock({ id: '1' }); + oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + // @ts-ignore + oauthService.getDecryptedData.mockResolvedValue({ csrfSecret: 'invalid' }); + oauthService.getOAuthCredentials.mockResolvedValueOnce({ requestTokenUrl: 'https://example.domain/oauth/request_token', accessTokenUrl: 'https://example.domain/oauth/access_token', signatureMethod: 'HMAC-SHA1', }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post('/oauth/access_token', 'oauth_token=token&oauth_verifier=verifier') - .once() - .reply(200, 'access_token=new_token'); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'invalid' }, + { accessTokenUrl: 'https://example.domain/oauth/access_token' }, + ]); + jest.mocked(axios).post.mockResolvedValueOnce({ data: { access_token: 'new_token' } }); await controller.handleCallback(req, res); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', + // @ts-ignore + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth1Api', + oauthTokenData: { access_token: 'new_token' }, }), - ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ oauthTokenData: { access_token: 'new_token' } }), + ['csrfSecret'], ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); - expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( - additionalData, - credential, - credential.type, - 'internal', - undefined, - true, - ); }); }); }); diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts index 57b69314ac5..6e17f64efd9 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts @@ -1,60 +1,23 @@ import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; -import { Time } from '@n8n/constants'; -import type { CredentialsEntity, User } from '@n8n/db'; -import { CredentialsRepository, GLOBAL_OWNER_ROLE } from '@n8n/db'; +import { type CredentialsEntity, type User } from '@n8n/db'; import { Container } from '@n8n/di'; -import Csrf from 'csrf'; -import { type Response } from 'express'; -import { captor, mock } from 'jest-mock-extended'; -import { Cipher, type InstanceSettings, ExternalSecretsProxy } from 'n8n-core'; -import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; -import nock from 'nock'; -import * as pkceChallenge from 'pkce-challenge'; - -import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; +import { mock } from 'jest-mock-extended'; +import type { Response } from 'express'; import { OAuth2CredentialController } from '@/controllers/oauth/oauth2-credential.controller'; -import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; -import { CredentialsHelper } from '@/credentials-helper'; -import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; -import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { NotFoundError } from '@/errors/response-errors/not-found.error'; -import { ExternalHooks } from '@/external-hooks'; import type { OAuthRequest } from '@/requests'; -import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; +import { OauthService } from '@/oauth/oauth.service'; +import { ExternalHooks } from '@/external-hooks'; -jest.mock('@/workflow-execute-additional-data'); +jest.mock('axios'); +jest.mock('@n8n/client-oauth2'); +jest.mock('pkce-challenge'); describe('OAuth2CredentialController', () => { - mockInstance(Logger); - mockInstance(ExternalSecretsProxy); - mockInstance(VariablesService, { - getAllCached: async () => [], - }); - const additionalData = mock(); - (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); - - const cipher = new Cipher(mock({ encryptionKey: 'password' })); - Container.set(Cipher, cipher); - + const oauthService = mockInstance(OauthService); const externalHooks = mockInstance(ExternalHooks); - const credentialsHelper = mockInstance(CredentialsHelper); - const credentialsRepository = mockInstance(CredentialsRepository); - const credentialsFinderService = mockInstance(CredentialsFinderService); - const csrfSecret = 'csrf-secret'; - const user = mock({ - id: '123', - password: 'password', - authIdentities: [], - role: GLOBAL_OWNER_ROLE, - }); - const credential = mock({ - id: '1', - name: 'Test Credential', - type: 'oAuth2Api', - data: cipher.encrypt({}), - }); + mockInstance(Logger); const controller = Container.get(OAuth2CredentialController); @@ -64,335 +27,147 @@ describe('OAuth2CredentialController', () => { beforeEach(() => { jest.setSystemTime(new Date(timestamp)); jest.clearAllMocks(); - - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue({ - clientId: 'test-client-id', - clientSecret: 'oauth-secret', - authUrl: 'https://example.domain/o/oauth2/v2/auth', - accessTokenUrl: 'https://example.domain/token', - }); }); describe('getAuthUri', () => { - it('should throw a BadRequestError when credentialId is missing in the query', async () => { - const req = mock({ query: { id: '' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - new BadRequestError('Required credential ID is missing'), - ); - }); - - it('should throw a NotFoundError when no matching credential is found for the user', async () => { - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(null); - - const req = mock({ user, query: { id: '1' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - new NotFoundError('Credential not found'), - ); - }); - it('should return a valid auth URI', async () => { - jest.spyOn(Csrf.prototype, 'secretSync').mockReturnValueOnce(csrfSecret); - jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token'); - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - - const req = mock({ user, query: { id: '1' } }); - const authUri = await controller.getAuthUri(req); - expect(authUri).toEqual( - 'https://example.domain/o/oauth2/v2/auth?client_id=test-client-id&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback&response_type=code&state=eyJ0b2tlbiI6InRva2VuIiwiY2lkIjoiMSIsImNyZWF0ZWRBdCI6MTcwNjc1MDYyNTY3OCwidXNlcklkIjoiMTIzIn0%3D&scope=openid', - ); - const state = new URL(authUri).searchParams.get('state'); - expect(JSON.parse(Buffer.from(state!, 'base64').toString())).toEqual({ - token: 'token', - cid: '1', - createdAt: timestamp, - userId: '123', + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetUri = jest.fn().mockReturnValue({ + toString: () => + 'https://example.domain/oauth2/auth?client_id=client_id&redirect_uri=http://localhost:5678/rest/oauth2-credential/callback&response_type=code&state=state&scope=openid', }); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getUri: mockGetUri, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.createCsrfState.mockReturnValueOnce(['csrf-secret', 'state']); + oauthService.getOAuthCredentials.mockResolvedValueOnce({ + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'header', + }); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + user: mock({ id: '123' }), + query: { id: '1' }, + }); + + const authUri = await controller.getAuthUri(req); + + expect(authUri).toContain('https://example.domain/oauth2/auth'); + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ csrfSecret: 'csrf-secret' }), + ); + expect(externalHooks.run).toHaveBeenCalledWith('oauth2.authenticate', expect.any(Array)); + }); + + it('should return a valid auth URI with PKCE', async () => { + const pkceChallenge = await import('pkce-challenge'); + jest.mocked(pkceChallenge.default).mockResolvedValueOnce({ + code_verifier: 'code_verifier', + code_challenge: 'code_challenge', + }); + + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetUri = jest.fn().mockReturnValue({ + toString: () => + 'https://example.domain/oauth2/auth?client_id=client_id&redirect_uri=http://localhost:5678/rest/oauth2-credential/callback&response_type=code&state=state&scope=openid&code_challenge=code_challenge&code_challenge_method=S256', + }); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getUri: mockGetUri, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.createCsrfState.mockReturnValueOnce(['csrf-secret', 'state']); + oauthService.getOAuthCredentials.mockResolvedValueOnce({ + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'pkce', + authentication: 'header', + }); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + user: mock({ id: '123' }), + query: { id: '1' }, + }); + + const authUri = await controller.getAuthUri(req); + + expect(authUri).toContain('code_challenge=code_challenge'); + expect(authUri).toContain('code_challenge_method=S256'); + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth2Api', + csrfSecret: 'csrf-secret', + codeVerifier: 'code_verifier', }), ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ csrfSecret: 'csrf-secret' }), - ); - expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( - additionalData, - credential, - credential.type, - 'internal', - undefined, - false, - ); }); - it.each([ - [ - ['authorization_code', 'refresh_token'], - ['client_secret_basic', 'client_secret_post', 'none'], - ['S256'], - ['authorization_code', 'refresh_token'], - 'none', - { - code_challenge: 'code-challenge', - code_challenge_method: 'S256', - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - [ - ['authorization_code', 'refresh_token'], - ['client_secret_basic', 'client_secret_post'], - ['S256'], - ['authorization_code', 'refresh_token'], - 'none', - { - code_challenge: 'code-challenge', - code_challenge_method: 'S256', - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - [ - ['authorization_code', 'refresh_token'], - ['client_secret_basic', 'client_secret_post'], - [], - ['authorization_code', 'refresh_token'], - 'client_secret_basic', - { - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - [ - ['authorization_code', 'refresh_token'], - ['client_secret_post'], - [], - ['authorization_code', 'refresh_token'], - 'client_secret_post', - { - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - [ - ['client_credentials'], - ['client_secret_basic', 'client_secret_post'], - [], - ['client_credentials'], - 'client_secret_basic', - { - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - [ - ['client_credentials'], - ['client_secret_post'], - [], - ['client_credentials'], - 'client_secret_post', - { - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid', - }, - ], - ])( - 'should return a valid auth URI for dynamic client registration', - async ( - supportedGrantTypes, - supportedTokenEndpointAuthMethods, - supportedCodeChallengeMethods, - expectedGrantTypes, - expectedTokenEndpointAuthMethod, - expectedQueryParams, - ) => { - jest.spyOn(Csrf.prototype, 'secretSync').mockReturnValueOnce(csrfSecret); - jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token'); - jest.spyOn(pkceChallenge, 'default').mockResolvedValueOnce({ - code_verifier: 'code-verifier', - code_challenge: 'code-challenge', - }); - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue({ - useDynamicClientRegistration: true, - serverUrl: 'https://example.com', - }); - nock('https://example.com') - .get('/.well-known/oauth-authorization-server') - .reply(200, { - authorization_endpoint: 'https://example.com/auth', - token_endpoint: 'https://example.com/token', - registration_endpoint: 'https://example.com/registration', - grant_types_supported: supportedGrantTypes, - token_endpoint_auth_methods_supported: supportedTokenEndpointAuthMethods, - code_challenge_methods_supported: supportedCodeChallengeMethods, - }) - .post('/registration', { - redirect_uris: ['http://localhost:5678/rest/oauth2-credential/callback'], - token_endpoint_auth_method: expectedTokenEndpointAuthMethod, - grant_types: expectedGrantTypes, - response_types: ['code'], - client_name: 'n8n', - client_uri: 'https://n8n.io/', - }) - .reply(200, { client_id: 'test-client-id', client_secret: 'test-client-secret' }); - - const req = mock({ user, query: { id: '1' } }); - const authUri = await controller.getAuthUri(req); - - const url = new URL(authUri); - expect(url.origin).toEqual('https://example.com'); - expect(url.pathname).toEqual('/auth'); - Object.entries(expectedQueryParams).forEach(([param, value]) => { - expect(url.searchParams.get(param)).toEqual(value); - }); - }, - ); - - it.each([ - [ - { - authorization_endpoint: 'invalid', - token_endpoint: 'https://example.com/token', - registration_endpoint: 'https://example.com/registration', - }, - ], - [ - { - authorization_endpoint: 'https://example.com/auth', - token_endpoint: 'invalid', - registration_endpoint: 'https://example.com/registration', - }, - ], - [ - { - authorization_endpoint: 'https://example.com/auth', - token_endpoint: 'https://example.com/token', - registration_endpoint: 'invalid', - }, - ], - ])( - 'should throw a BadRequestError when OAuth2 server metadata is invalid', - async (response) => { - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue({ - useDynamicClientRegistration: true, - serverUrl: 'https://example.com', - }); - nock('https://example.com') - .get('/.well-known/oauth-authorization-server') - .reply(200, response); - - const req = mock({ user, query: { id: '1' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - /Invalid OAuth2 server metadata/, - ); - }, - ); - - it('should throw a BadRequestError when the registration response is invalid', async () => { - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue({ - useDynamicClientRegistration: true, - serverUrl: 'https://example.com', + it('should return a valid auth URI with auth query parameters', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetUri = jest.fn().mockReturnValue({ + toString: () => + 'https://example.domain/oauth2/auth?client_id=client_id&redirect_uri=http://localhost:5678/rest/oauth2-credential/callback&response_type=code&state=state&scope=openid&access_type=offline', }); - nock('https://example.com') - .get('/.well-known/oauth-authorization-server') - .reply(200, { - authorization_endpoint: 'https://example.com/auth', - token_endpoint: 'https://example.com/token', - registration_endpoint: 'https://example.com/registration', - grant_types_supported: ['authorization_code', 'refresh_token'], - token_endpoint_auth_methods_supported: ['client_secret_basic'], - }) - .post('/registration', { - redirect_uris: ['http://localhost:5678/rest/oauth2-credential/callback'], - token_endpoint_auth_method: 'client_secret_basic', - grant_types: ['authorization_code', 'refresh_token'], - response_types: ['code'], - client_name: 'n8n', - client_uri: 'https://n8n.io/', - }) - .reply(200, { invalid: 'invalid' }); - - const req = mock({ user, query: { id: '1' } }); - await expect(controller.getAuthUri(req)).rejects.toThrowError( - /Invalid client registration response/, + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getUri: mockGetUri, + }, + }) as any, ); - }); - it('should request scopes from scopes_supported for dynamic client registration', async () => { - jest.spyOn(Csrf.prototype, 'secretSync').mockReturnValueOnce(csrfSecret); - jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token'); - jest.spyOn(pkceChallenge, 'default').mockResolvedValueOnce({ - code_verifier: 'code-verifier', - code_challenge: 'code-challenge', + const mockResolvedCredential = mock({ id: '1' }); + oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.createCsrfState.mockReturnValueOnce(['csrf-secret', 'state']); + oauthService.getOAuthCredentials.mockResolvedValueOnce({ + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + authQueryParameters: 'access_type=offline', + grantType: 'authorizationCode', + authentication: 'header', + }); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + user: mock({ id: '123' }), + query: { id: '1' }, }); - credentialsFinderService.findCredentialForUser.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue({ - useDynamicClientRegistration: true, - serverUrl: 'https://example.com', - }); - nock('https://example.com') - .get('/.well-known/oauth-authorization-server') - .reply(200, { - authorization_endpoint: 'https://example.com/auth', - token_endpoint: 'https://example.com/token', - registration_endpoint: 'https://example.com/registration', - grant_types_supported: ['authorization_code', 'refresh_token'], - token_endpoint_auth_methods_supported: ['client_secret_basic'], - code_challenge_methods_supported: ['S256'], - scopes_supported: ['openid', 'somescope'], - }) - .post('/registration', { - redirect_uris: ['http://localhost:5678/rest/oauth2-credential/callback'], - token_endpoint_auth_method: 'none', - grant_types: ['authorization_code', 'refresh_token'], - response_types: ['code'], - client_name: 'n8n', - client_uri: 'https://n8n.io/', - scope: 'openid somescope', - }) - .reply(200, { client_id: 'test-client-id', client_secret: 'test-client-secret' }); - const req = mock({ user, query: { id: '1' } }); const authUri = await controller.getAuthUri(req); - const url = new URL(authUri); - expect(url.origin).toEqual('https://example.com'); - expect(url.pathname).toEqual('/auth'); - Object.entries({ - code_challenge: 'code-challenge', - code_challenge_method: 'S256', - client_id: 'test-client-id', - redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', - response_type: 'code', - scope: 'openid somescope', - }).forEach(([param, value]) => { - expect(url.searchParams.get(param)).toEqual(value); - }); + expect(authUri).toContain('access_type=offline'); }); }); @@ -401,264 +176,280 @@ describe('OAuth2CredentialController', () => { JSON.stringify({ token: 'token', cid: '1', + userId: '123', createdAt: timestamp, }), ).toString('base64'); const res = mock(); - const req = mock({ - query: { code: 'code', state: validState }, - originalUrl: '?code=code', - }); it('should render the error page when required query params are missing', async () => { - const invalidReq = mock({ - query: { code: undefined, state: undefined }, - }); + const invalidReq = mock(); + invalidReq.query = { state: 'test' } as OAuthRequest.OAuth2Credential.Callback['query']; await controller.handleCallback(invalidReq, res); - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Insufficient parameters for OAuth2 callback.', - reason: 'Received following query parameters: undefined', - }, - }); - expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); - }); - - it('should render the error page when `state` query param is invalid', async () => { - const invalidReq = mock({ - query: { code: 'code', state: 'invalid-state' }, - }); - - await controller.handleCallback(invalidReq, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Invalid state format', - }, - }); - expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); - }); - - it('should render the error page when credential is not found in DB', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(null); - - await controller.handleCallback(req, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'OAuth callback failed because of insufficient permissions', - }, - }); - expect(credentialsRepository.findOneBy).toHaveBeenCalledTimes(1); - expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ id: '1' }); - }); - - it('should render the error page when csrfSecret on the saved credential does not match the state', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(false); - - await controller.handleCallback(req, res); - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'The OAuth callback state is invalid!', - }, - }); - expect(externalHooks.run).not.toHaveBeenCalled(); - }); - - it('should render the error page when state is older than 5 minutes', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - - jest.advanceTimersByTime(10 * Time.minutes.toMilliseconds); - - await controller.handleCallback(req, res); - - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'The OAuth callback state is invalid!', - }, - }); - expect(externalHooks.run).not.toHaveBeenCalled(); - }); - - it('should render the error page when code exchange fails', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post( - '/token', - 'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback', - ) - .reply(403, { error: 'Code could not be exchanged' }); - - await controller.handleCallback(req, res); - - expect(externalHooks.run).toHaveBeenCalled(); - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Code could not be exchanged', - reason: '{"error":"Code could not be exchanged"}', - }, - }); - }); - - it('should render the error page when code exchange fails, and the server responses with html', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post( - '/token', - 'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback', - ) - .reply(403, 'Code could not be exchanged', { - 'Content-Type': 'text/html', - }); - - await controller.handleCallback(req, res); - - expect(externalHooks.run).toHaveBeenCalled(); - expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { - error: { - message: 'Unsupported content type: text/html', - reason: '"Code could not be exchanged"', - }, - }); + expect(oauthService.renderCallbackError).toHaveBeenCalledWith( + res, + 'Insufficient parameters for OAuth2 callback.', + 'Received following query parameters: {"state":"test"}', + ); }); it('should exchange the code for a valid token, and save it to DB', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ csrfSecret }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post( - '/token', - 'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback', - ) - .reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' }); + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = jest.fn().mockResolvedValue({ + data: { access_token: 'new_token', refresh_token: 'refresh_token' }, + }); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getToken: mockGetToken, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'header', + }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); await controller.handleCallback(req, res); - expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', [ + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, expect.objectContaining({ - clientId: 'test-client-id', - redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', - }), - ]); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', - expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth2Api', - }), - ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ - oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, + oauthTokenData: { access_token: 'new_token', refresh_token: 'refresh_token' }, }), + ['csrfSecret'], ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); - expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( - additionalData, - credential, - credential.type, - 'internal', + expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', expect.any(Array)); + }); + + it('should handle PKCE flow', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = jest.fn().mockResolvedValue({ + data: { access_token: 'new_token' }, + }); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getToken: mockGetToken, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret', codeVerifier: 'code_verifier' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'pkce', + authentication: 'header', + }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(mockGetToken).toHaveBeenCalledWith( + expect.stringContaining('code=auth_code'), + expect.objectContaining({ + body: { code_verifier: 'code_verifier' }, + }), + ); + expect(oauthService.encryptAndSaveData).toHaveBeenCalled(); + }); + + it('should handle body authentication method', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = jest.fn().mockResolvedValue({ + data: { access_token: 'new_token' }, + }); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getToken: mockGetToken, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'body', + }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(mockGetToken).toHaveBeenCalledWith( + expect.stringContaining('code=auth_code'), + expect.objectContaining({ + body: expect.objectContaining({ + client_id: 'client_id', + client_secret: 'client_secret', + }), + }), + ); + }); + + it('should handle callback with additional query parameters', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = jest.fn().mockResolvedValue({ + data: { access_token: 'new_token' }, + }); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getToken: mockGetToken, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'header', + }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + extra: 'param', + } as OAuthRequest.OAuth2Credential.Callback['query'] & { extra: string }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state&extra=param', + }); + + await controller.handleCallback(req, res); + + expect(mockGetToken).toHaveBeenCalled(); + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ + oauthTokenData: expect.objectContaining({ + access_token: 'new_token', + callbackQueryString: expect.objectContaining({ + extra: 'param', + }), + }), + }), + ['csrfSecret'], + ); + }); + + it('should handle errors and render error page', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = jest.fn().mockRejectedValue(new Error('Token exchange failed')); + jest.mocked(ClientOAuth2).mockImplementation( + () => + ({ + code: { + getToken: mockGetToken, + }, + }) as any, + ); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'header', + }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(oauthService.renderCallbackError).toHaveBeenCalledWith( + res, + 'Token exchange failed', undefined, - true, ); }); - - it('merges oauthTokenData if it already exists', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ - csrfSecret, - oauthTokenData: { token: true }, - }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post( - '/token', - 'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback', - ) - .reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' }); - - await controller.handleCallback(req, res); - - expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', [ - expect.objectContaining({ - clientId: 'test-client-id', - redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', - }), - ]); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', - expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth2Api', - }), - ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ - oauthTokenData: { - token: true, - access_token: 'access-token', - refresh_token: 'refresh-token', - }, - }), - ); - expect(res.render).toHaveBeenCalledWith('oauth-callback'); - }); - - it('overwrites oauthTokenData if it is a string', async () => { - credentialsRepository.findOneBy.mockResolvedValueOnce(credential); - credentialsHelper.getDecrypted.mockResolvedValueOnce({ - csrfSecret, - oauthTokenData: CREDENTIAL_BLANKING_VALUE, - }); - jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true); - nock('https://example.domain') - .post( - '/token', - 'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback', - ) - .reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' }); - - await controller.handleCallback(req, res); - - expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', [ - expect.objectContaining({ - clientId: 'test-client-id', - redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', - }), - ]); - const dataCaptor = captor(); - expect(credentialsRepository.update).toHaveBeenCalledWith( - '1', - expect.objectContaining({ - data: dataCaptor, - id: '1', - name: 'Test Credential', - type: 'oAuth2Api', - }), - ); - expect(cipher.decrypt(dataCaptor.value)).toEqual( - JSON.stringify({ - oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, - }), - ); - expect(res.render).toHaveBeenCalledWith('oauth-callback'); - }); }); }); diff --git a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts index 9809e86d938..d762203693a 100644 --- a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts @@ -9,7 +9,9 @@ import clientOAuth1 from 'oauth-1.0a'; import { OAuthRequest } from '@/requests'; -import { AbstractOAuthController, skipAuthOnOAuthCallback } from './abstract-oauth.controller'; +import { OauthService, OauthVersion, skipAuthOnOAuthCallback } from '@/oauth/oauth.service'; +import { Logger } from '@n8n/backend-common'; +import { ExternalHooks } from '@/external-hooks'; interface OAuth1CredentialData { signatureMethod: 'HMAC-SHA256' | 'HMAC-SHA512' | 'HMAC-SHA1'; @@ -27,24 +29,24 @@ const algorithmMap = { } as const; @RestController('/oauth1-credential') -export class OAuth1CredentialController extends AbstractOAuthController { - override oauthVersion = 1; +export class OAuth1CredentialController { + constructor( + private readonly oauthService: OauthService, + private readonly externalHooks: ExternalHooks, + private readonly logger: Logger, + ) {} /** Get Authorization url */ @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth1Credential.Auth): Promise { - const credential = await this.getCredential(req); - const additionalData = await this.getAdditionalData(); - const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData); - const oauthCredentials = await this.applyDefaultsAndOverwrites( - credential, - decryptedDataOriginal, - additionalData, - ); - const [csrfSecret, state] = this.createCsrfState( - credential.id, - skipAuthOnOAuthCallback ? undefined : req.user.id, - ); + const credential = await this.oauthService.getCredential(req); + const oauthCredentials = + await this.oauthService.getOAuthCredentials(credential); + + const [csrfSecret, state] = this.oauthService.createCsrfState({ + cid: credential.id, + userId: skipAuthOnOAuthCallback ? undefined : req.user.id, + }); const signatureMethod = oauthCredentials.signatureMethod; @@ -62,7 +64,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { }; const oauthRequestData = { - oauth_callback: `${this.baseUrl}/callback?state=${state}`, + oauth_callback: `${this.oauthService.getBaseUrl(OauthVersion.V1)}/callback?state=${state}`, }; await this.externalHooks.run('oauth1.authenticate', [oAuthOptions, oauthRequestData]); @@ -91,7 +93,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { const returnUri = `${oauthCredentials.authUrl}?oauth_token=${responseJson.oauth_token}`; - await this.encryptAndSaveData(credential, { csrfSecret }); + await this.oauthService.encryptAndSaveData(credential, { csrfSecret }); this.logger.debug('OAuth1 authorization successful for new credential', { userId: req.user.id, @@ -108,7 +110,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { const { oauth_verifier, oauth_token, state: encodedState } = req.query; if (!oauth_verifier || !oauth_token || !encodedState) { - return this.renderCallbackError( + return this.oauthService.renderCallbackError( res, 'Insufficient parameters for OAuth1 callback.', `Received following query parameters: ${JSON.stringify(req.query)}`, @@ -116,7 +118,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { } const [credential, _, oauthCredentials] = - await this.resolveCredential(req); + await this.oauthService.resolveCredential(req); // Form URL encoded body https://datatracker.ietf.org/doc/html/rfc5849#section-3.5.2 const oauthToken = await axios.post( @@ -131,15 +133,18 @@ export class OAuth1CredentialController extends AbstractOAuthController { const oauthTokenData = Object.fromEntries(paramParser.entries()); - await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); + console.log('oauthTokenData', oauthTokenData); + + await this.oauthService.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); this.logger.debug('OAuth1 callback successful for new credential', { credentialId: credential.id, }); return res.render('oauth-callback'); } catch (e) { + console.log('error', e); const error = ensureError(e); - return this.renderCallbackError( + return this.oauthService.renderCallbackError( res, error.message, 'body' in error ? jsonStringify(error.body) : undefined, diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index 7e1f6c8bc86..3f708efa27e 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -20,44 +20,31 @@ import { import pkceChallenge from 'pkce-challenge'; import * as qs from 'querystring'; -import { AbstractOAuthController, skipAuthOnOAuthCallback } from './abstract-oauth.controller'; import { oAuthAuthorizationServerMetadataSchema, dynamicClientRegistrationResponseSchema, } from './oauth2-dynamic-client-registration.schema'; -import { GENERIC_OAUTH2_CREDENTIALS_WITH_EDITABLE_SCOPE as GENERIC_OAUTH2_CREDENTIALS_WITH_EDITABLE_SCOPE } from '@/constants'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { OAuthRequest } from '@/requests'; +import { OauthService, OauthVersion, skipAuthOnOAuthCallback } from '@/oauth/oauth.service'; +import { Logger } from '@n8n/backend-common'; +import { ExternalHooks } from '@/external-hooks'; @RestController('/oauth2-credential') -export class OAuth2CredentialController extends AbstractOAuthController { - override oauthVersion = 2; +export class OAuth2CredentialController { + constructor( + private readonly oauthService: OauthService, + private readonly logger: Logger, + private readonly externalHooks: ExternalHooks, + ) {} /** Get Authorization url */ @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth2Credential.Auth): Promise { - const credential = await this.getCredential(req); - const additionalData = await this.getAdditionalData(); - const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData); - - // At some point in the past we saved hidden scopes to credentials (but shouldn't) - // Delete scope before applying defaults to make sure new scopes are present on reconnect - // Generic Oauth2 API is an exception because it needs to save the scope - - if ( - decryptedDataOriginal?.scope && - credential.type.includes('OAuth2') && - !GENERIC_OAUTH2_CREDENTIALS_WITH_EDITABLE_SCOPE.includes(credential.type) - ) { - delete decryptedDataOriginal.scope; - } - - const oauthCredentials = await this.applyDefaultsAndOverwrites( - credential, - decryptedDataOriginal, - additionalData, - ); + const credential = await this.oauthService.getCredential(req); + const oauthCredentials: OAuth2CredentialData = + await this.oauthService.getOAuthCredentials(credential); const toUpdate: ICredentialDataDecryptedObject = {}; @@ -102,7 +89,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { authentication, ); const registerPayload = { - redirect_uris: [`${this.baseUrl}/callback`], + redirect_uris: [`${this.oauthService.getBaseUrl(OauthVersion.V2)}/callback`], token_endpoint_auth_method, grant_types, response_types: ['code'], @@ -135,10 +122,10 @@ export class OAuth2CredentialController extends AbstractOAuthController { } // Generate a CSRF prevention token and send it as an OAuth2 state string - const [csrfSecret, state] = this.createCsrfState( - credential.id, - skipAuthOnOAuthCallback ? undefined : req.user.id, - ); + const [csrfSecret, state] = this.oauthService.createCsrfState({ + cid: credential.id, + userId: skipAuthOnOAuthCallback ? undefined : req.user.id, + }); const oAuthOptions = { ...this.convertCredentialToOptions(oauthCredentials), @@ -162,7 +149,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { toUpdate.codeVerifier = code_verifier; } - await this.encryptAndSaveData(credential, toUpdate); + await this.oauthService.encryptAndSaveData(credential, toUpdate); const oAuthObj = new ClientOAuth2(oAuthOptions); const returnUri = oAuthObj.code.getUri(); @@ -181,7 +168,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { try { const { code, state: encodedState } = req.query; if (!code || !encodedState) { - return this.renderCallbackError( + return this.oauthService.renderCallbackError( res, 'Insufficient parameters for OAuth2 callback.', `Received following query parameters: ${JSON.stringify(req.query)}`, @@ -189,7 +176,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { } const [credential, decryptedDataOriginal, oauthCredentials] = - await this.resolveCredential(req); + await this.oauthService.resolveCredential(req); let options: Partial = {}; @@ -233,7 +220,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { ...oauthToken.data, }; - await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); + await this.oauthService.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); this.logger.debug('OAuth2 callback successful for credential', { credentialId: credential.id, @@ -242,7 +229,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { return res.render('oauth-callback'); } catch (e) { const error = ensureError(e); - return this.renderCallbackError( + return this.oauthService.renderCallbackError( res, error.message, 'body' in error ? jsonStringify(error.body) : undefined, @@ -257,7 +244,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { accessTokenUri: credential.accessTokenUrl ?? '', authorizationUri: credential.authUrl ?? '', authentication: credential.authentication ?? 'header', - redirectUri: `${this.baseUrl}/callback`, + redirectUri: `${this.oauthService.getBaseUrl(OauthVersion.V2)}/callback`, scopes: split(credential.scope ?? 'openid', ','), scopesSeparator: credential.scope?.includes(',') ? ',' : ' ', ignoreSSLIssues: credential.ignoreSSLIssues ?? false, diff --git a/packages/cli/src/oauth/__tests__/oauth.service.test.ts b/packages/cli/src/oauth/__tests__/oauth.service.test.ts new file mode 100644 index 00000000000..5539ca89a68 --- /dev/null +++ b/packages/cli/src/oauth/__tests__/oauth.service.test.ts @@ -0,0 +1,693 @@ +import { Logger } from '@n8n/backend-common'; +import { GlobalConfig } from '@n8n/config'; +import { Time } from '@n8n/constants'; +import { mockInstance } from '@n8n/backend-test-utils'; +import type { AuthenticatedRequest, CredentialsEntity, ICredentialsDb, User } from '@n8n/db'; +import { CredentialsRepository } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { mock } from 'jest-mock-extended'; +import type { Response } from 'express'; +import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; +import { UnexpectedError } from 'n8n-workflow'; + +import { OauthService, OauthVersion, shouldSkipAuthOnOAuthCallback } from '@/oauth/oauth.service'; +import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; +import { CredentialsHelper } from '@/credentials-helper'; +import { AuthError } from '@/errors/response-errors/auth.error'; +import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import type { OAuthRequest } from '@/requests'; +import { UrlService } from '@/services/url.service'; +import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; + +jest.mock('@/workflow-execute-additional-data'); + +describe('OauthService', () => { + const logger = mockInstance(Logger); + const credentialsHelper = mockInstance(CredentialsHelper); + const credentialsRepository = mockInstance(CredentialsRepository); + const credentialsFinderService = mockInstance(CredentialsFinderService); + const urlService = mockInstance(UrlService); + const globalConfig = mockInstance(GlobalConfig); + + let service: OauthService; + + const timestamp = 1706750625678; + jest.useFakeTimers({ advanceTimers: true }); + + beforeEach(() => { + jest.setSystemTime(new Date(timestamp)); + jest.clearAllMocks(); + + globalConfig.endpoints = { rest: 'rest' } as any; + urlService.getInstanceBaseUrl.mockReturnValue('http://localhost:5678'); + jest + .mocked(WorkflowExecuteAdditionalData.getBase) + .mockResolvedValue(mock()); + + service = new OauthService( + logger, + credentialsHelper, + credentialsRepository, + credentialsFinderService, + urlService, + globalConfig, + ); + }); + + describe('shouldSkipAuthOnOAuthCallback', () => { + it('should return false when env var is not set', () => { + delete process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK; + expect(shouldSkipAuthOnOAuthCallback()).toBe(false); + }); + + it('should return false when env var is "false"', () => { + process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK = 'false'; + expect(shouldSkipAuthOnOAuthCallback()).toBe(false); + }); + + it('should return true when env var is "true"', () => { + process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK = 'true'; + expect(shouldSkipAuthOnOAuthCallback()).toBe(true); + }); + + it('should return true when env var is "TRUE" (case insensitive)', () => { + process.env.N8N_SKIP_AUTH_ON_OAUTH_CALLBACK = 'TRUE'; + expect(shouldSkipAuthOnOAuthCallback()).toBe(true); + }); + }); + + describe('getBaseUrl', () => { + it('should return correct URL for OAuth1', () => { + const url = service.getBaseUrl(OauthVersion.V1); + expect(url).toBe('http://localhost:5678/rest/oauth1-credential'); + expect(urlService.getInstanceBaseUrl).toHaveBeenCalled(); + }); + + it('should return correct URL for OAuth2', () => { + const url = service.getBaseUrl(OauthVersion.V2); + expect(url).toBe('http://localhost:5678/rest/oauth2-credential'); + }); + }); + + describe('getCredential', () => { + it('should throw BadRequestError when credential ID is missing', async () => { + const req = { + query: {}, + user: mock({ id: '123' }), + } as unknown as OAuthRequest.OAuth2Credential.Auth; + + Object.defineProperty(req.query, 'id', { + value: undefined, + writable: true, + enumerable: true, + }); + + const promise = service.getCredential(req); + await expect(promise).rejects.toThrow(BadRequestError); + await expect(promise).rejects.toThrow('Required credential ID is missing'); + }); + + it('should throw NotFoundError when credential is not found', async () => { + const req = mock({ + query: { id: 'credential-id' }, + user: mock({ id: '123' }), + }); + + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + + await expect(service.getCredential(req)).rejects.toThrow(NotFoundError); + expect(logger.error).toHaveBeenCalledWith( + 'OAuth credential authorization failed because the current user does not have the correct permissions', + { userId: '123' }, + ); + }); + + it('should return credential when found', async () => { + const mockCredential = mock({ id: 'credential-id' }); + const req = mock({ + query: { id: 'credential-id' }, + user: mock({ id: '123' }), + }); + + credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); + + const result = await service.getCredential(req); + + expect(result).toBe(mockCredential); + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + 'credential-id', + req.user, + ['credential:read'], + ); + }); + }); + + describe('getAdditionalData', () => { + it('should return workflow execute additional data', async () => { + const mockAdditionalData = mock(); + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + + const result = await (service as any).getAdditionalData(); + + expect(result).toBe(mockAdditionalData); + expect(WorkflowExecuteAdditionalData.getBase).toHaveBeenCalled(); + }); + }); + + describe('getDecryptedDataForAuthUri', () => { + it('should call getDecryptedData with raw=false', async () => { + const credential = mock({ id: '1', type: 'test' }); + const additionalData = mock(); + const mockDecryptedData = { clientId: 'test' }; + + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + + const result = await (service as any).getDecryptedDataForAuthUri(credential, additionalData); + + expect(result).toBe(mockDecryptedData); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + false, + ); + }); + }); + + describe('getDecryptedDataForCallback', () => { + it('should call getDecryptedData with raw=true', async () => { + const credential = mock({ id: '1', type: 'test' }); + const additionalData = mock(); + const mockDecryptedData = { csrfSecret: 'secret' }; + + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + + const result = await (service as any).getDecryptedDataForCallback(credential, additionalData); + + expect(result).toBe(mockDecryptedData); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + true, + ); + }); + }); + + describe('applyDefaultsAndOverwrites', () => { + it('should apply defaults and overwrites', async () => { + const credential = mock({ id: '1', type: 'test' }); + const decryptedData = { clientId: 'test' }; + const additionalData = mock(); + const mockResult = { clientId: 'test', clientSecret: 'secret' }; + + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockResult); + + const result = await (service as any).applyDefaultsAndOverwrites( + credential, + decryptedData, + additionalData, + ); + + expect(result).toBe(mockResult); + expect(credentialsHelper.applyDefaultsAndOverwrites).toHaveBeenCalledWith( + additionalData, + decryptedData, + credential, + credential.type, + 'internal', + undefined, + undefined, + ); + }); + }); + + describe('encryptAndSaveData', () => { + it('should encrypt and save data to repository', async () => { + const { Cipher } = await import('n8n-core'); + const cipher = Container.get(Cipher); + const encryptedData = cipher.encrypt({ existing: 'data' }); + + const credential = mock({ + id: '1', + type: 'test', + data: encryptedData, + }); + const toUpdate = { clientId: 'new-id' }; + const toDelete = ['oldField']; + + await service.encryptAndSaveData(credential, toUpdate, toDelete); + + expect(credentialsRepository.update).toHaveBeenCalledWith('1', { + id: '1', + name: expect.anything(), + type: 'test', + data: expect.any(String), + updatedAt: expect.any(Date), + }); + }); + + it('should use empty array for toDelete when not provided', async () => { + const { Cipher } = await import('n8n-core'); + const cipher = Container.get(Cipher); + const encryptedData = cipher.encrypt({ existing: 'data' }); + + const credential = mock({ + id: '1', + type: 'test', + data: encryptedData, + }); + const toUpdate = { clientId: 'new-id' }; + + await service.encryptAndSaveData(credential, toUpdate); + + expect(credentialsRepository.update).toHaveBeenCalledWith('1', { + id: '1', + name: expect.anything(), + type: 'test', + data: expect.any(String), + updatedAt: expect.any(Date), + }); + }); + }); + + describe('getCredentialWithoutUser', () => { + it('should return credential from repository', async () => { + const mockCredential = mock({ id: '1' }); + credentialsRepository.findOneBy.mockResolvedValue(mockCredential as any); + + const result = await (service as any).getCredentialWithoutUser('1'); + + expect(result).toBe(mockCredential); + expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ id: '1' }); + }); + + it('should return null when credential not found', async () => { + credentialsRepository.findOneBy.mockResolvedValue(null); + + const result = await (service as any).getCredentialWithoutUser('1'); + + expect(result).toBeNull(); + }); + }); + + describe('createCsrfState', () => { + it('should create CSRF state with correct structure', () => { + const data = { cid: 'credential-id', userId: 'user-id' }; + jest.setSystemTime(new Date(timestamp)); + + const [csrfSecret, encodedState] = service.createCsrfState(data); + + expect(typeof csrfSecret).toBe('string'); + expect(csrfSecret.length).toBeGreaterThan(0); + + const decoded = JSON.parse(Buffer.from(encodedState, 'base64').toString()); + expect(decoded.cid).toBe('credential-id'); + expect(decoded.userId).toBe('user-id'); + expect(decoded.token).toBeDefined(); + expect(decoded.createdAt).toBe(timestamp); + }); + + it('should include additional data in state', () => { + const data = { cid: 'credential-id', customField: 'custom-value' }; + jest.setSystemTime(new Date(timestamp)); + + const [, encodedState] = service.createCsrfState(data); + + const decoded = JSON.parse(Buffer.from(encodedState, 'base64').toString()); + expect(decoded.customField).toBe('custom-value'); + }); + }); + + describe('decodeCsrfState', () => { + it('should decode valid CSRF state', () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'user-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + const result = (service as any).decodeCsrfState(encodedState, req); + + expect(result).toEqual(state); + }); + + it('should throw error when state format is invalid', () => { + const invalidState = 'not-base64-json'; + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + expect(() => (service as any).decodeCsrfState(invalidState, req)).toThrow( + 'Invalid state format', + ); + }); + + it('should throw UnexpectedError when cid is missing', () => { + const state = { + token: 'token', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + expect(() => (service as any).decodeCsrfState(encodedState, req)).toThrow(UnexpectedError); + }); + + it('should throw UnexpectedError when token is missing', () => { + const state = { + cid: 'credential-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + expect(() => (service as any).decodeCsrfState(encodedState, req)).toThrow(UnexpectedError); + }); + + it('should throw AuthError when userId does not match', () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'different-user-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + expect(() => (service as any).decodeCsrfState(encodedState, req)).toThrow(AuthError); + expect(() => (service as any).decodeCsrfState(encodedState, req)).toThrow('Unauthorized'); + }); + + it('should throw AuthError when req.user is undefined', () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'user-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + const req = mock({ + user: undefined, + }); + + expect(() => (service as any).decodeCsrfState(encodedState, req)).toThrow(AuthError); + }); + }); + + describe('verifyCsrfState', () => { + it('should return true for valid CSRF state', () => { + const csrfSecret = 'csrf-secret'; + const token = new (require('csrf'))(); + const stateToken = token.create(csrfSecret); + + const state = { + token: stateToken, + cid: 'credential-id', + createdAt: Date.now(), + }; + const decrypted = { csrfSecret }; + + const result = (service as any).verifyCsrfState(decrypted, state); + + expect(result).toBe(true); + }); + + it('should return false when CSRF state is expired', () => { + const csrfSecret = 'csrf-secret'; + const token = new (require('csrf'))(); + const stateToken = token.create(csrfSecret); + + const expiredTime = Date.now() - 6 * Time.minutes.toMilliseconds; + const state = { + token: stateToken, + cid: 'credential-id', + createdAt: expiredTime, + }; + const decrypted = { csrfSecret }; + + const result = (service as any).verifyCsrfState(decrypted, state); + + expect(result).toBe(false); + }); + + it('should return false when csrfSecret is missing', () => { + const token = new (require('csrf'))(); + const csrfSecret = 'csrf-secret'; + const stateToken = token.create(csrfSecret); + + const state = { + token: stateToken, + cid: 'credential-id', + createdAt: Date.now(), + }; + const decrypted = {}; + + const result = (service as any).verifyCsrfState(decrypted, state); + + expect(result).toBe(false); + }); + + it('should return false when token verification fails', () => { + const state = { + token: 'invalid-token', + cid: 'credential-id', + createdAt: Date.now(), + }; + const decrypted = { csrfSecret: 'csrf-secret' }; + + const result = (service as any).verifyCsrfState(decrypted, state); + + expect(result).toBe(false); + }); + }); + + describe('resolveCredential', () => { + it('should resolve credential successfully', async () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'user-id', + createdAt: timestamp, + }; + + const mockCredential = mock({ id: 'credential-id' }); + const mockDecryptedData = { csrfSecret: 'csrf-secret' }; + const mockOAuthCredentials = { clientId: 'client-id' }; + const mockAdditionalData = mock(); + + const token = new (require('csrf'))(); + const stateToken = token.create('csrf-secret'); + state.token = stateToken; + + const req = mock({ + query: { state: Buffer.from(JSON.stringify(state)).toString('base64') }, + user: mock({ id: 'user-id' }), + }); + + credentialsRepository.findOneBy.mockResolvedValue(mockCredential); + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + jest.spyOn(service as any, 'verifyCsrfState').mockReturnValue(true); + + const result = await service.resolveCredential(req); + + expect(result).toEqual([mockCredential, mockDecryptedData, mockOAuthCredentials]); + }); + + it('should throw UnexpectedError when credential is not found', async () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'user-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + + const req = mock({ + query: { state: encodedState }, + user: mock({ id: 'user-id' }), + }); + + credentialsRepository.findOneBy.mockResolvedValue(null); + + await expect(service.resolveCredential(req)).rejects.toThrow(UnexpectedError); + await expect(service.resolveCredential(req)).rejects.toThrow( + 'OAuth callback failed because of insufficient permissions', + ); + }); + + it('should throw UnexpectedError when CSRF state is invalid', async () => { + const state = { + token: 'token', + cid: 'credential-id', + userId: 'user-id', + createdAt: timestamp, + }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + + const mockCredential = mock({ id: 'credential-id' }); + const mockDecryptedData = { csrfSecret: 'csrf-secret' }; + const mockOAuthCredentials = { clientId: 'client-id' }; + const mockAdditionalData = mock(); + + const req = mock({ + query: { state: encodedState }, + user: mock({ id: 'user-id' }), + }); + + credentialsRepository.findOneBy.mockResolvedValue(mockCredential); + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + jest.spyOn(service as any, 'verifyCsrfState').mockReturnValue(false); + + await expect(service.resolveCredential(req)).rejects.toThrow(UnexpectedError); + await expect(service.resolveCredential(req)).rejects.toThrow( + 'The OAuth callback state is invalid!', + ); + }); + }); + + describe('renderCallbackError', () => { + it('should render error page with message', () => { + const res = mock(); + const message = 'Test error message'; + + service.renderCallbackError(res, message); + + expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { + error: { message }, + }); + }); + + it('should render error page with message and reason', () => { + const res = mock(); + const message = 'Test error message'; + const reason = 'Test reason'; + + service.renderCallbackError(res, message, reason); + + expect(res.render).toHaveBeenCalledWith('oauth-error-callback', { + error: { message, reason }, + }); + }); + }); + + describe('getOAuthCredentials', () => { + it('should return OAuth credentials', async () => { + const credential = mock({ + id: '1', + type: 'testOAuth2Api', + }); + const mockDecryptedData = { clientId: 'client-id' }; + const mockOAuthCredentials = { clientId: 'client-id', clientSecret: 'secret' }; + const mockAdditionalData = mock(); + + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + const result = await service.getOAuthCredentials(credential); + + expect(result).toBe(mockOAuthCredentials); + }); + + it('should delete scope for non-generic OAuth2 credentials', async () => { + const credential = mock({ + id: '1', + type: 'testOAuth2Api', + }); + const mockDecryptedData = { clientId: 'client-id', scope: 'old-scope' }; + const mockOAuthCredentials = { clientId: 'client-id' }; + const mockAdditionalData = mock(); + + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + await service.getOAuthCredentials(credential); + + expect(credentialsHelper.applyDefaultsAndOverwrites).toHaveBeenCalledWith( + mockAdditionalData, + { clientId: 'client-id' }, + credential, + credential.type, + 'internal', + undefined, + undefined, + ); + }); + + it('should not delete scope for generic OAuth2 credentials with editable scope', async () => { + const credential = mock({ + id: '1', + type: 'oAuth2Api', + }); + const mockDecryptedData = { clientId: 'client-id', scope: 'old-scope' }; + const mockOAuthCredentials = { clientId: 'client-id', scope: 'old-scope' }; + const mockAdditionalData = mock(); + + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + await service.getOAuthCredentials(credential); + + expect(credentialsHelper.applyDefaultsAndOverwrites).toHaveBeenCalledWith( + mockAdditionalData, + { clientId: 'client-id', scope: 'old-scope' }, + credential, + credential.type, + 'internal', + undefined, + undefined, + ); + }); + + it('should not delete scope for non-OAuth2 credentials', async () => { + const credential = mock({ + id: '1', + type: 'testApi', + }); + const mockDecryptedData = { clientId: 'client-id', scope: 'old-scope' }; + const mockOAuthCredentials = { clientId: 'client-id', scope: 'old-scope' }; + const mockAdditionalData = mock(); + + jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); + credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); + credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); + + await service.getOAuthCredentials(credential); + + expect(credentialsHelper.applyDefaultsAndOverwrites).toHaveBeenCalledWith( + mockAdditionalData, + { clientId: 'client-id', scope: 'old-scope' }, + credential, + credential.type, + 'internal', + undefined, + undefined, + ); + }); + }); +}); diff --git a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts b/packages/cli/src/oauth/oauth.service.ts similarity index 77% rename from packages/cli/src/controllers/oauth/abstract-oauth.controller.ts rename to packages/cli/src/oauth/oauth.service.ts index 8ab590b9eff..2874ff82ed3 100644 --- a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts +++ b/packages/cli/src/oauth/oauth.service.ts @@ -10,28 +10,33 @@ import { Credentials } from 'n8n-core'; import type { ICredentialDataDecryptedObject, IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import { jsonParse, UnexpectedError } from 'n8n-workflow'; -import { RESPONSE_ERROR_MESSAGES } from '@/constants'; +import { + GENERIC_OAUTH2_CREDENTIALS_WITH_EDITABLE_SCOPE, + RESPONSE_ERROR_MESSAGES, +} from '@/constants'; import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { CredentialsHelper } from '@/credentials-helper'; import { AuthError } from '@/errors/response-errors/auth.error'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; -import { ExternalHooks } from '@/external-hooks'; import type { OAuthRequest } from '@/requests'; import { UrlService } from '@/services/url.service'; import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; -type CsrfStateParam = { - /** Id of the oAuth credential in the DB */ - cid: string; +type CsrfStateRequired = { /** Random CSRF token, used to verify the signature of the CSRF state */ token: string; /** Creation timestamp of the CSRF state. Used for expiration. */ createdAt: number; - /** User who initiated OAuth flow, included to prevent cross-user credential hijacking. Optional only if `skipAuthOnOAuthCallback` is enabled. */ - userId?: string; }; +type CreateCsrfStateData = { + cid: string; + [key: string]: unknown; +}; + +type CsrfState = CsrfStateRequired & CreateCsrfStateData; + const MAX_CSRF_AGE = 5 * Time.minutes.toMilliseconds; export function shouldSkipAuthOnOAuthCallback() { @@ -41,13 +46,15 @@ export function shouldSkipAuthOnOAuthCallback() { export const skipAuthOnOAuthCallback = shouldSkipAuthOnOAuthCallback(); -@Service() -export abstract class AbstractOAuthController { - abstract oauthVersion: number; +export const enum OauthVersion { + V1 = 1, + V2 = 2, +} +@Service() +export class OauthService { constructor( protected readonly logger: Logger, - protected readonly externalHooks: ExternalHooks, private readonly credentialsHelper: CredentialsHelper, private readonly credentialsRepository: CredentialsRepository, private readonly credentialsFinderService: CredentialsFinderService, @@ -55,14 +62,12 @@ export abstract class AbstractOAuthController { private readonly globalConfig: GlobalConfig, ) {} - get baseUrl() { + getBaseUrl(oauthVersion: OauthVersion) { const restUrl = `${this.urlService.getInstanceBaseUrl()}/${this.globalConfig.endpoints.rest}`; - return `${restUrl}/oauth${this.oauthVersion}-credential`; + return `${restUrl}/oauth${oauthVersion}-credential`; } - protected async getCredential( - req: OAuthRequest.OAuth2Credential.Auth, - ): Promise { + async getCredential(req: OAuthRequest.OAuth2Credential.Auth): Promise { const { id: credentialId } = req.query; if (!credentialId) { @@ -77,7 +82,7 @@ export abstract class AbstractOAuthController { if (!credential) { this.logger.error( - `OAuth${this.oauthVersion} credential authorization failed because the current user does not have the correct permissions`, + 'OAuth credential authorization failed because the current user does not have the correct permissions', { userId: req.user.id }, ); throw new NotFoundError(RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL); @@ -141,7 +146,7 @@ export abstract class AbstractOAuthController { )) as unknown as T; } - protected async encryptAndSaveData( + async encryptAndSaveData( credential: ICredentialsDb, toUpdate: ICredentialDataDecryptedObject, toDelete: string[] = [], @@ -159,21 +164,20 @@ export abstract class AbstractOAuthController { return await this.credentialsRepository.findOneBy({ id: credentialId }); } - createCsrfState(credentialsId: string, userId?: string): [string, string] { + createCsrfState(data: CreateCsrfStateData): [string, string] { const token = new Csrf(); const csrfSecret = token.secretSync(); - const state: CsrfStateParam = { + const state: CsrfState = { token: token.create(csrfSecret), - cid: credentialsId, createdAt: Date.now(), - userId, + ...data, }; return [csrfSecret, Buffer.from(JSON.stringify(state)).toString('base64')]; } - protected decodeCsrfState(encodedState: string, req: AuthenticatedRequest): CsrfStateParam { + protected decodeCsrfState(encodedState: string, req: AuthenticatedRequest): CsrfState { const errorMessage = 'Invalid state format'; - const decoded = jsonParse(Buffer.from(encodedState, 'base64').toString(), { + const decoded = jsonParse(Buffer.from(encodedState, 'base64').toString(), { errorMessage, }); @@ -190,7 +194,7 @@ export abstract class AbstractOAuthController { protected verifyCsrfState( decrypted: ICredentialDataDecryptedObject & { csrfSecret?: string }, - state: CsrfStateParam, + state: CsrfState, ) { const token = new Csrf(); @@ -201,7 +205,7 @@ export abstract class AbstractOAuthController { ); } - protected async resolveCredential( + async resolveCredential( req: OAuthRequest.OAuth1Credential.Callback | OAuthRequest.OAuth2Credential.Callback, ): Promise<[ICredentialsDb, ICredentialDataDecryptedObject, T]> { const { state: encodedState } = req.query; @@ -230,7 +234,31 @@ export abstract class AbstractOAuthController { return [credential, decryptedDataOriginal, oauthCredentials]; } - protected renderCallbackError(res: Response, message: string, reason?: string) { + renderCallbackError(res: Response, message: string, reason?: string) { res.render('oauth-error-callback', { error: { message, reason } }); } + + async getOAuthCredentials(credential: CredentialsEntity): Promise { + const additionalData = await this.getAdditionalData(); + const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData); + + // At some point in the past we saved hidden scopes to credentials (but shouldn't) + // Delete scope before applying defaults to make sure new scopes are present on reconnect + // Generic Oauth2 API is an exception because it needs to save the scope + if ( + decryptedDataOriginal?.scope && + credential.type.includes('OAuth2') && + !GENERIC_OAUTH2_CREDENTIALS_WITH_EDITABLE_SCOPE.includes(credential.type) + ) { + delete decryptedDataOriginal.scope; + } + + const oauthCredentials = await this.applyDefaultsAndOverwrites( + credential, + decryptedDataOriginal, + additionalData, + ); + + return oauthCredentials; + } } diff --git a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts index e098d7c95b9..18bbfc373ef 100644 --- a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts +++ b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts @@ -5,8 +5,8 @@ import { response as Response } from 'express'; import nock from 'nock'; import { parse as parseQs } from 'querystring'; -import { OAuth2CredentialController } from '@/controllers/oauth/oauth2-credential.controller'; import { CredentialsHelper } from '@/credentials-helper'; +import { OauthService } from '@/oauth/oauth.service'; import { saveCredential } from '@test-integration/db/credentials'; import { createMember, createOwner } from '@test-integration/db/users'; import type { SuperAgentTest } from '@test-integration/types'; @@ -52,8 +52,8 @@ describe('OAuth2 API', () => { }); it('should return a valid auth URL when the auth flow is initiated', async () => { - const controller = Container.get(OAuth2CredentialController); - const csrfSpy = jest.spyOn(controller, 'createCsrfState').mockClear(); + const oauthService = Container.get(OauthService); + const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); const response = await ownerAgent .get('/oauth2-credential/auth') @@ -76,8 +76,8 @@ describe('OAuth2 API', () => { }); it('should fail on auth when callback is called as another user', async () => { - const controller = Container.get(OAuth2CredentialController); - const csrfSpy = jest.spyOn(controller, 'createCsrfState').mockClear(); + const oauthService = Container.get(OauthService); + const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); const renderSpy = (Response.render = jest.fn(function () { this.end(); })); @@ -98,8 +98,8 @@ describe('OAuth2 API', () => { }); it('should handle a valid callback without auth', async () => { - const controller = Container.get(OAuth2CredentialController); - const csrfSpy = jest.spyOn(controller, 'createCsrfState').mockClear(); + const oauthService = Container.get(OauthService); + const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); const renderSpy = (Response.render = jest.fn(function () { this.end(); }));