MM-14722 - Don't let bots create other bot accounts (#10808)

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

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 {

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)
 }

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)
+
+}

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"

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",