fix: Setup wizard forced back into registration on upgrade when Show_Setup_Wizard is env-overridden

.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.

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));
 

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;
 			}
 

apps/meteor/server/startup/cloudRegistration.ts

@@ -3,15 +3,23 @@ import { Settings } from '@rocket.chat/models';
 export async function ensureCloudWorkspaceRegistered(): Promise<void> {
 	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) {
 		return;
 	}
 
 	// skip if the setup wizard still not completed
-	if (showSetupWizard !== 'completed') {
+	if (showSetupWizard?.value !== 'completed') {
+		return;
+	}
+
+	// skip if the value was pinned via env override (e.g. air-gapped deployments) and hasn't
+	// been changed since; forcing the wizard would fight the operator's explicit config
+	if (showSetupWizard.valueSource === 'processEnvValue' && showSetupWizard.value === showSetupWizard.processEnvValue) {
 		return;
 	}
 

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

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';
 

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<string, string | undefined>, showSetupWizard: Record<string, unknown> | 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;
+	});
+});