feat(core): reimplement blocking workflow updates on interim changes (#4446)

* 📘 Update request type

* 📘 Update FE types

*  Adjust store

*  Set received hash

*  Send and load hash

*  Make helper more flexible

* 🗃️ Add new field to entity

* 🚨 Add check to endpoint

* 🧪 Add tests

*  Add `forceSave` flag

* 🐛 Fix workflow update failing on new workflow

* 🧪 Add more tests

*  Move check to `updateWorkflow()`

*  Refactor to accommodate latest changes

* 🧪 Refactor tests to keep them passing

*  Improve syntax
This commit is contained in:
Iván Ovejero 2022-10-31 10:35:24 +01:00 committed by GitHub
parent 263e6f30da
commit 46905fd2cb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 394 additions and 44 deletions

View file

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

View file

@ -48,6 +48,7 @@ export declare namespace WorkflowRequest {
settings: IWorkflowSettings;
active: boolean;
tags: string[];
hash: string;
}>;
type Create = AuthenticatedRequest<{}, {}, RequestBody>;
@ -56,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 }>;

View file

@ -183,6 +183,7 @@ EEWorkflowController.patch(
'/:id(\\d+)',
ResponseHelper.send(async (req: WorkflowRequest.Update) => {
const { id: workflowId } = req.params;
const forceSave = req.query.forceSave === 'true';
const updateData = new WorkflowEntity();
const { tags, ...rest } = req.body;
@ -193,6 +194,7 @@ EEWorkflowController.patch(
updateData,
workflowId,
tags,
forceSave,
);
const { id, ...remainder } = updatedWorkflow;

View file

@ -121,6 +121,7 @@ export class EEWorkflowsService extends WorkflowsService {
workflow: WorkflowEntity,
workflowId: string,
tags?: string[],
forceSave?: boolean,
): Promise<WorkflowEntity> {
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);
}
}

View file

@ -52,6 +52,7 @@ export class WorkflowsService {
workflow: WorkflowEntity,
workflowId: string,
tags?: string[],
forceSave?: boolean,
): Promise<WorkflowEntity> {
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);
@ -118,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');

View file

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

View file

@ -11,7 +11,8 @@ import config from '../../config';
import type { AuthAgent, SaveCredentialFunction } from './shared/types';
import { makeWorkflow } from './shared/utils';
import { randomCredentialPayload } from './shared/random';
import { INode, INodes } from 'n8n-workflow';
import { ActiveWorkflowRunner } from '../../src';
import { INode } from 'n8n-workflow';
jest.mock('../../src/telemetry');
@ -26,6 +27,7 @@ let globalMemberRole: Role;
let credentialOwnerRole: Role;
let authAgent: AuthAgent;
let saveCredential: SaveCredentialFunction;
let workflowRunner: ActiveWorkflowRunner.ActiveWorkflowRunner;
beforeAll(async () => {
app = await utils.initTestServer({
@ -47,6 +49,9 @@ beforeAll(async () => {
utils.initTestTelemetry();
config.set('enterprise.workflowSharingEnabled', true);
await utils.initNodeTypes();
workflowRunner = await utils.initActiveWorkflowRunner();
});
beforeEach(async () => {
@ -287,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 response = await authAgent(owner).patch(`/workflows/${workflow.id}`).send({
const createResponse = await authAgent(owner).post('/workflows').send(workflow);
const { id, hash } = createResponse.body.data;
const response = await authAgent(owner).patch(`/workflows/${id}`).send({
name: 'new name',
hash,
});
expect(response.statusCode).toBe(200);
@ -326,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',
@ -357,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',
@ -432,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 response = await authAgent(member2).patch(`/workflows/${workflow.id}`).send({
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/${id}`).send({
hash,
nodes: changedNodes,
});
@ -443,3 +513,219 @@ describe('PATCH /workflows/:id', () => {
expect(response.body.data.nodes).toMatchObject(originalNodes);
});
});
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 });
// 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 name
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 nodes
const updateAttemptResponse = await authAgent(owner)
.patch(`/workflows/${id}`)
.send({ nodes: [], 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 updating workflow nodes 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({ nodes: [], 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',
);
});
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',
);
});
});

View file

@ -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<Partial<IUser>>;
ownedBy?: Partial<IUser>;
hash?: string;
}
// Identical to cli.Interfaces.ts

View file

@ -589,9 +589,11 @@ export default mixins(
delete data.settings!.maxExecutionTimeout;
this.isLoading = true;
data.hash = this.$store.getters.workflowHash;
try {
await this.restApi().updateWorkflow(this.workflowId, data);
const workflow = await this.restApi().updateWorkflow(this.$route.params.name, data);
this.$store.commit('setWorkflowHash', workflow.hash);
} catch (error) {
this.$showError(
error,

View file

@ -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);
@ -701,7 +706,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});
@ -768,6 +776,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}});

View file

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

View file

@ -793,6 +793,8 @@ 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);
const tagIds = tags.map((tag) => tag.id);