From 73897c7662a432834eb6f9d0f9ace8d986c1acb5 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 13 Jan 2025 13:48:16 +0100 Subject: [PATCH 1/4] fix: Don't break oauth credentials when updating them and allow fixing broken oauth credentials by repeating the authorization flow (#12563) --- .../oauth2-credential.controller.test.ts | 82 ++++++++++++++++++- .../oauth/oauth2-credential.controller.ts | 2 +- .../__tests__/credentials.controller.test.ts | 2 +- .../src/credentials/credentials.controller.ts | 2 +- .../credentials/credentials.api.test.ts | 68 +++++++++++++++ 5 files changed, 152 insertions(+), 4 deletions(-) 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, From 2110e9a0513b8c36beb85302e0d38a2658ea5d6e Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Mon, 13 Jan 2025 15:09:07 +0200 Subject: [PATCH 2/4] fix(editor): Fix the `openselectivenodecreator` custom action on new canvas (#12580) --- packages/editor-ui/src/views/NodeView.v2.vue | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/editor-ui/src/views/NodeView.v2.vue b/packages/editor-ui/src/views/NodeView.v2.vue index 8c16967273..8ea44cc3a1 100644 --- a/packages/editor-ui/src/views/NodeView.v2.vue +++ b/packages/editor-ui/src/views/NodeView.v2.vue @@ -31,6 +31,7 @@ import type { IWorkflowDb, IWorkflowTemplate, NodeCreatorOpenSource, + NodeFilterType, ToggleNodeCreatorOptions, WorkflowDataWithTemplateId, XYPosition, @@ -1559,13 +1560,15 @@ function registerCustomActions() { registerCustomAction({ key: 'openSelectiveNodeCreator', action: ({ + creatorview: creatorView, connectiontype: connectionType, node, }: { + creatorview: NodeFilterType; connectiontype: NodeConnectionType; node: string; }) => { - void onOpenSelectiveNodeCreator(node, connectionType); + nodeCreatorStore.openSelectiveNodeCreator({ node, connectionType, creatorView }); }, }); From c8e3c5399efde93486c1dd5c373cb2c5ff8a0691 Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Mon, 13 Jan 2025 17:13:11 +0200 Subject: [PATCH 3/4] fix(editor): Fix selection rectangle context menu on new canvas (#12584) --- packages/editor-ui/src/components/canvas/Canvas.vue | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/editor-ui/src/components/canvas/Canvas.vue b/packages/editor-ui/src/components/canvas/Canvas.vue index 1501f58745..f9697b8645 100644 --- a/packages/editor-ui/src/components/canvas/Canvas.vue +++ b/packages/editor-ui/src/components/canvas/Canvas.vue @@ -519,6 +519,13 @@ function onOpenContextMenu(event: MouseEvent) { }); } +function onOpenSelectionContextMenu({ event }: { event: MouseEvent }) { + contextMenu.open(event, { + source: 'canvas', + nodeIds: selectedNodeIds.value, + }); +} + function onOpenNodeContextMenu( id: string, event: MouseEvent, @@ -692,6 +699,7 @@ provide(CanvasKey, { @node-drag-stop="onNodeDragStop" @node-click="onNodeClick" @selection-drag-stop="onSelectionDragStop" + @selection-context-menu="onOpenSelectionContextMenu" @dragover="onDragOver" @drop="onDrop" > From 89f93fd20a03b222f8c43b2b33c5a0bb9f53edb5 Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Mon, 13 Jan 2025 16:23:31 +0100 Subject: [PATCH 4/4] feat(editor): Don't show Sub-Workflow id for RunDataAi 'View Execution' link (no-changelog) (#12578) --- .../src/components/RunDataAi/RunDataAiContent.vue | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/editor-ui/src/components/RunDataAi/RunDataAiContent.vue b/packages/editor-ui/src/components/RunDataAi/RunDataAiContent.vue index e065e46250..adacd5f30e 100644 --- a/packages/editor-ui/src/components/RunDataAi/RunDataAiContent.vue +++ b/packages/editor-ui/src/components/RunDataAi/RunDataAiContent.vue @@ -148,13 +148,7 @@ const outputError = computed(() => { @click.stop="trackOpeningRelatedExecution(runMeta, 'ai')" > - {{ - i18n.baseText('runData.openSubExecutionWithId', { - interpolate: { - id: runMeta.subExecution?.executionId, - }, - }) - }} + {{ i18n.baseText('runData.openSubExecutionSingle') }}