diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts index f5c87452fa3..07e2180ebca 100644 --- a/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts @@ -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(); + }); }); diff --git a/webapp/channels/src/actions/user_actions.test.ts b/webapp/channels/src/actions/user_actions.test.ts index 50822fb5ed7..7906eb170c8 100644 --- a/webapp/channels/src/actions/user_actions.test.ts +++ b/webapp/channels/src/actions/user_actions.test.ts @@ -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, 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]}]; diff --git a/webapp/channels/src/actions/user_actions.ts b/webapp/channels/src/actions/user_actions.ts index 82973852d5c..802cb89a176 100644 --- a/webapp/channels/src/actions/user_actions.ts +++ b/webapp/channels/src/actions/user_actions.ts @@ -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(); + 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): ActionFuncAsync { return async (doDispatch, doGetState) => { const newTeamId = teamId || getCurrentTeamId(doGetState()); diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx index d56fd7064d8..d5b98f7bec1 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx @@ -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( + , + ); + + // 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, diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx index 07022e3a7b5..0d1c5527d29 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx @@ -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, 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]);