diff --git a/server/channels/api4/command.go b/server/channels/api4/command.go index 8b4229e8f0..edc47d3355 100644 --- 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 diff --git a/server/channels/api4/command_test.go b/server/channels/api4/command_test.go index 6065a03a56..953a1671b4 100644 --- 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) { diff --git a/server/channels/api4/post_test.go b/server/channels/api4/post_test.go index 0989c5042f..b171db1b99 100644 --- 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) { diff --git a/server/channels/app/command.go b/server/channels/app/command.go index 675e996fc5..c857fe3a83 100644 --- 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) diff --git a/server/channels/app/plugin_api.go b/server/channels/app/plugin_api.go index f347d5f710..5a4d354950 100644 --- 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) } diff --git a/server/channels/app/plugin_api_test.go b/server/channels/app/plugin_api_test.go index 25bb36f1c4..1367558acf 100644 --- 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) { diff --git a/server/channels/app/slashcommands/command_test.go b/server/channels/app/slashcommands/command_test.go index c41f0256f5..2ece885f4f 100644 --- 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) { diff --git a/server/i18n/en.json b/server/i18n/en.json index 42f4fd0744..a773289a04 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -719,6 +719,10 @@ "id": "api.command.invite_people.sent", "translation": "Email invite(s) sent" }, + { + "id": "api.command.move_command.creator_not_in_team.app_error", + "translation": "Creator not in team" + }, { "id": "api.command.team_mismatch.app_error", "translation": "Unable to update commands across teams." @@ -5074,6 +5078,10 @@ "id": "app.command.updatecommand.internal_error", "translation": "Unable to update the command." }, + { + "id": "app.command.validatecommandtriggeruniqueness.internal_error", + "translation": "The specified trigger keyword already exists." + }, { "id": "app.command_webhook.create_command_webhook.existing", "translation": "You cannot update an existing CommandWebhook." @@ -8650,7 +8658,7 @@ }, { "id": "ent.saml.update_cpa.multiple_errors", - "translation": "Error(s) updating CPA values" + "translation": "Error(s) patching CPA value" }, { "id": "ent.user.complete_switch_with_oauth.blank_email.app_error", diff --git a/server/public/model/command.go b/server/public/model/command.go index d48fee8f33..096afd8554 100644 --- a/server/public/model/command.go +++ b/server/public/model/command.go @@ -5,6 +5,7 @@ package model import ( "net/http" + "regexp" "strings" ) @@ -15,6 +16,8 @@ const ( MaxTriggerLength = 128 ) +var validCommandTriggerChars = regexp.MustCompile(`^[A-Za-z0-9_./-]+$`) + type Command struct { Id string `json:"id"` Token string `json:"token"` @@ -96,7 +99,7 @@ func (o *Command) IsValid() *AppError { return NewAppError("Command.IsValid", "model.command.is_valid.team_id.app_error", nil, "", http.StatusBadRequest) } - if len(o.Trigger) < MinTriggerLength || len(o.Trigger) > MaxTriggerLength || strings.Index(o.Trigger, "/") == 0 || strings.Contains(o.Trigger, " ") { + if len(o.Trigger) < MinTriggerLength || len(o.Trigger) > MaxTriggerLength || strings.Index(o.Trigger, "/") == 0 || !validCommandTriggerChars.MatchString(o.Trigger) { return NewAppError("Command.IsValid", "model.command.is_valid.trigger.app_error", nil, "", http.StatusBadRequest) } diff --git a/server/public/model/command_test.go b/server/public/model/command_test.go index aa63ea6998..469145083b 100644 --- a/server/public/model/command_test.go +++ b/server/public/model/command_test.go @@ -72,6 +72,31 @@ func TestCommandIsValid(t *testing.T) { o.Trigger = strings.Repeat("1", 128) require.Nil(t, o.IsValid()) + validTriggers := []string{"abc", "ABC", "abc123", "a_b-c.d/e"} + for _, trigger := range validTriggers { + o.Trigger = trigger + require.Nil(t, o.IsValid(), "trigger should be valid: %q", trigger) + } + + invalidTriggers := []string{ + " trigger", + "tri gger", + "tri\tger", + "tri\nger", + "tri\rger", + "tri\x00ger", + "/trigger", + "tri?ger", + "tri*ger", + "trígger", + "trigger😀", + } + for _, trigger := range invalidTriggers { + o.Trigger = trigger + require.NotNil(t, o.IsValid(), "trigger should be invalid: %q", trigger) + } + + o.Trigger = "trigger" o.URL = "" require.NotNil(t, o.IsValid(), "should be invalid")