feat(core): block workflow update on interim change (n8n-io#4374)

* ⚡ Add `updatedAt` to store * ⚡ Set `updatedAt` in store * 👕 Update FE types * 👕 Update BE types * ⚡ Set `updatedAt` on workflow open * ⚡ Add endpoint check * ⚡ Add first update check * 🔥 Remove log * ⚡ Simplify check * ⚡ Make `makeWorkflow` more flexible * 🗃️ Make `updatedAt` default consistent * 🧪 Adjust tests checking for `updatedAt` * 🧪 Add tests for interim changes block * ✏️ Remove unneeded quotes * ⏪ Simplify without using `-1` * 👕 Simplify interfaces
valya · Oct 20, 2022 · e83b9bd · e83b9bd
1 parent 2f87b9f
commit e83b9bd
Show file tree

Hide file tree

Showing 8 changed files with 116 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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
@@ -294,4 +294,78 @@ describe('POST /workflows', () => {
 		const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
 		expect(usedCredentials).toHaveLength(1);
 	});
+
+	it('PATCH /workflows/:id should be blocked on interim change - owner blocked', 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('PATCH /workflows/:id should be blocked on interim change - member blocked', 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',
+		);
+	});
 });
diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts
@@ -241,6 +241,7 @@ export interface IWorkflowData {
 	settings?: IWorkflowSettings;
 	tags?: string[];
 	pinData?: IPinData;
+	updatedAt?: string;
 }
 
 export interface IWorkflowDataUpdate {
@@ -252,6 +253,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/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,
+					updatedAt: this.$store.getters.workflowUpdatedAt,
 				};
 
 				const workflowId = this.$store.getters.workflowId;
@@ -678,6 +679,8 @@ export const workflowHelpers = mixins(
 				} else {
 					this.$store.commit('setWorkflowInactive', workflowId);
 				}
+
+				this.$store.commit('setWorkflowUpdatedAt', data.updatedAt);
 			},
 
 			async saveCurrentWorkflow({name, tags}: {name?: string, tags?: string[]} = {}, redirect = true): Promise<boolean> {
@@ -714,6 +717,7 @@ export const workflowHelpers = mixins(
 
 					this.$store.commit('setStateDirty', false);
 					this.$store.commit('removeActiveAction', 'workflowSaving');
+					this.$store.commit('setWorkflowUpdatedAt', workflowData.updatedAt);
 					this.$externalHooks().run('workflow.afterUpdate', { workflowData });
 
 					return true;

diff --git a/packages/editor-ui/src/store.ts b/packages/editor-ui/src/store.ts
@@ -464,6 +464,11 @@ export const store = new Vuex.Store({
 			state.workflow.id = id;
 		},
 
+		// updatedAt
+		setWorkflowUpdatedAt (state, updatedAt: string) {
+			state.workflow.updatedAt = updatedAt;
+		},
+
 		// Name
 		setWorkflowName(state, data) {
 			if (data.setStateDirty === true) {
@@ -1009,6 +1014,9 @@ export const store = new Vuex.Store({
 		workflowId: (state): string => {
 			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
@@ -735,6 +735,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 || {});