From fa0abd8ca80205f8fac7862fa674db5d0b4d6387 Mon Sep 17 00:00:00 2001 From: Yuliia Pominchuk <31064937+yuliia-pominchuk@users.noreply.github.com> Date: Tue, 21 Jul 2026 16:02:57 +0200 Subject: [PATCH] fix(editor): Reassign users when deleting a custom instance role (#34642) --- packages/@n8n/api-types/src/dto/index.ts | 1 + .../src/dto/roles/role-delete-query.dto.ts | 11 + .../db/src/repositories/role.repository.ts | 29 +++ .../__tests__/role.controller.test.ts | 14 +- .../cli/src/controllers/role.controller.ts | 4 +- packages/cli/src/services/role.service.ts | 26 ++- .../controllers/role.controller.test.ts | 26 +++ .../integration/services/role.service.test.ts | 107 ++++++++++ .../frontend/@n8n/i18n/src/locales/en.json | 7 + .../@n8n/rest-api-client/src/api/roles.ts | 9 +- .../frontend/@n8n/stores/src/roles.store.ts | 4 +- .../roles/composables/useRoleDeletion.test.ts | 201 ++++++++++++++++++ .../roles/composables/useRoleDeletion.ts | 153 +++++++++++++ .../roles/composables/useRolesListActions.ts | 55 ++--- .../roles/instance/InstanceRoleView.vue | 56 ++--- .../roles/instance/InstanceRolesView.test.ts | 45 ++++ .../roles/instance/InstanceRolesView.vue | 26 ++- .../DeleteInstanceRoleModal.test.ts | 117 ++++++++++ .../components/DeleteInstanceRoleModal.vue | 142 +++++++++++++ .../roles/project/ProjectRolesView.test.ts | 6 +- 20 files changed, 950 insertions(+), 89 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/roles/role-delete-query.dto.ts create mode 100644 packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.test.ts create mode 100644 packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.ts create mode 100644 packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.test.ts create mode 100644 packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.vue diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 8ea0a3989c8..e2f9022d57b 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -187,6 +187,7 @@ export { } from './roles/list-role-mapping-rule-query.dto'; export { RoleListQueryDto } from './roles/role-list-query.dto'; export { RoleGetQueryDto } from './roles/role-get-query.dto'; +export { RoleDeleteQueryDto } from './roles/role-delete-query.dto'; export { RoleAssignmentsResponseDto, type RoleProjectAssignment, diff --git a/packages/@n8n/api-types/src/dto/roles/role-delete-query.dto.ts b/packages/@n8n/api-types/src/dto/roles/role-delete-query.dto.ts new file mode 100644 index 00000000000..bdd2539b038 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/roles/role-delete-query.dto.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; + +import { Z } from '../../zod-class'; + +/** + * Query DTO for deleting a role. When the role still has users assigned, + * `reassignRoleSlug` selects the role those users are moved to before deletion. + */ +export class RoleDeleteQueryDto extends Z.class({ + reassignRoleSlug: z.string().min(1).optional(), +}) {} diff --git a/packages/@n8n/db/src/repositories/role.repository.ts b/packages/@n8n/db/src/repositories/role.repository.ts index dcd8fd09775..9646448418d 100644 --- a/packages/@n8n/db/src/repositories/role.repository.ts +++ b/packages/@n8n/db/src/repositories/role.repository.ts @@ -196,6 +196,35 @@ export class RoleRepository extends Repository { } } + /** + * Reassign every user currently holding `role` to `toSlug`, then delete `role`, + * all within a single transaction so users are never left orphaned by a partial failure. + */ + async reassignUsersAndRemove(role: Role, toSlug: string) { + await this.manager.transaction(async (trx) => { + if (role.roleType === 'global') { + await trx + .createQueryBuilder() + .update(User) + .set({ role: { slug: toSlug } }) + .where('roleSlug = :fromSlug', { fromSlug: role.slug }) + .execute(); + } else if (role.roleType === 'project') { + await trx + .createQueryBuilder() + .update(ProjectRelation) + .set({ role: { slug: toSlug } }) + .where('role = :fromSlug', { fromSlug: role.slug }) + .execute(); + } + + const result = await trx.delete(Role, { slug: role.slug }); + if (result.affected !== 1) { + throw new Error(`Failed to delete role "${role.slug}"`); + } + }); + } + private async updateEntityWithManager( entityManager: EntityManager, slug: string, diff --git a/packages/cli/src/controllers/__tests__/role.controller.test.ts b/packages/cli/src/controllers/__tests__/role.controller.test.ts index 92308d2d5b6..bd8945bb3b4 100644 --- a/packages/cli/src/controllers/__tests__/role.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/role.controller.test.ts @@ -69,13 +69,25 @@ describe('RoleController', () => { slug: 'custom-editor', } as Role); - await controller.deleteRole(request, mock(), 'custom-editor'); + await controller.deleteRole(request, mock(), 'custom-editor', mock()); expect(eventService.emit).toHaveBeenCalledWith('custom-role-deleted', { userId: '123', roleSlug: 'custom-editor', }); }); + + it('should pass the reassignment role through to the service', async () => { + const request = managerRequest(); + roleService.getRole.mockResolvedValue({ roleType: 'global' } as Role); + roleService.removeCustomRole.mockResolvedValue({ slug: 'custom-editor' } as Role); + + await controller.deleteRole(request, mock(), 'custom-editor', { + reassignRoleSlug: 'global:member', + }); + + expect(roleService.removeCustomRole).toHaveBeenCalledWith('custom-editor', 'global:member'); + }); }); }); }); diff --git a/packages/cli/src/controllers/role.controller.ts b/packages/cli/src/controllers/role.controller.ts index ae27cfa0999..5f0639ad961 100644 --- a/packages/cli/src/controllers/role.controller.ts +++ b/packages/cli/src/controllers/role.controller.ts @@ -1,6 +1,7 @@ import { CreateRoleDto, RoleAssignmentsResponseDto, + RoleDeleteQueryDto, RoleGetQueryDto, RoleListQueryDto, RoleMembersResponseDto, @@ -132,10 +133,11 @@ export class RoleController { req: AuthenticatedRequest, _res: Response, @Param('slug') slug: string, + @Query query: RoleDeleteQueryDto, ): Promise { const role = await this.roleService.getRole(slug); this.assertCanManageRoleType(req.user, role.roleType); - const result = await this.roleService.removeCustomRole(slug); + const result = await this.roleService.removeCustomRole(slug, query.reassignRoleSlug); this.eventService.emit('custom-role-deleted', { userId: req.user.id, roleSlug: result.slug, diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index 4f1b0caf69e..374798cbaa6 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -139,7 +139,7 @@ export class RoleService { return { members }; } - async removeCustomRole(slug: string) { + async removeCustomRole(slug: string, reassignRoleSlug?: string) { const role = await this.roleRepository.findBySlug(slug); if (!role) { throw new NotFoundError('Role not found'); @@ -150,7 +150,7 @@ export class RoleService { // Check if any users is globally or project assigned to the role const usersWithRole = await this.roleRepository.countUsersWithRole(role); - if (usersWithRole > 0) { + if (usersWithRole > 0 && !reassignRoleSlug) { throw new BadRequestError('Cannot delete role assigned to users'); } @@ -161,7 +161,11 @@ export class RoleService { throw new BadRequestError(`Cannot delete role: ${blockers.join('; ')}`); } - await this.roleRepository.removeBySlug(slug); + if (usersWithRole > 0 && reassignRoleSlug) { + await this.reassignUsersAndRemoveRole(role, reassignRoleSlug); + } else { + await this.roleRepository.removeBySlug(slug); + } // Invalidate cache after role deletion await this.roleCacheService.invalidateCache(); @@ -169,6 +173,22 @@ export class RoleService { return this.dbRoleToRoleDTO(role); } + private async reassignUsersAndRemoveRole(role: Role, reassignRoleSlug: string) { + if (reassignRoleSlug === role.slug) { + throw new BadRequestError('Cannot reassign users to the role being deleted'); + } + + const reassignRole = await this.roleRepository.findBySlug(reassignRoleSlug); + if (!reassignRole) { + throw new BadRequestError(`Reassignment role "${reassignRoleSlug}" does not exist`); + } + if (reassignRole.roleType !== role.roleType) { + throw new BadRequestError('Reassignment role must be of the same type as the deleted role'); + } + + await this.roleRepository.reassignUsersAndRemove(role, reassignRoleSlug); + } + private async resolveScopes( scopeSlugs: string[] | undefined, roleType: 'project' | 'global', diff --git a/packages/cli/test/integration/controllers/role.controller.test.ts b/packages/cli/test/integration/controllers/role.controller.test.ts index 640da0a9ac3..c666137811c 100644 --- a/packages/cli/test/integration/controllers/role.controller.test.ts +++ b/packages/cli/test/integration/controllers/role.controller.test.ts @@ -1128,6 +1128,32 @@ describe('RoleController', () => { expect(roleService.removeCustomRole).toHaveBeenCalledTimes(1); }); + it('should forward the reassignRoleSlug query param to the service', async () => { + // + // ARRANGE + // + const roleSlug = 'global:test-role'; + roleService.removeCustomRole.mockResolvedValue({ + slug: roleSlug, + displayName: 'Deleted Role', + description: null, + systemRole: false, + roleType: 'global', + scopes: [], + licensed: true, + }); + + // + // ACT + // + await ownerAgent.delete(`/roles/${roleSlug}?reassignRoleSlug=global:member`).expect(200); + + // + // ASSERT + // + expect(roleService.removeCustomRole).toHaveBeenCalledWith(roleSlug, 'global:member'); + }); + it('should handle service errors gracefully', async () => { // // ARRANGE diff --git a/packages/cli/test/integration/services/role.service.test.ts b/packages/cli/test/integration/services/role.service.test.ts index f9874b697c3..db82cd0d2a1 100644 --- a/packages/cli/test/integration/services/role.service.test.ts +++ b/packages/cli/test/integration/services/role.service.test.ts @@ -1371,6 +1371,113 @@ describe('RoleService', () => { ); }); + it('should reassign globally assigned users to another role, then delete', async () => { + // + // ARRANGE + // + const testScopes = await createTestScopes(); + const roleInUse = await createCustomRoleWithScopes([testScopes.readScope], { + displayName: 'Role In Use', + roleType: 'global', + }); + const targetRole = await createRole({ + displayName: 'Target Role', + roleType: 'global', + systemRole: false, + }); + + const user = await createMember(); + user.role = roleInUse; + await userRepository.save(user); + + // + // ACT + // + const result = await roleService.removeCustomRole(roleInUse.slug, targetRole.slug); + + // + // ASSERT + // + expect(result.slug).toBe(roleInUse.slug); + expect(await roleRepository.findBySlug(roleInUse.slug)).toBeNull(); + + const reassignedUser = await userRepository.findOneOrFail({ + where: { id: user.id }, + relations: ['role'], + }); + expect(reassignedUser.role.slug).toBe(targetRole.slug); + }); + + it('should throw when the reassignment role does not exist', async () => { + const testScopes = await createTestScopes(); + const roleInUse = await createCustomRoleWithScopes([testScopes.readScope], { + displayName: 'Role In Use', + roleType: 'global', + }); + const user = await createMember(); + user.role = roleInUse; + await userRepository.save(user); + + await expect( + roleService.removeCustomRole(roleInUse.slug, 'global:does-not-exist'), + ).rejects.toThrow('Reassignment role "global:does-not-exist" does not exist'); + + // Role is preserved when reassignment fails. + expect(await roleRepository.findBySlug(roleInUse.slug)).not.toBeNull(); + }); + + it('should throw when the reassignment role is of a different type', async () => { + const testScopes = await createTestScopes(); + const roleInUse = await createCustomRoleWithScopes([testScopes.readScope], { + displayName: 'Global Role In Use', + roleType: 'global', + }); + const projectTarget = await createCustomRoleWithScopes([testScopes.readScope], { + displayName: 'Project Target', + roleType: 'project', + }); + const user = await createMember(); + user.role = roleInUse; + await userRepository.save(user); + + await expect( + roleService.removeCustomRole(roleInUse.slug, projectTarget.slug), + ).rejects.toThrow('Reassignment role must be of the same type as the deleted role'); + }); + + it('should throw when reassigning users to the role being deleted', async () => { + const testScopes = await createTestScopes(); + const roleInUse = await createCustomRoleWithScopes([testScopes.readScope], { + displayName: 'Role In Use', + roleType: 'global', + }); + const user = await createMember(); + user.role = roleInUse; + await userRepository.save(user); + + await expect(roleService.removeCustomRole(roleInUse.slug, roleInUse.slug)).rejects.toThrow( + 'Cannot reassign users to the role being deleted', + ); + }); + + it('should ignore the reassignment role when no users are assigned', async () => { + const targetRole = await createRole({ + displayName: 'Target Role', + roleType: 'global', + systemRole: false, + }); + const unusedRole = await createRole({ + displayName: 'Unused Role', + roleType: 'global', + systemRole: false, + }); + + const result = await roleService.removeCustomRole(unusedRole.slug, targetRole.slug); + + expect(result.slug).toBe(unusedRole.slug); + expect(await roleRepository.findBySlug(unusedRole.slug)).toBeNull(); + }); + describe('when referenced by an SSO role mapping rule', () => { let roleMappingRuleRepository: RoleMappingRuleRepository; diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 1f8f96aef50..dc8d7c49d86 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -2911,6 +2911,13 @@ "roles.instance.action.fetch.error": "Error fetching role", "roles.instance.action.delete.warning": "Deleting this role is permanent and cannot be undone.", "roles.instance.action.delete.button": "Delete role", + "roles.instance.action.delete.reassign.title": "Delete {roleName} role", + "roles.instance.action.delete.reassign.userCount": "{count} user | {count} users", + "roles.instance.action.delete.reassign.description": "are currently assigned to this role. Choose a role to reassign them to before deleting.", + "roles.instance.action.delete.reassign.label": "Reassign users to", + "roles.instance.action.delete.reassign.placeholder": "Select a role", + "roles.instance.action.delete.reassign.confirm": "Delete and reassign users", + "roles.instance.action.delete.reassign.confirmWithRole": "Delete role and reassign users to {roleName}", "roles.instance.action.create.success": "Role created successfully", "roles.instance.action.create.error": "Couldn't create role", "roles.create.validation.nameMinLength": "Enter a name of at least {min} characters", diff --git a/packages/frontend/@n8n/rest-api-client/src/api/roles.ts b/packages/frontend/@n8n/rest-api-client/src/api/roles.ts index ad320785eaf..bc23121c542 100644 --- a/packages/frontend/@n8n/rest-api-client/src/api/roles.ts +++ b/packages/frontend/@n8n/rest-api-client/src/api/roles.ts @@ -33,8 +33,13 @@ export const updateRole = async ( return await makeRestApiRequest(context, 'PATCH', `/roles/${slug}`, body); }; -export const deleteRole = async (context: IRestApiContext, slug: string): Promise => { - return await makeRestApiRequest(context, 'DELETE', `/roles/${slug}`); +export const deleteRole = async ( + context: IRestApiContext, + slug: string, + reassignRoleSlug?: string, +): Promise => { + const query = reassignRoleSlug ? `?reassignRoleSlug=${encodeURIComponent(reassignRoleSlug)}` : ''; + return await makeRestApiRequest(context, 'DELETE', `/roles/${slug}${query}`); }; export const getRoleAssignments = async ( diff --git a/packages/frontend/@n8n/stores/src/roles.store.ts b/packages/frontend/@n8n/stores/src/roles.store.ts index 256e6b7cd43..a5ac7c9d2a7 100644 --- a/packages/frontend/@n8n/stores/src/roles.store.ts +++ b/packages/frontend/@n8n/stores/src/roles.store.ts @@ -101,8 +101,8 @@ export const useRolesStore = defineStore('roles', () => { return await rolesApi.getRoleBySlug(rootStore.restApiContext, payload); }; - const deleteRole = async (slug: string): Promise => { - return await rolesApi.deleteRole(rootStore.restApiContext, slug); + const deleteRole = async (slug: string, reassignRoleSlug?: string): Promise => { + return await rolesApi.deleteRole(rootStore.restApiContext, slug, reassignRoleSlug); }; const updateRole = async (slug: string, body: UpdateRoleDto): Promise => { diff --git a/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.test.ts b/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.test.ts new file mode 100644 index 00000000000..ed6afe61e45 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.test.ts @@ -0,0 +1,201 @@ +import { createTestingPinia } from '@pinia/testing'; +import { mount } from '@vue/test-utils'; +import { defineComponent } from 'vue'; +import { useRolesStore } from '@/app/stores/roles.store'; +import { mockedStore, waitAllPromises, type MockedStore } from '@/__tests__/utils'; +import type { Role } from '@n8n/permissions'; +import { useRoleDeletion } from './useRoleDeletion'; + +const mockShowError = vi.fn(); +const mockShowMessage = vi.fn(); +const mockConfirm = vi.fn(); +const mockTrack = vi.fn(); +const mockPush = vi.fn(); + +vi.mock('@/app/composables/useToast', () => ({ + useToast: () => ({ showError: mockShowError, showMessage: mockShowMessage }), +})); + +vi.mock('@/app/composables/useMessage', () => ({ + useMessage: () => ({ confirm: mockConfirm }), +})); + +vi.mock('@/app/composables/useTelemetry', () => ({ + useTelemetry: () => ({ track: mockTrack }), +})); + +vi.mock('vue-router', async () => { + const actual = await vi.importActual('vue-router'); + return { + ...actual, + useRouter: () => ({ push: mockPush }), + }; +}); + +function withSetup() { + let result!: ReturnType; + const comp = defineComponent({ + setup() { + result = useRoleDeletion(); + return () => null; + }, + }); + const wrapper = mount(comp); + return { result, unmount: () => wrapper.unmount() }; +} + +const mockRole: Role = { + displayName: 'Support', + slug: 'support', + description: 'A custom instance role', + scopes: ['user:read'], + licensed: true, + systemRole: false, + roleType: 'global', + usedByUsers: 3, +}; + +let rolesStore: MockedStore; + +describe('useRoleDeletion', () => { + beforeEach(() => { + vi.clearAllMocks(); + createTestingPinia(); + rolesStore = mockedStore(useRolesStore); + }); + + describe('reassign-and-delete flow', () => { + it('opens the reassign modal instead of deleting immediately when a global role has assigned users', async () => { + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...mockRole, usedByUsers: 3 }); + const { result, unmount } = withSetup(); + + await result.requestDelete(mockRole, { roleType: 'global' }); + + expect(result.reassignState.value).toEqual({ + role: mockRole, + userCount: 3, + roleType: 'global', + redirectTo: undefined, + }); + expect(rolesStore.deleteRole).not.toHaveBeenCalled(); + unmount(); + }); + + it('calls deleteRole with both the deleted role slug and the chosen reassignment slug', async () => { + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...mockRole, usedByUsers: 3 }); + rolesStore.deleteRole.mockResolvedValue(mockRole); + rolesStore.roles.global = [mockRole]; + + const { result, unmount } = withSetup(); + await result.requestDelete(mockRole, { roleType: 'global' }); + + result.confirmReassignDelete('global:member'); + await waitAllPromises(); + + expect(rolesStore.deleteRole).toHaveBeenCalledWith('support', 'global:member'); + unmount(); + }); + + it('clears the pending reassign state as soon as a target role is selected', async () => { + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...mockRole, usedByUsers: 3 }); + rolesStore.deleteRole.mockResolvedValue(mockRole); + rolesStore.roles.global = [mockRole]; + + const { result, unmount } = withSetup(); + await result.requestDelete(mockRole, { roleType: 'global' }); + expect(result.reassignState.value).not.toBeNull(); + + result.confirmReassignDelete('global:member'); + + expect(result.reassignState.value).toBeNull(); + unmount(); + }); + + it('on success: removes the role locally, refetches roles, shows a success toast, tracks telemetry, and redirects', async () => { + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...mockRole, usedByUsers: 3 }); + rolesStore.deleteRole.mockResolvedValue(mockRole); + rolesStore.roles.global = [mockRole]; + rolesStore.fetchRoles.mockResolvedValue(); + + const redirectTo = { name: 'roles-settings' }; + const { result, unmount } = withSetup(); + await result.requestDelete(mockRole, { roleType: 'global', redirectTo }); + + result.confirmReassignDelete('global:member'); + await waitAllPromises(); + + expect(rolesStore.roles.global).toEqual([]); + // Reassignment shifts the target role's member count, so roles are refetched. + expect(rolesStore.fetchRoles).toHaveBeenCalled(); + expect(mockShowMessage).toHaveBeenCalledWith({ + title: 'Role deleted', + type: 'success', + }); + expect(mockTrack).toHaveBeenCalledWith( + 'User successfully deleted role', + expect.objectContaining({ role_id: mockRole.slug, role_name: mockRole.displayName }), + ); + expect(mockPush).toHaveBeenCalledWith(redirectTo); + unmount(); + }); + + it('on failure: shows an error toast and does not remove the role locally', async () => { + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...mockRole, usedByUsers: 3 }); + const error = new Error('boom'); + rolesStore.deleteRole.mockRejectedValue(error); + rolesStore.roles.global = [mockRole]; + + const { result, unmount } = withSetup(); + await result.requestDelete(mockRole, { roleType: 'global' }); + + result.confirmReassignDelete('global:member'); + await waitAllPromises(); + + expect(rolesStore.roles.global).toEqual([mockRole]); + expect(mockShowError).toHaveBeenCalledWith(error, 'Error deleting role'); + expect(mockShowMessage).not.toHaveBeenCalled(); + unmount(); + }); + }); + + describe('standard delete flow (no assigned users / project roles)', () => { + it('deletes immediately without a reassignment slug once confirmed', async () => { + const projectRole = { ...mockRole, roleType: 'project' as const, usedByUsers: undefined }; + mockConfirm.mockResolvedValue('confirm'); + rolesStore.deleteRole.mockResolvedValue(projectRole); + rolesStore.roles.project = [projectRole]; + + const { result, unmount } = withSetup(); + await result.requestDelete(projectRole, { roleType: 'project' }); + + expect(rolesStore.deleteRole).toHaveBeenCalledWith('support', undefined); + expect(result.reassignState.value).toBeNull(); + unmount(); + }); + + it('does not delete when the confirmation dialog is dismissed', async () => { + mockConfirm.mockResolvedValue('cancel'); + const projectRole = { ...mockRole, roleType: 'project' as const, usedByUsers: undefined }; + + const { result, unmount } = withSetup(); + await result.requestDelete(projectRole, { roleType: 'project' }); + + expect(rolesStore.deleteRole).not.toHaveBeenCalled(); + unmount(); + }); + }); + + describe('onBeforeDelete guard', () => { + it('aborts the deletion when the guard returns false', async () => { + const onBeforeDelete = vi.fn().mockResolvedValue(false); + + const { result, unmount } = withSetup(); + await result.requestDelete(mockRole, { roleType: 'global', onBeforeDelete }); + + expect(onBeforeDelete).toHaveBeenCalledWith(mockRole); + expect(rolesStore.fetchRoleBySlug).not.toHaveBeenCalled(); + expect(rolesStore.deleteRole).not.toHaveBeenCalled(); + unmount(); + }); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.ts b/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.ts new file mode 100644 index 00000000000..08fef7338c3 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/roles/composables/useRoleDeletion.ts @@ -0,0 +1,153 @@ +import { useMessage } from '@/app/composables/useMessage'; +import { useTelemetry } from '@/app/composables/useTelemetry'; +import { useToast } from '@/app/composables/useToast'; +import { MODAL_CONFIRM } from '@/app/constants'; +import { useRolesStore } from '@/app/stores/roles.store'; +import { useI18n } from '@n8n/i18n'; +import type { Role } from '@n8n/permissions'; +import { ref } from 'vue'; +import { useRouter, type RouteLocationRaw } from 'vue-router'; + +type RoleType = 'global' | 'project'; + +interface RequestDeleteOptions { + roleType: RoleType; + /** Where to navigate after a successful deletion (e.g. the detail view returns to the list). */ + redirectTo?: RouteLocationRaw; + /** + * Guard run before anything else. Return `false` to abort the deletion + * (e.g. when the role is still in use and can't be deleted). + */ + onBeforeDelete?: (role: Role) => Promise; +} + +/** A pending reassign-then-delete, carrying what the modal needs plus the follow-up. */ +interface ReassignState extends Pick { + role: Role; + userCount: number; +} + +/** + * Encapsulates the full role-deletion UX so every entry point (roles list, role detail) + * behaves identically: guard → for instance roles with assigned users open the + * reassign-then-delete modal, otherwise confirm and delete. + */ +export function useRoleDeletion() { + const rolesStore = useRolesStore(); + const { showError, showMessage } = useToast(); + const message = useMessage(); + const i18n = useI18n(); + const telemetry = useTelemetry(); + const router = useRouter(); + + /** When set, an instance role with assigned users is awaiting reassignment in the modal. */ + const reassignState = ref(null); + + /** + * Fetch the current number of users assigned to a role. Returns `null` (and surfaces + * an error toast) if the lookup fails, so callers can abort rather than act on a + * stale, page-load snapshot. + */ + async function resolveAssignedUserCount(slug: string): Promise { + try { + const role = await rolesStore.fetchRoleBySlug({ slug }); + return role.usedByUsers ?? 0; + } catch (error) { + showError(error, i18n.baseText('roles.action.delete.error')); + return null; + } + } + + async function performRoleDeletion(options: { + role: Role; + roleType: RoleType; + reassignRoleSlug?: string; + redirectTo?: RouteLocationRaw; + }): Promise { + const { role, roleType, reassignRoleSlug, redirectTo } = options; + + try { + await rolesStore.deleteRole(role.slug, reassignRoleSlug); + + const index = rolesStore.roles[roleType].findIndex((r) => r.slug === role.slug); + if (index !== -1) { + rolesStore.roles[roleType].splice(index, 1); + } + + // Reassignment shifts the target role's member count, so refetch to reconcile. + if (reassignRoleSlug) { + void rolesStore.fetchRoles(); + } + + showMessage({ title: i18n.baseText('roles.action.delete.success'), type: 'success' }); + telemetry.track('User successfully deleted role', { + role_id: role.slug, + role_name: role.displayName, + role_type: roleType === 'global' ? 'instance' : 'project', + permissions: role.scopes, + }); + + if (redirectTo) { + void router.push(redirectTo); + } + } catch (error) { + showError(error, i18n.baseText('roles.action.delete.error')); + } + } + + /** + * Entry point for deleting a role. Instance roles with assigned users open the + * reassign modal (resolved later via {@link confirmReassignDelete}); everything else + * goes through a confirmation dialog and is deleted immediately. + */ + async function requestDelete(role: Role, options: RequestDeleteOptions): Promise { + const { roleType, redirectTo, onBeforeDelete } = options; + + if (onBeforeDelete) { + const proceed = await onBeforeDelete(role); + if (!proceed) return; + } + + if (roleType === 'global') { + const userCount = await resolveAssignedUserCount(role.slug); + if (userCount === null) return; + + if (userCount > 0) { + reassignState.value = { role, userCount, roleType, redirectTo }; + return; + } + } + + const confirmed = await message.confirm( + i18n.baseText('roles.action.delete.text', { interpolate: { roleName: role.displayName } }), + i18n.baseText('roles.action.delete.title', { interpolate: { roleName: role.displayName } }), + { + type: 'warning', + confirmButtonText: i18n.baseText('roles.action.delete'), + cancelButtonText: i18n.baseText('roles.action.cancel'), + }, + ); + if (confirmed !== MODAL_CONFIRM) return; + + await performRoleDeletion({ role, roleType, redirectTo }); + } + + /** Complete a pending reassign-then-delete with the chosen replacement role. */ + function confirmReassignDelete(reassignRoleSlug: string): void { + const state = reassignState.value; + if (!state) return; + reassignState.value = null; + void performRoleDeletion({ + role: state.role, + roleType: state.roleType, + reassignRoleSlug, + redirectTo: state.redirectTo, + }); + } + + function cancelReassign(): void { + reassignState.value = null; + } + + return { reassignState, requestDelete, confirmReassignDelete, cancelReassign }; +} diff --git a/packages/frontend/editor-ui/src/features/roles/composables/useRolesListActions.ts b/packages/frontend/editor-ui/src/features/roles/composables/useRolesListActions.ts index 6dc589f303d..acf1b40da66 100644 --- a/packages/frontend/editor-ui/src/features/roles/composables/useRolesListActions.ts +++ b/packages/frontend/editor-ui/src/features/roles/composables/useRolesListActions.ts @@ -1,11 +1,10 @@ -import { useMessage } from '@/app/composables/useMessage'; import { useTelemetry } from '@/app/composables/useTelemetry'; import { useToast } from '@/app/composables/useToast'; -import { MODAL_CONFIRM } from '@/app/constants'; import { useRolesStore } from '@/app/stores/roles.store'; import { useI18n } from '@n8n/i18n'; import type { Role } from '@n8n/permissions'; import { useRouter } from 'vue-router'; +import { useRoleDeletion } from './useRoleDeletion'; export interface RoleListViews { /** Route name for read-only system-role detail page. */ @@ -31,12 +30,10 @@ export function useRolesListActions({ }: UseRolesListActionsOptions) { const { showError, showMessage } = useToast(); const rolesStore = useRolesStore(); - const message = useMessage(); const i18n = useI18n(); const telemetry = useTelemetry(); const router = useRouter(); - - const storeKey = roleType === 'project' ? 'project' : 'global'; + const { reassignState, requestDelete, confirmReassignDelete, cancelReassign } = useRoleDeletion(); async function duplicateRole(item: Role): Promise { try { @@ -51,7 +48,7 @@ export function useRolesListActions({ }); // Optimistic update — background fetch reconciles with server state. - rolesStore.roles[storeKey].push(role); + rolesStore.roles[roleType].push(role); void rolesStore.fetchRoles(); telemetry.track('User duplicated role', { @@ -75,39 +72,7 @@ export function useRolesListActions({ } async function deleteRole(item: Role): Promise { - if (onBeforeDelete) { - const proceed = await onBeforeDelete(item); - if (!proceed) return; - } - - const confirmed = await message.confirm( - i18n.baseText('roles.action.delete.text', { - interpolate: { roleName: item.displayName }, - }), - i18n.baseText('roles.action.delete.title', { - interpolate: { roleName: item.displayName }, - }), - { - type: 'warning', - confirmButtonText: i18n.baseText('roles.action.delete'), - cancelButtonText: i18n.baseText('roles.action.cancel'), - }, - ); - - if (confirmed !== MODAL_CONFIRM) return; - - try { - await rolesStore.deleteRole(item.slug); - - const index = rolesStore.roles[storeKey].findIndex((r) => r.slug === item.slug); - if (index !== -1) { - rolesStore.roles[storeKey].splice(index, 1); - } - - showMessage({ title: i18n.baseText('roles.action.delete.success'), type: 'success' }); - } catch (error) { - showError(error, i18n.baseText('roles.action.delete.error')); - } + await requestDelete(item, { roleType, onBeforeDelete }); } const actions = { duplicate: duplicateRole, delete: deleteRole } as const; @@ -132,5 +97,15 @@ export function useRolesListActions({ }); } - return { duplicateRole, deleteRole, actions, rowActions, handleAction, handleRowClick }; + return { + duplicateRole, + deleteRole, + actions, + rowActions, + handleAction, + handleRowClick, + reassignState, + confirmReassignDelete, + cancelReassign, + }; } diff --git a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.vue b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.vue index d43e99ae614..f077f28f274 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.vue +++ b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRoleView.vue @@ -10,8 +10,10 @@ import { computed, toRaw } from 'vue'; import { useRouter } from 'vue-router'; import RoleEditorLayout, { type RoleEditorLabels } from '../components/RoleEditorLayout.vue'; +import { useRoleDeletion } from '../composables/useRoleDeletion'; import { useRoleEditorForm } from '../composables/useRoleEditorForm'; import InstanceRoleAssignmentsTab from './InstanceRoleAssignmentsTab.vue'; +import DeleteInstanceRoleModal from './components/DeleteInstanceRoleModal.vue'; import ScopeGroupSelector from './components/ScopeGroupSelector.vue'; import { ALL_INSTANCE_SCOPES } from './instanceRoleScopes'; @@ -21,6 +23,7 @@ const { showMessage, showError } = useToast(); const i18n = useI18n(); const message = useMessage(); const telemetry = useTelemetry(); +const { reassignState, requestDelete, confirmReassignDelete, cancelReassign } = useRoleDeletion(); const props = defineProps<{ roleSlug?: string }>(); @@ -61,6 +64,10 @@ const editorLabels = computed(() => ({ // System roles populate the preset buttons (clicking copies their scopes). const presetRoles = computed(() => rolesStore.processedInstanceRoles.filter((r) => r.systemRole)); +const reassignTargetRoles = computed(() => + rolesStore.processedInstanceRoles.filter((r) => r.slug !== reassignState.value?.role.slug), +); + function onBackClick() { void router.push({ name: VIEWS.ROLES_SETTINGS, query: { tab: 'instance' } }); } @@ -180,42 +187,10 @@ async function handleSubmit() { async function deleteRole() { if (!initialState.value) return; - const deleteConfirmed = await message.confirm( - i18n.baseText('roles.action.delete.text', { - interpolate: { roleName: initialState.value.displayName }, - }), - i18n.baseText('roles.action.delete.title', { - interpolate: { roleName: initialState.value.displayName }, - }), - { - type: 'warning', - confirmButtonText: i18n.baseText('roles.action.delete'), - cancelButtonText: i18n.baseText('roles.action.cancel'), - }, - ); - - if (deleteConfirmed !== MODAL_CONFIRM) return; - - try { - await rolesStore.deleteRole(initialState.value.slug); - - const index = rolesStore.roles.global.findIndex( - (role) => role.slug === initialState.value?.slug, - ); - if (index !== -1) { - rolesStore.roles.global.splice(index, 1); - } - - showMessage({ title: i18n.baseText('roles.action.delete.success'), type: 'success' }); - telemetry.track('User successfully deleted role', { - role_id: initialState.value.slug, - role_name: initialState.value.displayName, - permissions: initialState.value.scopes, - }); - void router.push({ name: VIEWS.ROLES_SETTINGS, query: { tab: 'instance' } }); - } catch (error) { - showError(error, i18n.baseText('roles.action.delete.error')); - } + await requestDelete(initialState.value, { + roleType: 'global', + redirectTo: { name: VIEWS.ROLES_SETTINGS, query: { tab: 'instance' } }, + }); } @@ -278,6 +253,15 @@ async function deleteRole() {
+ + diff --git a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.test.ts b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.test.ts index d92c2edb90b..b9d04188305 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.test.ts +++ b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.test.ts @@ -23,12 +23,26 @@ vi.mock('@/app/composables/useMessage', () => ({ useMessage: () => ({ confirm: vi.fn() }), })); +// Render the dialog only when open so presence assertions reflect the modal's visibility. +const ElDialogStub = { + props: ['modelValue'], + template: ` +
+ + + +
+ `, +}; + const renderComponent = createComponentRenderer(InstanceRolesView, { global: { stubs: { RouterLink: { template: '', }, + ElDialog: ElDialogStub, + N8nSelect: true, }, }, }); @@ -128,4 +142,35 @@ describe('InstanceRolesView', () => { expect.objectContaining({ roleType: 'global', scopes: [] }), ); }); + + it('should open the reassign modal when deleting a custom role with assigned users', async () => { + const customRole = { ...mockCustomRoles[0], usedByUsers: 3 }; + rolesStore.processedInstanceRoles = [...mockSystemRoles, customRole]; + rolesStore.roles.global = [...mockSystemRoles, customRole]; + // The delete flow fetches the current count before deciding. + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...customRole, usedByUsers: 3 }); + + const { getByTestId, getAllByTestId } = renderComponent(); + await userEvent.click(within(getAllByTestId('action-toggle')[0]).getByRole('button')); + await userEvent.click(getByTestId('action-delete')); + + expect(rolesStore.fetchRoleBySlug).toHaveBeenCalledWith({ slug: customRole.slug }); + expect(getByTestId('delete-instance-role-modal')).toBeInTheDocument(); + // No deletion happens just from opening the modal. + expect(rolesStore.deleteRole).not.toHaveBeenCalled(); + }); + + it('should not open the reassign modal when the custom role has no assigned users', async () => { + const customRole = { ...mockCustomRoles[0], usedByUsers: 0 }; + rolesStore.processedInstanceRoles = [customRole]; + rolesStore.roles.global = [customRole]; + rolesStore.fetchRoleBySlug.mockResolvedValue({ ...customRole, usedByUsers: 0 }); + + const { queryByTestId, getByTestId, getAllByTestId } = renderComponent(); + await userEvent.click(within(getAllByTestId('action-toggle')[0]).getByRole('button')); + await userEvent.click(getByTestId('action-delete')); + + // Falls back to the standard confirm dialog rather than the reassign modal. + expect(queryByTestId('delete-instance-role-modal')).not.toBeInTheDocument(); + }); }); diff --git a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.vue b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.vue index 22bc9e537f6..09efa27965f 100644 --- a/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.vue +++ b/packages/frontend/editor-ui/src/features/roles/instance/InstanceRolesView.vue @@ -1,19 +1,34 @@ diff --git a/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.test.ts b/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.test.ts new file mode 100644 index 00000000000..d895706e94a --- /dev/null +++ b/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.test.ts @@ -0,0 +1,117 @@ +import { createComponentRenderer } from '@/__tests__/render'; +import { createPinia, setActivePinia } from 'pinia'; +import { waitFor } from '@testing-library/vue'; +import userEvent from '@testing-library/user-event'; +import type { Role } from '@n8n/permissions'; +import DeleteInstanceRoleModal from './DeleteInstanceRoleModal.vue'; + +// The real ElDialog teleports to #app-modals; stub it so the modal content renders inline. +const ElDialogStub = { + props: ['modelValue'], + template: ` +
+ + + +
+ `, +}; + +const role: Role = { + displayName: 'Support Agent', + slug: 'global:support-agent', + description: null, + scopes: [], + licensed: true, + systemRole: false, + roleType: 'global', +}; + +const availableRoles: Role[] = [ + { + displayName: 'Admin', + slug: 'global:admin', + description: null, + scopes: [], + licensed: true, + systemRole: true, + roleType: 'global', + }, + { + displayName: 'Member', + slug: 'global:member', + description: null, + scopes: [], + licensed: true, + systemRole: true, + roleType: 'global', + }, +]; + +const renderComponent = createComponentRenderer(DeleteInstanceRoleModal, { + props: { modelValue: true, role, userCount: 3, availableRoles }, + global: { + stubs: { + ElDialog: ElDialogStub, + }, + }, +}); + +describe('DeleteInstanceRoleModal', () => { + beforeEach(() => { + vi.clearAllMocks(); + setActivePinia(createPinia()); + }); + + it('should show the role name, user count and reassignment prompt', () => { + const { getByText } = renderComponent(); + + expect(getByText('Delete Support Agent role')).toBeInTheDocument(); + expect(getByText('3 users')).toBeInTheDocument(); + expect(getByText(/are currently assigned to this role/)).toBeInTheDocument(); + }); + + it('should use the singular form for a single assigned user', () => { + const { getByText } = renderComponent({ + props: { modelValue: true, role, userCount: 1, availableRoles }, + }); + + expect(getByText('1 user')).toBeInTheDocument(); + }); + + it('should disable confirm until a role is picked, with a neutral default label', () => { + const { getByTestId } = renderComponent(); + + const confirmButton = getByTestId('confirm-delete-reassign-role'); + expect(confirmButton).toBeDisabled(); + expect(confirmButton).toHaveTextContent('Delete and reassign users'); + }); + + it('should reflect the chosen role in the confirm label and emit on confirm', async () => { + const { getByTestId, getByText, emitted } = renderComponent(); + + // N8nSelect (ElSelect): open the dropdown, then pick the option. + await userEvent.click(getByTestId('reassign-role-select')); + await waitFor(() => expect(getByText('Admin')).toBeInTheDocument()); + await userEvent.click(getByText('Admin')); + + const confirmButton = getByTestId('confirm-delete-reassign-role'); + await waitFor(() => + expect(confirmButton).toHaveTextContent('Delete role and reassign users to Admin'), + ); + expect(confirmButton).toBeEnabled(); + + await userEvent.click(confirmButton); + + expect(emitted().confirm).toEqual([['global:admin']]); + }); + + it('should close without emitting confirm when cancelled', async () => { + const { getByTestId, emitted } = renderComponent(); + + await userEvent.click(getByTestId('cancel-delete-role')); + + expect(emitted().confirm).toBeUndefined(); + expect(emitted()['update:modelValue']).toEqual([[false]]); + }); +}); diff --git a/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.vue b/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.vue new file mode 100644 index 00000000000..6523b08f545 --- /dev/null +++ b/packages/frontend/editor-ui/src/features/roles/instance/components/DeleteInstanceRoleModal.vue @@ -0,0 +1,142 @@ + + + + + diff --git a/packages/frontend/editor-ui/src/features/roles/project/ProjectRolesView.test.ts b/packages/frontend/editor-ui/src/features/roles/project/ProjectRolesView.test.ts index def7ac306bb..c6155f1dec4 100644 --- a/packages/frontend/editor-ui/src/features/roles/project/ProjectRolesView.test.ts +++ b/packages/frontend/editor-ui/src/features/roles/project/ProjectRolesView.test.ts @@ -326,7 +326,7 @@ describe('ProjectRolesView', () => { }, ); - expect(rolesStore.deleteRole).toHaveBeenCalledWith(mockRole.slug); + expect(rolesStore.deleteRole).toHaveBeenCalledWith(mockRole.slug, undefined); expect(rolesStore.roles.project).not.toContain(mockRole); expect(mockShowMessage).toHaveBeenCalledWith({ title: 'Role deleted', type: 'success' }); }); @@ -371,7 +371,7 @@ describe('ProjectRolesView', () => { const deleteButton = getByTestId('action-delete'); await userEvent.click(deleteButton); - expect(rolesStore.deleteRole).toHaveBeenCalledWith(mockRole.slug); + expect(rolesStore.deleteRole).toHaveBeenCalledWith(mockRole.slug, undefined); expect(mockShowError).toHaveBeenCalledWith(error, 'Error deleting role'); expect(rolesStore.roles.project).toHaveLength(1); expect(rolesStore.roles.project[0]).toStrictEqual(mockRole); @@ -397,7 +397,7 @@ describe('ProjectRolesView', () => { const deleteButton = getByTestId('action-delete'); await userEvent.click(deleteButton); - expect(rolesStore.deleteRole).toHaveBeenCalledWith('non-existent-role'); + expect(rolesStore.deleteRole).toHaveBeenCalledWith('non-existent-role', undefined); expect(mockShowMessage).toHaveBeenCalledWith({ title: 'Role deleted', type: 'success' }); // Store should remain unchanged since role wasn't found expect(rolesStore.roles.project).toHaveLength(1);