From df75277a0c31d436c0febc03619112d50c713e3d Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Fri, 12 Apr 2024 19:41:50 +0200 Subject: [PATCH] [MM-55323] Allow end users to fetch the group members list of groups which allow @-mentions (#26551) * Allow end users to fetch the group members list of groups which allow @-mentions * Update server/channels/api4/group_test.go Co-authored-by: Ibrahim Serdar Acikgoz * Fix test name * Move into subtest --------- Co-authored-by: Ibrahim Serdar Acikgoz --- server/channels/api4/group.go | 39 ++++++++++------- server/channels/api4/group_test.go | 66 +++++++++++++++++++++++++++++ server/channels/api4/user.go | 28 ++---------- server/public/model/client4.go | 13 ++++++ server/public/model/group_member.go | 5 +++ 5 files changed, 111 insertions(+), 40 deletions(-) diff --git a/server/channels/api4/group.go b/server/channels/api4/group.go index 92fa623a69..58ddc507ca 100644 --- a/server/channels/api4/group.go +++ b/server/channels/api4/group.go @@ -16,6 +16,7 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/app" "github.com/mattermost/mattermost/server/v8/channels/audit" "github.com/mattermost/mattermost/server/v8/channels/store" + "github.com/mattermost/mattermost/server/v8/channels/web" ) func (api *API) InitGroup() { @@ -695,24 +696,13 @@ func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } - group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil) - if appErr != nil { - c.Err = appErr - return - } - - appErr = licensedAndConfiguredForGroupBySource(c.App, group.Source) + appErr := hasPermissionToReadGroupMembers(c, c.Params.GroupId) if appErr != nil { appErr.Where = "Api4.getGroupMembers" c.Err = appErr return } - if group.Source == model.GroupSourceLdap && !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) { - c.SetPermissionError(model.PermissionSysconsoleReadUserManagementGroups) - return - } - restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext, c.AppContext.Session().UserId) if appErr != nil { c.Err = appErr @@ -725,10 +715,7 @@ func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } - b, err := json.Marshal(struct { - Members []*model.User `json:"members"` - Count int `json:"total_member_count"` - }{ + b, err := json.Marshal(model.GroupMemberList{ Members: members, Count: count, }) @@ -1376,6 +1363,26 @@ func deleteGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) { w.Write(b) } +// hasPermissionToReadGroupMembers check if a user has the permission to read the list of members of a given team. +func hasPermissionToReadGroupMembers(c *web.Context, groupID string) *model.AppError { + group, err := c.App.GetGroup(groupID, nil, nil) + if err != nil { + return err + } + + if lcErr := licensedAndConfiguredForGroupBySource(c.App, group.Source); lcErr != nil { + return lcErr + } + + if group.Source == model.GroupSourceLdap && !group.AllowReference { + if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) { + return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionSysconsoleReadUserManagementGroups}) + } + } + + return nil +} + // licensedAndConfiguredForGroupBySource returns an app error if not properly license or configured for the given group type. The returned app error // will have a blank 'Where' field, which should be subsequently set by the caller, for example: // diff --git a/server/channels/api4/group_test.go b/server/channels/api4/group_test.go index b4e9f8b9e7..411c91fda6 100644 --- a/server/channels/api4/group_test.go +++ b/server/channels/api4/group_test.go @@ -1704,6 +1704,72 @@ func TestGetGroupsByUserId(t *testing.T) { assert.ElementsMatch(t, []*model.Group{group1, group2}, groups) } +func TestGetGroupMembers(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + id := model.NewId() + group, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn-foo_" + id, + Name: model.NewString("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewString(model.NewId()), + }) + assert.Nil(t, appErr) + + user1, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user1", Password: "test-password-1", Username: "test-user-1", Roles: model.SystemUserRoleId}) + assert.Nil(t, appErr) + + user2, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user2", Password: "test-password-2", Username: "test-user-2", Roles: model.SystemUserRoleId}) + assert.Nil(t, appErr) + + _, appErr = th.App.UpsertGroupMembers(group.Id, []string{user1.Id, user2.Id}) + require.Nil(t, appErr) + + t.Run("Requires ldap license", func(t *testing.T) { + members, response, err := th.SystemAdminClient.GetGroupMembers(context.Background(), group.Id) + assert.Error(t, err) + CheckNotImplementedStatus(t, response) + assert.Nil(t, members) + }) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + t.Run("Non admins are not allowed to get members for LDAP groups", func(t *testing.T) { + members, response, err := th.Client.GetGroupMembers(context.Background(), group.Id) + assert.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, members) + }) + + t.Run("Admins are allowed to get members for LDAP groups", func(t *testing.T) { + members, response, err := th.SystemAdminClient.GetGroupMembers(context.Background(), group.Id) + assert.NoError(t, err) + CheckOKStatus(t, response) + require.NotNil(t, members) + assert.Equal(t, 2, members.Count) + }) + + t.Run("If AllowReference is enabled, non admins are allowed to get members for LDAP groups", func(t *testing.T) { + group.AllowReference = true + group, appErr = th.App.UpdateGroup(group) + assert.Nil(t, appErr) + + t.Cleanup(func() { + group.AllowReference = false + group, appErr = th.App.UpdateGroup(group) + assert.Nil(t, appErr) + }) + + members, response, err := th.Client.GetGroupMembers(context.Background(), group.Id) + assert.NoError(t, err) + CheckOKStatus(t, response) + require.NotNil(t, members) + assert.Equal(t, 2, members.Count) + }) +} + func TestGetGroupStats(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 4a9b0acb73..0584c8c8a9 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -20,7 +20,6 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/audit" "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/channels/utils" - "github.com/mattermost/mattermost/server/v8/channels/web" ) func (api *API) InitUser() { @@ -875,7 +874,7 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { profiles, appErr = c.App.GetUsersInChannelPage(userGetOptions, c.IsSystemAdmin()) } } else if inGroupId != "" { - if gErr := requireGroupAccess(c, inGroupId); gErr != nil { + if gErr := hasPermissionToReadGroupMembers(c, inGroupId); gErr != nil { gErr.Where = "Api.getUsers" c.Err = gErr return @@ -895,7 +894,7 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { profiles, _, appErr = c.App.GetGroupMemberUsersPage(inGroupId, c.Params.Page, c.Params.PerPage, userGetOptions.ViewRestrictions) } } else if notInGroupId != "" { - appErr = requireGroupAccess(c, notInGroupId) + appErr = hasPermissionToReadGroupMembers(c, notInGroupId) if appErr != nil { appErr.Where = "Api.getUsers" c.Err = appErr @@ -935,25 +934,6 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { w.Write(js) } -func requireGroupAccess(c *web.Context, groupID string) *model.AppError { - group, err := c.App.GetGroup(groupID, nil, nil) - if err != nil { - return err - } - - if lcErr := licensedAndConfiguredForGroupBySource(c.App, group.Source); lcErr != nil { - return lcErr - } - - if group.Source == model.GroupSourceLdap { - if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) { - return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionSysconsoleReadUserManagementGroups}) - } - } - - return nil -} - func getUsersByIds(c *Context, w http.ResponseWriter, r *http.Request) { userIDs, err := model.SortedArrayFromJSON(r.Body) if err != nil { @@ -1067,7 +1047,7 @@ func searchUsers(c *Context, w http.ResponseWriter, r *http.Request) { } if props.InGroupId != "" { - if appErr := requireGroupAccess(c, props.InGroupId); appErr != nil { + if appErr := hasPermissionToReadGroupMembers(c, props.InGroupId); appErr != nil { appErr.Where = "Api.searchUsers" c.Err = appErr return @@ -1075,7 +1055,7 @@ func searchUsers(c *Context, w http.ResponseWriter, r *http.Request) { } if props.NotInGroupId != "" { - if appErr := requireGroupAccess(c, props.NotInGroupId); appErr != nil { + if appErr := hasPermissionToReadGroupMembers(c, props.NotInGroupId); appErr != nil { appErr.Where = "Api.searchUsers" c.Err = appErr return diff --git a/server/public/model/client4.go b/server/public/model/client4.go index f1074186d9..c0605cf4f6 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -7690,6 +7690,19 @@ func (c *Client4) PatchGroup(ctx context.Context, groupID string, patch *GroupPa return &g, BuildResponse(r), nil } +func (c *Client4) GetGroupMembers(ctx context.Context, groupID string) (*GroupMemberList, *Response, error) { + r, err := c.DoAPIGet(ctx, c.groupRoute(groupID)+"/members", "") + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + var ml GroupMemberList + if err := json.NewDecoder(r.Body).Decode(&ml); err != nil { + return nil, nil, NewAppError("UpsertGroupMembers", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + return &ml, BuildResponse(r), nil +} + func (c *Client4) UpsertGroupMembers(ctx context.Context, groupID string, userIds *GroupModifyMembers) ([]*GroupMember, *Response, error) { payload, err := json.Marshal(userIds) if err != nil { diff --git a/server/public/model/group_member.go b/server/public/model/group_member.go index d18d78499b..9cd4e9a5ec 100644 --- a/server/public/model/group_member.go +++ b/server/public/model/group_member.go @@ -21,3 +21,8 @@ func (gm *GroupMember) IsValid() *AppError { } return nil } + +type GroupMemberList struct { + Members []*User `json:"members"` + Count int `json:"total_member_count"` +}