From c529d5190a485a84ae21fccabc22e2d42bd2f6c6 Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Fri, 29 May 2020 10:46:52 -0400 Subject: [PATCH] MM-25040: Restrict associated groups to channels when team is group-constrained. (#14619) * MM-25040: Only return team-associated groups if the team is group-constrained. MM-25040: Prevents associating a group to a channel if the team doesn't have the group first. * MM-25040: Fix lints. * MM-25040: Still add the groupteam if the team is not group-constrained. * MM-25040: Wraps groupteam upsert in else branch for efficiency. * MM-25040: Removes unnecessary page iteration. * MM-25040: Fix typo. * MM-25040: Moves filtering to SQL. * MM-25040: Updates tests, check pagination. * MM-25040: Fix lint error. * MM-25040: Adds some more group store tests. * MM-25040: Fix for wrong test parameter. --- api4/group.go | 7 ++- api4/group_test.go | 83 ++++++++++++++++++++++++ app/app_iface.go | 3 +- app/group.go | 57 ++++++++++++----- app/group_test.go | 27 ++++++++ i18n/en.json | 4 ++ model/client4.go | 3 +- model/group.go | 6 ++ store/sqlstore/group_store.go | 21 +++++++ store/storetest/group_store.go | 54 +++++++++++++--- web/params.go | 111 +++++++++++++++++---------------- 11 files changed, 293 insertions(+), 83 deletions(-) diff --git a/api4/group.go b/api4/group.go index 568f851e2b..3ba8c2c4ad 100644 --- a/api4/group.go +++ b/api4/group.go @@ -717,9 +717,10 @@ func getGroups(c *Context, w http.ResponseWriter, r *http.Request) { } opts := model.GroupSearchOpts{ - Q: c.Params.Q, - IncludeMemberCount: c.Params.IncludeMemberCount, - FilterAllowReference: c.Params.FilterAllowReference, + Q: c.Params.Q, + IncludeMemberCount: c.Params.IncludeMemberCount, + FilterAllowReference: c.Params.FilterAllowReference, + FilterParentTeamPermitted: c.Params.FilterParentTeamPermitted, } if teamID != "" { diff --git a/api4/group_test.go b/api4/group_test.go index 60cc79a5a8..e87312ff46 100644 --- a/api4/group_test.go +++ b/api4/group_test.go @@ -1007,3 +1007,86 @@ func TestGetGroupsByUserId(t *testing.T) { assert.ElementsMatch(t, []*model.Group{group1, group2}, groups) } + +func TestGetGroupsGroupConstrainedParentTeam(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.SetLicense(model.NewTestLicense("ldap")) + + var groups []*model.Group + for i := 0; i < 4; i++ { + id := model.NewId() + group, err := th.App.CreateGroup(&model.Group{ + DisplayName: fmt.Sprintf("dn-foo_%d", i), + Name: model.NewString("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewId(), + }) + require.Nil(t, err) + groups = append(groups, group) + } + + team := th.CreateTeam() + + id := model.NewId() + channel := &model.Channel{ + DisplayName: "dn_" + id, + Name: "name" + id, + Type: model.CHANNEL_PRIVATE, + TeamId: team.Id, + GroupConstrained: model.NewBool(true), + } + channel, err := th.App.CreateChannel(channel, false) + require.Nil(t, err) + + // normal result of groups are returned if the team is not group-constrained + apiGroups, response := th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id}) + require.Nil(t, response.Error) + require.Contains(t, apiGroups, groups[0]) + require.Contains(t, apiGroups, groups[1]) + require.Contains(t, apiGroups, groups[2]) + + team.GroupConstrained = model.NewBool(true) + team, err = th.App.UpdateTeam(team) + require.Nil(t, err) + + // team is group-constrained but has no associated groups + apiGroups, response = th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id, FilterParentTeamPermitted: true}) + require.Nil(t, response.Error) + require.Len(t, apiGroups, 0) + + for _, group := range []*model.Group{groups[0], groups[2], groups[3]} { + _, err = th.App.UpsertGroupSyncable(model.NewGroupTeam(group.Id, team.Id, false)) + require.Nil(t, err) + } + + // set of the teams groups are returned + apiGroups, response = th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id, FilterParentTeamPermitted: true}) + require.Nil(t, response.Error) + require.Contains(t, apiGroups, groups[0]) + require.NotContains(t, apiGroups, groups[1]) + require.Contains(t, apiGroups, groups[2]) + + // paged results function as expected + apiGroups, response = th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id, FilterParentTeamPermitted: true, PageOpts: &model.PageOpts{PerPage: 2, Page: 0}}) + require.Nil(t, response.Error) + require.Len(t, apiGroups, 2) + require.Equal(t, apiGroups[0].Id, groups[0].Id) + require.Equal(t, apiGroups[1].Id, groups[2].Id) + + apiGroups, response = th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id, FilterParentTeamPermitted: true, PageOpts: &model.PageOpts{PerPage: 2, Page: 1}}) + require.Nil(t, response.Error) + require.Len(t, apiGroups, 1) + require.Equal(t, apiGroups[0].Id, groups[3].Id) + + _, err = th.App.UpsertGroupSyncable(model.NewGroupChannel(groups[0].Id, channel.Id, false)) + require.Nil(t, err) + + // as usual it doesn't return groups already associated to the channel + apiGroups, response = th.SystemAdminClient.GetGroups(model.GroupSearchOpts{NotAssociatedToChannel: channel.Id}) + require.Nil(t, response.Error) + require.NotContains(t, apiGroups, groups[0]) + require.Contains(t, apiGroups, groups[2]) +} diff --git a/app/app_iface.go b/app/app_iface.go index 21d8c0d5c6..5a6e392603 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -158,6 +158,8 @@ type AppIface interface { GetEmojiStaticUrl(emojiName string) (string, *model.AppError) // GetEnvironmentConfig returns a map of configuration keys whose values have been overridden by an environment variable. GetEnvironmentConfig() map[string]interface{} + // GetGroupsByTeam returns the paged list and the total count of group associated to the given team. + GetGroupsByTeam(teamId string, opts model.GroupSearchOpts) ([]*model.GroupWithSchemeAdmin, int, *model.AppError) // GetHubForUserId returns the hub for a given user id. GetHubForUserId(userId string) *Hub // GetKnownUsers returns the list of user ids of users with any direct @@ -555,7 +557,6 @@ type AppIface interface { GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) ([]*model.GroupWithSchemeAdmin, int, *model.AppError) GetGroupsByIDs(groupIDs []string) ([]*model.Group, *model.AppError) GetGroupsBySource(groupSource model.GroupSource) ([]*model.Group, *model.AppError) - GetGroupsByTeam(teamId string, opts model.GroupSearchOpts) ([]*model.GroupWithSchemeAdmin, int, *model.AppError) GetGroupsByUserId(userId string) ([]*model.Group, *model.AppError) GetIncomingWebhook(hookId string) (*model.IncomingWebhook, *model.AppError) GetIncomingWebhooksForTeamPage(teamId string, page, perPage int) ([]*model.IncomingWebhook, *model.AppError) diff --git a/app/group.go b/app/group.go index 5788e66e06..29bbe0d4ee 100644 --- a/app/group.go +++ b/app/group.go @@ -4,6 +4,8 @@ package app import ( + "net/http" + "github.com/mattermost/mattermost-server/v5/model" ) @@ -86,6 +88,43 @@ func (a *App) UpsertGroupSyncable(groupSyncable *model.GroupSyncable) (*model.Gr return nil, err } + // reject the syncable creation if the group isn't already associated to the parent team + if groupSyncable.Type == model.GroupSyncableTypeChannel { + var channel *model.Channel + channel, err = a.Srv().Store.Channel().Get(groupSyncable.SyncableId, true) + if err != nil { + return nil, err + } + + var team *model.Team + team, err = a.Srv().Store.Team().Get(channel.TeamId) + if err != nil { + return nil, err + } + if team.IsGroupConstrained() { + var teamGroups []*model.GroupWithSchemeAdmin + teamGroups, err = a.Srv().Store.Group().GetGroupsByTeam(channel.TeamId, model.GroupSearchOpts{}) + if err != nil { + return nil, err + } + var permittedGroup bool + for _, teamGroup := range teamGroups { + if teamGroup.Group.Id == groupSyncable.GroupId { + permittedGroup = true + break + } + } + if !permittedGroup { + return nil, model.NewAppError("App.UpsertGroupSyncable", "group_not_associated_to_synced_team", nil, "", http.StatusBadRequest) + } + } else { + _, err = a.UpsertGroupSyncable(model.NewGroupTeam(groupSyncable.GroupId, team.Id, groupSyncable.AutoAdd)) + if err != nil { + return nil, err + } + } + } + if gs == nil { gs, err = a.Srv().Store.Group().CreateGroupSyncable(groupSyncable) if err != nil { @@ -98,23 +137,6 @@ func (a *App) UpsertGroupSyncable(groupSyncable *model.GroupSyncable) (*model.Gr } } - // if the type is channel, then upsert the associated GroupTeam [MM-14675] - if gs.Type == model.GroupSyncableTypeChannel { - channel, err := a.Srv().Store.Channel().Get(gs.SyncableId, true) - if err != nil { - return nil, err - } - _, err = a.UpsertGroupSyncable(&model.GroupSyncable{ - GroupId: gs.GroupId, - SyncableId: channel.TeamId, - Type: model.GroupSyncableTypeTeam, - AutoAdd: gs.AutoAdd, - }) - if err != nil { - return nil, err - } - } - var messageWs *model.WebSocketEvent if gs.Type == model.GroupSyncableTypeTeam { messageWs = model.NewWebSocketEvent(model.WEBSOCKET_EVENT_RECEIVED_GROUP_ASSOCIATED_TO_TEAM, gs.SyncableId, "", "", nil) @@ -217,6 +239,7 @@ func (a *App) GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) ( return groups, int(count), nil } +// GetGroupsByTeam returns the paged list and the total count of group associated to the given team. func (a *App) GetGroupsByTeam(teamId string, opts model.GroupSearchOpts) ([]*model.GroupWithSchemeAdmin, int, *model.AppError) { groups, err := a.Srv().Store.Group().GetGroupsByTeam(teamId, opts) if err != nil { diff --git a/app/group_test.go b/app/group_test.go index 27da49d82a..150ed201c0 100644 --- a/app/group_test.go +++ b/app/group_test.go @@ -158,6 +158,33 @@ func TestUpsertGroupSyncable(t *testing.T) { require.Equal(t, int64(0), gs.DeleteAt) } +func TestUpsertGroupSyncableTeamGroupConstrained(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + group1 := th.CreateGroup() + group2 := th.CreateGroup() + + team := th.CreateTeam() + team.GroupConstrained = model.NewBool(true) + team, err := th.App.UpdateTeam(team) + require.Nil(t, err) + _, err = th.App.UpsertGroupSyncable(model.NewGroupTeam(group1.Id, team.Id, false)) + + channel := th.CreateChannel(team) + + _, err = th.App.UpsertGroupSyncable(model.NewGroupChannel(group2.Id, channel.Id, false)) + require.NotNil(t, err) + require.Equal(t, err.Id, "group_not_associated_to_synced_team") + + gs, err := th.App.GetGroupSyncable(group2.Id, channel.Id, model.GroupSyncableTypeChannel) + require.Nil(t, gs) + require.NotNil(t, err) + + _, err = th.App.UpsertGroupSyncable(model.NewGroupChannel(group1.Id, channel.Id, false)) + require.Nil(t, err) +} + func TestGetGroupSyncable(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/i18n/en.json b/i18n/en.json index 732246b867..4f24b9bb17 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4486,6 +4486,10 @@ "id": "ent.saml.service_disable.app_error", "translation": "SAML 2.0 is not configured or supported on this server." }, + { + "id": "group_not_associated_to_synced_team", + "translation": "Group cannot be associated to the channel until it is first associated to the parent group-synced team." + }, { "id": "groups.unsupported_syncable_type", "translation": "Unsupported syncable type '{{.Value}}'." diff --git a/model/client4.go b/model/client4.go index e41a9b8b4e..3eacb46176 100644 --- a/model/client4.go +++ b/model/client4.go @@ -3811,13 +3811,14 @@ func (c *Client4) GetGroupsAssociatedToChannelsByTeam(teamId string, opts GroupS // GetGroups retrieves Mattermost Groups func (c *Client4) GetGroups(opts GroupSearchOpts) ([]*Group, *Response) { path := fmt.Sprintf( - "%s?include_member_count=%v¬_associated_to_team=%v¬_associated_to_channel=%v&filter_allow_reference=%v&q=%v", + "%s?include_member_count=%v¬_associated_to_team=%v¬_associated_to_channel=%v&filter_allow_reference=%v&q=%v&filter_parent_team_permitted=%v", c.GetGroupsRoute(), opts.IncludeMemberCount, opts.NotAssociatedToTeam, opts.NotAssociatedToChannel, opts.FilterAllowReference, opts.Q, + opts.FilterParentTeamPermitted, ) if opts.Since > 0 { path = fmt.Sprintf("%s&since=%v", path, opts.Since) diff --git a/model/group.go b/model/group.go index 361121374d..4896683d59 100644 --- a/model/group.go +++ b/model/group.go @@ -81,6 +81,12 @@ type GroupSearchOpts struct { FilterAllowReference bool PageOpts *PageOpts Since int64 + + // FilterParentTeamPermitted filters the groups to the intersect of the + // set associated to the parent team and those returned by the query. + // If the parent team is not group-constrained or if NotAssociatedToChannel + // is not set then this option is ignored. + FilterParentTeamPermitted bool } type PageOpts struct { diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 89f30e2a08..331d3c2e99 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -1216,6 +1216,27 @@ func (s *SqlGroupStore) GetGroups(page, perPage int, opts model.GroupSearchOpts) `, opts.NotAssociatedToChannel) } + if opts.FilterParentTeamPermitted && len(opts.NotAssociatedToChannel) == 26 { + groupsQuery = groupsQuery.Where(` + g.Id IN ( + SELECT + GroupId + FROM + GroupTeams + WHERE + GroupTeams.DeleteAt = 0 + AND GroupTeams.TeamId = ( + SELECT + TeamId + FROM + Channels + WHERE + Id = ? + ) + ) + `, opts.NotAssociatedToChannel) + } + queryString, args, err := groupsQuery.ToSql() if err != nil { return nil, model.NewAppError("SqlGroupStore.GetGroups", "store.sql_group.app_error", nil, err.Error(), http.StatusInternalServerError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 6d7792d89d..dadfb8c596 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -2994,14 +2994,15 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { func testGetGroups(t *testing.T, ss store.Store) { // Create Team1 team1 := &model.Team{ - DisplayName: "Team1", - Description: model.NewId(), - CompanyName: model.NewId(), - AllowOpenInvite: false, - InviteId: model.NewId(), - Name: "zz" + model.NewId(), - Email: "success+" + model.NewId() + "@simulator.amazonses.com", - Type: model.TEAM_OPEN, + DisplayName: "Team1", + Description: model.NewId(), + CompanyName: model.NewId(), + AllowOpenInvite: false, + InviteId: model.NewId(), + Name: "zz" + model.NewId(), + Email: "success+" + model.NewId() + "@simulator.amazonses.com", + Type: model.TEAM_OPEN, + GroupConstrained: model.NewBool(true), } team1, err := ss.Team().Save(team1) require.Nil(t, err) @@ -3082,6 +3083,16 @@ func testGetGroups(t *testing.T, ss store.Store) { channel2, nErr = ss.Channel().Save(channel2, 9999) require.Nil(t, nErr) + // Create Channel3 + channel3 := &model.Channel{ + TeamId: team1.Id, + DisplayName: "Channel3", + Name: model.NewId(), + Type: model.CHANNEL_PRIVATE, + } + channel3, nErr = ss.Channel().Save(channel3, 9999) + require.Nil(t, nErr) + // Create Group3 group3, err := ss.Group().Create(&model.Group{ Name: model.NewString(model.NewId() + "-group-3"), @@ -3335,6 +3346,33 @@ func testGetGroups(t *testing.T, ss store.Store) { return len(groups) == 0 }, }, + { + Name: "Filter groups from group-constrained teams", + Opts: model.GroupSearchOpts{NotAssociatedToChannel: channel3.Id, FilterParentTeamPermitted: true}, + Page: 0, + PerPage: 100, + Resultf: func(groups []*model.Group) bool { + return len(groups) == 2 && groups[0].Id == group1.Id && groups[1].Id == group2.Id + }, + }, + { + Name: "Filter groups from group-constrained page 0", + Opts: model.GroupSearchOpts{NotAssociatedToChannel: channel3.Id, FilterParentTeamPermitted: true}, + Page: 0, + PerPage: 1, + Resultf: func(groups []*model.Group) bool { + return groups[0].Id == group1.Id + }, + }, + { + Name: "Filter groups from group-constrained page 1", + Opts: model.GroupSearchOpts{NotAssociatedToChannel: channel3.Id, FilterParentTeamPermitted: true}, + Page: 1, + PerPage: 1, + Resultf: func(groups []*model.Group) bool { + return groups[0].Id == group2.Id + }, + }, } for _, tc := range testCases { diff --git a/web/params.go b/web/params.go index 9f62382073..af3981a94d 100644 --- a/web/params.go +++ b/web/params.go @@ -23,59 +23,60 @@ const ( ) type Params struct { - UserId string - TeamId string - InviteId string - TokenId string - ChannelId string - PostId string - FileId string - Filename string - PluginId string - CommandId string - HookId string - ReportId string - EmojiId string - AppId string - Email string - Username string - TeamName string - ChannelName string - PreferenceName string - EmojiName string - Category string - Service string - JobId string - JobType string - ActionId string - RoleId string - RoleName string - SchemeId string - Scope string - GroupId string - Page int - PerPage int - LogsPerPage int - Permanent bool - RemoteId string - SyncableId string - SyncableType model.GroupSyncableType - BotUserId string - Q string - IsLinked *bool - IsConfigured *bool - NotAssociatedToTeam string - NotAssociatedToChannel string - Paginate *bool - IncludeMemberCount bool - NotAssociatedToGroup string - ExcludeDefaultChannels bool - LimitAfter int - LimitBefore int - GroupIDs string - IncludeTotalCount bool - IncludeDeleted bool - FilterAllowReference bool + UserId string + TeamId string + InviteId string + TokenId string + ChannelId string + PostId string + FileId string + Filename string + PluginId string + CommandId string + HookId string + ReportId string + EmojiId string + AppId string + Email string + Username string + TeamName string + ChannelName string + PreferenceName string + EmojiName string + Category string + Service string + JobId string + JobType string + ActionId string + RoleId string + RoleName string + SchemeId string + Scope string + GroupId string + Page int + PerPage int + LogsPerPage int + Permanent bool + RemoteId string + SyncableId string + SyncableType model.GroupSyncableType + BotUserId string + Q string + IsLinked *bool + IsConfigured *bool + NotAssociatedToTeam string + NotAssociatedToChannel string + Paginate *bool + IncludeMemberCount bool + NotAssociatedToGroup string + ExcludeDefaultChannels bool + LimitAfter int + LimitBefore int + GroupIDs string + IncludeTotalCount bool + IncludeDeleted bool + FilterAllowReference bool + FilterParentTeamPermitted bool } func ParamsFromRequest(r *http.Request) *Params { @@ -282,6 +283,10 @@ func ParamsFromRequest(r *http.Request) *Params { params.FilterAllowReference = val } + if val, err := strconv.ParseBool(query.Get("filter_parent_team_permitted")); err == nil { + params.FilterParentTeamPermitted = val + } + if val, err := strconv.ParseBool(query.Get("paginate")); err == nil { params.Paginate = &val }