From 73691fd12222c0273b47176984194cbe1251d56a Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 1 Oct 2024 15:16:24 -0300 Subject: [PATCH 1/8] fix: Private apps are auto-enabled when updated regardless of any condition --- packages/apps-engine/src/server/AppManager.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 69a264f29c1d..4d4819a47478 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -720,7 +720,8 @@ export class AppManager { aff.setApp(app); if (updateOptions.loadApp) { - await this.updateLocal(stored, app); + const shouldEnableApp = AppStatusUtils.isEnabled(descriptor.status); + await this.updateLocal(stored, app, shouldEnableApp); await this.bridges .getAppActivationBridge() @@ -742,7 +743,7 @@ export class AppManager { * With an instance of a ProxiedApp, start it up and replace * the reference in the local app collection */ - public async updateLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer) { + public async updateLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer, enable: boolean) { const app = await (async () => { if (appPackageOrInstance instanceof Buffer) { const parseResult = await this.getParser().unpackageApp(appPackageOrInstance); @@ -762,7 +763,14 @@ export class AppManager { this.apps.set(app.getID(), app); - await this.runStartUpProcess(stored, app, false, true); + // Should enable === true, then we go through the entire start up process + // Otherwise, we only initialize it. + if (enable) { + // Start up the app + await this.runStartUpProcess(stored, app, false, true); + } else { + await this.initializeApp(stored, app, true, true); + } } public getLanguageContent(): { [key: string]: object } { From 75beef8659bfa5065312f619fb0a80b42c98dbd7 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 1 Oct 2024 23:44:23 -0300 Subject: [PATCH 2/8] add e2e ui tests for CE --- .../e2e/apps/private-apps-upload.spec.ts | 27 +++++++++++++++++++ .../tests/e2e/page-objects/marketplace.ts | 8 ++++++ 2 files changed, 35 insertions(+) diff --git a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts index 05540dfc011f..33325810456c 100644 --- a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts +++ b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts @@ -66,5 +66,32 @@ test.describe.serial('Private apps upload', () => { await poMarketplace.appMenu.click(); await expect(poMarketplace.btnEnableApp).toBeDisabled(); }); + + test('expect updated private app in CE to be kept as disabled', async ({ page }) => { + const fileChooserPromise = page.waitForEvent('filechooser'); + + await poMarketplace.btnUploadPrivateApp.click(); + await expect(poMarketplace.btnConfirmAppUploadModal).toBeEnabled(); + await poMarketplace.btnConfirmAppUploadModal.click(); + + await expect(poMarketplace.btnInstallPrivateApp).toBeDisabled(); + await poMarketplace.btnUploadPrivateAppFile.click(); + const fileChooser = await fileChooserPromise; + await fileChooser.setFiles('./tests/e2e/fixtures/files/test-app_0.0.1.zip'); + + await expect(poMarketplace.btnInstallPrivateApp).toBeEnabled(); + await poMarketplace.btnInstallPrivateApp.click(); + + await expect(poMarketplace.confirmAppUploadModalTitle).toHaveText('Private apps limit reached'); + await expect(poMarketplace.btnConfirmAppUploadModal).toBeEnabled(); + await poMarketplace.btnConfirmAppUploadModal.click(); + + await poMarketplace.btnConfirmAppUpdate.click(); + + await page.getByRole('button', { name: 'Agree' }).click(); + await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); + await page.reload(); + await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); + }); }); }); diff --git a/apps/meteor/tests/e2e/page-objects/marketplace.ts b/apps/meteor/tests/e2e/page-objects/marketplace.ts index bd66e1bad203..3132ccaf1a1a 100644 --- a/apps/meteor/tests/e2e/page-objects/marketplace.ts +++ b/apps/meteor/tests/e2e/page-objects/marketplace.ts @@ -42,4 +42,12 @@ export class Marketplace { get btnEnableApp(): Locator { return this.page.getByRole('menuitem', { name: 'Enable' }); } + + get btnDisableApp(): Locator { + return this.page.getByRole('menuitem', { name: 'Disable' }); + } + + get btnConfirmAppUpdate(): Locator { + return this.page.locator('role=button[name="Yes"]'); + } } From 4143aaee5bd3cb50133cc8d3d554562d2cbeb3ea Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Wed, 2 Oct 2024 00:38:31 -0300 Subject: [PATCH 3/8] Create changeset --- .changeset/selfish-experts-develop.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/selfish-experts-develop.md diff --git a/.changeset/selfish-experts-develop.md b/.changeset/selfish-experts-develop.md new file mode 100644 index 000000000000..a48f7e90dba2 --- /dev/null +++ b/.changeset/selfish-experts-develop.md @@ -0,0 +1,6 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/apps-engine": patch +--- + +Fixes issue with previously disabled private apps being auto enabled on update From f59d602d462b876368e1c79bb98f7ba9ef49aeb6 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 2 Oct 2024 00:50:32 -0300 Subject: [PATCH 4/8] add UI tests for EE --- .../e2e/apps/private-apps-upload.spec.ts | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts index 33325810456c..362a5e058893 100644 --- a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts +++ b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts @@ -32,6 +32,55 @@ test.describe.serial('Private apps upload', () => { await page.getByRole('button', { name: 'Agree' }).click(); await expect(poMarketplace.appStatusTag).toHaveText('Enabled'); }); + + test('expect to allow admin to update a enabled private app in EE, which should remain enabled', async ({ page }) => { + const fileChooserPromise = page.waitForEvent('filechooser'); + + await poMarketplace.btnUploadPrivateApp.click(); + await expect(poMarketplace.btnInstallPrivateApp).toBeDisabled(); + + await poMarketplace.btnUploadPrivateAppFile.click(); + const fileChooser = await fileChooserPromise; + await fileChooser.setFiles('./tests/e2e/fixtures/files/test-app_0.0.1.zip'); + + await expect(poMarketplace.btnInstallPrivateApp).toBeEnabled(); + await poMarketplace.btnInstallPrivateApp.click(); + await poMarketplace.btnConfirmAppUpdate.click(); + await page.getByRole('button', { name: 'Agree' }).click(); + + await page.goto('/marketplace/private'); + await poMarketplace.lastAppRow.click(); + await expect(poMarketplace.appStatusTag).toHaveText('Enabled'); + }); + + test('expect to allow disabling a recently installed private app in EE', async () => { + await poMarketplace.lastAppRow.click(); + await expect(poMarketplace.appStatusTag).toHaveText('Enabled'); + await poMarketplace.appMenu.click(); + await expect(poMarketplace.btnDisableApp).toBeEnabled(); + await poMarketplace.btnDisableApp.click(); + await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); + }); + + test('expect to allow admin to update a disabled private app in EE, which should remain disabled', async ({ page }) => { + const fileChooserPromise = page.waitForEvent('filechooser'); + + await poMarketplace.btnUploadPrivateApp.click(); + await expect(poMarketplace.btnInstallPrivateApp).toBeDisabled(); + + await poMarketplace.btnUploadPrivateAppFile.click(); + const fileChooser = await fileChooserPromise; + await fileChooser.setFiles('./tests/e2e/fixtures/files/test-app_0.0.1.zip'); + + await expect(poMarketplace.btnInstallPrivateApp).toBeEnabled(); + await poMarketplace.btnInstallPrivateApp.click(); + await poMarketplace.btnConfirmAppUpdate.click(); + await page.getByRole('button', { name: 'Agree' }).click(); + + await page.goto('/marketplace/private'); + await poMarketplace.lastAppRow.click(); + await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); + }); }); test.describe('Community Edition', () => { @@ -89,8 +138,8 @@ test.describe.serial('Private apps upload', () => { await poMarketplace.btnConfirmAppUpdate.click(); await page.getByRole('button', { name: 'Agree' }).click(); - await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); - await page.reload(); + await page.goto('/marketplace/private'); + await poMarketplace.lastAppRow.click(); await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); }); }); From 2ee31dc0807dba50c7f46ba373d04df7624ef52e Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 2 Oct 2024 17:52:21 -0300 Subject: [PATCH 5/8] fix typecheck errors --- apps/meteor/ee/server/apps/communication/websockets.ts | 3 ++- apps/meteor/server/services/apps-engine/service.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/meteor/ee/server/apps/communication/websockets.ts b/apps/meteor/ee/server/apps/communication/websockets.ts index 83a161427143..dbf894cb3eed 100644 --- a/apps/meteor/ee/server/apps/communication/websockets.ts +++ b/apps/meteor/ee/server/apps/communication/websockets.ts @@ -92,7 +92,8 @@ export class AppServerListener { const appPackage = await this.orch.getAppSourceStorage()!.fetch(storageItem); - await this.orch.getManager()!.updateLocal(storageItem, appPackage); + const isEnabled = AppStatusUtils.isEnabled(storageItem.status); + await this.orch.getManager()!.updateLocal(storageItem, appPackage, isEnabled); this.clientStreamer.emitWithoutBroadcast(AppEvents.APP_UPDATED, appId); } diff --git a/apps/meteor/server/services/apps-engine/service.ts b/apps/meteor/server/services/apps-engine/service.ts index 19838fd8411d..fb79b6c2ec49 100644 --- a/apps/meteor/server/services/apps-engine/service.ts +++ b/apps/meteor/server/services/apps-engine/service.ts @@ -62,7 +62,8 @@ export class AppsEngineService extends ServiceClassInternal implements IAppsEngi return; } - await Apps.self?.getManager()?.updateLocal(storageItem, appPackage); + const isEnabled = AppStatusUtils.isEnabled(storageItem.status); + await Apps.self?.getManager()?.updateLocal(storageItem, appPackage, isEnabled); }); this.onEvent('apps.statusUpdate', async (appId: string, status: AppStatus): Promise => { From a198a513a91e50cfedc42ff879d2001bb7a5abe7 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 2 Oct 2024 20:11:01 -0300 Subject: [PATCH 6/8] use old status as a reference for the new one --- packages/apps-engine/src/server/AppManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 4d4819a47478..81fa81d6addc 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -720,7 +720,7 @@ export class AppManager { aff.setApp(app); if (updateOptions.loadApp) { - const shouldEnableApp = AppStatusUtils.isEnabled(descriptor.status); + const shouldEnableApp = AppStatusUtils.isEnabled(old.status); await this.updateLocal(stored, app, shouldEnableApp); await this.bridges From 844a1b5a04f6d47ac3ba6ededc3277fda30d9d90 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 2 Oct 2024 20:11:19 -0300 Subject: [PATCH 7/8] update e2e UI tests to follow the new UI rules --- apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts index 362a5e058893..7350c500de6a 100644 --- a/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts +++ b/apps/meteor/tests/e2e/apps/private-apps-upload.spec.ts @@ -59,6 +59,7 @@ test.describe.serial('Private apps upload', () => { await poMarketplace.appMenu.click(); await expect(poMarketplace.btnDisableApp).toBeEnabled(); await poMarketplace.btnDisableApp.click(); + await poMarketplace.btnConfirmAppUpdate.click(); await expect(poMarketplace.appStatusTag).toHaveText('Disabled'); }); From 48ed6c6a221824de568654e782b0ef880b99c300 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 3 Oct 2024 11:37:36 -0300 Subject: [PATCH 8/8] split updateLocal method in 2 --- .../server/apps/communication/websockets.ts | 6 ++++- .../server/services/apps-engine/service.ts | 6 ++++- packages/apps-engine/src/server/AppManager.ts | 26 ++++++++++++------- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/apps/meteor/ee/server/apps/communication/websockets.ts b/apps/meteor/ee/server/apps/communication/websockets.ts index dbf894cb3eed..e6fc97464b8d 100644 --- a/apps/meteor/ee/server/apps/communication/websockets.ts +++ b/apps/meteor/ee/server/apps/communication/websockets.ts @@ -93,7 +93,11 @@ export class AppServerListener { const appPackage = await this.orch.getAppSourceStorage()!.fetch(storageItem); const isEnabled = AppStatusUtils.isEnabled(storageItem.status); - await this.orch.getManager()!.updateLocal(storageItem, appPackage, isEnabled); + if (isEnabled) { + await this.orch.getManager()!.updateAndStartupLocal(storageItem, appPackage); + } else { + await this.orch.getManager()!.updateAndInitializeLocal(storageItem, appPackage); + } this.clientStreamer.emitWithoutBroadcast(AppEvents.APP_UPDATED, appId); } diff --git a/apps/meteor/server/services/apps-engine/service.ts b/apps/meteor/server/services/apps-engine/service.ts index fb79b6c2ec49..486a78856394 100644 --- a/apps/meteor/server/services/apps-engine/service.ts +++ b/apps/meteor/server/services/apps-engine/service.ts @@ -63,7 +63,11 @@ export class AppsEngineService extends ServiceClassInternal implements IAppsEngi } const isEnabled = AppStatusUtils.isEnabled(storageItem.status); - await Apps.self?.getManager()?.updateLocal(storageItem, appPackage, isEnabled); + if (isEnabled) { + await Apps.self?.getManager()?.updateAndStartupLocal(storageItem, appPackage); + } else { + await Apps.self?.getManager()?.updateAndInitializeLocal(storageItem, appPackage); + } }); this.onEvent('apps.statusUpdate', async (appId: string, status: AppStatus): Promise => { diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 81fa81d6addc..60a1f97fcc74 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -721,7 +721,11 @@ export class AppManager { if (updateOptions.loadApp) { const shouldEnableApp = AppStatusUtils.isEnabled(old.status); - await this.updateLocal(stored, app, shouldEnableApp); + if (shouldEnableApp) { + await this.updateAndStartupLocal(stored, app); + } else { + await this.updateAndInitializeLocal(stored, app); + } await this.bridges .getAppActivationBridge() @@ -743,7 +747,7 @@ export class AppManager { * With an instance of a ProxiedApp, start it up and replace * the reference in the local app collection */ - public async updateLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer, enable: boolean) { + async updateLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer): Promise { const app = await (async () => { if (appPackageOrInstance instanceof Buffer) { const parseResult = await this.getParser().unpackageApp(appPackageOrInstance); @@ -762,15 +766,17 @@ export class AppManager { await this.purgeAppConfig(app, { keepScheduledJobs: true }); this.apps.set(app.getID(), app); + return app; + } - // Should enable === true, then we go through the entire start up process - // Otherwise, we only initialize it. - if (enable) { - // Start up the app - await this.runStartUpProcess(stored, app, false, true); - } else { - await this.initializeApp(stored, app, true, true); - } + public async updateAndStartupLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer) { + const app = await this.updateLocal(stored, appPackageOrInstance); + await this.runStartUpProcess(stored, app, false, true); + } + + public async updateAndInitializeLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer) { + const app = await this.updateLocal(stored, appPackageOrInstance); + await this.initializeApp(stored, app, true, true); } public getLanguageContent(): { [key: string]: object } {