[MM-21793] Allow bots to be added to group synced channels and teams (#13672)

* MM-21793: Allow bots to be removed and added from group synced teams and channels

* MM-21793: Add tests for adding and removing a team and channel members

* MM-21793 Add punctuation to comments and remove unnecessary variable

api4/apitestlib.go

@@ -253,6 +253,26 @@ func (me *TestHelper) CreateWebSocketClientWithClient(client *model.Client4) (*m
 	return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", me.App.Srv.ListenAddr.Port), client.AuthToken)
 }
 
+func (me *TestHelper) CreateBotWithSystemAdminClient() *model.Bot {
+	return me.CreateBotWithClient((me.SystemAdminClient))
+}
+
+func (me *TestHelper) CreateBotWithClient(client *model.Client4) *model.Bot {
+	bot := &model.Bot{
+		Username:    GenerateTestUsername(),
+		DisplayName: "a bot",
+		Description: "bot",
+	}
+
+	utils.DisableDebugLogForTest()
+	rbot, resp := client.CreateBot(bot)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
+	utils.EnableDebugLogForTest()
+	return rbot
+}
+
 func (me *TestHelper) CreateUser() *model.User {
 	return me.CreateUserWithClient(me.Client)
 }

api4/channel.go

@@ -1398,12 +1398,18 @@ func removeChannelMember(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	user, err := c.App.GetUser(c.Params.UserId)
+	if err != nil {
+		c.Err = err
+		return
+	}
+
 	if !(channel.Type == model.CHANNEL_OPEN || channel.Type == model.CHANNEL_PRIVATE) {
 		c.Err = model.NewAppError("removeChannelMember", "api.channel.remove_channel_member.type.app_error", nil, "", http.StatusBadRequest)
 		return
 	}
 
-	if channel.IsGroupConstrained() && (c.Params.UserId != c.App.Session.UserId) {
+	if channel.IsGroupConstrained() && (c.Params.UserId != c.App.Session.UserId) && !user.IsBot {
 		c.Err = model.NewAppError("removeChannelMember", "api.channel.remove_member.group_constrained.app_error", nil, "", http.StatusBadRequest)
 		return
 	}

api4/channel_test.go

@@ -2387,6 +2387,12 @@ func TestRemoveChannelMember(t *testing.T) {
 	defer th.TearDown()
 	Client := th.Client
 
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.EnableBotAccountCreation = true
+	})
+	bot := th.CreateBotWithSystemAdminClient()
+	th.App.AddUserToTeam(team.Id, bot.UserId, "")
+
 	pass, resp := Client.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser2.Id)
 	CheckNoError(t, resp)
 	require.True(t, pass, "should have passed")
@@ -2536,6 +2542,8 @@ func TestRemoveChannelMember(t *testing.T) {
 	CheckNoError(t, resp)
 	_, resp = th.SystemAdminClient.AddChannelMember(privateChannel.Id, user2.Id)
 	CheckNoError(t, resp)
+	_, resp = th.SystemAdminClient.AddChannelMember(privateChannel.Id, bot.UserId)
+	CheckNoError(t, resp)
 
 	_, resp = Client.RemoveUserFromChannel(privateChannel.Id, user2.Id)
 	CheckForbiddenStatus(t, resp)
@@ -2564,6 +2572,10 @@ func TestRemoveChannelMember(t *testing.T) {
 	directChannel, resp := Client.CreateDirectChannel(user1.Id, user2.Id)
 	CheckNoError(t, resp)
 
+	// If the channel is group-constrained a user can remove a bot
+	_, resp = Client.RemoveUserFromChannel(privateChannel.Id, bot.UserId)
+	CheckNoError(t, resp)
+
 	_, resp = Client.RemoveUserFromChannel(directChannel.Id, user1.Id)
 	CheckBadRequestStatus(t, resp)
 

api4/team.go

@@ -626,7 +626,13 @@ func removeTeamMember(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if team.IsGroupConstrained() && (c.Params.UserId != c.App.Session.UserId) {
+	user, err := c.App.GetUser(c.Params.UserId)
+	if err != nil {
+		c.Err = err
+		return
+	}
+
+	if team.IsGroupConstrained() && (c.Params.UserId != c.App.Session.UserId) && !user.IsBot {
 		c.Err = model.NewAppError("removeTeamMember", "api.team.remove_member.group_constrained.app_error", nil, "", http.StatusBadRequest)
 		return
 	}

api4/team_test.go

@@ -1721,6 +1721,11 @@ func TestAddTeamMembers(t *testing.T) {
 		otherUser.Id,
 	}
 
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.EnableBotAccountCreation = true
+	})
+	bot := th.CreateBotWithSystemAdminClient()
+
 	err := th.App.RemoveUserFromTeam(th.BasicTeam.Id, th.BasicUser2.Id, "")
 	require.Nil(t, err)
 
@@ -1813,6 +1818,10 @@ func TestAddTeamMembers(t *testing.T) {
 	_, resp = Client.AddTeamMembers(team.Id, userList)
 	CheckErrorMessage(t, resp, "api.team.add_members.user_denied")
 
+	// Ensure that a group synced team can still add bots
+	_, resp = Client.AddTeamMembers(team.Id, []string{bot.UserId})
+	CheckNoError(t, resp)
+
 	// Associate group to team
 	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		GroupId:    th.Group.Id,
@@ -1834,6 +1843,11 @@ func TestRemoveTeamMember(t *testing.T) {
 	defer th.TearDown()
 	Client := th.Client
 
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.EnableBotAccountCreation = true
+	})
+	bot := th.CreateBotWithSystemAdminClient()
+
 	pass, resp := Client.RemoveTeamMember(th.BasicTeam.Id, th.BasicUser.Id)
 	CheckNoError(t, resp)
 
@@ -1860,6 +1874,9 @@ func TestRemoveTeamMember(t *testing.T) {
 	_, resp = th.SystemAdminClient.AddTeamMember(th.BasicTeam.Id, th.SystemAdminUser.Id)
 	CheckNoError(t, resp)
 
+	_, resp = th.SystemAdminClient.AddTeamMember(th.BasicTeam.Id, bot.UserId)
+	CheckNoError(t, resp)
+
 	// If the team is group-constrained the user cannot be removed
 	th.BasicTeam.GroupConstrained = model.NewBool(true)
 	_, err := th.App.UpdateTeam(th.BasicTeam)
@@ -1867,6 +1884,10 @@ func TestRemoveTeamMember(t *testing.T) {
 	_, resp = th.SystemAdminClient.RemoveTeamMember(th.BasicTeam.Id, th.BasicUser.Id)
 	require.Equal(t, "api.team.remove_member.group_constrained.app_error", resp.Error.Id)
 
+	// Can remove a bot even if team is group-constrained
+	_, resp = th.SystemAdminClient.RemoveTeamMember(th.BasicTeam.Id, bot.UserId)
+	CheckNoError(t, resp)
+
 	// Can remove self even if team is group-constrained
 	_, resp = th.SystemAdminClient.RemoveTeamMember(th.BasicTeam.Id, th.SystemAdminUser.Id)
 	CheckNoError(t, resp)

app/channel.go

@@ -1696,7 +1696,7 @@ func (a *App) removeUserFromChannel(userIdToRemove string, removerUserId string,
 		}
 	}
 
-	if channel.IsGroupConstrained() && userIdToRemove != removerUserId {
+	if channel.IsGroupConstrained() && userIdToRemove != removerUserId && !user.IsBot {
 		nonMembers, err := a.FilterNonGroupChannelMembers([]string{userIdToRemove}, channel)
 		if err != nil {
 			return model.NewAppError("removeUserFromChannel", "api.channel.remove_user_from_channel.app_error", nil, "", http.StatusInternalServerError)

app/channel_test.go

@@ -1169,7 +1169,12 @@ func TestAddUserToChannel(t *testing.T) {
 	user1 := model.User{Email: strings.ToLower(model.NewId()) + "success+test@example.com", Nickname: "Darth Vader", Username: "vader" + model.NewId(), Password: "passwd1", AuthService: ""}
 	ruser1, _ := th.App.CreateUser(&user1)
 	defer th.App.PermanentDeleteUser(&user1)
+	bot := th.CreateBot()
+	botUser, _ := th.App.GetUser(bot.UserId)
+	defer th.App.PermanentDeleteBot(botUser.Id)
+
 	th.App.AddTeamMember(th.BasicTeam.Id, ruser1.Id)
+	th.App.AddTeamMember(th.BasicTeam.Id, bot.UserId)
 
 	group := th.CreateGroup()
 
@@ -1208,8 +1213,82 @@ func TestAddUserToChannel(t *testing.T) {
 	err = th.App.JoinChannel(th.BasicChannel, ruser2.Id)
 	require.Nil(t, err)
 
+	// Should allow a bot to be added to a public group synced channel
+	_, err = th.App.AddUserToChannel(botUser, th.BasicChannel)
+	require.Nil(t, err)
+
 	// verify user was added as an admin
 	cm2, err := th.App.GetChannelMember(th.BasicChannel.Id, ruser2.Id)
 	require.Nil(t, err)
 	require.True(t, cm2.SchemeAdmin)
+
+	privateChannel := th.CreatePrivateChannel(th.BasicTeam)
+	privateChannel.GroupConstrained = model.NewBool(true)
+	_, err = th.App.UpdateChannel(privateChannel)
+	require.Nil(t, err)
+
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
+		GroupId:    group.Id,
+		SyncableId: privateChannel.Id,
+		Type:       model.GroupSyncableTypeChannel,
+	})
+	require.Nil(t, err)
+
+	// Should allow a group synced user to be added to a group synced private channel
+	_, err = th.App.AddUserToChannel(ruser1, privateChannel)
+	require.Nil(t, err)
+
+	// Should allow a bot to be added to a private group synced channel
+	_, err = th.App.AddUserToChannel(botUser, privateChannel)
+	require.Nil(t, err)
+}
+
+func TestRemoveUserFromChannel(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	user := model.User{Email: strings.ToLower(model.NewId()) + "success+test@example.com", Nickname: "Darth Vader", Username: "vader" + model.NewId(), Password: "passwd1", AuthService: ""}
+	ruser, _ := th.App.CreateUser(&user)
+	defer th.App.PermanentDeleteUser(ruser)
+
+	bot := th.CreateBot()
+	botUser, _ := th.App.GetUser(bot.UserId)
+	defer th.App.PermanentDeleteBot(botUser.Id)
+
+	th.App.AddTeamMember(th.BasicTeam.Id, ruser.Id)
+	th.App.AddTeamMember(th.BasicTeam.Id, bot.UserId)
+
+	privateChannel := th.CreatePrivateChannel(th.BasicTeam)
+
+	_, err := th.App.AddUserToChannel(ruser, privateChannel)
+	require.Nil(t, err)
+	_, err = th.App.AddUserToChannel(botUser, privateChannel)
+	require.Nil(t, err)
+
+	group := th.CreateGroup()
+	_, err = th.App.UpsertGroupMember(group.Id, ruser.Id)
+	require.Nil(t, err)
+
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
+		GroupId:    group.Id,
+		SyncableId: privateChannel.Id,
+		Type:       model.GroupSyncableTypeChannel,
+	})
+	require.Nil(t, err)
+
+	privateChannel.GroupConstrained = model.NewBool(true)
+	_, err = th.App.UpdateChannel(privateChannel)
+	require.Nil(t, err)
+
+	// Should not allow a group synced user to be removed from channel
+	err = th.App.RemoveUserFromChannel(ruser.Id, th.SystemAdminUser.Id, privateChannel)
+	assert.Equal(t, err.Id, "api.channel.remove_members.denied")
+
+	// Should allow a user to remove themselves from group synced channel
+	err = th.App.RemoveUserFromChannel(ruser.Id, ruser.Id, privateChannel)
+	require.Nil(t, err)
+
+	// Should allow a bot to be removed from a group synced channel
+	err = th.App.RemoveUserFromChannel(botUser.Id, th.SystemAdminUser.Id, privateChannel)
+	require.Nil(t, err)
 }

app/helper_test.go

@@ -194,6 +194,28 @@ func (me *TestHelper) CreateUserOrGuest(guest bool) *model.User {
 	return user
 }
 
+func (me *TestHelper) CreateBot() *model.Bot {
+	id := model.NewId()
+
+	bot := &model.Bot{
+		Username:    "bot" + id,
+		DisplayName: "a bot",
+		Description: "bot",
+		OwnerId:     me.BasicUser.Id,
+	}
+
+	me.App.Log.SetConsoleLevel(mlog.LevelError)
+	bot, err := me.App.CreateBot(bot)
+	if err != nil {
+		mlog.Error(err.Error())
+
+		time.Sleep(time.Second)
+		panic(err)
+	}
+	me.App.Log.SetConsoleLevel(mlog.LevelDebug)
+	return bot
+}
+
 func (me *TestHelper) CreateChannel(team *model.Team) *model.Channel {
 	return me.createChannel(team, model.CHANNEL_OPEN)
 }

app/user.go

@@ -2064,69 +2064,49 @@ func (a *App) RestrictUsersGetByPermissions(userId string, options *model.UserGe
 }
 
 // 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) {
+// associated to the team excluding bots.
+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
+	return a.filterNonGroupUsers(userIds, teamGroupUsers)
 }
 
 // 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) {
+// associated to the channel excluding bots
+func (a *App) FilterNonGroupChannelMembers(userIds []string, channel *model.Channel) ([]string, error) {
 	channelGroupUsers, err := a.GetChannelGroupUsers(channel.Id)
 	if err != nil {
 		return nil, err
 	}
+	return a.filterNonGroupUsers(userIds, channelGroupUsers)
+}
 
-	// possible if no groups associated or no group members in any of the associated groups
-	if len(channelGroupUsers) == 0 {
-		return userIDs, nil
+// filterNonGroupUsers is a helper function that takes a list of user ids and a list of users
+// and returns the list of normal users present in userIds but not in groupUsers.
+func (a *App) filterNonGroupUsers(userIds []string, groupUsers []*model.User) ([]string, error) {
+	nonMemberIds := []string{}
+	users, err := a.Srv.Store.User().GetProfileByIds(userIds, nil, false)
+	if err != nil {
+		return nil, err
 	}
 
-	nonMemberIDs := []string{}
+	for _, user := range users {
+		userIsMember := user.IsBot
 
-	for _, userID := range userIDs {
-		userIsMember := false
-
-		for _, pu := range channelGroupUsers {
-			if pu.Id == userID {
+		for _, pu := range groupUsers {
+			if pu.Id == user.Id {
 				userIsMember = true
 				break
 			}
 		}
-
 		if !userIsMember {
-			nonMemberIDs = append(nonMemberIDs, userID)
+			nonMemberIds = append(nonMemberIds, user.Id)
 		}
 	}
 
-	return nonMemberIDs, nil
+	return nonMemberIds, nil
 }
 
 func (a *App) RestrictUsersSearchByPermissions(userId string, options *model.UserSearchOptions) (*model.UserSearchOptions, *model.AppError) {

i18n/en.json

@@ -812,7 +812,7 @@
   },
   {
     "id": "api.command_invite.group_constrained_user_denied",
-    "translation": "This channel is managed by groups.  This user is not part of a group that is synched to this channel."
+    "translation": "This channel is managed by groups.  This user is not part of a group that is synced to this channel."
   },
   {
     "id": "api.command_invite.hint",
@@ -1940,7 +1940,7 @@
   },
   {
     "id": "api.team.add_members.user_denied",
-    "translation": "This team is managed by groups.  This user is not part of a group that is synched to this team."
+    "translation": "This team is managed by groups.  This user is not part of a group that is synced to this team."
   },
   {
     "id": "api.team.add_user_to_team.added",