From 561882f59972f80562dcce473eb71690688540fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 17 Feb 2023 10:59:09 +0100 Subject: [PATCH] refactor(core): Improve instance owner setup and add unit tests (no-changelog) (#5499) * refactor(core): Avoid fetching all workflows and credentials for the owner setup screen * refactor(core): Add unit tests for the owner controller --- .../cli/src/controllers/owner.controller.ts | 48 +++++-- packages/cli/src/requests.ts | 2 +- .../cli/test/integration/owner.api.test.ts | 12 +- .../cli/test/integration/shared/constants.ts | 5 +- .../unit/controllers/me.controller.test.ts | 22 +-- .../unit/controllers/owner.controller.test.ts | 134 ++++++++++++++++++ packages/cli/test/unit/shared/testData.ts | 6 + packages/editor-ui/src/api/users.ts | 8 +- packages/editor-ui/src/stores/users.ts | 4 + packages/editor-ui/src/views/SetupView.vue | 14 +- 10 files changed, 204 insertions(+), 51 deletions(-) create mode 100644 packages/cli/test/unit/controllers/owner.controller.test.ts create mode 100644 packages/cli/test/unit/shared/testData.ts diff --git a/packages/cli/src/controllers/owner.controller.ts b/packages/cli/src/controllers/owner.controller.ts index 31779f92e0..8040c5aaac 100644 --- a/packages/cli/src/controllers/owner.controller.ts +++ b/packages/cli/src/controllers/owner.controller.ts @@ -1,6 +1,6 @@ import validator from 'validator'; import { validateEntity } from '@/GenericHelpers'; -import { Post, RestController } from '@/decorators'; +import { Get, Post, RestController } from '@/decorators'; import { BadRequestError } from '@/ResponseHelper'; import { hashPassword, @@ -13,9 +13,10 @@ import type { Repository } from 'typeorm'; import type { ILogger } from 'n8n-workflow'; import type { Config } from '@/config'; import { OwnerRequest } from '@/requests'; -import type { IDatabaseCollections, IInternalHooksClass } from '@/Interfaces'; +import type { IDatabaseCollections, IInternalHooksClass, ICredentialsDb } from '@/Interfaces'; import type { Settings } from '@db/entities/Settings'; import type { User } from '@db/entities/User'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; @RestController('/owner') export class OwnerController { @@ -29,6 +30,10 @@ export class OwnerController { private readonly settingsRepository: Repository; + private readonly credentialsRepository: Repository; + + private readonly workflowsRepository: Repository; + constructor({ config, logger, @@ -38,23 +43,38 @@ export class OwnerController { config: Config; logger: ILogger; internalHooks: IInternalHooksClass; - repositories: Pick; + repositories: Pick; }) { this.config = config; this.logger = logger; this.internalHooks = internalHooks; this.userRepository = repositories.User; this.settingsRepository = repositories.Settings; + this.credentialsRepository = repositories.Credentials; + this.workflowsRepository = repositories.Workflow; + } + + @Get('/pre-setup') + async preSetup(): Promise<{ credentials: number; workflows: number }> { + if (this.config.getEnv('userManagement.isInstanceOwnerSetUp')) { + throw new BadRequestError('Instance owner already setup'); + } + + const [credentials, workflows] = await Promise.all([ + this.credentialsRepository.countBy({}), + this.workflowsRepository.countBy({}), + ]); + return { credentials, workflows }; } /** * Promote a shell into the owner of the n8n instance, * and enable `isInstanceOwnerSetUp` setting. */ - @Post('/') - async promoteOwner(req: OwnerRequest.Post, res: Response) { + @Post('/setup') + async setupOwner(req: OwnerRequest.Post, res: Response) { const { email, firstName, lastName, password } = req.body; - const { id: userId } = req.user; + const { id: userId, globalRole } = req.user; if (this.config.getEnv('userManagement.isInstanceOwnerSetUp')) { this.logger.debug( @@ -63,7 +83,7 @@ export class OwnerController { userId, }, ); - throw new BadRequestError('Invalid request'); + throw new BadRequestError('Instance owner already setup'); } if (!email || !validator.isEmail(email)) { @@ -84,12 +104,8 @@ export class OwnerController { throw new BadRequestError('First and last names are mandatory'); } - let owner = await this.userRepository.findOne({ - relations: ['globalRole'], - where: { id: userId }, - }); - - if (!owner || (owner.globalRole.scope === 'global' && owner.globalRole.name !== 'owner')) { + // TODO: This check should be in a middleware outside this class + if (globalRole.scope === 'global' && globalRole.name !== 'owner') { this.logger.debug( 'Request to claim instance ownership failed because user shell does not exist or has wrong role!', { @@ -99,6 +115,8 @@ export class OwnerController { throw new BadRequestError('Invalid request'); } + let owner = req.user; + Object.assign(owner, { email, firstName, @@ -110,7 +128,7 @@ export class OwnerController { owner = await this.userRepository.save(owner); - this.logger.info('Owner was set up successfully', { userId: req.user.id }); + this.logger.info('Owner was set up successfully', { userId }); await this.settingsRepository.update( { key: 'userManagement.isInstanceOwnerSetUp' }, @@ -119,7 +137,7 @@ export class OwnerController { this.config.set('userManagement.isInstanceOwnerSetUp', true); - this.logger.debug('Setting isInstanceOwnerSetUp updated successfully', { userId: req.user.id }); + this.logger.debug('Setting isInstanceOwnerSetUp updated successfully', { userId }); await issueCookie(res, owner); diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index e0636d7611..0a1be41429 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -27,7 +27,7 @@ export type AuthenticatedRequest< ResponseBody = {}, RequestBody = {}, RequestQuery = {}, -> = express.Request & { +> = Omit, 'user'> & { user: User; mailer?: UserManagementMailer.UserManagementMailer; globalMemberRole?: Role; diff --git a/packages/cli/test/integration/owner.api.test.ts b/packages/cli/test/integration/owner.api.test.ts index 546a6c6556..0f0e79d5a6 100644 --- a/packages/cli/test/integration/owner.api.test.ts +++ b/packages/cli/test/integration/owner.api.test.ts @@ -38,7 +38,7 @@ afterAll(async () => { await testDb.terminate(); }); -test('POST /owner should create owner and enable isInstanceOwnerSetUp', async () => { +test('POST /owner/setup should create owner and enable isInstanceOwnerSetUp', async () => { const ownerShell = await testDb.createUserShell(globalOwnerRole); const newOwnerData = { @@ -48,7 +48,7 @@ test('POST /owner should create owner and enable isInstanceOwnerSetUp', async () password: randomValidPassword(), }; - const response = await authAgent(ownerShell).post('/owner').send(newOwnerData); + const response = await authAgent(ownerShell).post('/owner/setup').send(newOwnerData); expect(response.statusCode).toBe(200); @@ -90,7 +90,7 @@ test('POST /owner should create owner and enable isInstanceOwnerSetUp', async () expect(isInstanceOwnerSetUpSetting).toBe(true); }); -test('POST /owner should create owner with lowercased email', async () => { +test('POST /owner/setup should create owner with lowercased email', async () => { const ownerShell = await testDb.createUserShell(globalOwnerRole); const newOwnerData = { @@ -100,7 +100,7 @@ test('POST /owner should create owner with lowercased email', async () => { password: randomValidPassword(), }; - const response = await authAgent(ownerShell).post('/owner').send(newOwnerData); + const response = await authAgent(ownerShell).post('/owner/setup').send(newOwnerData); expect(response.statusCode).toBe(200); @@ -113,13 +113,13 @@ test('POST /owner should create owner with lowercased email', async () => { expect(storedOwner.email).toBe(newOwnerData.email.toLowerCase()); }); -test('POST /owner should fail with invalid inputs', async () => { +test('POST /owner/setup should fail with invalid inputs', async () => { const ownerShell = await testDb.createUserShell(globalOwnerRole); const authOwnerAgent = authAgent(ownerShell); await Promise.all( INVALID_POST_OWNER_PAYLOADS.map(async (invalidPayload) => { - const response = await authOwnerAgent.post('/owner').send(invalidPayload); + const response = await authOwnerAgent.post('/owner/setup').send(invalidPayload); expect(response.statusCode).toBe(400); }), ); diff --git a/packages/cli/test/integration/shared/constants.ts b/packages/cli/test/integration/shared/constants.ts index c04c1e7c83..3608225eaa 100644 --- a/packages/cli/test/integration/shared/constants.ts +++ b/packages/cli/test/integration/shared/constants.ts @@ -31,7 +31,7 @@ export const ROUTES_REQUIRING_AUTHENTICATION: Readonly = [ 'PATCH /me', 'PATCH /me/password', 'POST /me/survey', - 'POST /owner', + 'POST /owner/setup', 'GET /non-existent', ]; @@ -42,7 +42,8 @@ export const ROUTES_REQUIRING_AUTHORIZATION: Readonly = [ 'POST /users', 'DELETE /users/123', 'POST /users/123/reinvite', - 'POST /owner', + 'POST /owner/pre-setup', + 'POST /owner/setup', 'POST /owner/skip-setup', ]; diff --git a/packages/cli/test/unit/controllers/me.controller.test.ts b/packages/cli/test/unit/controllers/me.controller.test.ts index 1606915398..f926c4fc4f 100644 --- a/packages/cli/test/unit/controllers/me.controller.test.ts +++ b/packages/cli/test/unit/controllers/me.controller.test.ts @@ -9,21 +9,18 @@ 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'; describe('MeController', () => { const logger = mock(); const externalHooks = mock(); const internalHooks = mock(); const userRepository = mock>(); - - let controller: MeController; - beforeAll(() => { - controller = new MeController({ - logger, - externalHooks, - internalHooks, - repositories: { User: userRepository }, - }); + const controller = new MeController({ + logger, + externalHooks, + internalHooks, + repositories: { User: userRepository }, }); describe('updateCurrentUser', () => { @@ -88,12 +85,7 @@ describe('MeController', () => { }); describe('should throw if newPassword is not valid', () => { - Object.entries({ - pass: 'Password must be 8 to 64 characters long. Password must contain at least 1 number. Password must contain at least 1 uppercase letter.', - password: - 'Password must contain at least 1 number. Password must contain at least 1 uppercase letter.', - password1: 'Password must contain at least 1 uppercase letter.', - }).forEach(([newPassword, errorMessage]) => { + Object.entries(badPasswords).forEach(([newPassword, errorMessage]) => { it(newPassword, async () => { const req = mock({ user: mock({ password: passwordHash }), diff --git a/packages/cli/test/unit/controllers/owner.controller.test.ts b/packages/cli/test/unit/controllers/owner.controller.test.ts new file mode 100644 index 0000000000..a460dc8bc5 --- /dev/null +++ b/packages/cli/test/unit/controllers/owner.controller.test.ts @@ -0,0 +1,134 @@ +import type { Repository } from 'typeorm'; +import type { CookieOptions, Response } from 'express'; +import { anyObject, captor, mock } from 'jest-mock-extended'; +import type { ILogger } from 'n8n-workflow'; +import jwt from 'jsonwebtoken'; +import type { ICredentialsDb, IInternalHooksClass } from '@/Interfaces'; +import type { User } from '@db/entities/User'; +import type { Settings } from '@db/entities/Settings'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; +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'; + +describe('OwnerController', () => { + const config = mock(); + const logger = mock(); + const internalHooks = mock(); + const userRepository = mock>(); + const settingsRepository = mock>(); + const credentialsRepository = mock>(); + const workflowsRepository = mock>(); + const controller = new OwnerController({ + config, + logger, + internalHooks, + repositories: { + User: userRepository, + Settings: settingsRepository, + Credentials: credentialsRepository, + Workflow: workflowsRepository, + }, + }); + + describe('preSetup', () => { + it('should throw a BadRequestError if the instance owner is already setup', async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(true); + expect(controller.preSetup()).rejects.toThrowError( + new BadRequestError('Instance owner already setup'), + ); + }); + + it('should a return credential and workflow count', async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); + credentialsRepository.countBy.mockResolvedValue(7); + workflowsRepository.countBy.mockResolvedValue(31); + const { credentials, workflows } = await controller.preSetup(); + expect(credentials).toBe(7); + expect(workflows).toBe(31); + }); + }); + + describe('setupOwner', () => { + it('should throw a BadRequestError if the instance owner is already setup', async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(true); + expect(controller.setupOwner(mock(), mock())).rejects.toThrowError( + new BadRequestError('Instance owner already setup'), + ); + }); + + it('should throw a BadRequestError if the email is invalid', async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); + const req = mock({ body: { email: 'invalid email' } }); + expect(controller.setupOwner(req, mock())).rejects.toThrowError( + new BadRequestError('Invalid email address'), + ); + }); + + describe('should throw if the password is invalid', () => { + Object.entries(badPasswords).forEach(([password, errorMessage]) => { + it(password, async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); + const req = mock({ body: { email: 'valid@email.com', password } }); + expect(controller.setupOwner(req, mock())).rejects.toThrowError( + new BadRequestError(errorMessage), + ); + }); + }); + }); + + it('should throw a BadRequestError if firstName & lastName are missing ', async () => { + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); + const req = mock({ + body: { email: 'valid@email.com', password: 'NewPassword123', firstName: '', lastName: '' }, + }); + expect(controller.setupOwner(req, mock())).rejects.toThrowError( + new BadRequestError('First and last names are mandatory'), + ); + }); + + it('should setup the instance owner successfully', async () => { + const user = mock({ + id: 'userId', + globalRole: { scope: 'global', name: 'owner' }, + authIdentities: [], + }); + const req = mock({ + body: { + email: 'valid@email.com', + password: 'NewPassword123', + firstName: 'Jane', + lastName: 'Doe', + }, + user, + }); + const res = mock(); + config.getEnv.calledWith('userManagement.isInstanceOwnerSetUp').mockReturnValue(false); + userRepository.save.calledWith(anyObject()).mockResolvedValue(user); + jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); + + await controller.setupOwner(req, res); + + expect(userRepository.save).toHaveBeenCalledWith(user); + + const cookieOptions = captor(); + expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions); + expect(cookieOptions.value.httpOnly).toBe(true); + expect(cookieOptions.value.sameSite).toBe('lax'); + }); + }); + + describe('skipSetup', () => { + it('should skip setting up the instance owner', async () => { + await controller.skipSetup(); + expect(settingsRepository.update).toHaveBeenCalledWith( + { key: 'userManagement.skipInstanceOwnerSetup' }, + { value: JSON.stringify(true) }, + ); + expect(config.set).toHaveBeenCalledWith('userManagement.skipInstanceOwnerSetup', true); + }); + }); +}); diff --git a/packages/cli/test/unit/shared/testData.ts b/packages/cli/test/unit/shared/testData.ts new file mode 100644 index 0000000000..4bba6f3827 --- /dev/null +++ b/packages/cli/test/unit/shared/testData.ts @@ -0,0 +1,6 @@ +export const badPasswords = { + pass: 'Password must be 8 to 64 characters long. Password must contain at least 1 number. Password must contain at least 1 uppercase letter.', + password: + 'Password must contain at least 1 number. Password must contain at least 1 uppercase letter.', + password1: 'Password must contain at least 1 uppercase letter.', +}; diff --git a/packages/editor-ui/src/api/users.ts b/packages/editor-ui/src/api/users.ts index d7cabf2cbf..bc14132d20 100644 --- a/packages/editor-ui/src/api/users.ts +++ b/packages/editor-ui/src/api/users.ts @@ -22,11 +22,17 @@ export async function logout(context: IRestApiContext): Promise { await makeRestApiRequest(context, 'POST', '/logout'); } +export function preOwnerSetup( + context: IRestApiContext, +): Promise<{ credentials: number; workflows: number }> { + return makeRestApiRequest(context, 'GET', '/owner/pre-setup'); +} + export function setupOwner( context: IRestApiContext, params: { firstName: string; lastName: string; email: string; password: string }, ): Promise { - return makeRestApiRequest(context, 'POST', '/owner', params as unknown as IDataObject); + return makeRestApiRequest(context, 'POST', '/owner/setup', params as unknown as IDataObject); } export function skipOwnerSetup(context: IRestApiContext): Promise { diff --git a/packages/editor-ui/src/stores/users.ts b/packages/editor-ui/src/stores/users.ts index d7b3e79cc6..9c7cf2b50c 100644 --- a/packages/editor-ui/src/stores/users.ts +++ b/packages/editor-ui/src/stores/users.ts @@ -7,6 +7,7 @@ import { login, loginCurrentUser, logout, + preOwnerSetup, reinvite, sendForgotPasswordEmail, setupOwner, @@ -158,6 +159,9 @@ export const useUsersStore = defineStore(STORES.USERS, { await logout(rootStore.getRestApiContext); this.currentUserId = null; }, + async preOwnerSetup() { + return preOwnerSetup(useRootStore().getRestApiContext); + }, async createOwner(params: { firstName: string; lastName: string; diff --git a/packages/editor-ui/src/views/SetupView.vue b/packages/editor-ui/src/views/SetupView.vue index 2e33cd8a48..5d24237a67 100644 --- a/packages/editor-ui/src/views/SetupView.vue +++ b/packages/editor-ui/src/views/SetupView.vue @@ -28,9 +28,9 @@ export default mixins(showMessage, restApi).extend({ AuthView, }, async mounted() { - const getAllCredentialsPromise = this.getAllCredentials(); - const getAllWorkflowsPromise = this.getAllWorkflows(); - await Promise.all([getAllCredentialsPromise, getAllWorkflowsPromise]); + const { credentials, workflows } = await this.usersStore.preOwnerSetup(); + this.credentialsCount = credentials; + this.workflowsCount = workflows; }, data() { const FORM_CONFIG: IFormBoxConfig = { @@ -102,14 +102,6 @@ export default mixins(showMessage, restApi).extend({ ...mapStores(useCredentialsStore, useSettingsStore, useUIStore, useUsersStore), }, methods: { - async getAllCredentials() { - const credentials = await this.credentialsStore.fetchAllCredentials(); - this.credentialsCount = credentials.length; - }, - async getAllWorkflows() { - const workflows = await this.restApi().getWorkflows(); - this.workflowsCount = workflows.length; - }, async confirmSetupOrGoBack(): Promise { if (this.workflowsCount === 0 && this.credentialsCount === 0) { return true;