diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts index 53bba08c58..1984d12f59 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts +++ b/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 --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index 0f563993ff..e188670fde 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/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 --git a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts index 13e72e8003..68d2f26750 100644 --- a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts +++ b/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 --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 4cc0b500f2..73888e1977 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/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 --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index a988569890..ece8340e83 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/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,