fix: Don't break oauth credentials when updating them and allow fixing broken oauth credentials by repeating the authorization flow (#12563)

Author: despairblue

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

@@ -6,7 +6,7 @@ import { Cipher } from 'n8n-core';
 import { Logger } from 'n8n-core';
 import nock from 'nock';
 
-import { Time } from '@/constants';
+import { CREDENTIAL_BLANKING_VALUE, Time } from '@/constants';
 import { OAuth2CredentialController } from '@/controllers/oauth/oauth2-credential.controller';
 import { CredentialsHelper } from '@/credentials-helper';
 import type { CredentialsEntity } from '@/databases/entities/credentials-entity';
@@ -257,5 +257,85 @@ describe('OAuth2CredentialController', () => {
 			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
 		});
+
+		it('merges oauthTokenData if it already exists', async () => {
+			credentialsRepository.findOneBy.mockResolvedValueOnce(credential);
+			credentialsHelper.getDecrypted.mockResolvedValueOnce({
+				csrfSecret,
+				oauthTokenData: { token: true },
+			});
+			jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true);
+			nock('https://example.domain')
+				.post(
+					'/token',
+					'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback',
+				)
+				.reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' });
+			cipher.encrypt.mockReturnValue('encrypted');
+
+			await controller.handleCallback(req, res);
+
+			expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', [
+				expect.objectContaining({
+					clientId: 'test-client-id',
+					redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback',
+				}),
+			]);
+			expect(cipher.encrypt).toHaveBeenCalledWith({
+				oauthTokenData: {
+					token: true,
+					access_token: 'access-token',
+					refresh_token: 'refresh-token',
+				},
+			});
+			expect(credentialsRepository.update).toHaveBeenCalledWith(
+				'1',
+				expect.objectContaining({
+					data: 'encrypted',
+					id: '1',
+					name: 'Test Credential',
+					type: 'oAuth2Api',
+				}),
+			);
+			expect(res.render).toHaveBeenCalledWith('oauth-callback');
+		});
+
+		it('overwrites oauthTokenData if it is a string', async () => {
+			credentialsRepository.findOneBy.mockResolvedValueOnce(credential);
+			credentialsHelper.getDecrypted.mockResolvedValueOnce({
+				csrfSecret,
+				oauthTokenData: CREDENTIAL_BLANKING_VALUE,
+			});
+			jest.spyOn(Csrf.prototype, 'verify').mockReturnValueOnce(true);
+			nock('https://example.domain')
+				.post(
+					'/token',
+					'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback',
+				)
+				.reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' });
+			cipher.encrypt.mockReturnValue('encrypted');
+
+			await controller.handleCallback(req, res);
+
+			expect(externalHooks.run).toHaveBeenCalledWith('oauth2.callback', [
+				expect.objectContaining({
+					clientId: 'test-client-id',
+					redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback',
+				}),
+			]);
+			expect(cipher.encrypt).toHaveBeenCalledWith({
+				oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' },
+			});
+			expect(credentialsRepository.update).toHaveBeenCalledWith(
+				'1',
+				expect.objectContaining({
+					data: 'encrypted',
+					id: '1',
+					name: 'Test Credential',
+					type: 'oAuth2Api',
+				}),
+			);
+			expect(res.render).toHaveBeenCalledWith('oauth-callback');
+		});
 	});
 });

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

@@ -133,7 +133,7 @@ export class OAuth2CredentialController extends AbstractOAuthController {
 				set(oauthToken.data, 'callbackQueryString', omit(req.query, 'state', 'code'));
 			}
 
-			if (decryptedDataOriginal.oauthTokenData) {
+			if (typeof decryptedDataOriginal.oauthTokenData === 'object') {
 				// Only overwrite supplied data as some providers do for example just return the
 				// refresh_token on the very first request and not on subsequent ones.
 				Object.assign(decryptedDataOriginal.oauthTokenData, oauthToken.data);

Diff for: packages/cli/src/credentials/__tests__/credentials.controller.test.ts

@@ -33,7 +33,7 @@ describe('CredentialsController', () => {
 	});
 
 	describe('createCredentials', () => {
-		it('it should create new credentials and emit "credentials-created"', async () => {
+		it('should create new credentials and emit "credentials-created"', async () => {
 			// Arrange
 
 			const newCredentialsPayload = createNewCredentialsPayload();

Diff for: packages/cli/src/credentials/credentials.controller.ts

@@ -198,7 +198,7 @@ export class CredentialsController {
 			throw new BadRequestError('Managed credentials cannot be updated');
 		}
 
-		const decryptedData = this.credentialsService.decrypt(credential);
+		const decryptedData = this.credentialsService.decrypt(credential, true);
 		const preparedCredentialData = await this.credentialsService.prepareUpdateData(
 			req.body,
 			decryptedData,

Diff for: packages/cli/test/integration/credentials/credentials.api.test.ts

@@ -4,6 +4,7 @@ import type { Scope } from '@sentry/node';
 import { Credentials } from 'n8n-core';
 import { randomString } from 'n8n-workflow';
 
+import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
 import { CredentialsService } from '@/credentials/credentials.service';
 import type { Project } from '@/databases/entities/project';
 import type { User } from '@/databases/entities/user';
@@ -1164,6 +1165,73 @@ describe('PATCH /credentials/:id', () => {
 		expect(shellCredential.name).toBe(patchPayload.name); // updated
 	});
 
+	test('should not store redacted value in the db for oauthTokenData', async () => {
+		// ARRANGE
+		const credentialService = Container.get(CredentialsService);
+		const redactSpy = jest.spyOn(credentialService, 'redact').mockReturnValueOnce({
+			accessToken: CREDENTIAL_BLANKING_VALUE,
+			oauthTokenData: CREDENTIAL_BLANKING_VALUE,
+		});
+
+		const payload = randomCredentialPayload();
+		payload.data.oauthTokenData = { tokenData: true };
+		const savedCredential = await saveCredential(payload, {
+			user: owner,
+			role: 'credential:owner',
+		});
+
+		// ACT
+		const patchPayload = { ...payload, data: { foo: 'bar' } };
+		await authOwnerAgent.patch(`/credentials/${savedCredential.id}`).send(patchPayload).expect(200);
+
+		// ASSERT
+		const response = await authOwnerAgent
+			.get(`/credentials/${savedCredential.id}`)
+			.query({ includeData: true })
+			.expect(200);
+
+		const { id, data } = response.body.data;
+
+		expect(id).toBe(savedCredential.id);
+		expect(data).toEqual({
+			...patchPayload.data,
+			// should be the original
+			oauthTokenData: payload.data.oauthTokenData,
+		});
+		expect(redactSpy).not.toHaveBeenCalled();
+	});
+
+	test('should not allow to overwrite oauthTokenData', async () => {
+		// ARRANGE
+		const payload = randomCredentialPayload();
+		payload.data.oauthTokenData = { tokenData: true };
+		const savedCredential = await saveCredential(payload, {
+			user: owner,
+			role: 'credential:owner',
+		});
+
+		// ACT
+		const patchPayload = {
+			...payload,
+			data: { accessToken: 'new', oauthTokenData: { tokenData: false } },
+		};
+		await authOwnerAgent.patch(`/credentials/${savedCredential.id}`).send(patchPayload).expect(200);
+
+		// ASSERT
+		const response = await authOwnerAgent
+			.get(`/credentials/${savedCredential.id}`)
+			.query({ includeData: true })
+			.expect(200);
+
+		const { id, data } = response.body.data;
+
+		expect(id).toBe(savedCredential.id);
+		// was overwritten
+		expect(data.accessToken).toBe(patchPayload.data.accessToken);
+		// was not overwritten
+		expect(data.oauthTokenData).toEqual(payload.data.oauthTokenData);
+	});
+
 	test('should fail with invalid inputs', async () => {
 		const savedCredential = await saveCredential(randomCredentialPayload(), {
 			user: owner,