From f49aebb740f49cabdad51bb481d51d279c85e3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 11 Dec 2023 13:35:09 +0100 Subject: [PATCH] refactor(core): Move instance owner retrieval to ownership service (no-changelog) (#7980) ## Summary Provide details about your pull request and what it adds, fixes, or changes. Photos and videos are recommended. Continue breaking down `UserManagementHelper.ts` ... #### How to test the change: 1. ... ## Issues fixed Include links to Github issue or Community forum post or **Linear ticket**: > Important in order to close automatically and provide context to reviewers ... ## Review / Merge checklist - [ ] PR title and summary are descriptive. **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** ([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md)) - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [ ] Tests included. > A bug is not considered fixed, unless a test is added to prevent it from happening again. A feature is not complete without tests. > > *(internal)* You can use Slack commands to trigger [e2e tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227) or [deploy test instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce) or [deploy early access version on Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e). --- .../UserManagement/UserManagementHelper.ts | 12 ------- packages/cli/src/commands/execute.ts | 4 +-- packages/cli/src/commands/executeBatch.ts | 4 +-- .../sourceControlGit.service.ee.ts | 11 +++--- .../cli/src/services/ownership.service.ts | 17 ++++++--- .../unit/services/ownership.service.test.ts | 35 +++++++++++++++---- 6 files changed, 52 insertions(+), 31 deletions(-) diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index 7ba50b9c2..17ac7509b 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -8,7 +8,6 @@ import { MAX_PASSWORD_LENGTH, MIN_PASSWORD_LENGTH } from '@db/entities/User'; import config from '@/config'; import { License } from '@/License'; import { getWebhookBaseUrl } from '@/WebhookHelpers'; -import { RoleService } from '@/services/role.service'; import { UserRepository } from '@db/repositories/user.repository'; import type { Scope } from '@n8n/permissions'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -18,17 +17,6 @@ export function isSharingEnabled(): boolean { return Container.get(License).isSharingEnabled(); } -export async function getInstanceOwner() { - const globalOwnerRole = await Container.get(RoleService).findGlobalOwnerRole(); - - return Container.get(UserRepository).findOneOrFail({ - relations: ['globalRole'], - where: { - globalRoleId: globalOwnerRole.id, - }, - }); -} - /** * Return the n8n instance base URL without trailing slash. */ diff --git a/packages/cli/src/commands/execute.ts b/packages/cli/src/commands/execute.ts index e33be3e4e..c298639fb 100644 --- a/packages/cli/src/commands/execute.ts +++ b/packages/cli/src/commands/execute.ts @@ -7,11 +7,11 @@ import { ApplicationError, ExecutionBaseError } from 'n8n-workflow'; import { ActiveExecutions } from '@/ActiveExecutions'; import { WorkflowRunner } from '@/WorkflowRunner'; import type { IWorkflowExecutionDataProcess } from '@/Interfaces'; -import { getInstanceOwner } from '@/UserManagement/UserManagementHelper'; import { findCliWorkflowStart, isWorkflowIdValid } from '@/utils'; import { BaseCommand } from './BaseCommand'; import { Container } from 'typedi'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; +import { OwnershipService } from '@/services/ownership.service'; export class Execute extends BaseCommand { static description = '\nExecutes a given workflow'; @@ -98,7 +98,7 @@ export class Execute extends BaseCommand { const startingNode = findCliWorkflowStart(workflowData.nodes); - const user = await getInstanceOwner(); + const user = await Container.get(OwnershipService).getInstanceOwner(); const runData: IWorkflowExecutionDataProcess = { executionMode: 'cli', startNodes: [startingNode.name], diff --git a/packages/cli/src/commands/executeBatch.ts b/packages/cli/src/commands/executeBatch.ts index c146bea8f..e931ce8e9 100644 --- a/packages/cli/src/commands/executeBatch.ts +++ b/packages/cli/src/commands/executeBatch.ts @@ -12,7 +12,6 @@ import { ActiveExecutions } from '@/ActiveExecutions'; import { WorkflowRunner } from '@/WorkflowRunner'; import type { IWorkflowDb, IWorkflowExecutionDataProcess } from '@/Interfaces'; import type { User } from '@db/entities/User'; -import { getInstanceOwner } from '@/UserManagement/UserManagementHelper'; import { findCliWorkflowStart } from '@/utils'; import { BaseCommand } from './BaseCommand'; import { Container } from 'typedi'; @@ -24,6 +23,7 @@ import type { IWorkflowExecutionProgress, } from '../types/commands.types'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; +import { OwnershipService } from '@/services/ownership.service'; const re = /\d+/; @@ -276,7 +276,7 @@ export class ExecuteBatch extends BaseCommand { ExecuteBatch.githubWorkflow = true; } - ExecuteBatch.instanceOwner = await getInstanceOwner(); + ExecuteBatch.instanceOwner = await Container.get(OwnershipService).getInstanceOwner(); const query = Container.get(WorkflowRepository).createQueryBuilder('workflows'); diff --git a/packages/cli/src/environments/sourceControl/sourceControlGit.service.ee.ts b/packages/cli/src/environments/sourceControl/sourceControlGit.service.ee.ts index 056badb83..4a440f14e 100644 --- a/packages/cli/src/environments/sourceControl/sourceControlGit.service.ee.ts +++ b/packages/cli/src/environments/sourceControl/sourceControlGit.service.ee.ts @@ -20,9 +20,9 @@ import { } from './constants'; import { sourceControlFoldersExistCheck } from './sourceControlHelper.ee'; import type { User } from '@db/entities/User'; -import { getInstanceOwner } from '../../UserManagement/UserManagementHelper'; import { Logger } from '@/Logger'; import { ApplicationError } from 'n8n-workflow'; +import { OwnershipService } from '@/services/ownership.service'; @Service() export class SourceControlGitService { @@ -30,7 +30,10 @@ export class SourceControlGitService { private gitOptions: Partial = {}; - constructor(private readonly logger: Logger) {} + constructor( + private readonly logger: Logger, + private readonly ownershipService: OwnershipService, + ) {} /** * Run pre-checks before initialising git @@ -103,8 +106,8 @@ export class SourceControlGitService { } if (!(await this.hasRemote(sourceControlPreferences.repositoryUrl))) { if (sourceControlPreferences.connected && sourceControlPreferences.repositoryUrl) { - const user = await getInstanceOwner(); - await this.initRepository(sourceControlPreferences, user); + const instanceOwner = await this.ownershipService.getInstanceOwner(); + await this.initRepository(sourceControlPreferences, instanceOwner); } } } diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index 3bda4d04f..5850a9198 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -3,7 +3,7 @@ import { CacheService } from './cache.service'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; import type { User } from '@db/entities/User'; import { RoleService } from './role.service'; -import { UserService } from './user.service'; +import { UserRepository } from '@/databases/repositories/user.repository'; import type { ListQuery } from '@/requests'; import { ApplicationError } from 'n8n-workflow'; @@ -11,7 +11,7 @@ import { ApplicationError } from 'n8n-workflow'; export class OwnershipService { constructor( private cacheService: CacheService, - private userService: UserService, + private userRepository: UserRepository, private roleService: RoleService, private sharedWorkflowRepository: SharedWorkflowRepository, ) {} @@ -20,9 +20,9 @@ export class OwnershipService { * Retrieve the user who owns the workflow. Note that workflow ownership is **immutable**. */ async getWorkflowOwnerCached(workflowId: string) { - const cachedValue = (await this.cacheService.get(`cache:workflow-owner:${workflowId}`)) as User; + const cachedValue = await this.cacheService.get(`cache:workflow-owner:${workflowId}`); - if (cachedValue) return this.userService.create(cachedValue); + if (cachedValue) return this.userRepository.create(cachedValue); const workflowOwnerRole = await this.roleService.findWorkflowOwnerRole(); @@ -67,4 +67,13 @@ export class OwnershipService { return entity; } + + async getInstanceOwner() { + const globalOwnerRole = await this.roleService.findGlobalOwnerRole(); + + return this.userRepository.findOneOrFail({ + where: { globalRoleId: globalOwnerRole.id }, + relations: ['globalRole'], + }); + } } diff --git a/packages/cli/test/unit/services/ownership.service.test.ts b/packages/cli/test/unit/services/ownership.service.test.ts index 601dccc66..983ca278a 100644 --- a/packages/cli/test/unit/services/ownership.service.test.ts +++ b/packages/cli/test/unit/services/ownership.service.test.ts @@ -2,10 +2,8 @@ import { OwnershipService } from '@/services/ownership.service'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; import { Role } from '@db/entities/Role'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; -import { CacheService } from '@/services/cache.service'; import { User } from '@db/entities/User'; import { RoleService } from '@/services/role.service'; -import { UserService } from '@/services/user.service'; import { CredentialsEntity } from '@db/entities/CredentialsEntity'; import type { SharedCredentials } from '@db/entities/SharedCredentials'; import { mockInstance } from '../../shared/mocking'; @@ -16,6 +14,8 @@ import { randomName, } from '../../integration/shared/random'; import { WorkflowEntity } from '@/databases/entities/WorkflowEntity'; +import { UserRepository } from '@/databases/repositories/user.repository'; +import { mock } from 'jest-mock-extended'; const wfOwnerRole = () => Object.assign(new Role(), { @@ -31,26 +31,33 @@ const mockCredRole = (name: 'owner' | 'editor'): Role => id: randomInteger(), }); +const mockInstanceOwnerRole = () => + Object.assign(new Role(), { + scope: 'global', + name: 'owner', + id: randomInteger(), + }); + const mockCredential = (): CredentialsEntity => Object.assign(new CredentialsEntity(), randomCredentialPayload()); -const mockUser = (): User => +const mockUser = (attributes?: Partial): User => Object.assign(new User(), { id: randomInteger(), email: randomEmail(), firstName: randomName(), lastName: randomName(), + ...attributes, }); describe('OwnershipService', () => { - const cacheService = mockInstance(CacheService); const roleService = mockInstance(RoleService); - const userService = mockInstance(UserService); + const userRepository = mockInstance(UserRepository); const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository); const ownershipService = new OwnershipService( - cacheService, - userService, + mock(), + userRepository, roleService, sharedWorkflowRepository, ); @@ -174,4 +181,18 @@ describe('OwnershipService', () => { expect(sharedWith).toHaveLength(0); }); }); + + describe('getInstanceOwner()', () => { + test('should find owner using global owner role ID', async () => { + const instanceOwnerRole = mockInstanceOwnerRole(); + roleService.findGlobalOwnerRole.mockResolvedValue(instanceOwnerRole); + + await ownershipService.getInstanceOwner(); + + expect(userRepository.findOneOrFail).toHaveBeenCalledWith({ + where: { globalRoleId: instanceOwnerRole.id }, + relations: ['globalRole'], + }); + }); + }); });