Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Oct 20, 2022
Merged
7 changes: 6 additions & 1 deletion packages/cli/src/requests.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }>;

Expand Down
17 changes: 17 additions & 0 deletions packages/cli/src/workflows/workflows.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand Down
11 changes: 4 additions & 7 deletions packages/cli/test/integration/shared/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -706,10 +706,7 @@ export const emptyPackage = () => {
// workflow
// ----------------------------------

export function makeWorkflow({
withPinData,
withCredential,
}: {
export function makeWorkflow(options?: {
withPinData: boolean;
withCredential?: { id: string; name: string };
}) {
Expand All @@ -724,9 +721,9 @@ export function makeWorkflow({
position: [740, 240],
};

if (withCredential) {
if (options?.withCredential) {
node.credentials = {
spotifyApi: withCredential,
spotifyApi: options.withCredential,
};
}

Expand All @@ -735,7 +732,7 @@ export function makeWorkflow({
workflow.connections = {};
workflow.nodes = [node];

if (withPinData) {
if (options?.withPinData) {
workflow.pinData = MOCK_PINDATA;
}

Expand Down
74 changes: 74 additions & 0 deletions packages/cli/test/integration/workflows.controller.ee.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
});
});
2 changes: 2 additions & 0 deletions packages/editor-ui/src/Interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ export interface IWorkflowData {
settings?: IWorkflowSettings;
tags?: string[];
pinData?: IPinData;
updatedAt?: string;
}

export interface IWorkflowDataUpdate {
Expand All @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions packages/editor-ui/src/components/mixins/workflowHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions packages/editor-ui/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions packages/editor-ui/src/views/NodeView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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 || {});
Expand Down