mudhorn/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2025-02-21 02:56:40 -08:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse source
This commit is contained in:
Danny Martini 2025-01-13 13:48:16 +01:00 committed by GitHub
parent bee7267fe3
commit 73897c7662
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 152 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

82
packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts
View file

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

2
packages/cli/src/controllers/oauth/oauth2-credential.controller.ts
View file

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

2
packages/cli/src/credentials/__tests__/credentials.controller.test.ts
View file

@ -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();

2
packages/cli/src/credentials/credentials.controller.ts
View file

@ -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,

68
packages/cli/test/integration/credentials/credentials.api.test.ts
View file

@ -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,