opensource/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2026-09-01 15:00:08 +08:00
Code Issues Packages Projects Releases Wiki Activity

Fix permissions in GetGroupsByNames (#35119)

Browse Source 
The reliance on ViewUsersRestrictions was causing a SQL bug, since the
original query did not join with the Users table. Instead, use
model.GroupSearchOpts to rely on AllowReference, which should be used to
filter the results in all cases, except when the user is a sysadmin (has
the PermissionSysconsoleReadUserManagementGroups permission).

Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Alejandro García Montoro
2026-02-09 11:13:48 +01:00
committed by GitHub
parent 892492a0a8
commit 139ff4ded2
8 changed files with 112 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/channels/api4/group.go
+5 -5
View File
@@ -873,13 +873,13 @@ func getGroupsByNames(c *Context, w http.ResponseWriter, r *http.Request) {
return
}
restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext, c.AppContext.Session().UserId)
if appErr != nil {
c.Err = appErr
return
filterAllowReference := !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups)
opts := model.GroupSearchOpts{
FilterAllowReference: filterAllowReference,
}
groups, appErr := c.App.GetGroupsByNames(groupNames, restrictions)
groups, appErr := c.App.GetGroupsByNames(groupNames, opts)
if appErr != nil {
c.Err = appErr
return
server/channels/api4/group_test.go
+85
View File
@@ -2350,6 +2350,91 @@ func TestGetGroupsByNames(t *testing.T) {
})
}
func TestGetGroupsByNamesAllowReference(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
// Create group with AllowReference=true
id1 := model.NewId()
groupAllowRef, appErr := th.App.CreateGroup(&model.Group{
DisplayName: "dn-allow_" + id1,
Name: model.NewPointer("allow" + id1),
Source: model.GroupSourceLdap,
RemoteId: model.NewPointer(model.NewId()),
AllowReference: true,
})
require.Nil(t, appErr)
// Create group with AllowReference=false
id2 := model.NewId()
groupNoRef, appErr := th.App.CreateGroup(&model.Group{
DisplayName: "dn-noref_" + id2,
Name: model.NewPointer("noref" + id2),
Source: model.GroupSourceLdap,
RemoteId: model.NewPointer(model.NewId()),
AllowReference: false,
})
require.Nil(t, appErr)
// Login as regular user
th.LoginBasic(t)
tests := []struct {
name string
client *model.Client4
searchNames []string
expectedGroups []*model.Group
description string
}{
{
name: "admin sees all groups",
client: th.SystemAdminClient,
searchNames: []string{*groupAllowRef.Name, *groupNoRef.Name},
expectedGroups: []*model.Group{groupAllowRef, groupNoRef},
description: "admin with sysconsole permission should see all groups",
},
{
name: "admin sees group with AllowReference=false",
client: th.SystemAdminClient,
searchNames: []string{*groupNoRef.Name},
expectedGroups: []*model.Group{groupNoRef},
description: "admin should see group even when AllowReference=false",
},
{
name: "regular user sees only AllowReference=true",
client: th.Client,
searchNames: []string{*groupAllowRef.Name, *groupNoRef.Name},
expectedGroups: []*model.Group{groupAllowRef},
description: "regular user should only see groups with AllowReference=true",
},
{
name: "regular user cannot see AllowReference=false group",
client: th.Client,
searchNames: []string{*groupNoRef.Name},
expectedGroups: []*model.Group{},
description: "regular user should not see groups with AllowReference=false",
},
{
name: "regular user sees AllowReference=true group",
client: th.Client,
searchNames: []string{*groupAllowRef.Name},
expectedGroups: []*model.Group{groupAllowRef},
description: "regular user should see groups with AllowReference=true",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
groups, resp, err := tc.client.GetGroupsByNames(context.Background(), tc.searchNames)
require.NoError(t, err)
CheckOKStatus(t, resp)
assert.ElementsMatch(t, tc.expectedGroups, groups, tc.description)
})
}
}
func TestGetGroupsByUserId(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
server/channels/app/group.go
+2 -2
View File
@@ -98,8 +98,8 @@ func (a *App) GetGroupsByUserId(userID string, opts model.GroupSearchOpts) ([]*m
return groups, nil
}
func (a *App) GetGroupsByNames(names []string, restrictions *model.ViewUsersRestrictions) ([]*model.Group, *model.AppError) {
groups, err := a.Srv().Store().Group().GetByNames(names, restrictions)
func (a *App) GetGroupsByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, *model.AppError) {
groups, err := a.Srv().Store().Group().GetByNames(names, opts)
if err != nil {
return nil, model.NewAppError("GetGroupsByNames", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
server/channels/store/retrylayer/retrylayer.go
+2 -2
View File
@@ -5903,11 +5903,11 @@ func (s *RetryLayerGroupStore) GetByName(name string, opts model.GroupSearchOpts
}
func (s *RetryLayerGroupStore) GetByNames(names []string, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, error) {
func (s *RetryLayerGroupStore) GetByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, error) {
tries := 0
for {
result, err := s.GroupStore.GetByNames(names, viewRestrictions)
result, err := s.GroupStore.GetByNames(names, opts)
if err == nil {
return result, nil
}
server/channels/store/sqlstore/group_store.go
+6 -2
View File
@@ -296,10 +296,14 @@ func (s *SqlGroupStore) GetByName(name string, opts model.GroupSearchOpts) (*mod
return &group, nil
}
func (s *SqlGroupStore) GetByNames(names []string, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, error) {
func (s *SqlGroupStore) GetByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, error) {
groups := []*model.Group{}
query := s.userGroupsSelectQuery.Where(sq.Eq{"Name": names})
query = applyViewRestrictionsFilter(query, viewRestrictions, true)
if opts.FilterAllowReference {
query = query.Where("AllowReference = true")
}
if err := s.GetReplica().SelectBuilder(&groups, query); err != nil {
return nil, errors.Wrap(err, "failed to find Groups by names")
}
server/channels/store/store.go
+1 -1
View File
@@ -898,7 +898,7 @@ type GroupStore interface {
CreateWithUserIds(group *model.GroupWithUserIds) (*model.Group, error)
Get(groupID string) (*model.Group, error)
GetByName(name string, opts model.GroupSearchOpts) (*model.Group, error)
GetByNames(names []string, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, error)
GetByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, error)
GetByIDs(groupIDs []string) ([]*model.Group, error)
GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, error)
GetAllBySource(groupSource model.GroupSource) ([]*model.Group, error)
server/channels/store/storetest/mocks/GroupStore.go
+9 -9
View File
@@ -662,9 +662,9 @@ func (_m *GroupStore) GetByName(name string, opts model.GroupSearchOpts) (*model
return r0, r1
}
// GetByNames provides a mock function with given fields: names, viewRestrictions
func (_m *GroupStore) GetByNames(names []string, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, error) {
ret := _m.Called(names, viewRestrictions)
// GetByNames provides a mock function with given fields: names, opts
func (_m *GroupStore) GetByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, error) {
ret := _m.Called(names, opts)
if len(ret) == 0 {
panic("no return value specified for GetByNames")
@@ -672,19 +672,19 @@ func (_m *GroupStore) GetByNames(names []string, viewRestrictions *model.ViewUse
var r0 []*model.Group
var r1 error
if rf, ok := ret.Get(0).(func([]string, *model.ViewUsersRestrictions) ([]*model.Group, error)); ok {
return rf(names, viewRestrictions)
if rf, ok := ret.Get(0).(func([]string, model.GroupSearchOpts) ([]*model.Group, error)); ok {
return rf(names, opts)
}
if rf, ok := ret.Get(0).(func([]string, *model.ViewUsersRestrictions) []*model.Group); ok {
r0 = rf(names, viewRestrictions)
if rf, ok := ret.Get(0).(func([]string, model.GroupSearchOpts) []*model.Group); ok {
r0 = rf(names, opts)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).([]*model.Group)
}
}
if rf, ok := ret.Get(1).(func([]string, *model.ViewUsersRestrictions) error); ok {
r1 = rf(names, viewRestrictions)
if rf, ok := ret.Get(1).(func([]string, model.GroupSearchOpts) error); ok {
r1 = rf(names, opts)
} else {
r1 = ret.Error(1)
}
server/channels/store/timerlayer/timerlayer.go
+2 -2
View File
@@ -4805,10 +4805,10 @@ func (s *TimerLayerGroupStore) GetByName(name string, opts model.GroupSearchOpts
return result, err
}
func (s *TimerLayerGroupStore) GetByNames(names []string, viewRestrictions *model.ViewUsersRestrictions) ([]*model.Group, error) {
func (s *TimerLayerGroupStore) GetByNames(names []string, opts model.GroupSearchOpts) ([]*model.Group, error) {
start := time.Now()
result, err := s.GroupStore.GetByNames(names, viewRestrictions)
result, err := s.GroupStore.GetByNames(names, opts)
elapsed := float64(time.Since(start)) / float64(time.Second)
if s.Root.Metrics != nil {