From c226cabc048a4e33e956346523e4605e85979d08 Mon Sep 17 00:00:00 2001 From: Thomas Balthazar Date: Tue, 31 May 2016 16:51:28 +0200 Subject: 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 --- api/post.go | 58 +++++----- api/post_test.go | 122 +++++++++++---------- model/outgoing_webhook.go | 47 ++++++++ model/outgoing_webhook_test.go | 43 ++++++++ store/sql_webhook_store.go | 2 + .../components/backstage/add_outgoing_webhook.jsx | 40 +++++++ .../backstage/installed_outgoing_webhook.jsx | 46 ++++---- webapp/sass/routes/_backstage.scss | 1 + 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 (
@@ -209,6 +220,35 @@ export default class AddOutgoingWebhook extends React.Component { />
+
+ +
+ +
+
- ); - urls.push( -
- ); - } + let urls = ( +
+ + + +
+ ); return (
@@ -138,6 +135,17 @@ export default class InstalledOutgoingWebhook extends React.Component {
{description} +
+ + + +
{triggerWords}
@@ -162,11 +170,7 @@ export default class InstalledOutgoingWebhook extends React.Component { />
-
- - {urls} - -
+ {urls}