From e1a27cf57f552463fc70b05c702a60b9b9f0b307 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 8 Jan 2024 13:28:51 +0100 Subject: [PATCH] Adds a team check to get groups by team (#25462) * Adds a team check to get groups by team * Update to take into account groups_by_channel and new permissions * Update API docs --- api/v4/source/groups.yaml | 13 +++++---- server/channels/api4/group.go | 10 +++++++ server/channels/api4/group_test.go | 47 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/api/v4/source/groups.yaml b/api/v4/source/groups.yaml index de8aea6bd9..ced5d1221e 100644 --- a/api/v4/source/groups.yaml +++ b/api/v4/source/groups.yaml @@ -74,7 +74,7 @@ description: Boolean which filters the group entries with the `allow_reference` attribute set. schema: type: boolean - default: false + default: false responses: "200": description: Group list retrieval successful @@ -122,10 +122,10 @@ - allow_reference description: Group object to create. properties: - name: + name: type: string description: The unique group name used for at-mentioning. - display_name: + display_name: type: string description: The display name of the group which can include spaces. source: @@ -1006,6 +1006,9 @@ description: | Retrieve the list of groups associated with a given team. + ##### Permissions + Must have the `list_team_channels` permission. + __Minimum server version__: 5.11 operationId: GetGroupsByTeam parameters: @@ -1061,7 +1064,7 @@ Retrieve the set of groups associated with the channels in the given team grouped by channel. ##### Permissions - Must have `manage_system` permission or can access only for current user + Must have the `list_team_channels` permission. __Minimum server version__: 5.11 operationId: GetGroupsAssociatedToChannelsByTeam @@ -1095,7 +1098,7 @@ description: Boolean to determine whether the pagination should be applied or not schema: type: boolean - default: false + default: false responses: "200": description: Group list retrieval successful diff --git a/server/channels/api4/group.go b/server/channels/api4/group.go index 681f4cad43..d0c2b45dd0 100644 --- a/server/channels/api4/group.go +++ b/server/channels/api4/group.go @@ -831,6 +831,10 @@ func getGroupsByTeamCommon(c *Context, r *http.Request) ([]byte, *model.AppError return nil, model.NewAppError("Api4.getGroupsByTeam", "api.ldap_groups.license_error", nil, "", http.StatusForbidden) } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionListTeamChannels) { + return nil, c.App.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionListTeamChannels}) + } + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, @@ -859,6 +863,7 @@ func getGroupsByTeamCommon(c *Context, r *http.Request) ([]byte, *model.AppError return b, nil } + func getGroupsByChannelCommon(c *Context, r *http.Request) ([]byte, *model.AppError) { if c.App.Channels().License() == nil || !*c.App.Channels().License().Features.LDAPGroups { return nil, model.NewAppError("Api4.getGroupsByChannel", "api.ldap_groups.license_error", nil, "", http.StatusForbidden) @@ -922,6 +927,11 @@ func getGroupsAssociatedToChannelsByTeam(c *Context, w http.ResponseWriter, r *h return } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionListTeamChannels) { + c.Err = c.App.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionListTeamChannels}) + return + } + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, diff --git a/server/channels/api4/group_test.go b/server/channels/api4/group_test.go index 2b2a966017..dd2df8ae7c 100644 --- a/server/channels/api4/group_test.go +++ b/server/channels/api4/group_test.go @@ -1111,6 +1111,23 @@ func TestGetGroupsAssociatedToChannelsByTeam(t *testing.T) { groups, _, err = th.SystemAdminClient.GetGroupsAssociatedToChannelsByTeam(context.Background(), model.NewId(), opts) assert.NoError(t, err) assert.Empty(t, groups) + + t.Run("should get the groups ok when belonging to the team", func(t *testing.T) { + groups, resp, err := th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.NotEmpty(t, groups) + }) + + t.Run("should return forbidden when the user doesn't have the right permissions", func(t *testing.T) { + require.Nil(t, th.App.RemoveUserFromTeam(th.Context, th.BasicTeam.Id, th.BasicUser.Id, th.SystemAdminUser.Id)) + defer th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, th.BasicUser.Id, th.SystemAdminUser.Id) + + groups, resp, err := th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + require.Empty(t, groups) + }) } func TestGetGroupsByTeam(t *testing.T) { @@ -1189,6 +1206,36 @@ func TestGetGroupsByTeam(t *testing.T) { assert.NoError(t, err) assert.Empty(t, groups) }) + + t.Run("groups should be fetched only by users with the right permissions", func(t *testing.T) { + th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { + groups, _, _, err := client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, opts) + require.NoError(t, err) + require.Len(t, groups, 1) + require.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewBool(true)}}, groups) + require.NotNil(t, groups[0].SchemeAdmin) + require.True(t, *groups[0].SchemeAdmin) + }, "groups can be fetched by system admins even if they're not part of a team") + + t.Run("user can fetch groups if it's part of the team", func(t *testing.T) { + groups, _, _, err := th.Client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, opts) + require.NoError(t, err) + require.Len(t, groups, 1) + require.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewBool(true)}}, groups) + require.NotNil(t, groups[0].SchemeAdmin) + require.True(t, *groups[0].SchemeAdmin) + }) + + t.Run("user can't fetch groups if it's not part of the team", func(t *testing.T) { + require.Nil(t, th.App.RemoveUserFromTeam(th.Context, th.BasicTeam.Id, th.BasicUser.Id, th.SystemAdminUser.Id)) + defer th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, th.BasicUser.Id, th.SystemAdminUser.Id) + + groups, _, response, err := th.Client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, opts) + require.Error(t, err) + CheckForbiddenStatus(t, response) + require.Empty(t, groups) + }) + }) } func TestGetGroups(t *testing.T) {