feat(core): block workflow update on interim change (#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
This commit is contained in:
Iván Ovejero 2022-10-20 15:30:44 +02:00 committed by GitHub
parent 2f87b9fbf6
commit e83b9bd983
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 116 additions and 8 deletions

View file

@ -56,7 +56,12 @@ export declare namespace WorkflowRequest {
type Delete = Get; type Delete = Get;
type Update = AuthenticatedRequest<{ id: string }, {}, RequestBody>; type Update = AuthenticatedRequest<
{ id: string },
{},
RequestBody & { updatedAt: string },
{ forceSave?: string }
>;
type NewName = AuthenticatedRequest<{}, {}, {}, { name?: string }>; type NewName = AuthenticatedRequest<{}, {}, {}, { name?: string }>;

View file

@ -329,6 +329,7 @@ workflowsController.patch(
`/:id`, `/:id`,
ResponseHelper.send(async (req: WorkflowRequest.Update) => { ResponseHelper.send(async (req: WorkflowRequest.Update) => {
const { id: workflowId } = req.params; const { id: workflowId } = req.params;
const { forceSave } = req.query;
const updateData = new WorkflowEntity(); const updateData = new WorkflowEntity();
const { tags, ...rest } = req.body; 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 // check credentials for old format
await WorkflowHelpers.replaceInvalidCredentials(updateData); await WorkflowHelpers.replaceInvalidCredentials(updateData);

View file

@ -706,10 +706,7 @@ export const emptyPackage = () => {
// workflow // workflow
// ---------------------------------- // ----------------------------------
export function makeWorkflow({ export function makeWorkflow(options?: {
withPinData,
withCredential,
}: {
withPinData: boolean; withPinData: boolean;
withCredential?: { id: string; name: string }; withCredential?: { id: string; name: string };
}) { }) {
@ -724,9 +721,9 @@ export function makeWorkflow({
position: [740, 240], position: [740, 240],
}; };
if (withCredential) { if (options?.withCredential) {
node.credentials = { node.credentials = {
spotifyApi: withCredential, spotifyApi: options.withCredential,
}; };
} }
@ -735,7 +732,7 @@ export function makeWorkflow({
workflow.connections = {}; workflow.connections = {};
workflow.nodes = [node]; workflow.nodes = [node];
if (withPinData) { if (options?.withPinData) {
workflow.pinData = MOCK_PINDATA; workflow.pinData = MOCK_PINDATA;
} }

View file

@ -294,4 +294,78 @@ describe('POST /workflows', () => {
const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id); const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
expect(usedCredentials).toHaveLength(1); 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',
);
});
}); });

View file

@ -241,6 +241,7 @@ export interface IWorkflowData {
settings?: IWorkflowSettings; settings?: IWorkflowSettings;
tags?: string[]; tags?: string[];
pinData?: IPinData; pinData?: IPinData;
updatedAt?: string;
} }
export interface IWorkflowDataUpdate { export interface IWorkflowDataUpdate {
@ -252,6 +253,7 @@ export interface IWorkflowDataUpdate {
active?: boolean; active?: boolean;
tags?: ITag[] | string[]; // string[] when store or requested, ITag[] from API response tags?: ITag[] | string[]; // string[] when store or requested, ITag[] from API response
pinData?: IPinData; pinData?: IPinData;
updatedAt?: string;
} }
export interface IWorkflowToShare extends IWorkflowDataUpdate { export interface IWorkflowToShare extends IWorkflowDataUpdate {

View file

@ -400,6 +400,7 @@ export const workflowHelpers = mixins(
active: this.$store.getters.isActive, active: this.$store.getters.isActive,
settings: this.$store.getters.workflowSettings, settings: this.$store.getters.workflowSettings,
tags: this.$store.getters.workflowTags, tags: this.$store.getters.workflowTags,
updatedAt: this.$store.getters.workflowUpdatedAt,
}; };
const workflowId = this.$store.getters.workflowId; const workflowId = this.$store.getters.workflowId;
@ -678,6 +679,8 @@ export const workflowHelpers = mixins(
} else { } else {
this.$store.commit('setWorkflowInactive', workflowId); this.$store.commit('setWorkflowInactive', workflowId);
} }
this.$store.commit('setWorkflowUpdatedAt', data.updatedAt);
}, },
async saveCurrentWorkflow({name, tags}: {name?: string, tags?: string[]} = {}, redirect = true): Promise<boolean> { 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('setStateDirty', false);
this.$store.commit('removeActiveAction', 'workflowSaving'); this.$store.commit('removeActiveAction', 'workflowSaving');
this.$store.commit('setWorkflowUpdatedAt', workflowData.updatedAt);
this.$externalHooks().run('workflow.afterUpdate', { workflowData }); this.$externalHooks().run('workflow.afterUpdate', { workflowData });
return true; return true;

View file

@ -464,6 +464,11 @@ export const store = new Vuex.Store({
state.workflow.id = id; state.workflow.id = id;
}, },
// updatedAt
setWorkflowUpdatedAt (state, updatedAt: string) {
state.workflow.updatedAt = updatedAt;
},
// Name // Name
setWorkflowName(state, data) { setWorkflowName(state, data) {
if (data.setStateDirty === true) { if (data.setStateDirty === true) {
@ -1009,6 +1014,9 @@ export const store = new Vuex.Store({
workflowId: (state): string => { workflowId: (state): string => {
return state.workflow.id; return state.workflow.id;
}, },
workflowUpdatedAt (state): string | number {
return state.workflow.updatedAt;
},
workflowSettings: (state): IWorkflowSettings => { workflowSettings: (state): IWorkflowSettings => {
if (state.workflow.settings === undefined) { if (state.workflow.settings === undefined) {

View file

@ -735,6 +735,7 @@ export default mixins(
this.$store.commit('setActive', data.active || false); this.$store.commit('setActive', data.active || false);
this.$store.commit('setWorkflowId', workflowId); this.$store.commit('setWorkflowId', workflowId);
this.$store.commit('setWorkflowUpdatedAt', data.updatedAt);
this.$store.commit('setWorkflowName', { newName: data.name, setStateDirty: false }); this.$store.commit('setWorkflowName', { newName: data.name, setStateDirty: false });
this.$store.commit('setWorkflowSettings', data.settings || {}); this.$store.commit('setWorkflowSettings', data.settings || {});
this.$store.commit('setWorkflowPinData', data.pinData || {}); this.$store.commit('setWorkflowPinData', data.pinData || {});