From 96a9de68a009f066341668d6352dbba6dbac6a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 22 Aug 2023 15:58:05 +0200 Subject: [PATCH] refactor(core): Move all user DB access to `UserRepository` (#6910) Prep for https://linear.app/n8n/issue/PAY-646 --- packages/cli/src/Server.ts | 28 ++++++--- .../src/UserManagement/PermissionChecker.ts | 4 +- packages/cli/src/WorkflowHelpers.ts | 4 +- .../cli/src/controllers/auth.controller.ts | 14 ++--- packages/cli/src/controllers/me.controller.ts | 41 +++++-------- .../cli/src/controllers/owner.controller.ts | 12 ++-- .../controllers/passwordReset.controller.ts | 36 +++++------ .../cli/src/controllers/users.controller.ts | 54 +++++++--------- .../src/credentials/credentials.service.ee.ts | 4 +- packages/cli/src/services/events.service.ts | 4 +- .../cli/src/services/ownership.service.ts | 7 ++- packages/cli/src/services/user.service.ts | 61 +++++++++++++++++++ packages/cli/src/user/user.service.ts | 31 ---------- .../src/workflows/workflows.services.ee.ts | 4 +- .../integration/shared/utils/testServer.ts | 25 +++++--- .../cli/test/unit/PermissionChecker.test.ts | 14 +++-- .../unit/controllers/me.controller.test.ts | 23 +++---- .../unit/controllers/owner.controller.test.ts | 13 ++-- .../test/unit/services/events.service.test.ts | 7 ++- .../unit/services/ownership.service.test.ts | 7 ++- 20 files changed, 209 insertions(+), 184 deletions(-) create mode 100644 packages/cli/src/services/user.service.ts delete mode 100644 packages/cli/src/user/user.service.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index b682e1d8b5..dd627551a0 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -167,8 +167,6 @@ import { SourceControlService } from '@/environments/sourceControl/sourceControl import { SourceControlController } from '@/environments/sourceControl/sourceControl.controller.ee'; import { ExecutionRepository } from '@db/repositories'; import type { ExecutionEntity } from '@db/entities/ExecutionEntity'; -import { JwtService } from './services/jwt.service'; -import { RoleService } from './services/role.service'; const exec = promisify(callbackExec); @@ -485,22 +483,34 @@ export class Server extends AbstractServer { const internalHooks = Container.get(InternalHooks); const mailer = Container.get(UserManagementMailer); const postHog = this.postHog; - const jwtService = Container.get(JwtService); const controllers: object[] = [ new EventBusController(), - new AuthController({ config, internalHooks, repositories, logger, postHog }), - new OwnerController({ config, internalHooks, repositories, logger, postHog }), - new MeController({ externalHooks, internalHooks, repositories, logger }), + new AuthController({ + config, + internalHooks, + repositories, + logger, + postHog, + }), + new OwnerController({ + config, + internalHooks, + repositories, + logger, + }), + new MeController({ + externalHooks, + internalHooks, + logger, + }), new NodeTypesController({ config, nodeTypes }), new PasswordResetController({ config, externalHooks, internalHooks, mailer, - repositories, logger, - jwtService, }), Container.get(TagsController), new TranslationController(config, this.credentialTypes), @@ -513,8 +523,6 @@ export class Server extends AbstractServer { activeWorkflowRunner, logger, postHog, - jwtService, - roleService: Container.get(RoleService), }), Container.get(SamlController), Container.get(SourceControlController), diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 493a0859f9..4f4eb2bed3 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -11,7 +11,7 @@ import config from '@/config'; import type { SharedCredentials } from '@db/entities/SharedCredentials'; import { isSharingEnabled } from './UserManagementHelper'; import { WorkflowsService } from '@/workflows/workflows.services'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { OwnershipService } from '@/services/ownership.service'; import Container from 'typedi'; import { RoleService } from '@/services/role.service'; @@ -135,7 +135,7 @@ export class PermissionChecker { } if (policy === 'workflowsFromSameOwner') { - const user = await UserService.get({ id: userId }); + const user = await Container.get(UserService).findOne({ where: { id: userId } }); if (!user) { throw new WorkflowOperationError( 'Fatal error: user not found. Please contact the system administrator.', diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 0d5140d2ac..3e8c887e8f 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -39,7 +39,7 @@ import omit from 'lodash/omit'; // eslint-disable-next-line import/no-cycle import { PermissionChecker } from './UserManagement/PermissionChecker'; import { isWorkflowIdValid } from './utils'; -import { UserService } from './user/user.service'; +import { UserService } from './services/user.service'; import type { SharedWorkflow } from '@db/entities/SharedWorkflow'; import type { RoleNames } from '@db/entities/Role'; import { RoleService } from './services/role.service'; @@ -517,7 +517,7 @@ export async function isBelowOnboardingThreshold(user: User): Promise { // user is above threshold --> set flag in settings if (!belowThreshold) { - void UserService.updateUserSettings(user.id, { isOnboarded: true }); + void Container.get(UserService).updateSettings(user.id, { isOnboarded: true }); } return belowThreshold; diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index dc43ff167a..0ee765760b 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -29,9 +29,9 @@ import { isLdapCurrentAuthenticationMethod, isSamlCurrentAuthenticationMethod, } from '@/sso/ssoHelpers'; -import type { UserRepository } from '@db/repositories'; import { InternalHooks } from '../InternalHooks'; import { License } from '@/License'; +import { UserService } from '@/services/user.service'; @RestController() export class AuthController { @@ -41,7 +41,7 @@ export class AuthController { private readonly internalHooks: IInternalHooksClass; - private readonly userRepository: UserRepository; + private readonly userService: UserService; private readonly postHog?: PostHogClient; @@ -49,7 +49,6 @@ export class AuthController { config, logger, internalHooks, - repositories, postHog, }: { config: Config; @@ -61,8 +60,8 @@ export class AuthController { this.config = config; this.logger = logger; this.internalHooks = internalHooks; - this.userRepository = repositories.User; this.postHog = postHog; + this.userService = Container.get(UserService); } /** @@ -137,10 +136,7 @@ export class AuthController { } try { - user = await this.userRepository.findOneOrFail({ - relations: ['globalRole'], - where: {}, - }); + user = await this.userService.findOneOrFail({ where: {} }); } catch (error) { throw new InternalServerError( 'No users found in database - did you wipe the users table? Create at least one user.', @@ -189,7 +185,7 @@ export class AuthController { } } - const users = await this.userRepository.find({ where: { id: In([inviterId, inviteeId]) } }); + const users = await this.userService.findMany({ where: { id: In([inviterId, inviteeId]) } }); if (users.length !== 2) { this.logger.debug( 'Request to resolve signup token failed because the ID of the inviter and/or the ID of the invitee were not found in database', diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 0ff4cd78f0..bfebaf0a71 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -11,7 +11,6 @@ import { BadRequestError } from '@/ResponseHelper'; import { validateEntity } from '@/GenericHelpers'; import { issueCookie } from '@/auth/jwt'; import type { User } from '@db/entities/User'; -import type { UserRepository } from '@db/repositories'; import { Response } from 'express'; import type { ILogger } from 'n8n-workflow'; import { @@ -20,15 +19,11 @@ import { UserSettingsUpdatePayload, UserUpdatePayload, } from '@/requests'; -import type { - PublicUser, - IDatabaseCollections, - IExternalHooksClass, - IInternalHooksClass, -} from '@/Interfaces'; +import type { PublicUser, IExternalHooksClass, IInternalHooksClass } from '@/Interfaces'; import { randomBytes } from 'crypto'; import { isSamlLicensedAndEnabled } from '../sso/saml/samlHelpers'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; +import Container from 'typedi'; @Authorized() @RestController('/me') @@ -39,23 +34,21 @@ export class MeController { private readonly internalHooks: IInternalHooksClass; - private readonly userRepository: UserRepository; + private readonly userService: UserService; constructor({ logger, externalHooks, internalHooks, - repositories, }: { logger: ILogger; externalHooks: IExternalHooksClass; internalHooks: IInternalHooksClass; - repositories: Pick; }) { this.logger = logger; this.externalHooks = externalHooks; this.internalHooks = internalHooks; - this.userRepository = repositories.User; + this.userService = Container.get(UserService); } /** @@ -99,11 +92,8 @@ export class MeController { } } - await this.userRepository.update(userId, payload); - const user = await this.userRepository.findOneOrFail({ - where: { id: userId }, - relations: { globalRole: true }, - }); + await this.userService.update(userId, payload); + const user = await this.userService.findOneOrFail({ where: { id: userId } }); this.logger.info('User updated successfully', { userId }); @@ -154,7 +144,7 @@ export class MeController { req.user.password = await hashPassword(validPassword); - const user = await this.userRepository.save(req.user); + const user = await this.userService.save(req.user); this.logger.info('Password updated successfully', { userId: user.id }); await issueCookie(res, user); @@ -186,8 +176,9 @@ export class MeController { throw new BadRequestError('Personalization answers are mandatory'); } - await this.userRepository.save({ + await this.userService.save({ id: req.user.id, + // @ts-ignore personalizationAnswers, }); @@ -205,9 +196,7 @@ export class MeController { async createAPIKey(req: AuthenticatedRequest) { const apiKey = `n8n_api_${randomBytes(40).toString('hex')}`; - await this.userRepository.update(req.user.id, { - apiKey, - }); + await this.userService.update(req.user.id, { apiKey }); void this.internalHooks.onApiKeyCreated({ user: req.user, @@ -230,9 +219,7 @@ export class MeController { */ @Delete('/api-key') async deleteAPIKey(req: AuthenticatedRequest) { - await this.userRepository.update(req.user.id, { - apiKey: null, - }); + await this.userService.update(req.user.id, { apiKey: null }); void this.internalHooks.onApiKeyDeleted({ user: req.user, @@ -250,9 +237,9 @@ export class MeController { const payload = plainToInstance(UserSettingsUpdatePayload, req.body); const { id } = req.user; - await UserService.updateUserSettings(id, payload); + await this.userService.updateSettings(id, payload); - const user = await this.userRepository.findOneOrFail({ + const user = await this.userService.findOneOrFail({ select: ['settings'], where: { id }, }); diff --git a/packages/cli/src/controllers/owner.controller.ts b/packages/cli/src/controllers/owner.controller.ts index c09b4c87a5..3437884f1f 100644 --- a/packages/cli/src/controllers/owner.controller.ts +++ b/packages/cli/src/controllers/owner.controller.ts @@ -14,7 +14,9 @@ import type { ILogger } from 'n8n-workflow'; import type { Config } from '@/config'; import { OwnerRequest } from '@/requests'; import type { IDatabaseCollections, IInternalHooksClass } from '@/Interfaces'; -import type { SettingsRepository, UserRepository } from '@db/repositories'; +import type { SettingsRepository } from '@db/repositories'; +import { UserService } from '@/services/user.service'; +import Container from 'typedi'; import type { PostHogClient } from '@/posthog'; @Authorized(['global', 'owner']) @@ -26,7 +28,7 @@ export class OwnerController { private readonly internalHooks: IInternalHooksClass; - private readonly userRepository: UserRepository; + private readonly userService: UserService; private readonly settingsRepository: SettingsRepository; @@ -42,13 +44,13 @@ export class OwnerController { config: Config; logger: ILogger; internalHooks: IInternalHooksClass; - repositories: Pick; + repositories: Pick; postHog?: PostHogClient; }) { this.config = config; this.logger = logger; this.internalHooks = internalHooks; - this.userRepository = repositories.User; + this.userService = Container.get(UserService); this.settingsRepository = repositories.Settings; this.postHog = postHog; } @@ -112,7 +114,7 @@ export class OwnerController { await validateEntity(owner); - owner = await this.userRepository.save(owner); + owner = await this.userService.save(owner); this.logger.info('Owner was set up successfully', { userId }); diff --git a/packages/cli/src/controllers/passwordReset.controller.ts b/packages/cli/src/controllers/passwordReset.controller.ts index 16aef76648..e012f0409a 100644 --- a/packages/cli/src/controllers/passwordReset.controller.ts +++ b/packages/cli/src/controllers/passwordReset.controller.ts @@ -18,18 +18,18 @@ import type { UserManagementMailer } from '@/UserManagement/email'; import { Response } from 'express'; import type { ILogger } from 'n8n-workflow'; import type { Config } from '@/config'; -import type { UserRepository } from '@db/repositories'; import { PasswordResetRequest } from '@/requests'; -import type { IDatabaseCollections, IExternalHooksClass, IInternalHooksClass } from '@/Interfaces'; +import type { IExternalHooksClass, IInternalHooksClass } from '@/Interfaces'; import { issueCookie } from '@/auth/jwt'; import { isLdapEnabled } from '@/Ldap/helpers'; import { isSamlCurrentAuthenticationMethod } from '@/sso/ssoHelpers'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { License } from '@/License'; import { Container } from 'typedi'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; import { TokenExpiredError } from 'jsonwebtoken'; -import type { JwtService, JwtPayload } from '@/services/jwt.service'; +import type { JwtPayload } from '@/services/jwt.service'; +import { JwtService } from '@/services/jwt.service'; @RestController() export class PasswordResetController { @@ -43,34 +43,30 @@ export class PasswordResetController { private readonly mailer: UserManagementMailer; - private readonly userRepository: UserRepository; - private readonly jwtService: JwtService; + private readonly userService: UserService; + constructor({ config, logger, externalHooks, internalHooks, mailer, - repositories, - jwtService, }: { config: Config; logger: ILogger; externalHooks: IExternalHooksClass; internalHooks: IInternalHooksClass; mailer: UserManagementMailer; - repositories: Pick; - jwtService: JwtService; }) { this.config = config; this.logger = logger; this.externalHooks = externalHooks; this.internalHooks = internalHooks; this.mailer = mailer; - this.userRepository = repositories.User; - this.jwtService = jwtService; + this.jwtService = Container.get(JwtService); + this.userService = Container.get(UserService); } /** @@ -105,7 +101,7 @@ export class PasswordResetController { } // User should just be able to reset password if one is already present - const user = await this.userRepository.findOne({ + const user = await this.userService.findOne({ where: { email, password: Not(IsNull()), @@ -154,7 +150,7 @@ export class PasswordResetController { }, ); - const url = await UserService.generatePasswordResetUrl(baseUrl, resetPasswordToken); + const url = this.userService.generatePasswordResetUrl(baseUrl, resetPasswordToken); try { await this.mailer.passwordReset({ @@ -204,10 +200,8 @@ export class PasswordResetController { const decodedToken = this.verifyResetPasswordToken(resetPasswordToken); - const user = await this.userRepository.findOne({ - where: { - id: decodedToken.sub, - }, + const user = await this.userService.findOne({ + where: { id: decodedToken.sub }, relations: ['globalRole'], }); @@ -255,7 +249,7 @@ export class PasswordResetController { const decodedToken = this.verifyResetPasswordToken(resetPasswordToken); - const user = await this.userRepository.findOne({ + const user = await this.userService.findOne({ where: { id: decodedToken.sub }, relations: ['authIdentities'], }); @@ -272,9 +266,7 @@ export class PasswordResetController { const passwordHash = await hashPassword(validPassword); - await this.userRepository.update(user.id, { - password: passwordHash, - }); + await this.userService.update(user.id, { password: passwordHash }); this.logger.info('User password updated successfully', { userId: user.id }); diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index 4af36805f6..93c1a61ff4 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -38,18 +38,14 @@ import type { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; import { AuthIdentity } from '@db/entities/AuthIdentity'; import type { PostHogClient } from '@/posthog'; import { isSamlLicensedAndEnabled } from '../sso/saml/samlHelpers'; -import type { - SharedCredentialsRepository, - SharedWorkflowRepository, - UserRepository, -} from '@db/repositories'; -import { UserService } from '@/user/user.service'; +import type { SharedCredentialsRepository, SharedWorkflowRepository } from '@db/repositories'; import { plainToInstance } from 'class-transformer'; import { License } from '@/License'; import { Container } from 'typedi'; import { RESPONSE_ERROR_MESSAGES } from '@/constants'; -import type { JwtService } from '@/services/jwt.service'; -import type { RoleService } from '@/services/role.service'; +import { JwtService } from '@/services/jwt.service'; +import { RoleService } from '@/services/role.service'; +import { UserService } from '@/services/user.service'; @Authorized(['global', 'owner']) @RestController('/users') @@ -62,8 +58,6 @@ export class UsersController { private internalHooks: IInternalHooksClass; - private userRepository: UserRepository; - private sharedCredentialsRepository: SharedCredentialsRepository; private sharedWorkflowRepository: SharedWorkflowRepository; @@ -78,6 +72,8 @@ export class UsersController { private roleService: RoleService; + private userService: UserService; + constructor({ config, logger, @@ -86,33 +82,29 @@ export class UsersController { repositories, activeWorkflowRunner, mailer, - jwtService, postHog, - roleService, }: { config: Config; logger: ILogger; externalHooks: IExternalHooksClass; internalHooks: IInternalHooksClass; - repositories: Pick; + repositories: Pick; activeWorkflowRunner: ActiveWorkflowRunner; mailer: UserManagementMailer; - jwtService: JwtService; postHog?: PostHogClient; - roleService: RoleService; }) { this.config = config; this.logger = logger; this.externalHooks = externalHooks; this.internalHooks = internalHooks; - this.userRepository = repositories.User; this.sharedCredentialsRepository = repositories.SharedCredentials; this.sharedWorkflowRepository = repositories.SharedWorkflow; this.activeWorkflowRunner = activeWorkflowRunner; this.mailer = mailer; - this.jwtService = jwtService; + this.jwtService = Container.get(JwtService); this.postHog = postHog; - this.roleService = roleService; + this.roleService = Container.get(RoleService); + this.userService = Container.get(UserService); } /** @@ -185,7 +177,7 @@ export class UsersController { } // remove/exclude existing users from creation - const existingUsers = await this.userRepository.find({ + const existingUsers = await this.userService.findMany({ where: { email: In(Object.keys(createUsers)) }, }); existingUsers.forEach((user) => { @@ -202,7 +194,7 @@ export class UsersController { this.logger.debug(total > 1 ? `Creating ${total} user shells...` : 'Creating 1 user shell...'); try { - await this.userRepository.manager.transaction(async (transactionManager) => + await this.userService.getManager().transaction(async (transactionManager) => Promise.all( usersToSetUp.map(async (email) => { const newUser = Object.assign(new User(), { @@ -323,7 +315,7 @@ export class UsersController { const validPassword = validatePassword(password); - const users = await this.userRepository.find({ + const users = await this.userService.findMany({ where: { id: In([inviterId, inviteeId]) }, relations: ['globalRole'], }); @@ -353,7 +345,7 @@ export class UsersController { invitee.lastName = lastName; invitee.password = await hashPassword(validPassword); - const updatedUser = await this.userRepository.save(invitee); + const updatedUser = await this.userService.save(invitee); await issueCookie(res, updatedUser); @@ -371,7 +363,7 @@ export class UsersController { @Authorized('any') @Get('/') async listUsers(req: UserRequest.List) { - const users = await this.userRepository.find({ relations: ['globalRole', 'authIdentities'] }); + const users = await this.userService.findMany({ relations: ['globalRole', 'authIdentities'] }); return users.map( (user): PublicUser => addInviteLinkToUser(sanitizeUser(user, ['personalizationAnswers']), req.user.id), @@ -381,7 +373,7 @@ export class UsersController { @Authorized(['global', 'owner']) @Get('/:id/password-reset-link') async getUserPasswordResetLink(req: UserRequest.PasswordResetLink) { - const user = await this.userRepository.findOneOrFail({ + const user = await this.userService.findOneOrFail({ where: { id: req.params.id }, }); if (!user) { @@ -397,7 +389,7 @@ export class UsersController { const baseUrl = getInstanceBaseUrl(); - const link = await UserService.generatePasswordResetUrl(baseUrl, resetPasswordToken); + const link = this.userService.generatePasswordResetUrl(baseUrl, resetPasswordToken); return { link, }; @@ -410,9 +402,9 @@ export class UsersController { const id = req.params.id; - await UserService.updateUserSettings(id, payload); + await this.userService.updateSettings(id, payload); - const user = await this.userRepository.findOneOrFail({ + const user = await this.userService.findOneOrFail({ select: ['settings'], where: { id }, }); @@ -443,7 +435,7 @@ export class UsersController { ); } - const users = await this.userRepository.find({ + const users = await this.userService.findMany({ where: { id: In([transferId, idToDelete]) }, }); @@ -475,7 +467,7 @@ export class UsersController { if (transferId) { const transferee = users.find((user) => user.id === transferId); - await this.userRepository.manager.transaction(async (transactionManager) => { + await this.userService.getManager().transaction(async (transactionManager) => { // Get all workflow ids belonging to user to delete const sharedWorkflowIds = await transactionManager .getRepository(SharedWorkflow) @@ -550,7 +542,7 @@ export class UsersController { }), ]); - await this.userRepository.manager.transaction(async (transactionManager) => { + await this.userService.getManager().transaction(async (transactionManager) => { const ownedWorkflows = await Promise.all( ownedSharedWorkflows.map(async ({ workflow }) => { if (workflow.active) { @@ -597,7 +589,7 @@ export class UsersController { throw new InternalServerError('Email sending must be set up in order to invite other users'); } - const reinvitee = await this.userRepository.findOneBy({ id: idToReinvite }); + const reinvitee = await this.userService.findOneBy({ id: idToReinvite }); if (!reinvitee) { this.logger.debug( 'Request to reinvite a user failed because the ID of the reinvitee was not found in database', diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index 4e9bb0a9c5..32534da52f 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -4,7 +4,7 @@ import * as Db from '@/Db'; import { CredentialsEntity } from '@db/entities/CredentialsEntity'; import { SharedCredentials } from '@db/entities/SharedCredentials'; import type { User } from '@db/entities/User'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { CredentialsService } from './credentials.service'; import type { CredentialWithSharings } from './credentials.types'; import { RoleService } from '@/services/role.service'; @@ -78,7 +78,7 @@ export class EECredentialsService extends CredentialsService { credential: CredentialsEntity, shareWithIds: string[], ): Promise { - const users = await UserService.getByIds(transaction, shareWithIds); + const users = await Container.get(UserService).getByIds(transaction, shareWithIds); const role = await Container.get(RoleService).findCredentialUserRole(); const newSharedCredentials = users diff --git a/packages/cli/src/services/events.service.ts b/packages/cli/src/services/events.service.ts index 0b800a0d4b..8bdd6c273c 100644 --- a/packages/cli/src/services/events.service.ts +++ b/packages/cli/src/services/events.service.ts @@ -4,7 +4,7 @@ import type { INode, IRun, IWorkflowBase } from 'n8n-workflow'; import { LoggerProxy } from 'n8n-workflow'; import { StatisticsNames } from '@db/entities/WorkflowStatistics'; import { WorkflowStatisticsRepository } from '@db/repositories'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { OwnershipService } from './ownership.service'; @Service() @@ -51,7 +51,7 @@ export class EventsService extends EventEmitter { }; if (!owner.settings?.userActivated) { - await UserService.updateUserSettings(owner.id, { + await Container.get(UserService).updateSettings(owner.id, { firstSuccessfulWorkflowId: workflowId, userActivated: true, }); diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index c756f817e2..76200aebb2 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -1,8 +1,9 @@ import { Service } from 'typedi'; import { CacheService } from './cache.service'; -import { SharedWorkflowRepository, UserRepository } from '@/databases/repositories'; +import { SharedWorkflowRepository } from '@/databases/repositories'; import type { User } from '@/databases/entities/User'; import { RoleService } from './role.service'; +import { UserService } from './user.service'; import type { ListQuery } from '@/requests'; import type { Role } from '@/databases/entities/Role'; @@ -10,7 +11,7 @@ import type { Role } from '@/databases/entities/Role'; export class OwnershipService { constructor( private cacheService: CacheService, - private userRepository: UserRepository, + private userService: UserService, private roleService: RoleService, private sharedWorkflowRepository: SharedWorkflowRepository, ) {} @@ -21,7 +22,7 @@ export class OwnershipService { async getWorkflowOwnerCached(workflowId: string) { const cachedValue = (await this.cacheService.get(`cache:workflow-owner:${workflowId}`)) as User; - if (cachedValue) return this.userRepository.create(cachedValue); + if (cachedValue) return this.userService.create(cachedValue); const workflowOwnerRole = await this.roleService.findWorkflowOwnerRole(); diff --git a/packages/cli/src/services/user.service.ts b/packages/cli/src/services/user.service.ts new file mode 100644 index 0000000000..678482a340 --- /dev/null +++ b/packages/cli/src/services/user.service.ts @@ -0,0 +1,61 @@ +import { Service } from 'typedi'; +import type { EntityManager, FindManyOptions, FindOneOptions, FindOptionsWhere } from 'typeorm'; +import { In } from 'typeorm'; +import { User } from '@db/entities/User'; +import type { IUserSettings } from 'n8n-workflow'; +import { UserRepository } from '@/databases/repositories'; + +@Service() +export class UserService { + constructor(private readonly userRepository: UserRepository) {} + + async findOne(options: FindOneOptions) { + return this.userRepository.findOne({ relations: ['globalRole'], ...options }); + } + + async findOneOrFail(options: FindOneOptions) { + return this.userRepository.findOneOrFail({ relations: ['globalRole'], ...options }); + } + + async findMany(options: FindManyOptions) { + return this.userRepository.find({ relations: ['globalRole'], ...options }); + } + + async findOneBy(options: FindOptionsWhere) { + return this.userRepository.findOneBy(options); + } + + create(data: Partial) { + return this.userRepository.create(data); + } + + async save(user: Partial) { + return this.userRepository.save(user); + } + + async update(userId: string, data: Partial) { + return this.userRepository.update(userId, data); + } + + async getByIds(transaction: EntityManager, ids: string[]) { + return transaction.find(User, { where: { id: In(ids) } }); + } + + getManager() { + return this.userRepository.manager; + } + + async updateSettings(userId: string, newSettings: Partial) { + const { settings } = await this.userRepository.findOneOrFail({ where: { id: userId } }); + + return this.userRepository.update(userId, { settings: { ...settings, ...newSettings } }); + } + + generatePasswordResetUrl(instanceBaseUrl: string, token: string) { + const url = new URL(`${instanceBaseUrl}/change-password`); + + url.searchParams.append('token', token); + + return url.toString(); + } +} diff --git a/packages/cli/src/user/user.service.ts b/packages/cli/src/user/user.service.ts deleted file mode 100644 index cb04758e3b..0000000000 --- a/packages/cli/src/user/user.service.ts +++ /dev/null @@ -1,31 +0,0 @@ -import type { EntityManager, FindOptionsWhere } from 'typeorm'; -import { In } from 'typeorm'; -import * as Db from '@/Db'; -import { User } from '@db/entities/User'; -import type { IUserSettings } from 'n8n-workflow'; - -export class UserService { - static async get(where: FindOptionsWhere): Promise { - return Db.collections.User.findOne({ - relations: ['globalRole'], - where, - }); - } - - static async getByIds(transaction: EntityManager, ids: string[]) { - return transaction.find(User, { where: { id: In(ids) } }); - } - - static async updateUserSettings(id: string, userSettings: Partial) { - const { settings: currentSettings } = await Db.collections.User.findOneOrFail({ - where: { id }, - }); - return Db.collections.User.update(id, { settings: { ...currentSettings, ...userSettings } }); - } - - static async generatePasswordResetUrl(instanceBaseUrl: string, token: string): Promise { - const url = new URL(`${instanceBaseUrl}/change-password`); - url.searchParams.append('token', token); - return url.toString(); - } -} diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index 91c1b44daf..ab5e5d0aac 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -7,7 +7,7 @@ import type { ICredentialsDb } from '@/Interfaces'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; import type { User } from '@db/entities/User'; import { WorkflowEntity } from '@db/entities/WorkflowEntity'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { WorkflowsService } from './workflows.services'; import type { CredentialUsedByWorkflow, @@ -61,7 +61,7 @@ export class EEWorkflowsService extends WorkflowsService { workflow: WorkflowEntity, shareWithIds: string[], ): Promise { - const users = await UserService.getByIds(transaction, shareWithIds); + const users = await Container.get(UserService).getByIds(transaction, shareWithIds); const role = await Container.get(RoleService).findWorkflowEditorRole(); const newSharedWorkflows = users.reduce((acc, user) => { diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index c96422698e..ab098e5c64 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -50,7 +50,6 @@ import { AUTHLESS_ENDPOINTS, PUBLIC_API_REST_PATH_SEGMENT, REST_PATH_SEGMENT } f import type { EndpointGroup, SetupProps, TestServer } from '../types'; import { mockInstance } from './mocking'; import { JwtService } from '@/services/jwt.service'; -import { RoleService } from '@/services/role.service'; import { MetricsService } from '@/services/metrics.service'; /** @@ -198,7 +197,12 @@ export const setupTestServer = ({ registerController( app, config, - new AuthController({ config, logger, internalHooks, repositories }), + new AuthController({ + config, + logger, + internalHooks, + repositories, + }), ); break; case 'ldap': @@ -229,7 +233,11 @@ export const setupTestServer = ({ registerController( app, config, - new MeController({ logger, externalHooks, internalHooks, repositories }), + new MeController({ + logger, + externalHooks, + internalHooks, + }), ); break; case 'passwordReset': @@ -242,8 +250,6 @@ export const setupTestServer = ({ externalHooks, internalHooks, mailer, - repositories, - jwtService, }), ); break; @@ -251,7 +257,12 @@ export const setupTestServer = ({ registerController( app, config, - new OwnerController({ config, logger, internalHooks, repositories }), + new OwnerController({ + config, + logger, + internalHooks, + repositories, + }), ); break; case 'users': @@ -266,8 +277,6 @@ export const setupTestServer = ({ repositories, activeWorkflowRunner: Container.get(ActiveWorkflowRunner), logger, - jwtService, - roleService: Container.get(RoleService), }), ); break; diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index c3d4341ea9..ab1443c0c7 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -10,7 +10,7 @@ import { User } from '@db/entities/User'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { NodeTypes } from '@/NodeTypes'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; import * as UserManagementHelper from '@/UserManagement/UserManagementHelper'; import { WorkflowsService } from '@/workflows/workflows.services'; @@ -234,6 +234,8 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { const sharedWorkflowNotOwner = new SharedWorkflow(); sharedWorkflowNotOwner.role = nonOwnerMockRole; + const userService = mockInstance(UserService); + test('sets default policy from environment when subworkflow has none', async () => { config.set('workflows.callerPolicyDefaultOption', 'none'); jest @@ -260,7 +262,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); - jest.spyOn(UserService, 'get').mockImplementation(async () => fakeUser); + jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser); jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => { return sharedWorkflowNotOwner; }); @@ -294,7 +296,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - jest.spyOn(UserService, 'get').mockImplementation(async () => fakeUser); + jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser); jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => { return sharedWorkflowNotOwner; }); @@ -320,7 +322,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); - jest.spyOn(UserService, 'get').mockImplementation(async () => fakeUser); + jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser); jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => { return sharedWorkflowOwner; }); @@ -343,7 +345,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - jest.spyOn(UserService, 'get').mockImplementation(async () => fakeUser); + jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser); jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => { return sharedWorkflowNotOwner; }); @@ -369,7 +371,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - jest.spyOn(UserService, 'get').mockImplementation(async () => fakeUser); + jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser); jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => { return sharedWorkflowNotOwner; }); diff --git a/packages/cli/test/unit/controllers/me.controller.test.ts b/packages/cli/test/unit/controllers/me.controller.test.ts index 36299ab863..950957c12c 100644 --- a/packages/cli/test/unit/controllers/me.controller.test.ts +++ b/packages/cli/test/unit/controllers/me.controller.test.ts @@ -4,23 +4,24 @@ import { mock, anyObject, captor } from 'jest-mock-extended'; import type { ILogger } from 'n8n-workflow'; import type { IExternalHooksClass, IInternalHooksClass } from '@/Interfaces'; import type { User } from '@db/entities/User'; -import type { UserRepository } from '@db/repositories'; import { MeController } from '@/controllers'; import { AUTH_COOKIE_NAME } from '@/constants'; import { BadRequestError } from '@/ResponseHelper'; import type { AuthenticatedRequest, MeRequest } from '@/requests'; import { badPasswords } from '../shared/testData'; +import { UserService } from '@/services/user.service'; +import Container from 'typedi'; describe('MeController', () => { const logger = mock(); const externalHooks = mock(); const internalHooks = mock(); - const userRepository = mock(); + const userService = mock(); + Container.set(UserService, userService); const controller = new MeController({ logger, externalHooks, internalHooks, - repositories: { User: userRepository }, }); describe('updateCurrentUser', () => { @@ -48,12 +49,12 @@ describe('MeController', () => { const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; const req = mock({ user, body: reqBody }); const res = mock(); - userRepository.findOneOrFail.mockResolvedValue(user); + userService.findOneOrFail.mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); await controller.updateCurrentUser(req, res); - expect(userRepository.update).toHaveBeenCalled(); + expect(userService.update).toHaveBeenCalled(); const cookieOptions = captor(); expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions); @@ -76,7 +77,7 @@ describe('MeController', () => { const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; const req = mock({ user, body: reqBody }); const res = mock(); - userRepository.findOneOrFail.mockResolvedValue(user); + userService.findOneOrFail.mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); // Add invalid data to the request payload @@ -84,9 +85,9 @@ describe('MeController', () => { await controller.updateCurrentUser(req, res); - expect(userRepository.update).toHaveBeenCalled(); + expect(userService.update).toHaveBeenCalled(); - const updatedUser = userRepository.update.mock.calls[0][1]; + const updatedUser = userService.update.mock.calls[0][1]; expect(updatedUser.email).toBe(reqBody.email); expect(updatedUser.firstName).toBe(reqBody.firstName); expect(updatedUser.lastName).toBe(reqBody.lastName); @@ -138,7 +139,7 @@ describe('MeController', () => { body: { currentPassword: 'old_password', newPassword: 'NewPassword123' }, }); const res = mock(); - userRepository.save.calledWith(req.user).mockResolvedValue(req.user); + userService.save.calledWith(req.user).mockResolvedValue(req.user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'new-signed-token'); await controller.updatePassword(req, res); @@ -171,7 +172,7 @@ describe('MeController', () => { describe('createAPIKey', () => { it('should create and save an API key', async () => { const { apiKey } = await controller.createAPIKey(req); - expect(userRepository.update).toHaveBeenCalledWith(req.user.id, { apiKey }); + expect(userService.update).toHaveBeenCalledWith(req.user.id, { apiKey }); }); }); @@ -185,7 +186,7 @@ describe('MeController', () => { describe('deleteAPIKey', () => { it('should delete the API key', async () => { await controller.deleteAPIKey(req); - expect(userRepository.update).toHaveBeenCalledWith(req.user.id, { apiKey: null }); + expect(userService.update).toHaveBeenCalledWith(req.user.id, { apiKey: null }); }); }); }); diff --git a/packages/cli/test/unit/controllers/owner.controller.test.ts b/packages/cli/test/unit/controllers/owner.controller.test.ts index 10879d0062..5c48c1a1d3 100644 --- a/packages/cli/test/unit/controllers/owner.controller.test.ts +++ b/packages/cli/test/unit/controllers/owner.controller.test.ts @@ -4,26 +4,29 @@ import type { ILogger } from 'n8n-workflow'; import jwt from 'jsonwebtoken'; import type { IInternalHooksClass } from '@/Interfaces'; import type { User } from '@db/entities/User'; -import type { SettingsRepository, UserRepository } from '@db/repositories'; +import type { SettingsRepository } from '@db/repositories'; import type { Config } from '@/config'; import { BadRequestError } from '@/ResponseHelper'; import type { OwnerRequest } from '@/requests'; import { OwnerController } from '@/controllers'; import { badPasswords } from '../shared/testData'; import { AUTH_COOKIE_NAME } from '@/constants'; +import { UserService } from '@/services/user.service'; +import Container from 'typedi'; +import { mockInstance } from '../../integration/shared/utils'; describe('OwnerController', () => { const config = mock(); const logger = mock(); const internalHooks = mock(); - const userRepository = mock(); + const userService = mockInstance(UserService); + Container.set(UserService, userService); const settingsRepository = mock(); const controller = new OwnerController({ config, logger, internalHooks, repositories: { - User: userRepository, Settings: settingsRepository, }, }); @@ -83,12 +86,12 @@ describe('OwnerController', () => { }); const res = mock(); config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); - userRepository.save.calledWith(anyObject()).mockResolvedValue(user); + userService.save.calledWith(anyObject()).mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); await controller.setupOwner(req, res); - expect(userRepository.save).toHaveBeenCalledWith(user); + expect(userService.save).toHaveBeenCalledWith(user); const cookieOptions = captor(); expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions); diff --git a/packages/cli/test/unit/services/events.service.test.ts b/packages/cli/test/unit/services/events.service.test.ts index cc64029557..4c8b8b1089 100644 --- a/packages/cli/test/unit/services/events.service.test.ts +++ b/packages/cli/test/unit/services/events.service.test.ts @@ -14,7 +14,7 @@ import type { User } from '@db/entities/User'; import type { WorkflowStatistics } from '@db/entities/WorkflowStatistics'; import { WorkflowStatisticsRepository } from '@db/repositories'; import { EventsService } from '@/services/events.service'; -import { UserService } from '@/user/user.service'; +import { UserService } from '@/services/user.service'; import { OwnershipService } from '@/services/ownership.service'; import { mockInstance } from '../../integration/shared/utils'; @@ -24,6 +24,7 @@ describe('EventsService', () => { const dbType = config.getEnv('database.type'); const fakeUser = mock({ id: 'abcde-fghij' }); const ownershipService = mockInstance(OwnershipService); + const userService = mockInstance(UserService); const entityManager = mock(); const dataSource = mock({ @@ -39,7 +40,7 @@ describe('EventsService', () => { config.set('diagnostics.enabled', true); config.set('deployment.type', 'n8n-testing'); mocked(ownershipService.getWorkflowOwnerCached).mockResolvedValue(fakeUser); - const updateUserSettingsMock = jest.spyOn(UserService, 'updateUserSettings').mockImplementation(); + const updateSettingsMock = jest.spyOn(userService, 'updateSettings').mockImplementation(); const eventsService = new EventsService( new WorkflowStatisticsRepository(dataSource), @@ -88,7 +89,7 @@ describe('EventsService', () => { mockDBCall(); await eventsService.workflowExecutionCompleted(workflow, runData); - expect(updateUserSettingsMock).toHaveBeenCalledTimes(1); + expect(updateSettingsMock).toHaveBeenCalledTimes(1); expect(onFirstProductionWorkflowSuccess).toBeCalledTimes(1); expect(onFirstProductionWorkflowSuccess).toHaveBeenNthCalledWith(1, { user_id: fakeUser.id, diff --git a/packages/cli/test/unit/services/ownership.service.test.ts b/packages/cli/test/unit/services/ownership.service.test.ts index fc1852c2cf..2a5b97fe0e 100644 --- a/packages/cli/test/unit/services/ownership.service.test.ts +++ b/packages/cli/test/unit/services/ownership.service.test.ts @@ -1,5 +1,5 @@ import { OwnershipService } from '@/services/ownership.service'; -import { SharedWorkflowRepository, UserRepository } from '@/databases/repositories'; +import { SharedWorkflowRepository } from '@/databases/repositories'; import { mockInstance } from '../../integration/shared/utils'; import { Role } from '@/databases/entities/Role'; import { randomInteger } from '../../integration/shared/random'; @@ -7,6 +7,7 @@ import { SharedWorkflow } from '@/databases/entities/SharedWorkflow'; import { CacheService } from '@/services/cache.service'; import { User } from '@/databases/entities/User'; import { RoleService } from '@/services/role.service'; +import { UserService } from '@/services/user.service'; const wfOwnerRole = () => Object.assign(new Role(), { @@ -18,12 +19,12 @@ const wfOwnerRole = () => describe('OwnershipService', () => { const cacheService = mockInstance(CacheService); const roleService = mockInstance(RoleService); - const userRepository = mockInstance(UserRepository); + const userService = mockInstance(UserService); const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository); const ownershipService = new OwnershipService( cacheService, - userRepository, + userService, roleService, sharedWorkflowRepository, );