more tests and refactor oauth1 code to be consistent with oauth2

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

@@ -8,7 +8,7 @@ import Container from 'typedi';
 import { Time } from '@/constants';
 import { OAuth1CredentialController } from '@/controllers/oauth/oauth1-credential.controller';
 import { CredentialsHelper } from '@/credentials-helper';
-import { CredentialsEntity } from '@/databases/entities/credentials-entity';
+import type { CredentialsEntity } from '@/databases/entities/credentials-entity';
 import type { User } from '@/databases/entities/user';
 import { CredentialsRepository } from '@/databases/repositories/credentials.repository';
 import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository';
@@ -81,6 +81,7 @@ describe('OAuth1CredentialController', () => {
 			credentialsHelper.applyDefaultsAndOverwrites.mockReturnValueOnce({
 				requestTokenUrl: 'https://example.domain/oauth/request_token',
 				authUrl: 'https://example.domain/oauth/authorize',
+				accessTokenUrl: 'https://example.domain/oauth/access_token',
 				signatureMethod: 'HMAC-SHA1',
 			});
 			nock('https://example.domain')
@@ -172,7 +173,7 @@ describe('OAuth1CredentialController', () => {
 		});
 
 		it('should render the error page when state differs from the stored state in the credential', async () => {
-			credentialsRepository.findOneBy.mockResolvedValue(new CredentialsEntity());
+			credentialsRepository.findOneBy.mockResolvedValue(credential);
 			credentialsHelper.getDecrypted.mockResolvedValue({ csrfSecret: 'invalid' });
 
 			const req = mock<OAuthRequest.OAuth1Credential.Callback>();
@@ -193,7 +194,7 @@ describe('OAuth1CredentialController', () => {
 		});
 
 		it('should render the error page when state is older than 5 minutes', async () => {
-			credentialsRepository.findOneBy.mockResolvedValue(new CredentialsEntity());
+			credentialsRepository.findOneBy.mockResolvedValue(credential);
 			credentialsHelper.getDecrypted.mockResolvedValue({ csrfSecret });
 			jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true);
 
@@ -215,5 +216,48 @@ describe('OAuth1CredentialController', () => {
 				},
 			});
 		});
+
+		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.mockReturnValueOnce({
+				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');
+			cipher.encrypt.mockReturnValue('encrypted');
+
+			const req = mock<OAuthRequest.OAuth1Credential.Callback>();
+			const res = mock<Response>();
+			req.query = {
+				oauth_verifier: 'verifier',
+				oauth_token: 'token',
+				state: validState,
+			} as OAuthRequest.OAuth1Credential.Callback['query'];
+
+			await controller.handleCallback(req, res);
+
+			expect(cipher.encrypt).toHaveBeenCalledWith({
+				oauthTokenData: { access_token: 'new_token' },
+			});
+			expect(credentialsRepository.update).toHaveBeenCalledWith(
+				'1',
+				expect.objectContaining({
+					data: 'encrypted',
+					id: '1',
+					name: 'Test Credential',
+					type: 'oAuth1Api',
+				}),
+			);
+			expect(res.render).toHaveBeenCalledWith('oauth-callback');
+		});
 	});
 });

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

@@ -2,15 +2,14 @@ import type { AxiosRequestConfig } from 'axios';
 import axios from 'axios';
 import { createHmac } from 'crypto';
 import { Response } from 'express';
+import { jsonStringify } from 'n8n-workflow';
 import type { RequestOptions } from 'oauth-1.0a';
 import clientOAuth1 from 'oauth-1.0a';
 
 import { Time } from '@/constants';
 import { Get, RestController } from '@/decorators';
 import { AuthError } from '@/errors/response-errors/auth.error';
-import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { OAuthRequest } from '@/requests';
-import { sendErrorResponse } from '@/response-helper';
 
 import {
 	AbstractOAuthController,
@@ -159,31 +158,18 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 				return this.renderCallbackError(res, 'The OAuth1 state expired. Please try again.');
 			}
 
-			const options: AxiosRequestConfig = {
-				method: 'POST',
-				url: oauthCredentials.accessTokenUrl,
-				params: {
-					oauth_token,
-					oauth_verifier,
-				},
-			};
-
-			let oauthToken;
-
-			try {
-				oauthToken = await axios.request(options);
-			} catch (error) {
-				this.logger.error('Unable to fetch tokens for OAuth1 callback', { credentialId });
-				const errorResponse = new NotFoundError('Unable to get access tokens!');
-				return sendErrorResponse(res, errorResponse);
-			}
+			const oauthToken = await axios.post<string>(oauthCredentials.accessTokenUrl, {
+				oauth_token,
+				oauth_verifier,
+			});
 
 			// Response comes as x-www-form-urlencoded string so convert it to JSON
 
-			const paramParser = new URLSearchParams(oauthToken.data as string);
+			const paramParser = new URLSearchParams(oauthToken.data);
 
 			const oauthTokenJson = Object.fromEntries(paramParser.entries());
 
+			delete decryptedDataOriginal.csrfSecret;
 			decryptedDataOriginal.oauthTokenData = oauthTokenJson;
 
 			await this.encryptAndSaveData(credential, decryptedDataOriginal);
@@ -193,8 +179,12 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 			});
 			return res.render('oauth-callback');
 		} catch (error) {
-			this.logger.error('OAuth1 callback failed because of insufficient user permissions');
-			return sendErrorResponse(res, error as Error);
+			return this.renderCallbackError(
+				res,
+				(error as Error).message,
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+				'body' in error ? jsonStringify(error.body) : undefined,
+			);
 		}
 	}
 }

packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts

@@ -87,7 +87,7 @@ describe('OAuth2 API', () => {
 
 		nock('https://test.domain').post('/oauth2/token').reply(200, { access_token: 'updated_token' });
 
-		await testServer.authlessAgent
+		await ownerAgent
 			.get('/oauth2-credential/callback')
 			.query({ code: 'auth_code', state })
 			.expect(200);