From bb1facb1f5aac6fe74723e364fcf746fabecba98 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 10 Dec 2019 12:57:54 -0400 Subject: [PATCH] MM-20755: fix post actions in DMs/GMs (#13248) * MM-20755: fix post actions in DMs/GMs The fix for https://github.com/mattermost/mattermost-server/issues/12377 assumed that all channels have teams, but this is false for DMs and GMs. Test for this and avoid failing on a missing team as such. Fixes: https://mattermost.atlassian.net/browse/MM-20755 * tweak code for clarity --- app/integration_action.go | 25 +- app/integration_action_test.go | 420 ++++++++++++++++++--------------- 2 files changed, 244 insertions(+), 201 deletions(-) diff --git a/app/integration_action.go b/app/integration_action.go index 603d906a47f..ffbddd8cd3f 100644 --- a/app/integration_action.go +++ b/app/integration_action.go @@ -21,13 +21,14 @@ import ( "bytes" "encoding/json" "fmt" - "github.com/gorilla/mux" "io/ioutil" "net/http" "net/url" "path" "strings" + "github.com/gorilla/mux" + "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" "github.com/mattermost/mattermost-server/v5/utils" @@ -158,10 +159,17 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st } teamChan := make(chan store.StoreResult, 1) + go func() { + defer close(teamChan) + + // Direct and group channels won't have teams. + if upstreamRequest.TeamId == "" { + return + } + team, err := a.Srv.Store.Team().Get(upstreamRequest.TeamId) teamChan <- store.StoreResult{Data: team, Err: err} - close(teamChan) }() ur := <-userChan @@ -171,12 +179,15 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st user := ur.Data.(*model.User) upstreamRequest.UserName = user.Username - tr := <-teamChan - if tr.Err != nil { - return "", tr.Err + tr, ok := <-teamChan + if ok { + if tr.Err != nil { + return "", tr.Err + } + + team := tr.Data.(*model.Team) + upstreamRequest.TeamName = team.Name } - team := tr.Data.(*model.Team) - upstreamRequest.TeamName = team.Name if upstreamRequest.Type == model.POST_ACTION_TYPE_SELECT { if selectedOption != "" { diff --git a/app/integration_action_test.go b/app/integration_action_test.go index aef7f9bed38..9c8feb7f666 100644 --- a/app/integration_action_test.go +++ b/app/integration_action_test.go @@ -68,250 +68,282 @@ func TestPostActionInvalidURL(t *testing.T) { } func TestPostAction(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + testCases := []struct { + Description string + Channel func(th *TestHelper) *model.Channel + }{ + {"public channel", func(th *TestHelper) *model.Channel { + return th.BasicChannel + }}, + {"direct channel", func(th *TestHelper) *model.Channel { + user1 := th.CreateUser() - th.App.UpdateConfig(func(cfg *model.Config) { - *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" - }) + return th.CreateDmChannel(user1) + }}, + {"group channel", func(th *TestHelper) *model.Channel { + user1 := th.CreateUser() + user2 := th.CreateUser() - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - request := model.PostActionIntegrationRequestFromJson(r.Body) - assert.NotNil(t, request) + return th.CreateGroupChannel(user1, user2) + }}, + } - assert.Equal(t, request.UserId, th.BasicUser.Id) - assert.Equal(t, request.UserName, th.BasicUser.Username) - assert.Equal(t, request.ChannelId, th.BasicChannel.Id) - assert.Equal(t, request.ChannelName, th.BasicChannel.Name) - assert.Equal(t, request.TeamId, th.BasicTeam.Id) - assert.Equal(t, request.TeamName, th.BasicTeam.Name) - assert.True(t, len(request.TriggerId) > 0) - if request.Type == model.POST_ACTION_TYPE_SELECT { - assert.Equal(t, request.DataSource, "some_source") - assert.Equal(t, request.Context["selected_option"], "selected") - } else { - assert.Equal(t, request.DataSource, "") - } - assert.Equal(t, "foo", request.Context["s"]) - assert.EqualValues(t, 3, request.Context["n"]) - fmt.Fprintf(w, `{"post": {"message": "updated"}, "ephemeral_text": "foo"}`) - })) - defer ts.Close() + for _, testCase := range testCases { + t.Run(testCase.Description, func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() - interactivePost := model.Post{ - Message: "Interactive post", - ChannelId: th.BasicChannel.Id, - PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), - UserId: th.BasicUser.Id, - Props: model.StringInterface{ - "attachments": []*model.SlackAttachment{ - { - Text: "hello", - Actions: []*model.PostAction{ + channel := testCase.Channel(th) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" + }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + request := model.PostActionIntegrationRequestFromJson(r.Body) + assert.NotNil(t, request) + + assert.Equal(t, request.UserId, th.BasicUser.Id) + assert.Equal(t, request.UserName, th.BasicUser.Username) + assert.Equal(t, request.ChannelId, channel.Id) + assert.Equal(t, request.ChannelName, channel.Name) + if channel.Type == model.CHANNEL_DIRECT || channel.Type == model.CHANNEL_GROUP { + assert.Empty(t, request.TeamId) + assert.Empty(t, request.TeamName) + } else { + assert.Equal(t, request.TeamId, th.BasicTeam.Id) + assert.Equal(t, request.TeamName, th.BasicTeam.Name) + } + assert.True(t, len(request.TriggerId) > 0) + if request.Type == model.POST_ACTION_TYPE_SELECT { + assert.Equal(t, request.DataSource, "some_source") + assert.Equal(t, request.Context["selected_option"], "selected") + } else { + assert.Equal(t, request.DataSource, "") + } + assert.Equal(t, "foo", request.Context["s"]) + assert.EqualValues(t, 3, request.Context["n"]) + fmt.Fprintf(w, `{"post": {"message": "updated"}, "ephemeral_text": "foo"}`) + })) + defer ts.Close() + + interactivePost := model.Post{ + Message: "Interactive post", + ChannelId: channel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + "attachments": []*model.SlackAttachment{ { - Integration: &model.PostActionIntegration{ - Context: model.StringInterface{ - "s": "foo", - "n": 3, + Text: "hello", + Actions: []*model.PostAction{ + { + Integration: &model.PostActionIntegration{ + Context: model.StringInterface{ + "s": "foo", + "n": 3, + }, + URL: ts.URL, + }, + Name: "action", + Type: "some_type", + DataSource: "some_source", }, - URL: ts.URL, }, - Name: "action", - Type: "some_type", - DataSource: "some_source", }, }, }, - }, - }, - } + } - post, err := th.App.CreatePostAsUser(&interactivePost, "") - require.Nil(t, err) + post, err := th.App.CreatePostAsUser(&interactivePost, "") + require.Nil(t, err) - attachments, ok := post.Props["attachments"].([]*model.SlackAttachment) - require.True(t, ok) + attachments, ok := post.Props["attachments"].([]*model.SlackAttachment) + require.True(t, ok) - require.NotEmpty(t, attachments[0].Actions) - require.NotEmpty(t, attachments[0].Actions[0].Id) + require.NotEmpty(t, attachments[0].Actions) + require.NotEmpty(t, attachments[0].Actions[0].Id) - menuPost := model.Post{ - Message: "Interactive post", - ChannelId: th.BasicChannel.Id, - PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), - UserId: th.BasicUser.Id, - Props: model.StringInterface{ - "attachments": []*model.SlackAttachment{ - { - Text: "hello", - Actions: []*model.PostAction{ + menuPost := model.Post{ + Message: "Interactive post", + ChannelId: channel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + "attachments": []*model.SlackAttachment{ { - Integration: &model.PostActionIntegration{ - Context: model.StringInterface{ - "s": "foo", - "n": 3, + Text: "hello", + Actions: []*model.PostAction{ + { + Integration: &model.PostActionIntegration{ + Context: model.StringInterface{ + "s": "foo", + "n": 3, + }, + URL: ts.URL, + }, + Name: "action", + Type: model.POST_ACTION_TYPE_SELECT, + DataSource: "some_source", }, - URL: ts.URL, }, - Name: "action", - Type: model.POST_ACTION_TYPE_SELECT, - DataSource: "some_source", }, }, }, - }, - }, - } + } - post2, err := th.App.CreatePostAsUser(&menuPost, "") - require.Nil(t, err) + post2, err := th.App.CreatePostAsUser(&menuPost, "") + require.Nil(t, err) - attachments2, ok := post2.Props["attachments"].([]*model.SlackAttachment) - require.True(t, ok) + attachments2, ok := post2.Props["attachments"].([]*model.SlackAttachment) + require.True(t, ok) - require.NotEmpty(t, attachments2[0].Actions) - require.NotEmpty(t, attachments2[0].Actions[0].Id) + require.NotEmpty(t, attachments2[0].Actions) + require.NotEmpty(t, attachments2[0].Actions[0].Id) - clientTriggerId, err := th.App.DoPostAction(post.Id, "notavalidid", th.BasicUser.Id, "") - require.NotNil(t, err) - assert.Equal(t, http.StatusNotFound, err.StatusCode) - assert.True(t, clientTriggerId == "") + clientTriggerId, err := th.App.DoPostAction(post.Id, "notavalidid", th.BasicUser.Id, "") + require.NotNil(t, err) + assert.Equal(t, http.StatusNotFound, err.StatusCode) + assert.True(t, clientTriggerId == "") - clientTriggerId, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "") - require.Nil(t, err) - assert.True(t, len(clientTriggerId) == 26) + clientTriggerId, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "") + require.Nil(t, err) + assert.True(t, len(clientTriggerId) == 26) - clientTriggerId, err = th.App.DoPostAction(post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected") - require.Nil(t, err) - assert.True(t, len(clientTriggerId) == 26) + clientTriggerId, err = th.App.DoPostAction(post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected") + require.Nil(t, err) + assert.True(t, len(clientTriggerId) == 26) - th.App.UpdateConfig(func(cfg *model.Config) { - *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "" - }) + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "" + }) - _, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "") - require.NotNil(t, err) - require.True(t, strings.Contains(err.Error(), "address forbidden")) + _, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "") + require.NotNil(t, err) + require.True(t, strings.Contains(err.Error(), "address forbidden")) - interactivePostPlugin := model.Post{ - Message: "Interactive post", - ChannelId: th.BasicChannel.Id, - PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), - UserId: th.BasicUser.Id, - Props: model.StringInterface{ - "attachments": []*model.SlackAttachment{ - { - Text: "hello", - Actions: []*model.PostAction{ + interactivePostPlugin := model.Post{ + Message: "Interactive post", + ChannelId: channel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + "attachments": []*model.SlackAttachment{ { - Integration: &model.PostActionIntegration{ - Context: model.StringInterface{ - "s": "foo", - "n": 3, + Text: "hello", + Actions: []*model.PostAction{ + { + Integration: &model.PostActionIntegration{ + Context: model.StringInterface{ + "s": "foo", + "n": 3, + }, + URL: ts.URL + "/plugins/myplugin/myaction", + }, + Name: "action", + Type: "some_type", + DataSource: "some_source", }, - URL: ts.URL + "/plugins/myplugin/myaction", }, - Name: "action", - Type: "some_type", - DataSource: "some_source", }, }, }, - }, - }, - } + } - postplugin, err := th.App.CreatePostAsUser(&interactivePostPlugin, "") - require.Nil(t, err) + postplugin, err := th.App.CreatePostAsUser(&interactivePostPlugin, "") + require.Nil(t, err) - attachmentsPlugin, ok := postplugin.Props["attachments"].([]*model.SlackAttachment) - require.True(t, ok) + attachmentsPlugin, ok := postplugin.Props["attachments"].([]*model.SlackAttachment) + require.True(t, ok) - _, err = th.App.DoPostAction(postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "") - require.Nil(t, err) + _, err = th.App.DoPostAction(postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "") + require.Nil(t, err) - th.App.UpdateConfig(func(cfg *model.Config) { - *cfg.ServiceSettings.SiteURL = "http://127.1.1.1" - }) + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = "http://127.1.1.1" + }) - interactivePostSiteURL := model.Post{ - Message: "Interactive post", - ChannelId: th.BasicChannel.Id, - PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), - UserId: th.BasicUser.Id, - Props: model.StringInterface{ - "attachments": []*model.SlackAttachment{ - { - Text: "hello", - Actions: []*model.PostAction{ + interactivePostSiteURL := model.Post{ + Message: "Interactive post", + ChannelId: channel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + "attachments": []*model.SlackAttachment{ { - Integration: &model.PostActionIntegration{ - Context: model.StringInterface{ - "s": "foo", - "n": 3, + Text: "hello", + Actions: []*model.PostAction{ + { + Integration: &model.PostActionIntegration{ + Context: model.StringInterface{ + "s": "foo", + "n": 3, + }, + URL: "http://127.1.1.1/plugins/myplugin/myaction", + }, + Name: "action", + Type: "some_type", + DataSource: "some_source", }, - URL: "http://127.1.1.1/plugins/myplugin/myaction", }, - Name: "action", - Type: "some_type", - DataSource: "some_source", }, }, }, - }, - }, - } + } - postSiteURL, err := th.App.CreatePostAsUser(&interactivePostSiteURL, "") - require.Nil(t, err) + postSiteURL, err := th.App.CreatePostAsUser(&interactivePostSiteURL, "") + require.Nil(t, err) - attachmentsSiteURL, ok := postSiteURL.Props["attachments"].([]*model.SlackAttachment) - require.True(t, ok) + attachmentsSiteURL, ok := postSiteURL.Props["attachments"].([]*model.SlackAttachment) + require.True(t, ok) - _, err = th.App.DoPostAction(postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "") - require.NotNil(t, err) - require.False(t, strings.Contains(err.Error(), "address forbidden")) + _, err = th.App.DoPostAction(postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "") + require.NotNil(t, err) + require.False(t, strings.Contains(err.Error(), "address forbidden")) - th.App.UpdateConfig(func(cfg *model.Config) { - *cfg.ServiceSettings.SiteURL = ts.URL + "/subpath" - }) + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = ts.URL + "/subpath" + }) - interactivePostSubpath := model.Post{ - Message: "Interactive post", - ChannelId: th.BasicChannel.Id, - PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), - UserId: th.BasicUser.Id, - Props: model.StringInterface{ - "attachments": []*model.SlackAttachment{ - { - Text: "hello", - Actions: []*model.PostAction{ + interactivePostSubpath := model.Post{ + Message: "Interactive post", + ChannelId: channel.Id, + PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()), + UserId: th.BasicUser.Id, + Props: model.StringInterface{ + "attachments": []*model.SlackAttachment{ { - Integration: &model.PostActionIntegration{ - Context: model.StringInterface{ - "s": "foo", - "n": 3, + Text: "hello", + Actions: []*model.PostAction{ + { + Integration: &model.PostActionIntegration{ + Context: model.StringInterface{ + "s": "foo", + "n": 3, + }, + URL: ts.URL + "/subpath/plugins/myplugin/myaction", + }, + Name: "action", + Type: "some_type", + DataSource: "some_source", }, - URL: ts.URL + "/subpath/plugins/myplugin/myaction", }, - Name: "action", - Type: "some_type", - DataSource: "some_source", }, }, }, - }, - }, + } + + postSubpath, err := th.App.CreatePostAsUser(&interactivePostSubpath, "") + require.Nil(t, err) + + attachmentsSubpath, ok := postSubpath.Props["attachments"].([]*model.SlackAttachment) + require.True(t, ok) + + _, err = th.App.DoPostAction(postSubpath.Id, attachmentsSubpath[0].Actions[0].Id, th.BasicUser.Id, "") + require.Nil(t, err) + + }) } - - postSubpath, err := th.App.CreatePostAsUser(&interactivePostSubpath, "") - require.Nil(t, err) - - attachmentsSubpath, ok := postSubpath.Props["attachments"].([]*model.SlackAttachment) - require.True(t, ok) - - _, err = th.App.DoPostAction(postSubpath.Id, attachmentsSubpath[0].Actions[0].Id, th.BasicUser.Id, "") - require.Nil(t, err) } func TestPostActionProps(t *testing.T) { @@ -446,17 +478,17 @@ func TestSubmitInteractiveDialog(t *testing.T) { setupPluginApiTest(t, ` package main - + import ( "net/http" "github.com/mattermost/mattermost-server/v5/plugin" "github.com/mattermost/mattermost-server/v5/model" ) - + type MyPlugin struct { plugin.MattermostPlugin } - + func (p *MyPlugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) { response := &model.SubmitDialogResponse{ Errors: map[string]string{"name1": "some error"}, @@ -464,7 +496,7 @@ func TestSubmitInteractiveDialog(t *testing.T) { w.WriteHeader(http.StatusOK) _, _ = w.Write(response.ToJson()) } - + func main() { plugin.ClientMain(&MyPlugin{}) } @@ -724,23 +756,23 @@ func TestPostActionRelativePluginURL(t *testing.T) { setupPluginApiTest(t, ` package main - + import ( "net/http" "github.com/mattermost/mattermost-server/v5/plugin" "github.com/mattermost/mattermost-server/v5/model" ) - + type MyPlugin struct { plugin.MattermostPlugin } - + func (p *MyPlugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) { response := &model.PostActionIntegrationResponse{} w.WriteHeader(http.StatusOK) _, _ = w.Write(response.ToJson()) } - + func main() { plugin.ClientMain(&MyPlugin{}) }