From 3e34359fa9abcb0b52305a97d83b5180a7fb32aa Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Wed, 4 Sep 2024 15:18:52 +0000 Subject: [PATCH 1/4] feat: write REST API endpoint to update role --- .../api/v1/admin/roles/update-role.ee.js | 24 +++ .../api/v1/admin/roles/update-role.ee.test.js | 177 ++++++++++++++++++ packages/backend/src/models/permission.js | 21 +++ packages/backend/src/models/role.js | 37 ++++ .../src/routes/api/v1/admin/roles.ee.js | 9 + .../rest/api/v1/admin/roles/update-role.ee.js | 32 ++++ 6 files changed, 300 insertions(+) create mode 100644 packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.js create mode 100644 packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.test.js create mode 100644 packages/backend/test/mocks/rest/api/v1/admin/roles/update-role.ee.js diff --git a/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.js b/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.js new file mode 100644 index 00000000..de8a9aa4 --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.js @@ -0,0 +1,24 @@ +import { renderObject } from '../../../../../helpers/renderer.js'; +import Role from '../../../../../models/role.js'; + +export default async (request, response) => { + const role = await Role.query() + .findById(request.params.roleId) + .throwIfNotFound(); + + const updatedRoleWithPermissions = await role.updateWithPermissions( + roleParams(request) + ); + + renderObject(response, updatedRoleWithPermissions); +}; + +const roleParams = (request) => { + const { name, description, permissions } = request.body; + + return { + name, + description, + permissions, + }; +}; diff --git a/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.test.js b/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.test.js new file mode 100644 index 00000000..8d6a3636 --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/roles/update-role.ee.test.js @@ -0,0 +1,177 @@ +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import request from 'supertest'; +import app from '../../../../../app.js'; +import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id.js'; +import { createRole } from '../../../../../../test/factories/role.js'; +import { createPermission } from '../../../../../../test/factories/permission.js'; +import { createUser } from '../../../../../../test/factories/user.js'; +import updateRoleMock from '../../../../../../test/mocks/rest/api/v1/admin/roles/update-role.ee.js'; +import * as license from '../../../../../helpers/license.ee.js'; + +describe('PATCH /api/v1/admin/roles/:roleId', () => { + let adminRole, viewerRole, currentUser, token; + + beforeEach(async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + adminRole = await createRole({ name: 'Admin' }); + viewerRole = await createRole({ name: 'Viewer' }); + + await createPermission({ + action: 'read', + subject: 'Connection', + }); + + await createPermission({ + action: 'read', + subject: 'Flow', + }); + + currentUser = await createUser({ roleId: adminRole.id }); + + token = await createAuthTokenByUserId(currentUser.id); + }); + + it('should return the updated role along with permissions', async () => { + const roleData = { + name: 'Updated role name', + description: 'A new description', + permissions: [ + { + action: 'read', + subject: 'Execution', + conditions: ['isCreator'], + }, + ], + }; + + const response = await request(app) + .patch(`/api/v1/admin/roles/${viewerRole.id}`) + .set('Authorization', token) + .send(roleData) + .expect(200); + + const refetchedViewerRole = await viewerRole + .$query() + .withGraphFetched({ permissions: true }); + + const expectedPayload = await updateRoleMock( + { + ...refetchedViewerRole, + ...roleData, + isAdmin: false, + }, + [ + { + ...refetchedViewerRole.permissions[0], + ...roleData.permissions[0], + }, + ] + ); + + expect(response.body).toStrictEqual(expectedPayload); + }); + + it('should return the updated role with sanitized permissions', async () => { + const validPermission = { + action: 'create', + subject: 'Connection', + conditions: ['isCreator'], + }; + + const invalidPermission = { + action: 'publish', + subject: 'Connection', + conditions: ['isCreator'], + }; + + const roleData = { + permissions: [validPermission, invalidPermission], + }; + + const response = await request(app) + .patch(`/api/v1/admin/roles/${viewerRole.id}`) + .set('Authorization', token) + .send(roleData) + .expect(200); + + const refetchedViewerRole = await viewerRole.$query().withGraphFetched({ + permissions: true, + }); + + const expectedPayload = updateRoleMock(refetchedViewerRole, [ + { + ...refetchedViewerRole.permissions[0], + ...validPermission, + }, + ]); + + expect(response.body).toStrictEqual(expectedPayload); + }); + + it('should return not authorized response for updating admin role', async () => { + const roleData = { + name: 'Updated role name', + description: 'A new description', + permissions: [ + { + action: 'read', + subject: 'Execution', + conditions: ['isCreator'], + }, + ], + }; + + await request(app) + .patch(`/api/v1/admin/roles/${adminRole.id}`) + .set('Authorization', token) + .send(roleData) + .expect(403); + }); + + it('should return unprocessable entity response for invalid role data', async () => { + const roleData = { + description: 123, + permissions: [], + }; + + const response = await request(app) + .patch(`/api/v1/admin/roles/${viewerRole.id}`) + .set('Authorization', token) + .send(roleData) + .expect(422); + + expect(response.body).toStrictEqual({ + errors: { + description: ['must be string,null'], + }, + meta: { + type: 'ModelValidation', + }, + }); + }); + + it('should return unique violation response for duplicate role data', async () => { + await createRole({ name: 'Editor' }); + + const roleData = { + name: 'Editor', + permissions: [], + }; + + const response = await request(app) + .patch(`/api/v1/admin/roles/${viewerRole.id}`) + .set('Authorization', token) + .send(roleData) + .expect(422); + + expect(response.body).toStrictEqual({ + errors: { + name: ["'name' must be unique."], + }, + meta: { + type: 'UniqueViolationError', + }, + }); + }); +}); diff --git a/packages/backend/src/models/permission.js b/packages/backend/src/models/permission.js index 7ebd79c0..6ef9ae0d 100644 --- a/packages/backend/src/models/permission.js +++ b/packages/backend/src/models/permission.js @@ -1,4 +1,5 @@ import Base from './base.js'; +import permissionCatalog from '../helpers/permission-catalog.ee.js'; class Permission extends Base { static tableName = 'permissions'; @@ -17,6 +18,26 @@ class Permission extends Base { updatedAt: { type: 'string' }, }, }; + + static sanitize(permissions) { + const sanitizedPermissions = permissions.filter((permission) => { + const { action, subject, conditions } = permission; + + const relevantAction = permissionCatalog.actions.find( + (actionCatalogItem) => actionCatalogItem.key === action + ); + const validSubject = relevantAction.subjects.includes(subject); + const validConditions = conditions.every((condition) => { + return !!permissionCatalog.conditions.find( + (conditionCatalogItem) => conditionCatalogItem.key === condition + ); + }); + + return validSubject && validConditions; + }); + + return sanitizedPermissions; + } } export default Permission; diff --git a/packages/backend/src/models/role.js b/packages/backend/src/models/role.js index 0b0d7852..2c74bdd4 100644 --- a/packages/backend/src/models/role.js +++ b/packages/backend/src/models/role.js @@ -1,6 +1,7 @@ import Base from './base.js'; import Permission from './permission.js'; import User from './user.js'; +import NotAuthorizedError from '../errors/not-authorized.js'; class Role extends Base { static tableName = 'roles'; @@ -48,6 +49,42 @@ class Role extends Base { static async findAdmin() { return await this.query().findOne({ name: 'Admin' }); } + + async updateWithPermissions(data) { + if (this.isAdmin) { + throw new NotAuthorizedError('The admin role cannot be altered!'); + } + + const { name, description, permissions } = data; + + return await Role.transaction(async (trx) => { + await this.$relatedQuery('permissions', trx).delete(); + + if (permissions?.length) { + const sanitizedPermissions = Permission.sanitize(permissions).map( + (permission) => ({ + ...permission, + roleId: this.id, + }) + ); + + await Permission.query().insert(sanitizedPermissions); + } + + await this.$query(trx).patch({ + name, + description, + }); + + return await this.$query(trx) + .leftJoinRelated({ + permissions: true, + }) + .withGraphFetched({ + permissions: true, + }); + }); + } } export default Role; diff --git a/packages/backend/src/routes/api/v1/admin/roles.ee.js b/packages/backend/src/routes/api/v1/admin/roles.ee.js index 6ec88967..d429462d 100644 --- a/packages/backend/src/routes/api/v1/admin/roles.ee.js +++ b/packages/backend/src/routes/api/v1/admin/roles.ee.js @@ -5,6 +5,7 @@ import { checkIsEnterprise } from '../../../../helpers/check-is-enterprise.js'; import createRoleAction from '../../../../controllers/api/v1/admin/roles/create-role.ee.js'; import getRolesAction from '../../../../controllers/api/v1/admin/roles/get-roles.ee.js'; import getRoleAction from '../../../../controllers/api/v1/admin/roles/get-role.ee.js'; +import updateRoleAction from '../../../../controllers/api/v1/admin/roles/update-role.ee.js'; const router = Router(); @@ -32,4 +33,12 @@ router.get( getRoleAction ); +router.patch( + '/:roleId', + authenticateUser, + authorizeAdmin, + checkIsEnterprise, + updateRoleAction +); + export default router; diff --git a/packages/backend/test/mocks/rest/api/v1/admin/roles/update-role.ee.js b/packages/backend/test/mocks/rest/api/v1/admin/roles/update-role.ee.js new file mode 100644 index 00000000..164ea4b5 --- /dev/null +++ b/packages/backend/test/mocks/rest/api/v1/admin/roles/update-role.ee.js @@ -0,0 +1,32 @@ +const updateRoleMock = (role, permissions = []) => { + const data = { + id: role.id, + name: role.name, + isAdmin: role.isAdmin, + description: role.description, + createdAt: role.createdAt.getTime(), + updatedAt: role.updatedAt.getTime(), + permissions: permissions.map((permission) => ({ + id: permission.id, + action: permission.action, + conditions: permission.conditions, + roleId: permission.roleId, + subject: permission.subject, + createdAt: permission.createdAt.getTime(), + updatedAt: permission.updatedAt.getTime(), + })), + }; + + return { + data: data, + meta: { + count: 1, + currentPage: null, + isArray: false, + totalPages: null, + type: 'Role', + }, + }; +}; + +export default updateRoleMock; From 9df1b29d70cdea685d0551db7438118bb4828c05 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Wed, 4 Sep 2024 15:19:10 +0000 Subject: [PATCH 2/4] feat(EditRole): use REST API endpoint to update role --- packages/web/src/hooks/useAdminUpdateRole.js | 25 ++++++++++++++++++++ packages/web/src/pages/EditRole/index.ee.jsx | 19 ++++++--------- 2 files changed, 32 insertions(+), 12 deletions(-) create mode 100644 packages/web/src/hooks/useAdminUpdateRole.js diff --git a/packages/web/src/hooks/useAdminUpdateRole.js b/packages/web/src/hooks/useAdminUpdateRole.js new file mode 100644 index 00000000..7504c10f --- /dev/null +++ b/packages/web/src/hooks/useAdminUpdateRole.js @@ -0,0 +1,25 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import api from 'helpers/api'; + +export default function useAdminUpdateRole(roleId) { + const queryClient = useQueryClient(); + + const query = useMutation({ + mutationFn: async (payload) => { + const { data } = await api.patch(`/v1/admin/roles/${roleId}`, payload); + + return data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ + queryKey: ['admin', 'roles'], + }); + + queryClient.invalidateQueries({ + queryKey: ['admin', 'roles', roleId], + }); + }, + }); + + return query; +} diff --git a/packages/web/src/pages/EditRole/index.ee.jsx b/packages/web/src/pages/EditRole/index.ee.jsx index e52ced94..cc399637 100644 --- a/packages/web/src/pages/EditRole/index.ee.jsx +++ b/packages/web/src/pages/EditRole/index.ee.jsx @@ -1,4 +1,3 @@ -import { useMutation } from '@apollo/client'; import LoadingButton from '@mui/lab/LoadingButton'; import Grid from '@mui/material/Grid'; import Skeleton from '@mui/material/Skeleton'; @@ -13,20 +12,21 @@ import PageTitle from 'components/PageTitle'; import PermissionCatalogField from 'components/PermissionCatalogField/index.ee'; import TextField from 'components/TextField'; import * as URLS from 'config/urls'; -import { UPDATE_ROLE } from 'graphql/mutations/update-role.ee'; import { getPermissions, getRoleWithComputedPermissions, } from 'helpers/computePermissions.ee'; import useFormatMessage from 'hooks/useFormatMessage'; +import useAdminUpdateRole from 'hooks/useAdminUpdateRole'; import useRole from 'hooks/useRole.ee'; export default function EditRole() { const formatMessage = useFormatMessage(); - const [updateRole, { loading }] = useMutation(UPDATE_ROLE); const navigate = useNavigate(); const { roleId } = useParams(); const { data, loading: isRoleLoading } = useRole({ roleId }); + const { mutateAsync: updateRole, isPending: isUpdateRolePending } = + useAdminUpdateRole(roleId); const role = data?.data; const enqueueSnackbar = useEnqueueSnackbar(); @@ -34,14 +34,9 @@ export default function EditRole() { try { const newPermissions = getPermissions(roleData.computedPermissions); await updateRole({ - variables: { - input: { - id: roleId, - name: roleData.name, - description: roleData.description, - permissions: newPermissions, - }, - }, + name: roleData.name, + description: roleData.description, + permissions: newPermissions, }); enqueueSnackbar(formatMessage('editRole.successfullyUpdated'), { @@ -111,7 +106,7 @@ export default function EditRole() { variant="contained" color="primary" sx={{ boxShadow: 2 }} - loading={loading} + loading={isUpdateRolePending} disabled={role?.isAdmin || isRoleLoading} data-test="update-button" > From c9ba219de1ca73a8799b3f141037e1067d28500c Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Wed, 4 Sep 2024 15:19:19 +0000 Subject: [PATCH 3/4] chore: remove redundant update role mutation --- .../backend/src/graphql/mutation-resolvers.js | 2 -- packages/backend/src/graphql/schema.graphql | 14 -------------- .../web/src/graphql/mutations/update-role.ee.js | 16 ---------------- 3 files changed, 32 deletions(-) delete mode 100644 packages/web/src/graphql/mutations/update-role.ee.js diff --git a/packages/backend/src/graphql/mutation-resolvers.js b/packages/backend/src/graphql/mutation-resolvers.js index 509fb5ad..8483637a 100644 --- a/packages/backend/src/graphql/mutation-resolvers.js +++ b/packages/backend/src/graphql/mutation-resolvers.js @@ -10,7 +10,6 @@ import resetConnection from './mutations/reset-connection.js'; import updateConnection from './mutations/update-connection.js'; import updateCurrentUser from './mutations/update-current-user.js'; import updateFlowStatus from './mutations/update-flow-status.js'; -import updateRole from './mutations/update-role.ee.js'; import updateStep from './mutations/update-step.js'; import upsertSamlAuthProvidersRoleMappings from './mutations/upsert-saml-auth-providers-role-mappings.ee.js'; @@ -39,7 +38,6 @@ const mutationResolvers = { updateConnection, updateCurrentUser, updateFlowStatus, - updateRole, updateStep, updateUser, upsertSamlAuthProvidersRoleMappings, diff --git a/packages/backend/src/graphql/schema.graphql b/packages/backend/src/graphql/schema.graphql index 2735654e..5598f59a 100644 --- a/packages/backend/src/graphql/schema.graphql +++ b/packages/backend/src/graphql/schema.graphql @@ -18,7 +18,6 @@ type Mutation { updateConnection(input: UpdateConnectionInput): Connection updateCurrentUser(input: UpdateCurrentUserInput): User updateFlowStatus(input: UpdateFlowStatusInput): Flow - updateRole(input: UpdateRoleInput): Role updateStep(input: UpdateStepInput): Step updateUser(input: UpdateUserInput): User upsertSamlAuthProvidersRoleMappings( @@ -335,19 +334,6 @@ input UpdateCurrentUserInput { fullName: String } -input PermissionInput { - action: String! - subject: String! - conditions: [String] -} - -input UpdateRoleInput { - id: String! - name: String! - description: String - permissions: [PermissionInput] -} - input DeleteRoleInput { id: String! } diff --git a/packages/web/src/graphql/mutations/update-role.ee.js b/packages/web/src/graphql/mutations/update-role.ee.js deleted file mode 100644 index 15f741c6..00000000 --- a/packages/web/src/graphql/mutations/update-role.ee.js +++ /dev/null @@ -1,16 +0,0 @@ -import { gql } from '@apollo/client'; -export const UPDATE_ROLE = gql` - mutation UpdateRole($input: UpdateRoleInput) { - updateRole(input: $input) { - id - name - description - permissions { - id - action - subject - conditions - } - } - } -`; From ea667bb6a9f5571eaac82570e3f82164ae8ada60 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Fri, 6 Sep 2024 09:26:34 +0000 Subject: [PATCH 4/4] refactor(useAdminUpdateRole): remove redundant invalidateQueries --- packages/web/src/hooks/useAdminUpdateRole.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/web/src/hooks/useAdminUpdateRole.js b/packages/web/src/hooks/useAdminUpdateRole.js index 7504c10f..072824aa 100644 --- a/packages/web/src/hooks/useAdminUpdateRole.js +++ b/packages/web/src/hooks/useAdminUpdateRole.js @@ -14,10 +14,6 @@ export default function useAdminUpdateRole(roleId) { queryClient.invalidateQueries({ queryKey: ['admin', 'roles'], }); - - queryClient.invalidateQueries({ - queryKey: ['admin', 'roles', roleId], - }); }, });