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.
2020-05-29 10:46:52 -04:00
--- 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 != "" {
--- 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])
+}
--- 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)
--- 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 {
--- 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()
--- 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}}'."
--- 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&not_associated_to_team=%v&not_associated_to_channel=%v&filter_allow_reference=%v&q=%v",
+		"%s?include_member_count=%v&not_associated_to_team=%v&not_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)
--- 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 {
--- 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)
--- 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 {
--- 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
 	}