From afa35ac5750c3c84b7b3e35c31115d7f6afdc540 Mon Sep 17 00:00:00 2001 From: Farhan Munshi <3207297+fm2munsh@users.noreply.github.com> Date: Mon, 27 Jan 2020 11:45:14 -0500 Subject: [PATCH] [MM-21946] [MM-21945] Ensure deleted groups are not returned from the groups API (#13747) * MM-21946 Ensure deleted groups are not returned from the groups API * MM-21946 Only append to query at the end * MM-21946 Add DeleteAt check to the top of the GetGroups function and clean up the formatting a bit * MM-21946 Move the From statement into the next block --- store/sqlstore/group_store.go | 17 ++++++----- store/storetest/group_store.go | 52 ++++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 967afb358d..5f80565748 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -905,7 +905,7 @@ func (s *SqlGroupStore) groupsBySyncableBaseQuery(st model.GroupSyncableType, t From("UserGroups ug"). LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = ug.Id"). LeftJoin(fmt.Sprintf("%[1]s ON %[1]s.GroupId = ug.Id", table)). - Where(fmt.Sprintf("%[1]s.DeleteAt = 0 AND %[1]s.%[2]s = ?", table, idCol), syncableID). + Where(fmt.Sprintf("ug.DeleteAt = 0 AND %[1]s.DeleteAt = 0 AND %[1]s.%[2]s = ?", table, idCol), syncableID). OrderBy("ug.DisplayName") } @@ -963,18 +963,21 @@ func (s *SqlGroupStore) GetGroupsByTeam(teamId string, opts model.GroupSearchOpt func (s *SqlGroupStore) GetGroups(page, perPage int, opts model.GroupSearchOpts) ([]*model.Group, *model.AppError) { var groups []*model.Group - groupsQuery := s.getQueryBuilder().Select("g.*").From("UserGroups g").Limit(uint64(perPage)).Offset(uint64(page * perPage)).OrderBy("g.DisplayName") + groupsQuery := s.getQueryBuilder().Select("g.*") if opts.IncludeMemberCount { groupsQuery = s.getQueryBuilder(). Select("g.*, coalesce(Members.MemberCount, 0) AS MemberCount"). - From("UserGroups g"). - LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = g.Id"). - Limit(uint64(perPage)). - Offset(uint64(page * perPage)). - OrderBy("g.DisplayName") + LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = g.Id") } + groupsQuery = groupsQuery. + From("UserGroups g"). + Where("g.DeleteAt = 0"). + Limit(uint64(perPage)). + Offset(uint64(page * perPage)). + OrderBy("g.DisplayName") + if len(opts.Q) > 0 { pattern := fmt.Sprintf("%%%s%%", sanitizeSearchTerm(opts.Q, "\\")) operatorKeyword := "ILIKE" diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index cd8eb2713f..5d9cb49cff 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -2046,7 +2046,7 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { channel1, err := ss.Channel().Save(channel1, 9999) require.Nil(t, err) - // Create Groups 1 and 2 + // Create Groups 1, 2 and a deleted group group1, err := ss.Group().Create(&model.Group{ Name: model.NewId(), DisplayName: "group-1", @@ -2063,8 +2063,17 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { }) require.Nil(t, err) + deletedGroup, err := ss.Group().Create(&model.Group{ + Name: model.NewId(), + DisplayName: "group-deleted", + RemoteId: model.NewId(), + Source: model.GroupSourceLdap, + DeleteAt: 1, + }) + require.Nil(t, err) + // And associate them with Channel1 - for _, g := range []*model.Group{group1, group2} { + for _, g := range []*model.Group{group1, group2, deletedGroup} { _, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{ AutoAdd: true, SyncableId: channel1.Id, @@ -2263,7 +2272,7 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { team1, err := ss.Team().Save(team1) require.Nil(t, err) - // Create Groups 1 and 2 + // Create Groups 1, 2 and a deleted group group1, err := ss.Group().Create(&model.Group{ Name: model.NewId(), DisplayName: "group-1", @@ -2280,8 +2289,17 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { }) require.Nil(t, err) + deletedGroup, err := ss.Group().Create(&model.Group{ + Name: model.NewId(), + DisplayName: "group-deleted", + RemoteId: model.NewId(), + Source: model.GroupSourceLdap, + DeleteAt: 1, + }) + require.Nil(t, err) + // And associate them with Team1 - for _, g := range []*model.Group{group1, group2} { + for _, g := range []*model.Group{group1, group2, deletedGroup} { _, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{ AutoAdd: true, SyncableId: team1.Id, @@ -2348,6 +2366,9 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { _, err = ss.User().Update(user2, true) require.Nil(t, err) + _, err = ss.Group().UpsertMember(deletedGroup.Id, user1.Id) + require.Nil(t, err) + group1WithMemberCount := *group1 group1WithMemberCount.MemberCount = model.NewInt(1) @@ -2512,8 +2533,17 @@ func testGetGroups(t *testing.T, ss store.Store) { }) require.Nil(t, err) + deletedGroup, err := ss.Group().Create(&model.Group{ + Name: model.NewId() + "-group-deleted", + DisplayName: "group-deleted", + RemoteId: model.NewId(), + Source: model.GroupSourceLdap, + DeleteAt: 1, + }) + require.Nil(t, err) + // And associate them with Team1 - for _, g := range []*model.Group{group1, group2} { + for _, g := range []*model.Group{group1, group2, deletedGroup} { _, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{ AutoAdd: true, SyncableId: team1.Id, @@ -2606,6 +2636,9 @@ func testGetGroups(t *testing.T, ss store.Store) { _, err = ss.Group().UpsertMember(group1.Id, user2.Id) require.Nil(t, err) + _, err = ss.Group().UpsertMember(deletedGroup.Id, user1.Id) + require.Nil(t, err) + user2.DeleteAt = 1 ss.User().Update(user2, true) @@ -2701,6 +2734,9 @@ func testGetGroups(t *testing.T, ss store.Store) { if g.Id == group1.Id && *g.MemberCount != 1 { return false } + if g.DeleteAt != 0 { + return false + } } return true }, @@ -2718,6 +2754,9 @@ func testGetGroups(t *testing.T, ss store.Store) { if g.Id == group3.Id { return false } + if g.DeleteAt != 0 { + return false + } } return true }, @@ -2735,6 +2774,9 @@ func testGetGroups(t *testing.T, ss store.Store) { if g.Id == group1.Id || g.Id == group2.Id { return false } + if g.DeleteAt != 0 { + return false + } } return true },