From ae951f64497f74aa9aec2195d3134e63f3f82a18 Mon Sep 17 00:00:00 2001 From: scott lee davis Date: Tue, 14 May 2019 13:15:23 -0700 Subject: [PATCH] MM-14722 - Don't let bots create other bot accounts (#10808) --- api4/bot.go | 14 +++++++++ api4/bot_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++ app/plugin_api.go | 6 ++++ app/plugin_api_test.go | 22 ++++++++++++++ i18n/en.json | 4 +++ model/permission.go | 7 +++++ 6 files changed, 120 insertions(+) diff --git a/api4/bot.go b/api4/bot.go index b1a7ad4353..2c9373f135 100644 --- a/api4/bot.go +++ b/api4/bot.go @@ -36,6 +36,13 @@ func createBot(c *Context, w http.ResponseWriter, r *http.Request) { return } + if user, err := c.App.GetUser(c.App.Session.UserId); err == nil { + if user.IsBot { + c.SetPermissionError(model.PERMISSION_CREATE_BOT) + return + } + } + if !*c.App.Config().ServiceSettings.CreateBotAccounts { c.Err = model.NewAppError("createBot", "api.bot.create_disabled", nil, "", http.StatusForbidden) return @@ -195,6 +202,13 @@ func assignBot(c *Context, w http.ResponseWriter, r *http.Request) { return } + if user, err := c.App.GetUser(userId); err == nil { + if user.IsBot { + c.SetPermissionError(model.PERMISSION_ASSIGN_BOT) + return + } + } + bot, err := c.App.UpdateBotOwner(botUserId, userId) if err != nil { c.Err = err diff --git a/api4/bot_test.go b/api4/bot_test.go index 8a5b909d69..976c727e85 100644 --- a/api4/bot_test.go +++ b/api4/bot_test.go @@ -91,6 +91,42 @@ func TestCreateBot(t *testing.T) { CheckErrorMessage(t, resp, "model.bot.is_valid.description.app_error") }) + + t.Run("bot attempt to create bot fails", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.CreateBotAccounts = true + }) + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) + th.AddPermissionToRole(model.PERMISSION_CREATE_BOT.Id, model.TEAM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_EDIT_OTHER_USERS.Id, model.TEAM_USER_ROLE_ID) + th.App.UpdateUserRoles(th.BasicUser.Id, model.TEAM_USER_ROLE_ID+" "+model.SYSTEM_USER_ACCESS_TOKEN_ROLE_ID, false) + + bot, resp := th.Client.CreateBot(&model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "a bot", + Description: "bot", + }) + CheckCreatedStatus(t, resp) + defer th.App.PermanentDeleteBot(bot.UserId) + th.App.UpdateUserRoles(bot.UserId, model.TEAM_USER_ROLE_ID+" "+model.SYSTEM_USER_ACCESS_TOKEN_ROLE_ID, false) + + rtoken, resp := th.Client.CreateUserAccessToken(bot.UserId, "test token") + CheckNoError(t, resp) + th.Client.AuthToken = rtoken.Token + + _, resp = th.Client.CreateBot(&model.Bot{ + Username: GenerateTestUsername(), + OwnerId: bot.UserId, + DisplayName: "a bot2", + Description: "bot2", + }) + CheckErrorMessage(t, resp, "api.context.permissions.app_error") + }) + } func TestPatchBot(t *testing.T) { @@ -1021,6 +1057,37 @@ func TestAssignBot(t *testing.T) { CheckOKStatus(t, resp) require.Equal(t, th.BasicUser2.Id, after.OwnerId) }) + + t.Run("bot assigned to bot fails", func(t *testing.T) { + defer th.RestoreDefaultRolePermissions(th.SaveDefaultRolePermissions()) + + th.AddPermissionToRole(model.PERMISSION_CREATE_BOT.Id, model.SYSTEM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_READ_BOTS.Id, model.SYSTEM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_READ_OTHERS_BOTS.Id, model.SYSTEM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_MANAGE_BOTS.Id, model.SYSTEM_USER_ROLE_ID) + th.AddPermissionToRole(model.PERMISSION_MANAGE_OTHERS_BOTS.Id, model.SYSTEM_USER_ROLE_ID) + + bot := &model.Bot{ + Username: GenerateTestUsername(), + Description: "bot", + } + bot, resp := th.Client.CreateBot(bot) + CheckCreatedStatus(t, resp) + defer th.App.PermanentDeleteBot(bot.UserId) + + bot2, resp := th.Client.CreateBot(&model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "a bot", + Description: "bot", + }) + + CheckCreatedStatus(t, resp) + defer th.App.PermanentDeleteBot(bot2.UserId) + + _, resp = th.Client.AssignBot(bot.UserId, bot2.UserId) + CheckErrorMessage(t, resp, "api.context.permissions.app_error") + + }) } func sToP(s string) *string { diff --git a/app/plugin_api.go b/app/plugin_api.go index b3464c5903..c2047cff3a 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -725,6 +725,12 @@ func (api *PluginAPI) CreateBot(bot *model.Bot) (*model.Bot, *model.AppError) { if bot.OwnerId == "" { bot.OwnerId = api.id } + // Bots cannot be owners of other bots + if user, err := api.app.GetUser(bot.OwnerId); err == nil { + if user.IsBot { + return nil, model.NewAppError("CreateBot", "plugin_api.bot_cant_create_bot", nil, "", http.StatusBadRequest) + } + } return api.app.CreateBot(bot) } diff --git a/app/plugin_api_test.go b/app/plugin_api_test.go index abec98e7b7..381743cb8d 100644 --- a/app/plugin_api_test.go +++ b/app/plugin_api_test.go @@ -755,3 +755,25 @@ func TestPluginAPIKVCompareAndSet(t *testing.T) { }) } } + +func TestPluginCreateBot(t *testing.T) { + th := Setup(t) + defer th.TearDown() + api := th.SetupPluginAPI() + + bot, err := api.CreateBot(&model.Bot{ + Username: model.NewRandomString(10), + DisplayName: "bot", + Description: "bot", + }) + require.Nil(t, err) + + _, err = api.CreateBot(&model.Bot{ + Username: model.NewRandomString(10), + OwnerId: bot.UserId, + DisplayName: "bot2", + Description: "bot2", + }) + require.NotNil(t, err) + +} diff --git a/i18n/en.json b/i18n/en.json index 10075abb67..53a265c932 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -5170,6 +5170,10 @@ "id": "plugin.api.update_user_status.bad_status", "translation": "Unable to set the user status. Unknown user status." }, + { + "id": "plugin_api.bot_cant_create_bot", + "translation": "Bot user cannot create bot user." + }, { "id": "plugin_api.get_file_link.disabled.app_error", "translation": "Public links have been disabled" diff --git a/model/permission.go b/model/permission.go index ffa9ace12f..09a506c840 100644 --- a/model/permission.go +++ b/model/permission.go @@ -81,6 +81,7 @@ var PERMISSION_CREATE_USER_ACCESS_TOKEN *Permission var PERMISSION_READ_USER_ACCESS_TOKEN *Permission var PERMISSION_REVOKE_USER_ACCESS_TOKEN *Permission var PERMISSION_CREATE_BOT *Permission +var PERMISSION_ASSIGN_BOT *Permission var PERMISSION_READ_BOTS *Permission var PERMISSION_READ_OTHERS_BOTS *Permission var PERMISSION_MANAGE_BOTS *Permission @@ -490,6 +491,12 @@ func initializePermissions() { "authentication.permissions.create_bot.description", PERMISSION_SCOPE_SYSTEM, } + PERMISSION_ASSIGN_BOT = &Permission{ + "assign_bot", + "authentication.permissions.assign_bot.name", + "authentication.permissions.assign_bot.description", + PERMISSION_SCOPE_SYSTEM, + } PERMISSION_READ_BOTS = &Permission{ "read_bots", "authentication.permissions.read_bots.name",