From e67fe4c89d53d0eec312839e9d9b8fc16e5c7cd5 Mon Sep 17 00:00:00 2001 From: Michael Kochell Date: Wed, 9 Jan 2019 15:41:53 -0500 Subject: [PATCH] handle errors in HandleCommandResponse method (#9888) --- app/command.go | 40 ++++++++++++++----- app/command_test.go | 95 ++++++++++++++++++++++++++++++++++++--------- i18n/en.json | 4 ++ 3 files changed, 110 insertions(+), 29 deletions(-) diff --git a/app/command.go b/app/command.go index 0b43abd56b..d42ad2b596 100644 --- a/app/command.go +++ b/app/command.go @@ -41,13 +41,6 @@ func (a *App) CreateCommandPost(post *model.Post, teamId string, response *model post.Message = model.ParseSlackLinksToMarkdown(response.Text) post.CreateAt = model.GetMillis() - _, err := a.GetChannelMember(post.ChannelId, post.UserId) - if err != nil { - err = model.NewAppError("CreateCommandPost", "api.command.command_post.forbidden.app_error", nil, err.Error(), http.StatusForbidden) - mlog.Error(err.Error()) - return nil, err - } - if strings.HasPrefix(post.Type, model.POST_SYSTEM_MESSAGE_PREFIX) { err := model.NewAppError("CreateCommandPost", "api.context.invalid_param.app_error", map[string]interface{}{"Name": "post.type"}, "", http.StatusBadRequest) return nil, err @@ -297,14 +290,36 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, * } func (a *App) HandleCommandResponse(command *model.Command, args *model.CommandArgs, response *model.CommandResponse, builtIn bool) (*model.CommandResponse, *model.AppError) { - a.HandleCommandResponsePost(command, args, response, builtIn) + trigger := "" + if len(args.Command) != 0 { + parts := strings.Split(args.Command, " ") + trigger = parts[0][1:] + trigger = strings.ToLower(trigger) + } + + var lastError *model.AppError + _, err := a.HandleCommandResponsePost(command, args, response, builtIn) + + if err != nil { + mlog.Error(err.Error()) + lastError = err + } if response.ExtraResponses != nil { for _, resp := range response.ExtraResponses { - a.HandleCommandResponsePost(command, args, resp, builtIn) + _, err := a.HandleCommandResponsePost(command, args, resp, builtIn) + + if err != nil { + mlog.Error(err.Error()) + lastError = err + } } } + if lastError != nil { + return response, model.NewAppError("command", "api.command.execute_command.create_post_failed.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusInternalServerError) + } + return response, nil } @@ -318,6 +333,11 @@ func (a *App) HandleCommandResponsePost(command *model.Command, args *model.Comm post.Props = response.Props if len(response.ChannelId) != 0 { + _, err := a.GetChannelMember(response.ChannelId, args.UserId) + if err != nil { + err = model.NewAppError("HandleCommandResponsePost", "api.command.command_post.forbidden.app_error", nil, err.Error(), http.StatusForbidden) + return nil, err + } post.ChannelId = response.ChannelId } @@ -354,7 +374,7 @@ func (a *App) HandleCommandResponsePost(command *model.Command, args *model.Comm response.Attachments = a.ProcessSlackAttachments(response.Attachments) if _, err := a.CreateCommandPost(post, args.TeamId, response); err != nil { - mlog.Error(err.Error()) + return post, err } return post, nil diff --git a/app/command_test.go b/app/command_test.go index 8594477d69..098d2bca71 100644 --- a/app/command_test.go +++ b/app/command_test.go @@ -64,18 +64,6 @@ func TestCreateCommandPost(t *testing.T) { if err == nil || err.Id != "api.context.invalid_param.app_error" { t.Fatal("should have failed - bad post type") } - - channel := th.CreateChannel(th.BasicTeam) - - post = &model.Post{ - ChannelId: th.BasicChannel.Id, - UserId: channel.Id, - } - - _, err = th.App.CreateCommandPost(post, th.BasicTeam.Id, resp) - if err == nil || err.Id != "api.command.command_post.forbidden.app_error" { - t.Fatal("should have failed - forbidden channel post") - } } func TestHandleCommandResponsePost(t *testing.T) { @@ -85,16 +73,17 @@ func TestHandleCommandResponsePost(t *testing.T) { command := &model.Command{} args := &model.CommandArgs{ ChannelId: th.BasicChannel.Id, - TeamId: th.BasicTeam.Id, - UserId: th.BasicUser.Id, - RootId: "root_id", - ParentId: "parent_id", + TeamId: th.BasicTeam.Id, + UserId: th.BasicUser.Id, + RootId: "", + ParentId: "", } resp := &model.CommandResponse{ - Type: model.POST_EPHEMERAL, - Props: model.StringInterface{"some_key": "some value"}, - Text: "some message", + Type: model.POST_DEFAULT, + ResponseType: model.COMMAND_RESPONSE_TYPE_IN_CHANNEL, + Props: model.StringInterface{"some_key": "some value"}, + Text: "some message", } builtIn := true @@ -199,4 +188,72 @@ func TestHandleCommandResponsePost(t *testing.T) { post, err = th.App.HandleCommandResponsePost(command, args, resp, builtIn) assert.Nil(t, err) assert.Equal(t, "@here", resp.Attachments[0].Text) + + channel = th.CreatePrivateChannel(th.BasicTeam) + resp.ChannelId = channel.Id + args.UserId = th.BasicUser2.Id + post, err = th.App.HandleCommandResponsePost(command, args, resp, builtIn) + + if err == nil || err.Id != "api.command.command_post.forbidden.app_error" { + t.Fatal("should have failed - forbidden channel post") + } +} +func TestHandleCommandResponse(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + command := &model.Command{} + + args := &model.CommandArgs{ + Command: "/invite username", + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + } + + resp := &model.CommandResponse{ + Text: "message 1", + Type: model.POST_SYSTEM_GENERIC, + } + + builtIn := true + + _, err := th.App.HandleCommandResponse(command, args, resp, builtIn) + if err == nil || err.Id != "api.command.execute_command.create_post_failed.app_error" { + t.Fatal("should have failed - invalid post type") + } + + resp = &model.CommandResponse{ + Text: "message 1", + } + + _, err = th.App.HandleCommandResponse(command, args, resp, builtIn) + assert.Nil(t, err) + + resp = &model.CommandResponse{ + Text: "message 1", + ExtraResponses: []*model.CommandResponse{ + &model.CommandResponse{ + Text: "message 2", + }, + &model.CommandResponse{ + Type: model.POST_SYSTEM_GENERIC, + Text: "message 3", + }, + }, + } + + _, err = th.App.HandleCommandResponse(command, args, resp, builtIn) + if err == nil || err.Id != "api.command.execute_command.create_post_failed.app_error" { + t.Fatal("should have failed - invalid post type on extra response") + } + + resp = &model.CommandResponse{ + ExtraResponses: []*model.CommandResponse{ + &model.CommandResponse{}, + &model.CommandResponse{}, + }, + } + + _, err = th.App.HandleCommandResponse(command, args, resp, builtIn) + assert.Nil(t, err) } diff --git a/i18n/en.json b/i18n/en.json index faab739335..44df2164da 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -359,6 +359,10 @@ "id": "api.command.duplicate_trigger.app_error", "translation": "This trigger word is already in use. Please choose another word." }, + { + "id": "api.command.execute_command.create_post_failed.app_error", + "translation": "Command '{{.Trigger}}' failed to post response. Please contact your System Administrator." + }, { "id": "api.command.execute_command.debug", "translation": "Executing cmd=%v userId=%v"