Skip to content

Commit

Permalink
[MM-42421] Prevent guests from seeing users through groups API (#21151)…
Browse files Browse the repository at this point in the history 
… (#21649)

Automatic Merge
  • Loading branch information
@cyrilzhang-mm
cyrilzhang-mm committed Dec 7, 2022
1 parent 435b616 commit 7af0988
Show file tree
Hide file tree
Showing 17 changed files with 406 additions and 177 deletions.
53 changes: 43 additions & 10 deletions api4/group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,15 @@ func getGroup(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext.Session().UserId)
if appErr != nil {
c.Err = appErr
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, &model.GetGroupOpts{
IncludeMemberCount: c.Params.IncludeMemberCount,
})
}, restrictions)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -190,7 +196,7 @@ func patchGroup(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, nil)
group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -300,7 +306,7 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, nil)
group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -611,7 +617,7 @@ func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, nil)
group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil)
if appErr != nil {
c.Err = appErr
return
Expand All @@ -629,7 +635,13 @@ func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

members, count, appErr := c.App.GetGroupMemberUsersPage(c.Params.GroupId, c.Params.Page, c.Params.PerPage)
restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext.Session().UserId)
if appErr != nil {
c.Err = appErr
return
}

members, count, appErr := c.App.GetGroupMemberUsersPage(c.Params.GroupId, c.Params.Page, c.Params.PerPage, restrictions)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -667,7 +679,7 @@ func getGroupStats(c *Context, w http.ResponseWriter, r *http.Request) {
}

groupID := c.Params.GroupId
count, appErr := c.App.GetGroupMemberCount(groupID)
count, appErr := c.App.GetGroupMemberCount(groupID, nil)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -927,12 +939,33 @@ func getGroups(c *Context, w http.ResponseWriter, r *http.Request) {
opts.Since = since
}

groups, appErr := c.App.GetGroups(c.Params.Page, c.Params.PerPage, opts)
restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext.Session().UserId)
if appErr != nil {
c.Err = appErr
return
}

var (
groups = []*model.Group{}
canSee bool = true
)

if opts.FilterHasMember != "" {
canSee, appErr = c.App.UserCanSeeOtherUser(c.AppContext.Session().UserId, opts.FilterHasMember)
if appErr != nil {
c.Err = appErr
return
}
}

if canSee {
groups, appErr = c.App.GetGroups(c.Params.Page, c.Params.PerPage, opts, restrictions)
if appErr != nil {
c.Err = appErr
return
}
}

var (
b []byte
err error
Expand Down Expand Up @@ -966,7 +999,7 @@ func deleteGroup(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, err := c.App.GetGroup(c.Params.GroupId, nil)
group, err := c.App.GetGroup(c.Params.GroupId, nil, nil)
if err != nil {
c.Err = err
return
Expand Down Expand Up @@ -1009,7 +1042,7 @@ func addGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, nil)
group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -1063,7 +1096,7 @@ func deleteGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

group, appErr := c.App.GetGroup(c.Params.GroupId, nil)
group, appErr := c.App.GetGroup(c.Params.GroupId, nil, nil)
if appErr != nil {
c.Err = appErr
return
Expand Down
2 changes: 1 addition & 1 deletion api4/group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1502,7 +1502,7 @@ func TestAddMembersToGroup(t *testing.T) {

assert.Len(t, groupMembers, 2)

count, countErr := th.App.GetGroupMemberCount(group.Id)
count, countErr := th.App.GetGroupMemberCount(group.Id, nil)
assert.Nil(t, countErr)

assert.Equal(t, count, int64(2))
Expand Down
6 changes: 3 additions & 3 deletions api4/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

profiles, _, appErr = c.App.GetGroupMemberUsersPage(inGroupId, c.Params.Page, c.Params.PerPage)
profiles, _, appErr = c.App.GetGroupMemberUsersPage(inGroupId, c.Params.Page, c.Params.PerPage, userGetOptions.ViewRestrictions)
if appErr != nil {
c.Err = appErr
return
Expand All @@ -842,7 +842,7 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

profiles, appErr = c.App.GetUsersNotInGroupPage(notInGroupId, c.Params.Page, c.Params.PerPage)
profiles, appErr = c.App.GetUsersNotInGroupPage(notInGroupId, c.Params.Page, c.Params.PerPage, userGetOptions.ViewRestrictions)
if appErr != nil {
c.Err = appErr
return
Expand Down Expand Up @@ -876,7 +876,7 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
}

func requireGroupAccess(c *web.Context, groupID string) *model.AppError {
group, err := c.App.GetGroup(groupID, nil)
group, err := c.App.GetGroup(groupID, nil, nil)
if err != nil {
return err
}
Expand Down
37 changes: 34 additions & 3 deletions api4/user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2717,13 +2717,26 @@ func TestGetUsersInGroup(t *testing.T) {
})
assert.Nil(t, appErr)

cid := model.NewId()
customGroup, appErr := th.App.CreateGroup(&model.Group{
DisplayName: "dn-foo_" + cid,
Name: model.NewString("name" + cid),
Source: model.GroupSourceCustom,
Description: "description_" + cid,
RemoteId: model.NewString(model.NewId()),
})
assert.Nil(t, appErr)

user1, err := 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, err)

t.Run("Requires ldap license", func(t *testing.T) {
_, response, err := th.SystemAdminClient.GetUsersInGroup(group.Id, 0, 60, "")
require.Error(t, err)
CheckForbiddenStatus(t, response)
})

th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))

t.Run("Requires manage system permission to access users in group", func(t *testing.T) {
th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
Expand All @@ -2732,8 +2745,6 @@ func TestGetUsersInGroup(t *testing.T) {
CheckForbiddenStatus(t, response)
})

user1, err := 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, err)
_, err = th.App.UpsertGroupMember(group.Id, user1.Id)
assert.Nil(t, err)

Expand All @@ -2748,6 +2759,26 @@ func TestGetUsersInGroup(t *testing.T) {
require.NoError(t, err)
assert.Empty(t, users)
})

_, err = th.App.UpsertGroupMember(customGroup.Id, user1.Id)
assert.Nil(t, err)

t.Run("Returns users in custom group when called by regular user", func(t *testing.T) {
th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
users, _, err := th.Client.GetUsersInGroup(customGroup.Id, 0, 60, "")
require.NoError(t, err)
assert.Equal(t, users[0].Id, user1.Id)
})

t.Run("Returns no users in custom group when called by guest user", func(t *testing.T) {
th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
th.App.DemoteUserToGuest(th.Context, th.BasicUser)

users, _, err := th.Client.GetUsersInGroup(customGroup.Id, 0, 60, "")
require.NoError(t, err)
assert.Equal(t, len(users), 0)
})

}

func TestUpdateUserMfa(t *testing.T) {
Expand Down
10 changes: 5 additions & 5 deletions app/app_iface.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 12 additions & 11 deletions app/group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"github.com/mattermost/mattermost-server/v6/store"
)

func (a *App) GetGroup(id string, opts *model.GetGroupOpts) (*model.Group, *model.AppError) {
func (a *App) GetGroup(id string, opts *model.GetGroupOpts, viewRestrictions *model.ViewUsersRestrictions) (*model.Group, *model.AppError) {
group, err := a.Srv().Store.Group().Get(id)
if err != nil {
var nfErr *store.ErrNotFound
Expand All @@ -26,7 +26,7 @@ func (a *App) GetGroup(id string, opts *model.GetGroupOpts) (*model.Group, *mode
}

if opts != nil && opts.IncludeMemberCount {
memberCount, err := a.Srv().Store.Group().GetMemberCount(id)
memberCount, err := a.Srv().Store.Group().GetMemberCountWithRestrictions(id, viewRestrictions)
if err != nil {
return nil, model.NewAppError("GetGroup", "app.member_count", nil, "", http.StatusInternalServerError).Wrap(err)
}
Expand Down Expand Up @@ -217,8 +217,8 @@ func (a *App) DeleteGroup(groupID string) (*model.Group, *model.AppError) {
return deletedGroup, nil
}

func (a *App) GetGroupMemberCount(groupID string) (int64, *model.AppError) {
count, err := a.Srv().Store.Group().GetMemberCount(groupID)
func (a *App) GetGroupMemberCount(groupID string, viewRestrictions *model.ViewUsersRestrictions) (int64, *model.AppError) {
count, err := a.Srv().Store.Group().GetMemberCountWithRestrictions(groupID, viewRestrictions)
if err != nil {
return 0, model.NewAppError("GetGroupMemberCount", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
Expand All @@ -235,20 +235,21 @@ func (a *App) GetGroupMemberUsers(groupID string) ([]*model.User, *model.AppErro
return users, nil
}

func (a *App) GetGroupMemberUsersPage(groupID string, page int, perPage int) ([]*model.User, int, *model.AppError) {
members, err := a.Srv().Store.Group().GetMemberUsersPage(groupID, page, perPage)
func (a *App) GetGroupMemberUsersPage(groupID string, page int, perPage int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, int, *model.AppError) {
members, err := a.Srv().Store.Group().GetMemberUsersPage(groupID, page, perPage, viewRestrictions)
if err != nil {
return nil, 0, model.NewAppError("GetGroupMemberUsersPage", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err)
}

count, appErr := a.GetGroupMemberCount(groupID)
count, appErr := a.GetGroupMemberCount(groupID, viewRestrictions)
if appErr != nil {
return nil, 0, appErr
}
return a.sanitizeProfiles(members, false), int(count), nil
}
func (a *App) GetUsersNotInGroupPage(groupID string, page int, perPage int) ([]*model.User, *model.AppError) {
members, err := a.Srv().Store.Group().GetNonMemberUsersPage(groupID, page, perPage)

func (a *App) GetUsersNotInGroupPage(groupID string, page int, perPage int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) {
members, err := a.Srv().Store.Group().GetNonMemberUsersPage(groupID, page, perPage, viewRestrictions)
if err != nil {
return nil, model.NewAppError("GetUsersNotInGroupPage", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
Expand Down Expand Up @@ -579,8 +580,8 @@ func (a *App) GetGroupsAssociatedToChannelsByTeam(teamID string, opts model.Grou
return groupsAssociatedByChannelId, nil
}

func (a *App) GetGroups(page, perPage int, opts model.GroupSearchOpts) ([]*model.Group, *model.AppError) {
groups, err := a.Srv().Store.Group().GetGroups(page, perPage, opts)
func (a *App) GetGroups(page, perPage int, opts model.GroupSearchOpts, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, *model.AppError) {
groups, err := a.Srv().Store.Group().GetGroups(page, perPage, opts, viewRestrictions)
if err != nil {
return nil, model.NewAppError("GetGroups", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
Expand Down
10 changes: 5 additions & 5 deletions app/group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,19 @@ func TestGetGroup(t *testing.T) {
defer th.TearDown()
group := th.CreateGroup()

group, err := th.App.GetGroup(group.Id, nil)
group, err := th.App.GetGroup(group.Id, nil, nil)
require.Nil(t, err)
require.NotNil(t, group)

nilGroup, err := th.App.GetGroup(model.NewId(), nil)
nilGroup, err := th.App.GetGroup(model.NewId(), nil, nil)
require.NotNil(t, err)
require.Nil(t, nilGroup)

group, err = th.App.GetGroup(group.Id, &model.GetGroupOpts{IncludeMemberCount: false})
group, err = th.App.GetGroup(group.Id, &model.GetGroupOpts{IncludeMemberCount: false}, nil)
require.Nil(t, err)
require.Nil(t, group.MemberCount)

group, err = th.App.GetGroup(group.Id, &model.GetGroupOpts{IncludeMemberCount: true})
group, err = th.App.GetGroup(group.Id, &model.GetGroupOpts{IncludeMemberCount: true}, nil)
require.Nil(t, err)
require.NotNil(t, group.MemberCount)
}
Expand Down Expand Up @@ -369,7 +369,7 @@ func TestGetGroups(t *testing.T) {
defer th.TearDown()
group := th.CreateGroup()

groups, err := th.App.GetGroups(0, 60, model.GroupSearchOpts{})
groups, err := th.App.GetGroups(0, 60, model.GroupSearchOpts{}, nil)
require.Nil(t, err)
require.ElementsMatch(t, []*model.Group{group}, groups)
}
Expand Down
2 changes: 1 addition & 1 deletion app/notification.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ func (a *App) getGroupsAllowedForReferenceInChannel(channel *model.Channel, team
return groupsMap, nil
}

groups, err := a.Srv().Store.Group().GetGroups(0, 0, opts)
groups, err := a.Srv().Store.Group().GetGroups(0, 0, opts, nil)
if err != nil {
return nil, errors.Wrap(err, "unable to get groups")
}
Expand Down

0 comments on commit 7af0988

Please sign in to comment.