From 0b6c28422ccad5b304d05d7cc0321e003f9f3293 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Thu, 5 Sep 2024 15:31:24 +0000 Subject: [PATCH 1/5] feat: write REST API endpoint to delete role --- .../api/v1/admin/roles/delete-role.ee.js | 11 ++ .../api/v1/admin/roles/delete-role.ee.test.js | 112 ++++++++++++++++++ packages/backend/src/models/role.js | 55 +++++++++ .../src/routes/api/v1/admin/roles.ee.js | 9 ++ 4 files changed, 187 insertions(+) create mode 100644 packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.js create mode 100644 packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js diff --git a/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.js b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.js new file mode 100644 index 00000000..99eb00fd --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.js @@ -0,0 +1,11 @@ +import Role from '../../../../../models/role.js'; + +export default async (request, response) => { + const role = await Role.query() + .findById(request.params.roleId) + .throwIfNotFound(); + + await role.deleteWithPermissions(); + + response.status(204).end(); +}; diff --git a/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js new file mode 100644 index 00000000..a9396bf0 --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js @@ -0,0 +1,112 @@ +import Crypto from 'node:crypto'; +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 { createSamlAuthProvider } from '../../../../../../test/factories/saml-auth-provider.ee.js'; +import { createUser } from '../../../../../../test/factories/user.js'; +import * as license from '../../../../../helpers/license.ee.js'; + +describe('DELETE /api/v1/admin/roles/:roleId', () => { + let adminRole, currentUser, token; + + beforeEach(async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + adminRole = await createRole({ name: 'Admin' }); + + currentUser = await createUser({ roleId: adminRole.id }); + + token = await createAuthTokenByUserId(currentUser.id); + }); + + it('should return HTTP 204 for unused role', async () => { + const role = await createRole(); + const permission = await createPermission({ roleId: role.id }); + + await request(app) + .delete(`/api/v1/admin/roles/${role.id}`) + .set('Authorization', token) + .expect(204); + + const refetchedRole = await role.$query(); + const refetchedPermission = await permission.$query(); + + expect(refetchedRole).toBeUndefined(); + expect(refetchedPermission).toBeUndefined(); + }); + + it('should return HTTP 404 for not existing role UUID', async () => { + const notExistingRoleUUID = Crypto.randomUUID(); + + await request(app) + .delete(`/api/v1/admin/roles/${notExistingRoleUUID}`) + .set('Authorization', token) + .expect(404); + }); + + it('should return not authorized response for deleting admin role', async () => { + await request(app) + .delete(`/api/v1/admin/roles/${adminRole.id}`) + .set('Authorization', token) + .expect(403); + }); + + it('should return unprocessable entity response for role used by users', async () => { + const role = await createRole(); + await createUser({ roleId: role.id }); + + const response = await request(app) + .delete(`/api/v1/admin/roles/${role.id}`) + .set('Authorization', token) + .expect(422); + + expect(response.body).toStrictEqual({ + errors: { + role: [`All users must be migrated away from the "${role.name}" role.`], + }, + meta: { + type: 'ValidationError', + }, + }); + }); + + it('should return unprocessable entity response for role used by saml auth providers', async () => { + const samlAuthProvider = await createSamlAuthProvider(); + + const response = await request(app) + .delete(`/api/v1/admin/roles/${samlAuthProvider.defaultRoleId}`) + .set('Authorization', token) + .expect(422); + + expect(response.body).toStrictEqual({ + errors: { + samlAuthProvider: [ + 'You need to change the default role in the SAML configuration before deleting this role.', + ], + }, + meta: { + type: 'ValidationError', + }, + }); + }); + + it('should not delete role and permissions on unsuccessful response', async () => { + const role = await createRole(); + const permission = await createPermission({ roleId: role.id }); + await createUser({ roleId: role.id }); + + await request(app) + .delete(`/api/v1/admin/roles/${role.id}`) + .set('Authorization', token) + .expect(422); + + const refetchedRole = await role.$query(); + const refetchedPermission = await permission.$query(); + + expect(refetchedRole).toStrictEqual(role); + expect(refetchedPermission).toStrictEqual(permission); + }); +}); diff --git a/packages/backend/src/models/role.js b/packages/backend/src/models/role.js index 2c74bdd4..6760c321 100644 --- a/packages/backend/src/models/role.js +++ b/packages/backend/src/models/role.js @@ -1,6 +1,8 @@ +import { ValidationError } from 'objection'; import Base from './base.js'; import Permission from './permission.js'; import User from './user.js'; +import SamlAuthProvider from './saml-auth-provider.ee.js'; import NotAuthorizedError from '../errors/not-authorized.js'; class Role extends Base { @@ -85,6 +87,59 @@ class Role extends Base { }); }); } + + async deleteWithPermissions() { + return await Role.transaction(async (trx) => { + await this.$relatedQuery('permissions', trx).delete(); + + return await this.$query(trx).delete(); + }); + } + + async $beforeDelete(queryContext) { + await super.$beforeDelete(queryContext); + + if (this.isAdmin) { + throw new NotAuthorizedError('The admin role cannot be deleted!'); + } + + const userCount = await this.$relatedQuery('users').limit(1).resultSize(); + const hasUsers = userCount > 0; + + if (hasUsers) { + throw new ValidationError({ + data: { + role: [ + { + message: `All users must be migrated away from the "${this.name}" role.`, + }, + ], + }, + type: 'ValidationError', + }); + } + + const samlAuthProviderUsingDefaultRole = await SamlAuthProvider.query() + .where({ + default_role_id: this.id, + }) + .limit(1) + .first(); + + if (samlAuthProviderUsingDefaultRole) { + throw new ValidationError({ + data: { + samlAuthProvider: [ + { + message: + 'You need to change the default role in the SAML configuration before deleting this role.', + }, + ], + }, + type: 'ValidationError', + }); + } + } } 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 d429462d..9715d705 100644 --- a/packages/backend/src/routes/api/v1/admin/roles.ee.js +++ b/packages/backend/src/routes/api/v1/admin/roles.ee.js @@ -6,6 +6,7 @@ import createRoleAction from '../../../../controllers/api/v1/admin/roles/create- 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'; +import deleteRoleAction from '../../../../controllers/api/v1/admin/roles/delete-role.ee.js'; const router = Router(); @@ -41,4 +42,12 @@ router.patch( updateRoleAction ); +router.delete( + '/:roleId', + authenticateUser, + authorizeAdmin, + checkIsEnterprise, + deleteRoleAction +); + export default router; From 66fe84e12666242b8cafa6659ccac74a407b74d3 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Thu, 5 Sep 2024 15:31:50 +0000 Subject: [PATCH 2/5] feat(DeleteRoleButton): use REST API endpoint to delete role --- .../components/DeleteRoleButton/index.ee.jsx | 26 ++++++++++++------- packages/web/src/hooks/useAdminDeleteRole.js | 25 ++++++++++++++++++ 2 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 packages/web/src/hooks/useAdminDeleteRole.js diff --git a/packages/web/src/components/DeleteRoleButton/index.ee.jsx b/packages/web/src/components/DeleteRoleButton/index.ee.jsx index 9ce25473..5a40c1f9 100644 --- a/packages/web/src/components/DeleteRoleButton/index.ee.jsx +++ b/packages/web/src/components/DeleteRoleButton/index.ee.jsx @@ -1,31 +1,26 @@ import PropTypes from 'prop-types'; -import { useMutation } from '@apollo/client'; import DeleteIcon from '@mui/icons-material/Delete'; import IconButton from '@mui/material/IconButton'; import useEnqueueSnackbar from 'hooks/useEnqueueSnackbar'; import * as React from 'react'; -import { useQueryClient } from '@tanstack/react-query'; import Can from 'components/Can'; import ConfirmationDialog from 'components/ConfirmationDialog'; -import { DELETE_ROLE } from 'graphql/mutations/delete-role.ee'; import useFormatMessage from 'hooks/useFormatMessage'; +import useAdminDeleteRole from 'hooks/useAdminDeleteRole'; function DeleteRoleButton(props) { const { disabled, roleId } = props; const [showConfirmation, setShowConfirmation] = React.useState(false); const formatMessage = useFormatMessage(); const enqueueSnackbar = useEnqueueSnackbar(); - const queryClient = useQueryClient(); - const [deleteRole] = useMutation(DELETE_ROLE, { - variables: { input: { id: roleId } }, - }); + const { mutateAsync: deleteRole } = useAdminDeleteRole(roleId); const handleConfirm = React.useCallback(async () => { try { await deleteRole(); - queryClient.invalidateQueries({ queryKey: ['admin', 'roles'] }); + setShowConfirmation(false); enqueueSnackbar(formatMessage('deleteRoleButton.successfullyDeleted'), { variant: 'success', @@ -34,9 +29,22 @@ function DeleteRoleButton(props) { }, }); } catch (error) { + const errors = Object.values( + error.response.data.errors || [['Failed while deleting!']], + ); + + for (const [error] of errors) { + enqueueSnackbar(error, { + variant: 'error', + SnackbarProps: { + 'data-test': 'snackbar-delete-role-error', + }, + }); + } + throw new Error('Failed while deleting!'); } - }, [deleteRole, enqueueSnackbar, formatMessage, queryClient]); + }, [deleteRole, enqueueSnackbar, formatMessage]); return ( <> diff --git a/packages/web/src/hooks/useAdminDeleteRole.js b/packages/web/src/hooks/useAdminDeleteRole.js new file mode 100644 index 00000000..cbc672bc --- /dev/null +++ b/packages/web/src/hooks/useAdminDeleteRole.js @@ -0,0 +1,25 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import api from 'helpers/api'; + +export default function useAdminDeleteRole(roleId) { + const queryClient = useQueryClient(); + + const query = useMutation({ + mutationFn: async (payload) => { + const { data } = await api.delete(`/v1/admin/roles/${roleId}`); + + return data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ + queryKey: ['admin', 'roles'], + }); + + queryClient.invalidateQueries({ + queryKey: ['admin', 'roles', roleId], + }); + }, + }); + + return query; +} From e5366534ed8bcebad4152a8e0b404699fc376f17 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Thu, 5 Sep 2024 15:31:59 +0000 Subject: [PATCH 3/5] chore: remove redundant delete role mutation --- .../backend/src/graphql/mutation-resolvers.js | 2 -- .../src/graphql/mutations/delete-role.ee.js | 36 ------------------- packages/backend/src/graphql/schema.graphql | 5 --- .../src/graphql/mutations/delete-role.ee.js | 6 ---- 4 files changed, 49 deletions(-) delete mode 100644 packages/backend/src/graphql/mutations/delete-role.ee.js delete mode 100644 packages/web/src/graphql/mutations/delete-role.ee.js diff --git a/packages/backend/src/graphql/mutation-resolvers.js b/packages/backend/src/graphql/mutation-resolvers.js index 8483637a..15e15beb 100644 --- a/packages/backend/src/graphql/mutation-resolvers.js +++ b/packages/backend/src/graphql/mutation-resolvers.js @@ -2,7 +2,6 @@ import createConnection from './mutations/create-connection.js'; import createStep from './mutations/create-step.js'; import createUser from './mutations/create-user.ee.js'; import deleteFlow from './mutations/delete-flow.js'; -import deleteRole from './mutations/delete-role.ee.js'; import duplicateFlow from './mutations/duplicate-flow.js'; import generateAuthUrl from './mutations/generate-auth-url.js'; import registerUser from './mutations/register-user.ee.js'; @@ -28,7 +27,6 @@ const mutationResolvers = { createUser, deleteCurrentUser, deleteFlow, - deleteRole, deleteStep, duplicateFlow, executeFlow, diff --git a/packages/backend/src/graphql/mutations/delete-role.ee.js b/packages/backend/src/graphql/mutations/delete-role.ee.js deleted file mode 100644 index 9b0249a2..00000000 --- a/packages/backend/src/graphql/mutations/delete-role.ee.js +++ /dev/null @@ -1,36 +0,0 @@ -import Role from '../../models/role.js'; -import SamlAuthProvider from '../../models/saml-auth-provider.ee.js'; - -const deleteRole = async (_parent, params, context) => { - context.currentUser.can('delete', 'Role'); - - const role = await Role.query().findById(params.input.id).throwIfNotFound(); - const count = await role.$relatedQuery('users').resultSize(); - - if (count > 0) { - throw new Error('All users must be migrated away from the role!'); - } - - if (role.isAdmin) { - throw new Error('Admin role cannot be deleted!'); - } - - const samlAuthProviderUsingDefaultRole = await SamlAuthProvider.query() - .where({ default_role_id: role.id }) - .limit(1) - .first(); - - if (samlAuthProviderUsingDefaultRole) { - throw new Error( - 'You need to change the default role in the SAML configuration before deleting this role.' - ); - } - - // delete permissions first - await role.$relatedQuery('permissions').delete(); - await role.$query().delete(); - - return true; -}; - -export default deleteRole; diff --git a/packages/backend/src/graphql/schema.graphql b/packages/backend/src/graphql/schema.graphql index 5598f59a..4657302c 100644 --- a/packages/backend/src/graphql/schema.graphql +++ b/packages/backend/src/graphql/schema.graphql @@ -8,7 +8,6 @@ type Mutation { createUser(input: CreateUserInput): UserWithAcceptInvitationUrl deleteCurrentUser: Boolean deleteFlow(input: DeleteFlowInput): Boolean - deleteRole(input: DeleteRoleInput): Boolean deleteStep(input: DeleteStepInput): Step duplicateFlow(input: DuplicateFlowInput): Flow executeFlow(input: ExecuteFlowInput): executeFlowType @@ -334,10 +333,6 @@ input UpdateCurrentUserInput { fullName: String } -input DeleteRoleInput { - id: String! -} - """ The `JSONObject` scalar type represents JSON objects as specified by [ECMA-404](http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf). """ diff --git a/packages/web/src/graphql/mutations/delete-role.ee.js b/packages/web/src/graphql/mutations/delete-role.ee.js deleted file mode 100644 index 709f3b55..00000000 --- a/packages/web/src/graphql/mutations/delete-role.ee.js +++ /dev/null @@ -1,6 +0,0 @@ -import { gql } from '@apollo/client'; -export const DELETE_ROLE = gql` - mutation DeleteRole($input: DeleteRoleInput) { - deleteRole(input: $input) - } -`; From 687295f7723241e74419c7b4a547b457bd2f3b5e Mon Sep 17 00:00:00 2001 From: "Jakub P." Date: Thu, 5 Sep 2024 22:31:06 +0200 Subject: [PATCH 4/5] test: update roles related snackbar locator --- packages/e2e-tests/tests/admin/manage-roles.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/e2e-tests/tests/admin/manage-roles.spec.js b/packages/e2e-tests/tests/admin/manage-roles.spec.js index fa9eace1..00299c5d 100644 --- a/packages/e2e-tests/tests/admin/manage-roles.spec.js +++ b/packages/e2e-tests/tests/admin/manage-roles.spec.js @@ -221,7 +221,7 @@ test.describe('Role management page', () => { await adminRolesPage.snackbar.waitFor({ state: 'attached', }); - const snackbar = await adminRolesPage.getSnackbarData('snackbar-error'); + const snackbar = await adminRolesPage.getSnackbarData('snackbar-delete-role-error'); await expect(snackbar.variant).toBe('error'); await adminRolesPage.closeSnackbar(); await modal.close(); From 61a1ce57c24b0788799ce316831b11baa8053a7a Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Fri, 6 Sep 2024 09:52:03 +0000 Subject: [PATCH 5/5] refactor(useAdminDeleteRole): remove redundant invalidateQueries --- packages/web/src/hooks/useAdminDeleteRole.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/web/src/hooks/useAdminDeleteRole.js b/packages/web/src/hooks/useAdminDeleteRole.js index cbc672bc..6f4c5496 100644 --- a/packages/web/src/hooks/useAdminDeleteRole.js +++ b/packages/web/src/hooks/useAdminDeleteRole.js @@ -14,10 +14,6 @@ export default function useAdminDeleteRole(roleId) { queryClient.invalidateQueries({ queryKey: ['admin', 'roles'], }); - - queryClient.invalidateQueries({ - queryKey: ['admin', 'roles', roleId], - }); }, });