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 +++++++++++++++++++++++++++++-------------------------- 2 files changed, 92 insertions(+), 88 deletions(-) (limited to 'api') 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 -- cgit v1.2.3-1-g7c22