From e6702ce69ab691c8e7c564f22bb81bffb54013ff Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Mon, 21 Aug 2023 16:04:25 +0100 Subject: [PATCH 1/7] feature(ee): WIP support partial updates of review workflow stage permissions --- .../ReviewWorkflows/pages/EditView/EditView.js | 17 ++++++++--------- .../ReviewWorkflows/utils/validateWorkflow.js | 2 +- .../server/services/review-workflows/stages.js | 7 ++++++- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js index d83f47bea9..5557f03019 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js @@ -115,16 +115,15 @@ export function ReviewWorkflowsEditView() { // changed; this enables partial updates e.g. for users who don't have // permissions to see roles stages: workflow.stages.map((stage) => { + const localPermissions = stage?.permissions ?? []; + const serverStage = serverState.workflow.stages.find((s) => s.id === stage.id) || {}; + const serverPermissions = serverStage?.permissions ?? []; + const hasUpdatedPermissions = - stage?.permissions?.length > 0 - ? stage.permissions.some( - ({ role }) => - !serverState.workflow.stages.find( - (stage) => - !!(stage.permissions ?? []).find((permission) => permission.role === role) - ) - ) - : false; + localPermissions?.length !== serverPermissions?.length || + localPermissions.some(({ role }) => { + return !serverPermissions.some(({ role: serverRole }) => serverRole === role); + }); return { ...stage, diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js index 1c1fc22225..32e49bfebb 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js @@ -78,7 +78,7 @@ export async function validateWorkflow({ values, formatMessage }) { ) .strict() .min( - 1, + 0, formatMessage({ id: 'Settings.review-workflows.validation.stage.permissions', defaultMessage: 'Must be either an array or undefined', diff --git a/packages/core/admin/ee/server/services/review-workflows/stages.js b/packages/core/admin/ee/server/services/review-workflows/stages.js index e72a9a67d1..8cc268e310 100644 --- a/packages/core/admin/ee/server/services/review-workflows/stages.js +++ b/packages/core/admin/ee/server/services/review-workflows/stages.js @@ -153,7 +153,12 @@ module.exports = ({ strapi }) => { // Update the workflow stages await mapAsync(updated, (destStage) => { const srcStage = srcStages.find((s) => s.id === destStage.id); - return this.update(srcStage, destStage); + return this.update(srcStage, { + ...destStage, + // If the destination stage has no permissions property do not overwrite + // the permissions and use the source stage permissions instead + permissions: destStage?.permissions ?? srcStage.permissions, + }); }); // Delete the stages that are not in the new stages list From 895b960f69b30d367f1ea2e42c9abbdc20776abd Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Fri, 25 Aug 2023 09:55:53 +0100 Subject: [PATCH 2/7] test(ee): add test for partial stage permission update --- .../server/services/__tests__/stages.test.js | 24 +++++++++++++++++++ .../services/review-workflows/stages.js | 12 +++------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/core/admin/ee/server/services/__tests__/stages.test.js b/packages/core/admin/ee/server/services/__tests__/stages.test.js index 132f105d72..23fc7c78dc 100644 --- a/packages/core/admin/ee/server/services/__tests__/stages.test.js +++ b/packages/core/admin/ee/server/services/__tests__/stages.test.js @@ -247,5 +247,29 @@ describe('Review workflows - Stages service', () => { expect(entityServiceMock.update).toBeCalled(); expect(entityServiceMock.delete).toBeCalled(); }); + + test('Undefined destination stage permissions should apply a partial permission update', async () => { + const srcStages = workflowMock.stages.map((stage) => ({ + ...stage, + permissions: [{ id: 1, role: 'role', action: 'action' }], + })); + + const destStages = workflowMock.stages.map((stage, index) => ({ + ...stage, + name: `Updated Name ${index}`, + })); + + await stagesService.replaceStages(srcStages, destStages); + + expect(entityServiceMock.create).not.toBeCalled(); + expect(entityServiceMock.delete).not.toBeCalled(); + expect(entityServiceMock.update).toBeCalledTimes(4); + + destStages.forEach((stage, index) => { + expect(entityServiceMock.update).toBeCalledWith('admin::workflow-stage', stage.id, { + data: { ...stage, permissions: srcStages[index].permissions }, + }); + }); + }); }); }); diff --git a/packages/core/admin/ee/server/services/review-workflows/stages.js b/packages/core/admin/ee/server/services/review-workflows/stages.js index 8cc268e310..f16cda75b7 100644 --- a/packages/core/admin/ee/server/services/review-workflows/stages.js +++ b/packages/core/admin/ee/server/services/review-workflows/stages.js @@ -78,11 +78,9 @@ module.exports = ({ strapi }) => { }, async update(srcStage, destStage) { - let stagePermissions = []; + let stagePermissions = srcStage?.permissions ?? []; const stageId = destStage.id; - await this.deleteStagePermissions([srcStage]); - if (destStage.permissions) { const permissions = await mapAsync(destStage.permissions, (permission) => stagePermissionsService.register(permission.role, permission.action, stageId) @@ -153,12 +151,8 @@ module.exports = ({ strapi }) => { // Update the workflow stages await mapAsync(updated, (destStage) => { const srcStage = srcStages.find((s) => s.id === destStage.id); - return this.update(srcStage, { - ...destStage, - // If the destination stage has no permissions property do not overwrite - // the permissions and use the source stage permissions instead - permissions: destStage?.permissions ?? srcStage.permissions, - }); + + return this.update(srcStage, destStage); }); // Delete the stages that are not in the new stages list From 9e080a4d84f25b8baee0f80288501743abf7a38b Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Fri, 25 Aug 2023 16:17:46 +0100 Subject: [PATCH 3/7] chore(ee): remove src stages when stages are being updated chore(ee): remove minimum length validation for stage array --- .../utils/tests/validateWorkflow.test.js | 21 ------------------- .../ReviewWorkflows/utils/validateWorkflow.js | 9 +------- .../services/review-workflows/stages.js | 2 ++ 3 files changed, 3 insertions(+), 29 deletions(-) diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/tests/validateWorkflow.test.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/tests/validateWorkflow.test.js index f7c65a84d1..9d76a58bd6 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/tests/validateWorkflow.test.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/tests/validateWorkflow.test.js @@ -160,27 +160,6 @@ describe('Settings | Review Workflows | validateWorkflow()', () => { }) ).toEqual(true); - expect( - await setup({ - name: 'name', - stages: [ - { - name: 'stage-1', - color: '#ffffff', - permissions: [], - }, - ], - }) - ).toMatchInlineSnapshot(` - { - "stages": [ - { - "permissions": "Must be either an array or undefined", - }, - ], - } - `); - expect( await setup({ name: 'name', diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js index 32e49bfebb..0398806b49 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/utils/validateWorkflow.js @@ -76,14 +76,7 @@ export async function validateWorkflow({ values, formatMessage }) { }), }) ) - .strict() - .min( - 0, - formatMessage({ - id: 'Settings.review-workflows.validation.stage.permissions', - defaultMessage: 'Must be either an array or undefined', - }) - ), + .strict(), }) ) .min(1), diff --git a/packages/core/admin/ee/server/services/review-workflows/stages.js b/packages/core/admin/ee/server/services/review-workflows/stages.js index f16cda75b7..2e96d17203 100644 --- a/packages/core/admin/ee/server/services/review-workflows/stages.js +++ b/packages/core/admin/ee/server/services/review-workflows/stages.js @@ -82,6 +82,8 @@ module.exports = ({ strapi }) => { const stageId = destStage.id; if (destStage.permissions) { + await this.deleteStagePermissions([srcStage]); + const permissions = await mapAsync(destStage.permissions, (permission) => stagePermissionsService.register(permission.role, permission.action, stageId) ); From 35b185027eb589d879f6933eaf1d35b9b931f93c Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Thu, 31 Aug 2023 10:07:12 +0100 Subject: [PATCH 4/7] fix: WIP use unique names for restricted roles --- api-tests/core/admin/ee/provider-login.test.api.js | 2 +- api-tests/core/admin/ee/provider-options.test.api.js | 2 +- api-tests/core/upload/admin/configure-view.test.api.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api-tests/core/admin/ee/provider-login.test.api.js b/api-tests/core/admin/ee/provider-login.test.api.js index f4c120cc01..4b3d64b958 100644 --- a/api-tests/core/admin/ee/provider-login.test.api.js +++ b/api-tests/core/admin/ee/provider-login.test.api.js @@ -25,7 +25,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-role', + name: 'restricted-provider-login-role', description: '', }; diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index bb7488244a..59e4556574 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -25,7 +25,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-role', + name: 'restricted-provider-options-role', description: '', }; diff --git a/api-tests/core/upload/admin/configure-view.test.api.js b/api-tests/core/upload/admin/configure-view.test.api.js index 134682067e..3f223cd717 100644 --- a/api-tests/core/upload/admin/configure-view.test.api.js +++ b/api-tests/core/upload/admin/configure-view.test.api.js @@ -40,7 +40,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-role', + name: 'restricted-configure-view-role', description: '', }; From d71e6766be0c5d9252d655bf5751b8754b70065b Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Thu, 31 Aug 2023 11:36:25 +0100 Subject: [PATCH 5/7] fix(ee): clean up roles and users after review workflows API test --- .../admin/ee/review-workflows.test.api.js | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/api-tests/core/admin/ee/review-workflows.test.api.js b/api-tests/core/admin/ee/review-workflows.test.api.js index 0ce21e656c..3b0d8bbd2a 100644 --- a/api-tests/core/admin/ee/review-workflows.test.api.js +++ b/api-tests/core/admin/ee/review-workflows.test.api.js @@ -648,8 +648,14 @@ describeOnCondition(edition === 'EE')('Review workflows', () => { let utils; let entry; let restrictedRequest; + let restrictedUser; let restrictedRole; + const deleteFixtures = async () => { + await utils.deleteUserById(restrictedUser.id); + await utils.deleteRolesById([restrictedRole.id]); + }; + beforeAll(async () => { // Update workflow to assign content type await requests.admin.put(`/admin/review-workflows/workflows/${testWorkflow.id}?populate=*`, { @@ -658,24 +664,28 @@ describeOnCondition(edition === 'EE')('Review workflows', () => { entry = await createEntry(productUID, { name: 'Product' }); - const restrictedUser = { - email: 'restricted@user.io', - password: 'Restricted123', - }; - utils = createUtils(strapi); const role = await utils.createRole({ name: 'restricted-role', description: '', }); + restrictedRole = role; - await utils.createUserIfNotExists({ - ...restrictedUser, + const restrictedUserInfo = { + email: 'restricted@user.io', + password: 'Restricted123', + }; + + restrictedUser = await utils.createUserIfNotExists({ + ...restrictedUserInfo, roles: [role.id], }); - restrictedRole = role; - restrictedRequest = await createAuthRequest({ strapi, userInfo: restrictedUser }); + restrictedRequest = await createAuthRequest({ strapi, userInfo: restrictedUserInfo }); + }); + + afterAll(async () => { + await deleteFixtures(); }); test("It shouldn't be available for public", async () => { From 792c7ab15364027e2531dc5ed9a11fa32eda3277 Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Thu, 31 Aug 2023 11:38:23 +0100 Subject: [PATCH 6/7] Revert "fix: WIP use unique names for restricted roles" This reverts commit 35b185027eb589d879f6933eaf1d35b9b931f93c. --- api-tests/core/admin/ee/provider-login.test.api.js | 2 +- api-tests/core/admin/ee/provider-options.test.api.js | 2 +- api-tests/core/upload/admin/configure-view.test.api.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api-tests/core/admin/ee/provider-login.test.api.js b/api-tests/core/admin/ee/provider-login.test.api.js index 4b3d64b958..f4c120cc01 100644 --- a/api-tests/core/admin/ee/provider-login.test.api.js +++ b/api-tests/core/admin/ee/provider-login.test.api.js @@ -25,7 +25,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-provider-login-role', + name: 'restricted-role', description: '', }; diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index 59e4556574..bb7488244a 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -25,7 +25,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-provider-options-role', + name: 'restricted-role', description: '', }; diff --git a/api-tests/core/upload/admin/configure-view.test.api.js b/api-tests/core/upload/admin/configure-view.test.api.js index 3f223cd717..134682067e 100644 --- a/api-tests/core/upload/admin/configure-view.test.api.js +++ b/api-tests/core/upload/admin/configure-view.test.api.js @@ -40,7 +40,7 @@ const restrictedUser = { }; const restrictedRole = { - name: 'restricted-configure-view-role', + name: 'restricted-role', description: '', }; From 2671fbc0327346572f427c28d529af94f854ca98 Mon Sep 17 00:00:00 2001 From: Jamie Howard Date: Mon, 4 Sep 2023 15:49:27 +0100 Subject: [PATCH 7/7] fix(ee): revert permission change FE logic --- .../pages/EditView/EditView.js | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js index 0de6f814e0..a65519d495 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/ReviewWorkflows/pages/EditView/EditView.js @@ -115,15 +115,21 @@ export function ReviewWorkflowsEditView() { // changed; this enables partial updates e.g. for users who don't have // permissions to see roles stages: workflow.stages.map((stage) => { - const localPermissions = stage?.permissions ?? []; - const serverStage = serverState.workflow.stages.find((s) => s.id === stage.id) || {}; - const serverPermissions = serverStage?.permissions ?? []; + let hasUpdatedPermissions = true; + const serverStage = serverState.workflow.stages.find( + (serverStage) => serverStage.id === stage?.id + ); - const hasUpdatedPermissions = - localPermissions?.length !== serverPermissions?.length || - localPermissions.some(({ role }) => { - return !serverPermissions.some(({ role: serverRole }) => serverRole === role); - }); + if (serverStage) { + hasUpdatedPermissions = + serverStage.permissions?.length !== stage.permission?.length || + !serverStage.permissions.every( + (serverPermission) => + !!stage.permissions.find( + (permission) => permission.role === serverPermission.role + ) + ); + } return { ...stage,