From cddd012a2fa834ca6b3d85296caadcf3b8cc7bb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 25 Oct 2022 09:08:06 +0200 Subject: [PATCH] feat(core): block workflow update on interim change (#4397) * :zap: Add `updatedAt` to store * :zap: Set `updatedAt` in store * :shirt: Update FE types * :shirt: Update BE types * :zap: Set `updatedAt` on workflow open * :zap: Add endpoint check * :zap: Add first update check * :fire: Remove log * :zap: Simplify check * :zap: Make `makeWorkflow` more flexible * :card_file_box: Make `updatedAt` default consistent * :test_tube: Adjust tests checking for `updatedAt` * :test_tube: Add tests for interim changes block * :pencil2: Remove unneeded quotes * :rewind: Simplify without using `-1` * :shirt: Simplify interfaces * :bug: Fix calls to `setWorkflowUpdatedAt` setter * :track: Move update to API call * :zap: Restrict check to multiple users only * :test_tube: Add more tests * :bug: Account for activation outside of canvas * :pencil2: Add warning comment * :fire: Remove unneeded check * :rewind: Revert to `new Date()` for `-1` * :bug: Fix display for never updated --- packages/cli/src/requests.d.ts | 7 +- .../cli/src/workflows/workflows.controller.ts | 17 ++ packages/cli/test/integration/shared/utils.ts | 11 +- .../workflows.controller.ee.test.ts | 150 ++++++++++++++++++ packages/editor-ui/src/Interface.ts | 2 + .../editor-ui/src/components/WorkflowCard.vue | 10 +- .../src/components/WorkflowSettings.vue | 3 +- .../src/components/mixins/workflowHelpers.ts | 6 + .../src/plugins/i18n/locales/en.json | 1 + packages/editor-ui/src/store.ts | 10 ++ packages/editor-ui/src/views/NodeView.vue | 1 + 11 files changed, 208 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/requests.d.ts b/packages/cli/src/requests.d.ts index 53ad893ec6ab3..6c077f958f448 100644 --- a/packages/cli/src/requests.d.ts +++ b/packages/cli/src/requests.d.ts @@ -56,7 +56,12 @@ export declare namespace WorkflowRequest { type Delete = Get; - type Update = AuthenticatedRequest<{ id: string }, {}, RequestBody>; + type Update = AuthenticatedRequest< + { id: string }, + {}, + RequestBody & { updatedAt: string }, + { 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 e6a2ea88abb26..4fb03ead89a0d 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, ...rest } = req.body; @@ -355,6 +356,22 @@ workflowsController.patch( ); } + const lastKnownDate = new Date(req.body.updatedAt).getTime(); + const storedDate = new Date(shared.workflow.updatedAt).getTime(); + + if (!forceSave && lastKnownDate !== storedDate) { + LoggerProxy.info( + 'User was blocked from updating a workflow that was changed by another user', + { workflowId, userId: req.user.id }, + ); + + 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); diff --git a/packages/cli/test/integration/shared/utils.ts b/packages/cli/test/integration/shared/utils.ts index 39c81799a68f3..223050082c6b7 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 }; }) { @@ -724,9 +721,9 @@ export function makeWorkflow({ 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; } diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 30d304d89c377..758a09164b540 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -295,3 +295,153 @@ 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, updatedAt: ownerLastKnownDate } = 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 { updatedAt: memberLastKnownDate } = memberGetResponse.body.data; + + await authAgent(member) + .patch(`/workflows/${id}`) + .send({ name: 'Update by member', updatedAt: memberLastKnownDate }); + + // owner blocked from updating workflow + + const updateAttemptResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update attempt by owner', updatedAt: ownerLastKnownDate }); + + 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, updatedAt: ownerFirstUpdateDate } = createResponse.body.data; + + const updateResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update by owner', updatedAt: ownerFirstUpdateDate }); + const { updatedAt: ownerSecondUpdateDate } = 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 { updatedAt: memberLastKnownDate } = memberGetResponse.body.data; + + // owner re-updates workflow + + await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Owner update again', updatedAt: ownerSecondUpdateDate }); + + // member blocked from updating workflow + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ name: 'Update attempt by member', updatedAt: memberLastKnownDate }); + + 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, updatedAt: ownerLastKnownDate } = 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 { updatedAt: memberLastKnownDate } = memberGetResponse.body.data; + + await authAgent(member) + .patch(`/workflows/${id}`) + .send({ active: true, updatedAt: memberLastKnownDate }); + + // owner blocked from activating workflow + + const activationAttemptResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ active: true, updatedAt: ownerLastKnownDate }); + + 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, updatedAt: ownerFirstUpdateDate } = createResponse.body.data; + + const updateResponse = await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ name: 'Update by owner', updatedAt: ownerFirstUpdateDate }); + const { updatedAt: ownerSecondUpdateDate } = 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 { updatedAt: memberLastKnownDate } = memberGetResponse.body.data; + + // owner activates workflow + + await authAgent(owner) + .patch(`/workflows/${id}`) + .send({ active: true, updatedAt: ownerSecondUpdateDate }); + + // member blocked from activating workflow + + const updateAttemptResponse = await authAgent(member) + .patch(`/workflows/${id}`) + .send({ active: true, updatedAt: memberLastKnownDate }); + + expect(updateAttemptResponse.status).toBe(400); + expect(updateAttemptResponse.body.message).toContain( + 'cannot be saved because it was changed by another user', + ); + }); +}); diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index 4613303b1caf7..7ea10e7e1381e 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; + updatedAt?: 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; + updatedAt?: string; } export interface IWorkflowToShare extends IWorkflowDataUpdate { diff --git a/packages/editor-ui/src/components/WorkflowCard.vue b/packages/editor-ui/src/components/WorkflowCard.vue index 4af6ba0336e8b..0b76843a5aa10 100644 --- a/packages/editor-ui/src/components/WorkflowCard.vue +++ b/packages/editor-ui/src/components/WorkflowCard.vue @@ -10,7 +10,15 @@
- {{$locale.baseText('workflows.item.updated')}} | + + + {{ $locale.baseText('workflows.item.neverUpdated') }} | + + + {{ $locale.baseText('workflows.item.updated') }} + | + + {{$locale.baseText('workflows.item.created')}} {{ formattedCreatedAtDate }} { return state.workflow.id; }, + workflowUpdatedAt (state): string | number { + return state.workflow.updatedAt; + }, workflowSettings: (state): IWorkflowSettings => { if (state.workflow.settings === undefined) { diff --git a/packages/editor-ui/src/views/NodeView.vue b/packages/editor-ui/src/views/NodeView.vue index 8cdbde4c8a7b3..7d42d6fc61260 100644 --- a/packages/editor-ui/src/views/NodeView.vue +++ b/packages/editor-ui/src/views/NodeView.vue @@ -715,6 +715,7 @@ export default mixins( this.$store.commit('setActive', data.active || false); this.$store.commit('setWorkflowId', workflowId); + this.$store.commit('setWorkflowUpdatedAt', data.updatedAt); this.$store.commit('setWorkflowName', { newName: data.name, setStateDirty: false }); this.$store.commit('setWorkflowSettings', data.settings || {}); this.$store.commit('setWorkflowPinData', data.pinData || {});