github-mirror/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2026-05-30 00:07:02 +02:00
Code Issues Actions 16 Packages Projects Releases Wiki Activity Graph

refactor(core): Remove admin API token scopes when user is demoted from admin to member via SSO (#22359)

Browse Source
This commit is contained in:
Konstantin Tieber 2025-11-27 11:14:48 +01:00 committed by GitHub
parent 7ea31c34b1
commit e1f154d7a8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 89 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
packages/@n8n/db/src/repositories/user.repository.ts
View File

@ -44,6 +44,9 @@ export class UserRepository extends Repository<User> {
* 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<Repository<User>['update']>) {
return await super.update(...args);

2
packages/cli/src/controllers/users.controller.ts
View File

@ -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,

22
packages/cli/src/modules/provisioning.ee/__tests__/provisioning.service.ee.test.ts
View File

@ -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<GlobalConfig>();
const settingsRepository = mock<SettingsRepository>();
const userRepository = mock<UserRepository>();
const userService = mock<UserService>();
const entityManager = mock<EntityManager>();
const projectRepository = mock<ProjectRepository>({ manager: entityManager });
const projectService = mock<ProjectService>();
@ -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<Role>({ 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<User>({ role: { slug: 'global:member' } });
const roleSlug = 'global:owner';
roleRepository.findOneOrFail.mockResolvedValue(
mock<Role>({ 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<User>({ role: { slug: 'global:owner' } });
const roleSlug = 'global:owner';
roleRepository.findOneOrFail.mockResolvedValue(
mock<Role>({ 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<User>({ role: { slug: 'global:member' } });
const roleSlug = 'global:owner';
roleRepository.findOneOrFail.mockResolvedValue(
mock<Role>({ 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`,

4
packages/cli/src/modules/provisioning.ee/provisioning.service.ee.ts
View File

@ -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,

73
packages/cli/src/services/__tests__/user.service.test.ts
View File

@ -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<EntityManager>();
const userRepository = mockInstance(UserRepository, {
manager: mock<EntityManager>({
transaction: async (cb) =>
typeof cb === 'function' ? await cb(mock<EntityManager>()) : await Promise.resolve(),
}),
manager,
});
const roleService = mock<RoleService>();
const mailer = mock<UserManagementMailer>();
const publicApiKeyService = mock<PublicApiKeyService>();
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<unknown>;
return await runInTransaction(mock<EntityManager>());
});
});
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<unknown>;
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();
});
});
});

10
packages/cli/src/services/user.service.ts
View File

@ -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);
}
});
}