From f2a3ff1e2ee576ee4e652fcb34601087aa233abf Mon Sep 17 00:00:00 2001 From: Hanzei <16541325+hanzei@users.noreply.github.com> Date: Tue, 15 Jan 2019 14:07:43 +0100 Subject: [PATCH] Improve error handling and logging (#16) * Improve error handling This commits improves the error handling by not longer returning 200 for invalid JSON requests. It also rewrites the code to use idiomatic error handling. * Add logging of http requests See #17 * Require teamName * Improve debug message * Check attachment * Post debug messages for failed webhook --- server/plugin.go | 101 +++++++++++++++++++++++++++++++++--------- server/plugin_test.go | 10 ++++- server/webhook.go | 10 ++++- 3 files changed, 98 insertions(+), 23 deletions(-) diff --git a/server/plugin.go b/server/plugin.go index 094999a31..3c75e543a 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -25,45 +25,104 @@ type Plugin struct { } func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) { + p.API.LogDebug("HTTP request", "Host", r.Host, "RequestURI", r.RequestURI, "Method", r.Method, "query", r.URL.Query().Encode()) + config := p.getConfiguration() if !config.Enabled || config.Secret == "" || config.UserName == "" { - http.Error(w, "This plugin is not configured.", http.StatusForbidden) + errorMessage := "This plugin is not configured" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusForbidden) return - } else if r.URL.Path != "/webhook" { + } + + if r.URL.Path != "/webhook" { + p.postHTTPDebugMessage("Invalid URL path") http.NotFound(w, r) return - } else if r.Method != http.MethodPost { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + } + + if r.Method != http.MethodPost { + errorMessage := "Method not allowed" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusMethodNotAllowed) return - } else if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("secret")), []byte(config.Secret)) != 1 { - http.Error(w, "You must provide the configured secret.", http.StatusForbidden) + } + + if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("secret")), []byte(config.Secret)) != 1 { + errorMessage := "You must provide the configured secret" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusForbidden) return } - var webhook Webhook + teamName := r.URL.Query().Get("team") + if teamName == "" { + errorMessage := "You must provide a teamName" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusBadRequest) + return + } + channelID := r.URL.Query().Get("channel") + if channelID == "" { + errorMessage := "You must provide a channelID" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusBadRequest) + return + } - if err := json.NewDecoder(r.Body).Decode(&webhook); err != nil { + var webhook *Webhook + if err := json.NewDecoder(r.Body).Decode(&webhook); err != nil || webhook == nil { + p.postHTTPDebugMessage(err.Error()) http.Error(w, err.Error(), http.StatusBadRequest) - } else if attachment, err := webhook.SlackAttachment(); err != nil { + return + } + if err := webhook.IsValid(); err != nil { + p.postHTTPDebugMessage(err.Error()) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + attachment, err := webhook.SlackAttachment() + if err != nil { + p.postHTTPDebugMessage(err.Error()) http.Error(w, err.Error(), http.StatusInternalServerError) - } else if attachment == nil { return - } else if r.URL.Query().Get("channel") == "" { - http.Error(w, "You must provide a channel.", http.StatusBadRequest) - } else if user, err := p.API.GetUserByUsername(config.UserName); err != nil { - http.Error(w, err.Message, err.StatusCode) - } else if channel, err := p.API.GetChannelByNameForTeamName(r.URL.Query().Get("team"), r.URL.Query().Get("channel"), false); err != nil { - http.Error(w, err.Message, err.StatusCode) - } else if _, err := p.API.CreatePost(&model.Post{ + } + if attachment == nil { + errorMessage := "Failed to create post" + p.postHTTPDebugMessage(errorMessage) + http.Error(w, errorMessage, http.StatusInternalServerError) + } + + user, appErr := p.API.GetUserByUsername(config.UserName) + if appErr != nil { + p.postHTTPDebugMessage(appErr.Message) + http.Error(w, appErr.Message, appErr.StatusCode) + return + } + + channel, appErr := p.API.GetChannelByNameForTeamName(teamName, channelID, false) + if appErr != nil { + p.postHTTPDebugMessage(appErr.Message) + http.Error(w, appErr.Message, appErr.StatusCode) + return + } + + post := &model.Post{ ChannelId: channel.Id, - Type: model.POST_SLACK_ATTACHMENT, UserId: user.Id, Props: map[string]interface{}{ "from_webhook": "true", "use_user_icon": "true", - "attachments": []*model.SlackAttachment{attachment}, }, - }); err != nil { - http.Error(w, err.Message, err.StatusCode) } + model.ParseSlackAttachment(post, []*model.SlackAttachment{attachment}) + if _, appErr := p.API.CreatePost(post); appErr != nil { + p.postHTTPDebugMessage(appErr.Message) + http.Error(w, appErr.Message, appErr.StatusCode) + return + } +} + +func (p *Plugin) postHTTPDebugMessage(errorMessage string) { + p.API.LogDebug("Failed to serve HTTP request", "Error message", errorMessage) } diff --git a/server/plugin_test.go b/server/plugin_test.go index 86801e699..de66c9340 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -73,6 +73,11 @@ func TestPlugin(t *testing.T) { Request: httptest.NewRequest("POST", "/webhook?team=theteam&secret=thesecret", validRequestBody()), ExpectedStatusCode: http.StatusBadRequest, }, + "NoTeam": { + Configuration: validConfiguration, + Request: httptest.NewRequest("POST", "/webhook?channel=thechannel&secret=thesecret", validRequestBody()), + ExpectedStatusCode: http.StatusBadRequest, + }, "WrongSecret": { Configuration: validConfiguration, Request: httptest.NewRequest("POST", "/webhook?team=theteam&channel=thechannel&secret=notthesecret", validRequestBody()), @@ -86,7 +91,7 @@ func TestPlugin(t *testing.T) { "UnknownJSONPayload": { Configuration: validConfiguration, Request: httptest.NewRequest("POST", "/webhook?team=theteam&channel=thechannel&secret=thesecret", ioutil.NopCloser(bytes.NewBufferString("{}"))), - ExpectedStatusCode: http.StatusOK, + ExpectedStatusCode: http.StatusBadRequest, }, "InvalidChannel": { Configuration: validConfiguration, @@ -132,6 +137,9 @@ func TestPlugin(t *testing.T) { t.Run(name, func(t *testing.T) { api := &plugintest.API{} + api.On("LogDebug", mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string")).Return(nil) + api.On("LogDebug", mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string"), mock.AnythingOfTypeArgument("string")).Return(nil) + api.On("GetUserByUsername", "theuser").Return(&model.User{ Id: "theuserid", }, (*model.AppError)(nil)) diff --git a/server/webhook.go b/server/webhook.go index aed34413d..537be5e92 100644 --- a/server/webhook.go +++ b/server/webhook.go @@ -5,6 +5,7 @@ package main import ( "bytes" + "errors" "strings" "text/template" @@ -56,7 +57,7 @@ type Webhook struct { } } -// Returns the text to be placed in the resulting post or an empty string if nothing should be +// SlackAttachment returns the text to be placed in the resulting post or an empty string if nothing should be // posted. func (w *Webhook) SlackAttachment() (*model.SlackAttachment, error) { switch w.WebhookEvent { @@ -172,3 +173,10 @@ func (w *Webhook) renderText(tplBody string) (string, error) { } return buf.String(), nil } + +func (w *Webhook) IsValid() error { + if w.WebhookEvent == "" { + return errors.New("WebhookEvent missing") + } + return nil +}