From 7de548bf62bd17da7da407a85e64cda1d012bb4c Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Mon, 6 Jul 2026 14:24:28 +0200 Subject: [PATCH] MM-69466: re-fetch admin config on config_changed to prevent concurrent save clobber (#37338) (#37355) (cherry picked from commit 68389fabbb182d246870dad0f1fe4e6f50d9da48) Co-authored-by: Jesse Hallam --- .../concurrent_config_save.spec.ts | 145 ++++++++++++++++++ .../admin_console/admin_console.tsx | 5 + .../common/hooks/useAdminConfigSync.test.ts | 96 ++++++++++++ .../common/hooks/useAdminConfigSync.ts | 40 +++++ 4 files changed, 286 insertions(+) create mode 100644 e2e-tests/playwright/specs/functional/system_console/concurrent_config_save.spec.ts create mode 100644 webapp/channels/src/components/common/hooks/useAdminConfigSync.test.ts create mode 100644 webapp/channels/src/components/common/hooks/useAdminConfigSync.ts diff --git a/e2e-tests/playwright/specs/functional/system_console/concurrent_config_save.spec.ts b/e2e-tests/playwright/specs/functional/system_console/concurrent_config_save.spec.ts new file mode 100644 index 00000000000..1384fb52f8e --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/concurrent_config_save.spec.ts @@ -0,0 +1,145 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import type {AdminConfig} from '@mattermost/types/config'; + +import {expect, test} from '@mattermost/playwright-lib'; + +// Wait for the GET /api/v4/config re-fetch that our config_changed websocket +// handler triggers. This is the signal that entities.admin.config in the +// browser's Redux store is now up-to-date with the server's current state. +async function waitForAdminConfigRefresh(page: import('@playwright/test').Page) { + return page.waitForResponse( + (resp) => resp.url().endsWith('/api/v4/config') && resp.request().method() === 'GET' && resp.status() === 200, + {timeout: 10000}, + ); +} + +// Each test case opens a System Console page, makes a parallel API change to a +// completely separate config section (simulating another admin), then saves the +// open page and asserts both changes survived. +test.describe('System Console > Concurrent config saves', () => { + test('Emoji page: parallel change to TeamSettings is not clobbered', async ({pw}) => { + const {adminUser, adminClient} = await pw.initSetup(); + + if (!adminUser) { + throw new Error('Failed to create admin user'); + } + + // # Set a known baseline for both sections under test + await adminClient.patchConfig({ + ServiceSettings: {EnableCustomEmoji: false}, + TeamSettings: {MaxUsersPerTeam: 50}, + } as Partial); + + // # Open the Emoji settings page + const {page} = await pw.testBrowser.login(adminUser); + await page.goto('/admin_console/site_config/emoji'); + + const emojiSection = page.getByTestId('sysconsole_section_EmojiSettings'); + await expect(emojiSection).toBeVisible(); + const saveButton = emojiSection.getByRole('button', {name: 'Save'}); + + // # Simulate another admin saving an unrelated section via API. + // Set up the response listener before triggering the change so we don't + // miss the GET /api/v4/config that our config_changed handler dispatches. + const configRefresh = waitForAdminConfigRefresh(page); + await adminClient.patchConfig({TeamSettings: {MaxUsersPerTeam: 99}} as Partial); + + // # Wait for the browser to re-fetch the admin config before we save, + // confirming that this.props.config is now fresh. Then assert the + // toggle's current state to confirm the Redux store update has + // propagated to the UI before we interact. + await configRefresh; + const emojiToggle = emojiSection.getByTestId('ServiceSettings.EnableCustomEmojitrue'); + await expect(emojiToggle).not.toBeChecked(); + + // # Change a setting on the open Emoji page and save + await emojiToggle.click(); + await saveButton.click(); + await pw.waitUntil(async () => (await saveButton.textContent()) === 'Save'); + + // * Both the UI change and the parallel API change must be present + const finalConfig = await adminClient.getConfig(); + expect(finalConfig.ServiceSettings.EnableCustomEmoji).toBe(true); + expect(finalConfig.TeamSettings.MaxUsersPerTeam).toBe(99); + }); + + test('Announcement Banner page: parallel change to ServiceSettings is not clobbered', async ({pw}) => { + const {adminUser, adminClient} = await pw.initSetup(); + + if (!adminUser) { + throw new Error('Failed to create admin user'); + } + + // # Set a known baseline for both sections under test + await adminClient.patchConfig({ + AnnouncementSettings: {EnableBanner: false}, + ServiceSettings: {EnableCustomEmoji: false}, + } as Partial); + + // # Open the Announcement Banner settings page + const {page} = await pw.testBrowser.login(adminUser); + await page.goto('/admin_console/site_config/announcement_banner'); + + const bannerSection = page.getByTestId('sysconsole_section_AnnouncementSettings'); + await expect(bannerSection).toBeVisible(); + const saveButton = bannerSection.getByRole('button', {name: 'Save'}); + + // # Simulate another admin saving an unrelated section via API + const configRefresh = waitForAdminConfigRefresh(page); + await adminClient.patchConfig({ServiceSettings: {EnableCustomEmoji: true}} as Partial); + await configRefresh; + const bannerToggle = bannerSection.getByTestId('AnnouncementSettings.EnableBannertrue'); + await expect(bannerToggle).not.toBeChecked(); + + // # Change a setting on the open Announcement Banner page and save + await bannerToggle.click(); + await saveButton.click(); + await pw.waitUntil(async () => (await saveButton.textContent()) === 'Save'); + + // * Both changes must be present + const finalConfig = await adminClient.getConfig(); + expect(finalConfig.AnnouncementSettings.EnableBanner).toBe(true); + expect(finalConfig.ServiceSettings.EnableCustomEmoji).toBe(true); + }); + + test('Users and Teams page: parallel change to EmailSettings is not clobbered', async ({pw}) => { + const {adminUser, adminClient} = await pw.initSetup(); + + if (!adminUser) { + throw new Error('Failed to create admin user'); + } + + // # Set a known baseline for both sections under test + await adminClient.patchConfig({ + TeamSettings: {EnableJoinLeaveMessageByDefault: false}, + EmailSettings: {EnableSignUpWithEmail: true}, + } as Partial); + + // # Open the Users and Teams settings page + const {page} = await pw.testBrowser.login(adminUser); + await page.goto('/admin_console/site_config/users_and_teams'); + + const teamSection = page.getByTestId('sysconsole_section_UserAndTeamsSettings'); + await expect(teamSection).toBeVisible(); + const saveButton = teamSection.getByRole('button', {name: 'Save'}); + + // # Simulate another admin saving EmailSettings via API + const configRefresh = waitForAdminConfigRefresh(page); + await adminClient.patchConfig({EmailSettings: {EnableSignUpWithEmail: false}} as Partial); + await configRefresh; + const joinLeaveToggle = teamSection.getByTestId('TeamSettings.EnableJoinLeaveMessageByDefaulttrue'); + await expect(joinLeaveToggle).not.toBeChecked(); + + // # Toggle Enable Join/Leave messages on the Users and Teams page and save + await joinLeaveToggle.click(); + await saveButton.click(); + await pw.waitUntil(async () => (await saveButton.textContent()) === 'Save'); + + // * Both changes must be present + const finalConfig = await adminClient.getConfig(); + expect(finalConfig.TeamSettings.EnableJoinLeaveMessageByDefault).toBe(true); + expect(finalConfig.EmailSettings.EnableSignUpWithEmail).toBe(false); + }); +}); diff --git a/webapp/channels/src/components/admin_console/admin_console.tsx b/webapp/channels/src/components/admin_console/admin_console.tsx index 32661f389b8..da08bee4c3b 100644 --- a/webapp/channels/src/components/admin_console/admin_console.tsx +++ b/webapp/channels/src/components/admin_console/admin_console.tsx @@ -18,6 +18,7 @@ import SchemaAdminSettings from 'components/admin_console/schema_admin_settings' import SearchKeywordMarking from 'components/admin_console/search_keyword_marking'; import AnnouncementBarController from 'components/announcement_bar'; import BackstageNavbar from 'components/backstage/components/backstage_navbar'; +import useAdminConfigSync from 'components/common/hooks/useAdminConfigSync'; import DiscardChangesModal from 'components/discard_changes_modal'; import GlobalClassificationBanner from 'components/global_classification_banner'; import ModalController from 'components/modal_controller'; @@ -87,6 +88,10 @@ const AdminConsole = (props: Props) => { const [search, setSearch] = useState(''); const handleFocusScroller = useFocusScroller(props.location); + // Keep the config fresh while the System Console is open so that concurrent + // saves by different admins do not clobber each other. + useAdminConfigSync(); + useEffect(() => { props.actions.getConfig(); props.actions.getEnvironmentConfig(); diff --git a/webapp/channels/src/components/common/hooks/useAdminConfigSync.test.ts b/webapp/channels/src/components/common/hooks/useAdminConfigSync.test.ts new file mode 100644 index 00000000000..1bb3e9bbf15 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/useAdminConfigSync.test.ts @@ -0,0 +1,96 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {act} from '@testing-library/react'; +import * as ReactRedux from 'react-redux'; + +import type {WebSocketMessage} from '@mattermost/client'; +import {WebSocketEvents} from '@mattermost/client'; + +import {getConfig} from 'mattermost-redux/actions/admin'; + +import {renderHookWithContext} from 'tests/react_testing_utils'; +import * as webSocketHooks from 'utils/use_websocket/hooks'; + +import useAdminConfigSync from './useAdminConfigSync'; + +jest.mock('mattermost-redux/actions/admin', () => ({ + ...jest.requireActual('mattermost-redux/actions/admin'), + getConfig: jest.fn(() => ({type: 'MOCK_GET_ADMIN_CONFIG'})), +})); + +jest.mock('utils/use_websocket/hooks', () => ({ + useWebSocket: jest.fn(), + useWebSocketClient: jest.fn(), +})); + +describe('useAdminConfigSync', () => { + const dispatchMock = jest.fn(); + const addReconnectListener = jest.fn(); + const removeReconnectListener = jest.fn(); + + let messageHandler: (msg: WebSocketMessage) => void; + + beforeEach(() => { + jest.spyOn(ReactRedux, 'useDispatch').mockReturnValue(dispatchMock); + + (webSocketHooks.useWebSocket as jest.Mock).mockImplementation(({handler}) => { + messageHandler = handler; + }); + (webSocketHooks.useWebSocketClient as jest.Mock).mockReturnValue({ + addReconnectListener, + removeReconnectListener, + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + (getConfig as jest.Mock).mockClear(); + dispatchMock.mockClear(); + addReconnectListener.mockClear(); + removeReconnectListener.mockClear(); + }); + + test('dispatches getConfig on a config_changed event', () => { + renderHookWithContext(useAdminConfigSync); + + act(() => { + messageHandler({event: WebSocketEvents.ConfigChanged} as WebSocketMessage); + }); + + expect(getConfig).toHaveBeenCalledTimes(1); + expect(dispatchMock).toHaveBeenCalledWith({type: 'MOCK_GET_ADMIN_CONFIG'}); + }); + + test('ignores other websocket events', () => { + renderHookWithContext(useAdminConfigSync); + + act(() => { + messageHandler({event: WebSocketEvents.Posted} as WebSocketMessage); + }); + + expect(getConfig).not.toHaveBeenCalled(); + }); + + test('refetches on websocket reconnect', () => { + renderHookWithContext(useAdminConfigSync); + + expect(addReconnectListener).toHaveBeenCalledTimes(1); + const reconnectListener = addReconnectListener.mock.calls[0][0]; + + act(() => { + reconnectListener(); + }); + + expect(getConfig).toHaveBeenCalledTimes(1); + }); + + test('removes the reconnect listener on unmount', () => { + const {unmount} = renderHookWithContext(useAdminConfigSync); + + act(unmount); + + expect(removeReconnectListener).toHaveBeenCalledTimes(1); + expect(removeReconnectListener).toHaveBeenCalledWith(addReconnectListener.mock.calls[0][0]); + }); +}); diff --git a/webapp/channels/src/components/common/hooks/useAdminConfigSync.ts b/webapp/channels/src/components/common/hooks/useAdminConfigSync.ts new file mode 100644 index 00000000000..20fd4ac5151 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/useAdminConfigSync.ts @@ -0,0 +1,40 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {useCallback, useEffect} from 'react'; +import {useDispatch} from 'react-redux'; + +import type {WebSocketMessage} from '@mattermost/client'; +import {WebSocketEvents} from '@mattermost/client'; + +import {getConfig} from 'mattermost-redux/actions/admin'; + +import {useWebSocket, useWebSocketClient} from 'utils/use_websocket/hooks'; + +/** + * Subscribes to config_changed WebSocket events and reconnects, re-fetching + * the full admin config each time so the in-memory state stays current. + */ +export default function useAdminConfigSync() { + const dispatch = useDispatch(); + const wsClient = useWebSocketClient(); + + const refetch = useCallback(() => { + dispatch(getConfig()); + }, [dispatch]); + + const handleWebSocketMessage = useCallback((msg: WebSocketMessage) => { + if (msg.event === WebSocketEvents.ConfigChanged) { + refetch(); + } + }, [refetch]); + + useWebSocket({handler: handleWebSocketMessage}); + + useEffect(() => { + wsClient.addReconnectListener(refetch); + return () => { + wsClient.removeReconnectListener(refetch); + }; + }, [wsClient, refetch]); +}