MM-64330 - filter abac users in channel invite (#31219)

* MM-64330 - filter abac users in channel invite

* implement cursor functionality for abac user filtering

* remove unnecessary comments

* refactor the backend implementation simplifying the functions

* refactor api to use opts as parameters, rename function

* add missing translation

* remove unnecesary test code

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
Этот коммит содержится в:
Pablo Vélez
2025-06-20 10:53:14 +02:00
коммит произвёл GitHub
родитель 968550d275
Коммит 5fc74cd401
11 изменённых файлов: 346 добавлений и 21 удалений

Просмотреть файл

@@ -840,7 +840,13 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}
profiles, appErr = c.App.GetUsersNotInChannelPage(inTeamId, notInChannelId, groupConstrainedBool, c.Params.Page, c.Params.PerPage, c.IsSystemAdmin(), restrictions)
if ok, _ := c.App.ChannelAccessControlled(c.AppContext, notInChannelId); ok {
// Get cursor_id from query parameters for cursor-based pagination
cursorId := r.URL.Query().Get("cursor_id")
profiles, appErr = c.App.GetUsersNotInAbacChannel(c.AppContext, inTeamId, notInChannelId, groupConstrainedBool, cursorId, c.Params.PerPage, c.IsSystemAdmin(), restrictions)
} else {
profiles, appErr = c.App.GetUsersNotInChannelPage(inTeamId, notInChannelId, groupConstrainedBool, c.Params.Page, c.Params.PerPage, c.IsSystemAdmin(), restrictions)
}
} else if notInTeamId != "" {
if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), notInTeamId, model.PermissionViewTeam) {
c.SetPermissionError(model.PermissionViewTeam)

Просмотреть файл

@@ -3965,7 +3965,7 @@ func (a *App) ChannelAccessControlled(c request.CTX, channelID string) (bool, *m
return false, nil
}
_, err := a.Srv().Store().AccessControlPolicy().Get(c, channelID)
channel, err := a.Srv().Store().Channel().Get(channelID, true)
var nfErr *store.ErrNotFound
if err != nil && !errors.As(err, &nfErr) {
return false, model.NewAppError("ChannelIsAccessControlled", "app.channel.get.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
@@ -3973,7 +3973,7 @@ func (a *App) ChannelAccessControlled(c request.CTX, channelID string) (bool, *m
return false, nil
}
return true, nil
return channel.PolicyEnforced, nil
}
func (a *App) handleChannelCategoryName(channel *model.Channel) {

Просмотреть файл

@@ -667,6 +667,28 @@ func (a *App) GetUsersNotInChannelPage(teamID string, channelID string, groupCon
return a.sanitizeProfiles(users, asAdmin), nil
}
func (a *App) GetUsersNotInAbacChannel(ctx request.CTX, teamID string, channelID string, groupConstrained bool, cursorID string, limit int, asAdmin bool, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) {
// Get the AccessControl service
acs := a.Srv().Channels().AccessControl
if acs == nil {
return nil, model.NewAppError("GetUsersNotInAbacChannel", "api.user.get_users_not_in_abac_channel.access_control_unavailable.app_error", nil, "", http.StatusInternalServerError)
}
// Use cursor-based pagination for ABAC channels
users, _, appErr := acs.QueryUsersForResource(ctx, channelID, "*", model.SubjectSearchOptions{
TeamID: teamID,
Limit: limit,
Cursor: model.SubjectCursor{
TargetID: cursorID, // Empty string means start from beginning
},
})
if appErr != nil {
return nil, appErr
}
return a.sanitizeProfiles(users, asAdmin), nil
}
func (a *App) GetUsersWithoutTeamPage(options *model.UserGetOptions, asAdmin bool) ([]*model.User, *model.AppError) {
users, err := a.ch.srv.userService.GetUsersWithoutTeamPage(options, asAdmin)
if err != nil {

Просмотреть файл

@@ -880,6 +880,126 @@ func TestGetUsersByStatus(t *testing.T) {
})
}
func TestGetUsersNotInAbacChannel(t *testing.T) {
th := Setup(t).InitBasic()
defer th.TearDown()
// Set license to EnterpriseAdvanced
th.App.Srv().SetLicense(model.NewTestLicense("enterprise.advanced"))
// Enable ABAC in config
th.App.UpdateConfig(func(cfg *model.Config) {
*cfg.AccessControlSettings.EnableAttributeBasedAccessControl = true
})
// Create an ABAC channel
abacChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam)
// Create three test users and add them to the team
user1 := th.CreateUser() // Will have matching attributes for ABAC
user2 := th.CreateUser() // Won't have matching attributes
user3 := th.CreateUser() // Won't have matching attributes
th.LinkUserToTeam(user1, th.BasicTeam)
th.LinkUserToTeam(user2, th.BasicTeam)
th.LinkUserToTeam(user3, th.BasicTeam)
// Create a policy with the same ID as the ABAC channel
channelPolicy := &model.AccessControlPolicy{
Type: model.AccessControlPolicyTypeChannel,
ID: abacChannel.Id,
Name: "Test Channel Policy",
Revision: 1,
Version: model.AccessControlPolicyVersionV0_1,
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"view", "join_channel"},
Expression: "user.attributes.program == \"test-program\"",
},
},
}
// Save the channel policy
var storeErr error
channelPolicy, storeErr = th.App.Srv().Store().AccessControlPolicy().Save(th.Context, channelPolicy)
require.NoError(t, storeErr)
require.NotNil(t, channelPolicy)
t.Cleanup(func() {
dErr := th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, channelPolicy.ID)
require.NoError(t, dErr)
})
// Mock the AccessControl service
mockAccessControl := &mocks.AccessControlServiceInterface{}
originalAccessControl := th.App.Srv().ch.AccessControl
th.App.Srv().ch.AccessControl = mockAccessControl
defer func() {
th.App.Srv().ch.AccessControl = originalAccessControl
}()
t.Run("Returns users with matching attributes using cursor pagination", func(t *testing.T) {
// Set up the mock to return user1 when querying for users
mockAccessControl.On("QueryUsersForResource",
mock.Anything,
abacChannel.Id,
"*",
mock.MatchedBy(func(opts model.SubjectSearchOptions) bool {
return opts.TeamID == th.BasicTeam.Id &&
opts.Limit == 50 &&
opts.Cursor.TargetID == ""
})).Return([]*model.User{user1}, int64(1), nil).Once()
// Call the new ABAC-specific function with th.Context as first parameter
users, appErr := th.App.GetUsersNotInAbacChannel(th.Context, th.BasicTeam.Id, abacChannel.Id, false, "", 50, true, nil)
require.Nil(t, appErr)
// Create a map of user IDs for easier lookup
userMap := make(map[string]bool)
for _, u := range users {
userMap[u.Id] = true
}
// Verify only user1 is returned
assert.True(t, userMap[user1.Id], "User1 should be returned for ABAC channel")
assert.False(t, userMap[user2.Id], "User2 should not be returned for ABAC channel")
assert.False(t, userMap[user3.Id], "User3 should not be returned for ABAC channel")
assert.Len(t, users, 1, "Should return exactly 1 user")
})
t.Run("Works with cursor-based pagination", func(t *testing.T) {
cursorID := "some-cursor-id"
// Set up the mock to return user1 when querying with cursor
mockAccessControl.On("QueryUsersForResource",
mock.Anything,
abacChannel.Id,
"*",
mock.MatchedBy(func(opts model.SubjectSearchOptions) bool {
return opts.TeamID == th.BasicTeam.Id &&
opts.Limit == 25 &&
opts.Cursor.TargetID == cursorID
})).Return([]*model.User{user1}, int64(1), nil).Once()
// Call with cursor ID and th.Context as first parameter
users, appErr := th.App.GetUsersNotInAbacChannel(th.Context, th.BasicTeam.Id, abacChannel.Id, false, cursorID, 25, true, nil)
require.Nil(t, appErr)
assert.Len(t, users, 1, "Should return exactly 1 user with cursor pagination")
})
t.Run("Returns error when AccessControl service is unavailable", func(t *testing.T) {
// Temporarily set AccessControl to nil
th.App.Srv().ch.AccessControl = nil
defer func() {
th.App.Srv().ch.AccessControl = mockAccessControl
}()
// Call should return error with th.Context as first parameter
users, appErr := th.App.GetUsersNotInAbacChannel(th.Context, th.BasicTeam.Id, abacChannel.Id, false, "", 50, true, nil)
require.NotNil(t, appErr)
require.Nil(t, users)
assert.Equal(t, "api.user.get_users_not_in_abac_channel.access_control_unavailable.app_error", appErr.Id)
})
}
func TestCreateUserWithInviteId(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic()

Просмотреть файл

@@ -4118,6 +4118,10 @@
"id": "api.user.get_users.validation.app_error",
"translation": "Error fetching roles during validation."
},
{
"id": "api.user.get_users_not_in_abac_channel.access_control_unavailable.app_error",
"translation": "Access control service unavailable. Cannot filter users for ABAC-enabled channel."
},
{
"id": "api.user.invalidate_password_recovery_tokens.error",
"translation": "Unable to get tokens by type when invalidating password recovery tokens"

Просмотреть файл

@@ -1396,8 +1396,23 @@ func (c *Client4) GetUsersInChannelByStatus(ctx context.Context, channelId strin
// GetUsersNotInChannel returns a page of users not in a channel. Page counting starts at 0.
func (c *Client4) GetUsersNotInChannel(ctx context.Context, teamId, channelId string, page int, perPage int, etag string) ([]*User, *Response, error) {
query := fmt.Sprintf("?in_team=%v&not_in_channel=%v&page=%v&per_page=%v", teamId, channelId, page, perPage)
r, err := c.DoAPIGet(ctx, c.usersRoute()+query, etag)
options := &GetUsersNotInChannelOptions{
TeamID: teamId,
Page: page,
Limit: perPage,
Etag: etag,
CursorID: "",
}
return c.GetUsersNotInChannelWithOptions(ctx, channelId, options)
}
// GetUsersNotInChannelWithOptionsStruct returns a page of users not in a channel using the options struct.
func (c *Client4) GetUsersNotInChannelWithOptions(ctx context.Context, channelId string, options *GetUsersNotInChannelOptions) ([]*User, *Response, error) {
query := fmt.Sprintf("?in_team=%v&not_in_channel=%v&page=%v&per_page=%v", options.TeamID, channelId, options.Page, options.Limit)
if options.CursorID != "" {
query += fmt.Sprintf("&cursor_id=%v", options.CursorID)
}
r, err := c.DoAPIGet(ctx, c.usersRoute()+query, options.Etag)
if err != nil {
return nil, BuildResponse(r), err
}
@@ -1407,7 +1422,7 @@ func (c *Client4) GetUsersNotInChannel(ctx context.Context, teamId, channelId st
return list, BuildResponse(r), nil
}
if err := json.NewDecoder(r.Body).Decode(&list); err != nil {
return nil, nil, NewAppError("GetUsersNotInChannel", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
return nil, nil, NewAppError("GetUsersNotInChannelWithOptionsStruct", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
return list, BuildResponse(r), nil
}

Просмотреть файл

@@ -243,6 +243,19 @@ type ViewUsersRestrictions struct {
Channels []string
}
//msgp:ignore GetUsersNotInChannelOptions
type GetUsersNotInChannelOptions struct {
TeamID string `json:"team_id"`
// Page-based pagination (used for non-ABAC channels)
// This will be discarded if the channel has an ABAC policy and CursorID will be used.
Page int `json:"page"`
Limit int `json:"limit"`
// Cursor-based pagination (used for ABAC channels)
// If CursorID is empty for ABAC channels, it will start from the beginning
CursorID string `json:"cursor_id"`
Etag string `json:"etag"`
}
func (r *ViewUsersRestrictions) Hash() string {
if r == nil {
return ""

Просмотреть файл

@@ -124,6 +124,14 @@ describe('components/channel_invite_modal', () => {
onExited: jest.fn(),
};
beforeAll(() => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
window.requestAnimationFrame = (_cb: FrameRequestCallback): number => {
// do not call cb at all
return 0;
};
});
test('should match snapshot for channel_invite_modal with profiles', () => {
const wrapper = shallowWithIntl(
<ChannelInviteModal
@@ -589,4 +597,97 @@ describe('components/channel_invite_modal', () => {
// Check that no tags are shown
expect(wrapper.find('AlertTag').exists()).toBe(false);
});
// the multiselect returns several elements with the same text, usiing a custom function to get the correct one specifing the tagName
const getUserSpan = (user: string) =>
screen.getByText((text, element) =>
element?.tagName === 'SPAN' && text.trim() === user,
) as HTMLElement;
test('should not include DM users when ABAC is enabled', async () => {
const channelWithPolicy = {...channel, policy_enforced: true};
const props = {
...baseProps,
channel: channelWithPolicy,
profilesNotInCurrentChannel: [users[0]],
profilesFromRecentDMs: [users[1]],
};
await act(async () => {
renderWithContext(<ChannelInviteModal {...props}/>);
});
const input = screen.getByRole('combobox', {name: /search for people/i});
await userEvent.type(input, 'user');
// now only one visible <span> should match "user-1"
expect(getUserSpan('user-1')).toBeInTheDocument();
// and no <span> with "user-2"
expect(screen.queryByText('user-2')).toBeNull();
});
test('should include DM users when ABAC is disabled', async () => {
const channelWithoutPolicy = {...channel, policy_enforced: false};
const props = {
...baseProps,
channel: channelWithoutPolicy,
profilesNotInCurrentChannel: [users[0]],
profilesFromRecentDMs: [users[1]],
};
await act(async () => {
renderWithContext(<ChannelInviteModal {...props}/>);
});
const input = screen.getByRole('combobox', {name: /search for people/i});
await userEvent.type(input, 'user');
// we should see both user-1 and user-2 in visible spans
expect(getUserSpan('user-1')).toBeInTheDocument();
expect(getUserSpan('user-2')).toBeInTheDocument();
});
test('should not reload data when search term is empty and ABAC is disabled', async () => {
const getProfilesNotInChannelMock = jest.fn().mockImplementation(() => Promise.resolve());
const channelWithoutPolicy = {
...channel,
policy_enforced: false,
};
const props = {
...baseProps,
channel: channelWithoutPolicy,
actions: {
...baseProps.actions,
getProfilesNotInChannel: getProfilesNotInChannelMock,
},
};
// Render the component
await act(async () => {
renderWithContext(
<ChannelInviteModal {...props}/>,
);
});
// Reset the mock after component mount to ignore initial data loading
getProfilesNotInChannelMock.mockClear();
// Find the search input
const input = screen.getByRole('combobox', {name: /search for people/i});
// Type something and then clear it
await userEvent.type(input, 'a');
await userEvent.clear(input);
// Wait for the search timeout
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 200));
});
// Should not call getProfilesNotInChannel after clearing the search
expect(getProfilesNotInChannelMock).not.toHaveBeenCalled();
});
});

Просмотреть файл

@@ -73,7 +73,7 @@ export type Props = {
isGroupsEnabled: boolean;
actions: {
addUsersToChannel: (channelId: string, userIds: string[]) => Promise<ActionResult>;
getProfilesNotInChannel: (teamId: string, channelId: string, groupConstrained: boolean, page: number, perPage?: number) => Promise<ActionResult>;
getProfilesNotInChannel: (teamId: string, channelId: string, groupConstrained: boolean, page: number, perPage?: number, cursorId?: string) => Promise<ActionResult>;
getProfilesInChannel: (channelId: string, page: number, perPage: number, sort: string, options: {active?: boolean}) => Promise<ActionResult>;
getTeamStats: (teamId: string) => void;
loadStatusesForProfilesList: (users: UserProfile[]) => void;
@@ -99,6 +99,7 @@ const ChannelInviteModalComponent = (props: Props) => {
const [loadingUsers, setLoadingUsers] = useState(true);
const [groupAndUserOptions, setGroupAndUserOptions] = useState<Array<UserProfileValue | GroupValue>>([]);
const [inviteError, setInviteError] = useState<string | undefined>(undefined);
const [pageCursors, setPageCursors] = useState<{[page: number]: string}>({});
const searchTimeoutId = useRef<number>(0);
const selectedItemRef = useRef<HTMLDivElement>(null);
@@ -173,15 +174,29 @@ const ChannelInviteModalComponent = (props: Props) => {
const getOptions = useCallback(() => {
const excludedAndNotInTeamUserIds = excludedUsers;
const filteredDmUsers = filterProfilesStartingWithTerm(props.profilesFromRecentDMs, term);
const dmUsers = filterOutDeletedAndExcludedAndNotInTeamUsers(filteredDmUsers, excludedAndNotInTeamUserIds).slice(0, USERS_FROM_DMS) as UserProfileValue[];
// Only include DM users if ABAC is not enabled
let dmUsers: UserProfileValue[] = [];
if (!props.channel.policy_enforced) {
const filteredDmUsers = filterProfilesStartingWithTerm(props.profilesFromRecentDMs, term);
dmUsers = filterOutDeletedAndExcludedAndNotInTeamUsers(filteredDmUsers, excludedAndNotInTeamUserIds).slice(0, USERS_FROM_DMS) as UserProfileValue[];
}
let users: UserProfileValue[];
const filteredUsers: UserProfile[] = filterProfilesStartingWithTerm(props.profilesNotInCurrentChannel.concat(props.profilesInCurrentChannel), term);
users = filterOutDeletedAndExcludedAndNotInTeamUsers(filteredUsers, excludedAndNotInTeamUserIds);
if (props.includeUsers) {
if (props.channel.policy_enforced) {
// When ABAC is enabled, only use the ABAC-filtered profilesNotInCurrentChannel
const filteredUsers = filterProfilesStartingWithTerm(props.profilesNotInCurrentChannel, term);
users = filterOutDeletedAndExcludedAndNotInTeamUsers(filteredUsers, excludedAndNotInTeamUserIds);
} else {
// When ABAC is not enabled, use the current logic
const filteredUsers = filterProfilesStartingWithTerm(props.profilesNotInCurrentChannel.concat(props.profilesInCurrentChannel), term);
users = filterOutDeletedAndExcludedAndNotInTeamUsers(filteredUsers, excludedAndNotInTeamUserIds);
}
// Only include explicitly added users if ABAC is not enabled
if (props.includeUsers && !props.channel.policy_enforced) {
users = [...users, ...Object.values(props.includeUsers)];
}
const groupsAndUsers = [
...filterGroupsMatchingTerm(props.groups, term) as GroupValue[],
...users,
@@ -200,6 +215,7 @@ const ChannelInviteModalComponent = (props: Props) => {
props.profilesInCurrentChannel,
props.includeUsers,
props.groups,
props.channel.policy_enforced,
excludedUsers,
filterOutDeletedAndExcludedAndNotInTeamUsers,
]);
@@ -231,19 +247,38 @@ const ChannelInviteModalComponent = (props: Props) => {
setLoadingUsers(loadingState);
}, []);
// Handle page change
// Handle page change with cursor-based pagination
const handlePageChange = useCallback((page: number, prevPage: number) => {
if (page > prevPage) {
setUsersLoadingState(true);
// Get cursor for this page (if we're going forward)
const cursorId = page > 0 ? pageCursors[page - 1] : '';
props.actions.getProfilesNotInChannel(
props.channel.team_id,
props.channel.id,
props.channel.group_constrained,
page + 1, USERS_PER_PAGE).then(() => setUsersLoadingState(false));
page + 1,
USERS_PER_PAGE,
cursorId,
).then((result) => {
// Store the cursor for the next page (ID of the last user)
if (result.data && result.data.length > 0) {
const lastUserId = result.data[result.data.length - 1].id;
setPageCursors((prev) => ({
...prev,
[page]: lastUserId,
}));
}
setUsersLoadingState(false);
}).catch(() => {
setUsersLoadingState(false);
});
props.actions.getProfilesInChannel(props.channel.id, page + 1, USERS_PER_PAGE, '', {active: true});
}
}, [props.actions, props.channel, setUsersLoadingState]);
}, [props.actions, props.channel, setUsersLoadingState, pageCursors, setPageCursors]);
// Handle form submission
const handleSubmit = useCallback(() => {
@@ -282,7 +317,8 @@ const ChannelInviteModalComponent = (props: Props) => {
setTerm(term);
if (!term) {
// If the search term is empty, don't make any API calls
// Reset cursor state when clearing search
setPageCursors({});
setUsersLoadingState(false);
return;
}
@@ -403,7 +439,7 @@ const ChannelInviteModalComponent = (props: Props) => {
// Initial data loading - only run when channel changes or component mounts
useEffect(() => {
props.actions.getProfilesNotInChannel(props.channel.team_id, props.channel.id, props.channel.group_constrained, 0).then(() => {
props.actions.getProfilesNotInChannel(props.channel.team_id, props.channel.id, props.channel.group_constrained, 0, USERS_PER_PAGE).then(() => {
setUsersLoadingState(false);
});
props.actions.getProfilesInChannel(props.channel.id, 0, USERS_PER_PAGE, '', {active: true});

Просмотреть файл

@@ -432,12 +432,12 @@ export function getProfilesInGroupChannels(channelsIds: string[]): ActionFuncAsy
};
}
export function getProfilesNotInChannel(teamId: string, channelId: string, groupConstrained: boolean, page: number, perPage: number = General.PROFILE_CHUNK_SIZE): ActionFuncAsync<UserProfile[]> {
export function getProfilesNotInChannel(teamId: string, channelId: string, groupConstrained: boolean, page: number, perPage: number = General.PROFILE_CHUNK_SIZE, cursorId = ''): ActionFuncAsync<UserProfile[]> {
return async (dispatch, getState) => {
let profiles;
try {
profiles = await Client4.getProfilesNotInChannel(teamId, channelId, groupConstrained, page, perPage);
profiles = await Client4.getProfilesNotInChannel(teamId, channelId, groupConstrained, page, perPage, cursorId);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));

Просмотреть файл

@@ -898,8 +898,16 @@ export default class Client4 {
);
};
getProfilesNotInChannel = (teamId: string, channelId: string, groupConstrained: boolean, page = 0, perPage = PER_PAGE_DEFAULT) => {
const queryStringObj: any = {in_team: teamId, not_in_channel: channelId, page, per_page: perPage};
getProfilesNotInChannel = (teamId: string, channelId: string, groupConstrained: boolean, page = 0, perPage = PER_PAGE_DEFAULT, cursorId = '') => {
const queryStringObj: any = {in_team: teamId, not_in_channel: channelId, per_page: perPage};
// If cursorId is provided, use cursor-based pagination
if (cursorId) {
queryStringObj.cursor_id = cursorId;
} else {
// Otherwise use traditional page-based pagination
queryStringObj.page = page;
}
if (groupConstrained) {
queryStringObj.group_constrained = true;
}