From efb71dd9adfe33ed21e16ff94ba322443d8eadbd 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 11:02:36 +0200 Subject: [PATCH] fix(core): Fix oauth2 callback and add integration tests (no-changelog) (#10272) --- .../oauth/abstractOAuth.controller.ts | 7 +- .../oauth/oAuth1Credential.controller.ts | 4 +- .../oauth/oAuth2Credential.controller.ts | 4 +- packages/cli/src/requests.ts | 4 +- .../controllers/oauth/oauth2.api.test.ts | 107 ++++++++++++++++++ packages/cli/test/integration/shared/types.ts | 1 + .../integration/shared/utils/testServer.ts | 4 + 7 files changed, 120 insertions(+), 11 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 f9ffd591fe..b9f0d64446 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'; @@ -71,8 +70,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( @@ -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/src/controllers/oauth/oAuth1Credential.controller.ts b/packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts index d967d2f834..2a50b00bf9 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 719043e983..5b7929495f 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, diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 02ef5acb6b..1ea265f239 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 = {}, 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 0000000000..970727c2c8 --- /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 0352386590..cb794d0f95 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 3fc4cb5642..7776b7e669 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;