MM-14753: Verifies that user can join teams and channels in spite of group constraints. (#10529)

* MM-147753: Verifies that users are allowed to be members of a team or a channel, based on group constraints, prior to allowing the API to add them.

* MM-14753: Allow methods to return meaningful results for deleted teams or channels.

* MM-14753: Renames methods to differentiate from permissions and other team and channel restrictions.

* MM-14753: Only check if users are team/channel members if team/channel is group constrained.

* MM-14753: Updates test function names.

* MM-14753: Changes a few method signatures.

* MM-14753: Small refactor and adds missing returns.

* MM-14753: Changes method names from Get* to Filter* name prefixes.

* MM-14753: Renames error variables.

* MM-14753: Updates method names for consistency with join table names.

* MM-14753: Adds case for non AppError return.

* Update i18n/en.json

api4/apitestlib.go

@@ -44,6 +44,7 @@ type TestHelper struct {
 	BasicDeletedChannel *model.Channel
 	BasicChannel2       *model.Channel
 	BasicPost           *model.Post
+	Group               *model.Group
 
 	SystemAdminClient *model.Client4
 	SystemAdminUser   *model.User
@@ -210,6 +211,7 @@ func (me *TestHelper) InitBasic() *TestHelper {
 	me.App.UpdateUserRoles(me.BasicUser.Id, model.SYSTEM_USER_ROLE_ID, false)
 	me.Client.DeleteChannel(me.BasicDeletedChannel.Id)
 	me.LoginBasic()
+	me.Group = me.CreateGroup()
 
 	return me
 }
@@ -509,6 +511,24 @@ func (me *TestHelper) GenerateTestEmail() string {
 	return strings.ToLower(model.NewId() + "@dockerhost")
 }
 
+func (me *TestHelper) CreateGroup() *model.Group {
+	id := model.NewId()
+	group := &model.Group{
+		Name:        "n-" + id,
+		DisplayName: "dn_" + id,
+		Source:      model.GroupSourceLdap,
+		RemoteId:    "ri_" + id,
+	}
+
+	utils.DisableDebugLogForTest()
+	group, err := me.App.CreateGroup(group)
+	if err != nil {
+		panic(err)
+	}
+	utils.EnableDebugLogForTest()
+	return group
+}
+
 func GenerateTestUsername() string {
 	return "fakeuser" + model.NewRandomString(10)
 }

api4/channel.go

@@ -1174,6 +1174,22 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	if channel.GroupConstrained != nil && *channel.GroupConstrained {
+		nonMembers, err := c.App.FilterNonGroupChannelMembers([]string{member.UserId}, channel)
+		if err != nil {
+			if v, ok := err.(*model.AppError); ok {
+				c.Err = v
+			} else {
+				c.Err = model.NewAppError("addChannelMember", "api.channel.add_members.error", nil, err.Error(), http.StatusBadRequest)
+			}
+			return
+		}
+		if len(nonMembers) > 0 {
+			c.Err = model.NewAppError("addChannelMember", "api.channel.add_members.user_denied", map[string]interface{}{"UserIDs": nonMembers}, "", http.StatusBadRequest)
+			return
+		}
+	}
+
 	cm, err := c.App.AddChannelMember(member.UserId, channel, c.App.Session.UserId, postRootId, c.App.Session.Id)
 	if err != nil {
 		c.Err = err

api4/channel_test.go

@@ -2021,6 +2021,30 @@ func TestAddChannelMember(t *testing.T) {
 	_, resp = Client.AddChannelMember(privateChannel.Id, user3.Id)
 	CheckNoError(t, resp)
 	Client.Logout()
+
+	// Set a channel to group-constrained
+	privateChannel.GroupConstrained = model.NewBool(true)
+	_, appErr := th.App.UpdateChannel(privateChannel)
+	require.Nil(t, appErr)
+
+	// User is not in associated groups so shouldn't be allowed
+	_, resp = th.SystemAdminClient.AddChannelMember(privateChannel.Id, user.Id)
+	CheckErrorMessage(t, resp, "api.channel.add_members.user_denied")
+
+	// Associate group to team
+	_, appErr = th.App.CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    th.Group.Id,
+		SyncableId: privateChannel.Id,
+		Type:       model.GroupSyncableTypeChannel,
+	})
+	require.Nil(t, appErr)
+
+	// Add user to group
+	_, appErr = th.App.CreateOrRestoreGroupMember(th.Group.Id, user.Id)
+	require.Nil(t, appErr)
+
+	_, resp = th.SystemAdminClient.AddChannelMember(privateChannel.Id, user.Id)
+	CheckNoError(t, resp)
 }
 
 func TestAddChannelMemberAddMyself(t *testing.T) {

api4/team.go

@@ -388,6 +388,28 @@ func addTeamMember(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	team, err := c.App.GetTeam(member.TeamId)
+	if err != nil {
+		c.Err = err
+		return
+	}
+
+	if team.GroupConstrained != nil && *team.GroupConstrained {
+		nonMembers, err := c.App.FilterNonGroupTeamMembers([]string{member.UserId}, team)
+		if err != nil {
+			if v, ok := err.(*model.AppError); ok {
+				c.Err = v
+			} else {
+				c.Err = model.NewAppError("addTeamMember", "api.team.add_members.error", nil, err.Error(), http.StatusBadRequest)
+			}
+			return
+		}
+		if len(nonMembers) > 0 {
+			c.Err = model.NewAppError("addTeamMember", "api.team.add_members.user_denied", map[string]interface{}{"UserIDs": nonMembers}, "", http.StatusBadRequest)
+			return
+		}
+	}
+
 	member, err = c.App.AddTeamMember(member.TeamId, member.UserId)
 
 	if err != nil {
@@ -432,11 +454,43 @@ func addTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 	var err *model.AppError
 	members := model.TeamMembersFromJson(r.Body)
 
-	if len(members) > MAX_ADD_MEMBERS_BATCH || len(members) == 0 {
+	if len(members) > MAX_ADD_MEMBERS_BATCH {
 		c.SetInvalidParam("too many members in batch")
 		return
 	}
 
+	if len(members) == 0 {
+		c.SetInvalidParam("no members in batch")
+		return
+	}
+
+	var memberIDs []string
+	for _, member := range members {
+		memberIDs = append(memberIDs, member.UserId)
+	}
+
+	team, err := c.App.GetTeam(c.Params.TeamId)
+	if err != nil {
+		c.Err = err
+		return
+	}
+
+	if team.GroupConstrained != nil && *team.GroupConstrained {
+		nonMembers, err := c.App.FilterNonGroupTeamMembers(memberIDs, team)
+		if err != nil {
+			if v, ok := err.(*model.AppError); ok {
+				c.Err = v
+			} else {
+				c.Err = model.NewAppError("addTeamMembers", "api.team.add_members.error", nil, err.Error(), http.StatusBadRequest)
+			}
+			return
+		}
+		if len(nonMembers) > 0 {
+			c.Err = model.NewAppError("addTeamMembers", "api.team.add_members.user_denied", map[string]interface{}{"UserIDs": nonMembers}, "", http.StatusBadRequest)
+			return
+		}
+	}
+
 	var userIds []string
 	for _, member := range members {
 		if member.TeamId != c.Params.TeamId {

api4/team_test.go

@@ -1442,6 +1442,30 @@ func TestAddTeamMember(t *testing.T) {
 	if tm != nil {
 		t.Fatal("should have not returned team member")
 	}
+
+	// Set a team to group-constrained
+	team.GroupConstrained = model.NewBool(true)
+	_, err := th.App.UpdateTeam(team)
+	require.Nil(t, err)
+
+	// User is not in associated groups so shouldn't be allowed
+	_, resp = th.SystemAdminClient.AddTeamMember(team.Id, otherUser.Id)
+	CheckErrorMessage(t, resp, "api.team.add_members.user_denied")
+
+	// Associate group to team
+	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    th.Group.Id,
+		SyncableId: team.Id,
+		Type:       model.GroupSyncableTypeTeam,
+	})
+	require.Nil(t, err)
+
+	// Add user to group
+	_, err = th.App.CreateOrRestoreGroupMember(th.Group.Id, otherUser.Id)
+	require.Nil(t, err)
+
+	_, resp = th.SystemAdminClient.AddTeamMember(team.Id, otherUser.Id)
+	CheckNoError(t, resp)
 }
 
 func TestAddTeamMemberMyself(t *testing.T) {
@@ -1578,7 +1602,7 @@ func TestAddTeamMembers(t *testing.T) {
 	CheckBadRequestStatus(t, resp)
 
 	_, resp = Client.AddTeamMembers(GenerateTestId(), userList)
-	CheckForbiddenStatus(t, resp)
+	CheckNotFoundStatus(t, resp)
 
 	testUserList := append(userList, GenerateTestId())
 	_, resp = Client.AddTeamMembers(team.Id, testUserList)
@@ -1633,6 +1657,30 @@ func TestAddTeamMembers(t *testing.T) {
 	// Should work as a regular user.
 	_, resp = Client.AddTeamMembers(team.Id, userList)
 	CheckNoError(t, resp)
+
+	// Set a team to group-constrained
+	team.GroupConstrained = model.NewBool(true)
+	_, err := th.App.UpdateTeam(team)
+	require.Nil(t, err)
+
+	// User is not in associated groups so shouldn't be allowed
+	_, resp = Client.AddTeamMembers(team.Id, userList)
+	CheckErrorMessage(t, resp, "api.team.add_members.user_denied")
+
+	// Associate group to team
+	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    th.Group.Id,
+		SyncableId: team.Id,
+		Type:       model.GroupSyncableTypeTeam,
+	})
+	require.Nil(t, err)
+
+	// Add user to group
+	_, err = th.App.CreateOrRestoreGroupMember(th.Group.Id, userList[0])
+	require.Nil(t, err)
+
+	_, resp = Client.AddTeamMembers(team.Id, userList)
+	CheckNoError(t, resp)
 }
 
 func TestRemoveTeamMember(t *testing.T) {

app/user.go

@@ -596,6 +596,24 @@ func (a *App) GetUsersWithoutTeam(offset int, limit int) ([]*model.User, *model.
 	return result.Data.([]*model.User), nil
 }
 
+// GetTeamGroupUsers returns the users who are associated to the team via GroupTeams and GroupMembers.
+func (a *App) GetTeamGroupUsers(teamID string) ([]*model.User, *model.AppError) {
+	result := <-a.Srv.Store.User().GetTeamGroupUsers(teamID)
+	if result.Err != nil {
+		return nil, result.Err
+	}
+	return result.Data.([]*model.User), nil
+}
+
+// GetChannelGroupUsers returns the users who are associated to the channel via GroupChannels and GroupMembers.
+func (a *App) GetChannelGroupUsers(channelID string) ([]*model.User, *model.AppError) {
+	result := <-a.Srv.Store.User().GetChannelGroupUsers(channelID)
+	if result.Err != nil {
+		return nil, result.Err
+	}
+	return result.Data.([]*model.User), nil
+}
+
 func (a *App) GetUsersByIds(userIds []string, asAdmin bool) ([]*model.User, *model.AppError) {
 	result := <-a.Srv.Store.User().GetProfileByIds(userIds, true)
 	if result.Err != nil {
@@ -1853,3 +1871,69 @@ func (a *App) UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provide
 
 	return nil
 }
+
+// FilterNonGroupTeamMembers returns the subset of the given user IDs of the users who are not members of groups
+// associated to the team.
+func (a *App) FilterNonGroupTeamMembers(userIDs []string, team *model.Team) ([]string, error) {
+	teamGroupUsers, err := a.GetTeamGroupUsers(team.Id)
+	if err != nil {
+		return nil, err
+	}
+
+	// possible if no groups associated or no group members in any of the associated groups
+	if len(teamGroupUsers) == 0 {
+		return userIDs, nil
+	}
+
+	nonMemberIDs := []string{}
+
+	for _, userID := range userIDs {
+		userIsMember := false
+
+		for _, pu := range teamGroupUsers {
+			if pu.Id == userID {
+				userIsMember = true
+				break
+			}
+		}
+
+		if !userIsMember {
+			nonMemberIDs = append(nonMemberIDs, userID)
+		}
+	}
+
+	return nonMemberIDs, nil
+}
+
+// FilterNonGroupChannelMembers returns the subset of the given user IDs of the users who are not members of groups
+// associated to the channel.
+func (a *App) FilterNonGroupChannelMembers(userIDs []string, channel *model.Channel) ([]string, error) {
+	channelGroupUsers, err := a.GetChannelGroupUsers(channel.Id)
+	if err != nil {
+		return nil, err
+	}
+
+	// possible if no groups associated or no group members in any of the associated groups
+	if len(channelGroupUsers) == 0 {
+		return userIDs, nil
+	}
+
+	nonMemberIDs := []string{}
+
+	for _, userID := range userIDs {
+		userIsMember := false
+
+		for _, pu := range channelGroupUsers {
+			if pu.Id == userID {
+				userIsMember = true
+				break
+			}
+		}
+
+		if !userIsMember {
+			nonMemberIDs = append(nonMemberIDs, userID)
+		}
+	}
+
+	return nonMemberIDs, nil
+}

i18n/en.json

@@ -1906,6 +1906,22 @@
     "id": "api.team.update_team_scheme.scheme_scope.error",
     "translation": "Unable to set the scheme to the team because the supplied scheme is not a team scheme."
   },
+  {
+    "id": "api.team.add_members.user_denied",
+    "translation": "Team membership denied to the following users because of group constraints: {{ .UserIDs }}"
+  },
+  {
+    "id": "api.channel.add_members.user_denied",
+    "translation": "Channel membership denied to the following users because of group constraints: {{ .UserIDs }}"
+  },
+  {
+    "id": "api.team.add_members.error",
+    "translation": "Error adding team member(s)."
+  },
+  {
+    "id": "api.channel.add_members.error",
+    "translation": "Error adding channel member(s)."
+  },
   {
     "id": "api.templates.deactivate_body.info",
     "translation": "You deactivated your account on {{ .SiteURL }}."

store/sqlstore/user_store.go

@@ -1536,3 +1536,83 @@ func (us SqlUserStore) GetUsersBatchForIndexing(startTime, endTime int64, limit
 		result.Data = usersForIndexing
 	})
 }
+
+func (us SqlUserStore) GetTeamGroupUsers(teamID string) store.StoreChannel {
+	return store.Do(func(result *store.StoreResult) {
+		query := us.usersQuery.
+			Where(`Id IN (
+				SELECT
+					GroupMembers.UserId
+				FROM
+					Teams
+					JOIN GroupTeams ON GroupTeams.TeamId = Teams.Id
+					JOIN UserGroups ON UserGroups.Id = GroupTeams.GroupId
+					JOIN GroupMembers ON GroupMembers.GroupId = UserGroups.Id
+				WHERE
+					Teams.Id = ?
+					AND GroupTeams.DeleteAt = 0
+					AND UserGroups.DeleteAt = 0
+					AND GroupMembers.DeleteAt = 0
+				GROUP BY
+					GroupMembers.UserId
+			)`, teamID)
+
+		queryString, args, err := query.ToSql()
+		if err != nil {
+			result.Err = model.NewAppError("SqlUserStore.UsersPermittedToTeam", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		var users []*model.User
+		if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil {
+			result.Err = model.NewAppError("SqlUserStore.UsersPermittedToTeam", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		for _, u := range users {
+			u.Sanitize(map[string]bool{})
+		}
+
+		result.Data = users
+	})
+}
+
+func (us SqlUserStore) GetChannelGroupUsers(channelID string) store.StoreChannel {
+	return store.Do(func(result *store.StoreResult) {
+		query := us.usersQuery.
+			Where(`Id IN (
+				SELECT
+					GroupMembers.UserId
+				FROM
+					Channels
+					JOIN GroupChannels ON GroupChannels.ChannelId = Channels.Id
+					JOIN UserGroups ON UserGroups.Id = GroupChannels.GroupId
+					JOIN GroupMembers ON GroupMembers.GroupId = UserGroups.Id
+				WHERE
+					Channels.Id = ?
+					AND GroupChannels.DeleteAt = 0
+					AND UserGroups.DeleteAt = 0
+					AND GroupMembers.DeleteAt = 0
+				GROUP BY
+					GroupMembers.UserId
+			)`, channelID)
+
+		queryString, args, err := query.ToSql()
+		if err != nil {
+			result.Err = model.NewAppError("SqlUserStore.GetChannelGroupUsers", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		var users []*model.User
+		if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil {
+			result.Err = model.NewAppError("SqlUserStore.GetChannelGroupUsers", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		for _, u := range users {
+			u.Sanitize(map[string]bool{})
+		}
+
+		result.Data = users
+	})
+}

store/store.go

@@ -294,6 +294,8 @@ type UserStore interface {
 	GetAllAfter(limit int, afterId string) StoreChannel
 	GetUsersBatchForIndexing(startTime, endTime int64, limit int) StoreChannel
 	Count(options model.UserCountOptions) StoreChannel
+	GetTeamGroupUsers(teamID string) StoreChannel
+	GetChannelGroupUsers(channelID string) StoreChannel
 }
 
 type BotStore interface {

store/storetest/mocks/UserStore.go

@@ -848,3 +848,35 @@ func (_m *UserStore) VerifyEmail(userId string, email string) store.StoreChannel
 
 	return r0
 }
+
+// GetTeamGroupUsers provides a mock function with given fields: userId, email
+func (_m *UserStore) GetTeamGroupUsers(teamID string) store.StoreChannel {
+	ret := _m.Called(teamID)
+
+	var r0 store.StoreChannel
+	if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok {
+		r0 = rf(teamID)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(store.StoreChannel)
+		}
+	}
+
+	return r0
+}
+
+// GetChannelGroupUsers provides a mock function with given fields: userId, email
+func (_m *UserStore) GetChannelGroupUsers(teamID string) store.StoreChannel {
+	ret := _m.Called(teamID)
+
+	var r0 store.StoreChannel
+	if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok {
+		r0 = rf(teamID)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(store.StoreChannel)
+		}
+	}
+
+	return r0
+}

store/storetest/user_store.go

@@ -65,6 +65,8 @@ func TestUserStore(t *testing.T, ss store.Store) {
 	t.Run("ClearAllCustomRoleAssignments", func(t *testing.T) { testUserStoreClearAllCustomRoleAssignments(t, ss) })
 	t.Run("GetAllAfter", func(t *testing.T) { testUserStoreGetAllAfter(t, ss) })
 	t.Run("GetUsersBatchForIndexing", func(t *testing.T) { testUserStoreGetUsersBatchForIndexing(t, ss) })
+	t.Run("GetTeamGroupUsers", func(t *testing.T) { testUserStoreGetTeamGroupUsers(t, ss) })
+	t.Run("GetChannelGroupUsers", func(t *testing.T) { testUserStoreGetChannelGroupUsers(t, ss) })
 }
 
 func testUserStoreSave(t *testing.T, ss store.Store) {
@@ -3430,3 +3432,252 @@ func testUserStoreGetUsersBatchForIndexing(t *testing.T, ss store.Store) {
 	assert.Equal(t, res4List[0].Username, u1.Username)
 	assert.Equal(t, res4List[1].Username, u2.Username)
 }
+
+func testUserStoreGetTeamGroupUsers(t *testing.T, ss store.Store) {
+	// create team
+	id := model.NewId()
+	res := <-ss.Team().Save(&model.Team{
+		DisplayName: "dn_" + id,
+		Name:        "n-" + id,
+		Email:       id + "@test.com",
+		Type:        model.TEAM_INVITE,
+	})
+	require.Nil(t, res.Err)
+	team := res.Data.(*model.Team)
+	require.NotNil(t, team)
+
+	// create users
+	var testUsers []*model.User
+	for i := 0; i < 3; i++ {
+		id = model.NewId()
+		res = <-ss.User().Save(&model.User{
+			Email:     id + "@test.com",
+			Username:  "un_" + id,
+			Nickname:  "nn_" + id,
+			FirstName: "f_" + id,
+			LastName:  "l_" + id,
+			Password:  "Password1",
+		})
+		require.Nil(t, res.Err)
+		user := res.Data.(*model.User)
+		require.NotNil(t, user)
+		testUsers = append(testUsers, user)
+	}
+	userGroupA := testUsers[0]
+	userGroupB := testUsers[1]
+	userNoGroup := testUsers[2]
+
+	// add non-group-member to the team (to prove that the query isn't just returning all members)
+	res = <-ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: userNoGroup.Id,
+	}, 999)
+	require.Nil(t, res.Err)
+
+	// create groups
+	var testGroups []*model.Group
+	for i := 0; i < 2; i++ {
+		id = model.NewId()
+		res = <-ss.Group().Create(&model.Group{
+			Name:        "n_" + id,
+			DisplayName: "dn_" + id,
+			Source:      model.GroupSourceLdap,
+			RemoteId:    "ri_" + id,
+		})
+		require.Nil(t, res.Err)
+		group := res.Data.(*model.Group)
+		require.NotNil(t, group)
+		testGroups = append(testGroups, group)
+	}
+	groupA := testGroups[0]
+	groupB := testGroups[1]
+
+	// add members to groups
+	res = <-ss.Group().CreateOrRestoreMember(groupA.Id, userGroupA.Id)
+	require.Nil(t, res.Err)
+	res = <-ss.Group().CreateOrRestoreMember(groupB.Id, userGroupB.Id)
+	require.Nil(t, res.Err)
+
+	// association one group to team
+	res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    groupA.Id,
+		SyncableId: team.Id,
+		Type:       model.GroupSyncableTypeTeam,
+	})
+	require.Nil(t, res.Err)
+
+	var users []*model.User
+
+	requireNUsers := func(n int) {
+		res = <-ss.User().GetTeamGroupUsers(team.Id)
+		require.Nil(t, res.Err)
+		users = res.Data.([]*model.User)
+		require.NotNil(t, users)
+		require.Len(t, users, n)
+	}
+
+	// team not group constrained returns users
+	requireNUsers(1)
+
+	// update team to be group-constrained
+	team.GroupConstrained = model.NewBool(true)
+	res = <-ss.Team().Update(team)
+	require.Nil(t, res.Err)
+
+	// still returns user (being group-constrained has no effect)
+	requireNUsers(1)
+
+	// associate other group to team
+	res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    groupB.Id,
+		SyncableId: team.Id,
+		Type:       model.GroupSyncableTypeTeam,
+	})
+	require.Nil(t, res.Err)
+
+	// should return users from all groups
+	// 2 users now that both groups have been associated to the team
+	requireNUsers(2)
+
+	// add team membership of allowed user
+	res = <-ss.Team().SaveMember(&model.TeamMember{
+		TeamId: team.Id,
+		UserId: userGroupA.Id,
+	}, 999)
+	require.Nil(t, res.Err)
+
+	// ensure allowed member still returned by query
+	requireNUsers(2)
+
+	// delete team membership of allowed user
+	res = <-ss.Team().RemoveMember(team.Id, userGroupA.Id)
+	require.Nil(t, res.Err)
+
+	// ensure removed allowed member still returned by query
+	requireNUsers(2)
+}
+
+func testUserStoreGetChannelGroupUsers(t *testing.T, ss store.Store) {
+	// create channel
+	id := model.NewId()
+	res := <-ss.Channel().Save(&model.Channel{
+		DisplayName: "dn_" + id,
+		Name:        "n-" + id,
+		Type:        model.CHANNEL_PRIVATE,
+	}, 999)
+	require.Nil(t, res.Err)
+	channel := res.Data.(*model.Channel)
+	require.NotNil(t, channel)
+
+	// create users
+	var testUsers []*model.User
+	for i := 0; i < 3; i++ {
+		id = model.NewId()
+		res = <-ss.User().Save(&model.User{
+			Email:     id + "@test.com",
+			Username:  "un_" + id,
+			Nickname:  "nn_" + id,
+			FirstName: "f_" + id,
+			LastName:  "l_" + id,
+			Password:  "Password1",
+		})
+		require.Nil(t, res.Err)
+		user := res.Data.(*model.User)
+		require.NotNil(t, user)
+		testUsers = append(testUsers, user)
+	}
+	userGroupA := testUsers[0]
+	userGroupB := testUsers[1]
+	userNoGroup := testUsers[2]
+
+	// add non-group-member to the channel (to prove that the query isn't just returning all members)
+	res = <-ss.Channel().SaveMember(&model.ChannelMember{
+		ChannelId:   channel.Id,
+		UserId:      userNoGroup.Id,
+		NotifyProps: model.GetDefaultChannelNotifyProps(),
+	})
+	require.Nil(t, res.Err)
+
+	// create groups
+	var testGroups []*model.Group
+	for i := 0; i < 2; i++ {
+		id = model.NewId()
+		res = <-ss.Group().Create(&model.Group{
+			Name:        "n_" + id,
+			DisplayName: "dn_" + id,
+			Source:      model.GroupSourceLdap,
+			RemoteId:    "ri_" + id,
+		})
+		require.Nil(t, res.Err)
+		group := res.Data.(*model.Group)
+		require.NotNil(t, group)
+		testGroups = append(testGroups, group)
+	}
+	groupA := testGroups[0]
+	groupB := testGroups[1]
+
+	// add members to groups
+	res = <-ss.Group().CreateOrRestoreMember(groupA.Id, userGroupA.Id)
+	require.Nil(t, res.Err)
+	res = <-ss.Group().CreateOrRestoreMember(groupB.Id, userGroupB.Id)
+	require.Nil(t, res.Err)
+
+	// association one group to channel
+	res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    groupA.Id,
+		SyncableId: channel.Id,
+		Type:       model.GroupSyncableTypeChannel,
+	})
+	require.Nil(t, res.Err)
+
+	var users []*model.User
+
+	requireNUsers := func(n int) {
+		res = <-ss.User().GetChannelGroupUsers(channel.Id)
+		require.Nil(t, res.Err)
+		users = res.Data.([]*model.User)
+		require.NotNil(t, users)
+		require.Len(t, users, n)
+	}
+
+	// channel not group constrained returns users
+	requireNUsers(1)
+
+	// update team to be group-constrained
+	channel.GroupConstrained = model.NewBool(true)
+	res = <-ss.Channel().Update(channel)
+	require.Nil(t, res.Err)
+
+	// still returns user (being group-constrained has no effect)
+	requireNUsers(1)
+
+	// associate other group to team
+	res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{
+		GroupId:    groupB.Id,
+		SyncableId: channel.Id,
+		Type:       model.GroupSyncableTypeChannel,
+	})
+	require.Nil(t, res.Err)
+
+	// should return users from all groups
+	// 2 users now that both groups have been associated to the team
+	requireNUsers(2)
+
+	// add team membership of allowed user
+	res = <-ss.Channel().SaveMember(&model.ChannelMember{
+		ChannelId:   channel.Id,
+		UserId:      userGroupA.Id,
+		NotifyProps: model.GetDefaultChannelNotifyProps(),
+	})
+	require.Nil(t, res.Err)
+
+	// ensure allowed member still returned by query
+	requireNUsers(2)
+
+	// delete team membership of allowed user
+	res = <-ss.Channel().RemoveMember(channel.Id, userGroupA.Id)
+	require.Nil(t, res.Err)
+
+	// ensure removed allowed member still returned by query
+	requireNUsers(2)
+}