[MM-67377] cherry-pick Fix (#35336) (#35657)

Automatic Merge
2026-03-20 12:30:54 +01:00
--- a/server/channels/api4/command.go
+++ b/server/channels/api4/command.go
@@ -169,6 +169,18 @@ func moveCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	if c.AppContext.Session().UserId != cmd.CreatorId && !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), cmd.TeamId, model.PermissionManageOthersSlashCommands) {
+		c.LogAudit("fail - inappropriate permissions")
+		c.SetPermissionError(model.PermissionManageOthersSlashCommands)
+		return
+	}
+
+	// Verify the command creator is a member of the destination team
+	if _, appErr = c.App.GetTeamMember(c.AppContext, cmr.TeamId, cmd.CreatorId); appErr != nil {
+		c.Err = model.NewAppError("moveCommand", "api.command.move_command.creator_not_in_team.app_error", nil, "", http.StatusBadRequest)
+		return
+	}
+
 	if appErr = c.App.MoveCommand(newTeam, cmd); appErr != nil {
 		c.Err = appErr
 		return
--- a/server/channels/api4/command_test.go
+++ b/server/channels/api4/command_test.go
@@ -162,6 +162,136 @@ func TestUpdateCommand(t *testing.T) {
 	_, resp, err := th.SystemAdminClient.UpdateCommand(context.Background(), cmd2)
 	require.Error(t, err)
 	CheckUnauthorizedStatus(t, resp)
+
+	// Permission tests
+	th.LoginBasic()
+
+	// Give BasicUser permission to manage their own commands
+	th.AddPermissionToRole(model.PermissionManageSlashCommands.Id, model.TeamUserRoleId)
+	defer th.RemovePermissionFromRole(model.PermissionManageSlashCommands.Id, model.TeamUserRoleId)
+
+	t.Run("UserCanUpdateTheirOwnCommand", func(t *testing.T) {
+		// Create a command owned by BasicUser
+		cmd := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger_own",
+		}
+		createdCmd, _ := th.App.CreateCommand(cmd)
+
+		// Update the command
+		createdCmd.URL = "http://newurl.com"
+		updatedCmd, _, err := th.Client.UpdateCommand(context.Background(), createdCmd)
+		require.NoError(t, err)
+		require.Equal(t, "http://newurl.com", updatedCmd.URL)
+	})
+
+	t.Run("UserWithoutManageOthersCannotUpdateOthersCommand", func(t *testing.T) {
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger_other",
+		}
+		createdCmd, _ := th.App.CreateCommand(cmd)
+
+		// Try to update the command
+		createdCmd.URL = "http://newurl.com"
+		_, resp, err := th.Client.UpdateCommand(context.Background(), createdCmd)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("UserWithManageOthersCanUpdateOthersCommand", func(t *testing.T) {
+		// Give BasicUser permission to manage others' commands
+		th.AddPermissionToRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+		defer th.RemovePermissionFromRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger_other2",
+		}
+		createdCmd, _ := th.App.CreateCommand(cmd)
+
+		// Update the command
+		createdCmd.URL = "http://newurl.com"
+		updatedCmd, _, err := th.Client.UpdateCommand(context.Background(), createdCmd)
+		require.NoError(t, err)
+		require.Equal(t, "http://newurl.com", updatedCmd.URL)
+	})
+
+	t.Run("UserWithOnlyManageOwnCannotUpdateOthersCommand", func(t *testing.T) {
+		// BasicUser should only have ManageOwn permission (already set up in the test)
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger_other3",
+		}
+		createdCmd, _ := th.App.CreateCommand(cmd)
+
+		// Try to update the command
+		createdCmd.URL = "http://newurl.com"
+		_, resp, err := th.Client.UpdateCommand(context.Background(), createdCmd)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("CannotUpdateCommandToDuplicateCustomTrigger", func(t *testing.T) {
+		cmdA := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com/a",
+			Method:    model.CommandMethodPost,
+			Trigger:   "duplicate_custom_a",
+		}
+		createdCmdA, appErr := th.App.CreateCommand(cmdA)
+		require.Nil(t, appErr)
+
+		cmdB := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com/b",
+			Method:    model.CommandMethodPost,
+			Trigger:   "duplicate_custom_b",
+		}
+		createdCmdB, appErr := th.App.CreateCommand(cmdB)
+		require.Nil(t, appErr)
+
+		createdCmdB.Trigger = createdCmdA.Trigger
+		_, resp, err := th.Client.UpdateCommand(context.Background(), createdCmdB)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+		CheckErrorID(t, err, "api.command.duplicate_trigger.app_error")
+	})
+
+	t.Run("CannotUpdateCommandToBuiltInTrigger", func(t *testing.T) {
+		cmd := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com/c",
+			Method:    model.CommandMethodPost,
+			Trigger:   "custom_for_builtin_collision",
+		}
+		createdCmd, appErr := th.App.CreateCommand(cmd)
+		require.Nil(t, appErr)
+
+		createdCmd.Trigger = "join"
+		_, resp, err := th.Client.UpdateCommand(context.Background(), createdCmd)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+		CheckErrorID(t, err, "api.command.duplicate_trigger.app_error")
+	})
 }

 func TestMoveCommand(t *testing.T) {
@@ -178,6 +308,8 @@ func TestMoveCommand(t *testing.T) {
 	}()
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true })

+	th.LinkUserToTeam(user, newTeam)
+
 	cmd1 := &model.Command{
 		CreatorId: user.Id,
 		TeamId:    team.Id,
@@ -222,6 +354,165 @@ func TestMoveCommand(t *testing.T) {
 	resp, err = th.SystemAdminClient.MoveCommand(context.Background(), newTeam.Id, rcmd2.Id)
 	require.Error(t, err)
 	CheckUnauthorizedStatus(t, resp)
+
+	// Set up for permission tests
+	th.LoginBasic()
+	th.LinkUserToTeam(th.BasicUser, newTeam)
+	th.LinkUserToTeam(th.BasicUser2, newTeam)
+
+	// Give BasicUser permission to manage their own commands on both teams
+	th.AddPermissionToRole(model.PermissionManageSlashCommands.Id, model.TeamUserRoleId)
+	defer th.RemovePermissionFromRole(model.PermissionManageSlashCommands.Id, model.TeamUserRoleId)
+
+	t.Run("UserWithoutManageOthersPermissionCannotMoveOthersCommand", func(t *testing.T) {
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger3",
+		}
+		rcmd, _ := th.App.CreateCommand(cmd)
+
+		// BasicUser should not be able to move BasicUser2's command
+		resp, err := th.Client.MoveCommand(context.Background(), newTeam.Id, rcmd.Id)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+
+		// Verify the command was not moved
+		movedCmd, _ := th.App.GetCommand(rcmd.Id)
+		require.Equal(t, team.Id, movedCmd.TeamId)
+	})
+
+	t.Run("UserWithManageOthersPermissionCanMoveOthersCommand", func(t *testing.T) {
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger4",
+		}
+		rcmd, _ := th.App.CreateCommand(cmd)
+
+		// Give BasicUser the permission to manage others' commands
+		th.AddPermissionToRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+		defer th.RemovePermissionFromRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+
+		// Now BasicUser should be able to move BasicUser2's command
+		_, err := th.Client.MoveCommand(context.Background(), newTeam.Id, rcmd.Id)
+		require.NoError(t, err)
+
+		// Verify the command was moved
+		movedCmd, _ := th.App.GetCommand(rcmd.Id)
+		require.Equal(t, newTeam.Id, movedCmd.TeamId)
+	})
+
+	t.Run("CreatorCanMoveTheirOwnCommand", func(t *testing.T) {
+		// Create a command owned by BasicUser
+		cmd := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger5",
+		}
+		rcmd, _ := th.App.CreateCommand(cmd)
+
+		// BasicUser should be able to move their own command
+		_, err := th.Client.MoveCommand(context.Background(), newTeam.Id, rcmd.Id)
+		require.NoError(t, err)
+
+		// Verify the command was moved
+		movedCmd, _ := th.App.GetCommand(rcmd.Id)
+		require.Equal(t, newTeam.Id, movedCmd.TeamId)
+	})
+
+	t.Run("UserWithOnlyManageOwnCannotMoveOthersCommand", func(t *testing.T) {
+		// BasicUser should only have ManageOwn permission (already set up in the test)
+		// Create a command owned by BasicUser2
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger6",
+		}
+		rcmd, _ := th.App.CreateCommand(cmd)
+
+		// BasicUser should not be able to move BasicUser2's command
+		resp, err := th.Client.MoveCommand(context.Background(), newTeam.Id, rcmd.Id)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+
+		// Verify the command was not moved
+		notMovedCmd, _ := th.App.GetCommand(rcmd.Id)
+		require.Equal(t, team.Id, notMovedCmd.TeamId)
+	})
+
+	t.Run("CannotMoveCommandWhenCreatorHasNoPermissionToNewTeam", func(t *testing.T) {
+		// Create a third team that the command creator (BasicUser2) is NOT a member of
+		thirdTeam := th.CreateTeam()
+		th.LinkUserToTeam(th.BasicUser, thirdTeam)
+
+		// Give BasicUser permission to manage others' commands
+		th.AddPermissionToRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+		defer th.RemovePermissionFromRole(model.PermissionManageOthersSlashCommands.Id, model.TeamUserRoleId)
+
+		// Create a command owned by BasicUser2
+		// Note: BasicUser2 is NOT a member of thirdTeam (only member of team and newTeam)
+		cmd := &model.Command{
+			CreatorId: th.BasicUser2.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com",
+			Method:    model.CommandMethodPost,
+			Trigger:   "trigger7",
+		}
+		rcmd, _ := th.App.CreateCommand(cmd)
+
+		// BasicUser attempts to move BasicUser2's command to thirdTeam
+		// This should fail because BasicUser2 doesn't have permission to thirdTeam
+		resp, err := th.Client.MoveCommand(context.Background(), thirdTeam.Id, rcmd.Id)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+
+		// Verify the command was not moved
+		notMovedCmd, _ := th.App.GetCommand(rcmd.Id)
+		require.Equal(t, team.Id, notMovedCmd.TeamId)
+	})
+
+	t.Run("CannotMoveCommandToTeamWithDuplicateTrigger", func(t *testing.T) {
+		trigger := "move_duplicate_trigger"
+
+		sourceCmd := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    team.Id,
+			URL:       "http://nowhere.com/source",
+			Method:    model.CommandMethodPost,
+			Trigger:   trigger,
+		}
+		sourceCreatedCmd, appErr := th.App.CreateCommand(sourceCmd)
+		require.Nil(t, appErr)
+
+		targetCmd := &model.Command{
+			CreatorId: th.BasicUser.Id,
+			TeamId:    newTeam.Id,
+			URL:       "http://nowhere.com/target",
+			Method:    model.CommandMethodPost,
+			Trigger:   trigger,
+		}
+		_, appErr = th.App.CreateCommand(targetCmd)
+		require.Nil(t, appErr)
+
+		resp, err := th.Client.MoveCommand(context.Background(), newTeam.Id, sourceCreatedCmd.Id)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+		CheckErrorID(t, err, "api.command.duplicate_trigger.app_error")
+
+		notMovedCmd, _ := th.App.GetCommand(sourceCreatedCmd.Id)
+		require.Equal(t, team.Id, notMovedCmd.TeamId)
+	})
 }

 func TestDeleteCommand(t *testing.T) {
--- a/server/channels/api4/post_test.go
+++ b/server/channels/api4/post_test.go
@@ -275,6 +275,8 @@ func TestCreatePost(t *testing.T) {
 		require.Error(t, err)
 		CheckUnauthorizedStatus(t, resp)
 		assert.Nil(t, rpost)
+
+		th.LoginBasic()
 	})

 	t.Run("should prevent creating post with files when user lacks upload_file permission in target channel", func(t *testing.T) {
--- a/server/channels/app/command.go
+++ b/server/channels/app/command.go
@@ -671,22 +671,8 @@ func (a *App) CreateCommand(cmd *model.Command) (*model.Command, *model.AppError
 func (a *App) createCommand(cmd *model.Command) (*model.Command, *model.AppError) {
 	cmd.Trigger = strings.ToLower(cmd.Trigger)

-	teamCmds, err := a.Srv().Store().Command().GetByTeam(cmd.TeamId)
-	if err != nil {
-		return nil, model.NewAppError("CreateCommand", "app.command.createcommand.internal_error", nil, "", http.StatusInternalServerError).Wrap(err)
-	}
-
-	for _, existingCommand := range teamCmds {
-		if cmd.Trigger == existingCommand.Trigger {
-			return nil, model.NewAppError("CreateCommand", "api.command.duplicate_trigger.app_error", nil, "", http.StatusBadRequest)
-		}
-	}
-
-	for _, builtInProvider := range commandProviders {
-		builtInCommand := builtInProvider.GetCommand(a, i18n.T)
-		if builtInCommand != nil && cmd.Trigger == builtInCommand.Trigger {
-			return nil, model.NewAppError("CreateCommand", "api.command.duplicate_trigger.app_error", nil, "", http.StatusBadRequest)
-		}
+	if appErr := a.validateCommandTriggerUniqueness(cmd.TeamId, cmd.Trigger, ""); appErr != nil {
+		return nil, appErr
 	}

 	command, nErr := a.Srv().Store().Command().Save(cmd)
@@ -703,6 +689,30 @@ func (a *App) createCommand(cmd *model.Command) (*model.Command, *model.AppError
 	return command, nil
 }

+func (a *App) validateCommandTriggerUniqueness(teamID, trigger, excludeCommandID string) *model.AppError {
+	trigger = strings.ToLower(trigger)
+
+	for _, builtInProvider := range commandProviders {
+		builtInCommand := builtInProvider.GetCommand(a, i18n.T)
+		if builtInCommand != nil && trigger == strings.ToLower(builtInCommand.Trigger) {
+			return model.NewAppError("validateCommandTriggerUniqueness", "api.command.duplicate_trigger.app_error", nil, "", http.StatusBadRequest)
+		}
+	}
+
+	teamCmds, err := a.Srv().Store().Command().GetByTeam(teamID)
+	if err != nil {
+		return model.NewAppError("validateCommandTriggerUniqueness", "app.command.validatecommandtriggeruniqueness.internal_error", nil, "", http.StatusInternalServerError).Wrap(err)
+	}
+
+	for _, existingCommand := range teamCmds {
+		if existingCommand.Id != excludeCommandID && trigger == strings.ToLower(existingCommand.Trigger) {
+			return model.NewAppError("validateCommandTriggerUniqueness", "api.command.duplicate_trigger.app_error", nil, "", http.StatusBadRequest)
+		}
+	}
+
+	return nil
+}
+
 func (a *App) GetCommand(commandID string) (*model.Command, *model.AppError) {
 	if !*a.Config().ServiceSettings.EnableCommands {
 		return nil, model.NewAppError("GetCommand", "api.command.disabled.app_error", nil, "", http.StatusNotImplemented)
@@ -736,6 +746,10 @@ func (a *App) UpdateCommand(oldCmd, updatedCmd *model.Command) (*model.Command,
 	updatedCmd.PluginId = oldCmd.PluginId
 	updatedCmd.TeamId = oldCmd.TeamId

+	if appErr := a.validateCommandTriggerUniqueness(updatedCmd.TeamId, updatedCmd.Trigger, updatedCmd.Id); appErr != nil {
+		return nil, appErr
+	}
+
 	command, err := a.Srv().Store().Command().Update(updatedCmd)
 	if err != nil {
 		var nfErr *store.ErrNotFound
@@ -754,6 +768,10 @@ func (a *App) UpdateCommand(oldCmd, updatedCmd *model.Command) (*model.Command,
 }

 func (a *App) MoveCommand(team *model.Team, command *model.Command) *model.AppError {
+	if appErr := a.validateCommandTriggerUniqueness(team.Id, command.Trigger, command.Id); appErr != nil {
+		return appErr
+	}
+
 	command.TeamId = team.Id

 	_, err := a.Srv().Store().Command().Update(command)
--- a/server/channels/app/plugin_api.go
+++ b/server/channels/app/plugin_api.go
@@ -1305,6 +1305,10 @@ func (api *PluginAPI) UpdateCommand(commandID string, updatedCmd *model.Command)
 		updatedCmd.TeamId = oldCmd.TeamId
 	}

+	if appErr := api.app.validateCommandTriggerUniqueness(updatedCmd.TeamId, updatedCmd.Trigger, updatedCmd.Id); appErr != nil {
+		return nil, appErr
+	}
+
 	return api.app.Srv().Store().Command().Update(updatedCmd)
 }

--- a/server/channels/app/plugin_api_test.go
+++ b/server/channels/app/plugin_api_test.go
@@ -2247,6 +2247,23 @@ func TestPluginAPIUpdateCommand(t *testing.T) {
 	require.NoError(t, appErr)
 	require.Equal(t, "anothernewtriggeragain", newCmd4.Trigger)
 	require.Equal(t, team1.Id, newCmd4.TeamId)
+
+	// Updating a command's trigger to one that already exists should fail.
+	cmd2 := &model.Command{
+		TeamId:  team1.Id,
+		Trigger: "uniquetrigger",
+		Method:  "G",
+		URL:     "http://test.com/uniquetrigger",
+	}
+	cmd2, appErr = api.CreateCommand(cmd2)
+	require.NoError(t, appErr)
+
+	cmd2.Trigger = "anotherNewTriggerAgain"
+	_, appErr = api.UpdateCommand(cmd2.Id, cmd2)
+	require.Error(t, appErr)
+	var appError *model.AppError
+	require.ErrorAs(t, appErr, &appError)
+	require.Equal(t, "api.command.duplicate_trigger.app_error", appError.Id)
 }

 func TestPluginAPIIsEnterpriseReady(t *testing.T) {
--- a/server/channels/app/slashcommands/command_test.go
+++ b/server/channels/app/slashcommands/command_test.go
@@ -64,6 +64,21 @@ func TestMoveCommand(t *testing.T) {
 	retrievedCommand, err = th.App.GetCommand(command.Id)
 	assert.Nil(t, err)
 	assert.EqualValues(t, targetTeam.Id, retrievedCommand.TeamId)
+
+	// Move a command to a team where the trigger already exists should fail.
+	command2 := &model.Command{}
+	command2.CreatorId = model.NewId()
+	command2.Method = model.CommandMethodPost
+	command2.TeamId = sourceTeam.Id
+	command2.URL = "http://nowhere.com/"
+	command2.Trigger = "trigger1"
+
+	command2, err = th.App.CreateCommand(command2)
+	assert.Nil(t, err)
+
+	moveErr := th.App.MoveCommand(targetTeam, command2)
+	assert.NotNil(t, moveErr)
+	assert.Equal(t, "api.command.duplicate_trigger.app_error", moveErr.Id)
 }

 func TestCreateCommandPost(t *testing.T) {