fix(core): Fix oauth2 callback and add integration tests (no-changelog) (#10272)

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) {
+	protected async getAdditionalData() {
-		return await WorkflowExecuteAdditionalData.getBase(user.id);
+		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 = {

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<string> {
 		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<OAuth1CredentialData>(
 			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<OAuth1CredentialData>(
 				credential,

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<string> {
 		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<OAuth2CredentialData>(
 				credential,

packages/cli/src/requests.ts

@@ -76,9 +76,7 @@ export type AuthlessRequest<
 	ResponseBody = {},
 	RequestBody = {},
 	RequestQuery = {},
-> = APIRequest<RouteParams, ResponseBody, RequestBody, RequestQuery> & {
+> = APIRequest<RouteParams, ResponseBody, RequestBody, RequestQuery>;
-	user: never;
-};
 
 export type AuthenticatedRequest<
 	RouteParams = {},

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' },
+		});
+	});
+});

packages/cli/test/integration/shared/types.ts

@@ -13,6 +13,7 @@ type EndpointGroup =
 	| 'me'
 	| 'users'
 	| 'auth'
+	| 'oauth2'
 	| 'owner'
 	| 'passwordReset'
 	| 'credentials'

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;