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

@@ -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)
 }

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))

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
+}