From a3a1af2f4a899c4240b39722c9d4e1e70fa12327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:39:36 +0200 Subject: [PATCH 01/16] :blue_book: Update request type --- packages/cli/src/requests.d.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/cli/src/requests.d.ts b/packages/cli/src/requests.d.ts index 53ad893ec6ab3..0f18a25a33a88 100644 --- a/packages/cli/src/requests.d.ts +++ b/packages/cli/src/requests.d.ts @@ -48,6 +48,7 @@ export declare namespace WorkflowRequest { settings: IWorkflowSettings; active: boolean; tags: string[]; + hash: string; }>; type Create = AuthenticatedRequest<{}, {}, RequestBody>; From 1a4fdc30abbc49bc15a3e45d4e5316e9fe101360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:39:49 +0200 Subject: [PATCH 02/16] :blue_book: Update FE types --- packages/editor-ui/src/Interface.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index 4613303b1caf7..5d8ae2b96d38d 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -268,6 +268,7 @@ export interface IWorkflowData { settings?: IWorkflowSettings; tags?: string[]; pinData?: IPinData; + hash?: string; } export interface IWorkflowDataUpdate { @@ -279,6 +280,7 @@ export interface IWorkflowDataUpdate { active?: boolean; tags?: ITag[] | string[]; // string[] when store or requested, ITag[] from API response pinData?: IPinData; + hash?: string; } export interface IWorkflowToShare extends IWorkflowDataUpdate { @@ -315,6 +317,7 @@ export interface IWorkflowDb { pinData?: IPinData; sharedWith?: Array>; ownedBy?: Partial; + hash?: string; } // Identical to cli.Interfaces.ts From 6e86bb37af86ff25b2c574d46e0c1f16b182d1b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:39:58 +0200 Subject: [PATCH 03/16] :zap: Adjust store --- packages/editor-ui/src/store.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/editor-ui/src/store.ts b/packages/editor-ui/src/store.ts index 4926e5fcc58f9..53556f30351f3 100644 --- a/packages/editor-ui/src/store.ts +++ b/packages/editor-ui/src/store.ts @@ -102,6 +102,7 @@ const state: IRootState = { settings: {}, tags: [], pinData: {}, + hash: '', }, workflowsById: {}, sidebarMenuItems: [], @@ -473,6 +474,10 @@ export const store = new Vuex.Store({ state.workflow.name = data.newName; }, + setWorkflowHash(state, hash: string) { + state.workflow.hash = hash; + }, + // replace invalid credentials in workflow replaceInvalidWorkflowCredentials(state, {credentials, invalid, type}) { state.workflow.nodes.forEach((node) => { @@ -761,6 +766,9 @@ export const store = new Vuex.Store({ subworkflowExecutionError: (state): Error | null => { return state.subworkflowExecutionError; }, + workflowHash: (state): string | undefined => { + return state.workflow.hash; + }, isActionActive: (state) => (action: string): boolean => { return state.activeActions.includes(action); From 335abd7b76a95bff5626622c963b3bf5d560dad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:40:17 +0200 Subject: [PATCH 04/16] :zap: Set received hash --- packages/editor-ui/src/views/NodeView.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/editor-ui/src/views/NodeView.vue b/packages/editor-ui/src/views/NodeView.vue index 8cdbde4c8a7b3..f0d78183ff7a0 100644 --- a/packages/editor-ui/src/views/NodeView.vue +++ b/packages/editor-ui/src/views/NodeView.vue @@ -718,6 +718,7 @@ export default mixins( this.$store.commit('setWorkflowName', { newName: data.name, setStateDirty: false }); this.$store.commit('setWorkflowSettings', data.settings || {}); this.$store.commit('setWorkflowPinData', data.pinData || {}); + this.$store.commit('setWorkflowHash', data.hash); const tags = (data.tags || []) as ITag[]; this.$store.commit('tags/upsertTags', tags); From 42a4c1409b868feb861dae5bba43201c57124e01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:40:42 +0200 Subject: [PATCH 05/16] :zap: Send and load hash --- packages/editor-ui/src/components/WorkflowSettings.vue | 4 +++- .../editor-ui/src/components/mixins/workflowHelpers.ts | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/editor-ui/src/components/WorkflowSettings.vue b/packages/editor-ui/src/components/WorkflowSettings.vue index 3f798e2546915..9a596a776fa8d 100644 --- a/packages/editor-ui/src/components/WorkflowSettings.vue +++ b/packages/editor-ui/src/components/WorkflowSettings.vue @@ -517,9 +517,11 @@ export default mixins( delete data.settings!.maxExecutionTimeout; this.isLoading = true; + data.hash = this.$store.getters.workflowHash; try { - await this.restApi().updateWorkflow(this.$route.params.name, data); + const workflow = await this.restApi().updateWorkflow(this.$route.params.name, data); + this.$store.commit('setWorkflowHash', workflow.hash); } catch (error) { this.$showError( error, diff --git a/packages/editor-ui/src/components/mixins/workflowHelpers.ts b/packages/editor-ui/src/components/mixins/workflowHelpers.ts index 15094047aacb5..74799767c5080 100644 --- a/packages/editor-ui/src/components/mixins/workflowHelpers.ts +++ b/packages/editor-ui/src/components/mixins/workflowHelpers.ts @@ -400,6 +400,7 @@ export const workflowHelpers = mixins( active: this.$store.getters.isActive, settings: this.$store.getters.workflowSettings, tags: this.$store.getters.workflowTags, + hash: this.$store.getters.workflowHash, }; const workflowId = this.$store.getters.workflowId; @@ -660,6 +661,9 @@ export const workflowHelpers = mixins( const isCurrentWorkflow = workflowId === this.$store.getters.workflowId; if (isCurrentWorkflow) { data = await this.getWorkflowDataToSave(); + } else { + const { hash } = await this.restApi().getWorkflow(workflowId); + data.hash = hash as string; } if (active !== undefined) { @@ -667,6 +671,7 @@ export const workflowHelpers = mixins( } const workflow = await this.restApi().updateWorkflow(workflowId, data); + this.$store.commit('setWorkflowHash', workflow.hash); if (isCurrentWorkflow) { this.$store.commit('setActive', !!workflow.active); @@ -700,7 +705,10 @@ export const workflowHelpers = mixins( workflowDataRequest.tags = tags; } + workflowDataRequest.hash = this.$store.getters.workflowHash; + const workflowData = await this.restApi().updateWorkflow(currentWorkflow, workflowDataRequest); + this.$store.commit('setWorkflowHash', workflowData.hash); if (name) { this.$store.commit('setWorkflowName', {newName: workflowData.name}); From a52929fe18b7ca4203467d0597a069975b6d18a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:40:54 +0200 Subject: [PATCH 06/16] :zap: Make helper more flexible --- packages/cli/test/integration/shared/utils.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/cli/test/integration/shared/utils.ts b/packages/cli/test/integration/shared/utils.ts index 39c81799a68f3..4f23843b91115 100644 --- a/packages/cli/test/integration/shared/utils.ts +++ b/packages/cli/test/integration/shared/utils.ts @@ -706,10 +706,7 @@ export const emptyPackage = () => { // workflow // ---------------------------------- -export function makeWorkflow({ - withPinData, - withCredential, -}: { +export function makeWorkflow(options?: { withPinData: boolean; withCredential?: { id: string; name: string }; }) { @@ -717,16 +714,16 @@ export function makeWorkflow({ const node: INode = { id: uuid(), - name: 'Spotify', - type: 'n8n-nodes-base.spotify', - parameters: { resource: 'track', operation: 'get', id: '123' }, + name: 'Cron', + type: 'n8n-nodes-base.cron', + parameters: {}, typeVersion: 1, position: [740, 240], }; - if (withCredential) { + if (options?.withCredential) { node.credentials = { - spotifyApi: withCredential, + spotifyApi: options.withCredential, }; } @@ -735,7 +732,7 @@ export function makeWorkflow({ workflow.connections = {}; workflow.nodes = [node]; - if (withPinData) { + if (options?.withPinData) { workflow.pinData = MOCK_PINDATA; } From 7aa6749eb989a37274ffcba5cff75ebdadc60fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:41:19 +0200 Subject: [PATCH 07/16] :card_file_box: Add new field to entity --- .../src/databases/entities/WorkflowEntity.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/cli/src/databases/entities/WorkflowEntity.ts b/packages/cli/src/databases/entities/WorkflowEntity.ts index d344c1dd25610..ffbe194fb72e9 100644 --- a/packages/cli/src/databases/entities/WorkflowEntity.ts +++ b/packages/cli/src/databases/entities/WorkflowEntity.ts @@ -1,3 +1,4 @@ +import crypto from 'crypto'; import { Length } from 'class-validator'; import type { @@ -10,6 +11,9 @@ import type { } from 'n8n-workflow'; import { + AfterLoad, + AfterUpdate, + AfterInsert, Column, Entity, Index, @@ -84,6 +88,30 @@ export class WorkflowEntity extends AbstractEntity implements IWorkflowDb { transformer: sqlite.jsonColumn, }) pinData: ISimplifiedPinData; + + /** + * Hash of editable workflow state. + */ + hash: string; + + @AfterLoad() + @AfterUpdate() + @AfterInsert() + setHash(): void { + const { name, active, nodes, connections, settings, staticData, pinData } = this; + + const state = JSON.stringify({ + name, + active, + nodes, + connections, + settings, + staticData, + pinData, + }); + + this.hash = crypto.createHash('md5').update(state).digest('hex'); + } } /** From f90f9ab5cc318f205ded37224123e0a46484bf24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:41:48 +0200 Subject: [PATCH 08/16] :rotating_light: Add check to endpoint --- packages/cli/src/workflows/workflows.controller.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index e6a2ea88abb26..62ace55510c54 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -331,7 +331,7 @@ workflowsController.patch( const { id: workflowId } = req.params; const updateData = new WorkflowEntity(); - const { tags, ...rest } = req.body; + const { tags, hash: incomingHash, ...rest } = req.body; Object.assign(updateData, rest); const shared = await Db.collections.SharedWorkflow.findOne({ @@ -355,6 +355,14 @@ workflowsController.patch( ); } + if (incomingHash !== shared.workflow.hash) { + throw new ResponseHelper.ResponseError( + `Workflow ID ${workflowId} cannot be saved because it was changed by another user.`, + undefined, + 400, + ); + } + // check credentials for old format await WorkflowHelpers.replaceInvalidCredentials(updateData); From c7103ee210f56d1e5fa24b242d593a9d980e17f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 26 Oct 2022 09:41:59 +0200 Subject: [PATCH 09/16] :test_tube: Add tests --- .../workflows.controller.ee.test.ts | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 30d304d89c377..cfdc251bc606c 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -11,6 +11,7 @@ import config from '../../config'; import type { AuthAgent, SaveCredentialFunction } from './shared/types'; import { makeWorkflow } from './shared/utils'; import { randomCredentialPayload } from './shared/random'; +import { ActiveWorkflowRunner } from '../../src'; jest.mock('../../src/telemetry'); @@ -25,6 +26,7 @@ let globalMemberRole: Role; let credentialOwnerRole: Role; let authAgent: AuthAgent; let saveCredential: SaveCredentialFunction; +let workflowRunner: ActiveWorkflowRunner.ActiveWorkflowRunner; beforeAll(async () => { app = await utils.initTestServer({ @@ -46,6 +48,9 @@ beforeAll(async () => { utils.initTestTelemetry(); config.set('enterprise.workflowSharingEnabled', true); + + await utils.initNodeTypes(); + workflowRunner = await utils.initActiveWorkflowRunner(); }); beforeEach(async () => { @@ -295,3 +300,149 @@ describe('POST /workflows', () => { expect(usedCredentials).toHaveLength(1); }); }); + +describe('PATCH /workflows/:id', () => { + it('should block owner update on interim update by member', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerHash } = createResponse.body.data; + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses and updates workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + + await authAgent(member) + .patch(`/workflows/${id}`) + .send({ name: 'Update by member', hash: memberHash }); + + // owner blocked from updating workflow + + const updateAttemptResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update attempt by owner', hash: ownerHash }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); + + it('should block member update on interim update by owner', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates, updates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerFirstHash } = createResponse.body.data; + + const updateResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update by owner', hash: ownerFirstHash }); + + const { hash: ownerSecondHash } = updateResponse.body.data; + + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + + // owner re-updates workflow + + await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Owner update again', hash: ownerSecondHash }); + + // member blocked from updating workflow + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ name: 'Update attempt by member', hash: memberHash }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); + + it('should block owner activation on interim activation by member', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerHash } = createResponse.body.data; + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses and activates workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + await authAgent(member).patch(`/workflows/${id}`).send({ active: true, hash: memberHash }); + + // owner blocked from activating workflow + + const activationAttemptResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ active: true, hash: ownerHash }); + + expect(activationAttemptResponse.status).toBe(400); + expect(activationAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); + + it('should block member activation on interim activation by owner', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates, updates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerFirstHash } = createResponse.body.data; + + const updateResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update by owner', hash: ownerFirstHash }); + const { hash: ownerSecondHash } = updateResponse.body.data; + + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + + // owner activates workflow + + await authAgent(owner).patch(`/workflows/${id}`).send({ active: true, hash: ownerSecondHash }); + + // member blocked from activating workflow + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ active: true, hash: memberHash }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); +}); From be5af42149c4e8e5e2fe41db7d2343f5f5ed16e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 11:09:19 +0200 Subject: [PATCH 10/16] :zap: Add `forceSave` flag --- packages/cli/src/requests.d.ts | 2 +- packages/cli/src/workflows/workflows.controller.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/requests.d.ts b/packages/cli/src/requests.d.ts index 0f18a25a33a88..022437f57ea90 100644 --- a/packages/cli/src/requests.d.ts +++ b/packages/cli/src/requests.d.ts @@ -57,7 +57,7 @@ export declare namespace WorkflowRequest { type Delete = Get; - type Update = AuthenticatedRequest<{ id: string }, {}, RequestBody>; + type Update = AuthenticatedRequest<{ id: string }, {}, RequestBody, { forceSave?: string }>; type NewName = AuthenticatedRequest<{}, {}, {}, { name?: string }>; diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 62ace55510c54..2b32d3d7f4cf0 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -329,6 +329,7 @@ workflowsController.patch( `/:id`, ResponseHelper.send(async (req: WorkflowRequest.Update) => { const { id: workflowId } = req.params; + const { forceSave } = req.query; const updateData = new WorkflowEntity(); const { tags, hash: incomingHash, ...rest } = req.body; @@ -355,7 +356,7 @@ workflowsController.patch( ); } - if (incomingHash !== shared.workflow.hash) { + if (!forceSave && incomingHash !== shared.workflow.hash) { throw new ResponseHelper.ResponseError( `Workflow ID ${workflowId} cannot be saved because it was changed by another user.`, undefined, From 4cdbb49ad0e69dd1241f4cb271b5c1754208cffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 11:09:48 +0200 Subject: [PATCH 11/16] :bug: Fix workflow update failing on new workflow --- packages/editor-ui/src/components/mixins/workflowHelpers.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/editor-ui/src/components/mixins/workflowHelpers.ts b/packages/editor-ui/src/components/mixins/workflowHelpers.ts index 74799767c5080..786853acdd488 100644 --- a/packages/editor-ui/src/components/mixins/workflowHelpers.ts +++ b/packages/editor-ui/src/components/mixins/workflowHelpers.ts @@ -775,6 +775,7 @@ export const workflowHelpers = mixins( const workflowData = await this.restApi().createNewWorkflow(workflowDataRequest); this.$store.commit('addWorkflow', workflowData); + this.$store.commit('setWorkflowHash', workflowData.hash); if (openInNewWindow) { const routeData = this.$router.resolve({name: VIEWS.WORKFLOW, params: {name: workflowData.id}}); From 3393015b44ddabf23852a8936e1ad4f76b83f421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 11:09:56 +0200 Subject: [PATCH 12/16] :test_tube: Add more tests --- .../workflows.controller.ee.test.ts | 82 +++++++++++++++++-- 1 file changed, 76 insertions(+), 6 deletions(-) diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index cfdc251bc606c..4b5e478e17303 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -302,7 +302,7 @@ describe('POST /workflows', () => { }); describe('PATCH /workflows/:id', () => { - it('should block owner update on interim update by member', async () => { + it('should block owner updating workflow nodes on interim update by member', async () => { const owner = await testDb.createUser({ globalRole: globalOwnerRole }); const member = await testDb.createUser({ globalRole: globalMemberRole }); @@ -314,7 +314,7 @@ describe('PATCH /workflows/:id', () => { .put(`/workflows/${id}/share`) .send({ shareWithIds: [member.id] }); - // member accesses and updates workflow + // member accesses and updates workflow name const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); const { hash: memberHash } = memberGetResponse.body.data; @@ -323,11 +323,11 @@ describe('PATCH /workflows/:id', () => { .patch(`/workflows/${id}`) .send({ name: 'Update by member', hash: memberHash }); - // owner blocked from updating workflow + // owner blocked from updating workflow nodes const updateAttemptResponse = await authAgent(owner) .patch(`/workflows/${id}`) - .send({ name: 'Update attempt by owner', hash: ownerHash }); + .send({ nodes: [], hash: ownerHash }); expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( @@ -335,7 +335,7 @@ describe('PATCH /workflows/:id', () => { ); }); - it('should block member update on interim update by owner', async () => { + it('should block member updating workflow nodes on interim update by owner', async () => { const owner = await testDb.createUser({ globalRole: globalOwnerRole }); const member = await testDb.createUser({ globalRole: globalMemberRole }); @@ -369,7 +369,7 @@ describe('PATCH /workflows/:id', () => { const updateAttemptResponse = await authAgent(member) .patch(`/workflows/${id}`) - .send({ name: 'Update attempt by member', hash: memberHash }); + .send({ nodes: [], hash: memberHash }); expect(updateAttemptResponse.status).toBe(400); expect(updateAttemptResponse.body.message).toContain( @@ -445,4 +445,74 @@ describe('PATCH /workflows/:id', () => { 'cannot be saved because it was changed by another user', ); }); + + it('should block member updating workflow settings on interim update by owner', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerHash } = createResponse.body.data; + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + + // owner updates workflow name + + await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Another name', hash: ownerHash }); + + // member blocked from updating workflow settings + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ settings: { saveManualExecutions: true }, hash: memberHash }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); + + it('should block member updating workflow name on interim update by owner', async () => { + const owner = await testDb.createUser({ globalRole: globalOwnerRole }); + const member = await testDb.createUser({ globalRole: globalMemberRole }); + + // owner creates and shares workflow + + const createResponse = await authAgent(owner).post('/workflows').send(makeWorkflow()); + const { id, hash: ownerHash } = createResponse.body.data; + await authAgent(owner) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member.id] }); + + // member accesses workflow + + const memberGetResponse = await authAgent(member).get(`/workflows/${id}`); + const { hash: memberHash } = memberGetResponse.body.data; + + // owner updates workflow settings + + await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ settings: { saveManualExecutions: true }, hash: ownerHash }); + + // member blocked from updating workflow name + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ settings: { saveManualExecutions: true }, hash: memberHash }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); }); From a3a225bc18d6b396db57c2528d49853abdc8e90a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 11:34:00 +0200 Subject: [PATCH 13/16] :zap: Move check to `updateWorkflow()` --- packages/cli/src/workflows/workflows.services.ee.ts | 5 +++-- packages/cli/src/workflows/workflows.services.ts | 9 +++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index 5ef6327703576..7bcd7c64cfc51 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -121,6 +121,7 @@ export class EEWorkflowsService extends WorkflowsService { workflow: WorkflowEntity, workflowId: string, tags?: string[], + forceSave?: boolean, ): Promise { const previousVersion = await EEWorkflowsService.get({ id: parseInt(workflowId, 10) }); if (!previousVersion) { @@ -128,13 +129,13 @@ export class EEWorkflowsService extends WorkflowsService { } const allCredentials = await EECredentials.getAll(user); try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call workflow = WorkflowHelpers.validateWorkflowCredentialUsage( workflow, previousVersion, allCredentials, ); } catch (error) { - console.log(error); throw new ResponseHelper.ResponseError( 'Invalid workflow credentials - make sure you have access to all credentials and try again.', undefined, @@ -142,6 +143,6 @@ export class EEWorkflowsService extends WorkflowsService { ); } - return super.updateWorkflow(user, workflow, workflowId, tags); + return super.updateWorkflow(user, workflow, workflowId, tags, forceSave); } } diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 9e7890e902c04..290701f4d1ee1 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -52,6 +52,7 @@ export class WorkflowsService { workflow: WorkflowEntity, workflowId: string, tags?: string[], + forceSave?: boolean, ): Promise { const shared = await Db.collections.SharedWorkflow.findOne({ relations: ['workflow'], @@ -74,6 +75,14 @@ export class WorkflowsService { ); } + if (!forceSave && workflow.hash !== shared.workflow.hash) { + throw new ResponseHelper.ResponseError( + `Workflow ID ${workflowId} cannot be saved because it was changed by another user.`, + undefined, + 400, + ); + } + // check credentials for old format await WorkflowHelpers.replaceInvalidCredentials(workflow); From 99e49e37cd17ab9add215765814a23263430da24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 12:28:52 +0200 Subject: [PATCH 14/16] :zap: Refactor to accommodate latest changes --- packages/cli/src/workflows/workflows.controller.ee.ts | 2 ++ packages/cli/src/workflows/workflows.controller.ts | 4 +--- packages/cli/src/workflows/workflows.services.ts | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index 6ecb04c85c69d..d43604b8aa25f 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -183,6 +183,7 @@ EEWorkflowController.patch( '/:id(\\d+)', ResponseHelper.send(async (req: WorkflowRequest.Update) => { const { id: workflowId } = req.params; + const forceSave = Boolean(req.query.forceSave); const updateData = new WorkflowEntity(); const { tags, ...rest } = req.body; @@ -193,6 +194,7 @@ EEWorkflowController.patch( updateData, workflowId, tags, + forceSave, ); const { id, ...remainder } = updatedWorkflow; diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index b52750e83b02b..5a36d6c65033f 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -360,10 +360,9 @@ workflowsController.patch( `/:id`, ResponseHelper.send(async (req: WorkflowRequest.Update) => { const { id: workflowId } = req.params; - const forceSave = Boolean(req.query.forceSave); const updateData = new WorkflowEntity(); - const { tags, hash: incomingHash, ...rest } = req.body; + const { tags, ...rest } = req.body; Object.assign(updateData, rest); const updatedWorkflow = await WorkflowsService.updateWorkflow( @@ -371,7 +370,6 @@ workflowsController.patch( updateData, workflowId, tags, - forceSave, ); const { id, ...remainder } = updatedWorkflow; diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 290701f4d1ee1..e904299e4a99c 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -127,7 +127,9 @@ export class WorkflowsService { await validateEntity(workflow); } - await Db.collections.Workflow.update(workflowId, workflow); + const { hash, ...rest } = workflow; + + await Db.collections.Workflow.update(workflowId, rest); if (tags && !config.getEnv('workflowTagsDisabled')) { const tablePrefix = config.getEnv('database.tablePrefix'); From 7f97bdb1bc32ab7331a38f05cca24f8d4acdb06d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 12:29:03 +0200 Subject: [PATCH 15/16] :test_tube: Refactor tests to keep them passing --- .../workflows.controller.ee.test.ts | 123 +++++++++++++----- 1 file changed, 94 insertions(+), 29 deletions(-) diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 35ef10f402e0f..e6035442866e7 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -292,35 +292,39 @@ describe('POST /workflows', () => { }); }); -describe('PATCH /workflows/:id', () => { +describe('PATCH /workflows/:id - validate credential permissions to user', () => { it('Should succeed when saving unchanged workflow nodes', async () => { const owner = await testDb.createUser({ globalRole: globalOwnerRole }); const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); - const workflow = await createWorkflow( - { - nodes: [ - { - id: 'uuid-1234', - name: 'Start', - parameters: {}, - position: [-20, 260], - type: 'n8n-nodes-base.start', - typeVersion: 1, - credentials: { - default: { - id: savedCredential.id.toString(), - name: savedCredential.name, - }, + const workflow = { + name: 'test', + active: false, + connections: {}, + nodes: [ + { + id: 'uuid-1234', + name: 'Start', + parameters: {}, + position: [-20, 260], + type: 'n8n-nodes-base.start', + typeVersion: 1, + credentials: { + default: { + id: savedCredential.id.toString(), + name: savedCredential.name, }, }, - ], - }, - owner, - ); + }, + ], + }; + + const createResponse = await authAgent(owner).post('/workflows').send(workflow); + const { id, hash } = createResponse.body.data; - const response = await authAgent(owner).patch(`/workflows/${workflow.id}`).send({ + const response = await authAgent(owner).patch(`/workflows/${id}`).send({ name: 'new name', + hash, }); expect(response.statusCode).toBe(200); @@ -331,11 +335,35 @@ describe('PATCH /workflows/:id', () => { const member = await testDb.createUser({ globalRole: globalMemberRole }); const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); - const workflow = await createWorkflow({}, owner); + const workflow = { + name: 'test', + active: false, + connections: {}, + nodes: [ + { + id: 'uuid-1234', + name: 'Start', + parameters: {}, + position: [-20, 260], + type: 'n8n-nodes-base.start', + typeVersion: 1, + credentials: { + default: { + id: savedCredential.id.toString(), + name: savedCredential.name, + }, + }, + }, + ], + }; + + const createResponse = await authAgent(owner).post('/workflows').send(workflow); + const { id, hash } = createResponse.body.data; const response = await authAgent(owner) - .patch(`/workflows/${workflow.id}`) + .patch(`/workflows/${id}`) .send({ + hash, nodes: [ { id: 'uuid-1234', @@ -362,11 +390,36 @@ describe('PATCH /workflows/:id', () => { const member = await testDb.createUser({ globalRole: globalMemberRole }); const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); - const workflow = await createWorkflow({}, member); + + const workflow = { + name: 'test', + active: false, + connections: {}, + nodes: [ + { + id: 'uuid-1234', + name: 'Start', + parameters: {}, + position: [-20, 260], + type: 'n8n-nodes-base.start', + typeVersion: 1, + credentials: { + default: { + id: savedCredential.id.toString(), + name: savedCredential.name, + }, + }, + }, + ], + }; + + const createResponse = await authAgent(owner).post('/workflows').send(workflow); + const { id, hash } = createResponse.body.data; const response = await authAgent(member) - .patch(`/workflows/${workflow.id}`) + .patch(`/workflows/${id}`) .send({ + hash, nodes: [ { id: 'uuid-1234', @@ -437,10 +490,22 @@ describe('PATCH /workflows/:id', () => { }, ]; - const workflow = await createWorkflow({ nodes: originalNodes }, member1); - await testDb.shareWorkflowWithUsers(workflow, [member2]); + const workflow = { + name: 'test', + active: false, + connections: {}, + nodes: originalNodes, + }; + + const createResponse = await authAgent(member1).post('/workflows').send(workflow); + const { id, hash } = createResponse.body.data; + + await authAgent(member1) + .put(`/workflows/${id}/share`) + .send({ shareWithIds: [member2.id] }); - const response = await authAgent(member2).patch(`/workflows/${workflow.id}`).send({ + const response = await authAgent(member2).patch(`/workflows/${id}`).send({ + hash, nodes: changedNodes, }); @@ -449,7 +514,7 @@ describe('PATCH /workflows/:id', () => { }); }); -describe('PATCH /workflows/:id', () => { +describe('PATCH /workflows/:id - validate interim updates', () => { it('should block owner updating workflow nodes on interim update by member', async () => { const owner = await testDb.createUser({ globalRole: globalOwnerRole }); const member = await testDb.createUser({ globalRole: globalMemberRole }); From 7790b73d7ee336424f041bdc0512fec123f945cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 27 Oct 2022 13:24:16 +0200 Subject: [PATCH 16/16] :zap: Improve syntax --- packages/cli/src/workflows/workflows.controller.ee.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index d43604b8aa25f..5955bd20a6f26 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -183,7 +183,7 @@ EEWorkflowController.patch( '/:id(\\d+)', ResponseHelper.send(async (req: WorkflowRequest.Update) => { const { id: workflowId } = req.params; - const forceSave = Boolean(req.query.forceSave); + const forceSave = req.query.forceSave === 'true'; const updateData = new WorkflowEntity(); const { tags, ...rest } = req.body;