From ce9632cca3107b5f79e47b9b94657a5ca0182f69 Mon Sep 17 00:00:00 2001 From: Ben Cooke Date: Mon, 31 Mar 2025 15:49:55 -0400 Subject: [PATCH] MM-63311 (#30387) * allow reference group changes --- server/channels/api4/group.go | 28 +- server/channels/api4/group_test.go | 470 ++++++++++++++---- server/channels/app/group.go | 4 +- server/channels/app/plugin_api.go | 2 +- .../channels/store/retrylayer/retrylayer.go | 4 +- server/channels/store/sqlstore/group_store.go | 10 +- server/channels/store/store.go | 2 +- .../channels/store/storetest/group_store.go | 22 +- .../store/storetest/mocks/GroupStore.go | 18 +- .../channels/store/timerlayer/timerlayer.go | 4 +- 10 files changed, 441 insertions(+), 123 deletions(-) diff --git a/server/channels/api4/group.go b/server/channels/api4/group.go index f04b95ff83..8339f73882 100644 --- a/server/channels/api4/group.go +++ b/server/channels/api4/group.go @@ -129,7 +129,7 @@ func getGroup(c *Context, w http.ResponseWriter, r *http.Request) { return } - if group.Source == model.GroupSourceLdap { + if !group.AllowReference { if !c.App.SessionHasPermissionToGroup(*c.AppContext.Session(), c.Params.GroupId, model.PermissionSysconsoleReadUserManagementGroups) { c.SetPermissionError(model.PermissionSysconsoleReadUserManagementGroups) return @@ -818,7 +818,13 @@ func getGroupsByUserId(c *Context, w http.ResponseWriter, r *http.Request) { return } - groups, appErr := c.App.GetGroupsByUserId(c.Params.UserId) + filterAllowReference := !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) + + opts := model.GroupSearchOpts{ + FilterAllowReference: filterAllowReference, + } + + groups, appErr := c.App.GetGroupsByUserId(c.Params.UserId, opts) if appErr != nil { c.Err = appErr return @@ -885,10 +891,12 @@ func getGroupsByTeamCommon(c *Context, r *http.Request) ([]byte, *model.AppError return nil, model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionListTeamChannels}) } + filterAllowReference := c.Params.FilterAllowReference || !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, - FilterAllowReference: c.Params.FilterAllowReference, + FilterAllowReference: filterAllowReference, } if c.Params.Paginate == nil || *c.Params.Paginate { opts.PageOpts = &model.PageOpts{Page: c.Params.Page, PerPage: c.Params.PerPage} @@ -934,10 +942,12 @@ func getGroupsByChannelCommon(c *Context, r *http.Request) ([]byte, *model.AppEr return nil, model.MakePermissionError(c.AppContext.Session(), []*model.Permission{permission}) } + filterAllowReference := c.Params.FilterAllowReference || !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, - FilterAllowReference: c.Params.FilterAllowReference, + FilterAllowReference: filterAllowReference, } if c.Params.Paginate == nil || *c.Params.Paginate { opts.PageOpts = &model.PageOpts{Page: c.Params.Page, PerPage: c.Params.PerPage} @@ -982,10 +992,12 @@ func getGroupsAssociatedToChannelsByTeam(c *Context, w http.ResponseWriter, r *h return } + filterAllowReference := c.Params.FilterAllowReference || !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, - FilterAllowReference: c.Params.FilterAllowReference, + FilterAllowReference: filterAllowReference, } if c.Params.Paginate == nil || *c.Params.Paginate { opts.PageOpts = &model.PageOpts{Page: c.Params.Page, PerPage: c.Params.PerPage} @@ -1054,10 +1066,12 @@ func getGroups(c *Context, w http.ResponseWriter, r *http.Request) { // Include archived groups includeArchived := r.URL.Query().Get("include_archived") == "true" + filterAllowReference := c.Params.FilterAllowReference || !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) + opts := model.GroupSearchOpts{ Q: c.Params.Q, IncludeMemberCount: c.Params.IncludeMemberCount, - FilterAllowReference: c.Params.FilterAllowReference, + FilterAllowReference: filterAllowReference, FilterArchived: c.Params.FilterArchived, FilterParentTeamPermitted: c.Params.FilterParentTeamPermitted, Source: source, @@ -1439,7 +1453,7 @@ func hasPermissionToReadGroupMembers(c *web.Context, groupID string) *model.AppE return lcErr } - if group.Source == model.GroupSourceLdap && !group.AllowReference { + if group.IsSyncable() && !group.AllowReference { if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementGroups) { return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionSysconsoleReadUserManagementGroups}) } diff --git a/server/channels/api4/group_test.go b/server/channels/api4/group_test.go index d9f62dec2c..022d98deb5 100644 --- a/server/channels/api4/group_test.go +++ b/server/channels/api4/group_test.go @@ -1229,11 +1229,36 @@ func TestGetGroupsByChannel(t *testing.T) { id := model.NewId() group, appErr := th.App.CreateGroup(&model.Group{ - DisplayName: "dn_" + id, - Name: model.NewPointer("name" + id), - Source: model.GroupSourceLdap, - Description: "description_" + id, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + assert.Nil(t, appErr) + + // Create a group with AllowReference=false + id2 := model.NewId() + groupNoRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id2, + Name: model.NewPointer("name" + id2), + Source: model.GroupSourceLdap, + Description: "description_" + id2, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: false, + }) + assert.Nil(t, appErr) + + // Create a group with AllowReference=true + id3 := model.NewId() + groupWithRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id3, + Name: model.NewPointer("name" + id3), + Source: model.GroupSourceLdap, + Description: "description_" + id3, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, }) assert.Nil(t, appErr) @@ -1245,6 +1270,22 @@ func TestGetGroupsByChannel(t *testing.T) { }) assert.Nil(t, appErr) + _, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicChannel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: groupNoRef.Id, + }) + assert.Nil(t, appErr) + + _, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicChannel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: groupWithRef.Id, + }) + assert.Nil(t, appErr) + opts := model.GroupSearchOpts{ PageOpts: &model.PageOpts{ Page: 0, @@ -1284,9 +1325,21 @@ func TestGetGroupsByChannel(t *testing.T) { var groups []*model.GroupWithSchemeAdmin groups, _, _, err = client.GetGroupsByChannel(context.Background(), th.BasicChannel.Id, opts) assert.NoError(t, err) - assert.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewPointer(false)}}, groups) - require.NotNil(t, groups[0].SchemeAdmin) - require.False(t, *groups[0].SchemeAdmin) + assert.Len(t, groups, 3) + + // Admin should see all groups + foundNoRef := false + foundWithRef := false + for _, g := range groups { + if g.Group.Id == groupNoRef.Id { + foundNoRef = true + } + if g.Group.Id == groupWithRef.Id { + foundWithRef = true + } + } + assert.True(t, foundNoRef, "Admin should see groups with AllowReference=false") + assert.True(t, foundWithRef, "Admin should see groups with AllowReference=true") }) // set syncable to true @@ -1297,10 +1350,15 @@ func TestGetGroupsByChannel(t *testing.T) { th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { groups, _, _, err := client.GetGroupsByChannel(context.Background(), th.BasicChannel.Id, opts) assert.NoError(t, err) - // ensure that SchemeAdmin field is updated - assert.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewPointer(true)}}, groups) - require.NotNil(t, groups[0].SchemeAdmin) - require.True(t, *groups[0].SchemeAdmin) + assert.Len(t, groups, 3) + + // Verify SchemeAdmin field is updated for the first group + for _, g := range groups { + if g.Group.Id == group.Id { + require.NotNil(t, g.SchemeAdmin) + require.True(t, *g.SchemeAdmin) + } + } groups, _, _, err = client.GetGroupsByChannel(context.Background(), model.NewId(), opts) CheckErrorID(t, err, "app.channel.get.existing.app_error") @@ -1322,6 +1380,30 @@ func TestGetGroupsAssociatedToChannelsByTeam(t *testing.T) { }) assert.Nil(t, appErr) + // Create a group with AllowReference=false + id2 := model.NewId() + groupNoRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id2, + Name: model.NewPointer("name" + id2), + Source: model.GroupSourceLdap, + Description: "description_" + id2, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: false, + }) + assert.Nil(t, appErr) + + // Create a group with AllowReference=true + id3 := model.NewId() + groupWithRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id3, + Name: model.NewPointer("name" + id3), + Source: model.GroupSourceLdap, + Description: "description_" + id3, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + assert.Nil(t, appErr) + groupSyncable, appErr := th.App.UpsertGroupSyncable(&model.GroupSyncable{ AutoAdd: true, SyncableId: th.BasicChannel.Id, @@ -1330,6 +1412,22 @@ func TestGetGroupsAssociatedToChannelsByTeam(t *testing.T) { }) assert.Nil(t, appErr) + _, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicChannel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: groupNoRef.Id, + }) + assert.Nil(t, appErr) + + _, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicChannel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: groupWithRef.Id, + }) + assert.Nil(t, appErr) + opts := model.GroupSearchOpts{ PageOpts: &model.PageOpts{ Page: 0, @@ -1354,39 +1452,53 @@ func TestGetGroupsAssociatedToChannelsByTeam(t *testing.T) { groups, _, err := th.SystemAdminClient.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) assert.NoError(t, err) - assert.Equal(t, map[string][]*model.GroupWithSchemeAdmin{ - th.BasicChannel.Id: { - {Group: *group, SchemeAdmin: model.NewPointer(false)}, - }, - }, groups) + // Admin should see all groups + assert.Len(t, groups[th.BasicChannel.Id], 3) - require.NotNil(t, groups[th.BasicChannel.Id][0].SchemeAdmin) - require.False(t, *groups[th.BasicChannel.Id][0].SchemeAdmin) + foundNoRef := false + foundWithRef := false + for _, g := range groups[th.BasicChannel.Id] { + if g.Group.Id == groupNoRef.Id { + foundNoRef = true + } + if g.Group.Id == groupWithRef.Id { + foundWithRef = true + } + } + assert.True(t, foundNoRef, "Admin should see groups with AllowReference=false") + assert.True(t, foundWithRef, "Admin should see groups with AllowReference=true") // set syncable to true groupSyncable.SchemeAdmin = true _, appErr = th.App.UpdateGroupSyncable(groupSyncable) require.Nil(t, appErr) - // ensure that SchemeAdmin field is updated - groups, _, err = th.SystemAdminClient.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) - assert.NoError(t, err) + // Test with regular user and FilterAllowReference + t.Run("regular user with FilterAllowReference", func(t *testing.T) { + optsWithFilter := opts + optsWithFilter.FilterAllowReference = true - assert.Equal(t, map[string][]*model.GroupWithSchemeAdmin{ - th.BasicChannel.Id: { - {Group: *group, SchemeAdmin: model.NewPointer(true)}, - }, - }, groups) + groups, _, err = th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, optsWithFilter) + assert.NoError(t, err) - require.NotNil(t, groups[th.BasicChannel.Id][0].SchemeAdmin) - require.True(t, *groups[th.BasicChannel.Id][0].SchemeAdmin) + // Regular user should only see groups with AllowReference=true + for _, groupList := range groups { + for _, g := range groupList { + if g.Group.Id == groupWithRef.Id { + assert.True(t, g.Group.AllowReference) + } + assert.NotEqual(t, g.Group.Id, groupNoRef.Id, "Non-admin user should not see groups with AllowReference=false") + } + } + }) 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) + var resp *model.Response + groups, resp, err = th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) require.NoError(t, err) CheckOKStatus(t, resp) require.NotEmpty(t, groups) @@ -1398,7 +1510,8 @@ func TestGetGroupsAssociatedToChannelsByTeam(t *testing.T) { _, _, appErr := th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, th.BasicUser.Id, th.SystemAdminUser.Id) require.Nil(t, appErr) }() - groups, resp, err := th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) + var resp *model.Response + groups, resp, err = th.Client.GetGroupsAssociatedToChannelsByTeam(context.Background(), th.BasicTeam.Id, opts) require.Error(t, err) CheckForbiddenStatus(t, resp) require.Empty(t, groups) @@ -1411,11 +1524,34 @@ func TestGetGroupsByTeam(t *testing.T) { id := model.NewId() group, err := th.App.CreateGroup(&model.Group{ - DisplayName: "dn_" + id, - Name: model.NewPointer("name" + id), - Source: model.GroupSourceLdap, - Description: "description_" + id, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn1_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + assert.Nil(t, err) + + id2 := model.NewId() + groupNoRef, err := th.App.CreateGroup(&model.Group{ + DisplayName: "dn2_" + id2, + Name: model.NewPointer("name" + id2), + Source: model.GroupSourceLdap, + Description: "description_" + id2, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: false, + }) + assert.Nil(t, err) + + id3 := model.NewId() + groupWithRef, err := th.App.CreateGroup(&model.Group{ + DisplayName: "dn3_" + id3, + Name: model.NewPointer("name" + id3), + Source: model.GroupSourceLdap, + Description: "description_" + id3, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, }) assert.Nil(t, err) @@ -1427,6 +1563,22 @@ func TestGetGroupsByTeam(t *testing.T) { }) assert.Nil(t, err) + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicTeam.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: groupNoRef.Id, + }) + assert.Nil(t, err) + + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: th.BasicTeam.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: groupWithRef.Id, + }) + assert.Nil(t, err) + opts := model.GroupSearchOpts{ PageOpts: &model.PageOpts{ Page: 0, @@ -1460,7 +1612,21 @@ func TestGetGroupsByTeam(t *testing.T) { th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { groups, _, _, err := client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, opts) assert.NoError(t, err) - assert.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewPointer(false)}}, groups) + existingGroups := []*model.GroupWithSchemeAdmin{ + { + Group: *group, + SchemeAdmin: model.NewPointer(false), + }, + { + Group: *groupNoRef, + SchemeAdmin: model.NewPointer(false), + }, + { + Group: *groupWithRef, + SchemeAdmin: model.NewPointer(false), + }, + } + assert.ElementsMatch(t, existingGroups, groups) require.NotNil(t, groups[0].SchemeAdmin) require.False(t, *groups[0].SchemeAdmin) }) @@ -1473,10 +1639,22 @@ func TestGetGroupsByTeam(t *testing.T) { th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { groups, _, _, err := client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, opts) assert.NoError(t, err) - // ensure that SchemeAdmin field is updated - assert.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewPointer(true)}}, groups) - require.NotNil(t, groups[0].SchemeAdmin) - require.True(t, *groups[0].SchemeAdmin) + existingGroups := []*model.GroupWithSchemeAdmin{ + { + Group: *group, + SchemeAdmin: model.NewPointer(true), + }, + { + Group: *groupNoRef, + SchemeAdmin: model.NewPointer(false), + }, + { + Group: *groupWithRef, + SchemeAdmin: model.NewPointer(false), + }, + } + + assert.ElementsMatch(t, existingGroups, groups) groups, _, _, err = client.GetGroupsByTeam(context.Background(), model.NewId(), opts) assert.NoError(t, err) @@ -1487,19 +1665,34 @@ func TestGetGroupsByTeam(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.NewPointer(true)}}, groups) - require.NotNil(t, groups[0].SchemeAdmin) - require.True(t, *groups[0].SchemeAdmin) + require.Len(t, groups, 3) + // Admin should see all groups + foundNoRef := false + foundWithRef := false + for _, g := range groups { + if g.Group.Id == groupNoRef.Id { + foundNoRef = true + } + if g.Group.Id == groupWithRef.Id { + foundWithRef = true + } + } + assert.True(t, foundNoRef, "Admin should see groups with AllowReference=false") + assert.True(t, foundWithRef, "Admin should see groups with AllowReference=true") }, "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) + optsWithFilter := opts + + groups, _, _, err := th.Client.GetGroupsByTeam(context.Background(), th.BasicTeam.Id, optsWithFilter) require.NoError(t, err) - require.Len(t, groups, 1) - require.ElementsMatch(t, []*model.GroupWithSchemeAdmin{{Group: *group, SchemeAdmin: model.NewPointer(true)}}, groups) - require.NotNil(t, groups[0].SchemeAdmin) - require.True(t, *groups[0].SchemeAdmin) + + for _, g := range groups { + if g.Group.Id == groupWithRef.Id { + assert.True(t, g.Group.AllowReference) + } + assert.NotEqual(t, g.Group.Id, groupNoRef.Id, "Non-admin user should not see groups with AllowReference=false") + } }) t.Run("user can't fetch groups if it's not part of the team", func(t *testing.T) { @@ -1536,11 +1729,36 @@ func TestGetGroups(t *testing.T) { id2 := model.NewId() group2, appErr := th.App.CreateGroup(&model.Group{ - DisplayName: "dn-foo_" + id2, - Name: model.NewPointer("name" + id2), - Source: model.GroupSourceCustom, - Description: "description_" + id2, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn-foo_" + id2, + Name: model.NewPointer("name" + id2), + Source: model.GroupSourceCustom, + Description: "description_" + id2, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + assert.Nil(t, appErr) + + // Create a group with AllowReference=false + id3 := model.NewId() + groupNoRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn-foo_" + id3, + Name: model.NewPointer("name" + id3), + Source: model.GroupSourceLdap, + Description: "description_" + id3, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: false, + }) + assert.Nil(t, appErr) + + // Create a group with AllowReference=true + id4 := model.NewId() + groupWithRef, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn-foo_" + id4, + Name: model.NewPointer("name" + id4), + Source: model.GroupSourceLdap, + Description: "description_" + id4, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, }) assert.Nil(t, appErr) @@ -1565,17 +1783,49 @@ func TestGetGroups(t *testing.T) { groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) - assert.ElementsMatch(t, []*model.Group{group, group2, th.Group}, groups) + assert.ElementsMatch(t, []*model.Group{group, th.Group, groupNoRef, groupWithRef, group2}, groups) assert.Nil(t, groups[0].MemberCount) }) - t.Run("basic search for LDAP groups", func(t *testing.T) { + t.Run("test FilterAllowReference for non-admin user", func(t *testing.T) { opts := baseOpts - groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) + opts.FilterAllowReference = true + + _, _, err := th.Client.Login(context.Background(), th.BasicUser.Email, th.BasicUser.Password) require.NoError(t, err) - CheckOKStatus(t, resp) - assert.ElementsMatch(t, []*model.Group{group, th.Group}, groups) - assert.Nil(t, groups[0].MemberCount) + + groups, _, err := th.Client.GetGroups(context.Background(), opts) + require.NoError(t, err) + + for _, g := range groups { + if g.Id == groupWithRef.Id { + assert.True(t, g.AllowReference) + } + assert.NotEqual(t, g.Id, groupNoRef.Id, "Non-admin user should not see groups with AllowReference=false") + } + + _, _, err = th.SystemAdminClient.Login(context.Background(), th.SystemAdminUser.Email, th.SystemAdminUser.Password) + require.NoError(t, err) + }) + + t.Run("test FilterAllowReference for admin user", func(t *testing.T) { + opts := baseOpts + + groups, _, err := th.SystemAdminClient.GetGroups(context.Background(), opts) + require.NoError(t, err) + + foundNoRef := false + foundWithRef := false + for _, g := range groups { + if g.Id == groupNoRef.Id { + foundNoRef = true + } + if g.Id == groupWithRef.Id { + foundWithRef = true + } + } + assert.True(t, foundNoRef, "Admin should see groups with AllowReference=false") + assert.True(t, foundWithRef, "Admin should see groups with AllowReference=true") }) t.Run("include member count", func(t *testing.T) { @@ -1593,7 +1843,48 @@ func TestGetGroups(t *testing.T) { groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) - assert.Len(t, groups, 1) + assert.Len(t, groups, 3) + }) + + t.Run("test FilterAllowReference for non-admin user", func(t *testing.T) { + opts := baseOpts + opts.FilterAllowReference = true + + _, _, err := th.Client.Login(context.Background(), th.BasicUser.Email, th.BasicUser.Password) + require.NoError(t, err) + + groups, _, err := th.Client.GetGroups(context.Background(), opts) + require.NoError(t, err) + + for _, g := range groups { + if g.Id == groupWithRef.Id { + assert.True(t, g.AllowReference) + } + assert.NotEqual(t, g.Id, groupNoRef.Id, "Non-admin user should not see groups with AllowReference=false") + } + + _, _, err = th.SystemAdminClient.Login(context.Background(), th.SystemAdminUser.Email, th.SystemAdminUser.Password) + require.NoError(t, err) + }) + + t.Run("test FilterAllowReference for admin user", func(t *testing.T) { + opts := baseOpts + + groups, _, err := th.SystemAdminClient.GetGroups(context.Background(), opts) + require.NoError(t, err) + + foundNoRef := false + foundWithRef := false + for _, g := range groups { + if g.Id == groupNoRef.Id { + foundNoRef = true + } + if g.Id == groupWithRef.Id { + foundWithRef = true + } + } + assert.True(t, foundNoRef, "Admin should see groups with AllowReference=false") + assert.True(t, foundWithRef, "Admin should see groups with AllowReference=true") }) t.Run("not associated to channel", func(t *testing.T) { @@ -1611,7 +1902,7 @@ func TestGetGroups(t *testing.T) { groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) - assert.ElementsMatch(t, []*model.Group{group, th.Group}, groups) + assert.ElementsMatch(t, []*model.Group{group, th.Group, groupNoRef, groupWithRef}, groups) }) t.Run("not associated to team", func(t *testing.T) { @@ -1626,10 +1917,10 @@ func TestGetGroups(t *testing.T) { require.NoError(t, err) CheckOKStatus(t, resp) - groups, resp, err := th.Client.GetGroups(context.Background(), opts) + groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) - assert.ElementsMatch(t, []*model.Group{group, th.Group}, groups) + assert.ElementsMatch(t, []*model.Group{group, th.Group, groupNoRef, groupWithRef}, groups) }) t.Run("since parameter", func(t *testing.T) { @@ -1639,7 +1930,7 @@ func TestGetGroups(t *testing.T) { require.NoError(t, err) CheckOKStatus(t, resp) assert.Len(t, groups, 1) - assert.Equal(t, groups[0].Id, group.Id) + assert.Equal(t, groups[0].Id, groupWithRef.Id) opts.Since = model.GetMillis() groups, resp, err = th.Client.GetGroups(context.Background(), opts) @@ -1655,14 +1946,14 @@ func TestGetGroups(t *testing.T) { // Test include_archived parameter opts.IncludeArchived = true - groups, resp, err := th.Client.GetGroups(context.Background(), opts) + groups, resp, err := th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) - assert.Len(t, groups, 2) + assert.Len(t, groups, 4) // Test returning only archived groups opts.FilterArchived = true - groups, resp, err = th.Client.GetGroups(context.Background(), opts) + groups, _, err = th.SystemAdminClient.GetGroups(context.Background(), opts) require.NoError(t, err) CheckOKStatus(t, resp) assert.Len(t, groups, 1) @@ -1767,7 +2058,7 @@ func TestGetGroups(t *testing.T) { require.NoError(t, err) CheckOKStatus(t, resp) // Should return all groups regardless of source when not specified - assert.Len(t, groups, 3) // group, and group2 + assert.Len(t, groups, 5) // Test with custom groups disabled and only_syncable_sources=true th.App.UpdateConfig(func(cfg *model.Config) { @@ -1777,7 +2068,7 @@ func TestGetGroups(t *testing.T) { require.NoError(t, err) CheckOKStatus(t, resp) // Should still only return LDAP groups - assert.Len(t, groups, 2) + assert.Len(t, groups, 4) for _, g := range groups { assert.True(t, g.Source == model.GroupSourceLdap || strings.HasPrefix(string(g.Source), string(model.GroupSourcePluginPrefix))) } @@ -1794,7 +2085,7 @@ func TestGetGroups(t *testing.T) { CheckOKStatus(t, resp) // Should only return groups from syncable sources (LDAP and plugin_ groups) - assert.Len(t, groups, 2) + assert.Len(t, groups, 4) for _, g := range groups { assert.True(t, g.Source == model.GroupSourceLdap || strings.HasPrefix(string(g.Source), string(model.GroupSourcePluginPrefix))) } @@ -1807,11 +2098,12 @@ func TestGetGroupsByUserId(t *testing.T) { id := model.NewId() group1, appErr := th.App.CreateGroup(&model.Group{ - DisplayName: "dn-foo_" + id, - Name: model.NewPointer("name" + id), - Source: model.GroupSourceLdap, - Description: "description_" + id, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn-foo_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, }) assert.Nil(t, appErr) @@ -1823,11 +2115,12 @@ func TestGetGroupsByUserId(t *testing.T) { id = model.NewId() group2, appErr := th.App.CreateGroup(&model.Group{ - DisplayName: "dn-foo_" + id, - Name: model.NewPointer("name" + id), - Source: model.GroupSourceLdap, - Description: "description_" + id, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn-foo_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, }) assert.Nil(t, appErr) @@ -1876,11 +2169,12 @@ func TestGetGroupMembers(t *testing.T) { id := model.NewId() group, appErr := th.App.CreateGroup(&model.Group{ - DisplayName: "dn-foo_" + id, - Name: model.NewPointer("name" + id), - Source: model.GroupSourceLdap, - Description: "description_" + id, - RemoteId: model.NewPointer(model.NewId()), + DisplayName: "dn-foo_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: false, }) assert.Nil(t, appErr) @@ -1902,7 +2196,7 @@ func TestGetGroupMembers(t *testing.T) { th.App.Srv().SetLicense(model.NewTestLicense("ldap")) - t.Run("Non admins are not allowed to get members for LDAP groups", func(t *testing.T) { + t.Run("Non admins are not allowed to get members for LDAP groups when allow reference is false", func(t *testing.T) { members, response, err := th.Client.GetGroupMembers(context.Background(), group.Id) assert.Error(t, err) CheckForbiddenStatus(t, response) diff --git a/server/channels/app/group.go b/server/channels/app/group.go index 3a388dff1d..f410640e53 100644 --- a/server/channels/app/group.go +++ b/server/channels/app/group.go @@ -85,8 +85,8 @@ func (a *App) GetGroupsBySource(groupSource model.GroupSource) ([]*model.Group, return groups, nil } -func (a *App) GetGroupsByUserId(userID string) ([]*model.Group, *model.AppError) { - groups, err := a.Srv().Store().Group().GetByUser(userID) +func (a *App) GetGroupsByUserId(userID string, opts model.GroupSearchOpts) ([]*model.Group, *model.AppError) { + groups, err := a.Srv().Store().Group().GetByUser(userID, opts) if err != nil { return nil, model.NewAppError("GetGroupsByUserId", "app.select_error", nil, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/channels/app/plugin_api.go b/server/channels/app/plugin_api.go index 158899e694..7d903bd446 100644 --- a/server/channels/app/plugin_api.go +++ b/server/channels/app/plugin_api.go @@ -661,7 +661,7 @@ func (api *PluginAPI) GetGroupsBySource(groupSource model.GroupSource) ([]*model } func (api *PluginAPI) GetGroupsForUser(userID string) ([]*model.Group, *model.AppError) { - return api.app.GetGroupsByUserId(userID) + return api.app.GetGroupsByUserId(userID, model.GroupSearchOpts{}) } func (api *PluginAPI) UpsertGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) { diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index be4cb0441d..c56b12c3e1 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -5397,11 +5397,11 @@ func (s *RetryLayerGroupStore) GetByRemoteID(remoteID string, groupSource model. } -func (s *RetryLayerGroupStore) GetByUser(userID string) ([]*model.Group, error) { +func (s *RetryLayerGroupStore) GetByUser(userID string, opts model.GroupSearchOpts) ([]*model.Group, error) { tries := 0 for { - result, err := s.GroupStore.GetByUser(userID) + result, err := s.GroupStore.GetByUser(userID, opts) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/group_store.go b/server/channels/store/sqlstore/group_store.go index 02b968cd23..da204373af 100644 --- a/server/channels/store/sqlstore/group_store.go +++ b/server/channels/store/sqlstore/group_store.go @@ -301,7 +301,7 @@ func (s *SqlGroupStore) GetAllBySource(groupSource model.GroupSource) ([]*model. return groups, nil } -func (s *SqlGroupStore) GetByUser(userId string) ([]*model.Group, error) { +func (s *SqlGroupStore) GetByUser(userID string, opts model.GroupSearchOpts) ([]*model.Group, error) { groups := []*model.Group{} builder := s.getQueryBuilder(). @@ -310,11 +310,15 @@ func (s *SqlGroupStore) GetByUser(userId string) ([]*model.Group, error) { Join("UserGroups ON UserGroups.Id = GroupMembers.GroupId"). Where(sq.Eq{ "GroupMembers.DeleteAt": 0, - "UserId": userId, + "UserId": userID, }) + if opts.FilterAllowReference { + builder = builder.Where("UserGroups.AllowReference = true") + } + if err := s.GetReplica().SelectBuilder(&groups, builder); err != nil { - return nil, errors.Wrapf(err, "failed to find Groups with userId=%s", userId) + return nil, errors.Wrapf(err, "failed to find Groups with userId=%s", userID) } return groups, nil diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 7dd9560591..8eb230c4b5 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -882,7 +882,7 @@ type GroupStore interface { GetByIDs(groupIDs []string) ([]*model.Group, error) GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, error) GetAllBySource(groupSource model.GroupSource) ([]*model.Group, error) - GetByUser(userID string) ([]*model.Group, error) + GetByUser(userID string, opts model.GroupSearchOpts) ([]*model.Group, error) Update(group *model.Group) (*model.Group, error) Delete(groupID string) (*model.Group, error) Restore(groupID string) (*model.Group, error) diff --git a/server/channels/store/storetest/group_store.go b/server/channels/store/storetest/group_store.go index 2921b329b2..2e24371733 100644 --- a/server/channels/store/storetest/group_store.go +++ b/server/channels/store/storetest/group_store.go @@ -545,11 +545,12 @@ func testGroupStoreGetByUser(t *testing.T, rctx request.CTX, ss store.Store) { require.NoError(t, err) g2 := &model.Group{ - Name: model.NewPointer(model.NewId()), - DisplayName: model.NewId(), - Description: model.NewId(), - Source: model.GroupSourceLdap, - RemoteId: model.NewPointer(model.NewId()), + Name: model.NewPointer(model.NewId()), + DisplayName: model.NewId(), + Description: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, } g2, err = ss.Group().Create(g2) require.NoError(t, err) @@ -576,7 +577,7 @@ func testGroupStoreGetByUser(t *testing.T, rctx request.CTX, ss store.Store) { _, err = ss.Group().UpsertMember(g2.Id, u2.Id) require.NoError(t, err) - groups, err := ss.Group().GetByUser(u1.Id) + groups, err := ss.Group().GetByUser(u1.Id, model.GroupSearchOpts{}) require.NoError(t, err) assert.Equal(t, 2, len(groups)) found1 := false @@ -592,14 +593,19 @@ func testGroupStoreGetByUser(t *testing.T, rctx request.CTX, ss store.Store) { assert.True(t, found1) assert.True(t, found2) - groups, err = ss.Group().GetByUser(u2.Id) + groups, err = ss.Group().GetByUser(u2.Id, model.GroupSearchOpts{}) require.NoError(t, err) require.Equal(t, 1, len(groups)) assert.Equal(t, g2.Id, groups[0].Id) - groups, err = ss.Group().GetByUser(model.NewId()) + groups, err = ss.Group().GetByUser(model.NewId(), model.GroupSearchOpts{}) require.NoError(t, err) assert.Equal(t, 0, len(groups)) + + groups, err = ss.Group().GetByUser(u1.Id, model.GroupSearchOpts{FilterAllowReference: true}) + require.NoError(t, err) + assert.Equal(t, 1, len(groups)) + assert.Equal(t, g2.Id, groups[0].Id) } func testGroupStoreUpdate(t *testing.T, rctx request.CTX, ss store.Store) { diff --git a/server/channels/store/storetest/mocks/GroupStore.go b/server/channels/store/storetest/mocks/GroupStore.go index d3b90728b4..d7cf25ffe6 100644 --- a/server/channels/store/storetest/mocks/GroupStore.go +++ b/server/channels/store/storetest/mocks/GroupStore.go @@ -692,9 +692,9 @@ func (_m *GroupStore) GetByRemoteID(remoteID string, groupSource model.GroupSour return r0, r1 } -// GetByUser provides a mock function with given fields: userID -func (_m *GroupStore) GetByUser(userID string) ([]*model.Group, error) { - ret := _m.Called(userID) +// GetByUser provides a mock function with given fields: userID, opts +func (_m *GroupStore) GetByUser(userID string, opts model.GroupSearchOpts) ([]*model.Group, error) { + ret := _m.Called(userID, opts) if len(ret) == 0 { panic("no return value specified for GetByUser") @@ -702,19 +702,19 @@ func (_m *GroupStore) GetByUser(userID string) ([]*model.Group, error) { var r0 []*model.Group var r1 error - if rf, ok := ret.Get(0).(func(string) ([]*model.Group, error)); ok { - return rf(userID) + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) ([]*model.Group, error)); ok { + return rf(userID, opts) } - if rf, ok := ret.Get(0).(func(string) []*model.Group); ok { - r0 = rf(userID) + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) []*model.Group); ok { + r0 = rf(userID, opts) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.Group) } } - if rf, ok := ret.Get(1).(func(string) error); ok { - r1 = rf(userID) + if rf, ok := ret.Get(1).(func(string, model.GroupSearchOpts) error); ok { + r1 = rf(userID, opts) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index a8d45609a5..66fe13b2c7 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -4355,10 +4355,10 @@ func (s *TimerLayerGroupStore) GetByRemoteID(remoteID string, groupSource model. return result, err } -func (s *TimerLayerGroupStore) GetByUser(userID string) ([]*model.Group, error) { +func (s *TimerLayerGroupStore) GetByUser(userID string, opts model.GroupSearchOpts) ([]*model.Group, error) { start := time.Now() - result, err := s.GroupStore.GetByUser(userID) + result, err := s.GroupStore.GetByUser(userID, opts) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {