From d3f9e26d6c5a78d1749394df3ba01404b92dedb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 31 Jul 2024 16:50:37 +0200 Subject: [PATCH 1/3] fix oauth flow --- .../cli/src/controllers/oauth/abstractOAuth.controller.ts | 4 ++-- .../cli/src/controllers/oauth/oAuth1Credential.controller.ts | 4 ++-- .../cli/src/controllers/oauth/oAuth2Credential.controller.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts b/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts index f9ffd591feb22..c584f46ffed98 100644 --- a/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts @@ -71,8 +71,8 @@ export abstract class AbstractOAuthController { return credential; } - protected async getAdditionalData(user: User) { - return await WorkflowExecuteAdditionalData.getBase(user.id); + protected async getAdditionalData() { + return await WorkflowExecuteAdditionalData.getBase(); } protected async getDecryptedData( diff --git a/packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts b/packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts index d967d2f83435a..2a50b00bf9d57 100644 --- a/packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts @@ -33,7 +33,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth1Credential.Auth): Promise { const credential = await this.getCredential(req); - const additionalData = await this.getAdditionalData(req.user); + const additionalData = await this.getAdditionalData(); const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); const oauthCredentials = this.applyDefaultsAndOverwrites( credential, @@ -127,7 +127,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { return this.renderCallbackError(res, errorMessage); } - const additionalData = await this.getAdditionalData(req.user); + const additionalData = await this.getAdditionalData(); const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); const oauthCredentials = this.applyDefaultsAndOverwrites( credential, diff --git a/packages/cli/src/controllers/oauth/oAuth2Credential.controller.ts b/packages/cli/src/controllers/oauth/oAuth2Credential.controller.ts index 719043e9834b7..5b7929495fd42 100644 --- a/packages/cli/src/controllers/oauth/oAuth2Credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oAuth2Credential.controller.ts @@ -20,7 +20,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth2Credential.Auth): Promise { const credential = await this.getCredential(req); - const additionalData = await this.getAdditionalData(req.user); + const additionalData = await this.getAdditionalData(); const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); // At some point in the past we saved hidden scopes to credentials (but shouldn't) @@ -107,7 +107,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { return this.renderCallbackError(res, errorMessage); } - const additionalData = await this.getAdditionalData(req.user); + const additionalData = await this.getAdditionalData(); const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); const oauthCredentials = this.applyDefaultsAndOverwrites( credential, From f583b16a7e3dd3beb7115477810de7d67e8940d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 31 Jul 2024 18:52:05 +0200 Subject: [PATCH 2/3] add integration tests for oauth2 flow --- .../oauth/abstractOAuth.controller.ts | 3 +- .../controllers/oauth/oauth2.api.test.ts | 107 ++++++++++++++++++ packages/cli/test/integration/shared/types.ts | 1 + .../integration/shared/utils/testServer.ts | 4 + 4 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts diff --git a/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts b/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts index c584f46ffed98..b9f0d64446ad5 100644 --- a/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstractOAuth.controller.ts @@ -6,7 +6,6 @@ import type { ICredentialDataDecryptedObject, IWorkflowExecuteAdditionalData } f import { jsonParse, ApplicationError } from 'n8n-workflow'; import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; -import type { User } from '@db/entities/User'; import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import type { ICredentialsDb } from '@/Interfaces'; @@ -119,7 +118,7 @@ export abstract class AbstractOAuthController { return await this.credentialsRepository.findOneBy({ id: credentialId }); } - protected createCsrfState(credentialsId: string): [string, string] { + createCsrfState(credentialsId: string): [string, string] { const token = new Csrf(); const csrfSecret = token.secretSync(); const state: CsrfStateParam = { diff --git a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts new file mode 100644 index 0000000000000..970727c2c8c64 --- /dev/null +++ b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts @@ -0,0 +1,107 @@ +import { Container } from 'typedi'; +import { response as Response } from 'express'; +import nock from 'nock'; +import { parse as parseQs } from 'querystring'; + +import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; +import type { User } from '@db/entities/User'; +import { CredentialsHelper } from '@/CredentialsHelper'; +import { OAuth2CredentialController } from '@/controllers/oauth/oAuth2Credential.controller'; + +import { createOwner } from '@test-integration/db/users'; +import { saveCredential } from '@test-integration/db/credentials'; +import * as testDb from '@test-integration/testDb'; +import { setupTestServer } from '@test-integration/utils'; +import type { SuperAgentTest } from '@test-integration/types'; + +describe('OAuth2 API', () => { + const testServer = setupTestServer({ endpointGroups: ['oauth2'] }); + + let owner: User; + let ownerAgent: SuperAgentTest; + let credential: CredentialsEntity; + const credentialData = { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://test.domain/oauth2/auth', + accessTokenUrl: 'https://test.domain/oauth2/token', + authQueryParameters: 'access_type=offline', + }; + + CredentialsHelper.prototype.applyDefaultsAndOverwrites = (_, decryptedDataOriginal) => + decryptedDataOriginal; + + beforeAll(async () => { + owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + }); + + beforeEach(async () => { + await testDb.truncate(['SharedCredentials', 'Credentials']); + credential = await saveCredential( + { + name: 'Test', + type: 'testOAuth2Api', + data: credentialData, + }, + { + user: owner, + role: 'credential:owner', + }, + ); + }); + + 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 response = await ownerAgent + .get('/oauth2-credential/auth') + .query({ id: credential.id }) + .expect(200); + const authUrl = new URL(response.body.data); + expect(authUrl.hostname).toBe('test.domain'); + expect(authUrl.pathname).toBe('/oauth2/auth'); + + expect(csrfSpy).toHaveBeenCalled(); + const [_, state] = csrfSpy.mock.results[0].value; + expect(parseQs(authUrl.search.slice(1))).toEqual({ + access_type: 'offline', + client_id: 'client_id', + redirect_uri: 'http://localhost:5678/rest/oauth2-credential/callback', + response_type: 'code', + state, + scope: 'openid', + }); + }); + + it('should handle a valid callback without auth', async () => { + const controller = Container.get(OAuth2CredentialController); + const csrfSpy = jest.spyOn(controller, 'createCsrfState').mockClear(); + const renderSpy = (Response.render = jest.fn(function () { + this.end(); + })); + + await ownerAgent.get('/oauth2-credential/auth').query({ id: credential.id }).expect(200); + + const [_, state] = csrfSpy.mock.results[0].value; + + nock('https://test.domain').post('/oauth2/token').reply(200, { access_token: 'updated_token' }); + + await testServer.authlessAgent + .get('/oauth2-credential/callback') + .query({ code: 'auth_code', state }) + .expect(200); + + expect(renderSpy).toHaveBeenCalledWith('oauth-callback'); + + const updatedCredential = await Container.get(CredentialsHelper).getCredentials( + credential, + credential.type, + ); + expect(updatedCredential.getData()).toEqual({ + ...credentialData, + oauthTokenData: { access_token: 'updated_token' }, + }); + }); +}); diff --git a/packages/cli/test/integration/shared/types.ts b/packages/cli/test/integration/shared/types.ts index 0352386590a3d..cb794d0f95b92 100644 --- a/packages/cli/test/integration/shared/types.ts +++ b/packages/cli/test/integration/shared/types.ts @@ -13,6 +13,7 @@ type EndpointGroup = | 'me' | 'users' | 'auth' + | 'oauth2' | 'owner' | 'passwordReset' | 'credentials' diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index 3fc4cb5642b49..7776b7e669415 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -159,6 +159,10 @@ export const setupTestServer = ({ await import('@/controllers/auth.controller'); break; + case 'oauth2': + await import('@/controllers/oauth/oAuth2Credential.controller'); + break; + case 'mfa': await import('@/controllers/mfa.controller'); break; From 84071955e467aeb479abcc27932a540804ef7b6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 1 Aug 2024 10:29:03 +0200 Subject: [PATCH 3/3] remove `user` from `AuthlessRequest` --- packages/cli/src/requests.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 02ef5acb6b015..1ea265f2394e7 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -76,9 +76,7 @@ export type AuthlessRequest< ResponseBody = {}, RequestBody = {}, RequestQuery = {}, -> = APIRequest & { - user: never; -}; +> = APIRequest; export type AuthenticatedRequest< RouteParams = {},