From 1cb92ffe16233ba3e99084da3e3f53a4c658086f Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Wed, 29 Nov 2023 14:48:36 +0000 Subject: [PATCH] feat: Replace owner checks with scope checks (no-changelog) (#7846) Github issue / Community forum post (link here to close automatically): --- packages/@n8n/permissions/src/types.ts | 6 ++--- packages/cli/src/ActiveWorkflowRunner.ts | 10 +++++---- packages/cli/src/CredentialsHelper.ts | 2 +- packages/cli/src/Server.ts | 8 +++++-- .../src/UserManagement/PermissionChecker.ts | 2 +- .../UserManagement/UserManagementHelper.ts | 10 +++++---- packages/cli/src/WorkflowHelpers.ts | 4 ++-- .../controllers/passwordReset.controller.ts | 5 ++++- .../cli/src/controllers/users.controller.ts | 10 +-------- .../workflowStatistics.controller.ts | 3 ++- .../credentials/credentials.controller.ee.ts | 7 ++++-- .../src/credentials/credentials.controller.ts | 22 +++++++++++++++---- .../src/credentials/credentials.service.ee.ts | 19 ++++++++-------- .../src/credentials/credentials.service.ts | 18 ++++++++++----- .../repositories/execution.repository.ts | 4 ++-- .../sharedCredentials.repository.ts | 2 +- .../cli/src/executions/executions.service.ts | 2 +- packages/cli/src/permissions/roles.ts | 3 +++ .../workflowHistory.service.ee.ts | 2 +- .../cli/src/workflows/workflows.controller.ts | 3 ++- .../src/workflows/workflows.services.ee.ts | 7 +++--- .../cli/src/workflows/workflows.services.ts | 21 ++++++++++++------ .../cli/test/integration/auth.api.test.ts | 1 - .../cli/test/integration/shared/db/users.ts | 5 +++-- .../cli/test/integration/users.api.test.ts | 17 +++++++------- .../sharedCredentials.repository.test.ts | 21 ++++++++++++++++-- 26 files changed, 136 insertions(+), 78 deletions(-) diff --git a/packages/@n8n/permissions/src/types.ts b/packages/@n8n/permissions/src/types.ts index ebd5015cb4..5896ca52cb 100644 --- a/packages/@n8n/permissions/src/types.ts +++ b/packages/@n8n/permissions/src/types.ts @@ -30,7 +30,7 @@ export type CommunityPackageScope = ResourceScope< 'install' | 'uninstall' | 'update' | 'list' | 'manage' >; export type CredentialScope = ResourceScope<'credential', DefaultOperations | 'share'>; -export type ExternalSecretScope = ResourceScope<'externalSecret', 'list'>; +export type ExternalSecretScope = ResourceScope<'externalSecret', 'list' | 'use'>; export type ExternalSecretProviderScope = ResourceScope< 'externalSecretsProvider', DefaultOperations | 'sync' @@ -46,9 +46,9 @@ export type OrchestrationScope = ResourceScope<'orchestration', 'read' | 'list'> export type SamlScope = ResourceScope<'saml', 'manage'>; export type SourceControlScope = ResourceScope<'sourceControl', 'pull' | 'push' | 'manage'>; export type TagScope = ResourceScope<'tag'>; -export type UserScope = ResourceScope<'user', DefaultOperations | 'resetPassword'>; +export type UserScope = ResourceScope<'user', DefaultOperations | 'resetPassword' | 'changeRole'>; export type VariableScope = ResourceScope<'variable'>; -export type WorkflowScope = ResourceScope<'workflow', DefaultOperations | 'share'>; +export type WorkflowScope = ResourceScope<'workflow', DefaultOperations | 'share' | 'execute'>; export type Scope = | AuditLogsScope diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index 966092100f..8ddc228a20 100644 --- a/packages/cli/src/ActiveWorkflowRunner.ts +++ b/packages/cli/src/ActiveWorkflowRunner.ts @@ -68,6 +68,7 @@ import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.reposi import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { MultiMainSetup } from '@/services/orchestration/main/MultiMainSetup.ee'; import { ActivationErrorsService } from '@/ActivationErrors.service'; +import type { Scope } from '@n8n/permissions'; import { NotFoundError } from './errors/response-errors/not-found.error'; const WEBHOOK_PROD_UNREGISTERED_HINT = @@ -270,8 +271,8 @@ export class ActiveWorkflowRunner implements IWebhookManager { /** * Get the IDs of active workflows from storage. */ - async allActiveInStorage(user?: User) { - const isFullAccess = !user || user.globalRole.name === 'owner'; + async allActiveInStorage(options?: { user: User; scope: Scope | Scope[] }) { + const isFullAccess = !options?.user || (await options.user.hasGlobalScope(options.scope)); const activationErrors = await this.activationErrorsService.getAll(); @@ -286,8 +287,9 @@ export class ActiveWorkflowRunner implements IWebhookManager { .filter((workflowId) => !activationErrors[workflowId]); } - const where = whereClause({ - user, + const where = await whereClause({ + user: options.user, + globalScope: 'workflow:list', entityType: 'workflow', }); diff --git a/packages/cli/src/CredentialsHelper.ts b/packages/cli/src/CredentialsHelper.ts index 3877bbd84c..6354755438 100644 --- a/packages/cli/src/CredentialsHelper.ts +++ b/packages/cli/src/CredentialsHelper.ts @@ -579,7 +579,7 @@ export class CredentialsHelper extends ICredentialsHelper { credentialType, 'internal' as WorkflowExecuteMode, undefined, - user.isOwner, + await user.hasGlobalScope('externalSecret:use'), ); } catch (error) { this.logger.debug('Credential test failed', error); diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index f12a08d6b1..30b2578889 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -448,7 +448,10 @@ export class Server extends AbstractServer { this.app.get( `/${this.restEndpoint}/active`, ResponseHelper.send(async (req: WorkflowRequest.GetAllActive) => { - return this.activeWorkflowRunner.allActiveInStorage(req.user); + return this.activeWorkflowRunner.allActiveInStorage({ + user: req.user, + scope: 'workflow:list', + }); }), ); @@ -460,8 +463,9 @@ export class Server extends AbstractServer { const shared = await Container.get(SharedWorkflowRepository).findOne({ relations: ['workflow'], - where: whereClause({ + where: await whereClause({ user: req.user, + globalScope: 'workflow:read', entityType: 'workflow', entityId: workflowId, }), diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 44bde9900a..2d18939b2e 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -32,7 +32,7 @@ export class PermissionChecker { relations: ['globalRole'], }); - if (user.globalRole.name === 'owner') return; + if (await user.hasGlobalScope('workflow:execute')) return; // allow if all creds used in this workflow are a subset of // all creds accessible to users who have access to this workflow diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index 1b74bf32e5..7ba50b9c21 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -10,6 +10,7 @@ import { License } from '@/License'; import { getWebhookBaseUrl } from '@/WebhookHelpers'; import { RoleService } from '@/services/role.service'; import { UserRepository } from '@db/repositories/user.repository'; +import type { Scope } from '@n8n/permissions'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ApplicationError } from 'n8n-workflow'; @@ -131,21 +132,22 @@ export function rightDiff( * Build a `where` clause for a TypeORM entity search, * checking for member access if the user is not an owner. */ -export function whereClause({ +export async function whereClause({ user, entityType, + globalScope, entityId = '', roles = [], }: { user: User; entityType: 'workflow' | 'credentials'; + globalScope: Scope; entityId?: string; roles?: string[]; -}): WhereClause { +}): Promise { const where: WhereClause = entityId ? { [entityType]: { id: entityId } } : {}; - // TODO: Decide if owner access should be restricted - if (user.globalRole.name !== 'owner') { + if (!(await user.hasGlobalScope(globalScope))) { where.user = { id: user.id }; if (roles?.length) { where.role = { name: In(roles) }; diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 31407da9d1..135de736e8 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -415,11 +415,11 @@ export async function replaceInvalidCredentials(workflow: WorkflowEntity): Promi /** * Get the IDs of the workflows that have been shared with the user. - * Returns all IDs if user is global owner (see `whereClause`) + * Returns all IDs if user has the 'workflow:read' scope (see `whereClause`) */ export async function getSharedWorkflowIds(user: User, roles?: RoleNames[]): Promise { const where: FindOptionsWhere = {}; - if (user.globalRole?.name !== 'owner') { + if (!(await user.hasGlobalScope('workflow:read'))) { where.userId = user.id; } if (roles?.length) { diff --git a/packages/cli/src/controllers/passwordReset.controller.ts b/packages/cli/src/controllers/passwordReset.controller.ts index 902a2071a8..f0429748aa 100644 --- a/packages/cli/src/controllers/passwordReset.controller.ts +++ b/packages/cli/src/controllers/passwordReset.controller.ts @@ -97,7 +97,10 @@ export class PasswordResetController { } if ( isSamlCurrentAuthenticationMethod() && - !(user?.globalRole.name === 'owner' || user?.settings?.allowSSOManualLogin === true) + !( + (user && (await user.hasGlobalScope('user:resetPassword'))) === true || + user?.settings?.allowSSOManualLogin === true + ) ) { this.logger.debug( 'Request to send password reset email failed because login is handled by SAML', diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index 619c18a8e0..4f7b67934c 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -38,7 +38,6 @@ export class UsersController { static ERROR_MESSAGES = { CHANGE_ROLE: { - NO_MEMBER: 'Member cannot change role for any user', MISSING_NEW_ROLE_KEY: 'Expected `newRole` to exist', MISSING_NEW_ROLE_VALUE: 'Expected `newRole` to have `name` and `scope`', NO_USER: 'Target user not found', @@ -326,13 +325,10 @@ export class UsersController { return { success: true }; } - // @TODO: Add scope check `@RequireGlobalScope('user:changeRole')` - // once this has been merged: https://github.com/n8n-io/n8n/pull/7737 - @Authorized('any') @Patch('/:id/role') + @RequireGlobalScope('user:changeRole') async changeRole(req: UserRequest.ChangeRole) { const { - NO_MEMBER, MISSING_NEW_ROLE_KEY, MISSING_NEW_ROLE_VALUE, NO_ADMIN_ON_OWNER, @@ -342,10 +338,6 @@ export class UsersController { NO_ADMIN_IF_UNLICENSED, } = UsersController.ERROR_MESSAGES.CHANGE_ROLE; - if (req.user.globalRole.scope === 'global' && req.user.globalRole.name === 'member') { - throw new UnauthorizedError(NO_MEMBER); - } - const { newRole } = req.body; if (!newRole) { diff --git a/packages/cli/src/controllers/workflowStatistics.controller.ts b/packages/cli/src/controllers/workflowStatistics.controller.ts index 5492f9884c..4a868bdc2e 100644 --- a/packages/cli/src/controllers/workflowStatistics.controller.ts +++ b/packages/cli/src/controllers/workflowStatistics.controller.ts @@ -37,8 +37,9 @@ export class WorkflowStatisticsController { const workflowId = req.params.id; const allowed = await this.sharedWorkflowRepository.exist({ relations: ['workflow'], - where: whereClause({ + where: await whereClause({ user, + globalScope: 'workflow:read', entityType: 'workflow', entityId: workflowId, }), diff --git a/packages/cli/src/credentials/credentials.controller.ee.ts b/packages/cli/src/credentials/credentials.controller.ee.ts index 0bc21d869a..71a165581b 100644 --- a/packages/cli/src/credentials/credentials.controller.ee.ts +++ b/packages/cli/src/credentials/credentials.controller.ee.ts @@ -50,7 +50,7 @@ EECredentialsController.get( const userSharing = credential.shared?.find((shared) => shared.user.id === req.user.id); - if (!userSharing && req.user.globalRole.name !== 'owner') { + if (!userSharing && !(await req.user.hasGlobalScope('credential:read'))) { throw new UnauthorizedError('Forbidden.'); } @@ -82,7 +82,10 @@ EECredentialsController.post( const credentialId = credentials.id; const { ownsCredential } = await EECredentials.isOwned(req.user, credentialId); - const sharing = await EECredentials.getSharing(req.user, credentialId); + const sharing = await EECredentials.getSharing(req.user, credentialId, { + allowGlobalScope: true, + globalScope: 'credential:read', + }); if (!ownsCredential) { if (!sharing) { throw new UnauthorizedError('Forbidden'); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 11ba2c0f38..75e533f0ed 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -58,7 +58,12 @@ credentialsController.get( const { id: credentialId } = req.params; const includeDecryptedData = req.query.includeData === 'true'; - const sharing = await CredentialsService.getSharing(req.user, credentialId, ['credentials']); + const sharing = await CredentialsService.getSharing( + req.user, + credentialId, + { allowGlobalScope: true, globalScope: 'credential:read' }, + ['credentials'], + ); if (!sharing) { throw new NotFoundError(`Credential with ID "${credentialId}" could not be found.`); @@ -91,7 +96,10 @@ credentialsController.post( ResponseHelper.send(async (req: CredentialRequest.Test): Promise => { const { credentials } = req.body; - const sharing = await CredentialsService.getSharing(req.user, credentials.id); + const sharing = await CredentialsService.getSharing(req.user, credentials.id, { + allowGlobalScope: true, + globalScope: 'credential:read', + }); const mergedCredentials = deepCopy(credentials); if (mergedCredentials.data && sharing?.credentials) { @@ -134,7 +142,10 @@ credentialsController.patch( ResponseHelper.send(async (req: CredentialRequest.Update): Promise => { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing(req.user, credentialId); + const sharing = await CredentialsService.getSharing(req.user, credentialId, { + allowGlobalScope: true, + globalScope: 'credential:update', + }); if (!sharing) { Container.get(Logger).info( @@ -184,7 +195,10 @@ credentialsController.delete( ResponseHelper.send(async (req: CredentialRequest.Delete) => { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing(req.user, credentialId); + const sharing = await CredentialsService.getSharing(req.user, credentialId, { + allowGlobalScope: true, + globalScope: 'credential:delete', + }); if (!sharing) { Container.get(Logger).info( diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index 67b65dcfb7..416df39bf0 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -4,7 +4,7 @@ import { CredentialsEntity } from '@db/entities/CredentialsEntity'; import { SharedCredentials } from '@db/entities/SharedCredentials'; import type { User } from '@db/entities/User'; import { UserService } from '@/services/user.service'; -import { CredentialsService } from './credentials.service'; +import { CredentialsService, type CredentialsGetSharedOptions } from './credentials.service'; import { RoleService } from '@/services/role.service'; import Container from 'typedi'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; @@ -14,9 +14,10 @@ export class EECredentialsService extends CredentialsService { user: User, credentialId: string, ): Promise<{ ownsCredential: boolean; credential?: CredentialsEntity }> { - const sharing = await this.getSharing(user, credentialId, ['credentials', 'role'], { - allowGlobalOwner: false, - }); + const sharing = await this.getSharing(user, credentialId, { allowGlobalScope: false }, [ + 'credentials', + 'role', + ]); if (!sharing || sharing.role.name !== 'owner') return { ownsCredential: false }; @@ -31,15 +32,15 @@ export class EECredentialsService extends CredentialsService { static async getSharing( user: User, credentialId: string, + options: CredentialsGetSharedOptions, relations: string[] = ['credentials'], - { allowGlobalOwner } = { allowGlobalOwner: true }, ): Promise { const where: FindOptionsWhere = { credentialsId: credentialId }; - // Omit user from where if the requesting user is the global - // owner. This allows the global owner to view and delete - // credentials they don't own. - if (!allowGlobalOwner || user.globalRole.name !== 'owner') { + // Omit user from where if the requesting user has relevant + // global credential permissions. This allows the user to + // access credentials they don't own. + if (!options.allowGlobalScope || !(await user.hasGlobalScope(options.globalScope))) { where.userId = user.id; } diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 7c1c816ee8..83e28f6d0d 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -11,6 +11,8 @@ import { Container } from 'typedi'; import type { FindManyOptions, FindOptionsWhere } from 'typeorm'; import { In, Like } from 'typeorm'; +import type { Scope } from '@n8n/permissions'; + import * as Db from '@/Db'; import type { ICredentialsDb } from '@/Interfaces'; import { CredentialsHelper, createCredentialsFromCredentialsEntity } from '@/CredentialsHelper'; @@ -28,6 +30,10 @@ import { Logger } from '@/Logger'; import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; +export type CredentialsGetSharedOptions = + | { allowGlobalScope: true; globalScope: Scope } + | { allowGlobalScope: false }; + export class CredentialsService { static async get( where: FindOptionsWhere, @@ -86,7 +92,7 @@ export class CredentialsService { ) { const findManyOptions = this.toFindManyOptions(options.listQueryOptions); - const returnAll = user.globalRole.name === 'owner' && !options.onlyOwn; + const returnAll = (await user.hasGlobalScope('credential:list')) && !options.onlyOwn; const isDefaultSelect = !options.listQueryOptions?.select; if (returnAll) { @@ -136,15 +142,15 @@ export class CredentialsService { static async getSharing( user: User, credentialId: string, + options: CredentialsGetSharedOptions, relations: string[] = ['credentials'], - { allowGlobalOwner } = { allowGlobalOwner: true }, ): Promise { const where: FindOptionsWhere = { credentialsId: credentialId }; - // Omit user from where if the requesting user is the global - // owner. This allows the global owner to view and delete - // credentials they don't own. - if (!allowGlobalOwner || user.globalRole.name !== 'owner') { + // Omit user from where if the requesting user has relevant + // global credential permissions. This allows the user to + // access credentials they don't own. + if (!options.allowGlobalScope || !(await user.hasGlobalScope(options.globalScope))) { Object.assign(where, { userId: user.id, role: { name: 'owner' }, diff --git a/packages/cli/src/databases/repositories/execution.repository.ts b/packages/cli/src/databases/repositories/execution.repository.ts index 7bd19f4c79..f8e1350588 100644 --- a/packages/cli/src/databases/repositories/execution.repository.ts +++ b/packages/cli/src/databases/repositories/execution.repository.ts @@ -273,10 +273,10 @@ export class ExecutionRepository extends Repository { filters: IGetExecutionsQueryFilter | undefined, accessibleWorkflowIds: string[], currentlyRunningExecutions: string[], - isOwner: boolean, + hasGlobalRead: boolean, ): Promise<{ count: number; estimated: boolean }> { const dbType = config.getEnv('database.type'); - if (dbType !== 'postgresdb' || (filters && Object.keys(filters).length > 0) || !isOwner) { + if (dbType !== 'postgresdb' || (filters && Object.keys(filters).length > 0) || !hasGlobalRead) { const query = this.createQueryBuilder('execution').andWhere( 'execution.workflowId IN (:...accessibleWorkflowIds)', { accessibleWorkflowIds }, diff --git a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts index d2a7388863..2ba0af3484 100644 --- a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts +++ b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts @@ -15,7 +15,7 @@ export class SharedCredentialsRepository extends Repository { relations: ['credentials'], where: { credentialsId, - ...(!user.isOwner ? { userId: user.id } : {}), + ...(!(await user.hasGlobalScope('credential:read')) ? { userId: user.id } : {}), }, }); if (!sharedCredential) return null; diff --git a/packages/cli/src/executions/executions.service.ts b/packages/cli/src/executions/executions.service.ts index 605867b29a..ab9999d81f 100644 --- a/packages/cli/src/executions/executions.service.ts +++ b/packages/cli/src/executions/executions.service.ts @@ -155,7 +155,7 @@ export class ExecutionsService { filter, sharedWorkflowIds, executingWorkflowIds, - req.user.globalRole.name === 'owner', + await req.user.hasGlobalScope('workflow:list'), ); const formattedExecutions = await Container.get(ExecutionRepository).searchExecutions( diff --git a/packages/cli/src/permissions/roles.ts b/packages/cli/src/permissions/roles.ts index b8f19349cb..59b4640828 100644 --- a/packages/cli/src/permissions/roles.ts +++ b/packages/cli/src/permissions/roles.ts @@ -32,6 +32,7 @@ export const ownerPermissions: Scope[] = [ 'externalSecretsProvider:list', 'externalSecretsProvider:sync', 'externalSecret:list', + 'externalSecret:use', 'ldap:manage', 'ldap:sync', 'logStreaming:manage', @@ -52,6 +53,7 @@ export const ownerPermissions: Scope[] = [ 'user:delete', 'user:list', 'user:resetPassword', + 'user:changeRole', 'variable:create', 'variable:read', 'variable:update', @@ -63,6 +65,7 @@ export const ownerPermissions: Scope[] = [ 'workflow:delete', 'workflow:list', 'workflow:share', + 'workflow:execute', ]; export const adminPermissions: Scope[] = ownerPermissions.concat(); export const memberPermissions: Scope[] = [ diff --git a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts b/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts index 84cb05f9a6..e5243551b2 100644 --- a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts +++ b/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts @@ -21,7 +21,7 @@ export class WorkflowHistoryService { private async getSharedWorkflow(user: User, workflowId: string): Promise { return this.sharedWorkflowRepository.findOne({ where: { - ...(!user.isOwner && { userId: user.id }), + ...(!(await user.hasGlobalScope('workflow:read')) && { userId: user.id }), workflowId, }, }); diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 588a1040d3..8b36e44f0a 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -211,9 +211,10 @@ workflowsController.get( const shared = await Container.get(SharedWorkflowRepository).findOne({ relations, - where: whereClause({ + where: await whereClause({ user: req.user, entityType: 'workflow', + globalScope: 'workflow:read', entityId: workflowId, roles: ['owner'], }), diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index 7ebdd02b7b..d6a9337b2d 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -24,9 +24,10 @@ export class EEWorkflowsService extends WorkflowsService { user: User, workflowId: string, ): Promise<{ ownsWorkflow: boolean; workflow?: WorkflowEntity }> { - const sharing = await this.getSharing(user, workflowId, ['workflow', 'role'], { - allowGlobalOwner: false, - }); + const sharing = await this.getSharing(user, workflowId, { allowGlobalScope: false }, [ + 'workflow', + 'role', + ]); if (!sharing || sharing.role.name !== 'owner') return { ownsWorkflow: false }; diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 40ad9cede9..d0485f9c46 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -28,6 +28,7 @@ import { OwnershipService } from '@/services/ownership.service'; import { isStringArray, isWorkflowIdValid } from '@/utils'; import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; import { BinaryDataService } from 'n8n-core'; +import type { Scope } from '@n8n/permissions'; import { Logger } from '@/Logger'; import { MultiMainSetup } from '@/services/orchestration/main/MultiMainSetup.ee'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; @@ -36,19 +37,23 @@ import { ExecutionRepository } from '@db/repositories/execution.repository'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; +export type WorkflowsGetSharedOptions = + | { allowGlobalScope: true; globalScope: Scope } + | { allowGlobalScope: false }; + export class WorkflowsService { static async getSharing( user: User, workflowId: string, + options: WorkflowsGetSharedOptions, relations: string[] = ['workflow'], - { allowGlobalOwner } = { allowGlobalOwner: true }, ): Promise { const where: FindOptionsWhere = { workflowId }; - // Omit user from where if the requesting user is the global - // owner. This allows the global owner to view and delete - // workflows they don't own. - if (!allowGlobalOwner || user.globalRole.name !== 'owner') { + // Omit user from where if the requesting user has relevant + // global workflow permissions. This allows the user to + // access workflows they don't own. + if (!options.allowGlobalScope || !(await user.hasGlobalScope(options.globalScope))) { where.userId = user.id; } @@ -195,8 +200,9 @@ export class WorkflowsService { ): Promise { const shared = await Container.get(SharedWorkflowRepository).findOne({ relations: ['workflow', 'role'], - where: whereClause({ + where: await whereClause({ user, + globalScope: 'workflow:update', entityType: 'workflow', entityId: workflowId, roles, @@ -476,8 +482,9 @@ export class WorkflowsService { const sharedWorkflow = await Container.get(SharedWorkflowRepository).findOne({ relations: ['workflow', 'role'], - where: whereClause({ + where: await whereClause({ user, + globalScope: 'workflow:delete', entityType: 'workflow', entityId: workflowId, roles: ['owner'], diff --git a/packages/cli/test/integration/auth.api.test.ts b/packages/cli/test/integration/auth.api.test.ts index 8b6b2d1957..5e61fc584a 100644 --- a/packages/cli/test/integration/auth.api.test.ts +++ b/packages/cli/test/integration/auth.api.test.ts @@ -97,7 +97,6 @@ describe('POST /login', () => { const ownerUser = await createUser({ password: randomValidPassword(), globalRole: globalOwnerRole, - isOwner: true, }); const response = await testServer.authAgentFor(ownerUser).get('/login'); diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index 25a8d7df18..ce7515b4b7 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -16,7 +16,7 @@ import { getGlobalAdminRole, getGlobalMemberRole, getGlobalOwnerRole } from './r */ export async function createUser(attributes: Partial = {}): Promise { const { email, password, firstName, lastName, globalRole, ...rest } = attributes; - const user: Partial = { + const user = Container.get(UserRepository).create({ email: email ?? randomEmail(), password: await hash(password ?? randomValidPassword(), 10), firstName: firstName ?? randomName(), @@ -24,7 +24,8 @@ export async function createUser(attributes: Partial = {}): Promise globalRoleId: (globalRole ?? (await getGlobalMemberRole())).id, globalRole, ...rest, - }; + }); + user.computeIsOwner(); return Container.get(UserRepository).save(user); } diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 342df5f672..a6a05481e0 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -362,7 +362,6 @@ describe('PATCH /users/:id/role', () => { let authlessAgent: SuperAgentTest; const { - NO_MEMBER, MISSING_NEW_ROLE_KEY, MISSING_NEW_ROLE_VALUE, NO_ADMIN_ON_OWNER, @@ -372,6 +371,8 @@ describe('PATCH /users/:id/role', () => { NO_ADMIN_IF_UNLICENSED, } = UsersController.ERROR_MESSAGES.CHANGE_ROLE; + const UNAUTHORIZED = 'Unauthorized'; + beforeAll(async () => { await testDb.truncate(['User']); @@ -406,7 +407,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to demote owner to admin', async () => { @@ -415,7 +416,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to demote admin to member', async () => { @@ -424,7 +425,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to promote other member to owner', async () => { @@ -433,7 +434,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to promote other member to admin', async () => { @@ -442,7 +443,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to promote self to admin', async () => { @@ -451,7 +452,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); test('should fail to promote self to owner', async () => { @@ -460,7 +461,7 @@ describe('PATCH /users/:id/role', () => { }); expect(response.statusCode).toBe(403); - expect(response.body.message).toBe(NO_MEMBER); + expect(response.body.message).toBe(UNAUTHORIZED); }); }); diff --git a/packages/cli/test/unit/repositories/sharedCredentials.repository.test.ts b/packages/cli/test/unit/repositories/sharedCredentials.repository.test.ts index 1a4803a35c..3755fff579 100644 --- a/packages/cli/test/unit/repositories/sharedCredentials.repository.test.ts +++ b/packages/cli/test/unit/repositories/sharedCredentials.repository.test.ts @@ -6,6 +6,8 @@ import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; import { SharedCredentials } from '@db/entities/SharedCredentials'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import { mockInstance } from '../../shared/mocking'; +import { memberPermissions, ownerPermissions } from '@/permissions/roles'; +import { hasScope } from '@n8n/permissions'; describe('SharedCredentialsRepository', () => { const entityManager = mockInstance(EntityManager); @@ -20,8 +22,23 @@ describe('SharedCredentialsRepository', () => { const credentialsId = 'cred_123'; const sharedCredential = mock(); sharedCredential.credentials = mock({ id: credentialsId }); - const owner = mock({ isOwner: true }); - const member = mock({ isOwner: false, id: 'test' }); + const owner = mock({ + isOwner: true, + hasGlobalScope: async (scope) => { + return hasScope(scope, { + global: ownerPermissions, + }); + }, + }); + const member = mock({ + isOwner: false, + id: 'test', + hasGlobalScope: async (scope) => { + return hasScope(scope, { + global: memberPermissions, + }); + }, + }); beforeEach(() => { jest.resetAllMocks();