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
This commit is contained in:
कारतोफ्फेलस्क्रिप्ट™ 2023-02-17 10:59:09 +01:00 committed by GitHub
parent 12104bc4a3
commit 561882f599
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 204 additions and 51 deletions

View file

@ -1,6 +1,6 @@
import validator from 'validator'; import validator from 'validator';
import { validateEntity } from '@/GenericHelpers'; import { validateEntity } from '@/GenericHelpers';
import { Post, RestController } from '@/decorators'; import { Get, Post, RestController } from '@/decorators';
import { BadRequestError } from '@/ResponseHelper'; import { BadRequestError } from '@/ResponseHelper';
import { import {
hashPassword, hashPassword,
@ -13,9 +13,10 @@ import type { Repository } from 'typeorm';
import type { ILogger } from 'n8n-workflow'; import type { ILogger } from 'n8n-workflow';
import type { Config } from '@/config'; import type { Config } from '@/config';
import { OwnerRequest } from '@/requests'; 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 { Settings } from '@db/entities/Settings';
import type { User } from '@db/entities/User'; import type { User } from '@db/entities/User';
import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
@RestController('/owner') @RestController('/owner')
export class OwnerController { export class OwnerController {
@ -29,6 +30,10 @@ export class OwnerController {
private readonly settingsRepository: Repository<Settings>; private readonly settingsRepository: Repository<Settings>;
private readonly credentialsRepository: Repository<ICredentialsDb>;
private readonly workflowsRepository: Repository<WorkflowEntity>;
constructor({ constructor({
config, config,
logger, logger,
@ -38,23 +43,38 @@ export class OwnerController {
config: Config; config: Config;
logger: ILogger; logger: ILogger;
internalHooks: IInternalHooksClass; internalHooks: IInternalHooksClass;
repositories: Pick<IDatabaseCollections, 'User' | 'Settings'>; repositories: Pick<IDatabaseCollections, 'User' | 'Settings' | 'Credentials' | 'Workflow'>;
}) { }) {
this.config = config; this.config = config;
this.logger = logger; this.logger = logger;
this.internalHooks = internalHooks; this.internalHooks = internalHooks;
this.userRepository = repositories.User; this.userRepository = repositories.User;
this.settingsRepository = repositories.Settings; 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, * Promote a shell into the owner of the n8n instance,
* and enable `isInstanceOwnerSetUp` setting. * and enable `isInstanceOwnerSetUp` setting.
*/ */
@Post('/') @Post('/setup')
async promoteOwner(req: OwnerRequest.Post, res: Response) { async setupOwner(req: OwnerRequest.Post, res: Response) {
const { email, firstName, lastName, password } = req.body; const { email, firstName, lastName, password } = req.body;
const { id: userId } = req.user; const { id: userId, globalRole } = req.user;
if (this.config.getEnv('userManagement.isInstanceOwnerSetUp')) { if (this.config.getEnv('userManagement.isInstanceOwnerSetUp')) {
this.logger.debug( this.logger.debug(
@ -63,7 +83,7 @@ export class OwnerController {
userId, userId,
}, },
); );
throw new BadRequestError('Invalid request'); throw new BadRequestError('Instance owner already setup');
} }
if (!email || !validator.isEmail(email)) { if (!email || !validator.isEmail(email)) {
@ -84,12 +104,8 @@ export class OwnerController {
throw new BadRequestError('First and last names are mandatory'); throw new BadRequestError('First and last names are mandatory');
} }
let owner = await this.userRepository.findOne({ // TODO: This check should be in a middleware outside this class
relations: ['globalRole'], if (globalRole.scope === 'global' && globalRole.name !== 'owner') {
where: { id: userId },
});
if (!owner || (owner.globalRole.scope === 'global' && owner.globalRole.name !== 'owner')) {
this.logger.debug( this.logger.debug(
'Request to claim instance ownership failed because user shell does not exist or has wrong role!', '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'); throw new BadRequestError('Invalid request');
} }
let owner = req.user;
Object.assign(owner, { Object.assign(owner, {
email, email,
firstName, firstName,
@ -110,7 +128,7 @@ export class OwnerController {
owner = await this.userRepository.save(owner); 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( await this.settingsRepository.update(
{ key: 'userManagement.isInstanceOwnerSetUp' }, { key: 'userManagement.isInstanceOwnerSetUp' },
@ -119,7 +137,7 @@ export class OwnerController {
this.config.set('userManagement.isInstanceOwnerSetUp', true); 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); await issueCookie(res, owner);

View file

@ -27,7 +27,7 @@ export type AuthenticatedRequest<
ResponseBody = {}, ResponseBody = {},
RequestBody = {}, RequestBody = {},
RequestQuery = {}, RequestQuery = {},
> = express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery> & { > = Omit<express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery>, 'user'> & {
user: User; user: User;
mailer?: UserManagementMailer.UserManagementMailer; mailer?: UserManagementMailer.UserManagementMailer;
globalMemberRole?: Role; globalMemberRole?: Role;

View file

@ -38,7 +38,7 @@ afterAll(async () => {
await testDb.terminate(); 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 ownerShell = await testDb.createUserShell(globalOwnerRole);
const newOwnerData = { const newOwnerData = {
@ -48,7 +48,7 @@ test('POST /owner should create owner and enable isInstanceOwnerSetUp', async ()
password: randomValidPassword(), 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); expect(response.statusCode).toBe(200);
@ -90,7 +90,7 @@ test('POST /owner should create owner and enable isInstanceOwnerSetUp', async ()
expect(isInstanceOwnerSetUpSetting).toBe(true); 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 ownerShell = await testDb.createUserShell(globalOwnerRole);
const newOwnerData = { const newOwnerData = {
@ -100,7 +100,7 @@ test('POST /owner should create owner with lowercased email', async () => {
password: randomValidPassword(), 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); 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()); 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 ownerShell = await testDb.createUserShell(globalOwnerRole);
const authOwnerAgent = authAgent(ownerShell); const authOwnerAgent = authAgent(ownerShell);
await Promise.all( await Promise.all(
INVALID_POST_OWNER_PAYLOADS.map(async (invalidPayload) => { 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); expect(response.statusCode).toBe(400);
}), }),
); );

View file

@ -31,7 +31,7 @@ export const ROUTES_REQUIRING_AUTHENTICATION: Readonly<string[]> = [
'PATCH /me', 'PATCH /me',
'PATCH /me/password', 'PATCH /me/password',
'POST /me/survey', 'POST /me/survey',
'POST /owner', 'POST /owner/setup',
'GET /non-existent', 'GET /non-existent',
]; ];
@ -42,7 +42,8 @@ export const ROUTES_REQUIRING_AUTHORIZATION: Readonly<string[]> = [
'POST /users', 'POST /users',
'DELETE /users/123', 'DELETE /users/123',
'POST /users/123/reinvite', 'POST /users/123/reinvite',
'POST /owner', 'POST /owner/pre-setup',
'POST /owner/setup',
'POST /owner/skip-setup', 'POST /owner/skip-setup',
]; ];

View file

@ -9,21 +9,18 @@ import { MeController } from '@/controllers';
import { AUTH_COOKIE_NAME } from '@/constants'; import { AUTH_COOKIE_NAME } from '@/constants';
import { BadRequestError } from '@/ResponseHelper'; import { BadRequestError } from '@/ResponseHelper';
import type { AuthenticatedRequest, MeRequest } from '@/requests'; import type { AuthenticatedRequest, MeRequest } from '@/requests';
import { badPasswords } from '../shared/testData';
describe('MeController', () => { describe('MeController', () => {
const logger = mock<ILogger>(); const logger = mock<ILogger>();
const externalHooks = mock<IExternalHooksClass>(); const externalHooks = mock<IExternalHooksClass>();
const internalHooks = mock<IInternalHooksClass>(); const internalHooks = mock<IInternalHooksClass>();
const userRepository = mock<Repository<User>>(); const userRepository = mock<Repository<User>>();
const controller = new MeController({
let controller: MeController; logger,
beforeAll(() => { externalHooks,
controller = new MeController({ internalHooks,
logger, repositories: { User: userRepository },
externalHooks,
internalHooks,
repositories: { User: userRepository },
});
}); });
describe('updateCurrentUser', () => { describe('updateCurrentUser', () => {
@ -88,12 +85,7 @@ describe('MeController', () => {
}); });
describe('should throw if newPassword is not valid', () => { describe('should throw if newPassword is not valid', () => {
Object.entries({ Object.entries(badPasswords).forEach(([newPassword, errorMessage]) => {
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]) => {
it(newPassword, async () => { it(newPassword, async () => {
const req = mock<MeRequest.Password>({ const req = mock<MeRequest.Password>({
user: mock({ password: passwordHash }), user: mock({ password: passwordHash }),

View file

@ -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<Config>();
const logger = mock<ILogger>();
const internalHooks = mock<IInternalHooksClass>();
const userRepository = mock<Repository<User>>();
const settingsRepository = mock<Repository<Settings>>();
const credentialsRepository = mock<Repository<ICredentialsDb>>();
const workflowsRepository = mock<Repository<WorkflowEntity>>();
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<OwnerRequest.Post>({ 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<OwnerRequest.Post>({ 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<OwnerRequest.Post>({
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<User>({
id: 'userId',
globalRole: { scope: 'global', name: 'owner' },
authIdentities: [],
});
const req = mock<OwnerRequest.Post>({
body: {
email: 'valid@email.com',
password: 'NewPassword123',
firstName: 'Jane',
lastName: 'Doe',
},
user,
});
const res = mock<Response>();
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<CookieOptions>();
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);
});
});
});

View file

@ -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.',
};

View file

@ -22,11 +22,17 @@ export async function logout(context: IRestApiContext): Promise<void> {
await makeRestApiRequest(context, 'POST', '/logout'); 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( export function setupOwner(
context: IRestApiContext, context: IRestApiContext,
params: { firstName: string; lastName: string; email: string; password: string }, params: { firstName: string; lastName: string; email: string; password: string },
): Promise<IUserResponse> { ): Promise<IUserResponse> {
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<void> { export function skipOwnerSetup(context: IRestApiContext): Promise<void> {

View file

@ -7,6 +7,7 @@ import {
login, login,
loginCurrentUser, loginCurrentUser,
logout, logout,
preOwnerSetup,
reinvite, reinvite,
sendForgotPasswordEmail, sendForgotPasswordEmail,
setupOwner, setupOwner,
@ -158,6 +159,9 @@ export const useUsersStore = defineStore(STORES.USERS, {
await logout(rootStore.getRestApiContext); await logout(rootStore.getRestApiContext);
this.currentUserId = null; this.currentUserId = null;
}, },
async preOwnerSetup() {
return preOwnerSetup(useRootStore().getRestApiContext);
},
async createOwner(params: { async createOwner(params: {
firstName: string; firstName: string;
lastName: string; lastName: string;

View file

@ -28,9 +28,9 @@ export default mixins(showMessage, restApi).extend({
AuthView, AuthView,
}, },
async mounted() { async mounted() {
const getAllCredentialsPromise = this.getAllCredentials(); const { credentials, workflows } = await this.usersStore.preOwnerSetup();
const getAllWorkflowsPromise = this.getAllWorkflows(); this.credentialsCount = credentials;
await Promise.all([getAllCredentialsPromise, getAllWorkflowsPromise]); this.workflowsCount = workflows;
}, },
data() { data() {
const FORM_CONFIG: IFormBoxConfig = { const FORM_CONFIG: IFormBoxConfig = {
@ -102,14 +102,6 @@ export default mixins(showMessage, restApi).extend({
...mapStores(useCredentialsStore, useSettingsStore, useUIStore, useUsersStore), ...mapStores(useCredentialsStore, useSettingsStore, useUIStore, useUsersStore),
}, },
methods: { 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<boolean> { async confirmSetupOrGoBack(): Promise<boolean> {
if (this.workflowsCount === 0 && this.credentialsCount === 0) { if (this.workflowsCount === 0 && this.credentialsCount === 0) {
return true; return true;