From fa769e46d77d78affcfac7c3e0084c501c3599b8 Mon Sep 17 00:00:00 2001 From: Farhan Munshi <3207297+fm2munsh@users.noreply.github.com> Date: Wed, 29 Jan 2020 11:01:06 -0500 Subject: [PATCH] [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 | 20 +++++++++++ api4/channel.go | 8 ++++- api4/channel_test.go | 12 +++++++ api4/team.go | 8 ++++- api4/team_test.go | 21 ++++++++++++ app/channel.go | 2 +- app/channel_test.go | 79 ++++++++++++++++++++++++++++++++++++++++++++ app/helper_test.go | 22 ++++++++++++ app/user.go | 60 +++++++++++---------------------- i18n/en.json | 4 +-- 10 files changed, 191 insertions(+), 45 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index cdcf490a7f..ebf7404d73 100644 --- a/api4/apitestlib.go +++ b/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) } diff --git a/api4/channel.go b/api4/channel.go index 4b6eb66e6a..1688753294 100644 --- a/api4/channel.go +++ b/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 } diff --git a/api4/channel_test.go b/api4/channel_test.go index 2e4a8beeef..2648359982 100644 --- a/api4/channel_test.go +++ b/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) diff --git a/api4/team.go b/api4/team.go index 460316d958..d970c21ce5 100644 --- a/api4/team.go +++ b/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 } diff --git a/api4/team_test.go b/api4/team_test.go index 16ab958475..07ae8c8493 100644 --- a/api4/team_test.go +++ b/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) diff --git a/app/channel.go b/app/channel.go index 0841550c14..bb93df581f 100644 --- a/app/channel.go +++ b/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) diff --git a/app/channel_test.go b/app/channel_test.go index df6923a4ad..9d39b0a3eb 100644 --- a/app/channel_test.go +++ b/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) } diff --git a/app/helper_test.go b/app/helper_test.go index a72627435a..9a6db5216f 100644 --- a/app/helper_test.go +++ b/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) } diff --git a/app/user.go b/app/user.go index eda8dfeff5..7df8c7deeb 100644 --- a/app/user.go +++ b/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) { diff --git a/i18n/en.json b/i18n/en.json index 2405e793b2..3e4253dad0 100644 --- a/i18n/en.json +++ b/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",