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

@@ -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
+	tr, ok := <-teamChan
-	if tr.Err != nil {
+	if ok {
-		return "", tr.Err
+		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 != "" {

app/integration_action_test.go

@@ -68,250 +68,282 @@ func TestPostActionInvalidURL(t *testing.T) {
 }
 
 func TestPostAction(t *testing.T) {
-	th := Setup(t).InitBasic()
+	testCases := []struct {
-	defer th.TearDown()
+		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) {
+			return th.CreateDmChannel(user1)
-		*cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1"
+		}},
-	})
+		{"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) {
+			return th.CreateGroupChannel(user1, user2)
-		request := model.PostActionIntegrationRequestFromJson(r.Body)
+		}},
-		assert.NotNil(t, request)
+	}
 
-		assert.Equal(t, request.UserId, th.BasicUser.Id)
+	for _, testCase := range testCases {
-		assert.Equal(t, request.UserName, th.BasicUser.Username)
+		t.Run(testCase.Description, func(t *testing.T) {
-		assert.Equal(t, request.ChannelId, th.BasicChannel.Id)
+			th := Setup(t).InitBasic()
-		assert.Equal(t, request.ChannelName, th.BasicChannel.Name)
+			defer th.TearDown()
-		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{
+			channel := testCase.Channel(th)
-		Message:       "Interactive post",
+
-		ChannelId:     th.BasicChannel.Id,
+			th.App.UpdateConfig(func(cfg *model.Config) {
-		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
+				*cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1"
-		UserId:        th.BasicUser.Id,
+			})
-		Props: model.StringInterface{
+
-			"attachments": []*model.SlackAttachment{
+			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				{
+				request := model.PostActionIntegrationRequestFromJson(r.Body)
-					Text: "hello",
+				assert.NotNil(t, request)
-					Actions: []*model.PostAction{
+
+				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{
+							Text: "hello",
-								Context: model.StringInterface{
+							Actions: []*model.PostAction{
-									"s": "foo",
+								{
-									"n": 3,
+									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, "")
+			post, err := th.App.CreatePostAsUser(&interactivePost, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
 
-	attachments, ok := post.Props["attachments"].([]*model.SlackAttachment)
+			attachments, ok := post.Props["attachments"].([]*model.SlackAttachment)
-	require.True(t, ok)
+			require.True(t, ok)
 
-	require.NotEmpty(t, attachments[0].Actions)
+			require.NotEmpty(t, attachments[0].Actions)
-	require.NotEmpty(t, attachments[0].Actions[0].Id)
+			require.NotEmpty(t, attachments[0].Actions[0].Id)
 
-	menuPost := model.Post{
+			menuPost := model.Post{
-		Message:       "Interactive post",
+				Message:       "Interactive post",
-		ChannelId:     th.BasicChannel.Id,
+				ChannelId:     channel.Id,
-		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
+				PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
-		UserId:        th.BasicUser.Id,
+				UserId:        th.BasicUser.Id,
-		Props: model.StringInterface{
+				Props: model.StringInterface{
-			"attachments": []*model.SlackAttachment{
+					"attachments": []*model.SlackAttachment{
-				{
-					Text: "hello",
-					Actions: []*model.PostAction{
 						{
-							Integration: &model.PostActionIntegration{
+							Text: "hello",
-								Context: model.StringInterface{
+							Actions: []*model.PostAction{
-									"s": "foo",
+								{
-									"n": 3,
+									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, "")
+			post2, err := th.App.CreatePostAsUser(&menuPost, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
 
-	attachments2, ok := post2.Props["attachments"].([]*model.SlackAttachment)
+			attachments2, ok := post2.Props["attachments"].([]*model.SlackAttachment)
-	require.True(t, ok)
+			require.True(t, ok)
 
-	require.NotEmpty(t, attachments2[0].Actions)
+			require.NotEmpty(t, attachments2[0].Actions)
-	require.NotEmpty(t, attachments2[0].Actions[0].Id)
+			require.NotEmpty(t, attachments2[0].Actions[0].Id)
 
-	clientTriggerId, err := th.App.DoPostAction(post.Id, "notavalidid", th.BasicUser.Id, "")
+			clientTriggerId, err := th.App.DoPostAction(post.Id, "notavalidid", th.BasicUser.Id, "")
-	require.NotNil(t, err)
+			require.NotNil(t, err)
-	assert.Equal(t, http.StatusNotFound, err.StatusCode)
+			assert.Equal(t, http.StatusNotFound, err.StatusCode)
-	assert.True(t, clientTriggerId == "")
+			assert.True(t, clientTriggerId == "")
 
-	clientTriggerId, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "")
+			clientTriggerId, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
-	assert.True(t, len(clientTriggerId) == 26)
+			assert.True(t, len(clientTriggerId) == 26)
 
-	clientTriggerId, err = th.App.DoPostAction(post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected")
+			clientTriggerId, err = th.App.DoPostAction(post2.Id, attachments2[0].Actions[0].Id, th.BasicUser.Id, "selected")
-	require.Nil(t, err)
+			require.Nil(t, err)
-	assert.True(t, len(clientTriggerId) == 26)
+			assert.True(t, len(clientTriggerId) == 26)
 
-	th.App.UpdateConfig(func(cfg *model.Config) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
-		*cfg.ServiceSettings.AllowedUntrustedInternalConnections = ""
+				*cfg.ServiceSettings.AllowedUntrustedInternalConnections = ""
-	})
+			})
 
-	_, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "")
+			_, err = th.App.DoPostAction(post.Id, attachments[0].Actions[0].Id, th.BasicUser.Id, "")
-	require.NotNil(t, err)
+			require.NotNil(t, err)
-	require.True(t, strings.Contains(err.Error(), "address forbidden"))
+			require.True(t, strings.Contains(err.Error(), "address forbidden"))
 
-	interactivePostPlugin := model.Post{
+			interactivePostPlugin := model.Post{
-		Message:       "Interactive post",
+				Message:       "Interactive post",
-		ChannelId:     th.BasicChannel.Id,
+				ChannelId:     channel.Id,
-		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
+				PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
-		UserId:        th.BasicUser.Id,
+				UserId:        th.BasicUser.Id,
-		Props: model.StringInterface{
+				Props: model.StringInterface{
-			"attachments": []*model.SlackAttachment{
+					"attachments": []*model.SlackAttachment{
-				{
-					Text: "hello",
-					Actions: []*model.PostAction{
 						{
-							Integration: &model.PostActionIntegration{
+							Text: "hello",
-								Context: model.StringInterface{
+							Actions: []*model.PostAction{
-									"s": "foo",
+								{
-									"n": 3,
+									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, "")
+			postplugin, err := th.App.CreatePostAsUser(&interactivePostPlugin, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
 
-	attachmentsPlugin, ok := postplugin.Props["attachments"].([]*model.SlackAttachment)
+			attachmentsPlugin, ok := postplugin.Props["attachments"].([]*model.SlackAttachment)
-	require.True(t, ok)
+			require.True(t, ok)
 
-	_, err = th.App.DoPostAction(postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "")
+			_, err = th.App.DoPostAction(postplugin.Id, attachmentsPlugin[0].Actions[0].Id, th.BasicUser.Id, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
 
-	th.App.UpdateConfig(func(cfg *model.Config) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
-		*cfg.ServiceSettings.SiteURL = "http://127.1.1.1"
+				*cfg.ServiceSettings.SiteURL = "http://127.1.1.1"
-	})
+			})
 
-	interactivePostSiteURL := model.Post{
+			interactivePostSiteURL := model.Post{
-		Message:       "Interactive post",
+				Message:       "Interactive post",
-		ChannelId:     th.BasicChannel.Id,
+				ChannelId:     channel.Id,
-		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
+				PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
-		UserId:        th.BasicUser.Id,
+				UserId:        th.BasicUser.Id,
-		Props: model.StringInterface{
+				Props: model.StringInterface{
-			"attachments": []*model.SlackAttachment{
+					"attachments": []*model.SlackAttachment{
-				{
-					Text: "hello",
-					Actions: []*model.PostAction{
 						{
-							Integration: &model.PostActionIntegration{
+							Text: "hello",
-								Context: model.StringInterface{
+							Actions: []*model.PostAction{
-									"s": "foo",
+								{
-									"n": 3,
+									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, "")
+			postSiteURL, err := th.App.CreatePostAsUser(&interactivePostSiteURL, "")
-	require.Nil(t, err)
+			require.Nil(t, err)
 
-	attachmentsSiteURL, ok := postSiteURL.Props["attachments"].([]*model.SlackAttachment)
+			attachmentsSiteURL, ok := postSiteURL.Props["attachments"].([]*model.SlackAttachment)
-	require.True(t, ok)
+			require.True(t, ok)
 
-	_, err = th.App.DoPostAction(postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "")
+			_, err = th.App.DoPostAction(postSiteURL.Id, attachmentsSiteURL[0].Actions[0].Id, th.BasicUser.Id, "")
-	require.NotNil(t, err)
+			require.NotNil(t, err)
-	require.False(t, strings.Contains(err.Error(), "address forbidden"))
+			require.False(t, strings.Contains(err.Error(), "address forbidden"))
 
-	th.App.UpdateConfig(func(cfg *model.Config) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
-		*cfg.ServiceSettings.SiteURL = ts.URL + "/subpath"
+				*cfg.ServiceSettings.SiteURL = ts.URL + "/subpath"
-	})
+			})
 
-	interactivePostSubpath := model.Post{
+			interactivePostSubpath := model.Post{
-		Message:       "Interactive post",
+				Message:       "Interactive post",
-		ChannelId:     th.BasicChannel.Id,
+				ChannelId:     channel.Id,
-		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
+				PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
-		UserId:        th.BasicUser.Id,
+				UserId:        th.BasicUser.Id,
-		Props: model.StringInterface{
+				Props: model.StringInterface{
-			"attachments": []*model.SlackAttachment{
+					"attachments": []*model.SlackAttachment{
-				{
-					Text: "hello",
-					Actions: []*model.PostAction{
 						{
-							Integration: &model.PostActionIntegration{
+							Text: "hello",
-								Context: model.StringInterface{
+							Actions: []*model.PostAction{
-									"s": "foo",
+								{
-									"n": 3,
+									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{})
 		}