diff --git a/packages/@n8n/db/src/repositories/user.repository.ts b/packages/@n8n/db/src/repositories/user.repository.ts index f20e992fd46..0837778377f 100644 --- a/packages/@n8n/db/src/repositories/user.repository.ts +++ b/packages/@n8n/db/src/repositories/user.repository.ts @@ -44,6 +44,9 @@ export class UserRepository extends Repository { * With `update` it would only receive the updated fields, e.g. the `id` * would be missing. test('does not use `Repository.update`, but * `Repository.save` instead'. + * + * Also don't use this method to change a user's role. + * Use `UserService.changeUserRole` instead. */ async update(...args: Parameters['update']>) { return await super.update(...args); diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index feab0f89b72..8e10d2817fc 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -344,7 +344,7 @@ export class UsersController { throw new ForbiddenError(NO_OWNER_ON_OWNER); } - await this.userService.changeUserRole(req.user, targetUser, payload); + await this.userService.changeUserRole(targetUser, payload); this.eventService.emit('user-changed-role', { userId: req.user.id, diff --git a/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts b/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts index b23f60691a2..17c007b6af8 100644 --- a/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts +++ b/packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts @@ -20,10 +20,12 @@ import { type ProjectService } from '@/services/project.service.ee'; import type { EntityManager } from '@n8n/typeorm'; import { type InstanceSettings } from 'n8n-core'; import { type EventService } from '@/events/event.service'; +import { type UserService } from '@/services/user.service'; const globalConfig = mock(); const settingsRepository = mock(); const userRepository = mock(); +const userService = mock(); const entityManager = mock(); const projectRepository = mock({ manager: entityManager }); const projectService = mock(); @@ -42,6 +44,7 @@ const provisioningService = new ProvisioningService( projectService, roleRepository, userRepository, + userService, logger, publisher, instanceSettings, @@ -230,55 +233,52 @@ describe('ProvisioningService', () => { roleRepository.findOneOrFail.mockResolvedValue( mock({ slug: 'global:member', roleType: 'global' }), ); - provisioningService['isInstanceRoleProvisioningEnabled'] = jest.fn().mockResolvedValue(true); await provisioningService.provisionInstanceRoleForUser(user, roleSlug); - expect(userRepository.update).toHaveBeenCalledWith(user.id, { role: { slug: roleSlug } }); + + expect(userService.changeUserRole).toHaveBeenCalledWith(user, { newRoleName: roleSlug }); expect(logger.warn).not.toHaveBeenCalled(); }); it('should provision the instance role for the user', async () => { const user = mock({ role: { slug: 'global:member' } }); const roleSlug = 'global:owner'; - roleRepository.findOneOrFail.mockResolvedValue( mock({ slug: 'global:owner', roleType: 'global' }), ); - provisioningService['isInstanceRoleProvisioningEnabled'] = jest.fn().mockResolvedValue(true); await provisioningService.provisionInstanceRoleForUser(user, roleSlug); - expect(userRepository.update).toHaveBeenCalledWith(user.id, { role: { slug: roleSlug } }); + + expect(userService.changeUserRole).toHaveBeenCalledWith(user, { newRoleName: roleSlug }); }); it('should do nothing if the role has not changed', async () => { const user = mock({ role: { slug: 'global:owner' } }); const roleSlug = 'global:owner'; - roleRepository.findOneOrFail.mockResolvedValue( mock({ slug: 'global:owner', roleType: 'global' }), ); - provisioningService['isInstanceRoleProvisioningEnabled'] = jest.fn().mockResolvedValue(true); await provisioningService.provisionInstanceRoleForUser(user, roleSlug); - expect(userRepository.update).not.toHaveBeenCalled(); + + expect(userService.changeUserRole).not.toHaveBeenCalled(); expect(logger.warn).not.toHaveBeenCalled(); }); it('should do nothing if the role is not a global role', async () => { const user = mock({ role: { slug: 'global:member' } }); const roleSlug = 'global:owner'; - roleRepository.findOneOrFail.mockResolvedValue( mock({ slug: 'global:owner', roleType: 'project' }), ); - provisioningService['isInstanceRoleProvisioningEnabled'] = jest.fn().mockResolvedValue(true); await provisioningService.provisionInstanceRoleForUser(user, roleSlug); - expect(userRepository.update).not.toHaveBeenCalled(); + + expect(userService.changeUserRole).not.toHaveBeenCalled(); expect(logger.warn).toHaveBeenCalledTimes(1); expect(logger.warn).toHaveBeenCalledWith( `Skipping instance role provisioning. Role ${roleSlug} is not a global role`, diff --git a/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts b/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts index 93cc8f18c88..e130be6d2ec 100644 --- a/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts +++ b/packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts @@ -21,6 +21,7 @@ import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ZodError } from 'zod'; import { ProjectService } from '@/services/project.service.ee'; import { InstanceSettings } from 'n8n-core'; +import { UserService } from '@/services/user.service'; @Service() export class ProvisioningService { @@ -34,6 +35,7 @@ export class ProvisioningService { private readonly projectService: ProjectService, private readonly roleRepository: RoleRepository, private readonly userRepository: UserRepository, + private readonly userService: UserService, private readonly logger: Logger, private readonly publisher: Publisher, private readonly instanceSettings: InstanceSettings, @@ -109,7 +111,7 @@ export class ProvisioningService { // No need to update record if the role hasn't changed if (user.role.slug !== dbRole.slug) { - await this.userRepository.update(user.id, { role: { slug: dbRole.slug } }); + await this.userService.changeUserRole(user, { newRoleName: dbRole.slug }); this.eventService.emit('sso-user-instance-role-updated', { userId: user.id, diff --git a/packages/cli/src/services/__tests__/user.service.test.ts b/packages/cli/src/services/__tests__/user.service.test.ts index 01beac3d71d..ae9c7a19b19 100644 --- a/packages/cli/src/services/__tests__/user.service.test.ts +++ b/packages/cli/src/services/__tests__/user.service.test.ts @@ -1,7 +1,7 @@ import { mockInstance } from '@n8n/backend-test-utils'; import { GlobalConfig } from '@n8n/config'; import type { Project } from '@n8n/db'; -import { GLOBAL_ADMIN_ROLE, GLOBAL_MEMBER_ROLE, User, UserRepository } from '@n8n/db'; +import { GLOBAL_ADMIN_ROLE, GLOBAL_MEMBER_ROLE, Role, User, UserRepository } from '@n8n/db'; import type { EntityManager } from '@n8n/typeorm'; import { mock } from 'jest-mock-extended'; import { v4 as uuid } from 'uuid'; @@ -12,6 +12,7 @@ import { UserService } from '@/services/user.service'; import type { UserManagementMailer } from '@/user-management/email'; import type { RoleService } from '../role.service'; +import { type PublicApiKeyService } from '../public-api-key.service'; describe('UserService', () => { const globalConfig = mockInstance(GlobalConfig, { @@ -23,21 +24,20 @@ describe('UserService', () => { editorBaseUrl: '', }); const urlService = new UrlService(globalConfig); + const manager = mock(); const userRepository = mockInstance(UserRepository, { - manager: mock({ - transaction: async (cb) => - typeof cb === 'function' ? await cb(mock()) : await Promise.resolve(), - }), + manager, }); const roleService = mock(); const mailer = mock(); + const publicApiKeyService = mock(); const userService = new UserService( mock(), userRepository, mailer, urlService, mock(), - mock(), + publicApiKeyService, roleService, globalConfig, ); @@ -50,6 +50,11 @@ describe('UserService', () => { afterEach(() => { jest.clearAllMocks(); + // Restore default transaction implementation after each test (because some mock it) + manager.transaction.mockImplementation(async (arg1: unknown, arg2?: unknown) => { + const runInTransaction = (arg2 ?? arg1) as (entityManager: EntityManager) => Promise; + return await runInTransaction(mock()); + }); }); describe('toPublic', () => { @@ -322,4 +327,60 @@ describe('UserService', () => { ); }); }); + + describe('changeUserRole', () => { + beforeEach(() => { + jest.clearAllMocks(); + manager.transaction.mockImplementation(async (arg1: unknown, arg2?: unknown) => { + const runInTransaction = (arg2 ?? arg1) as ( + entityManager: EntityManager, + ) => Promise; + return await runInTransaction(manager); + }); + }); + + it('throws an error if provided user role does not exist', async () => { + const user = new User(); + user.role = new Role(); + user.role.slug = 'global:member'; + + await expect( + userService.changeUserRole(user, { newRoleName: 'global:invalid' }), + ).rejects.toThrowError('Role nonexistent:role does not exist'); + }); + + it('updates the role of the given user', async () => { + const user = new User(); + user.id = uuid(); + user.role = new Role(); + user.role.slug = 'global:member'; + roleService.checkRolesExist.mockResolvedValueOnce(); + + await userService.changeUserRole(user, { newRoleName: 'global:admin' }); + + expect(manager.update).toHaveBeenCalledWith( + User, + { id: user.id }, + { role: { slug: 'global:admin' } }, + ); + expect(publicApiKeyService.removeOwnerOnlyScopesFromApiKeys).not.toHaveBeenCalled(); + }); + + it('removes higher privilege scopes from API tokens of user who is demoted from admin', async () => { + const user = new User(); + user.id = uuid(); + user.role = new Role(); + user.role.slug = 'global:admin'; + roleService.checkRolesExist.mockResolvedValueOnce(); + + await userService.changeUserRole(user, { newRoleName: 'global:member' }); + + expect(manager.update).toHaveBeenCalledWith( + User, + { id: user.id }, + { role: { slug: 'global:member' } }, + ); + expect(publicApiKeyService.removeOwnerOnlyScopesFromApiKeys).toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/services/user.service.ts b/packages/cli/src/services/user.service.ts index bab3d284c3f..69f46fbbd5c 100644 --- a/packages/cli/src/services/user.service.ts +++ b/packages/cli/src/services/user.service.ts @@ -269,20 +269,18 @@ export class UserService { return { usersInvited, usersCreated: toCreateUsers.map(({ email }) => email) }; } - async changeUserRole(user: User, targetUser: User, newRole: RoleChangeRequestDto) { + async changeUserRole(user: User, newRole: RoleChangeRequestDto) { // Check that new role exists await this.roleService.checkRolesExist([newRole.newRoleName], 'global'); return await this.userRepository.manager.transaction(async (trx) => { - await trx.update(User, { id: targetUser.id }, { role: { slug: newRole.newRoleName } }); + await trx.update(User, { id: user.id }, { role: { slug: newRole.newRoleName } }); const adminDowngradedToMember = - user.role.slug === 'global:owner' && - targetUser.role.slug === 'global:admin' && - newRole.newRoleName === 'global:member'; + user.role.slug === 'global:admin' && newRole.newRoleName === 'global:member'; if (adminDowngradedToMember) { - await this.publicApiKeyService.removeOwnerOnlyScopesFromApiKeys(targetUser, trx); + await this.publicApiKeyService.removeOwnerOnlyScopesFromApiKeys(user, trx); } }); }