Skip to content

Commit

Permalink
perf(core): Cache roles (#6803)
Browse files Browse the repository at this point in the history
* refactor: Create `RoleService`

* refactor: Refactor to use service

* refactor: Move `getUserRoleForWorkflow`

* refactor: Clear out old `RoleService`

* refactor: Consolidate utils into service

* refactor: Remove unused methods

* test: Add tests

* refactor: Remove redundant return types

* refactor: Missing utility

* chore: Remove commented out bit

* refactor: Make `Db.collections.Repository` inaccessible

* chore: Cleanup

* feat: Prepopulate cache

* chore: Remove logging

* fix: Account for tests where roles are undefined

* fix: Restore `prettier.prettierPath`

* test: Account for cache enabled and disabled

* fix: Restore `Role` in `Db.collections`

* refactor: Simplify by removing `orFail`

* refactor: Rename for clarity

* refactor: Use `cacheKey` for readability

* refactor: Validate role before creation

* refacator: Remove redundant `cache` prefix

* ci: Lint fix

* test: Fix e2e
  • Loading branch information
ivov authored Aug 3, 2023
1 parent f93270a commit e4f0418
Show file tree
Hide file tree
Showing 33 changed files with 280 additions and 214 deletions.
14 changes: 9 additions & 5 deletions packages/cli/src/Db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,23 +169,27 @@ export async function init(testConnectionOptions?: ConnectionOptions): Promise<v

collections.AuthIdentity = Container.get(AuthIdentityRepository);
collections.AuthProviderSyncHistory = Container.get(AuthProviderSyncHistoryRepository);
collections.Credentials = Container.get(CredentialsRepository);
collections.EventDestinations = Container.get(EventDestinationsRepository);
collections.Execution = Container.get(ExecutionRepository);
collections.ExecutionData = Container.get(ExecutionDataRepository);
collections.ExecutionMetadata = Container.get(ExecutionMetadataRepository);
collections.InstalledNodes = Container.get(InstalledNodesRepository);
collections.InstalledPackages = Container.get(InstalledPackagesRepository);
collections.Role = Container.get(RoleRepository);
collections.Settings = Container.get(SettingsRepository);
collections.SharedCredentials = Container.get(SharedCredentialsRepository);
collections.SharedWorkflow = Container.get(SharedWorkflowRepository);
collections.Tag = Container.get(TagRepository);
collections.User = Container.get(UserRepository);
collections.Variables = Container.get(VariablesRepository);
collections.Workflow = Container.get(WorkflowRepository);
collections.WorkflowStatistics = Container.get(WorkflowStatisticsRepository);
collections.WorkflowTagMapping = Container.get(WorkflowTagMappingRepository);

/**
* @important Do not remove these collections until cloud hooks are backwards compatible.
*/
collections.Role = Container.get(RoleRepository);
collections.User = Container.get(UserRepository);
collections.Settings = Container.get(SettingsRepository);
collections.Credentials = Container.get(CredentialsRepository);
collections.Workflow = Container.get(WorkflowRepository);
}

export async function migrate() {
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/InternalHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ import type {
} from '@/Interfaces';
import { Telemetry } from '@/telemetry';
import type { AuthProviderType } from '@db/entities/AuthIdentity';
import { RoleService } from './role/role.service';
import { eventBus } from './eventbus';
import { EventsService } from '@/services/events.service';
import type { User } from '@db/entities/User';
import { N8N_VERSION } from '@/constants';
import { NodeTypes } from './NodeTypes';
import type { ExecutionMetadata } from '@db/entities/ExecutionMetadata';
import { ExecutionRepository } from '@db/repositories';
import { RoleService } from './services/role.service';

function userToPayload(user: User): {
userId: string;
Expand Down Expand Up @@ -175,7 +175,7 @@ export class InternalHooks implements IInternalHooksClass {

let userRole: 'owner' | 'sharee' | undefined = undefined;
if (user.id && workflow.id) {
const role = await this.roleService.getUserRoleForWorkflow(user.id, workflow.id);
const role = await this.roleService.findRoleByUserAndWorkflow(user.id, workflow.id);
if (role) {
userRole = role.name === 'owner' ? 'owner' : 'sharee';
}
Expand Down Expand Up @@ -381,7 +381,7 @@ export class InternalHooks implements IInternalHooksClass {

let userRole: 'owner' | 'sharee' | undefined = undefined;
if (userId) {
const role = await this.roleService.getUserRoleForWorkflow(userId, workflow.id);
const role = await this.roleService.findRoleByUserAndWorkflow(userId, workflow.id);
if (role) {
userRole = role.name === 'owner' ? 'owner' : 'sharee';
}
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/Ldap/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import config from '@/config';
import type { Role } from '@db/entities/Role';
import { User } from '@db/entities/User';
import { AuthIdentity } from '@db/entities/AuthIdentity';
import { RoleRepository } from '@db/repositories';
import type { AuthProviderSyncHistory } from '@db/entities/AuthProviderSyncHistory';
import { LdapManager } from './LdapManager.ee';

Expand All @@ -32,6 +31,7 @@ import {
setCurrentAuthenticationMethod,
} from '@/sso/ssoHelpers';
import { InternalServerError } from '../ResponseHelper';
import { RoleService } from '@/services/role.service';

/**
* Check whether the LDAP feature is disabled in the instance
Expand Down Expand Up @@ -92,7 +92,7 @@ export const randomPassword = (): string => {
* Return the user role to be assigned to LDAP users
*/
export const getLdapUserRole = async (): Promise<Role> => {
return Container.get(RoleRepository).findGlobalMemberRoleOrFail();
return Container.get(RoleService).findGlobalMemberRole();
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ import type { ICredentialsDb } from '@/Interfaces';
import { CredentialsEntity } from '@db/entities/CredentialsEntity';
import { SharedCredentials } from '@db/entities/SharedCredentials';
import type { User } from '@db/entities/User';
import { RoleRepository } from '@db/repositories';
import { ExternalHooks } from '@/ExternalHooks';
import type { IDependency, IJsonSchema } from '../../../types';
import type { CredentialRequest } from '@/requests';
import { Container } from 'typedi';
import { RoleService } from '@/services/role.service';

export async function getCredentials(credentialId: string): Promise<ICredentialsDb | null> {
return Db.collections.Credentials.findOneBy({ id: credentialId });
Expand Down Expand Up @@ -58,7 +58,7 @@ export async function saveCredential(
user: User,
encryptedData: ICredentialsDb,
): Promise<CredentialsEntity> {
const role = await Container.get(RoleRepository).findCredentialOwnerRoleOrFail();
const role = await Container.get(RoleService).findCredentialOwnerRole();

await Container.get(ExternalHooks).run('credentials.create', [encryptedData]);

Expand Down
7 changes: 0 additions & 7 deletions packages/cli/src/PublicApi/v1/handlers/users/users.service.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { addNodeIds, replaceInvalidCredentials } from '@/WorkflowHelpers';
import type { WorkflowRequest } from '../../../types';
import { authorize, validCursor } from '../../shared/middlewares/global.middleware';
import { encodeNextCursor } from '../../shared/services/pagination.service';
import { getWorkflowOwnerRole } from '../users/users.service';
import {
getWorkflowById,
getSharedWorkflow,
Expand All @@ -26,6 +25,7 @@ import {
} from './workflows.service';
import { WorkflowsService } from '@/workflows/workflows.services';
import { InternalHooks } from '@/InternalHooks';
import { RoleService } from '@/services/role.service';

export = {
createWorkflow: [
Expand All @@ -39,7 +39,7 @@ export = {

addNodeIds(workflow);

const role = await getWorkflowOwnerRole();
const role = await Container.get(RoleService).findWorkflowOwnerRole();

const createdWorkflow = await createWorkflow(workflow, req.user, role);

Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/Server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ import { SourceControlController } from '@/environments/sourceControl/sourceCont
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);

Expand Down Expand Up @@ -496,6 +497,7 @@ export class Server extends AbstractServer {
logger,
postHog,
jwtService,
roleService: Container.get(RoleService),
}),
Container.get(SamlController),
Container.get(SourceControlController),
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/UserManagement/PermissionChecker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import { In } from 'typeorm';
import * as Db from '@/Db';
import config from '@/config';
import type { SharedCredentials } from '@db/entities/SharedCredentials';
import { getRoleId, isSharingEnabled } from './UserManagementHelper';
import { isSharingEnabled } from './UserManagementHelper';
import { WorkflowsService } from '@/workflows/workflows.services';
import { UserService } from '@/user/user.service';
import { OwnershipService } from '@/services/ownership.service';
import Container from 'typedi';
import { RoleService } from '@/services/role.service';

export class PermissionChecker {
/**
Expand Down Expand Up @@ -54,8 +55,9 @@ export class PermissionChecker {
const credentialsWhere: FindOptionsWhere<SharedCredentials> = { userId: In(workflowUserIds) };

if (!isSharingEnabled()) {
const role = await Container.get(RoleService).findCredentialOwnerRole();
// If credential sharing is not enabled, get only credentials owned by this user
credentialsWhere.roleId = await getRoleId('credential', 'owner');
credentialsWhere.roleId = role.id;
}

const credentialSharings = await Db.collections.SharedCredentials.find({
Expand Down
18 changes: 5 additions & 13 deletions packages/cli/src/UserManagement/UserManagementHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import * as ResponseHelper from '@/ResponseHelper';
import type { CurrentUser, PublicUser, WhereClause } from '@/Interfaces';
import type { User } from '@db/entities/User';
import { MAX_PASSWORD_LENGTH, MIN_PASSWORD_LENGTH } from '@db/entities/User';
import type { Role } from '@db/entities/Role';
import { RoleRepository } from '@db/repositories';
import config from '@/config';
import { License } from '@/License';
import { getWebhookBaseUrl } from '@/WebhookHelpers';
import type { PostHogClient } from '@/posthog';
import { RoleService } from '@/services/role.service';

export function isEmailSetUp(): boolean {
const smtp = config.getEnv('userManagement.emails.mode') === 'smtp';
Expand All @@ -27,22 +26,15 @@ export function isSharingEnabled(): boolean {
return Container.get(License).isSharingEnabled();
}

export async function getRoleId(scope: Role['scope'], name: Role['name']): Promise<Role['id']> {
return Container.get(RoleRepository)
.findRoleOrFail(scope, name)
.then((role) => role.id);
}

export async function getInstanceOwner(): Promise<User> {
const ownerRoleId = await getRoleId('global', 'owner');
export async function getInstanceOwner() {
const globalOwnerRole = await Container.get(RoleService).findGlobalOwnerRole();

const owner = await Db.collections.User.findOneOrFail({
return Db.collections.User.findOneOrFail({
relations: ['globalRole'],
where: {
globalRoleId: ownerRoleId,
globalRoleId: globalOwnerRole.id,
},
});
return owner;
}

/**
Expand Down
16 changes: 10 additions & 6 deletions packages/cli/src/WorkflowHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,14 @@ import { WorkflowRunner } from '@/WorkflowRunner';
import config from '@/config';
import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
import type { User } from '@db/entities/User';
import { RoleRepository } from '@db/repositories';
import omit from 'lodash/omit';
import { PermissionChecker } from './UserManagement/PermissionChecker';
import { isWorkflowIdValid } from './utils';
import { UserService } from './user/user.service';
import type { SharedWorkflow } from '@db/entities/SharedWorkflow';
import type { RoleNames } from '@db/entities/Role';
import { RoleService } from './services/role.service';
import { RoleRepository } from './databases/repositories';
import { VariablesService } from './environments/variables/variables.service';

const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');
Expand Down Expand Up @@ -378,10 +379,13 @@ export async function getSharedWorkflowIds(user: User, roles?: RoleNames[]): Pro
where.userId = user.id;
}
if (roles?.length) {
const roleIds = await Db.collections.Role.find({
select: ['id'],
where: { name: In(roles), scope: 'workflow' },
}).then((data) => data.map(({ id }) => id));
const roleIds = await Container.get(RoleRepository)
.find({
select: ['id'],
where: { name: In(roles), scope: 'workflow' },
})
.then((role) => role.map(({ id }) => id));

where.roleId = In(roleIds);
}
const sharedWorkflows = await Db.collections.SharedWorkflow.find({
Expand All @@ -398,7 +402,7 @@ export async function isBelowOnboardingThreshold(user: User): Promise<boolean> {
let belowThreshold = true;
const skippedTypes = ['n8n-nodes-base.start', 'n8n-nodes-base.stickyNote'];

const workflowOwnerRole = await Container.get(RoleRepository).findWorkflowOwnerRole();
const workflowOwnerRole = await Container.get(RoleService).findWorkflowOwnerRole();
const ownedWorkflowsIds = await Db.collections.SharedWorkflow.find({
where: {
userId: user.id,
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/commands/import/credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import type { User } from '@db/entities/User';
import { SharedCredentials } from '@db/entities/SharedCredentials';
import type { Role } from '@db/entities/Role';
import { CredentialsEntity } from '@db/entities/CredentialsEntity';
import { RoleRepository } from '@db/repositories';
import { disableAutoGeneratedIds } from '@db/utils/commandHelpers';
import { BaseCommand, UM_FIX_INSTRUCTION } from '../BaseCommand';
import type { ICredentialsEncrypted } from 'n8n-workflow';
import { jsonParse } from 'n8n-workflow';
import { RoleService } from '@/services/role.service';

export class ImportCredentialsCommand extends BaseCommand {
static description = 'Import credentials';
Expand Down Expand Up @@ -147,7 +147,7 @@ export class ImportCredentialsCommand extends BaseCommand {
}

private async initOwnerCredentialRole() {
const ownerCredentialRole = await Container.get(RoleRepository).findCredentialOwnerRole();
const ownerCredentialRole = await Container.get(RoleService).findCredentialOwnerRole();

if (!ownerCredentialRole) {
throw new Error(`Failed to find owner credential role. ${UM_FIX_INSTRUCTION}`);
Expand All @@ -170,7 +170,7 @@ export class ImportCredentialsCommand extends BaseCommand {
}

private async getOwner() {
const ownerGlobalRole = await Container.get(RoleRepository).findGlobalOwnerRole();
const ownerGlobalRole = await Container.get(RoleService).findGlobalOwnerRole();

const owner =
ownerGlobalRole &&
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/commands/import/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import { WorkflowEntity } from '@db/entities/WorkflowEntity';
import type { Role } from '@db/entities/Role';
import type { User } from '@db/entities/User';
import { setTagsForImport } from '@/TagHelpers';
import { RoleRepository } from '@db/repositories';
import { disableAutoGeneratedIds } from '@db/utils/commandHelpers';
import type { ICredentialsDb, IWorkflowToImport } from '@/Interfaces';
import { replaceInvalidCredentials } from '@/WorkflowHelpers';
import { BaseCommand, UM_FIX_INSTRUCTION } from '../BaseCommand';
import { generateNanoId } from '@db/utils/generators';
import { RoleService } from '@/services/role.service';

function assertHasWorkflowsToImport(workflows: unknown): asserts workflows is IWorkflowToImport[] {
if (!Array.isArray(workflows)) {
Expand Down Expand Up @@ -208,7 +208,7 @@ export class ImportWorkflowsCommand extends BaseCommand {
}

private async initOwnerWorkflowRole() {
const ownerWorkflowRole = await Container.get(RoleRepository).findWorkflowOwnerRole();
const ownerWorkflowRole = await Container.get(RoleService).findWorkflowOwnerRole();

if (!ownerWorkflowRole) {
throw new Error(`Failed to find owner workflow role. ${UM_FIX_INSTRUCTION}`);
Expand All @@ -231,7 +231,7 @@ export class ImportWorkflowsCommand extends BaseCommand {
}

private async getOwner() {
const ownerGlobalRole = await Container.get(RoleRepository).findGlobalOwnerRole();
const ownerGlobalRole = await Container.get(RoleService).findGlobalOwnerRole();

const owner =
ownerGlobalRole &&
Expand Down
8 changes: 4 additions & 4 deletions packages/cli/src/commands/user-management/reset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { Not } from 'typeorm';
import * as Db from '@/Db';
import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
import { User } from '@db/entities/User';
import { RoleRepository } from '@db/repositories';
import { BaseCommand } from '../BaseCommand';
import { RoleService } from '@/services/role.service';

const defaultUserProps = {
firstName: null,
Expand All @@ -21,8 +21,8 @@ export class Reset extends BaseCommand {
async run(): Promise<void> {
const owner = await this.getInstanceOwner();

const ownerWorkflowRole = await Container.get(RoleRepository).findWorkflowOwnerRoleOrFail();
const ownerCredentialRole = await Container.get(RoleRepository).findCredentialOwnerRoleOrFail();
const ownerWorkflowRole = await Container.get(RoleService).findWorkflowOwnerRole();
const ownerCredentialRole = await Container.get(RoleService).findCredentialOwnerRole();

await Db.collections.SharedWorkflow.update(
{ userId: Not(owner.id), roleId: ownerWorkflowRole.id },
Expand Down Expand Up @@ -60,7 +60,7 @@ export class Reset extends BaseCommand {
}

async getInstanceOwner(): Promise<User> {
const globalRole = await Container.get(RoleRepository).findGlobalOwnerRoleOrFail();
const globalRole = await Container.get(RoleService).findGlobalOwnerRole();

const owner = await Db.collections.User.findOneBy({ globalRoleId: globalRole.id });

Expand Down
4 changes: 3 additions & 1 deletion packages/cli/src/controllers/e2e.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class E2EController {
];

const [{ id: globalOwnerRoleId }, { id: globalMemberRoleId }] = await this.roleRepo.save(
roles.map(([name, scope], index) => ({ name, scope, id: index.toString() })),
roles.map(([name, scope], index) => ({ name, scope, id: (index + 1).toString() })),
);

const users = [];
Expand All @@ -151,6 +151,8 @@ export class E2EController {
);
}

console.log('users', users);

await this.userRepo.insert(users);

await this.settingsRepo.update(
Expand Down
Loading

0 comments on commit e4f0418

Please sign in to comment.