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

8 files changed, 250 insertions, 109 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
diff --git a/model/outgoing_webhook.go b/model/outgoing_webhook.go
index ef1807e7a..ee7a32f1f 100644
--- a/model/outgoing_webhook.go
+++ b/model/outgoing_webhook.go
@@ -7,6 +7,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"net/url"
+	"strconv"
 )
 
 type OutgoingWebhook struct {
@@ -22,6 +24,47 @@ type OutgoingWebhook struct {
 	CallbackURLs StringArray `json:"callback_urls"`
 	DisplayName  string      `json:"display_name"`
 	Description  string      `json:"description"`
+	ContentType  string      `json:"content_type"`
+}
+
+type OutgoingWebhookPayload struct {
+	Token       string `json:"token"`
+	TeamId      string `json:"team_id"`
+	TeamDomain  string `json:"team_domain"`
+	ChannelId   string `json:"channel_id"`
+	ChannelName string `json:"channel_name"`
+	Timestamp   int64  `json:"timestamp"`
+	UserId      string `json:"user_id"`
+	UserName    string `json:"user_name"`
+	PostId      string `json:"post_id"`
+	Text        string `json:"text"`
+	TriggerWord string `json:"trigger_word"`
+}
+
+func (o *OutgoingWebhookPayload) ToJSON() string {
+	b, err := json.Marshal(o)
+	if err != nil {
+		return ""
+	} else {
+		return string(b)
+	}
+}
+
+func (o *OutgoingWebhookPayload) ToFormValues() string {
+	v := url.Values{}
+	v.Set("token", o.Token)
+	v.Set("team_id", o.TeamId)
+	v.Set("team_domain", o.TeamDomain)
+	v.Set("channel_id", o.ChannelId)
+	v.Set("channel_name", o.ChannelName)
+	v.Set("timestamp", strconv.FormatInt(o.Timestamp/1000, 10))
+	v.Set("user_id", o.UserId)
+	v.Set("user_name", o.UserName)
+	v.Set("post_id", o.PostId)
+	v.Set("text", o.Text)
+	v.Set("trigger_word", o.TriggerWord)
+
+	return v.Encode()
 }
 
 func (o *OutgoingWebhook) ToJson() string {
@@ -124,6 +167,10 @@ func (o *OutgoingWebhook) IsValid() *AppError {
 		return NewLocAppError("OutgoingWebhook.IsValid", "model.outgoing_hook.is_valid.description.app_error", nil, "")
 	}
 
+	if len(o.ContentType) > 128 {
+		return NewLocAppError("OutgoingWebhook.IsValid", "model.outgoing_hook.is_valid.content_type.app_error", nil, "")
+	}
+
 	return nil
 }
 
diff --git a/model/outgoing_webhook_test.go b/model/outgoing_webhook_test.go
index 72c842e03..24b81d221 100644
--- a/model/outgoing_webhook_test.go
+++ b/model/outgoing_webhook_test.go
@@ -4,6 +4,8 @@
 package model
 
 import (
+	"net/url"
+	"reflect"
 	"strings"
 	"testing"
 )
@@ -107,8 +109,49 @@ func TestOutgoingWebhookIsValid(t *testing.T) {
 
 	o.Description = strings.Repeat("1", 128)
 	if err := o.IsValid(); err != nil {
+		t.Fatal("should be invalid")
+	}
+
+	o.ContentType = strings.Repeat("1", 129)
+	if err := o.IsValid(); err == nil {
 		t.Fatal(err)
 	}
+
+	o.ContentType = strings.Repeat("1", 128)
+	if err := o.IsValid(); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestOutgoingWebhookPayloadToFormValues(t *testing.T) {
+	p := &OutgoingWebhookPayload{
+		Token:       "Token",
+		TeamId:      "TeamId",
+		TeamDomain:  "TeamDomain",
+		ChannelId:   "ChannelId",
+		ChannelName: "ChannelName",
+		Timestamp:   123000,
+		UserId:      "UserId",
+		UserName:    "UserName",
+		PostId:      "PostId",
+		Text:        "Text",
+		TriggerWord: "TriggerWord",
+	}
+	v := url.Values{}
+	v.Set("token", "Token")
+	v.Set("team_id", "TeamId")
+	v.Set("team_domain", "TeamDomain")
+	v.Set("channel_id", "ChannelId")
+	v.Set("channel_name", "ChannelName")
+	v.Set("timestamp", "123")
+	v.Set("user_id", "UserId")
+	v.Set("user_name", "UserName")
+	v.Set("post_id", "PostId")
+	v.Set("text", "Text")
+	v.Set("trigger_word", "TriggerWord")
+	if got, want := p.ToFormValues(), v.Encode(); !reflect.DeepEqual(got, want) {
+		t.Fatalf("Got %+v, wanted %+v", got, want)
+	}
 }
 
 func TestOutgoingWebhookPreSave(t *testing.T) {
diff --git a/store/sql_webhook_store.go b/store/sql_webhook_store.go
index 3c1f404e1..72897771d 100644
--- a/store/sql_webhook_store.go
+++ b/store/sql_webhook_store.go
@@ -33,6 +33,7 @@ func NewSqlWebhookStore(sqlStore *SqlStore) WebhookStore {
 		tableo.ColMap("CallbackURLs").SetMaxSize(1024)
 		tableo.ColMap("DisplayName").SetMaxSize(64)
 		tableo.ColMap("Description").SetMaxSize(128)
+		tableo.ColMap("ContentType").SetMaxSize(128)
 	}
 
 	return s
@@ -44,6 +45,7 @@ func (s SqlWebhookStore) UpgradeSchemaIfNeeded() {
 
 	s.CreateColumnIfNotExists("OutgoingWebhooks", "DisplayName", "varchar(64)", "varchar(64)", "")
 	s.CreateColumnIfNotExists("OutgoingWebhooks", "Description", "varchar(128)", "varchar(128)", "")
+	s.CreateColumnIfNotExists("OutgoingWebhooks", "ContentType", "varchar(128)", "varchar(128)", "")
 }
 
 func (s SqlWebhookStore) CreateIndexesIfNotExists() {
diff --git a/webapp/components/backstage/add_outgoing_webhook.jsx b/webapp/components/backstage/add_outgoing_webhook.jsx
index 2fefd5965..d48be3ac4 100644
--- a/webapp/components/backstage/add_outgoing_webhook.jsx
+++ b/webapp/components/backstage/add_outgoing_webhook.jsx
@@ -21,6 +21,7 @@ export default class AddOutgoingWebhook extends React.Component {
 
         this.updateDisplayName = this.updateDisplayName.bind(this);
         this.updateDescription = this.updateDescription.bind(this);
+        this.updateContentType = this.updateContentType.bind(this);
         this.updateChannelId = this.updateChannelId.bind(this);
         this.updateTriggerWords = this.updateTriggerWords.bind(this);
         this.updateCallbackUrls = this.updateCallbackUrls.bind(this);
@@ -28,6 +29,7 @@ export default class AddOutgoingWebhook extends React.Component {
         this.state = {
             displayName: '',
             description: '',
+            contentType: 'application/x-www-form-urlencoded',
             channelId: '',
             triggerWords: '',
             callbackUrls: '',
@@ -103,6 +105,7 @@ export default class AddOutgoingWebhook extends React.Component {
             trigger_words: triggerWords,
             callback_urls: callbackUrls,
             display_name: this.state.displayName,
+            content_type: this.state.contentType,
             description: this.state.description
         };
 
@@ -132,6 +135,12 @@ export default class AddOutgoingWebhook extends React.Component {
         });
     }
 
+    updateContentType(e) {
+        this.setState({
+            contentType: e.target.value
+        });
+    }
+
     updateChannelId(e) {
         this.setState({
             channelId: e.target.value
@@ -151,6 +160,8 @@ export default class AddOutgoingWebhook extends React.Component {
     }
 
     render() {
+        const contentTypeOption1 = 'application/x-www-form-urlencoded';
+        const contentTypeOption2 = 'application/json';
         return (
             <div className='backstage-content'>
                 <BackstageHeader>
@@ -212,6 +223,35 @@ export default class AddOutgoingWebhook extends React.Component {
                         <div className='form-group'>
                             <label
                                 className='control-label col-sm-4'
+                                htmlFor='contentType'
+                            >
+                                <FormattedMessage
+                                    id='add_outgoing_webhook.content_Type'
+                                    defaultMessage='Content Type'
+                                />
+                            </label>
+                            <div className='col-md-5 col-sm-8'>
+                                <select
+                                    className='form-control'
+                                    value={this.state.contentType}
+                                    onChange={this.updateContentType}
+                                >
+                                    <option
+                                        value={contentTypeOption1}
+                                    >
+                                        {contentTypeOption1}
+                                    </option>
+                                    <option
+                                        value={contentTypeOption2}
+                                    >
+                                        {contentTypeOption2}
+                                    </option>
+                                </select>
+                            </div>
+                        </div>
+                        <div className='form-group'>
+                            <label
+                                className='control-label col-sm-4'
                                 htmlFor='channelId'
                             >
                                 <FormattedMessage
diff --git a/webapp/components/backstage/installed_outgoing_webhook.jsx b/webapp/components/backstage/installed_outgoing_webhook.jsx
index 16782b4df..99f2439ec 100644
--- a/webapp/components/backstage/installed_outgoing_webhook.jsx
+++ b/webapp/components/backstage/installed_outgoing_webhook.jsx
@@ -112,22 +112,19 @@ export default class InstalledOutgoingWebhook extends React.Component {
             );
         }
 
-        const urls = [];
-        for (const url of outgoingWebhook.callback_urls) {
-            urls.push(
-                <div
-                    key={url}
-                    className='item-details__url'
-                >
-                    {url}
-                </div>
-            );
-            urls.push(
-                <br
-                    key={'BR' + url}
-                />
-            );
-        }
+        let urls = (
+            <div className='item-details__row'>
+                <span className='item-details__url'>
+                    <FormattedMessage
+                        id='installed_integrations.callback_urls'
+                        defaultMessage='Callback URLs: {urls}'
+                        values={{
+                            urls: outgoingWebhook.callback_urls.join(', ')
+                        }}
+                    />
+                </span>
+            </div>
+        );
 
         return (
             <div className='backstage-list__item'>
@@ -138,6 +135,17 @@ export default class InstalledOutgoingWebhook extends React.Component {
                         </span>
                     </div>
                     {description}
+                    <div className='item-details__row'>
+                        <span className='item-details__content_type'>
+                            <FormattedMessage
+                                id='installed_integrations.content_type'
+                                defaultMessage='Content-Type: {contentType}'
+                                values={{
+                                    contentType: outgoingWebhook.content_type || 'application/x-www-form-urlencoded'
+                                }}
+                            />
+                        </span>
+                    </div>
                     {triggerWords}
                     <div className='item-details__row'>
                         <span className='item-details__token'>
@@ -162,11 +170,7 @@ export default class InstalledOutgoingWebhook extends React.Component {
                             />
                         </span>
                     </div>
-                    <div className='item-details__row'>
-                        <span className='item-details__urls'>
-                            {urls}
-                        </span>
-                    </div>
+                    {urls}
                 </div>
                 <div className='item-actions'>
                     <a
diff --git a/webapp/sass/routes/_backstage.scss b/webapp/sass/routes/_backstage.scss
index 716d07a4c..f9754f16d 100644
--- a/webapp/sass/routes/_backstage.scss
+++ b/webapp/sass/routes/_backstage.scss
@@ -220,6 +220,7 @@ body {
     }
 
     .item-details__description,
+    .item-details__content_type,
     .item-details__token,
     .item-details__trigger-words,
     .item-details__url,