Skip to content

Commit

Permalink
fix(core): Do not save credential overwrites data into the database (#…
Browse files Browse the repository at this point in the history 
…13170)
  • Loading branch information
@netroy
netroy authored Feb 10, 2025
1 parent dd6d30c commit 298a7b0
Show file tree
Hide file tree
Showing 7 changed files with 153 additions and 62 deletions.
27 changes: 15 additions & 12 deletions packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -35,7 +34,8 @@ describe('OAuth1CredentialController', () => {
const additionalData = mock<IWorkflowExecuteAdditionalData>();
(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);

const cipher = mockInstance(Cipher);
const cipher = new Cipher(mock<InstanceSettings>({ encryptionKey: 'password' }));
Container.set(Cipher, cipher);
const credentialsHelper = mockInstance(CredentialsHelper);
const credentialsRepository = mockInstance(CredentialsRepository);
const sharedCredentialsRepository = mockInstance(SharedCredentialsRepository);
Expand All @@ -51,6 +51,7 @@ describe('OAuth1CredentialController', () => {
id: '1',
name: 'Test Credential',
type: 'oAuth1Api',
data: cipher.encrypt({}),
});

const controller = Container.get(OAuth1CredentialController);
Expand Down Expand Up @@ -98,21 +99,23 @@ describe('OAuth1CredentialController', () => {
})
.once()
.reply(200, { oauth_token: 'random-token' });
cipher.encrypt.mockReturnValue('encrypted');

const req = mock<OAuthRequest.OAuth1Credential.Auth>({ 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,
Expand Down Expand Up @@ -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,
Expand Down
61 changes: 36 additions & 25 deletions packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -34,7 +33,9 @@ describe('OAuth2CredentialController', () => {
const additionalData = mock<IWorkflowExecuteAdditionalData>();
(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);

const cipher = mockInstance(Cipher);
const cipher = new Cipher(mock<InstanceSettings>({ encryptionKey: 'password' }));
Container.set(Cipher, cipher);

const externalHooks = mockInstance(ExternalHooks);
const credentialsHelper = mockInstance(CredentialsHelper);
const credentialsRepository = mockInstance(CredentialsRepository);
Expand All @@ -51,6 +52,7 @@ describe('OAuth2CredentialController', () => {
id: '1',
name: 'Test Credential',
type: 'oAuth2Api',
data: cipher.encrypt({}),
});

const controller = Container.get(OAuth2CredentialController);
Expand Down Expand Up @@ -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<OAuthRequest.OAuth2Credential.Auth>({ user, query: { id: '1' } });
const authUri = await controller.getAuthUri(req);
Expand All @@ -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,
Expand Down Expand Up @@ -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);

Expand All @@ -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,
Expand All @@ -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);

Expand All @@ -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');
});

Expand All @@ -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);

Expand All @@ -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');
});
});
Expand Down
7 changes: 4 additions & 3 deletions packages/cli/src/controllers/oauth/abstract-oauth.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
12 changes: 4 additions & 8 deletions packages/cli/src/controllers/oauth/oauth1-credential.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -116,7 +115,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
);
}

const [credential, decryptedDataOriginal, oauthCredentials] =
const [credential, _, oauthCredentials] =
await this.resolveCredential<OAuth1CredentialData>(req);

const oauthToken = await axios.post<string>(oauthCredentials.accessTokenUrl, {
Expand All @@ -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,
Expand Down
26 changes: 12 additions & 14 deletions packages/cli/src/controllers/oauth/oauth2-credential.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -60,18 +60,18 @@ 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 = {
...oAuthOptions.query,
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();
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 298a7b0

Please # to comment.