diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts index 488bcf61ff..515b9bad7a 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts @@ -1,9 +1,8 @@ import { Container } from '@n8n/di'; import Csrf from 'csrf'; import type { Response } from 'express'; -import { mock } from 'jest-mock-extended'; -import { Cipher } from 'n8n-core'; -import { Logger } from 'n8n-core'; +import { captor, mock } from 'jest-mock-extended'; +import { Cipher, type InstanceSettings, Logger } from 'n8n-core'; import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import nock from 'nock'; @@ -35,7 +34,8 @@ describe('OAuth1CredentialController', () => { const additionalData = mock(); (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); - const cipher = mockInstance(Cipher); + const cipher = new Cipher(mock({ encryptionKey: 'password' })); + Container.set(Cipher, cipher); const credentialsHelper = mockInstance(CredentialsHelper); const credentialsRepository = mockInstance(CredentialsRepository); const sharedCredentialsRepository = mockInstance(SharedCredentialsRepository); @@ -51,6 +51,7 @@ describe('OAuth1CredentialController', () => { id: '1', name: 'Test Credential', type: 'oAuth1Api', + data: cipher.encrypt({}), }); const controller = Container.get(OAuth1CredentialController); @@ -98,21 +99,23 @@ describe('OAuth1CredentialController', () => { }) .once() .reply(200, { oauth_token: 'random-token' }); - cipher.encrypt.mockReturnValue('encrypted'); const req = mock({ user, query: { id: '1' } }); const authUri = await controller.getAuthUri(req); expect(authUri).toEqual('https://example.domain/oauth/authorize?oauth_token=random-token'); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth1Api', }), ); - expect(cipher.encrypt).toHaveBeenCalledWith({ csrfSecret }); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ csrfSecret: 'csrf-secret' }), + ); expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( additionalData, credential, @@ -233,22 +236,22 @@ describe('OAuth1CredentialController', () => { }) .once() .reply(200, 'access_token=new_token'); - cipher.encrypt.mockReturnValue('encrypted'); await controller.handleCallback(req, res); - expect(cipher.encrypt).toHaveBeenCalledWith({ - oauthTokenData: { access_token: 'new_token' }, - }); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth1Api', }), ); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ oauthTokenData: { access_token: 'new_token' } }), + ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( additionalData, 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 d075dcd198..6ed9cb14ad 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 @@ -1,9 +1,8 @@ import { Container } from '@n8n/di'; import Csrf from 'csrf'; import { type Response } from 'express'; -import { mock } from 'jest-mock-extended'; -import { Cipher } from 'n8n-core'; -import { Logger } from 'n8n-core'; +import { captor, mock } from 'jest-mock-extended'; +import { Cipher, type InstanceSettings, Logger } from 'n8n-core'; import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import nock from 'nock'; @@ -34,7 +33,9 @@ describe('OAuth2CredentialController', () => { const additionalData = mock(); (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); - const cipher = mockInstance(Cipher); + const cipher = new Cipher(mock({ encryptionKey: 'password' })); + Container.set(Cipher, cipher); + const externalHooks = mockInstance(ExternalHooks); const credentialsHelper = mockInstance(CredentialsHelper); const credentialsRepository = mockInstance(CredentialsRepository); @@ -51,6 +52,7 @@ describe('OAuth2CredentialController', () => { id: '1', name: 'Test Credential', type: 'oAuth2Api', + data: cipher.encrypt({}), }); const controller = Container.get(OAuth2CredentialController); @@ -92,7 +94,6 @@ describe('OAuth2CredentialController', () => { jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token'); sharedCredentialsRepository.findCredentialForUser.mockResolvedValueOnce(credential); credentialsHelper.getDecrypted.mockResolvedValueOnce({}); - cipher.encrypt.mockReturnValue('encrypted'); const req = mock({ user, query: { id: '1' } }); const authUri = await controller.getAuthUri(req); @@ -106,15 +107,19 @@ describe('OAuth2CredentialController', () => { createdAt: timestamp, userId: '123', }); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth2Api', }), ); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ csrfSecret: 'csrf-secret' }), + ); expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( additionalData, credential, @@ -248,7 +253,6 @@ describe('OAuth2CredentialController', () => { '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); @@ -258,18 +262,21 @@ describe('OAuth2CredentialController', () => { redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', }), ]); - expect(cipher.encrypt).toHaveBeenCalledWith({ - oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, - }); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth2Api', }), ); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ + oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, + }), + ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( additionalData, @@ -294,7 +301,6 @@ describe('OAuth2CredentialController', () => { '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); @@ -304,22 +310,25 @@ describe('OAuth2CredentialController', () => { redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', }), ]); - expect(cipher.encrypt).toHaveBeenCalledWith({ - oauthTokenData: { - token: true, - access_token: 'access-token', - refresh_token: 'refresh-token', - }, - }); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth2Api', }), ); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ + oauthTokenData: { + token: true, + access_token: 'access-token', + refresh_token: 'refresh-token', + }, + }), + ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); }); @@ -336,7 +345,6 @@ describe('OAuth2CredentialController', () => { '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); @@ -346,18 +354,21 @@ describe('OAuth2CredentialController', () => { redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback', }), ]); - expect(cipher.encrypt).toHaveBeenCalledWith({ - oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, - }); + const dataCaptor = captor(); expect(credentialsRepository.update).toHaveBeenCalledWith( '1', expect.objectContaining({ - data: 'encrypted', + data: dataCaptor, id: '1', name: 'Test Credential', type: 'oAuth2Api', }), ); + expect(cipher.decrypt(dataCaptor.value)).toEqual( + JSON.stringify({ + oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' }, + }), + ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); }); }); diff --git a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts index 5098cec28d..2964437b49 100644 --- a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts @@ -136,10 +136,11 @@ export abstract class AbstractOAuthController { protected async encryptAndSaveData( credential: ICredentialsDb, - decryptedData: ICredentialDataDecryptedObject, + toUpdate: ICredentialDataDecryptedObject, + toDelete: string[] = [], ) { - const credentials = new Credentials(credential, credential.type); - credentials.setData(decryptedData); + const credentials = new Credentials(credential, credential.type, credential.data); + credentials.updateData(toUpdate, toDelete); await this.credentialsRepository.update(credential.id, { ...credentials.getDataToSave(), updatedAt: new Date(), diff --git a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts index fe9d22edd8..2c06e19fb3 100644 --- a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts @@ -91,8 +91,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { const returnUri = `${oauthCredentials.authUrl}?oauth_token=${responseJson.oauth_token}`; - decryptedDataOriginal.csrfSecret = csrfSecret; - await this.encryptAndSaveData(credential, decryptedDataOriginal); + await this.encryptAndSaveData(credential, { csrfSecret }); this.logger.debug('OAuth1 authorization successful for new credential', { userId: req.user.id, @@ -116,7 +115,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { ); } - const [credential, decryptedDataOriginal, oauthCredentials] = + const [credential, _, oauthCredentials] = await this.resolveCredential(req); const oauthToken = await axios.post(oauthCredentials.accessTokenUrl, { @@ -128,12 +127,9 @@ export class OAuth1CredentialController extends AbstractOAuthController { const paramParser = new URLSearchParams(oauthToken.data); - const oauthTokenJson = Object.fromEntries(paramParser.entries()); + const oauthTokenData = Object.fromEntries(paramParser.entries()); - delete decryptedDataOriginal.csrfSecret; - decryptedDataOriginal.oauthTokenData = oauthTokenJson; - - await this.encryptAndSaveData(credential, decryptedDataOriginal); + await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); this.logger.debug('OAuth1 callback successful for new credential', { credentialId: credential.id, diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index b9ed0d1126..16dc939b14 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -4,7 +4,7 @@ import { Response } from 'express'; import omit from 'lodash/omit'; import set from 'lodash/set'; import split from 'lodash/split'; -import { jsonStringify } from 'n8n-workflow'; +import { type ICredentialDataDecryptedObject, jsonStringify } from 'n8n-workflow'; import pkceChallenge from 'pkce-challenge'; import * as qs from 'querystring'; @@ -60,7 +60,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { await this.externalHooks.run('oauth2.authenticate', [oAuthOptions]); - decryptedDataOriginal.csrfSecret = csrfSecret; + const toUpdate: ICredentialDataDecryptedObject = { csrfSecret }; if (oauthCredentials.grantType === 'pkce') { const { code_verifier, code_challenge } = pkceChallenge(); oAuthOptions.query = { @@ -68,10 +68,10 @@ export class OAuth2CredentialController extends AbstractOAuthController { code_challenge, code_challenge_method: 'S256', }; - decryptedDataOriginal.codeVerifier = code_verifier; + toUpdate.codeVerifier = code_verifier; } - await this.encryptAndSaveData(credential, decryptedDataOriginal); + await this.encryptAndSaveData(credential, toUpdate); const oAuthObj = new ClientOAuth2(oAuthOptions); const returnUri = oAuthObj.code.getUri(); @@ -133,17 +133,15 @@ export class OAuth2CredentialController extends AbstractOAuthController { set(oauthToken.data, 'callbackQueryString', omit(req.query, 'state', 'code')); } - 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); - } else { - // No data exists so simply set - decryptedDataOriginal.oauthTokenData = oauthToken.data; - } + // 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. + let { oauthTokenData } = decryptedDataOriginal; + oauthTokenData = { + ...(typeof oauthTokenData === 'object' ? oauthTokenData : {}), + ...oauthToken.data, + }; - delete decryptedDataOriginal.csrfSecret; - await this.encryptAndSaveData(credential, decryptedDataOriginal); + await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']); this.logger.debug('OAuth2 callback successful for credential', { credentialId: credential.id, diff --git a/packages/core/src/__tests__/credentials.test.ts b/packages/core/src/__tests__/credentials.test.ts index 09b7586daf..b7a10d57d4 100644 --- a/packages/core/src/__tests__/credentials.test.ts +++ b/packages/core/src/__tests__/credentials.test.ts @@ -118,4 +118,74 @@ describe('Credentials', () => { }, ); }); + + describe('updateData', () => { + const nodeCredentials = { id: '123', name: 'Test Credential' }; + const credentialType = 'testApi'; + + test('should update existing data', () => { + const credentials = new Credentials( + nodeCredentials, + credentialType, + cipher.encrypt({ + username: 'olduser', + password: 'oldpass', + apiKey: 'oldkey', + }), + ); + + credentials.updateData({ username: 'newuser', password: 'newpass' }); + + expect(credentials.getData()).toEqual({ + username: 'newuser', + password: 'newpass', + apiKey: 'oldkey', + }); + }); + + test('should delete specified keys', () => { + const credentials = new Credentials( + nodeCredentials, + credentialType, + cipher.encrypt({ + username: 'testuser', + password: 'testpass', + apiKey: 'testkey', + }), + ); + + credentials.updateData({}, ['username', 'apiKey']); + + expect(credentials.getData()).toEqual({ + password: 'testpass', + }); + }); + + test('should update and delete keys in same operation', () => { + const credentials = new Credentials( + nodeCredentials, + credentialType, + cipher.encrypt({ + username: 'olduser', + password: 'oldpass', + apiKey: 'oldkey', + }), + ); + + credentials.updateData({ username: 'newuser' }, ['apiKey']); + + expect(credentials.getData()).toEqual({ + username: 'newuser', + password: 'oldpass', + }); + }); + + test('should throw an error if no data was previously set', () => { + const credentials = new Credentials(nodeCredentials, credentialType); + + expect(() => { + credentials.updateData({ username: 'newuser' }); + }).toThrow(CREDENTIAL_ERRORS.NO_DATA); + }); + }); }); diff --git a/packages/core/src/credentials.ts b/packages/core/src/credentials.ts index f2d10df156..6b66cf70b3 100644 --- a/packages/core/src/credentials.ts +++ b/packages/core/src/credentials.ts @@ -30,6 +30,18 @@ export class Credentials< this.data = this.cipher.encrypt(data); } + /** + * Update parts of the credential data. + * This decrypts the data, modifies it, and then re-encrypts the updated data back to a string. + */ + updateData(toUpdate: Partial, toDelete: Array = []) { + const updatedData: T = { ...this.getData(), ...toUpdate }; + for (const key of toDelete) { + delete updatedData[key]; + } + this.setData(updatedData); + } + /** * Returns the decrypted credential object */