Fix stale channel members RHS list after ABAC access-rule member removal (#36964)

* Fix stale channel members RHS list after ABAC member removal The channel members RHS list is built from the profilesInChannel and membersInChannel Redux stores, which are only pruned by the user_removed websocket handler when the affected channel is currently focused. When a removal is not handled over the websocket (e.g. an ABAC access-rule change processed while another channel is focused, or a missed event), reopening the members list performs an additive reload that never prunes removed members, so the list stays stale even though the member count is refreshed separately via getChannelStats. Make the first-page members reload authoritative: reconcile the channel member stores against the server response and prune members the server no longer returns. Scoped to the initial full-membership page to avoid pruning members that belong to later pages. Co-authored-by: mattermost-code <matty-code@mattermost.com> * Add tests for channel members RHS reconcile on reload Covers pruning members the server no longer returns on the first page, and the guards that prevent pruning when reconcile is disabled, on a full page (more pages may follow), or on subsequent pages. Co-authored-by: mattermost-code <matty-code@mattermost.com> * Strengthen channel members reconcile tests Cover pruning across both the member and profile stores, multiple stale members, the no-false-positive case (present members are never pruned), and assert the members RHS passes reconcile=true on its first-page mount reload (but not on pagination). Co-authored-by: mattermost-code <matty-code@mattermost.com> * Tighten reconcile tests with exact-set and perPage guard Co-authored-by: mattermost-code <matty-code@mattermost.com> * Tighten reconcile comment Co-authored-by: mattermost-code <matty-code@mattermost.com> * Add ABAC channel members RHS e2e coverage * Stabilize ABAC RHS e2e sync job wait --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: mattermost-code <matty-code@mattermost.com>
2026-09-24 16:05:00 +08:00 · 2026-06-11 07:38:18 -04:00
parent 471fd8d1dd
commit 6583982b26
5 changed files with 282 additions and 3 deletions
@@ -22,6 +22,8 @@ import {
    waitForAttributeViewToInclude,
 } from '../team_settings/helpers';

+import {waitForJobCompletion} from './helpers';
+
 /** Unique CPA value so only users this test sets match the rule (avoids clashing with leftover Engineering users on the server). */
 function uniqueDepartmentValue(testId: string): string {
    return `E2E-${testId}-${Date.now()}-${Math.random().toString(36).slice(2, 9)}`;
@@ -557,4 +559,112 @@ test.describe('Channel Settings Modal - Access Control Tab', () => {

        await channelSettings.close();
    });
+
+    test('MM-67326_c14 Applying access rules removes non-matching member from channel and RHS members', async ({
+        pw,
+    }) => {
+        await pw.skipIfNoLicense();
+        const {adminUser, adminClient, team} = await pw.initSetup();
+        await enableABACConfig(adminClient);
+        await ensureDepartmentAttribute(adminClient);
+
+        const departmentValue = uniqueDepartmentValue('c14');
+
+        // # Admin satisfies the rule
+        await setUserAttribute(adminClient, adminUser.id, 'Department', departmentValue);
+
+        const channel = await createPrivateChannel(adminClient, team.id);
+
+        // # Existing member does not satisfy the rule and should be removed when it is applied
+        const memberToRemove = await createTeamAdmin(adminClient, team.id);
+        await setUserAttribute(adminClient, memberToRemove.id, 'Department', `${departmentValue}-other`);
+        await adminClient.addToChannel(memberToRemove.id, channel.id);
+
+        // See c9: wait for the materialized AttributeView to surface the admin's
+        // freshly-written CPA value before clicking Save.
+        await waitForAttributeViewToInclude(adminClient, `user.attributes.Department == "${departmentValue}"`, [
+            adminUser.id,
+        ]);
+
+        const {page} = await pw.testBrowser.login(adminUser);
+        const channelsPage = new ChannelsPage(page);
+        await channelsPage.goto(team.name, channel.name);
+        await channelsPage.toBeVisible();
+
+        const channelSettings = await channelsPage.openChannelSettings();
+
+        // # Navigate to Access Control tab
+        await channelSettings.container.getByTestId('access_rules-tab-button').click();
+
+        const tab = channelSettings.container.locator('.ChannelSettingsModal__accessRulesTab');
+        await expect(tab).toBeVisible({timeout: 10000});
+
+        // # Add rule (unique value -> only admin remains allowed in the private channel)
+        await addAttributeRule(tab, page, departmentValue);
+
+        // # Click Save - confirmation modal appears because memberToRemove will be removed
+        const saveBtn = tab.locator('[data-testid="SaveChangesPanel__save-btn"]');
+        await expect(saveBtn).toBeEnabled({timeout: 10000});
+        await saveBtn.click();
+
+        const confirmModal = page.locator('#channel-access-rules-confirm-modal');
+        await confirmModal.waitFor({state: 'visible', timeout: 30000});
+
+        // * Summary message shows 0 users added and 1 member removed
+        await expect(confirmModal).toContainText('remove 1 current channel member');
+
+        // # Confirm and wait for the access-control sync job that applies the removal
+        const [syncJobResponse] = await Promise.all([
+            page.waitForResponse(
+                (response) => response.url().includes('/api/v4/jobs') && response.request().method() === 'POST',
+                {timeout: 10000},
+            ),
+            confirmModal.getByRole('button', {name: 'Save'}).click(),
+        ]);
+        await confirmModal.waitFor({state: 'hidden', timeout: 15000});
+
+        if (!syncJobResponse.ok()) {
+            throw new Error(`Failed to create access-control sync job: ${syncJobResponse.status()}`);
+        }
+        const syncJob = await syncJobResponse.json();
+        const syncJobId = syncJob.id as string;
+        const finished = await waitForJobCompletion(adminClient, syncJobId, {timeoutMs: 90_000});
+        expect(finished.status, `sync job did not succeed: ${JSON.stringify(finished)}`).toBe('success');
+
+        // * Poll until memberToRemove is no longer a channel member
+        await expect
+            .poll(
+                async () => {
+                    try {
+                        await adminClient.getChannelMember(channel.id, memberToRemove.id);
+                        return true;
+                    } catch {
+                        return false;
+                    }
+                },
+                {
+                    timeout: 15000,
+                    intervals: [500, 1000, 1000, 2000],
+                    message: `${memberToRemove.username} should be removed from the channel`,
+                },
+            )
+            .toBe(false);
+
+        await channelSettings.close();
+
+        // * The ABAC removal system message is the last visible post in the channel
+        await channelsPage.centerView.waitUntilLastPostContains('was removed from the channel', 30000);
+        const lastPost = await channelsPage.getLastPost();
+        await expect(lastPost.container).toContainText(memberToRemove.username);
+        await expect(lastPost.container).toContainText('was removed from the channel');
+
+        // # Open channel members RHS from the affected channel
+        await channelsPage.centerView.header.openChannelMenu();
+        await page.locator('#channelMembers').click();
+        await channelsPage.sidebarRight.toBeVisible();
+
+        // * Admin remains in the RHS members list, and removed member is absent
+        await expect(page.getByTestId(`memberline-${adminUser.id}`)).toBeVisible({timeout: 10000});
+        await expect(page.getByTestId(`memberline-${memberToRemove.id}`)).not.toBeVisible();
+    });
 });
@@ -196,6 +196,107 @@ describe('Actions.User', () => {
        expect(actualActions[0].type).toEqual(expectedActions[0].type);
    });

+    describe('loadProfilesAndReloadChannelMembers reconcile', () => {
+        // The mocked getProfilesInChannel resolves with a single member (user_1), so any
+        // other member present in the store for the channel is treated as stale (e.g. a
+        // member removed by an ABAC access-rule change while the user was not viewing the
+        // channel, whose user_removed websocket event was never applied).
+        // user_1 is the only member the mocked getProfilesInChannel returns, so it is the
+        // current membership. user_2 is stale in both stores; user_3 is stale only in the
+        // member store; user_4 is stale only in the profile store. This covers the union
+        // pruning across both entities.channels.membersInChannel and
+        // entities.users.profilesInChannel.
+        const buildState = (members: Record<string, unknown>, profileIds: string[]) => ({
+            ...initialState,
+            entities: {
+                ...initialState.entities,
+                channels: {
+                    ...initialState.entities.channels,
+                    membersInChannel: {reconcile_channel: members},
+                },
+                users: {
+                    ...initialState.entities.users,
+                    profilesInChannel: {reconcile_channel: new Set(profileIds)},
+                },
+            },
+        }) as unknown as GlobalState;
+
+        const reconcileState = buildState(
+            {
+                user_1: {channel_id: 'reconcile_channel', user_id: 'user_1'} as ChannelMembership,
+                user_2: {channel_id: 'reconcile_channel', user_id: 'user_2'} as ChannelMembership,
+                user_3: {channel_id: 'reconcile_channel', user_id: 'user_3'} as ChannelMembership,
+            },
+            ['user_1', 'user_2', 'user_4'],
+        );
+
+        const prunedUserIds = (actions: AnyAction[]): string[] => {
+            const ids: string[] = [];
+            for (const action of actions) {
+                const candidates = Array.isArray(action.payload) ? action.payload : [action];
+                for (const candidate of candidates) {
+                    if (candidate.type === 'RECEIVED_PROFILE_NOT_IN_CHANNEL' && candidate.data?.id === 'reconcile_channel') {
+                        ids.push(candidate.data.user_id);
+                    }
+                }
+            }
+            return ids;
+        };
+
+        test('prunes members the server no longer returns from both member and profile stores', async () => {
+            const testStore = mockStore(reconcileState);
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(0, 60, 'reconcile_channel', 'sort', {}, true));
+
+            const pruned = prunedUserIds(testStore.getActions());
+            expect([...pruned].sort()).toEqual(['user_2', 'user_3', 'user_4']);
+            expect(pruned).not.toContain('user_1');
+        });
+
+        test('does not prune members the server still returns', async () => {
+            // The server response (user_1) matches the only stored member, so nothing is
+            // stale and a present member must never be pruned.
+            const testStore = mockStore(buildState(
+                {user_1: {channel_id: 'reconcile_channel', user_id: 'user_1'} as ChannelMembership},
+                ['user_1'],
+            ));
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(0, 60, 'reconcile_channel', 'sort', {}, true));
+
+            expect(prunedUserIds(testStore.getActions())).toHaveLength(0);
+        });
+
+        test('does not prune when reconcile is disabled', async () => {
+            const testStore = mockStore(reconcileState);
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(0, 60, 'reconcile_channel', 'sort', {}, false));
+
+            expect(prunedUserIds(testStore.getActions())).toHaveLength(0);
+        });
+
+        test('does not prune on a full page since more pages may follow', async () => {
+            const testStore = mockStore(reconcileState);
+
+            // perPage equals the number of returned profiles (1), so the page is not known
+            // to hold the full membership and members beyond it must not be pruned.
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(0, 1, 'reconcile_channel', 'sort', {}, true));
+
+            expect(prunedUserIds(testStore.getActions())).toHaveLength(0);
+        });
+
+        test('does not prune on pages other than the first', async () => {
+            const testStore = mockStore(reconcileState);
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(1, 60, 'reconcile_channel', 'sort', {}, true));
+
+            expect(prunedUserIds(testStore.getActions())).toHaveLength(0);
+        });
+
+        test('does not prune when perPage is not provided', async () => {
+            // Without a page size the response cannot be known to hold the full membership.
+            const testStore = mockStore(reconcileState);
+            await testStore.dispatch(UserActions.loadProfilesAndReloadChannelMembers(0, undefined, 'reconcile_channel', 'sort', {}, true));
+
+            expect(prunedUserIds(testStore.getActions())).toHaveLength(0);
+        });
+    });
+
    test('loadProfilesAndTeamMembersAndChannelMembers', async () => {
        const expectedActions = [{type: 'MOCK_GET_PROFILES_IN_CHANNEL', args: ['current_channel_id', 0, 60, '', undefined]}];

@@ -1,10 +1,13 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.

+import {batchActions} from 'redux-batched-actions';
+
 import type {UserAutocomplete} from '@mattermost/types/autocomplete';
 import type {Channel} from '@mattermost/types/channels';
 import type {UserProfile, UserStatus} from '@mattermost/types/users';

+import {UserTypes} from 'mattermost-redux/action_types';
 import {getChannelAndMyMember, getChannelMembersByIds} from 'mattermost-redux/actions/channels';
 import {savePreferences} from 'mattermost-redux/actions/preferences';
 import {getTeamMembersByIds} from 'mattermost-redux/actions/teams';
@@ -53,11 +56,19 @@ export function loadProfilesAndReloadTeamMembers(page: number, perPage: number,
    };
 }

-export function loadProfilesAndReloadChannelMembers(page: number, perPage?: number, channelId?: string, sort = '', options = {}): ActionFuncAsync {
+export function loadProfilesAndReloadChannelMembers(page: number, perPage?: number, channelId?: string, sort = '', options = {}, reconcile = false): ActionFuncAsync {
    return async (doDispatch, doGetState) => {
        const newChannelId = channelId || getCurrentChannelId(doGetState());
        const {data} = await doDispatch(UserActions.getProfilesInChannel(newChannelId, page, perPage, sort, options));
        if (data) {
+            // getProfilesInChannel only adds to the channel member stores, so a removal
+            // missed over the websocket (e.g. an ABAC access-rule change handled while
+            // another channel was focused) leaves the member list stale. Prune members
+            // the server no longer returns, but only when this first page holds the full
+            // membership; a full page means more pages follow and must not be pruned.
+            if (reconcile && page === 0 && perPage !== undefined && data.length < perPage) {
+                doDispatch(pruneStaleChannelMembers(newChannelId, data));
+            }
            await Promise.all([
                doDispatch(loadChannelMembersForProfilesList(data, newChannelId, true)),
                doDispatch(loadStatusesForProfilesList(data)),
@@ -68,6 +79,42 @@ export function loadProfilesAndReloadChannelMembers(page: number, perPage?: numb
    };
 }

+function pruneStaleChannelMembers(channelId: string, currentProfiles: UserProfile[]): ActionFunc {
+    return (doDispatch, doGetState) => {
+        const state = doGetState();
+        const currentIds = new Set(currentProfiles.map((profile) => profile.id));
+
+        const staleIds = new Set<string>();
+        const storedMembers = getChannelMembersInChannels(state)[channelId];
+        if (storedMembers) {
+            Object.keys(storedMembers).forEach((userId) => {
+                if (!currentIds.has(userId)) {
+                    staleIds.add(userId);
+                }
+            });
+        }
+        const storedProfileIds = Selectors.getUserIdsInChannels(state)[channelId];
+        if (storedProfileIds) {
+            storedProfileIds.forEach((userId) => {
+                if (!currentIds.has(userId)) {
+                    staleIds.add(userId);
+                }
+            });
+        }
+
+        if (staleIds.size === 0) {
+            return {data: false};
+        }
+
+        doDispatch(batchActions(Array.from(staleIds).map((userId) => ({
+            type: UserTypes.RECEIVED_PROFILE_NOT_IN_CHANNEL,
+            data: {id: channelId, user_id: userId},
+        }))));
+
+        return {data: true};
+    };
+}
+
 export function loadProfilesAndTeamMembers(page: number, perPage: number, teamId: string, options?: Record<string, any>): ActionFuncAsync {
    return async (doDispatch, doGetState) => {
        const newTeamId = teamId || getCurrentTeamId(doGetState());
@@ -148,6 +148,27 @@ describe('channel_members_rhs/channel_members_rhs', () => {
        expect(screen.getByTestId('member-list')).toBeInTheDocument();
    });

+    test('reloads the first page authoritatively on mount so removed members are pruned', () => {
+        const loadProfilesAndReloadChannelMembers = jest.fn();
+        const props = {
+            ...baseProps,
+            actions: {
+                ...baseProps.actions,
+                loadProfilesAndReloadChannelMembers,
+            },
+        };
+
+        renderWithContext(
+            <ChannelMembersRHS
+                {...props as any}
+            />,
+        );
+
+        // The trailing `true` enables reconciliation so the first-page reload prunes
+        // members the server no longer returns (e.g. ABAC access-rule removals).
+        expect(loadProfilesAndReloadChannelMembers).toHaveBeenCalledWith(0, 100, 'channel_id', 'admin', {}, true);
+    });
+
    test('should show search bar when there are more than 20 members', () => {
        const props = {
            ...baseProps,
@@ -52,7 +52,7 @@ export interface Props {
        closeRightHandSide: () => void;
        goBack: () => void;
        setChannelMembersRhsSearchTerm: (terms: string) => void;
-        loadProfilesAndReloadChannelMembers: (page: number, perParge: number, channelId: string, sort: string) => void;
+        loadProfilesAndReloadChannelMembers: (page: number, perParge: number, channelId: string, sort: string, options?: Record<string, unknown>, reconcile?: boolean) => void;
        loadMyChannelMemberAndRole: (channelId: string) => void;
        setEditChannelMembers: (active: boolean) => void;
        searchProfilesAndChannelMembers: (term: string, options: any) => Promise<{data: UserProfile[]}>;
@@ -189,7 +189,7 @@ export default function ChannelMembersRHS({
        setPage(0);
        setIsNextPageLoading(false);
        actions.setChannelMembersRhsSearchTerm('');
-        actions.loadProfilesAndReloadChannelMembers(0, USERS_PER_PAGE, channel.id, ProfilesInChannelSortBy.Admin);
+        actions.loadProfilesAndReloadChannelMembers(0, USERS_PER_PAGE, channel.id, ProfilesInChannelSortBy.Admin, {}, true);
        actions.loadMyChannelMemberAndRole(channel.id);
    }, [channel.id, channel.type]);