mohiit1502/Automata
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor: Workflow sharing bug bash fixes (#4888)

Browse Source 
* fix: Prevent workflows with only manual trigger from being activated

* fix: Fix workflow id when sharing from workflows list

* fix: Update sharing modal translations

* fix: Allow sharees to disable workflows and fix issue with unique key when removing a user

* refactor: Improve error messages and change logging level to be less verbose

* fix: Broken user removal transfer issue

* feat: Implement workflow sharing BE telemetry

* chore: temporarily add sharing env vars

* feat: Implement BE telemetry for workflow sharing

* fix: Prevent issues with possibly missing workflow id

* feat: Replace WorkflowSharing flag references (no-changelog) (#4918)

* ci: Block all external network calls in tests (no-changelog) (#4930)

* setup nock to prevent tests from making any external requests

* mock all calls to posthog sdk

* feat: Replace WorkflowSharing flag references (no-changelog)

Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <netroy@users.noreply.github.com>

* refactor: Remove temporary feature flag for workflow sharing

* refactor: add sharing_role to both manual and node executions

* refactor: Allow changing name, position and disabled of read only nodes

* feat: Overhaul dynamic translations for local and cloud (#4943)

* feat: Overhaul dynamic translations for local and cloud

* fix: remove type casting

* chore: remove unused translations

* fix: fix workflow sharing translation

* test: Fix broken test

* refactor: remove unnecessary import

* refactor: Minor code improvements

* refactor: rename dynamicTranslations to contextBasedTranslationKeys

* fix: fix type imports

* refactor: Consolidate sharing feature check

* feat: update cred sharing unavailable translations

* feat: update upgrade message when user management not available

* fix: rename plan names to Pro and Power

* feat: update translations to no longer contain plan names

* wip: subworkflow permissions

* feat: add workflowsFromSameOwner caller policy

* feat: Fix subworkflow permissions

* shared entites should check for role when deleting users

* refactor: remove circular dependency

* role filter shouldn't be an array

* fixed role issue

* fix: Corrected behavior when removing users

* feat: show instance owner credential sharing message only if isnt sharee

* feat: update workflow caller policy caller ids labels

* feat: update upgrade plan links to contain instance ids

* fix: show check errors below creds message only to owner

* fix(editor): Hide usage page on cloud

* fix: update credential validation error message for sharee

* fix(core): Remove duplicate import

* fix(editor): Extending deployment types

* feat: Overhaul contextual translations (#4992)

feat: update how contextual translations work

* refactor: improve messageing for subworkflow permissions

* test: Fix issue with user deletion and transfer

* fix: Explicitly throw error message so it can be displayed in UI

Co-authored-by: Alex Grozav <alex@grozav.com>
Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <netroy@users.noreply.github.com>
Co-authored-by: freyamade <freya@n8n.io>
Co-authored-by: Csaba Tuncsik <csaba@n8n.io>
This commit is contained in:
Omar Ajoue
2022-12-21 16:42:07 +01:00
committed by GitHub
parent e225c3190e
commit 25e9f0817a
43 changed files with 597 additions and 344 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
packages/cli/src/UserManagement/PermissionChecker.ts
View File

@@ -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) => {

5
packages/cli/src/UserManagement/UserManagementHelper.ts
View File

@@ -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<User> {
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'],
});

84
packages/cli/src/UserManagement/routes/users.ts
View File

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