diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index f9f6a92409..3ae49bd00d 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -458,7 +458,11 @@ export interface IN8nUISettings { saveManualExecutions: boolean; executionTimeout: number; maxExecutionTimeout: number; - workflowCallerPolicyDefaultOption: 'any' | 'none' | 'workflowsFromAList'; + workflowCallerPolicyDefaultOption: + | 'any' + | 'none' + | 'workflowsFromAList' + | 'workflowsFromSameOwner'; oauthCallbackUrls: { oauth1: string; oauth2: string; @@ -498,7 +502,6 @@ export interface IN8nUISettings { }; enterprise: { sharing: boolean; - workflowSharing: boolean; }; hideUsagePage: boolean; license: { diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index e4b72b3154..626c5e6f72 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -17,6 +17,7 @@ import { IExecutionTrackProperties, } from '@/Interfaces'; import { Telemetry } from '@/telemetry'; +import { RoleService } from './role/role.service'; export class InternalHooksClass implements IInternalHooksClass { private versionCli: string; @@ -111,6 +112,14 @@ export class InternalHooksClass implements IInternalHooksClass { (note) => note.overlapping, ).length; + let userRole: 'owner' | 'sharee' | undefined = undefined; + if (userId && workflow.id) { + const role = await RoleService.getUserRoleForWorkflow(userId, workflow.id.toString()); + if (role) { + userRole = role.name === 'owner' ? 'owner' : 'sharee'; + } + } + return this.telemetry.track( 'User saved workflow', { @@ -122,6 +131,7 @@ export class InternalHooksClass implements IInternalHooksClass { version_cli: this.versionCli, num_tags: workflow.tags?.length ?? 0, public_api: publicApi, + sharing_role: userRole, }, { withPostHog: true }, ); @@ -196,6 +206,14 @@ export class InternalHooksClass implements IInternalHooksClass { nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); } + let userRole: 'owner' | 'sharee' | undefined = undefined; + if (userId) { + const role = await RoleService.getUserRoleForWorkflow(userId, workflow.id.toString()); + if (role) { + userRole = role.name === 'owner' ? 'owner' : 'sharee'; + } + } + const manualExecEventProperties: ITelemetryTrackProperties = { user_id: userId, workflow_id: workflow.id.toString(), @@ -205,6 +223,7 @@ export class InternalHooksClass implements IInternalHooksClass { node_graph_string: properties.node_graph_string as string, error_node_id: properties.error_node_id as string, webhook_domain: null, + sharing_role: userRole, }; if (!manualExecEventProperties.node_graph_string) { @@ -254,6 +273,16 @@ export class InternalHooksClass implements IInternalHooksClass { ]).then(() => {}); } + async onWorkflowSharingUpdate(workflowId: string, userId: string, userList: string[]) { + const properties: ITelemetryTrackProperties = { + workflow_id: workflowId, + user_id_sharer: userId, + user_id_list: userList, + }; + + return this.telemetry.track('User updated workflow sharing', properties, { withPostHog: true }); + } + async onN8nStop(): Promise { const timeoutPromise = new Promise((resolve) => { setTimeout(() => { diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 65217120d3..eb0728f6b2 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -356,7 +356,6 @@ class App { }, enterprise: { sharing: false, - workflowSharing: false, }, hideUsagePage: config.getEnv('hideUsagePage'), license: { @@ -389,7 +388,6 @@ class App { // refresh enterprise status Object.assign(this.frontendSettings.enterprise, { sharing: isSharingEnabled(), - workflowSharing: config.getEnv('enterprise.workflowSharingEnabled'), }); if (config.get('nodes.packagesMissing').length > 0) { @@ -1003,7 +1001,7 @@ class App { }); if (!shared) { - LoggerProxy.info('User attempted to access workflow errors without permissions', { + LoggerProxy.verbose('User attempted to access workflow errors without permissions', { workflowId, userId: req.user.id, }); diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 24e906b11a..40b0dda26c 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -1,9 +1,17 @@ -import { INode, NodeOperationError, Workflow } from 'n8n-workflow'; +import { + INode, + NodeOperationError, + SubworkflowOperationError, + Workflow, + WorkflowOperationError, +} from 'n8n-workflow'; import { FindManyOptions, In, ObjectLiteral } from 'typeorm'; import * as Db from '@/Db'; import config from '@/config'; import type { SharedCredentials } from '@db/entities/SharedCredentials'; -import { getRole } from './UserManagementHelper'; +import { getRole, getWorkflowOwner, isSharingEnabled } from './UserManagementHelper'; +import { WorkflowsService } from '@/workflows/workflows.services'; +import { UserService } from '@/user/user.service'; export class PermissionChecker { /** @@ -31,7 +39,7 @@ export class PermissionChecker { let workflowUserIds = [userId]; - if (workflow.id && config.getEnv('enterprise.workflowSharingEnabled')) { + if (workflow.id && isSharingEnabled()) { const workflowSharings = await Db.collections.SharedWorkflow.find({ relations: ['workflow'], where: { workflow: { id: Number(workflow.id) } }, @@ -44,7 +52,7 @@ export class PermissionChecker { where: { user: In(workflowUserIds) }, }; - if (!config.getEnv('enterprise.features.sharing')) { + if (!isSharingEnabled()) { // If credential sharing is not enabled, get only credentials owned by this user credentialsWhereCondition.where.role = await getRole('credential', 'owner'); } @@ -68,6 +76,72 @@ export class PermissionChecker { }); } + static async checkSubworkflowExecutePolicy( + subworkflow: Workflow, + userId: string, + parentWorkflowId?: string, + ) { + /** + * Important considerations: both the current workflow and the parent can have empty IDs. + * This happens when a user is executing an unsaved workflow manually running a workflow + * loaded from a file or code, for instance. + * This is an important topic to keep in mind for all security checks + */ + if (!subworkflow.id) { + // It's a workflow from code and not loaded from DB + // No checks are necessary since it doesn't have any sort of settings + return; + } + + let policy = + subworkflow.settings?.callerPolicy ?? config.getEnv('workflows.callerPolicyDefaultOption'); + + if (!isSharingEnabled()) { + // Community version allows only same owner workflows + policy = 'workflowsFromSameOwner'; + } + + const subworkflowOwner = await getWorkflowOwner(subworkflow.id); + + const errorToThrow = new SubworkflowOperationError( + `Target workflow ID ${subworkflow.id ?? ''} may not be called`, + subworkflowOwner.id === userId + ? 'Change the settings of the sub-workflow so it can be called by this one.' + : `${subworkflowOwner.firstName} (${subworkflowOwner.email}) can make this change. You may need to tell them the ID of this workflow, which is ${subworkflow.id}`, + ); + + if (policy === 'none') { + throw errorToThrow; + } + + if (policy === 'workflowsFromAList') { + if (parentWorkflowId === undefined) { + throw errorToThrow; + } + const allowedCallerIds = (subworkflow.settings.callerIds as string | undefined) + ?.split(',') + .map((id) => id.trim()) + .filter((id) => id !== ''); + + if (!allowedCallerIds?.includes(parentWorkflowId)) { + throw errorToThrow; + } + } + + if (policy === 'workflowsFromSameOwner') { + const user = await UserService.get({ id: userId }); + if (!user) { + throw new WorkflowOperationError( + 'Fatal error: user not found. Please contact the system administrator.', + ); + } + const sharing = await WorkflowsService.getSharing(user, subworkflow.id, ['role', 'user']); + if (!sharing || sharing.role.name !== 'owner') { + throw errorToThrow; + } + } + } + private static mapCredIdsToNodes(workflow: Workflow) { return Object.values(workflow.nodes).reduce<{ [credentialId: string]: INode[] }>( (map, node) => { diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index c7f65003df..e45e97869c 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -15,10 +15,13 @@ import config from '@/config'; import { getWebhookBaseUrl } from '../WebhookHelpers'; import { getLicense } from '@/License'; import { WhereClause } from '@/Interfaces'; +import { RoleService } from '@/role/role.service'; export async function getWorkflowOwner(workflowId: string | number): Promise { + const workflowOwnerRole = await RoleService.get({ name: 'owner', scope: 'workflow' }); + const sharedWorkflow = await Db.collections.SharedWorkflow.findOneOrFail({ - where: { workflow: { id: workflowId } }, + where: { workflow: { id: workflowId }, role: workflowOwnerRole }, relations: ['user', 'user.globalRole'], }); diff --git a/packages/cli/src/UserManagement/routes/users.ts b/packages/cli/src/UserManagement/routes/users.ts index c8e56dd41b..4f828b5192 100644 --- a/packages/cli/src/UserManagement/routes/users.ts +++ b/packages/cli/src/UserManagement/routes/users.ts @@ -25,6 +25,7 @@ import { import config from '@/config'; import { issueCookie } from '../auth/jwt'; import { InternalHooksManager } from '@/InternalHooksManager'; +import { RoleService } from '@/role/role.service'; export function usersNamespace(this: N8nApp): void { /** @@ -403,33 +404,94 @@ export function usersNamespace(this: N8nApp): void { const userToDelete = users.find((user) => user.id === req.params.id) as User; + const telemetryData: ITelemetryUserDeletionData = { + user_id: req.user.id, + target_user_old_status: userToDelete.isPending ? 'invited' : 'active', + target_user_id: idToDelete, + }; + + telemetryData.migration_strategy = transferId ? 'transfer_data' : 'delete_data'; + + if (transferId) { + telemetryData.migration_user_id = transferId; + } + + const [workflowOwnerRole, credentialOwnerRole] = await Promise.all([ + RoleService.get({ name: 'owner', scope: 'workflow' }), + RoleService.get({ name: 'owner', scope: 'credential' }), + ]); + if (transferId) { const transferee = users.find((user) => user.id === transferId); + await Db.transaction(async (transactionManager) => { + // Get all workflow ids belonging to user to delete + const sharedWorkflows = await transactionManager.getRepository(SharedWorkflow).find({ + where: { user: userToDelete, role: workflowOwnerRole }, + }); + + const sharedWorkflowIds = sharedWorkflows.map((sharedWorkflow) => + sharedWorkflow.workflowId.toString(), + ); + + // Prevents issues with unique key constraints since user being assigned + // workflows and credentials might be a sharee + await transactionManager.delete(SharedWorkflow, { + user: transferee, + workflow: In(sharedWorkflowIds.map((sharedWorkflowId) => ({ id: sharedWorkflowId }))), + }); + + // Transfer ownership of owned workflows await transactionManager.update( SharedWorkflow, - { user: userToDelete }, + { user: userToDelete, role: workflowOwnerRole }, { user: transferee }, ); + + // Now do the same for creds + + // Get all workflow ids belonging to user to delete + const sharedCredentials = await transactionManager.getRepository(SharedCredentials).find({ + where: { user: userToDelete, role: credentialOwnerRole }, + }); + + const sharedCredentialIds = sharedCredentials.map((sharedCredential) => + sharedCredential.credentialId.toString(), + ); + + // Prevents issues with unique key constraints since user being assigned + // workflows and credentials might be a sharee + await transactionManager.delete(SharedCredentials, { + user: transferee, + credentials: In( + sharedCredentialIds.map((sharedCredentialId) => ({ id: sharedCredentialId })), + ), + }); + + // Transfer ownership of owned credentials await transactionManager.update( SharedCredentials, - { user: userToDelete }, + { user: userToDelete, role: credentialOwnerRole }, { user: transferee }, ); + + // This will remove all shared workflows and credentials not owned await transactionManager.delete(User, { id: userToDelete.id }); }); + void InternalHooksManager.getInstance().onUserDeletion(req.user.id, telemetryData, false); + await this.externalHooks.run('user.deleted', [sanitizeUser(userToDelete)]); return { success: true }; } const [ownedSharedWorkflows, ownedSharedCredentials] = await Promise.all([ Db.collections.SharedWorkflow.find({ relations: ['workflow'], - where: { user: userToDelete }, + where: { user: userToDelete, role: workflowOwnerRole }, }), Db.collections.SharedCredentials.find({ relations: ['credentials'], - where: { user: userToDelete }, + where: { user: userToDelete, role: credentialOwnerRole }, }), ]); @@ -450,22 +512,8 @@ export function usersNamespace(this: N8nApp): void { await transactionManager.delete(User, { id: userToDelete.id }); }); - const telemetryData: ITelemetryUserDeletionData = { - user_id: req.user.id, - target_user_old_status: userToDelete.isPending ? 'invited' : 'active', - target_user_id: idToDelete, - }; - - telemetryData.migration_strategy = transferId ? 'transfer_data' : 'delete_data'; - - if (transferId) { - telemetryData.migration_user_id = transferId; - } - void InternalHooksManager.getInstance().onUserDeletion(req.user.id, telemetryData, false); - await this.externalHooks.run('user.deleted', [sanitizeUser(userToDelete)]); - return { success: true }; }), ); diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index 5f2ac8b757..1732682f65 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -65,6 +65,7 @@ import * as WorkflowHelpers from '@/WorkflowHelpers'; import { getUserById, getWorkflowOwner, whereClause } from '@/UserManagement/UserManagementHelper'; import { findSubworkflowStart } from '@/utils'; import { PermissionChecker } from './UserManagement/PermissionChecker'; +import { WorkflowsService } from './workflows/workflows.services'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); @@ -779,34 +780,6 @@ export async function getRunData( ): Promise { const mode = 'integrated'; - const policy = - workflowData.settings?.callerPolicy ?? config.getEnv('workflows.callerPolicyDefaultOption'); - - if (policy === 'none') { - throw new SubworkflowOperationError( - `Target workflow ID ${workflowData.id} may not be called by other workflows.`, - 'Please update the settings of the target workflow or ask its owner to do so.', - ); - } - - if ( - policy === 'workflowsFromAList' && - typeof workflowData.settings?.callerIds === 'string' && - parentWorkflowId !== undefined - ) { - const allowedCallerIds = workflowData.settings.callerIds - .split(',') - .map((id) => id.trim()) - .filter((id) => id !== ''); - - if (!allowedCallerIds.includes(parentWorkflowId)) { - throw new SubworkflowOperationError( - `Target workflow ID ${workflowData.id} may only be called by a list of workflows, which does not include current workflow ID ${parentWorkflowId}.`, - 'Please update the settings of the target workflow or ask its owner to do so.', - ); - } - } - const startingNode = findSubworkflowStart(workflowData.nodes); // Always start with empty data if no inputData got supplied @@ -852,7 +825,6 @@ export async function getRunData( export async function getWorkflowData( workflowInfo: IExecuteWorkflowInfo, - userId: string, parentWorkflowId?: string, parentWorkflowSettings?: IWorkflowSettings, ): Promise { @@ -869,23 +841,15 @@ export async function getWorkflowData( // to get initialized first await Db.init(); } - const user = await getUserById(userId); - let relations = ['workflow', 'workflow.tags']; - if (config.getEnv('workflowTagsDisabled')) { - relations = relations.filter((relation) => relation !== 'workflow.tags'); - } + const relations = config.getEnv('workflowTagsDisabled') ? [] : ['tags']; - const shared = await Db.collections.SharedWorkflow.findOne({ - relations, - where: whereClause({ - user, - entityType: 'workflow', - entityId: workflowInfo.id, - }), - }); - - workflowData = shared?.workflow; + workflowData = await WorkflowsService.get( + { id: parseInt(workflowInfo.id, 10) }, + { + relations, + }, + ); if (workflowData === undefined) { throw new Error(`The workflow with the id "${workflowInfo.id}" does not exist.`); @@ -911,7 +875,7 @@ export async function getWorkflowData( async function executeWorkflow( workflowInfo: IExecuteWorkflowInfo, additionalData: IWorkflowExecuteAdditionalData, - options?: { + options: { parentWorkflowId?: string; inputData?: INodeExecutionData[]; parentExecutionId?: string; @@ -926,13 +890,8 @@ async function executeWorkflow( const nodeTypes = NodeTypes(); const workflowData = - options?.loadedWorkflowData ?? - (await getWorkflowData( - workflowInfo, - additionalData.userId, - options?.parentWorkflowId, - options?.parentWorkflowSettings, - )); + options.loadedWorkflowData ?? + (await getWorkflowData(workflowInfo, options.parentWorkflowId, options.parentWorkflowSettings)); const workflowName = workflowData ? workflowData.name : undefined; const workflow = new Workflow({ @@ -947,23 +906,28 @@ async function executeWorkflow( }); const runData = - options?.loadedRunData ?? - (await getRunData(workflowData, additionalData.userId, options?.inputData)); + options.loadedRunData ?? + (await getRunData(workflowData, additionalData.userId, options.inputData)); let executionId; - if (options?.parentExecutionId !== undefined) { - executionId = options?.parentExecutionId; + if (options.parentExecutionId !== undefined) { + executionId = options.parentExecutionId; } else { executionId = - options?.parentExecutionId !== undefined - ? options?.parentExecutionId + options.parentExecutionId !== undefined + ? options.parentExecutionId : await ActiveExecutions.getInstance().add(runData); } let data; try { await PermissionChecker.check(workflow, additionalData.userId); + await PermissionChecker.checkSubworkflowExecutePolicy( + workflow, + additionalData.userId, + options.parentWorkflowId, + ); // Create new additionalData to have different workflow loaded and to call // different webhooks @@ -1005,7 +969,7 @@ async function executeWorkflow( runData.executionMode, runExecutionData, ); - if (options?.parentExecutionId !== undefined) { + if (options.parentExecutionId !== undefined) { // Must be changed to become typed return { startedAt: new Date(), @@ -1049,6 +1013,7 @@ async function executeWorkflow( throw { ...error, stack: error.stack, + message: error.message, }; } diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 83bd0b0b7b..8c39b9ad0f 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -24,6 +24,7 @@ import config from '@/config'; import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { User } from '@db/entities/User'; import { getWorkflowOwner, whereClause } from '@/UserManagement/UserManagementHelper'; +import omit from 'lodash.omit'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); @@ -558,15 +559,16 @@ export function validateWorkflowCredentialUsage( nodesWithCredentialsUserDoesNotHaveAccessTo.forEach((node) => { if (isTamperingAttempt(node.id)) { - Logger.info('Blocked workflow update due to tampering attempt', { + Logger.verbose('Blocked workflow update due to tampering attempt', { nodeType: node.type, nodeName: node.name, nodeId: node.id, nodeCredentials: node.credentials, }); // Node is new, so this is probably a tampering attempt. Throw an error - throw new Error( - 'Workflow contains new nodes with credentials the user does not have access to', + throw new NodeOperationError( + node, + `You don't have access to the credentials in the '${node.name}' node. Ask the owner to share them with you.`, ); } // Replace the node with the previous version of the node @@ -580,9 +582,14 @@ export function validateWorkflowCredentialUsage( nodeName: node.name, nodeId: node.id, }); - newWorkflowVersion.nodes[nodeIdx] = previousWorkflowVersion.nodes.find( + const previousNodeVersion = previousWorkflowVersion.nodes.find( (previousNode) => previousNode.id === node.id, - )!; + ); + // Allow changing only name, position and disabled status for read-only nodes + Object.assign( + newWorkflowVersion.nodes[nodeIdx], + omit(previousNodeVersion, ['name', 'position', 'disabled']), + ); }); return newWorkflowVersion; diff --git a/packages/cli/src/WorkflowRunnerProcess.ts b/packages/cli/src/WorkflowRunnerProcess.ts index b53d91f866..69e05af83d 100644 --- a/packages/cli/src/WorkflowRunnerProcess.ts +++ b/packages/cli/src/WorkflowRunnerProcess.ts @@ -48,6 +48,7 @@ import { InternalHooksManager } from '@/InternalHooksManager'; import { generateFailedExecutionFromError } from '@/WorkflowHelpers'; import { initErrorHandling } from '@/ErrorReporting'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; +import { getLicense } from './License'; class WorkflowRunnerProcess { data: IWorkflowExecutionDataProcessWithExecution | undefined; @@ -118,48 +119,11 @@ class WorkflowRunnerProcess { const binaryDataConfig = config.getEnv('binaryDataManager'); await BinaryDataManager.init(binaryDataConfig); - // Credentials should now be loaded from database. - // We check if any node uses credentials. If it does, then - // init database. - let shouldInitializeDb = false; - // eslint-disable-next-line array-callback-return - inputData.workflowData.nodes.map((node) => { - if (Object.keys(node.credentials === undefined ? {} : node.credentials).length > 0) { - shouldInitializeDb = true; - } - if (node.type === 'n8n-nodes-base.executeWorkflow') { - // With UM, child workflows from arbitrary JSON - // Should be persisted by the child process, - // so DB needs to be initialized - shouldInitializeDb = true; - } - }); + // Init db since we need to read the license. + await Db.init(); - // This code has been split into 4 ifs just to make it easier to understand - // Can be made smaller but in the end it will make it impossible to read. - if (shouldInitializeDb) { - // initialize db as we need to load credentials - await Db.init(); - } else if ( - inputData.workflowData.settings !== undefined && - inputData.workflowData.settings.saveExecutionProgress === true - ) { - // Workflow settings specifying it should save - await Db.init(); - } else if ( - inputData.workflowData.settings !== undefined && - inputData.workflowData.settings.saveExecutionProgress !== false && - config.getEnv('executions.saveExecutionProgress') - ) { - // Workflow settings not saying anything about saving but default settings says so - await Db.init(); - } else if ( - inputData.workflowData.settings === undefined && - config.getEnv('executions.saveExecutionProgress') - ) { - // Workflow settings not saying anything about saving but default settings says so - await Db.init(); - } + const license = getLicense(); + await license.init(instanceId, cli); // Start timeout for the execution let workflowTimeout = config.getEnv('executions.timeout'); // initialize with default @@ -245,7 +209,6 @@ class WorkflowRunnerProcess { ): Promise | IRun> => { const workflowData = await WorkflowExecuteAdditionalData.getWorkflowData( workflowInfo, - userId, options?.parentWorkflowId, options?.parentWorkflowSettings, ); diff --git a/packages/cli/src/api/workflowStats.api.ts b/packages/cli/src/api/workflowStats.api.ts index fac958cc62..31d7425658 100644 --- a/packages/cli/src/api/workflowStats.api.ts +++ b/packages/cli/src/api/workflowStats.api.ts @@ -28,7 +28,7 @@ async function checkWorkflowId(workflowId: string, user: User): Promise }); if (!shared) { - LoggerProxy.info('User attempted to read a workflow without permissions', { + LoggerProxy.verbose('User attempted to read a workflow without permissions', { workflowId, userId: user.id, }); diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 43b9061267..e36b637831 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -27,7 +27,6 @@ const config = convict(schema); if (inE2ETests) { config.set('enterprise.features.sharing', true); - config.set('enterprise.workflowSharingEnabled', true); } config.getEnv = config.get; diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 114e60d307..c0d659f824 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -217,8 +217,8 @@ export const schema = { }, callerPolicyDefaultOption: { doc: 'Default option for which workflows may call the current workflow', - format: ['any', 'none', 'workflowsFromAList'] as const, - default: 'any', + format: ['any', 'none', 'workflowsFromAList', 'workflowsFromSameOwner'] as const, + default: 'workflowsFromSameOwner', env: 'N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION', }, }, @@ -885,12 +885,6 @@ export const schema = { default: false, }, }, - // This is a temporary flag (acting as feature toggle) - // Will be removed when feature goes live - workflowSharingEnabled: { - format: Boolean, - default: false, - }, }, hiringBanner: { diff --git a/packages/cli/src/executions/executions.controller.ee.ts b/packages/cli/src/executions/executions.controller.ee.ts index 200f6faf0b..c455b32286 100644 --- a/packages/cli/src/executions/executions.controller.ee.ts +++ b/packages/cli/src/executions/executions.controller.ee.ts @@ -1,5 +1,4 @@ import express from 'express'; -import config from '@/config'; import { IExecutionFlattedResponse, IExecutionResponse, @@ -14,7 +13,7 @@ import { EEExecutionsService } from './executions.service.ee'; export const EEExecutionsController = express.Router(); EEExecutionsController.use((req, res, next) => { - if (!isSharingEnabled() || !config.getEnv('enterprise.workflowSharingEnabled')) { + if (!isSharingEnabled()) { // skip ee router and use free one next('router'); return; diff --git a/packages/cli/src/role/role.service.ts b/packages/cli/src/role/role.service.ts index e8d2cf70c7..753e631d9d 100644 --- a/packages/cli/src/role/role.service.ts +++ b/packages/cli/src/role/role.service.ts @@ -10,4 +10,15 @@ export class RoleService { static async trxGet(transaction: EntityManager, role: Partial) { return transaction.findOne(Role, role); } + + static async getUserRoleForWorkflow(userId: string, workflowId: string) { + const shared = await Db.collections.SharedWorkflow.findOne({ + where: { + workflow: { id: workflowId }, + user: { id: userId }, + }, + relations: ['role'], + }); + return shared?.role; + } } diff --git a/packages/cli/src/user/user.service.ts b/packages/cli/src/user/user.service.ts index 20bb4d0055..86c5e853bd 100644 --- a/packages/cli/src/user/user.service.ts +++ b/packages/cli/src/user/user.service.ts @@ -4,7 +4,9 @@ import { User } from '@db/entities/User'; export class UserService { static async get(user: Partial): Promise { - return Db.collections.User.findOne(user); + return Db.collections.User.findOne(user, { + relations: ['globalRole'], + }); } static async getByIds(transaction: EntityManager, ids: string[]) { diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index 04986499d4..b5c3962692 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -22,7 +22,7 @@ import * as GenericHelpers from '@/GenericHelpers'; export const EEWorkflowController = express.Router(); EEWorkflowController.use((req, res, next) => { - if (!isSharingEnabled() || !config.getEnv('enterprise.workflowSharingEnabled')) { + if (!isSharingEnabled()) { // skip ee router and use free one next('router'); return; @@ -73,6 +73,12 @@ EEWorkflowController.put( await EEWorkflows.share(trx, workflow, newShareeIds); } }); + + void InternalHooksManager.getInstance().onWorkflowSharingUpdate( + workflowId, + req.user.id, + shareWithIds, + ); }), ); @@ -94,7 +100,7 @@ EEWorkflowController.get( if (!userSharing && req.user.globalRole.name !== 'owner') { throw new ResponseHelper.UnauthorizedError( - 'It looks like you cannot access this workflow. Ask the owner to share it with you.', + 'You do not have permission to access this workflow. Ask the owner to share it with you', ); } diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 91871746d6..03ee00b77e 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -213,7 +213,7 @@ workflowsController.get( }); if (!shared) { - LoggerProxy.info('User attempted to access a workflow without permissions', { + LoggerProxy.verbose('User attempted to access a workflow without permissions', { workflowId, userId: req.user.id, }); @@ -286,7 +286,7 @@ workflowsController.delete( }); if (!shared) { - LoggerProxy.info('User attempted to delete a workflow without permissions', { + LoggerProxy.verbose('User attempted to delete a workflow without permissions', { workflowId, userId: req.user.id, }); diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index c2ecc0d676..f13ddf5456 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -15,6 +15,7 @@ import type { } from './workflows.types'; import { EECredentialsService as EECredentials } from '@/credentials/credentials.service.ee'; import { getSharedWorkflowIds } from '@/WorkflowHelpers'; +import { NodeOperationError } from 'n8n-workflow'; export class EEWorkflowsService extends WorkflowsService { static async getWorkflowIdsForUser(user: User) { @@ -189,6 +190,9 @@ export class EEWorkflowsService extends WorkflowsService { allCredentials, ); } catch (error) { + if (error instanceof NodeOperationError) { + throw new ResponseHelper.BadRequestError(error.message); + } throw new ResponseHelper.BadRequestError( 'Invalid workflow credentials - make sure you have access to all credentials and try again.', ); diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 2c20aaaac9..bd9c3345d4 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -22,7 +22,7 @@ import { WorkflowRunner } from '@/WorkflowRunner'; import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData'; import * as TestWebhooks from '@/TestWebhooks'; import { getSharedWorkflowIds } from '@/WorkflowHelpers'; -import { whereClause } from '@/UserManagement/UserManagementHelper'; +import { isSharingEnabled, whereClause } from '@/UserManagement/UserManagementHelper'; export interface IGetWorkflowsQueryFilter { id?: number | string; @@ -158,20 +158,26 @@ export class WorkflowsService { return []; } - const fields: Array = ['id', 'name', 'active', 'createdAt', 'updatedAt']; + const fields: Array = [ + 'id', + 'name', + 'active', + 'createdAt', + 'updatedAt', + 'nodes', + ]; const relations: string[] = []; if (!config.getEnv('workflowTagsDisabled')) { relations.push('tags'); } - const isSharingEnabled = config.getEnv('enterprise.features.sharing'); - if (isSharingEnabled) { + if (isSharingEnabled()) { relations.push('shared', 'shared.user', 'shared.role'); } const query: FindManyOptions = { - select: isSharingEnabled ? [...fields, 'nodes', 'versionId'] : fields, + select: isSharingEnabled() ? [...fields, 'versionId'] : fields, relations, where: { id: In(sharedWorkflowIds), @@ -210,7 +216,7 @@ export class WorkflowsService { }); if (!shared) { - LoggerProxy.info('User attempted to update a workflow without permissions', { + LoggerProxy.verbose('User attempted to update a workflow without permissions', { workflowId, userId: user.id, }); @@ -351,7 +357,7 @@ export class WorkflowsService { updatedWorkflow.active = false; // Now return the original error for UI to display - throw error; + throw new ResponseHelper.BadRequestError((error as Error).message); } } diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index bdd9d6e235..13b2fadd51 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -161,13 +161,13 @@ test('DELETE /users/:id should delete the user', async () => { const sharedWorkflow = await Db.collections.SharedWorkflow.findOne({ relations: ['user'], - where: { user: userToDelete }, + where: { user: userToDelete, role: workflowOwnerRole }, }); expect(sharedWorkflow).toBeUndefined(); // deleted const sharedCredential = await Db.collections.SharedCredentials.findOne({ relations: ['user'], - where: { user: userToDelete }, + where: { user: userToDelete, role: credentialOwnerRole }, }); expect(sharedCredential).toBeUndefined(); // deleted diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index f928260b38..1aa3b6d954 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -47,8 +47,6 @@ beforeAll(async () => { isSharingEnabled = jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(true); - config.set('enterprise.workflowSharingEnabled', true); // @TODO: Remove once temp flag is removed - await utils.initNodeTypes(); workflowRunner = await utils.initActiveWorkflowRunner(); @@ -666,7 +664,7 @@ describe('PATCH /workflows/:id - validate credential permissions to user', () => expect(response.statusCode).toBe(400); }); - it('Should succeed but prevent modifying nodes that are read-only for the requester', async () => { + it('Should succeed but prevent modifying node attributes other than position, name and disabled', async () => { const member1 = await testDb.createUser({ globalRole: globalMemberRole }); const member2 = await testDb.createUser({ globalRole: globalMemberRole }); @@ -676,7 +674,9 @@ describe('PATCH /workflows/:id - validate credential permissions to user', () => { id: 'uuid-1234', name: 'Start', - parameters: {}, + parameters: { + firstParam: 123, + }, position: [-20, 260], type: 'n8n-nodes-base.start', typeVersion: 1, @@ -693,8 +693,10 @@ describe('PATCH /workflows/:id - validate credential permissions to user', () => { id: 'uuid-1234', name: 'End', - parameters: {}, - position: [-20, 260], + parameters: { + firstParam: 456, + }, + position: [-20, 555], type: 'n8n-nodes-base.no-op', typeVersion: 1, credentials: { @@ -703,6 +705,27 @@ describe('PATCH /workflows/:id - validate credential permissions to user', () => name: 'fake credential', }, }, + disabled: true, + }, + ]; + + const expectedNodes: INode[] = [ + { + id: 'uuid-1234', + name: 'End', + parameters: { + firstParam: 123, + }, + position: [-20, 555], + type: 'n8n-nodes-base.start', + typeVersion: 1, + credentials: { + default: { + id: savedCredential.id.toString(), + name: savedCredential.name, + }, + }, + disabled: true, }, ]; @@ -726,7 +749,7 @@ describe('PATCH /workflows/:id - validate credential permissions to user', () => }); expect(response.statusCode).toBe(200); - expect(response.body.data.nodes).toMatchObject(originalNodes); + expect(response.body.data.nodes).toMatchObject(expectedNodes); }); }); diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index 51eb5b6cca..be4beb427d 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -804,7 +804,7 @@ export interface IN8nUISettings { }; enterprise: Record; deployment?: { - type: string; + type: string | 'default' | 'n8n-internal' | 'cloud' | 'desktop_mac' | 'desktop_win'; }; hideUsagePage: boolean; license: { @@ -1079,10 +1079,6 @@ export interface IModalState { httpNodeParameters?: string; } -export interface NestedRecord { - [key: string]: T | NestedRecord; -} - export type IRunDataDisplayMode = 'table' | 'json' | 'binary' | 'schema'; export type NodePanelType = 'input' | 'output'; @@ -1155,7 +1151,6 @@ export interface UIState { currentView: string; mainPanelPosition: number; fakeDoorFeatures: IFakeDoor[]; - dynamicTranslations: NestedRecord; draggable: { isDragging: boolean; type: string; diff --git a/packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue b/packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue index a626d91a54..fedb9bfdc9 100644 --- a/packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue +++ b/packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue @@ -3,7 +3,14 @@ - +
- +
import Vue from 'vue'; -import { ICredentialsResponse, IFakeDoor, IUser } from '@/Interface'; +import type { ICredentialsResponse, IUser } from '@/Interface'; import { CredentialInformation, @@ -391,9 +387,6 @@ export default mixins(showMessage, nodeHelpers).extend({ } return true; }, - credentialsFakeDoorFeatures(): IFakeDoor[] { - return this.uiStore.getFakeDoorByLocation('credentialsModal'); - }, credentialPermissions(): IPermissions { if (this.loading) { return {}; @@ -405,7 +398,7 @@ export default mixins(showMessage, nodeHelpers).extend({ ); }, sidebarItems(): IMenuItem[] { - const items: IMenuItem[] = [ + return [ { id: 'connection', label: this.$locale.baseText('credentialEdit.credentialEdit.connection'), @@ -415,26 +408,13 @@ export default mixins(showMessage, nodeHelpers).extend({ id: 'sharing', label: this.$locale.baseText('credentialEdit.credentialEdit.sharing'), position: 'top', - available: this.credentialType !== null && this.isSharingAvailable, + }, + { + id: 'details', + label: this.$locale.baseText('credentialEdit.credentialEdit.details'), + position: 'top', }, ]; - - if (this.credentialType !== null && !this.isSharingAvailable) { - for (const item of this.credentialsFakeDoorFeatures) { - items.push({ - id: `coming-soon/${item.id}`, - label: this.$locale.baseText(item.featureName as BaseTextKey), - position: 'top', - }); - } - } - - items.push({ - id: 'details', - label: this.$locale.baseText('credentialEdit.credentialEdit.details'), - position: 'top', - }); - return items; }, isSharingAvailable(): boolean { return this.settingsStore.isEnterpriseFeatureEnabled(EnterpriseEditionFeature.Sharing); diff --git a/packages/editor-ui/src/components/CredentialEdit/CredentialSharing.ee.vue b/packages/editor-ui/src/components/CredentialEdit/CredentialSharing.ee.vue index 7370a68bbb..18142f8f94 100644 --- a/packages/editor-ui/src/components/CredentialEdit/CredentialSharing.ee.vue +++ b/packages/editor-ui/src/components/CredentialEdit/CredentialSharing.ee.vue @@ -1,7 +1,22 @@