From d6b9e2aa8a6a227617a6cd529997e8ab8569dd8d Mon Sep 17 00:00:00 2001 From: David Langley Date: Fri, 14 Jun 2024 12:00:30 +0100 Subject: [PATCH] Fix config override of other settings levels (#12593) * Make config override other settings levels and add tests * fix documentation * lint * Use a const for finalLevel. * respect the explicit parameter * Use supportedLevelsAreOrdered for config overrides rather than a separate setting. * Fix typos * Fix mock in UserSetttingsDialog-test * Special case disabling of setting tos use config overrides. * remove logs --- .../views/elements/SettingsFlag.tsx | 7 ++ src/settings/Settings.tsx | 109 ++++++++---------- src/settings/SettingsStore.ts | 45 ++++++-- .../views/dialogs/UserSettingsDialog-test.tsx | 1 + test/settings/SettingsStore-test.ts | 26 +++++ 5 files changed, 118 insertions(+), 70 deletions(-) diff --git a/src/components/views/elements/SettingsFlag.tsx b/src/components/views/elements/SettingsFlag.tsx index d44a3323b7..86b82c64e4 100644 --- a/src/components/views/elements/SettingsFlag.tsx +++ b/src/components/views/elements/SettingsFlag.tsx @@ -62,6 +62,13 @@ export default class SettingsFlag extends React.Component { } private getSettingValue(): boolean { + // If a level defined in props is overridden by a level at a high presedence, it gets disabled + // and we should show the overridding value. + if ( + SettingsStore.settingIsOveriddenAtConfigLevel(this.props.name, this.props.roomId ?? null, this.props.level) + ) { + return !!SettingsStore.getValue(this.props.name); + } return !!SettingsStore.getValueAt( this.props.level, this.props.name, diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 3650e51814..fe8cd3fccc 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -71,6 +71,7 @@ const LEVELS_ROOM_SETTINGS_WITH_ROOM = [ const LEVELS_ACCOUNT_SETTINGS = [SettingLevel.DEVICE, SettingLevel.ACCOUNT, SettingLevel.CONFIG]; const LEVELS_DEVICE_ONLY_SETTINGS = [SettingLevel.DEVICE]; const LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG = [SettingLevel.DEVICE, SettingLevel.CONFIG]; +const LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED = [SettingLevel.CONFIG, SettingLevel.DEVICE]; const LEVELS_UI_FEATURE = [ SettingLevel.CONFIG, // in future we might have a .well-known level or something @@ -133,17 +134,6 @@ export type SettingValueType = export interface IBaseSetting { isFeature?: false | undefined; - /** - * If true, then the presence of this setting in `config.json` will disable the option in the UI. - * - * In other words, we prevent the user overriding the setting if an explicit value is given in `config.json`. - * XXX: note that users who have already set a non-default value before `config.json` is update will continue - * to use that value (and, indeed, won't be able to change it!): https://github.com/element-hq/element-web/issues/26877 - * - * Obviously, this only really makes sense if `supportedLevels` includes {@link SettingLevel.CONFIG}. - */ - configDisablesSetting?: true; - // Display names are strongly recommended for clarity. // Display name can also be an object for different levels. displayName?: @@ -268,70 +258,70 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_msc3531_hide_messages_pending_moderation": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, // Requires a reload since this setting is cached in EventUtils controller: new ReloadOnChangeController(), displayName: _td("labs|msc3531_hide_messages_pending_moderation"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_report_to_moderators": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, displayName: _td("labs|report_to_moderators"), description: _td("labs|report_to_moderators_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_latex_maths": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|latex_maths"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_pinning": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|pinning"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_wysiwyg_composer": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|wysiwyg_composer"), description: _td("labs|feature_wysiwyg_composer_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_mjolnir": { isFeature: true, labsGroup: LabGroup.Moderation, - configDisablesSetting: true, displayName: _td("labs|mjolnir"), description: _td("labs|currently_experimental"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_custom_themes": { isFeature: true, labsGroup: LabGroup.Themes, - configDisablesSetting: true, displayName: _td("labs|custom_themes"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "feature_dehydration": { isFeature: true, labsGroup: LabGroup.Encryption, - configDisablesSetting: true, displayName: _td("labs|dehydration"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "useOnlyCurrentProfiles": { @@ -350,25 +340,25 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_html_topic": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|html_topic"), default: false, }, "feature_bridge_state": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|bridge_state"), default: false, }, "feature_jump_to_date": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|jump_to_date"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, controller: new ServerSupportUnstableFeatureController( "feature_jump_to_date", @@ -398,8 +388,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_sliding_sync": { isFeature: true, labsGroup: LabGroup.Developer, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|sliding_sync"), description: _td("labs|sliding_sync_description"), shouldWarn: true, @@ -414,8 +404,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_element_call_video_rooms": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|element_call_video_rooms"), controller: new ReloadOnChangeController(), default: false, @@ -423,8 +413,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_group_calls": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|group_calls"), controller: new ReloadOnChangeController(), default: false, @@ -432,16 +422,16 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_disable_call_per_sender_encryption": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|feature_disable_call_per_sender_encryption"), default: false, }, "feature_allow_screen_share_only_mode": { isFeature: true, labsGroup: LabGroup.VoiceAndVideo, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, description: _td("labs|under_active_development"), displayName: _td("labs|allow_screen_share_only_mode"), controller: new ReloadOnChangeController(), @@ -450,8 +440,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_location_share_live": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|location_share_live"), description: _td("labs|location_share_live_description"), shouldWarn: true, @@ -460,8 +450,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_dynamic_room_predecessors": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|dynamic_room_predecessors"), description: _td("labs|dynamic_room_predecessors_description"), shouldWarn: true, @@ -470,8 +460,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.VoiceBroadcast]: { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|voice_broadcast"), default: false, }, @@ -483,8 +473,8 @@ export const SETTINGS: { [setting: string]: ISetting } = { [Features.OidcNativeFlow]: { isFeature: true, labsGroup: LabGroup.Developer, - configDisablesSetting: true, - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, displayName: _td("labs|oidc_native_flow"), description: _td("labs|oidc_native_flow_description"), default: false, @@ -493,7 +483,6 @@ export const SETTINGS: { [setting: string]: ISetting } = { // use the rust matrix-sdk-crypto-wasm for crypto. isFeature: true, labsGroup: LabGroup.Developer, - // unlike most features, `configDisablesSetting` is false here. supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, displayName: _td("labs|rust_crypto"), description: () => { @@ -527,10 +516,10 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_render_reaction_images": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|render_reaction_images"), description: _td("labs|render_reaction_images_description"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, /** @@ -609,28 +598,28 @@ export const SETTINGS: { [setting: string]: ISetting } = { "feature_ask_to_join": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, default: false, displayName: _td("labs|ask_to_join"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, }, "feature_new_room_decoration_ui": { isFeature: true, labsGroup: LabGroup.Rooms, - configDisablesSetting: true, displayName: _td("labs|new_room_decoration_ui"), description: _td("labs|under_active_development"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, controller: new ReloadOnChangeController(), }, "feature_notifications": { isFeature: true, labsGroup: LabGroup.Messaging, - configDisablesSetting: true, displayName: _td("labs|notifications"), description: _td("labs|unrealiable_e2e"), - supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, default: false, }, "useCompactLayout": { diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 7b1b6493d0..2afe03424c 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -505,8 +505,8 @@ export default class SettingsStore { * set for a particular room, otherwise it should be supplied. * * This takes into account both the value of {@link SettingController#settingDisabled} of the - * `SettingController`, if any; and, for settings where {@link IBaseSetting#configDisablesSetting} is true, - * whether the setting has been given a value in `config.json`. + * `SettingController`, if any; and, for settings where {@link IBaseSetting#supportedLevelsAreOrdered} is true, + * checks whether a level of higher precedence is set. * * Typically, if the user cannot set the setting, it should be hidden, to declutter the UI; * however some settings (typically, the labs flags) are exposed but greyed out, to unveil @@ -514,25 +514,27 @@ export default class SettingsStore { * * @param {string} settingName The name of the setting to check. * @param {String} roomId The room ID to check in, may be null. - * @param {SettingLevel} level The level to - * check at. + * @param {SettingLevel} level The level to check at. * @return {boolean} True if the user may set the setting, false otherwise. */ public static canSetValue(settingName: string, roomId: string | null, level: SettingLevel): boolean { + const setting = SETTINGS[settingName]; // Verify that the setting is actually a setting - if (!SETTINGS[settingName]) { + if (!setting) { throw new Error("Setting '" + settingName + "' does not appear to be a setting."); } - if (SETTINGS[settingName].controller?.settingDisabled) { + if (setting.controller?.settingDisabled) { return false; } // For some config settings (mostly: non-beta features), a value in config.json overrides the local setting - // (ie: we force them as enabled or disabled). - if (SETTINGS[settingName]?.configDisablesSetting) { - const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); - if (configVal === true || configVal === false) return false; + // (ie: we force them as enabled or disabled). In this case we should not let the user change the setting. + if ( + setting?.supportedLevelsAreOrdered && + SettingsStore.settingIsOveriddenAtConfigLevel(settingName, roomId, level) + ) { + return false; } const handler = SettingsStore.getHandler(settingName, level); @@ -540,6 +542,29 @@ export default class SettingsStore { return handler.canSetValue(settingName, roomId); } + /** + * Determines if the setting at the specified level is overidden by one at a config level. + * @param settingName The name of the setting to check. + * @param roomId The room ID to check in, may be null. + * @param level The level to check at. + * @returns + */ + public static settingIsOveriddenAtConfigLevel( + settingName: string, + roomId: string | null, + level: SettingLevel, + ): boolean { + const setting = SETTINGS[settingName]; + const levelOrders = getLevelOrder(setting); + const configIndex = levelOrders.indexOf(SettingLevel.CONFIG); + const levelIndex = levelOrders.indexOf(level); + if (configIndex === -1 || levelIndex === -1 || configIndex >= levelIndex) { + return false; + } + const configVal = SettingsStore.getValueAt(SettingLevel.CONFIG, settingName, roomId, true, true); + return configVal === true || configVal === false; + } + /** * Determines if the given level is supported on this device. * @param {SettingLevel} level The level diff --git a/test/components/views/dialogs/UserSettingsDialog-test.tsx b/test/components/views/dialogs/UserSettingsDialog-test.tsx index 72232d5e1b..f404b7f208 100644 --- a/test/components/views/dialogs/UserSettingsDialog-test.tsx +++ b/test/components/views/dialogs/UserSettingsDialog-test.tsx @@ -54,6 +54,7 @@ jest.mock("../../../../src/settings/SettingsStore", () => ({ getDescription: jest.fn(), shouldHaveWarning: jest.fn(), disabledMessage: jest.fn(), + settingIsOveriddenAtConfigLevel: jest.fn(), })); jest.mock("../../../../src/SdkConfig", () => ({ diff --git a/test/settings/SettingsStore-test.ts b/test/settings/SettingsStore-test.ts index 25f055d9b5..e1c27ab309 100644 --- a/test/settings/SettingsStore-test.ts +++ b/test/settings/SettingsStore-test.ts @@ -15,6 +15,7 @@ limitations under the License. */ import BasePlatform from "../../src/BasePlatform"; +import SdkConfig from "../../src/SdkConfig"; import { SettingLevel } from "../../src/settings/SettingLevel"; import SettingsStore from "../../src/settings/SettingsStore"; import { mockPlatformPeg } from "../test-utils"; @@ -27,6 +28,11 @@ const TEST_DATA = [ }, ]; +/** + * An existing setting that has {@link IBaseSetting#supportedLevelsAreOrdered} set to true. + */ +const SETTING_NAME_WITH_CONFIG_OVERRIDE = "feature_new_room_decoration_ui"; + describe("SettingsStore", () => { let platformSettings: Record; @@ -42,6 +48,7 @@ describe("SettingsStore", () => { getSettingValue: jest.fn().mockImplementation((settingName: string) => { return platformSettings[settingName]; }), + reload: jest.fn(), } as unknown as BasePlatform); TEST_DATA.forEach((d) => { @@ -49,6 +56,10 @@ describe("SettingsStore", () => { }); }); + beforeEach(() => { + SdkConfig.reset(); + }); + describe("getValueAt", () => { TEST_DATA.forEach((d) => { it(`should return the value "${d.level}"."${d.name}"`, () => { @@ -57,5 +68,20 @@ describe("SettingsStore", () => { expect(SettingsStore.getValueAt(d.level, d.name)).toBe(d.value); }); }); + + it(`supportedLevelsAreOrdered correctly overrides setting`, async () => { + SdkConfig.put({ + features: { + [SETTING_NAME_WITH_CONFIG_OVERRIDE]: false, + }, + }); + await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); + expect(SettingsStore.getValue(SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(false); + }); + + it(`supportedLevelsAreOrdered doesn't incorrectly override setting`, async () => { + await SettingsStore.setValue(SETTING_NAME_WITH_CONFIG_OVERRIDE, null, SettingLevel.DEVICE, true); + expect(SettingsStore.getValueAt(SettingLevel.DEVICE, SETTING_NAME_WITH_CONFIG_OVERRIDE)).toBe(true); + }); }); });