diff --git a/.changeset/setup-wizard-env-override-upgrade.md b/.changeset/setup-wizard-env-override-upgrade.md new file mode 100644 index 0000000000000..5b42fd6ceb472 --- /dev/null +++ b/.changeset/setup-wizard-env-override-upgrade.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes the setup wizard being forced back into the registration step on the first start after an upgrade when `OVERWRITE_SETTING_Show_Setup_Wizard=completed` is set, which affected air-gapped workspaces running offline licenses without cloud registration. diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 3b93ca5c0bfb6..18c0906774fd0 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -83,6 +83,11 @@ export const compareSettings = compareSettingsIgnoringKeys([ '_updatedAt', ]); +const hasOverwrittenValueChanged = (stored: ISetting, overwritten: ISetting): boolean => + !isEqual(stored.value, overwritten.value) || + stored.valueSource !== overwritten.valueSource || + !isEqual(stored.processEnvValue, overwritten.processEnvValue); + export class SettingsRegistry { private model: ISettingsModel; @@ -175,7 +180,7 @@ export class SettingsRegistry { } if (settingStored && isOverwritten) { - if (settingStored.value !== settingFromCodeOverwritten.value) { + if (hasOverwrittenValueChanged(settingStored, settingFromCodeOverwritten)) { const overwrittenKeys = Object.keys(settingFromCodeOverwritten); const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key)); diff --git a/apps/meteor/app/settings/server/functions/overrideGenerator.ts b/apps/meteor/app/settings/server/functions/overrideGenerator.ts index bd1b62b6d60cf..7e62385b11fec 100644 --- a/apps/meteor/app/settings/server/functions/overrideGenerator.ts +++ b/apps/meteor/app/settings/server/functions/overrideGenerator.ts @@ -40,7 +40,7 @@ export const overrideGenerator = try { const value = convertValue(overwriteValue, setting.type); - if (compareSettingsValue(value, setting.value, setting.type)) { + if (compareSettingsValue(value, setting.value, setting.type) && compareSettingsValue(value, setting.processEnvValue, setting.type)) { return setting; } diff --git a/apps/meteor/server/startup/cloudRegistration.ts b/apps/meteor/server/startup/cloudRegistration.ts index e69d4446d6ec2..9acc72e339bf7 100644 --- a/apps/meteor/server/startup/cloudRegistration.ts +++ b/apps/meteor/server/startup/cloudRegistration.ts @@ -3,7 +3,9 @@ import { Settings } from '@rocket.chat/models'; export async function ensureCloudWorkspaceRegistered(): Promise { const cloudWorkspaceClientId = await Settings.getValueById('Cloud_Workspace_Client_Id'); const cloudWorkspaceClientSecret = await Settings.getValueById('Cloud_Workspace_Client_Secret'); - const showSetupWizard = await Settings.getValueById('Show_Setup_Wizard'); + const showSetupWizard = await Settings.findOneById('Show_Setup_Wizard', { + projection: { value: 1, valueSource: 1, processEnvValue: 1 }, + }); // skip if both fields are already set, which means the workspace is already registered if (!!cloudWorkspaceClientId && !!cloudWorkspaceClientSecret) { @@ -11,7 +13,11 @@ export async function ensureCloudWorkspaceRegistered(): Promise { } // skip if the setup wizard still not completed - if (showSetupWizard !== 'completed') { + if (showSetupWizard?.value !== 'completed') { + return; + } + + if (showSetupWizard.valueSource === 'processEnvValue' && showSetupWizard.value === showSetupWizard.processEnvValue) { return; } diff --git a/apps/meteor/tests/unit/app/settings/server/functions/overrideGenerator.tests.ts b/apps/meteor/tests/unit/app/settings/server/functions/overrideGenerator.tests.ts index b68c478a94275..f50f5d966c24c 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/overrideGenerator.tests.ts +++ b/apps/meteor/tests/unit/app/settings/server/functions/overrideGenerator.tests.ts @@ -19,12 +19,28 @@ describe('overrideGenerator', () => { expect(overwritten).to.have.property('valueSource').that.equals('processEnvValue'); }); - it('should return the same object since the value didnt change', () => { + it('should stamp the env source even when the value didnt change', () => { const overwrite = overrideGenerator(() => 'test'); const setting = getSettingDefaults({ _id: 'test', value: 'test', type: 'string' }); const overwritten = overwrite(setting); + expect(setting).to.be.not.equal(overwritten); + expect(overwritten).to.have.property('value').that.equals('test'); + expect(overwritten).to.have.property('valueSource').that.equals('processEnvValue'); + expect(overwritten).to.have.property('processEnvValue').that.equals('test'); + }); + + it('should return the same object when the value and the env stamp are already in place', () => { + const overwrite = overrideGenerator(() => 'test'); + + const setting = { + ...getSettingDefaults({ _id: 'test', value: 'test', type: 'string' }), + valueSource: 'processEnvValue' as const, + processEnvValue: 'test', + }; + const overwritten = overwrite(setting); + expect(setting).to.be.equal(overwritten); }); }); diff --git a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts b/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts index 9775e36da035d..8df6b4d4fdd20 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts +++ b/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts @@ -141,6 +141,50 @@ describe('Settings', () => { }); }); + it('should stamp the env source when the override equals the stored value', async () => { + const addSetting = (registry: SettingsRegistry) => + registry.addGroup('group', async function () { + await this.section('section', async function () { + await this.add('my_setting_stamped', 0, { + type: 'int', + sorter: 0, + }); + }); + }); + + const bootRegistry = () => { + const settings = new CachedSettings(); + Settings.settings = settings; + for (const _id of ['group', 'my_setting_stamped']) { + const stored = Settings.findOne({ _id }); + if (stored) { + settings.set(stored); + } + } + settings.initialized(); + return new SettingsRegistry({ store: settings, model: Settings as any }); + }; + + await addSetting(bootRegistry()); + + expect(Settings.findOne({ _id: 'my_setting_stamped' })).to.include({ value: 0, valueSource: 'packageValue' }); + + process.env.OVERWRITE_SETTING_my_setting_stamped = '0'; + + await addSetting(bootRegistry()); + + expect(Settings).to.have.property('upsertCalls').to.be.equal(1); + expect(Settings.findOne({ _id: 'my_setting_stamped' })).to.include({ + value: 0, + processEnvValue: 0, + valueSource: 'processEnvValue', + }); + + await addSetting(bootRegistry()); + + expect(Settings).to.have.property('upsertCalls').to.be.equal(1); + }); + it('should respect override via environment as boolean', async () => { process.env.OVERWRITE_SETTING_my_setting_bool = 'true'; diff --git a/apps/meteor/tests/unit/server/startup/cloudRegistration.tests.ts b/apps/meteor/tests/unit/server/startup/cloudRegistration.tests.ts new file mode 100644 index 0000000000000..5e4df50d130e5 --- /dev/null +++ b/apps/meteor/tests/unit/server/startup/cloudRegistration.tests.ts @@ -0,0 +1,75 @@ +import { expect } from 'chai'; +import { beforeEach, describe, it } from 'mocha'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; + +const models = { + Settings: { + getValueById: sinon.stub(), + findOneById: sinon.stub(), + updateValueById: sinon.stub(), + }, +}; + +const { ensureCloudWorkspaceRegistered } = proxyquire.noCallThru().load('../../../../server/startup/cloudRegistration', { + '@rocket.chat/models': models, +}); + +describe('ensureCloudWorkspaceRegistered', () => { + const stubSettings = (values: Record, showSetupWizard: Record | null) => { + models.Settings.getValueById.callsFake(async (id: string) => values[id]); + models.Settings.findOneById.resolves(showSetupWizard); + }; + + beforeEach(() => { + models.Settings.getValueById.reset(); + models.Settings.findOneById.reset(); + models.Settings.updateValueById.reset(); + }); + + it('should not touch the setting when its value was pinned via env override and is unchanged', async () => { + stubSettings({}, { value: 'completed', valueSource: 'processEnvValue', processEnvValue: 'completed' }); + + await ensureCloudWorkspaceRegistered(); + + expect(models.Settings.updateValueById.called).to.be.false; + }); + + it('should flip the setting when the value diverged from the env override', async () => { + stubSettings({}, { value: 'completed', valueSource: 'processEnvValue', processEnvValue: 'pending' }); + + await ensureCloudWorkspaceRegistered(); + + expect(models.Settings.updateValueById.calledOnceWith('Show_Setup_Wizard', 'in_progress')).to.be.true; + }); + + it('should not touch the setting when the workspace is already registered', async () => { + stubSettings( + { + Cloud_Workspace_Client_Id: 'client-id', + Cloud_Workspace_Client_Secret: 'client-secret', + }, + { value: 'completed', valueSource: 'packageValue' }, + ); + + await ensureCloudWorkspaceRegistered(); + + expect(models.Settings.updateValueById.called).to.be.false; + }); + + it('should not touch the setting when the setup wizard is not completed', async () => { + stubSettings({}, { value: 'in_progress', valueSource: 'packageValue' }); + + await ensureCloudWorkspaceRegistered(); + + expect(models.Settings.updateValueById.called).to.be.false; + }); + + it('should flip the setup wizard to in_progress when unregistered, completed and not env-pinned', async () => { + stubSettings({}, { value: 'completed', valueSource: 'packageValue' }); + + await ensureCloudWorkspaceRegistered(); + + expect(models.Settings.updateValueById.calledOnceWith('Show_Setup_Wizard', 'in_progress')).to.be.true; + }); +});