make oauth1 consistent with oauth2. add tests

packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts

@@ -4,6 +4,7 @@ import type { Response } from 'express';
 import { mock } from 'jest-mock-extended';
 import { Cipher } from 'n8n-core';
 import { Logger } from 'n8n-core';
+import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow';
 import nock from 'nock';
 
 import { Time } from '@/constants';
@@ -19,15 +20,21 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { ExternalHooks } from '@/external-hooks';
 import type { OAuthRequest } from '@/requests';
 import { SecretsHelper } from '@/secrets-helpers.ee';
+import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data';
 import { mockInstance } from '@test/mocking';
 
+jest.mock('@/workflow-execute-additional-data');
+
 describe('OAuth1CredentialController', () => {
 	mockInstance(Logger);
 	mockInstance(ExternalHooks);
 	mockInstance(SecretsHelper);
 	mockInstance(VariablesService, {
 		getAllCached: async () => [],
 	});
+	const additionalData = mock<IWorkflowExecuteAdditionalData>();
+	(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);
+
 	const cipher = mockInstance(Cipher);
 	const credentialsHelper = mockInstance(CredentialsHelper);
 	const credentialsRepository = mockInstance(CredentialsRepository);
@@ -106,6 +113,14 @@ describe('OAuth1CredentialController', () => {
 				}),
 			);
 			expect(cipher.encrypt).toHaveBeenCalledWith({ csrfSecret });
+			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
+				additionalData,
+				credential,
+				credential.type,
+				'internal',
+				undefined,
+				false,
+			);
 		});
 	});
 
@@ -235,6 +250,14 @@ describe('OAuth1CredentialController', () => {
 				}),
 			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
+			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
+				additionalData,
+				credential,
+				credential.type,
+				'internal',
+				undefined,
+				true,
+			);
 		});
 	});
 });

packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts

@@ -4,6 +4,7 @@ import { type Response } from 'express';
 import { mock } from 'jest-mock-extended';
 import { Cipher } from 'n8n-core';
 import { Logger } from 'n8n-core';
+import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow';
 import nock from 'nock';
 
 import { CREDENTIAL_BLANKING_VALUE, Time } from '@/constants';
@@ -19,14 +20,20 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { ExternalHooks } from '@/external-hooks';
 import type { OAuthRequest } from '@/requests';
 import { SecretsHelper } from '@/secrets-helpers.ee';
+import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data';
 import { mockInstance } from '@test/mocking';
 
+jest.mock('@/workflow-execute-additional-data');
+
 describe('OAuth2CredentialController', () => {
 	mockInstance(Logger);
 	mockInstance(SecretsHelper);
 	mockInstance(VariablesService, {
 		getAllCached: async () => [],
 	});
+	const additionalData = mock<IWorkflowExecuteAdditionalData>();
+	(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);
+
 	const cipher = mockInstance(Cipher);
 	const externalHooks = mockInstance(ExternalHooks);
 	const credentialsHelper = mockInstance(CredentialsHelper);
@@ -108,6 +115,14 @@ describe('OAuth2CredentialController', () => {
 					type: 'oAuth2Api',
 				}),
 			);
+			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
+				additionalData,
+				credential,
+				credential.type,
+				'internal',
+				undefined,
+				false,
+			);
 		});
 	});
 
@@ -256,6 +271,14 @@ describe('OAuth2CredentialController', () => {
 				}),
 			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
+			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
+				additionalData,
+				credential,
+				credential.type,
+				'internal',
+				undefined,
+				true,
+			);
 		});
 
 		it('merges oauthTokenData if it already exists', async () => {

packages/cli/src/controllers/oauth/abstract-oauth.controller.ts

@@ -86,10 +86,30 @@ export abstract class AbstractOAuthController {
 		return await WorkflowExecuteAdditionalData.getBase();
 	}
 
-	protected async getDecryptedData(
+	/**
+	 * Allow decrypted data to evaluate expressions that include $secrets and apply overwrites
+	 */
+	protected async getDecryptedDataForAuthUri(
 		credential: ICredentialsDb,
 		additionalData: IWorkflowExecuteAdditionalData,
-		raw = true,
+	) {
+		return await this.getDecryptedData(credential, additionalData, false);
+	}
+
+	/**
+	 * Do not apply overwrites here because that removes the CSRF state, and breaks the oauth flow
+	 */
+	protected async getDecryptedDataForCallback(
+		credential: ICredentialsDb,
+		additionalData: IWorkflowExecuteAdditionalData,
+	) {
+		return await this.getDecryptedData(credential, additionalData, true);
+	}
+
+	private async getDecryptedData(
+		credential: ICredentialsDb,
+		additionalData: IWorkflowExecuteAdditionalData,
+		raw: boolean,
 	) {
 		return await this.credentialsHelper.getDecrypted(
 			additionalData,
@@ -184,7 +204,10 @@ export abstract class AbstractOAuthController {
 		}
 
 		const additionalData = await this.getAdditionalData();
-		const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData);
+		const decryptedDataOriginal = await this.getDecryptedDataForCallback(
+			credential,
+			additionalData,
+		);
 		const oauthCredentials = this.applyDefaultsAndOverwrites<T>(
 			credential,
 			decryptedDataOriginal,

packages/cli/src/controllers/oauth/oauth1-credential.controller.ts

@@ -35,7 +35,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 	async getAuthUri(req: OAuthRequest.OAuth1Credential.Auth): Promise<string> {
 		const credential = await this.getCredential(req);
 		const additionalData = await this.getAdditionalData();
-		const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData);
+		const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData);
 		const oauthCredentials = this.applyDefaultsAndOverwrites<OAuth1CredentialData>(
 			credential,
 			decryptedDataOriginal,

packages/cli/src/controllers/oauth/oauth2-credential.controller.ts

@@ -23,10 +23,7 @@ export class OAuth2CredentialController extends AbstractOAuthController {
 	async getAuthUri(req: OAuthRequest.OAuth2Credential.Auth): Promise<string> {
 		const credential = await this.getCredential(req);
 		const additionalData = await this.getAdditionalData();
-
-		// Allow decrypted data to evaluate expressions that include $secrets and apply overwrites
-		// This should only be done for the intial request to the auth URL which passes the client secret
-		const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData, false);
+		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