From 5eca7c8e28907050dd47686e7e80dedbb2a4996a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 19 Jul 2024 10:33:13 +0200 Subject: [PATCH] refactor(core): More decouplings from internal hooks (no-changelog) (#10099) --- .../ExternalSecretsManager.ee.ts | 15 ++++--- packages/cli/src/InternalHooks.ts | 28 ------------ packages/cli/src/commands/audit.ts | 4 +- .../variables/variables.service.ee.ts | 5 ++- .../cli/src/eventbus/event-relay.service.ts | 2 +- packages/cli/src/eventbus/event.types.ts | 20 +++++++++ packages/cli/src/license/license.service.ts | 11 +++-- .../telemetry-event-relay.service.ts | 44 +++++++++++++++++++ .../externalSecrets.api.test.ts | 4 ++ .../ExternalSecretsManager.test.ts | 9 +++- .../test/unit/license/license.service.test.ts | 16 ++++--- 11 files changed, 106 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/ExternalSecrets/ExternalSecretsManager.ee.ts b/packages/cli/src/ExternalSecrets/ExternalSecretsManager.ee.ts index e6384b672..28a711cce 100644 --- a/packages/cli/src/ExternalSecrets/ExternalSecretsManager.ee.ts +++ b/packages/cli/src/ExternalSecrets/ExternalSecretsManager.ee.ts @@ -13,7 +13,7 @@ import { Logger } from '@/Logger'; import { jsonParse, type IDataObject, ApplicationError } from 'n8n-workflow'; import { EXTERNAL_SECRETS_INITIAL_BACKOFF, EXTERNAL_SECRETS_MAX_BACKOFF } from './constants'; import { License } from '@/License'; -import { InternalHooks } from '@/InternalHooks'; +import { EventRelay } from '@/eventbus/event-relay.service'; import { updateIntervalTime } from './externalSecretsHelper.ee'; import { ExternalSecretsProviders } from './ExternalSecretsProviders.ee'; import { OrchestrationService } from '@/services/orchestration.service'; @@ -38,6 +38,7 @@ export class ExternalSecretsManager { private readonly license: License, private readonly secretsProviders: ExternalSecretsProviders, private readonly cipher: Cipher, + private readonly eventRelay: EventRelay, ) {} async init(): Promise { @@ -308,12 +309,12 @@ export class ExternalSecretsManager { try { testResult = await this.getProvider(vaultType)?.test(); } catch {} - void Container.get(InternalHooks).onExternalSecretsProviderSettingsSaved({ - user_id: userId, - vault_type: vaultType, - is_new: isNew, - is_valid: testResult?.[0] ?? false, - error_message: testResult?.[1], + this.eventRelay.emit('external-secrets-provider-settings-saved', { + userId, + vaultType, + isNew, + isValid: testResult?.[0] ?? false, + errorMessage: testResult?.[1], }); } diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index 8647ecf97..09ac2d241 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -756,32 +756,4 @@ export class InternalHooks { }): Promise { return await this.telemetry.track('Workflow first data fetched', data); } - - /** - * License - */ - async onLicenseRenewAttempt(data: { success: boolean }): Promise { - await this.telemetry.track('Instance attempted to refresh license', data); - } - - /** - * Audit - */ - async onAuditGeneratedViaCli() { - return await this.telemetry.track('Instance generated security audit via CLI command'); - } - - async onVariableCreated(createData: { variable_type: string }): Promise { - return await this.telemetry.track('User created variable', createData); - } - - async onExternalSecretsProviderSettingsSaved(saveData: { - user_id?: string | undefined; - vault_type: string; - is_valid: boolean; - is_new: boolean; - error_message?: string | undefined; - }): Promise { - return await this.telemetry.track('User updated external secrets settings', saveData); - } } diff --git a/packages/cli/src/commands/audit.ts b/packages/cli/src/commands/audit.ts index 0fe1d1f9b..94621aaab 100644 --- a/packages/cli/src/commands/audit.ts +++ b/packages/cli/src/commands/audit.ts @@ -7,7 +7,7 @@ import { RISK_CATEGORIES } from '@/security-audit/constants'; import config from '@/config'; import type { Risk } from '@/security-audit/types'; import { BaseCommand } from './BaseCommand'; -import { InternalHooks } from '@/InternalHooks'; +import { EventRelay } from '@/eventbus/event-relay.service'; export class SecurityAudit extends BaseCommand { static description = 'Generate a security audit report for this n8n instance'; @@ -62,7 +62,7 @@ export class SecurityAudit extends BaseCommand { process.stdout.write(JSON.stringify(result, null, 2)); } - void Container.get(InternalHooks).onAuditGeneratedViaCli(); + Container.get(EventRelay).emit('security-audit-generated-via-cli'); } async catch(error: Error) { diff --git a/packages/cli/src/environments/variables/variables.service.ee.ts b/packages/cli/src/environments/variables/variables.service.ee.ts index 80a484625..720c902ac 100644 --- a/packages/cli/src/environments/variables/variables.service.ee.ts +++ b/packages/cli/src/environments/variables/variables.service.ee.ts @@ -1,18 +1,19 @@ import { Container, Service } from 'typedi'; import type { Variables } from '@db/entities/Variables'; -import { InternalHooks } from '@/InternalHooks'; import { generateNanoId } from '@db/utils/generators'; import { canCreateNewVariable } from './environmentHelpers'; import { CacheService } from '@/services/cache/cache.service'; import { VariablesRepository } from '@db/repositories/variables.repository'; import { VariableCountLimitReachedError } from '@/errors/variable-count-limit-reached.error'; import { VariableValidationError } from '@/errors/variable-validation.error'; +import { EventRelay } from '@/eventbus/event-relay.service'; @Service() export class VariablesService { constructor( protected cacheService: CacheService, protected variablesRepository: VariablesRepository, + private readonly eventRelay: EventRelay, ) {} async getAllCached(): Promise { @@ -70,7 +71,7 @@ export class VariablesService { } this.validateVariable(variable); - void Container.get(InternalHooks).onVariableCreated({ variable_type: variable.type }); + this.eventRelay.emit('variable-created', { variableType: variable.type }); const saveResult = await this.variablesRepository.save( { ...variable, diff --git a/packages/cli/src/eventbus/event-relay.service.ts b/packages/cli/src/eventbus/event-relay.service.ts index 8f6bb4c5c..edc5d8aea 100644 --- a/packages/cli/src/eventbus/event-relay.service.ts +++ b/packages/cli/src/eventbus/event-relay.service.ts @@ -4,7 +4,7 @@ import type { Event } from './event.types'; @Service() export class EventRelay extends EventEmitter { - emit(eventName: K, arg: Event[K]) { + emit(eventName: K, arg?: Event[K]) { super.emit(eventName, arg); return true; } diff --git a/packages/cli/src/eventbus/event.types.ts b/packages/cli/src/eventbus/event.types.ts index a93b4c120..b00b1382d 100644 --- a/packages/cli/src/eventbus/event.types.ts +++ b/packages/cli/src/eventbus/event.types.ts @@ -252,4 +252,24 @@ export type Event = { credsPushed: number; variablesPushed: number; }; + + 'license-renewal-attempted': { + success: boolean; + }; + + 'security-audit-generated-via-cli': { + // no payload + }; + + 'variable-created': { + variableType: string; + }; + + 'external-secrets-provider-settings-saved': { + userId?: string; + vaultType: string; + isValid: boolean; + isNew: boolean; + errorMessage?: string; + }; }; diff --git a/packages/cli/src/license/license.service.ts b/packages/cli/src/license/license.service.ts index 543b95373..2eb1433b0 100644 --- a/packages/cli/src/license/license.service.ts +++ b/packages/cli/src/license/license.service.ts @@ -3,7 +3,7 @@ import axios from 'axios'; import { Logger } from '@/Logger'; import { License } from '@/License'; -import { InternalHooks } from '@/InternalHooks'; +import { EventRelay } from '@/eventbus/event-relay.service'; import type { User } from '@db/entities/User'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -26,9 +26,9 @@ export class LicenseService { constructor( private readonly logger: Logger, private readonly license: License, - private readonly internalHooks: InternalHooks, private readonly workflowRepository: WorkflowRepository, private readonly urlService: UrlService, + private readonly eventRelay: EventRelay, ) {} async getLicenseData() { @@ -78,13 +78,12 @@ export class LicenseService { await this.license.renew(); } catch (e) { const message = this.mapErrorMessage(e as LicenseError, 'renew'); - // not awaiting so as not to make the endpoint hang - void this.internalHooks.onLicenseRenewAttempt({ success: false }); + + this.eventRelay.emit('license-renewal-attempted', { success: false }); throw new BadRequestError(message); } - // not awaiting so as not to make the endpoint hang - void this.internalHooks.onLicenseRenewAttempt({ success: true }); + this.eventRelay.emit('license-renewal-attempted', { success: true }); } private mapErrorMessage(error: LicenseError, action: 'activate' | 'renew') { diff --git a/packages/cli/src/telemetry/telemetry-event-relay.service.ts b/packages/cli/src/telemetry/telemetry-event-relay.service.ts index 982376226..b4a6b45a0 100644 --- a/packages/cli/src/telemetry/telemetry-event-relay.service.ts +++ b/packages/cli/src/telemetry/telemetry-event-relay.service.ts @@ -41,6 +41,18 @@ export class TelemetryEventRelay { this.eventRelay.on('source-control-user-finished-push-ui', (event) => this.sourceControlUserFinishedPushUi(event), ); + this.eventRelay.on('license-renewal-attempted', (event) => { + this.licenseRenewalAttempted(event); + }); + this.eventRelay.on('security-audit-generated-via-cli', () => { + this.securityAuditGeneratedViaCli(); + }); + this.eventRelay.on('variable-created', (event) => { + this.variableCreated(event); + }); + this.eventRelay.on('external-secrets-provider-settings-saved', (event) => { + this.externalSecretsProviderSettingsSaved(event); + }); } private teamProjectUpdated({ userId, role, members, projectId }: Event['team-project-updated']) { @@ -153,4 +165,36 @@ export class TelemetryEventRelay { variables_pushed: variablesPushed, }); } + + private licenseRenewalAttempted({ success }: Event['license-renewal-attempted']) { + void this.telemetry.track('Instance attempted to refresh license', { + success, + }); + } + + private securityAuditGeneratedViaCli() { + void this.telemetry.track('Instance generated security audit via CLI command'); + } + + private variableCreated({ variableType }: Event['variable-created']) { + void this.telemetry.track('User created variable', { + variable_type: variableType, + }); + } + + private externalSecretsProviderSettingsSaved({ + userId, + vaultType, + isValid, + isNew, + errorMessage, + }: Event['external-secrets-provider-settings-saved']) { + void this.telemetry.track('User updated external secrets settings', { + user_id: userId, + vault_type: vaultType, + is_valid: isValid, + is_new: isNew, + error_message: errorMessage, + }); + } } diff --git a/packages/cli/test/integration/ExternalSecrets/externalSecrets.api.test.ts b/packages/cli/test/integration/ExternalSecrets/externalSecrets.api.test.ts index 1cf03848c..8cb665ebf 100644 --- a/packages/cli/test/integration/ExternalSecrets/externalSecrets.api.test.ts +++ b/packages/cli/test/integration/ExternalSecrets/externalSecrets.api.test.ts @@ -21,6 +21,7 @@ import { TestFailProvider, } from '../../shared/ExternalSecrets/utils'; import type { SuperAgentTest } from '../shared/types'; +import type { EventRelay } from '@/eventbus/event-relay.service'; let authOwnerAgent: SuperAgentTest; let authMemberAgent: SuperAgentTest; @@ -49,6 +50,8 @@ async function getExternalSecretsSettings(): Promise(); + const resetManager = async () => { Container.get(ExternalSecretsManager).shutdown(); Container.set( @@ -59,6 +62,7 @@ const resetManager = async () => { Container.get(License), mockProvidersInstance, Container.get(Cipher), + eventRelay, ), ); diff --git a/packages/cli/test/unit/ExternalSecrets/ExternalSecretsManager.test.ts b/packages/cli/test/unit/ExternalSecrets/ExternalSecretsManager.test.ts index b77b33bd0..cf72688d2 100644 --- a/packages/cli/test/unit/ExternalSecrets/ExternalSecretsManager.test.ts +++ b/packages/cli/test/unit/ExternalSecrets/ExternalSecretsManager.test.ts @@ -49,7 +49,14 @@ describe('External Secrets Manager', () => { }); license.isExternalSecretsEnabled.mockReturnValue(true); settingsRepo.getEncryptedSecretsProviderSettings.mockResolvedValue(settings); - manager = new ExternalSecretsManager(mock(), settingsRepo, license, providersMock, cipher); + manager = new ExternalSecretsManager( + mock(), + settingsRepo, + license, + providersMock, + cipher, + mock(), + ); }); afterEach(() => { diff --git a/packages/cli/test/unit/license/license.service.test.ts b/packages/cli/test/unit/license/license.service.test.ts index 7ac75ba5b..9ecaeae57 100644 --- a/packages/cli/test/unit/license/license.service.test.ts +++ b/packages/cli/test/unit/license/license.service.test.ts @@ -1,6 +1,6 @@ import { LicenseErrors, LicenseService } from '@/license/license.service'; import type { License } from '@/License'; -import type { InternalHooks } from '@/InternalHooks'; +import type { EventRelay } from '@/eventbus/event-relay.service'; import type { WorkflowRepository } from '@db/repositories/workflow.repository'; import type { TEntitlement } from '@n8n_io/license-sdk'; import { mock } from 'jest-mock-extended'; @@ -8,10 +8,16 @@ import { BadRequestError } from '@/errors/response-errors/bad-request.error'; describe('LicenseService', () => { const license = mock(); - const internalHooks = mock(); const workflowRepository = mock(); const entitlement = mock({ productId: '123' }); - const licenseService = new LicenseService(mock(), license, internalHooks, workflowRepository); + const eventRelay = mock(); + const licenseService = new LicenseService( + mock(), + license, + workflowRepository, + mock(), + eventRelay, + ); license.getMainPlan.mockReturnValue(entitlement); license.getTriggerLimit.mockReturnValue(400); @@ -61,7 +67,7 @@ describe('LicenseService', () => { license.renew.mockResolvedValueOnce(); await licenseService.renewLicense(); - expect(internalHooks.onLicenseRenewAttempt).toHaveBeenCalledWith({ success: true }); + expect(eventRelay.emit).toHaveBeenCalledWith('license-renewal-attempted', { success: true }); }); test('on failure', async () => { @@ -70,7 +76,7 @@ describe('LicenseService', () => { new BadRequestError('Activation key has expired'), ); - expect(internalHooks.onLicenseRenewAttempt).toHaveBeenCalledWith({ success: false }); + expect(eventRelay.emit).toHaveBeenCalledWith('license-renewal-attempted', { success: false }); }); }); });