From 75a094a8c03afc40b7872cd2115d82e69455286e Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Mon, 20 Feb 2023 12:22:27 +0100 Subject: [PATCH 01/34] fix: Fixes an issue when saving an active workflow without triggers would cause n8n to be stuck (#5513) fix: Allow saving and editing when an active workflow is saved without triggers --- packages/cli/src/workflows/workflows.services.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index 601f34807d..1dd4c1b1c7 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -332,7 +332,11 @@ export class WorkflowsService { ); } catch (error) { // If workflow could not be activated set it again to inactive - await Db.collections.Workflow.update(workflowId, { active: false }); + // and revert the versionId change so UI remains consistent + await Db.collections.Workflow.update(workflowId, { + active: false, + versionId: shared.workflow.versionId, + }); // Also set it in the returned data updatedWorkflow.active = false; From 1c0966957d8aeaccb4ef0bd6c624ee0351442f92 Mon Sep 17 00:00:00 2001 From: OlegIvaniv Date: Mon, 20 Feb 2023 15:59:38 +0100 Subject: [PATCH 02/34] fix(editor): Fix adding of wrong actions when filtering in the node actions panel (no-changelog) (#5518) --- cypress/e2e/4-node-creator.cy.ts | 14 ++++++++++++++ packages/editor-ui/src/components/Node.vue | 9 +++++++-- .../src/components/Node/NodeCreator/MainPanel.vue | 7 ++++--- .../editor-ui/src/plugins/i18n/locales/en.json | 1 + 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/cypress/e2e/4-node-creator.cy.ts b/cypress/e2e/4-node-creator.cy.ts index 4f43684c93..4068b2c1fd 100644 --- a/cypress/e2e/4-node-creator.cy.ts +++ b/cypress/e2e/4-node-creator.cy.ts @@ -112,6 +112,20 @@ describe('Node Creator', () => { NDVModal.getters.parameterInput('operation').should('contain.text', 'Crop'); }) + it('should search through actions and confirm added action', () => { + nodeCreatorFeature.actions.openNodeCreator(); + nodeCreatorFeature.getters.searchBar().find('input').clear().type('ftp'); + nodeCreatorFeature.getters.searchBar().find('input').realPress('{rightarrow}'); + nodeCreatorFeature.getters.activeSubcategory().should('have.text', 'FTP'); + nodeCreatorFeature.getters.searchBar().find('input').clear().type('file'); + // Navigate to rename action which should be the 4th item + nodeCreatorFeature.getters.searchBar().find('input').realPress('{downarrow}'); + nodeCreatorFeature.getters.searchBar().find('input').realPress('{downarrow}'); + nodeCreatorFeature.getters.searchBar().find('input').realPress('{downarrow}'); + nodeCreatorFeature.getters.searchBar().find('input').realPress('{rightarrow}'); + NDVModal.getters.parameterInput('operation').should('contain.text', 'Rename'); + }) + it('should render and select community node', () => { cy.intercept('GET', '/types/nodes.json').as('nodesIntercept'); cy.wait('@nodesIntercept').then(() => { diff --git a/packages/editor-ui/src/components/Node.vue b/packages/editor-ui/src/components/Node.vue index 203563af6c..981dc9a38c 100644 --- a/packages/editor-ui/src/components/Node.vue +++ b/packages/editor-ui/src/components/Node.vue @@ -22,8 +22,13 @@ v-touch:start="touchStart" v-touch:end="touchEnd" > - - + + + + +
(isRoot.value ? activeView.value.items : const searchItems = computed(() => { return state.activeNodeActions - ? transformCreateElements(selectedNodeActions.value) + ? transformCreateElements(selectedNodeActions.value, 'action') : transformCreateElements(filteredMergedAppNodes.value); }); @@ -251,6 +251,7 @@ function sortActions(nodeCreateElements: INodeCreateElement[]): INodeCreateEleme function transformCreateElements( createElements: Array, + type: 'node' | 'action' = 'node', ): INodeCreateElement[] { const sorted = [...createElements]; @@ -265,7 +266,7 @@ function transformCreateElements( // if we have more cases like this we should add more robust logic const isN8nNode = nodeType.name.includes(N8N_NODE_TYPE); return { - type: 'node', + type, category: nodeType.codex?.categories, key: nodeType.name, properties: { @@ -274,7 +275,7 @@ function transformCreateElements( }, includedByTrigger: isN8nNode || nodeType.group.includes('trigger'), includedByRegular: isN8nNode || !nodeType.group.includes('trigger'), - }; + } as INodeCreateElement; }); } diff --git a/packages/editor-ui/src/plugins/i18n/locales/en.json b/packages/editor-ui/src/plugins/i18n/locales/en.json index 4b01c2260d..77f03c9c87 100644 --- a/packages/editor-ui/src/plugins/i18n/locales/en.json +++ b/packages/editor-ui/src/plugins/i18n/locales/en.json @@ -663,6 +663,7 @@ "ndv.pinData.error.tooLarge.description": "You can pin at most 12MB of output per workflow.", "noTagsView.readyToOrganizeYourWorkflows": "Ready to organize your workflows?", "noTagsView.withWorkflowTagsYouReFree": "With workflow tags, you're free to create the perfect tagging system for your flows", + "node.thisIsATriggerNode": "This is a Trigger node. Learn more", "node.activateDeactivateNode": "Activate/Deactivate Node", "node.deleteNode": "Delete Node", "node.disabled": "Disabled", From 11b467137e7652c03c0578654b19dbc157b23220 Mon Sep 17 00:00:00 2001 From: Jonathan Bennetts Date: Mon, 20 Feb 2023 15:03:30 +0000 Subject: [PATCH 03/34] feat: Deprecate Read Binary File node (#5490) --- packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts b/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts index 453b13d1b8..3ebc913835 100644 --- a/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts +++ b/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts @@ -8,6 +8,7 @@ export class ReadBinaryFile implements INodeType { icon: 'fa:file-import', group: ['input'], version: 1, + hidden: true, description: 'Reads a binary file from disk', defaults: { name: 'Read Binary File', From 1c476770a778b7d034924db847a8757c383bd281 Mon Sep 17 00:00:00 2001 From: Jonathan Bennetts Date: Mon, 20 Feb 2023 15:54:59 +0000 Subject: [PATCH 04/34] fix(S3 Node): Fix issue with get many buckets not outputting data (#5514) --- packages/nodes-base/nodes/S3/S3.node.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/nodes-base/nodes/S3/S3.node.ts b/packages/nodes-base/nodes/S3/S3.node.ts index 2314e239c1..6cab88c6be 100644 --- a/packages/nodes-base/nodes/S3/S3.node.ts +++ b/packages/nodes-base/nodes/S3/S3.node.ts @@ -185,7 +185,11 @@ export class S3 implements INodeType { ); responseData = responseData.slice(0, qs.limit); } - returnData.push.apply(returnData, responseData); + const executionData = this.helpers.constructExecutionMetaData( + this.helpers.returnJsonArray(responseData), + { itemData: { item: i } }, + ); + returnData.push(...executionData); } //https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html From ee21b7a1cfed17936eb6bf50221b7f9983dd38e6 Mon Sep 17 00:00:00 2001 From: Jonathan Bennetts Date: Mon, 20 Feb 2023 16:04:43 +0000 Subject: [PATCH 05/34] fix(Baserow Node): Fix issue with get all not correctly using filters (#5519) --- packages/nodes-base/nodes/Baserow/Baserow.node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nodes-base/nodes/Baserow/Baserow.node.ts b/packages/nodes-base/nodes/Baserow/Baserow.node.ts index 1a3ad1f4ce..5b7fa2eb46 100644 --- a/packages/nodes-base/nodes/Baserow/Baserow.node.ts +++ b/packages/nodes-base/nodes/Baserow/Baserow.node.ts @@ -177,7 +177,7 @@ export class Baserow implements INodeType { const { order, filters, filterType, search } = this.getNodeParameter( 'additionalOptions', - 0, + i, ) as GetAllAdditionalOptions; const qs: IDataObject = {}; From 26a20ed47e8f580504b80150d7550ecb9a49be0d Mon Sep 17 00:00:00 2001 From: Mutasem Aldmour <4711238+mutdmour@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:35:35 +0300 Subject: [PATCH 06/34] feat: Support feature flag evaluation server side (#5511) * feat(editor): roll out schema view * feat(editor): add posthog tracking * refactor: use composables * refactor: clean up console log * refactor: clean up impl * chore: clean up impl * fix: fix demo var * chore: add comment * refactor: clean up * chore: wrap error func * refactor: clean up import * refactor: make store * feat: enable rudderstack usebeacon, move event to unload * chore: clean up alert * refactor: move tracking from hooks * fix: reload flags on login * fix: add func to setup * fix: clear duplicate import * chore: add console to tesT * chore: add console to tesT * fix: try reload * chore: randomize instnace id for testing * chore: randomize instnace id for testing * chore: add console logs for testing * chore: move random id to fe * chore: use query param for testing * feat: update PostHog api endpoint * feat: update rs host * feat: update rs host * feat: update rs endpoints * refactor: use api host for BE events as well * refactor: refactor out posthog client * feat: add feature flags to login * feat: add feature flags to login * feat: get feature flags to work * feat: add created at to be events * chore: add todos * chore: clean up store * chore: add created at to identify * feat: add posthog config to settings * feat: add bootstrapping * chore: clean up * chore: fix build * fix: get dates to work * fix: get posthog to recognize dates * chore: refactor * fix: update back to number * fix: update key * fix: get experiment evals to work * feat: add posthog to signup router * feat: add feature flags on sign up * chore: clean up * fix: fix import * chore: clean up loading script * feat: add timeout, fix: script loader * fix: test timeout and get working on 8080 * refactor: move out posthog * feat: add experiment tracking * fix: clear tracked on reset * fix: fix signup bug * fix: handle errors when telmetry is disabled * refactor: remove redundant await * fix: add back posthog to telemetry * test: fix test * test: fix test * test: add tests for posthog client * lint: fix * fix: fix issue with slow decide endpoint * lint: fix * lint: fix * lint: fix * lint: fix * chore: address PR feedback * chore: address PR feedback * feat: add onboarding experiment --- packages/cli/src/Interfaces.ts | 13 ++ packages/cli/src/InternalHooksManager.ts | 9 +- packages/cli/src/Server.ts | 19 ++- .../UserManagement/UserManagementHelper.ts | 28 +++- packages/cli/src/WorkflowRunnerProcess.ts | 7 +- packages/cli/src/commands/BaseCommand.ts | 7 +- packages/cli/src/commands/start.ts | 15 --- .../cli/src/controllers/auth.controller.ts | 23 +++- .../cli/src/controllers/users.controller.ts | 9 +- packages/cli/src/posthog/index.ts | 57 ++++++++ packages/cli/src/telemetry/index.ts | 36 +---- packages/cli/src/telemetry/scripts.ts | 17 --- packages/cli/test/integration/shared/utils.ts | 6 +- packages/cli/test/unit/PostHog.test.ts | 90 +++++++++++++ packages/cli/test/unit/Telemetry.test.ts | 8 +- packages/editor-ui/index.html | 2 + packages/editor-ui/src/App.vue | 17 +-- packages/editor-ui/src/Interface.ts | 46 ++++++- packages/editor-ui/src/api/users.ts | 7 +- .../editor-ui/src/components/Telemetry.vue | 9 -- packages/editor-ui/src/constants.ts | 16 ++- .../editor-ui/src/plugins/telemetry/index.ts | 2 + .../src/plugins/telemetry/telemetry.types.ts | 4 - packages/editor-ui/src/stores/posthog.ts | 124 ++++++++++++++++++ packages/editor-ui/src/stores/telemetry.ts | 21 +++ packages/editor-ui/src/stores/users.ts | 24 +++- packages/editor-ui/src/views/NodeView.vue | 7 +- .../editor-ui/src/views/WorkflowsView.vue | 8 +- packages/workflow/src/Interfaces.ts | 4 + 29 files changed, 513 insertions(+), 122 deletions(-) create mode 100644 packages/cli/src/posthog/index.ts delete mode 100644 packages/cli/src/telemetry/scripts.ts create mode 100644 packages/cli/test/unit/PostHog.test.ts create mode 100644 packages/editor-ui/src/stores/posthog.ts create mode 100644 packages/editor-ui/src/stores/telemetry.ts diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index 8ec9b0ccbd..7910ef38e2 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -22,6 +22,7 @@ import type { WorkflowExecuteMode, ExecutionStatus, IExecutionsSummary, + FeatureFlags, } from 'n8n-workflow'; import type { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; @@ -475,6 +476,14 @@ export interface IN8nUISettings { versionNotifications: IVersionNotificationSettings; instanceId: string; telemetry: ITelemetrySettings; + posthog: { + enabled: boolean; + apiHost: string; + apiKey: string; + autocapture: boolean; + disableSessionRecording: boolean; + debug: boolean; + }; personalizationSurveyEnabled: boolean; defaultLocale: string; userManagement: IUserManagementSettings; @@ -836,6 +845,10 @@ export interface PublicUser { inviteAcceptUrl?: string; } +export interface CurrentUser extends PublicUser { + featureFlags?: FeatureFlags; +} + export interface N8nApp { app: Application; restEndpoint: string; diff --git a/packages/cli/src/InternalHooksManager.ts b/packages/cli/src/InternalHooksManager.ts index a30a12c65e..e78564a3d7 100644 --- a/packages/cli/src/InternalHooksManager.ts +++ b/packages/cli/src/InternalHooksManager.ts @@ -1,6 +1,7 @@ import type { INodeTypes } from 'n8n-workflow'; import { InternalHooksClass } from '@/InternalHooks'; import { Telemetry } from '@/telemetry'; +import type { PostHogClient } from './posthog'; export class InternalHooksManager { private static internalHooksInstance: InternalHooksClass; @@ -13,9 +14,13 @@ export class InternalHooksManager { throw new Error('InternalHooks not initialized'); } - static async init(instanceId: string, nodeTypes: INodeTypes): Promise { + static async init( + instanceId: string, + nodeTypes: INodeTypes, + postHog: PostHogClient, + ): Promise { if (!this.internalHooksInstance) { - const telemetry = new Telemetry(instanceId); + const telemetry = new Telemetry(instanceId, postHog); await telemetry.init(); this.internalHooksInstance = new InternalHooksClass(telemetry, instanceId, nodeTypes); } diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 87e9b7463e..8d2b740a20 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -145,6 +145,7 @@ import { AbstractServer } from './AbstractServer'; import { configureMetrics } from './metrics'; import { setupBasicAuth } from './middlewares/basicAuth'; import { setupExternalJWTAuth } from './middlewares/externalJWTAuth'; +import { PostHogClient } from './posthog'; import { eventBus } from './eventbus'; import { isSamlEnabled } from './Saml/helpers'; @@ -167,6 +168,8 @@ class Server extends AbstractServer { credentialTypes: ICredentialTypes; + postHog: PostHogClient; + push: Push; constructor() { @@ -178,6 +181,7 @@ class Server extends AbstractServer { this.activeExecutionsInstance = ActiveExecutions.getInstance(); this.waitTracker = WaitTracker(); + this.postHog = new PostHogClient(); this.presetCredentialsLoaded = false; this.endpointPresetCredentials = config.getEnv('credentials.overwrite.endpoint'); @@ -232,6 +236,16 @@ class Server extends AbstractServer { }, instanceId: '', telemetry: telemetrySettings, + posthog: { + enabled: config.getEnv('diagnostics.enabled'), + apiHost: config.getEnv('diagnostics.config.posthog.apiHost'), + apiKey: config.getEnv('diagnostics.config.posthog.apiKey'), + autocapture: false, + disableSessionRecording: config.getEnv( + 'diagnostics.config.posthog.disableSessionRecording', + ), + debug: config.getEnv('logs.level') === 'debug', + }, personalizationSurveyEnabled: config.getEnv('personalization.enabled') && config.getEnv('diagnostics.enabled'), defaultLocale: config.getEnv('defaultLocale'), @@ -345,9 +359,10 @@ class Server extends AbstractServer { const logger = LoggerProxy; const internalHooks = InternalHooksManager.getInstance(); const mailer = getMailerInstance(); + const postHog = this.postHog; const controllers = [ - new AuthController({ config, internalHooks, repositories, logger }), + new AuthController({ config, internalHooks, repositories, logger, postHog }), new OwnerController({ config, internalHooks, repositories, logger }), new MeController({ externalHooks, internalHooks, repositories, logger }), new PasswordResetController({ config, externalHooks, internalHooks, repositories, logger }), @@ -359,6 +374,7 @@ class Server extends AbstractServer { repositories, activeWorkflowRunner, logger, + postHog, }), ]; controllers.forEach((controller) => registerController(app, config, controller)); @@ -378,6 +394,7 @@ class Server extends AbstractServer { await this.externalHooks.run('frontend.settings', [this.frontendSettings]); await this.initLicense(); + await this.postHog.init(this.frontendSettings.instanceId); const publicApiEndpoint = config.getEnv('publicApi.path'); const excludeEndpoints = config.getEnv('security.excludeEndpoints'); diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index a38a6ed310..bd1c2d3881 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -6,7 +6,7 @@ import { compare, genSaltSync, hash } from 'bcryptjs'; import * as Db from '@/Db'; import * as ResponseHelper from '@/ResponseHelper'; -import type { PublicUser, WhereClause } from '@/Interfaces'; +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'; @@ -15,6 +15,7 @@ import config from '@/config'; import { getWebhookBaseUrl } from '@/WebhookHelpers'; import { getLicense } from '@/License'; import { RoleService } from '@/role/role.service'; +import type { PostHogClient } from '@/posthog'; export async function getWorkflowOwner(workflowId: string): Promise { const workflowOwnerRole = await RoleService.get({ name: 'owner', scope: 'workflow' }); @@ -162,6 +163,31 @@ export function sanitizeUser(user: User, withoutKeys?: string[]): PublicUser { return sanitizedUser; } +export async function withFeatureFlags( + postHog: PostHogClient | undefined, + user: CurrentUser, +): Promise { + if (!postHog) { + return user; + } + + // native PostHog implementation has default 10s timeout and 3 retries.. which cannot be updated without affecting other functionality + // https://github.com/PostHog/posthog-js-lite/blob/a182de80a433fb0ffa6859c10fb28084d0f825c2/posthog-core/src/index.ts#L67 + const timeoutPromise = new Promise((resolve) => { + setTimeout(() => { + resolve(user); + }, 1500); + }); + + const fetchPromise = new Promise(async (resolve) => { + user.featureFlags = await postHog.getFeatureFlags(user); + + resolve(user); + }); + + return Promise.race([fetchPromise, timeoutPromise]); +} + export function addInviteLinkToUser(user: PublicUser, inviterId: string): PublicUser { if (user.isPending) { user.inviteAcceptUrl = generateUserInviteUrl(inviterId, user.id); diff --git a/packages/cli/src/WorkflowRunnerProcess.ts b/packages/cli/src/WorkflowRunnerProcess.ts index b8fd32f773..fe9a5eb64a 100644 --- a/packages/cli/src/WorkflowRunnerProcess.ts +++ b/packages/cli/src/WorkflowRunnerProcess.ts @@ -54,6 +54,7 @@ import { generateFailedExecutionFromError } from '@/WorkflowHelpers'; import { initErrorHandling } from '@/ErrorReporting'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; import { getLicense } from './License'; +import { PostHogClient } from './posthog'; class WorkflowRunnerProcess { data: IWorkflowExecutionDataProcessWithExecution | undefined; @@ -115,7 +116,11 @@ class WorkflowRunnerProcess { await externalHooks.init(); const instanceId = userSettings.instanceId ?? ''; - await InternalHooksManager.init(instanceId, nodeTypes); + + const postHog = new PostHogClient(); + await postHog.init(instanceId); + + await InternalHooksManager.init(instanceId, nodeTypes, postHog); const binaryDataConfig = config.getEnv('binaryDataManager'); await BinaryDataManager.init(binaryDataConfig); diff --git a/packages/cli/src/commands/BaseCommand.ts b/packages/cli/src/commands/BaseCommand.ts index 65b6dd27fc..13037cbf6c 100644 --- a/packages/cli/src/commands/BaseCommand.ts +++ b/packages/cli/src/commands/BaseCommand.ts @@ -18,6 +18,7 @@ import { NodeTypes } from '@/NodeTypes'; import type { LoadNodesAndCredentialsClass } from '@/LoadNodesAndCredentials'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import type { IExternalHooksClass } from '@/Interfaces'; +import { PostHogClient } from '@/posthog'; export const UM_FIX_INSTRUCTION = 'Please fix the database by running ./packages/cli/bin/n8n user-management:reset'; @@ -48,7 +49,11 @@ export abstract class BaseCommand extends Command { const credentialTypes = CredentialTypes(this.loadNodesAndCredentials); CredentialsOverwrites(credentialTypes); - await InternalHooksManager.init(this.userSettings.instanceId ?? '', this.nodeTypes); + const instanceId = this.userSettings.instanceId ?? ''; + const postHog = new PostHogClient(); + await postHog.init(instanceId); + + await InternalHooksManager.init(instanceId, this.nodeTypes, postHog); await Db.init().catch(async (error: Error) => this.exitWithCrash('There was an error initializing DB', error), diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index c3761f02e7..0c0c37b537 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -26,7 +26,6 @@ import * as TestWebhooks from '@/TestWebhooks'; import { WaitTracker } from '@/WaitTracker'; import { getAllInstalledPackages } from '@/CommunityNodes/packageModel'; import { handleLdapInit } from '@/Ldap/helpers'; -import { createPostHogLoadingScript } from '@/telemetry/scripts'; import { EDITOR_UI_DIST_DIR, GENERATED_STATIC_DIR } from '@/constants'; import { eventBus } from '@/eventbus'; import { BaseCommand } from './BaseCommand'; @@ -154,20 +153,6 @@ export class Start extends BaseCommand { }, ''); } - if (config.getEnv('diagnostics.enabled')) { - const phLoadingScript = createPostHogLoadingScript({ - apiKey: config.getEnv('diagnostics.config.posthog.apiKey'), - apiHost: config.getEnv('diagnostics.config.posthog.apiHost'), - autocapture: false, - disableSessionRecording: config.getEnv( - 'diagnostics.config.posthog.disableSessionRecording', - ), - debug: config.getEnv('logs.level') === 'debug', - }); - - scriptsString += phLoadingScript; - } - const closingTitleTag = ''; const compileFile = async (fileName: string) => { const filePath = path.join(EDITOR_UI_DIST_DIR, fileName); diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index 05edd7f663..0db2cbee35 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -1,7 +1,7 @@ import validator from 'validator'; import { Get, Post, RestController } from '@/decorators'; import { AuthError, BadRequestError, InternalServerError } from '@/ResponseHelper'; -import { sanitizeUser } from '@/UserManagement/UserManagementHelper'; +import { sanitizeUser, withFeatureFlags } from '@/UserManagement/UserManagementHelper'; import { issueCookie, resolveJwt } from '@/auth/jwt'; import { AUTH_COOKIE_NAME } from '@/constants'; import { Request, Response } from 'express'; @@ -11,8 +11,14 @@ import { LoginRequest, UserRequest } from '@/requests'; import type { Repository } from 'typeorm'; import { In } from 'typeorm'; import type { Config } from '@/config'; -import type { PublicUser, IDatabaseCollections, IInternalHooksClass } from '@/Interfaces'; +import type { + PublicUser, + IDatabaseCollections, + IInternalHooksClass, + CurrentUser, +} from '@/Interfaces'; import { handleEmailLogin, handleLdapLogin } from '@/auth'; +import type { PostHogClient } from '@/posthog'; @RestController() export class AuthController { @@ -24,21 +30,26 @@ export class AuthController { private readonly userRepository: Repository; + private readonly postHog?: PostHogClient; + constructor({ config, logger, internalHooks, repositories, + postHog, }: { config: Config; logger: ILogger; internalHooks: IInternalHooksClass; repositories: Pick; + postHog?: PostHogClient; }) { this.config = config; this.logger = logger; this.internalHooks = internalHooks; this.userRepository = repositories.User; + this.postHog = postHog; } /** @@ -56,7 +67,7 @@ export class AuthController { if (user) { await issueCookie(res, user); - return sanitizeUser(user); + return withFeatureFlags(this.postHog, sanitizeUser(user)); } throw new AuthError('Wrong username or password. Do you have caps lock on?'); @@ -66,7 +77,7 @@ export class AuthController { * Manually check the `n8n-auth` cookie. */ @Get('/login') - async currentUser(req: Request, res: Response): Promise { + async currentUser(req: Request, res: Response): Promise { // Manually check the existing cookie. // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const cookieContents = req.cookies?.[AUTH_COOKIE_NAME] as string | undefined; @@ -76,7 +87,7 @@ export class AuthController { // If logged in, return user try { user = await resolveJwt(cookieContents); - return sanitizeUser(user); + return await withFeatureFlags(this.postHog, sanitizeUser(user)); } catch (error) { res.clearCookie(AUTH_COOKIE_NAME); } @@ -102,7 +113,7 @@ export class AuthController { } await issueCookie(res, user); - return sanitizeUser(user); + return withFeatureFlags(this.postHog, sanitizeUser(user)); } /** diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index a71682ffb4..ddb8a5c6cc 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -16,6 +16,7 @@ import { isUserManagementEnabled, sanitizeUser, validatePassword, + withFeatureFlags, } from '@/UserManagement/UserManagementHelper'; import { issueCookie } from '@/auth/jwt'; import { BadRequestError, InternalServerError, NotFoundError } from '@/ResponseHelper'; @@ -33,6 +34,7 @@ import type { } from '@/Interfaces'; import type { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; import { AuthIdentity } from '@db/entities/AuthIdentity'; +import type { PostHogClient } from '@/posthog'; @RestController('/users') export class UsersController { @@ -56,6 +58,8 @@ export class UsersController { private mailer: UserManagementMailer; + private postHog?: PostHogClient; + constructor({ config, logger, @@ -64,6 +68,7 @@ export class UsersController { repositories, activeWorkflowRunner, mailer, + postHog, }: { config: Config; logger: ILogger; @@ -75,6 +80,7 @@ export class UsersController { >; activeWorkflowRunner: ActiveWorkflowRunner; mailer: UserManagementMailer; + postHog?: PostHogClient; }) { this.config = config; this.logger = logger; @@ -86,6 +92,7 @@ export class UsersController { this.sharedWorkflowRepository = repositories.SharedWorkflow; this.activeWorkflowRunner = activeWorkflowRunner; this.mailer = mailer; + this.postHog = postHog; } /** @@ -327,7 +334,7 @@ export class UsersController { await this.externalHooks.run('user.profile.update', [invitee.email, sanitizeUser(invitee)]); await this.externalHooks.run('user.password.update', [invitee.email, invitee.password]); - return sanitizeUser(updatedUser); + return withFeatureFlags(this.postHog, sanitizeUser(updatedUser)); } @Get('/') diff --git a/packages/cli/src/posthog/index.ts b/packages/cli/src/posthog/index.ts new file mode 100644 index 0000000000..a84173be24 --- /dev/null +++ b/packages/cli/src/posthog/index.ts @@ -0,0 +1,57 @@ +import type { PostHog } from 'posthog-node'; +import type { FeatureFlags, ITelemetryTrackProperties } from 'n8n-workflow'; +import config from '@/config'; +import type { PublicUser } from '..'; + +export class PostHogClient { + private postHog?: PostHog; + + private instanceId?: string; + + async init(instanceId: string) { + this.instanceId = instanceId; + const enabled = config.getEnv('diagnostics.enabled'); + if (!enabled) { + return; + } + + // eslint-disable-next-line @typescript-eslint/naming-convention + const { PostHog } = await import('posthog-node'); + this.postHog = new PostHog(config.getEnv('diagnostics.config.posthog.apiKey'), { + host: config.getEnv('diagnostics.config.posthog.apiHost'), + }); + + const logLevel = config.getEnv('logs.level'); + if (logLevel === 'debug') { + this.postHog.debug(true); + } + } + + async stop(): Promise { + if (this.postHog) { + return this.postHog.shutdown(); + } + } + + track(payload: { userId: string; event: string; properties: ITelemetryTrackProperties }): void { + this.postHog?.capture({ + distinctId: payload.userId, + sendFeatureFlags: true, + ...payload, + }); + } + + async getFeatureFlags(user: Pick): Promise { + if (!this.postHog) return Promise.resolve({}); + + const fullId = [this.instanceId, user.id].join('#'); + + // cannot use local evaluation because that requires PostHog personal api key with org-wide + // https://github.com/PostHog/posthog/issues/4849 + return this.postHog.getAllFlags(fullId, { + personProperties: { + created_at_timestamp: user.createdAt.getTime().toString(), + }, + }); + } +} diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index 66376b67f8..981798474d 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/packages/cli/src/telemetry/index.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-unsafe-call */ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ import type RudderStack from '@rudderstack/rudder-sdk-node'; -import type { PostHog } from 'posthog-node'; +import type { PostHogClient } from '../posthog'; import type { ITelemetryTrackProperties } from 'n8n-workflow'; import { LoggerProxy } from 'n8n-workflow'; import config from '@/config'; @@ -31,13 +31,11 @@ interface IExecutionsBuffer { export class Telemetry { private rudderStack?: RudderStack; - private postHog?: PostHog; - private pulseIntervalReference: NodeJS.Timeout; private executionCountsBuffer: IExecutionsBuffer = {}; - constructor(private instanceId: string) {} + constructor(private instanceId: string, private postHog: PostHogClient) {} async init() { const enabled = config.getEnv('diagnostics.enabled'); @@ -58,12 +56,6 @@ export class Telemetry { const { default: RudderStack } = await import('@rudderstack/rudder-sdk-node'); this.rudderStack = new RudderStack(key, url, { logLevel }); - // eslint-disable-next-line @typescript-eslint/naming-convention - const { PostHog } = await import('posthog-node'); - this.postHog = new PostHog(config.getEnv('diagnostics.config.posthog.apiKey'), { - host: config.getEnv('diagnostics.config.posthog.apiHost'), - }); - this.startPulse(); } } @@ -137,10 +129,8 @@ export class Telemetry { async trackN8nStop(): Promise { clearInterval(this.pulseIntervalReference); void this.track('User instance stopped'); - return new Promise((resolve) => { - if (this.postHog) { - this.postHog.shutdown(); - } + return new Promise(async (resolve) => { + await this.postHog.stop(); if (this.rudderStack) { this.rudderStack.flush(resolve); @@ -192,11 +182,7 @@ export class Telemetry { }; if (withPostHog) { - this.postHog?.capture({ - distinctId: payload.userId, - sendFeatureFlags: true, - ...payload, - }); + this.postHog?.track(payload); } return this.rudderStack.track(payload, resolve); @@ -206,19 +192,7 @@ export class Telemetry { }); } - async isFeatureFlagEnabled( - featureFlagName: string, - { user_id: userId }: ITelemetryTrackProperties = {}, - ): Promise { - if (!this.postHog) return Promise.resolve(false); - - const fullId = [this.instanceId, userId].join('#'); - - return this.postHog.isFeatureEnabled(featureFlagName, fullId); - } - // test helpers - getCountsBuffer(): IExecutionsBuffer { return this.executionCountsBuffer; } diff --git a/packages/cli/src/telemetry/scripts.ts b/packages/cli/src/telemetry/scripts.ts deleted file mode 100644 index 04c014ead0..0000000000 --- a/packages/cli/src/telemetry/scripts.ts +++ /dev/null @@ -1,17 +0,0 @@ -/** - * Create a script to init PostHog, for embedding before the Vue bundle in `` in `index.html`. - */ -export const createPostHogLoadingScript = ({ - apiKey, - apiHost, - autocapture, - disableSessionRecording, - debug, -}: { - apiKey: string; - apiHost: string; - autocapture: boolean; - disableSessionRecording: boolean; - debug: boolean; -}) => - ``; diff --git a/packages/cli/test/integration/shared/utils.ts b/packages/cli/test/integration/shared/utils.ts index adbd5986cb..3407d1b82f 100644 --- a/packages/cli/test/integration/shared/utils.ts +++ b/packages/cli/test/integration/shared/utils.ts @@ -74,6 +74,7 @@ import * as testDb from '../shared/testDb'; import { v4 as uuid } from 'uuid'; import { handleLdapInit } from '@/Ldap/helpers'; import { ldapController } from '@/Ldap/routes/ldap.controller.ee'; +import { PostHogClient } from '@/posthog'; const loadNodesAndCredentials: INodesAndCredentials = { loaded: { nodes: {}, credentials: {} }, @@ -107,8 +108,11 @@ export async function initTestServer({ const logger = getLogger(); LoggerProxy.init(logger); + const postHog = new PostHogClient(); + postHog.init('test-instance-id'); + // Pre-requisite: Mock the telemetry module before calling. - await InternalHooksManager.init('test-instance-id', mockNodeTypes); + await InternalHooksManager.init('test-instance-id', mockNodeTypes, postHog); testServer.app.use(bodyParser.json()); testServer.app.use(bodyParser.urlencoded({ extended: true })); diff --git a/packages/cli/test/unit/PostHog.test.ts b/packages/cli/test/unit/PostHog.test.ts new file mode 100644 index 0000000000..47e736014e --- /dev/null +++ b/packages/cli/test/unit/PostHog.test.ts @@ -0,0 +1,90 @@ +import { PostHog } from 'posthog-node'; +import { PostHogClient } from '@/posthog'; +import config from '@/config'; + +jest.mock('posthog-node'); + +describe('PostHog', () => { + const instanceId = 'test-id'; + const userId = 'distinct-id'; + const apiKey = 'api-key'; + const apiHost = 'api-host'; + + beforeAll(() => { + config.set('diagnostics.config.posthog.apiKey', apiKey); + config.set('diagnostics.config.posthog.apiHost', apiHost); + }); + + beforeEach(() => { + config.set('diagnostics.enabled', true); + jest.resetAllMocks(); + }); + + it('inits PostHog correctly', async () => { + const ph = new PostHogClient(); + await ph.init(instanceId); + + expect(PostHog.prototype.constructor).toHaveBeenCalledWith(apiKey, {host: apiHost}); + }); + + it('does not initialize or track if diagnostics are not enabled', async () => { + config.set('diagnostics.enabled', false); + + const ph = new PostHogClient(); + await ph.init(instanceId); + + ph.track({ + userId: 'test', + event: 'test', + properties: {}, + }); + + expect(PostHog.prototype.constructor).not.toHaveBeenCalled(); + expect(PostHog.prototype.capture).not.toHaveBeenCalled(); + }); + + it('captures PostHog events', async () => { + const event = 'test event'; + const properties = { + user_id: 'test', + test: true, + }; + + const ph = new PostHogClient(); + await ph.init(instanceId); + + ph.track({ + userId, + event, + properties, + }); + + expect(PostHog.prototype.capture).toHaveBeenCalledWith({ + distinctId: userId, + event, + userId, + properties, + sendFeatureFlags: true, + }); + }); + + it('gets feature flags', async () => { + const createdAt = new Date(); + const ph = new PostHogClient(); + await ph.init(instanceId); + + ph.getFeatureFlags({ + id: userId, + createdAt, + }); + + expect(PostHog.prototype.getAllFlags).toHaveBeenCalledWith( + `${instanceId}#${userId}`, + { + personProperties: { + created_at_timestamp: createdAt.getTime().toString(), + }, + } + ); + }); +}); \ No newline at end of file diff --git a/packages/cli/test/unit/Telemetry.test.ts b/packages/cli/test/unit/Telemetry.test.ts index 8c671eff08..67f4fec532 100644 --- a/packages/cli/test/unit/Telemetry.test.ts +++ b/packages/cli/test/unit/Telemetry.test.ts @@ -1,6 +1,7 @@ import { Telemetry } from '@/telemetry'; import config from '@/config'; import { flushPromises } from './Helpers'; +import { PostHogClient } from '@/posthog'; jest.unmock('@/telemetry'); jest.mock('@/license/License.service', () => { @@ -10,6 +11,7 @@ jest.mock('@/license/License.service', () => { }, }; }); +jest.mock('@/posthog'); describe('Telemetry', () => { let startPulseSpy: jest.SpyInstance; @@ -39,7 +41,11 @@ describe('Telemetry', () => { beforeEach(() => { spyTrack.mockClear(); - telemetry = new Telemetry(instanceId); + + const postHog = new PostHogClient(); + postHog.init(instanceId); + + telemetry = new Telemetry(instanceId, postHog); (telemetry as any).rudderStack = { flush: () => {}, identify: () => {}, diff --git a/packages/editor-ui/index.html b/packages/editor-ui/index.html index 0dc44757dc..35923d36b5 100644 --- a/packages/editor-ui/index.html +++ b/packages/editor-ui/index.html @@ -8,6 +8,8 @@ + + n8n.io - Workflow Automation diff --git a/packages/editor-ui/src/App.vue b/packages/editor-ui/src/App.vue index 358717f13a..fa1d40a00e 100644 --- a/packages/editor-ui/src/App.vue +++ b/packages/editor-ui/src/App.vue @@ -30,7 +30,7 @@ import Modals from './components/Modals.vue'; import LoadingView from './views/LoadingView.vue'; import Telemetry from './components/Telemetry.vue'; -import { HIRING_BANNER, LOCAL_STORAGE_THEME, POSTHOG_ASSUMPTION_TEST, VIEWS } from './constants'; +import { HIRING_BANNER, LOCAL_STORAGE_THEME, VIEWS } from './constants'; import mixins from 'vue-typed-mixins'; import { showMessage } from '@/mixins/showMessage'; @@ -179,17 +179,6 @@ export default mixins(showMessage, userHelpers, restApi, historyHelper).extend({ window.document.body.classList.add(`theme-${theme}`); } }, - trackExperiments() { - const assumption = window.posthog?.getFeatureFlag?.(POSTHOG_ASSUMPTION_TEST); - const isVideo = assumption === 'assumption-video'; - const isDemo = assumption === 'assumption-demo'; - - if (isVideo) { - this.$telemetry.track('User is part of video experiment'); - } else if (isDemo) { - this.$telemetry.track('User is part of demo experiment'); - } - }, }, async mounted() { this.setTheme(); @@ -206,10 +195,6 @@ export default mixins(showMessage, userHelpers, restApi, historyHelper).extend({ if (this.defaultLocale !== 'en') { await this.nodeTypesStore.getNodeTranslationHeaders(); } - - setTimeout(() => { - this.trackExperiments(); - }, 0); }, watch: { $route(route) { diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index 2a972f8ba1..dffc17b4ca 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -30,6 +30,7 @@ import { IDisplayOptions, IExecutionsSummary, IAbstractEventMessage, + FeatureFlags, } from 'n8n-workflow'; import { SignInType } from './constants'; import { FAKE_DOOR_FEATURES, TRIGGER_NODE_FILTER, REGULAR_NODE_FILTER } from './constants'; @@ -37,6 +38,37 @@ import { BulkCommand, Undoable } from '@/models/history'; export * from 'n8n-design-system/types'; +declare global { + interface Window { + posthog?: { + init( + key: string, + options?: { + api_host?: string; + autocapture?: boolean; + disable_session_recording?: boolean; + debug?: boolean; + bootstrap?: { + distinctId?: string; + isIdentifiedID?: boolean; + featureFlags: FeatureFlags; + }; + }, + ): void; + isFeatureEnabled?(flagName: string): boolean; + getFeatureFlag?(flagName: string): boolean | string; + identify?( + id: string, + userProperties?: Record, + userPropertiesOnce?: Record, + ): void; + reset?(resetDeviceId?: boolean): void; + onFeatureFlags?(callback: (keys: string[], map: FeatureFlags) => void): void; + reloadFeatureFlags?(): void; + }; + } +} + export type EndpointStyle = { width?: number; height?: number; @@ -551,13 +583,17 @@ export interface IUserResponse { signInType?: SignInType; } +export interface CurrentUserResponse extends IUserResponse { + featureFlags?: FeatureFlags; +} + export interface IUser extends IUserResponse { isDefaultUser: boolean; isPendingUser: boolean; isOwner: boolean; inviteAcceptUrl?: string; fullName?: string; - createdAt?: Date; + createdAt?: string; } export interface IVersionNotificationSettings { @@ -704,6 +740,14 @@ export interface IN8nUISettings { enabled: boolean; host: string; }; + posthog: { + enabled: boolean; + apiHost: string; + apiKey: string; + autocapture: boolean; + disableSessionRecording: boolean; + debug: boolean; + }; executionMode: string; pushBackend: 'sse' | 'websocket'; communityNodesEnabled: boolean; diff --git a/packages/editor-ui/src/api/users.ts b/packages/editor-ui/src/api/users.ts index bc14132d20..50b7b9043e 100644 --- a/packages/editor-ui/src/api/users.ts +++ b/packages/editor-ui/src/api/users.ts @@ -1,4 +1,5 @@ import { + CurrentUserResponse, IInviteResponse, IPersonalizationLatestVersion, IRestApiContext, @@ -7,14 +8,14 @@ import { import { IDataObject } from 'n8n-workflow'; import { makeRestApiRequest } from '@/utils/apiUtils'; -export function loginCurrentUser(context: IRestApiContext): Promise { +export function loginCurrentUser(context: IRestApiContext): Promise { return makeRestApiRequest(context, 'GET', '/login'); } export function login( context: IRestApiContext, params: { email: string; password: string }, -): Promise { +): Promise { return makeRestApiRequest(context, 'POST', '/login', params); } @@ -55,7 +56,7 @@ export function signup( lastName: string; password: string; }, -): Promise { +): Promise { const { inviteeId, ...props } = params; return makeRestApiRequest( context, diff --git a/packages/editor-ui/src/components/Telemetry.vue b/packages/editor-ui/src/components/Telemetry.vue index a2084af1bc..26b5279832 100644 --- a/packages/editor-ui/src/components/Telemetry.vue +++ b/packages/editor-ui/src/components/Telemetry.vue @@ -53,11 +53,6 @@ export default mixins(externalHooks).extend({ versionCli: this.rootStore.versionCli, }); - this.$externalHooks().run('telemetry.currentUserIdChanged', { - instanceId: this.rootStore.instanceId, - userId: this.currentUserId, - }); - this.isTelemetryInitialized = true; }, }, @@ -69,10 +64,6 @@ export default mixins(externalHooks).extend({ if (this.isTelemetryEnabled) { this.$telemetry.identify(this.rootStore.instanceId, userId); } - this.$externalHooks().run('telemetry.currentUserIdChanged', { - instanceId: this.rootStore.instanceId, - userId, - }); }, isTelemetryEnabledOnRoute(enabled) { if (enabled) { diff --git a/packages/editor-ui/src/constants.ts b/packages/editor-ui/src/constants.ts index 0a11af64c5..b97ba14864 100644 --- a/packages/editor-ui/src/constants.ts +++ b/packages/editor-ui/src/constants.ts @@ -506,4 +506,18 @@ export const EXPRESSION_EDITOR_PARSER_TIMEOUT = 15_000; // ms export const KEEP_AUTH_IN_NDV_FOR_NODES = [HTTP_REQUEST_NODE_TYPE, WEBHOOK_NODE_TYPE]; export const MAIN_AUTH_FIELD_NAME = 'authentication'; export const NODE_RESOURCE_FIELD_NAME = 'resource'; -export const POSTHOG_ASSUMPTION_TEST = 'adore-assumption-tests'; + +export const ASSUMPTION_EXPERIMENT = { + name: 'adore-assumption-tests-1', + control: 'control', + demo: 'assumption-demo', + video: 'assumption-video', +}; + +export const ONBOARDING_EXPERIMENT = { + name: 'onboarding-checklist', + control: 'control', + variant: 'variant', +}; + +export const EXPERIMENTS_TO_TRACK = [ASSUMPTION_EXPERIMENT.name, ONBOARDING_EXPERIMENT.name]; diff --git a/packages/editor-ui/src/plugins/telemetry/index.ts b/packages/editor-ui/src/plugins/telemetry/index.ts index 1958b591db..f4fcbdfb3c 100644 --- a/packages/editor-ui/src/plugins/telemetry/index.ts +++ b/packages/editor-ui/src/plugins/telemetry/index.ts @@ -6,6 +6,7 @@ import type { INodeCreateElement } from '@/Interface'; import type { IUserNodesPanelSession } from './telemetry.types'; import { useSettingsStore } from '@/stores/settings'; import { useRootStore } from '@/stores/n8nRootStore'; +import { useTelemetryStore } from '@/stores/telemetry'; export class Telemetry { private pageEventQueue: Array<{ route: Route }>; @@ -60,6 +61,7 @@ export class Telemetry { configUrl: 'https://api-rs.n8n.io', ...logging, }); + useTelemetryStore().init(this); this.identify(instanceId, userId, versionCli); diff --git a/packages/editor-ui/src/plugins/telemetry/telemetry.types.ts b/packages/editor-ui/src/plugins/telemetry/telemetry.types.ts index aedd823e6d..960875d920 100644 --- a/packages/editor-ui/src/plugins/telemetry/telemetry.types.ts +++ b/packages/editor-ui/src/plugins/telemetry/telemetry.types.ts @@ -9,10 +9,6 @@ declare module 'vue/types/vue' { declare global { interface Window { rudderanalytics: RudderStack; - posthog: { - isFeatureEnabled(flagName: string): boolean; - getFeatureFlag(flagName: string): boolean | string; - }; } } diff --git a/packages/editor-ui/src/stores/posthog.ts b/packages/editor-ui/src/stores/posthog.ts new file mode 100644 index 0000000000..d476bb64d7 --- /dev/null +++ b/packages/editor-ui/src/stores/posthog.ts @@ -0,0 +1,124 @@ +import { ref, Ref, watch } from 'vue'; +import { defineStore } from 'pinia'; +import { useUsersStore } from '@/stores/users'; +import { useRootStore } from '@/stores/n8nRootStore'; +import { useSettingsStore } from './settings'; +import { FeatureFlags } from 'n8n-workflow'; +import { EXPERIMENTS_TO_TRACK } from '@/constants'; +import { useTelemetryStore } from './telemetry'; + +export const usePostHogStore = defineStore('posthog', () => { + const usersStore = useUsersStore(); + const settingsStore = useSettingsStore(); + const telemetryStore = useTelemetryStore(); + const rootStore = useRootStore(); + + const featureFlags: Ref = ref(null); + const initialized: Ref = ref(false); + const trackedDemoExp: Ref = ref({}); + + const reset = () => { + window.posthog?.reset?.(); + featureFlags.value = null; + trackedDemoExp.value = {}; + }; + + const getVariant = (experiment: keyof FeatureFlags): FeatureFlags[keyof FeatureFlags] => { + return featureFlags.value?.[experiment]; + }; + + const isVariantEnabled = (experiment: string, variant: string) => { + return getVariant(experiment) === variant; + }; + + const identify = () => { + const instanceId = rootStore.instanceId; + const user = usersStore.currentUser; + const traits: Record = { instance_id: instanceId }; + + if (user && typeof user.createdAt === 'string') { + traits.created_at_timestamp = new Date(user.createdAt).getTime(); + } + + // For PostHog, main ID _cannot_ be `undefined` as done for RudderStack. + const id = user ? `${instanceId}#${user.id}` : instanceId; + window.posthog?.identify?.(id, traits); + }; + + const init = (evaluatedFeatureFlags?: FeatureFlags) => { + if (!window.posthog) { + return; + } + + const config = settingsStore.settings.posthog; + if (!config.enabled) { + return; + } + + const userId = usersStore.currentUserId; + if (!userId) { + return; + } + + const instanceId = rootStore.instanceId; + const distinctId = `${instanceId}#${userId}`; + + const options: Parameters[1] = { + api_host: config.apiHost, + autocapture: config.autocapture, + disable_session_recording: config.disableSessionRecording, + debug: config.debug, + }; + + if (evaluatedFeatureFlags) { + featureFlags.value = evaluatedFeatureFlags; + options.bootstrap = { + distinctId, + featureFlags: evaluatedFeatureFlags, + }; + } + + window.posthog?.init(config.apiKey, options); + + identify(); + + initialized.value = true; + }; + + const trackExperiment = (name: string) => { + const curr = featureFlags.value; + const prev = trackedDemoExp.value; + + if (!curr || curr[name] === undefined) { + return; + } + + if (curr[name] === prev[name]) { + return; + } + + const variant = curr[name]; + telemetryStore.track('User is part of experiment', { + name, + variant, + }); + + trackedDemoExp.value[name] = variant; + }; + + watch( + () => featureFlags.value, + () => { + setTimeout(() => { + EXPERIMENTS_TO_TRACK.forEach(trackExperiment); + }, 0); + }, + ); + + return { + init, + isVariantEnabled, + getVariant, + reset, + }; +}); diff --git a/packages/editor-ui/src/stores/telemetry.ts b/packages/editor-ui/src/stores/telemetry.ts new file mode 100644 index 0000000000..26338651a4 --- /dev/null +++ b/packages/editor-ui/src/stores/telemetry.ts @@ -0,0 +1,21 @@ +import type { Telemetry } from '@/plugins/telemetry'; +import { ITelemetryTrackProperties } from 'n8n-workflow'; +import { defineStore } from 'pinia'; +import { ref, Ref } from 'vue'; + +export const useTelemetryStore = defineStore('telemetry', () => { + const telemetry: Ref = ref(); + + const init = (tel: Telemetry) => { + telemetry.value = tel; + }; + + const track = (event: string, properties?: ITelemetryTrackProperties) => { + telemetry.value?.track(event, properties); + }; + + return { + init, + track, + }; +}); diff --git a/packages/editor-ui/src/stores/users.ts b/packages/editor-ui/src/stores/users.ts index 9c7cf2b50c..8d3963c866 100644 --- a/packages/editor-ui/src/stores/users.ts +++ b/packages/editor-ui/src/stores/users.ts @@ -34,6 +34,7 @@ import { getPersonalizedNodeTypes, isAuthorized, PERMISSIONS, ROLE } from '@/uti import { defineStore } from 'pinia'; import Vue from 'vue'; import { useRootStore } from './n8nRootStore'; +import { usePostHogStore } from './posthog'; import { useSettingsStore } from './settings'; import { useUIStore } from './ui'; @@ -141,23 +142,32 @@ export const useUsersStore = defineStore(STORES.USERS, { async loginWithCookie(): Promise { const rootStore = useRootStore(); const user = await loginCurrentUser(rootStore.getRestApiContext); - if (user) { - this.addUsers([user]); - this.currentUserId = user.id; + if (!user) { + return; } + + this.addUsers([user]); + this.currentUserId = user.id; + + usePostHogStore().init(user.featureFlags); }, async loginWithCreds(params: { email: string; password: string }): Promise { const rootStore = useRootStore(); const user = await login(rootStore.getRestApiContext, params); - if (user) { - this.addUsers([user]); - this.currentUserId = user.id; + if (!user) { + return; } + + this.addUsers([user]); + this.currentUserId = user.id; + + usePostHogStore().init(user.featureFlags); }, async logout(): Promise { const rootStore = useRootStore(); await logout(rootStore.getRestApiContext); this.currentUserId = null; + usePostHogStore().reset(); }, async preOwnerSetup() { return preOwnerSetup(useRootStore().getRestApiContext); @@ -197,6 +207,8 @@ export const useUsersStore = defineStore(STORES.USERS, { this.addUsers([user]); this.currentUserId = user.id; } + + usePostHogStore().init(user.featureFlags); }, async sendForgotPasswordEmail(params: { email: string }): Promise { const rootStore = useRootStore(); diff --git a/packages/editor-ui/src/views/NodeView.vue b/packages/editor-ui/src/views/NodeView.vue index 34fd4062b1..0dd1d2bea5 100644 --- a/packages/editor-ui/src/views/NodeView.vue +++ b/packages/editor-ui/src/views/NodeView.vue @@ -195,7 +195,7 @@ import { WEBHOOK_NODE_TYPE, TRIGGER_NODE_FILTER, EnterpriseEditionFeature, - POSTHOG_ASSUMPTION_TEST, + ASSUMPTION_EXPERIMENT, REGULAR_NODE_FILTER, MANUAL_TRIGGER_NODE_TYPE, } from '@/constants'; @@ -300,6 +300,7 @@ import { ready, } from '@jsplumb/browser-ui'; import { N8nPlusEndpoint } from '@/plugins/endpoints/N8nPlusEndpointType'; +import { usePostHogStore } from '@/stores/posthog'; interface AddNodeOptions { position?: XYPosition; @@ -2458,7 +2459,9 @@ export default mixins( }, async tryToAddWelcomeSticky(): Promise { const newWorkflow = this.workflowData; - if (window.posthog?.getFeatureFlag?.(POSTHOG_ASSUMPTION_TEST) === 'assumption-video') { + if ( + usePostHogStore().isVariantEnabled(ASSUMPTION_EXPERIMENT.name, ASSUMPTION_EXPERIMENT.video) + ) { // For novice users (onboardingFlowEnabled == true) // Inject welcome sticky note and zoom to fit diff --git a/packages/editor-ui/src/views/WorkflowsView.vue b/packages/editor-ui/src/views/WorkflowsView.vue index abe2a082ad..ca7143202a 100644 --- a/packages/editor-ui/src/views/WorkflowsView.vue +++ b/packages/editor-ui/src/views/WorkflowsView.vue @@ -127,7 +127,7 @@ import PageViewLayout from '@/components/layouts/PageViewLayout.vue'; import PageViewLayoutList from '@/components/layouts/PageViewLayoutList.vue'; import WorkflowCard from '@/components/WorkflowCard.vue'; import TemplateCard from '@/components/TemplateCard.vue'; -import { EnterpriseEditionFeature, POSTHOG_ASSUMPTION_TEST, VIEWS } from '@/constants'; +import { EnterpriseEditionFeature, ASSUMPTION_EXPERIMENT, VIEWS } from '@/constants'; import { debounceHelper } from '@/mixins/debounce'; import Vue from 'vue'; import { ITag, IUser, IWorkflowDb } from '@/Interface'; @@ -137,6 +137,7 @@ import { useUIStore } from '@/stores/ui'; import { useSettingsStore } from '@/stores/settings'; import { useUsersStore } from '@/stores/users'; import { useWorkflowsStore } from '@/stores/workflows'; +import { usePostHogStore } from '@/stores/posthog'; type IResourcesListLayoutInstance = Vue & { sendFiltersTelemetry: (source: string) => void }; @@ -183,7 +184,10 @@ export default mixins(showMessage, debounceHelper).extend({ return !!this.workflowsStore.activeWorkflows.length; }, isDemoTest(): boolean { - return window.posthog?.getFeatureFlag?.(POSTHOG_ASSUMPTION_TEST) === 'assumption-demo'; + return usePostHogStore().isVariantEnabled( + ASSUMPTION_EXPERIMENT.name, + ASSUMPTION_EXPERIMENT.demo, + ); }, statusFilterOptions(): Array<{ label: string; value: string | boolean }> { return [ diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index b113d0d843..1f9372c2c3 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -1812,6 +1812,10 @@ export interface ITelemetrySettings { config?: ITelemetryClientConfig; } +export interface FeatureFlags { + [featureFlag: string]: string | boolean | undefined; +} + export interface IConnectedNode { name: string; indicies: number[]; From f0f8d59fee223c6bc9f8459890ed4a31ef8cb0af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 21 Feb 2023 11:21:04 +0100 Subject: [PATCH 07/34] fix(core): Do not allow arbitrary path traversal in the credential-translation endpoint (#5522) --- packages/cli/src/Server.ts | 45 +------------- packages/cli/src/TranslationHelpers.ts | 16 ----- packages/cli/src/controllers/index.ts | 1 + .../src/controllers/translation.controller.ts | 58 +++++++++++++++++++ packages/cli/test/setup-mocks.ts | 2 + .../translation.controller.test.ts | 40 +++++++++++++ 6 files changed, 103 insertions(+), 59 deletions(-) create mode 100644 packages/cli/src/controllers/translation.controller.ts create mode 100644 packages/cli/test/unit/controllers/translation.controller.test.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 8d2b740a20..a5832346bd 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -57,7 +57,6 @@ import history from 'connect-history-api-fallback'; import config from '@/config'; import * as Queue from '@/Queue'; import { InternalHooksManager } from '@/InternalHooksManager'; -import { getCredentialTranslationPath } from '@/TranslationHelpers'; import { getSharedWorkflowIds } from '@/WorkflowHelpers'; import { nodesController } from '@/api/nodes.api'; @@ -88,6 +87,7 @@ import { MeController, OwnerController, PasswordResetController, + TranslationController, UsersController, } from '@/controllers'; @@ -366,6 +366,7 @@ class Server extends AbstractServer { new OwnerController({ config, internalHooks, repositories, logger }), new MeController({ externalHooks, internalHooks, repositories, logger }), new PasswordResetController({ config, externalHooks, internalHooks, repositories, logger }), + new TranslationController(config, this.credentialTypes), new UsersController({ config, mailer, @@ -606,48 +607,6 @@ class Server extends AbstractServer { ), ); - this.app.get( - `/${this.restEndpoint}/credential-translation`, - ResponseHelper.send( - async ( - req: express.Request & { query: { credentialType: string } }, - res: express.Response, - ): Promise => { - const translationPath = getCredentialTranslationPath({ - locale: this.frontendSettings.defaultLocale, - credentialType: req.query.credentialType, - }); - - try { - return require(translationPath); - } catch (error) { - return null; - } - }, - ), - ); - - // Returns node information based on node names and versions - const headersPath = pathJoin(NODES_BASE_DIR, 'dist', 'nodes', 'headers'); - this.app.get( - `/${this.restEndpoint}/node-translation-headers`, - ResponseHelper.send( - async (req: express.Request, res: express.Response): Promise => { - try { - await fsAccess(`${headersPath}.js`); - } catch (_) { - return; // no headers available - } - - try { - return require(headersPath); - } catch (error) { - res.status(500).send('Failed to load headers file'); - } - }, - ), - ); - // ---------------------------------------- // Node-Types // ---------------------------------------- diff --git a/packages/cli/src/TranslationHelpers.ts b/packages/cli/src/TranslationHelpers.ts index cc4319b04f..dd829534a6 100644 --- a/packages/cli/src/TranslationHelpers.ts +++ b/packages/cli/src/TranslationHelpers.ts @@ -1,7 +1,6 @@ import { join, dirname } from 'path'; import { readdir } from 'fs/promises'; import type { Dirent } from 'fs'; -import { NODES_BASE_DIR } from '@/constants'; const ALLOWED_VERSIONED_DIRNAME_LENGTH = [2, 3]; // e.g. v1, v10 @@ -47,18 +46,3 @@ export async function getNodeTranslationPath({ ? join(nodeDir, `v${maxVersion}`, 'translations', locale, `${nodeType}.json`) : join(nodeDir, 'translations', locale, `${nodeType}.json`); } - -/** - * Get the full path to a credential translation file in `/dist`. - */ -export function getCredentialTranslationPath({ - locale, - credentialType, -}: { - locale: string; - credentialType: string; -}): string { - const credsPath = join(NODES_BASE_DIR, 'dist', 'credentials'); - - return join(credsPath, 'translations', locale, `${credentialType}.json`); -} diff --git a/packages/cli/src/controllers/index.ts b/packages/cli/src/controllers/index.ts index 2ee7c7fd06..37ce548a54 100644 --- a/packages/cli/src/controllers/index.ts +++ b/packages/cli/src/controllers/index.ts @@ -2,4 +2,5 @@ export { AuthController } from './auth.controller'; export { MeController } from './me.controller'; export { OwnerController } from './owner.controller'; export { PasswordResetController } from './passwordReset.controller'; +export { TranslationController } from './translation.controller'; export { UsersController } from './users.controller'; diff --git a/packages/cli/src/controllers/translation.controller.ts b/packages/cli/src/controllers/translation.controller.ts new file mode 100644 index 0000000000..8d24f9e113 --- /dev/null +++ b/packages/cli/src/controllers/translation.controller.ts @@ -0,0 +1,58 @@ +import type { Request } from 'express'; +import { ICredentialTypes } from 'n8n-workflow'; +import { join } from 'path'; +import { access } from 'fs/promises'; +import { Get, RestController } from '@/decorators'; +import { BadRequestError, InternalServerError } from '@/ResponseHelper'; +import { Config } from '@/config'; +import { NODES_BASE_DIR } from '@/constants'; + +export const CREDENTIAL_TRANSLATIONS_DIR = 'n8n-nodes-base/dist/credentials/translations'; +export const NODE_HEADERS_PATH = join(NODES_BASE_DIR, 'dist/nodes/headers'); + +export declare namespace TranslationRequest { + export type Credential = Request<{}, {}, {}, { credentialType: string }>; +} + +@RestController('/') +export class TranslationController { + constructor(private config: Config, private credentialTypes: ICredentialTypes) {} + + @Get('/credential-translation') + async getCredentialTranslation(req: TranslationRequest.Credential) { + const { credentialType } = req.query; + + if (!this.credentialTypes.recognizes(credentialType)) + throw new BadRequestError(`Invalid Credential type: "${credentialType}"`); + + const defaultLocale = this.config.getEnv('defaultLocale'); + const translationPath = join( + CREDENTIAL_TRANSLATIONS_DIR, + defaultLocale, + `${credentialType}.json`, + ); + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return require(translationPath); + } catch (error) { + return null; + } + } + + @Get('/node-translation-headers') + async getNodeTranslationHeaders() { + try { + await access(`${NODE_HEADERS_PATH}.js`); + } catch (_) { + return; // no headers available + } + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return require(NODE_HEADERS_PATH); + } catch (error) { + throw new InternalServerError('Failed to load headers file'); + } + } +} diff --git a/packages/cli/test/setup-mocks.ts b/packages/cli/test/setup-mocks.ts index 1a9bb4e9c0..c6db2d147c 100644 --- a/packages/cli/test/setup-mocks.ts +++ b/packages/cli/test/setup-mocks.ts @@ -1,3 +1,5 @@ +import 'reflect-metadata'; + jest.mock('@sentry/node'); jest.mock('@n8n_io/license-sdk'); jest.mock('@/telemetry'); diff --git a/packages/cli/test/unit/controllers/translation.controller.test.ts b/packages/cli/test/unit/controllers/translation.controller.test.ts new file mode 100644 index 0000000000..a24d462f81 --- /dev/null +++ b/packages/cli/test/unit/controllers/translation.controller.test.ts @@ -0,0 +1,40 @@ +import { mock } from 'jest-mock-extended'; +import type { ICredentialTypes } from 'n8n-workflow'; +import type { Config } from '@/config'; +import { + TranslationController, + TranslationRequest, + CREDENTIAL_TRANSLATIONS_DIR, +} from '@/controllers/translation.controller'; +import { BadRequestError } from '@/ResponseHelper'; + +describe('TranslationController', () => { + const config = mock(); + const credentialTypes = mock(); + const controller = new TranslationController(config, credentialTypes); + + describe('getCredentialTranslation', () => { + it('should throw 400 on invalid credential types', async () => { + const credentialType = 'not-a-valid-credential-type'; + const req = mock({ query: { credentialType } }); + credentialTypes.recognizes.calledWith(credentialType).mockReturnValue(false); + + expect(controller.getCredentialTranslation(req)).rejects.toThrowError( + new BadRequestError(`Invalid Credential type: "${credentialType}"`), + ); + }); + + it('should return translation json on valid credential types', async () => { + const credentialType = 'credential-type'; + const req = mock({ query: { credentialType } }); + config.getEnv.calledWith('defaultLocale').mockReturnValue('de'); + credentialTypes.recognizes.calledWith(credentialType).mockReturnValue(true); + const response = { translation: 'string' }; + jest.mock(`${CREDENTIAL_TRANSLATIONS_DIR}/de/credential-type.json`, () => response, { + virtual: true, + }); + + expect(await controller.getCredentialTranslation(req)).toEqual(response); + }); + }); +}); From eef257406730a989ec8e7a056c3d4234300fdb25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 21 Feb 2023 11:21:17 +0100 Subject: [PATCH 08/34] fix(core): Do not allow arbitrary path traversal in BinaryDataManager (#5523) --- packages/cli/src/Server.ts | 34 +++++++++++-------- .../core/src/BinaryDataManager/FileSystem.ts | 27 +++++++++------ packages/core/src/errors.ts | 5 +++ packages/core/src/index.ts | 1 + 4 files changed, 42 insertions(+), 25 deletions(-) create mode 100644 packages/core/src/errors.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index a5832346bd..c90835b285 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -33,6 +33,7 @@ import { LoadNodeParameterOptions, LoadNodeListSearch, UserSettings, + FileNotFoundError, } from 'n8n-core'; import type { @@ -1149,21 +1150,26 @@ class Server extends AbstractServer { // TODO UM: check if this needs permission check for UM const identifier = req.params.path; const binaryDataManager = BinaryDataManager.getInstance(); - const binaryPath = binaryDataManager.getBinaryPath(identifier); - let { mode, fileName, mimeType } = req.query; - if (!fileName || !mimeType) { - try { - const metadata = await binaryDataManager.getBinaryMetadata(identifier); - fileName = metadata.fileName; - mimeType = metadata.mimeType; - res.setHeader('Content-Length', metadata.fileSize); - } catch {} + try { + const binaryPath = binaryDataManager.getBinaryPath(identifier); + let { mode, fileName, mimeType } = req.query; + if (!fileName || !mimeType) { + try { + const metadata = await binaryDataManager.getBinaryMetadata(identifier); + fileName = metadata.fileName; + mimeType = metadata.mimeType; + res.setHeader('Content-Length', metadata.fileSize); + } catch {} + } + if (mimeType) res.setHeader('Content-Type', mimeType); + if (mode === 'download') { + res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`); + } + res.sendFile(binaryPath); + } catch (error) { + if (error instanceof FileNotFoundError) res.writeHead(404).end(); + else throw error; } - if (mimeType) res.setHeader('Content-Type', mimeType); - if (mode === 'download') { - res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`); - } - res.sendFile(binaryPath); }, ); diff --git a/packages/core/src/BinaryDataManager/FileSystem.ts b/packages/core/src/BinaryDataManager/FileSystem.ts index b66093b3d2..df03356975 100644 --- a/packages/core/src/BinaryDataManager/FileSystem.ts +++ b/packages/core/src/BinaryDataManager/FileSystem.ts @@ -7,6 +7,7 @@ import type { BinaryMetadata } from 'n8n-workflow'; import { jsonParse } from 'n8n-workflow'; import type { IBinaryDataConfig, IBinaryDataManager } from '../Interfaces'; +import { FileNotFoundError } from '../errors'; const PREFIX_METAFILE = 'binarymeta'; const PREFIX_PERSISTED_METAFILE = 'persistedmeta'; @@ -85,17 +86,17 @@ export class BinaryDataFileSystem implements IBinaryDataManager { } getBinaryPath(identifier: string): string { - return path.join(this.storagePath, identifier); + return this.resolveStoragePath(identifier); } getMetadataPath(identifier: string): string { - return path.join(this.storagePath, `${identifier}.metadata`); + return this.resolveStoragePath(`${identifier}.metadata`); } async markDataForDeletionByExecutionId(executionId: string): Promise { const tt = new Date(new Date().getTime() + this.binaryDataTTL * 60000); return fs.writeFile( - path.join(this.getBinaryDataMetaPath(), `${PREFIX_METAFILE}_${executionId}_${tt.valueOf()}`), + this.resolveStoragePath('meta', `${PREFIX_METAFILE}_${executionId}_${tt.valueOf()}`), '', ); } @@ -116,8 +117,8 @@ export class BinaryDataFileSystem implements IBinaryDataManager { const timeAtNextHour = currentTime + 3600000 - (currentTime % 3600000); const timeoutTime = timeAtNextHour + this.persistedBinaryDataTTL * 60000; - const filePath = path.join( - this.getBinaryDataPersistMetaPath(), + const filePath = this.resolveStoragePath( + 'persistMeta', `${PREFIX_PERSISTED_METAFILE}_${executionId}_${timeoutTime}`, ); @@ -170,21 +171,18 @@ export class BinaryDataFileSystem implements IBinaryDataManager { const newBinaryDataId = this.generateFileName(prefix); return fs - .copyFile( - path.join(this.storagePath, binaryDataId), - path.join(this.storagePath, newBinaryDataId), - ) + .copyFile(this.resolveStoragePath(binaryDataId), this.resolveStoragePath(newBinaryDataId)) .then(() => newBinaryDataId); } async deleteBinaryDataByExecutionId(executionId: string): Promise { const regex = new RegExp(`${executionId}_*`); - const filenames = await fs.readdir(path.join(this.storagePath)); + const filenames = await fs.readdir(this.storagePath); const proms = filenames.reduce( (allProms, filename) => { if (regex.test(filename)) { - allProms.push(fs.rm(path.join(this.storagePath, filename))); + allProms.push(fs.rm(this.resolveStoragePath(filename))); } return allProms; @@ -253,4 +251,11 @@ export class BinaryDataFileSystem implements IBinaryDataManager { throw new Error(`Error finding file: ${filePath}`); } } + + private resolveStoragePath(...args: string[]) { + const returnPath = path.join(this.storagePath, ...args); + if (path.relative(this.storagePath, returnPath).startsWith('..')) + throw new FileNotFoundError('Invalid path detected'); + return returnPath; + } } diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts new file mode 100644 index 0000000000..c425675c89 --- /dev/null +++ b/packages/core/src/errors.ts @@ -0,0 +1,5 @@ +export class FileNotFoundError extends Error { + constructor(readonly filePath: string) { + super(`File not found: ${filePath}`); + } +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index cef794cfa5..7a77667f59 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -15,6 +15,7 @@ export * from './LoadNodeListSearch'; export * from './NodeExecuteFunctions'; export * from './WorkflowExecute'; export { eventEmitter, NodeExecuteFunctions, UserSettings }; +export * from './errors'; declare module 'http' { export interface IncomingMessage { From 510855d9581f07e5081a7bc11377cd6216ba7edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 21 Feb 2023 11:22:54 +0100 Subject: [PATCH 09/34] fix(core): User update endpoint should only allow updating email, firstName, and lastName (#5526) --- packages/cli/package.json | 1 + packages/cli/src/GenericHelpers.ts | 3 +- packages/cli/src/controllers/me.controller.ts | 35 +++++++------- packages/cli/src/databases/entities/User.ts | 5 +- packages/cli/src/requests.ts | 23 +++++++-- .../unit/controllers/me.controller.test.ts | 48 ++++++++++++++++--- pnpm-lock.yaml | 26 ++++++---- 7 files changed, 101 insertions(+), 40 deletions(-) diff --git a/packages/cli/package.json b/packages/cli/package.json index 6c7f275e80..df23a55365 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -129,6 +129,7 @@ "callsites": "^3.1.0", "change-case": "^4.1.1", "class-validator": "^0.14.0", + "class-transformer": "^0.5.1", "client-oauth2": "^4.2.5", "compression": "^1.7.4", "connect-history-api-fallback": "^1.6.0", diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts index 7d8011abde..c33e4dff88 100644 --- a/packages/cli/src/GenericHelpers.ts +++ b/packages/cli/src/GenericHelpers.ts @@ -22,6 +22,7 @@ import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; import type { TagEntity } from '@db/entities/TagEntity'; import type { User } from '@db/entities/User'; +import type { UserUpdatePayload } from '@/requests'; /** * Returns the base URL n8n is reachable from @@ -99,7 +100,7 @@ export async function generateUniqueName( } export async function validateEntity( - entity: WorkflowEntity | CredentialsEntity | TagEntity | User, + entity: WorkflowEntity | CredentialsEntity | TagEntity | User | UserUpdatePayload, ): Promise { const errors = await validate(entity); diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 463716b5f7..5c830a6803 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -1,4 +1,5 @@ import validator from 'validator'; +import { plainToInstance } from 'class-transformer'; import { Delete, Get, Patch, Post, RestController } from '@/decorators'; import { compareHash, @@ -7,13 +8,13 @@ import { validatePassword, } from '@/UserManagement/UserManagementHelper'; import { BadRequestError } from '@/ResponseHelper'; -import { User } from '@db/entities/User'; +import type { User } from '@db/entities/User'; import { validateEntity } from '@/GenericHelpers'; import { issueCookie } from '@/auth/jwt'; import { Response } from 'express'; import type { Repository } from 'typeorm'; import type { ILogger } from 'n8n-workflow'; -import { AuthenticatedRequest, MeRequest } from '@/requests'; +import { AuthenticatedRequest, MeRequest, UserUpdatePayload } from '@/requests'; import type { PublicUser, IDatabaseCollections, @@ -53,38 +54,40 @@ export class MeController { * Update the logged-in user's settings, except password. */ @Patch('/') - async updateCurrentUser(req: MeRequest.Settings, res: Response): Promise { - const { email } = req.body; + async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise { + const { id: userId, email: currentEmail } = req.user; + const payload = plainToInstance(UserUpdatePayload, req.body); + + const { email } = payload; if (!email) { this.logger.debug('Request to update user email failed because of missing email in payload', { - userId: req.user.id, - payload: req.body, + userId, + payload, }); throw new BadRequestError('Email is mandatory'); } if (!validator.isEmail(email)) { this.logger.debug('Request to update user email failed because of invalid email in payload', { - userId: req.user.id, + userId, invalidEmail: email, }); throw new BadRequestError('Invalid email address'); } - const { email: currentEmail } = req.user; - const newUser = new User(); + await validateEntity(payload); - Object.assign(newUser, req.user, req.body); + await this.userRepository.update(userId, payload); + const user = await this.userRepository.findOneOrFail({ + where: { id: userId }, + relations: { globalRole: true }, + }); - await validateEntity(newUser); - - const user = await this.userRepository.save(newUser); - - this.logger.info('User updated successfully', { userId: user.id }); + this.logger.info('User updated successfully', { userId }); await issueCookie(res, user); - const updatedKeys = Object.keys(req.body); + const updatedKeys = Object.keys(payload); void this.internalHooks.onUserUpdate({ user, fields_changed: updatedKeys, diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index 6cba438f79..d62bf8482f 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -111,6 +111,9 @@ export class User extends AbstractEntity implements IUser { @AfterLoad() @AfterUpdate() computeIsPending(): void { - this.isPending = this.password === null; + this.isPending = + this.globalRole?.name === 'owner' && this.globalRole.scope === 'global' + ? false + : this.password === null; } } diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 0a1be41429..f8c5af17b4 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -10,11 +10,28 @@ import type { IWorkflowSettings, } from 'n8n-workflow'; +import { IsEmail, IsString, Length } from 'class-validator'; +import { NoXss } from '@db/utils/customValidators'; import type { PublicUser, IExecutionDeleteFilter, IWorkflowDb } from '@/Interfaces'; import type { Role } from '@db/entities/Role'; import type { User } from '@db/entities/User'; import type * as UserManagementMailer from '@/UserManagement/email/UserManagementMailer'; +export class UserUpdatePayload implements Pick { + @IsEmail() + email: string; + + @NoXss() + @IsString({ message: 'First name must be of type string.' }) + @Length(1, 32, { message: 'First name must be $constraint1 to $constraint2 characters long.' }) + firstName: string; + + @NoXss() + @IsString({ message: 'Last name must be of type string.' }) + @Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' }) + lastName: string; +} + export type AuthlessRequest< RouteParams = {}, ResponseBody = {}, @@ -144,11 +161,7 @@ export declare namespace ExecutionRequest { // ---------------------------------- export declare namespace MeRequest { - export type Settings = AuthenticatedRequest< - {}, - {}, - Pick - >; + export type UserUpdate = AuthenticatedRequest<{}, {}, UserUpdatePayload>; export type Password = AuthenticatedRequest< {}, {}, diff --git a/packages/cli/test/unit/controllers/me.controller.test.ts b/packages/cli/test/unit/controllers/me.controller.test.ts index f926c4fc4f..4acfbaa509 100644 --- a/packages/cli/test/unit/controllers/me.controller.test.ts +++ b/packages/cli/test/unit/controllers/me.controller.test.ts @@ -25,40 +25,74 @@ describe('MeController', () => { describe('updateCurrentUser', () => { it('should throw BadRequestError if email is missing in the payload', async () => { - const req = mock({}); + const req = mock({}); expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError( new BadRequestError('Email is mandatory'), ); }); it('should throw BadRequestError if email is invalid', async () => { - const req = mock({ body: { email: 'invalid-email' } }); + const req = mock({ body: { email: 'invalid-email' } }); expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError( new BadRequestError('Invalid email address'), ); }); it('should update the user in the DB, and issue a new cookie', async () => { - const req = mock({ - user: mock({ id: '123', password: 'password', authIdentities: [] }), - body: { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }, + const user = mock({ + id: '123', + password: 'password', + authIdentities: [], + globalRoleId: '1', }); + const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; + const req = mock({ user, body: reqBody }); const res = mock(); - userRepository.save.calledWith(anyObject()).mockResolvedValue(req.user); + userRepository.findOneOrFail.mockResolvedValue(user); jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); await controller.updateCurrentUser(req, res); + expect(userRepository.update).toHaveBeenCalled(); + const cookieOptions = captor(); expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions); expect(cookieOptions.value.httpOnly).toBe(true); expect(cookieOptions.value.sameSite).toBe('lax'); expect(externalHooks.run).toHaveBeenCalledWith('user.profile.update', [ - req.user.email, + user.email, anyObject(), ]); }); + + it('should not allow updating any other fields on a user besides email and name', async () => { + const user = mock({ + id: '123', + password: 'password', + authIdentities: [], + globalRoleId: '1', + }); + const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' }; + const req = mock({ user, body: reqBody }); + const res = mock(); + userRepository.findOneOrFail.mockResolvedValue(user); + jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token'); + + // Add invalid data to the request payload + Object.assign(reqBody, { id: '0', globalRoleId: '42' }); + + await controller.updateCurrentUser(req, res); + + expect(userRepository.update).toHaveBeenCalled(); + + const updatedUser = userRepository.update.mock.calls[0][1]; + expect(updatedUser.email).toBe(reqBody.email); + expect(updatedUser.firstName).toBe(reqBody.firstName); + expect(updatedUser.lastName).toBe(reqBody.lastName); + expect(updatedUser.id).not.toBe('0'); + expect(updatedUser.globalRoleId).not.toBe('42'); + }); }); describe('updatePassword', () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 055fb2d89c..9e45e143d5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -163,6 +163,7 @@ importers: callsites: ^3.1.0 change-case: ^4.1.1 chokidar: 3.5.2 + class-transformer: ^0.5.1 class-validator: ^0.14.0 client-oauth2: ^4.2.5 compression: ^1.7.4 @@ -259,6 +260,7 @@ importers: bull: 4.10.2 callsites: 3.1.0 change-case: 4.1.2 + class-transformer: 0.5.1 class-validator: 0.14.0 client-oauth2: 4.3.3 compression: 1.7.4 @@ -4138,7 +4140,7 @@ packages: dependencies: '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/manager-api': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/preview-api': 7.0.0-beta.46 @@ -4346,7 +4348,7 @@ packages: '@storybook/client-logger': 7.0.0-beta.46 '@storybook/components': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/docs-tools': 7.0.0-beta.46 '@storybook/global': 5.0.0 '@storybook/manager-api': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe @@ -4566,7 +4568,7 @@ packages: '@babel/core': 7.20.12 '@babel/preset-env': 7.20.2_@babel+core@7.20.12 '@babel/types': 7.20.7 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/csf-tools': 7.0.0-beta.46 '@storybook/node-logger': 7.0.0-beta.46 '@storybook/types': 7.0.0-beta.46 @@ -4606,7 +4608,7 @@ packages: react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 dependencies: '@storybook/client-logger': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/theming': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/types': 7.0.0-beta.46 @@ -4677,7 +4679,7 @@ packages: '@storybook/builder-manager': 7.0.0-beta.46 '@storybook/core-common': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/csf-tools': 7.0.0-beta.46 '@storybook/docs-mdx': 0.0.1-next.6 '@storybook/global': 5.0.0 @@ -4747,7 +4749,7 @@ packages: resolution: {integrity: sha512-H7zXfL1wf/1jWi5MaFISt/taxE41fgpV/uLfi5CHcHLX9ZgeQs2B/2utpUgwvBsxiL+E/jKAt5cLeuZCIvglMg==} dependencies: '@babel/types': 7.20.7 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/types': 7.0.0-beta.46 fs-extra: 11.1.0 recast: 0.23.1 @@ -4762,8 +4764,8 @@ packages: lodash: 4.17.21 dev: true - /@storybook/csf/0.0.2-next.9: - resolution: {integrity: sha512-ECOLMK425s+z8oA0aVAhBhhquuwTsZrM4oha/5De44JG8uYGXhqVrv/l27oxZEkwytuiQu+9f65HxYli+DY+3w==} + /@storybook/csf/0.0.2-next.10: + resolution: {integrity: sha512-m2PFgBP/xRIF85VrDhvesn9ktaD2pN3VUjvMqkAL/cINp/3qXsCyI81uw7N5VEOkQAbWrY2FcydnvEPDEdE8fA==} dependencies: type-fest: 2.19.0 dev: true @@ -4799,7 +4801,7 @@ packages: '@storybook/channels': 7.0.0-beta.46 '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/router': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe '@storybook/theming': 7.0.0-beta.46_6l5554ty5ajsajah6yazvrjhoe @@ -4889,7 +4891,7 @@ packages: '@storybook/channels': 7.0.0-beta.46 '@storybook/client-logger': 7.0.0-beta.46 '@storybook/core-events': 7.0.0-beta.46 - '@storybook/csf': 0.0.2-next.9 + '@storybook/csf': 0.0.2-next.10 '@storybook/global': 5.0.0 '@storybook/types': 7.0.0-beta.46 '@types/qs': 6.9.7 @@ -8090,6 +8092,10 @@ packages: resolution: {integrity: sha512-kgMuFyE78OC6Dyu3Dy7vcx4uy97EIbVxJB/B0eJ3bUNAkwdNcxYzgKltnyADiYwsR7SEqkkUPsEUT//OVS6XMA==} dev: false + /class-transformer/0.5.1: + resolution: {integrity: sha512-SQa1Ws6hUbfC98vKGxZH3KFY0Y1lm5Zm0SY8XX9zbK7FJCyVEac3ATW0RIpwzW+oOfmHE5PMPufDG9hCfoEOMw==} + dev: false + /class-utils/0.3.6: resolution: {integrity: sha512-qOhPa/Fj7s6TY8H8esGu5QNpMMQxz79h+urzrNYN6mn+9BnxlDGf5QZ+XeCDsxSjPqsSR56XOZOJmpeurnLMeg==} engines: {node: '>=0.10.0'} From 684d71752064e25143e09666e539b91b3dcd5f71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milorad=20FIlipovi=C4=87?= Date: Tue, 21 Feb 2023 11:27:15 +0100 Subject: [PATCH 10/34] fix(editor): Fix unexpected date rendering on front-end (#5528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔥 Remove front-end date rendering logic that changes date timezone to workflow TZ --- packages/editor-ui/src/components/RunDataJson.vue | 8 ++------ packages/editor-ui/src/components/RunDataTable.vue | 11 +---------- packages/editor-ui/src/utils/typesUtils.ts | 14 -------------- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/packages/editor-ui/src/components/RunDataJson.vue b/packages/editor-ui/src/components/RunDataJson.vue index 149c2fa063..97f5d365a9 100644 --- a/packages/editor-ui/src/components/RunDataJson.vue +++ b/packages/editor-ui/src/components/RunDataJson.vue @@ -73,7 +73,7 @@ import VueJsonPretty from 'vue-json-pretty'; import { LOCAL_STORAGE_MAPPING_FLAG } from '@/constants'; import { IDataObject, INodeExecutionData } from 'n8n-workflow'; import Draggable from '@/components/Draggable.vue'; -import { parseDate, executionDataToJson, isString, shorten } from '@/utils'; +import { executionDataToJson, isString, shorten } from '@/utils'; import { INodeUi } from '@/Interface'; import { externalHooks } from '@/mixins/externalHooks'; import { mapStores } from 'pinia'; @@ -210,11 +210,7 @@ export default mixins(externalHooks).extend({ }, 1000); // ensure dest data gets set if drop }, getContent(value: unknown): string { - if (isString(value)) { - const parsedDate = parseDate(value, this.workflowsStore.workflow.settings?.timezone); - return parsedDate ? parsedDate.toString() : `"${value}"`; - } - return JSON.stringify(value); + return isString(value) ? `"${value}"` : JSON.stringify(value); }, getListItemName(path: string): string { return path.replace(/^(\["?\d"?]\.?)/g, ''); diff --git a/packages/editor-ui/src/components/RunDataTable.vue b/packages/editor-ui/src/components/RunDataTable.vue index 383e905e19..9f092a7fd7 100644 --- a/packages/editor-ui/src/components/RunDataTable.vue +++ b/packages/editor-ui/src/components/RunDataTable.vue @@ -159,7 +159,7 @@