PLT-2170 Send payload in application/json for outgoing webhooks (#3160)

* Send payload in application/json for outgoing webhooks

The Add outgoing webhook UI now has a 'Content-Type' field that allows
to choose between application/x-www-form-urlencoded and
application/json. All outgoing webhooks created before this change will
be considered as x-www-form-urlencoded. There's also a minor change in
the way the outgoing webhook summary is displayed: the 'Callback URLs'
label was missing.

* Fix JS formatting errors

* Increase ContentType field length to 128

2 files changed, 92 insertions, 88 deletions

diff --git a/api/post.go b/api/post.go
index d5d3564f2..875f30ba5 100644
--- a/api/post.go
+++ b/api/post.go
@@ -7,6 +7,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"html/template"
+	"io"
 	"net/http"
 	"net/url"
 	"path/filepath"
@@ -362,30 +363,24 @@ func handleWebhookEvents(c *Context, post *model.Post, team *model.Team, channel
 	}
 
 	hchan := Srv.Store.Webhook().GetOutgoingByTeam(c.TeamId)
-
-	hooks := []*model.OutgoingWebhook{}
-
-	if result := <-hchan; result.Err != nil {
+	result := <-hchan
+	if result.Err != nil {
 		l4g.Error(utils.T("api.post.handle_webhook_events_and_forget.getting.error"), result.Err)
 		return
-	} else {
-		hooks = result.Data.([]*model.OutgoingWebhook)
 	}
 
+	hooks := result.Data.([]*model.OutgoingWebhook)
 	if len(hooks) == 0 {
 		return
 	}
 
 	splitWords := strings.Fields(post.Message)
-
 	if len(splitWords) == 0 {
 		return
 	}
-
 	firstWord := splitWords[0]
 
 	relevantHooks := []*model.OutgoingWebhook{}
-
 	for _, hook := range hooks {
 		if hook.ChannelId == post.ChannelId {
 			if len(hook.TriggerWords) == 0 || hook.HasTriggerWord(firstWord) {
@@ -398,25 +393,28 @@ func handleWebhookEvents(c *Context, post *model.Post, team *model.Team, channel
 
 	for _, hook := range relevantHooks {
 		go func(hook *model.OutgoingWebhook) {
-			p := url.Values{}
-			p.Set("token", hook.Token)
-
-			p.Set("team_id", hook.TeamId)
-			p.Set("team_domain", team.Name)
-
-			p.Set("channel_id", post.ChannelId)
-			p.Set("channel_name", channel.Name)
-
-			p.Set("timestamp", strconv.FormatInt(post.CreateAt/1000, 10))
-
-			p.Set("user_id", post.UserId)
-			p.Set("user_name", user.Username)
-
-			p.Set("post_id", post.Id)
-
-			p.Set("text", post.Message)
-			p.Set("trigger_word", firstWord)
-
+			payload := &model.OutgoingWebhookPayload{
+				Token:       hook.Token,
+				TeamId:      hook.TeamId,
+				TeamDomain:  team.Name,
+				ChannelId:   post.ChannelId,
+				ChannelName: channel.Name,
+				Timestamp:   post.CreateAt,
+				UserId:      post.UserId,
+				UserName:    user.Username,
+				PostId:      post.Id,
+				Text:        post.Message,
+				TriggerWord: firstWord,
+			}
+			var body io.Reader
+			var contentType string
+			if hook.ContentType == "application/json" {
+				body = strings.NewReader(payload.ToJSON())
+				contentType = "application/json"
+			} else {
+				body = strings.NewReader(payload.ToFormValues())
+				contentType = "application/x-www-form-urlencoded"
+			}
 			tr := &http.Transport{
 				TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
 			}
@@ -424,8 +422,8 @@ func handleWebhookEvents(c *Context, post *model.Post, team *model.Team, channel
 
 			for _, url := range hook.CallbackURLs {
 				go func(url string) {
-					req, _ := http.NewRequest("POST", url, strings.NewReader(p.Encode()))
-					req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+					req, _ := http.NewRequest("POST", url, body)
+					req.Header.Set("Content-Type", contentType)
 					req.Header.Set("Accept", "application/json")
 					if resp, err := client.Do(req); err != nil {
 						l4g.Error(utils.T("api.post.handle_webhook_events_and_forget.event_post.error"), err.Error())
diff --git a/api/post_test.go b/api/post_test.go
index b23511384..8a07cdb59 100644
--- a/api/post_test.go
+++ b/api/post_test.go
@@ -4,9 +4,11 @@
 package api
 
 import (
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
-	"strconv"
+	"net/url"
+	"reflect"
 	"strings"
 
 	"fmt"
@@ -110,7 +112,11 @@ func TestCreatePost(t *testing.T) {
 	}
 }
 
-func TestCreatePostWithOutgoingHook(t *testing.T) {
+func testCreatePostWithOutgoingHook(
+	t *testing.T,
+	hookContentType string,
+	expectedContentType string,
+) {
 	th := Setup().InitSystemAdmin()
 	Client := th.SystemAdminClient
 	team := th.SystemAdminTeam
@@ -131,67 +137,52 @@ func TestCreatePostWithOutgoingHook(t *testing.T) {
 	// success/failure.
 	success := make(chan bool)
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		err := r.ParseForm()
-		if err != nil {
-			t.Logf("Error parsing form: %q", err)
+		requestContentType := r.Header.Get("Content-Type")
+		if requestContentType != expectedContentType {
+			t.Logf("Content-Type is %s, should be %s", requestContentType, expectedContentType)
 			success <- false
 			return
 		}
 
-		if got, want := r.Form.Get("token"), hook.Token; got != want {
-			t.Logf("Token is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("team_id"), hook.TeamId; got != want {
-			t.Logf("TeamId is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("team_domain"), team.Name; got != want {
-			t.Logf("TeamDomain is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("channel_id"), post.ChannelId; got != want {
-			t.Logf("ChannelId is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("channel_name"), channel.Name; got != want {
-			t.Logf("ChannelName is %s, should be %s", got, want)
-			success <- false
-			return
+		expectedPayload := &model.OutgoingWebhookPayload{
+			Token:       hook.Token,
+			TeamId:      hook.TeamId,
+			TeamDomain:  team.Name,
+			ChannelId:   post.ChannelId,
+			ChannelName: channel.Name,
+			Timestamp:   post.CreateAt,
+			UserId:      post.UserId,
+			UserName:    user.Username,
+			PostId:      post.Id,
+			Text:        post.Message,
+			TriggerWord: strings.Fields(post.Message)[0],
 		}
-		if got, want := r.Form.Get("timestamp"), strconv.FormatInt(post.CreateAt/1000, 10); got != want {
-			t.Logf("Timestamp is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("user_id"), post.UserId; got != want {
-			t.Logf("UserId is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("user_name"), user.Username; got != want {
-			t.Logf("Username is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("post_id"), post.Id; got != want {
-			t.Logf("PostId is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("text"), post.Message; got != want {
-			t.Logf("Message is %s, should be %s", got, want)
-			success <- false
-			return
-		}
-		if got, want := r.Form.Get("trigger_word"), strings.Fields(post.Message)[0]; got != want {
-			t.Logf("TriggerWord is %s, should be %s", got, want)
-			success <- false
-			return
+
+		// depending on the Content-Type, we expect to find a JSON or form encoded payload
+		if requestContentType == "application/json" {
+			decoder := json.NewDecoder(r.Body)
+			o := &model.OutgoingWebhookPayload{}
+			decoder.Decode(&o)
+
+			if !reflect.DeepEqual(expectedPayload, o) {
+				t.Logf("JSON payload is %+v, should be %+v", o, expectedPayload)
+				success <- false
+				return
+			}
+		} else {
+			err := r.ParseForm()
+			if err != nil {
+				t.Logf("Error parsing form: %q", err)
+				success <- false
+				return
+			}
+
+			expectedFormValues, _ := url.ParseQuery(expectedPayload.ToFormValues())
+			if !reflect.DeepEqual(expectedFormValues, r.Form) {
+				t.Logf("Form values are %q, should be %q", r.Form, expectedFormValues)
+				success <- false
+				return
+			}
 		}
 
 		success <- true
@@ -202,6 +193,7 @@ func TestCreatePostWithOutgoingHook(t *testing.T) {
 	triggerWord := "bingo"
 	hook = &model.OutgoingWebhook{
 		ChannelId:    channel.Id,
+		ContentType:  hookContentType,
 		TriggerWords: []string{triggerWord},
 		CallbackURLs: []string{ts.URL},
 	}
@@ -237,6 +229,20 @@ func TestCreatePostWithOutgoingHook(t *testing.T) {
 	}
 }
 
+func TestCreatePostWithOutgoingHook_form_urlencoded(t *testing.T) {
+	testCreatePostWithOutgoingHook(t, "application/x-www-form-urlencoded", "application/x-www-form-urlencoded")
+}
+
+func TestCreatePostWithOutgoingHook_json(t *testing.T) {
+	testCreatePostWithOutgoingHook(t, "application/json", "application/json")
+}
+
+// hooks created before we added the ContentType field should be considered as
+// application/x-www-form-urlencoded
+func TestCreatePostWithOutgoingHook_no_content_type(t *testing.T) {
+	testCreatePostWithOutgoingHook(t, "", "application/x-www-form-urlencoded")
+}
+
 func TestUpdatePost(t *testing.T) {
 	th := Setup().InitBasic()
 	Client := th.BasicClient