From 9b349b12f4aae315a67944b284e6e43e9afe5916 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 23 Feb 2017 16:17:45 +0000 Subject: [PATCH 1/3] Update util dep to pull in util.JSONResponse --- vendor/manifest | 2 +- .../src/github.com/matrix-org/util/context.go | 35 ++++ .../src/github.com/matrix-org/util/error.go | 18 -- vendor/src/github.com/matrix-org/util/json.go | 154 +++++++++------- .../github.com/matrix-org/util/json_test.go | 170 ++++++++++++++++-- 5 files changed, 280 insertions(+), 99 deletions(-) create mode 100644 vendor/src/github.com/matrix-org/util/context.go delete mode 100644 vendor/src/github.com/matrix-org/util/error.go diff --git a/vendor/manifest b/vendor/manifest index 1bf7484..93d78e9 100644 --- a/vendor/manifest +++ b/vendor/manifest @@ -150,7 +150,7 @@ { "importpath": "github.com/matrix-org/util", "repository": "https://github.com/matrix-org/util", - "revision": "9b44af331cdd83d702e4f16433e47341d983c23b", + "revision": "ccef6dc7c24a7c896d96b433a9107b7c47ecf828", "branch": "master" }, { diff --git a/vendor/src/github.com/matrix-org/util/context.go b/vendor/src/github.com/matrix-org/util/context.go new file mode 100644 index 0000000..d8def4f --- /dev/null +++ b/vendor/src/github.com/matrix-org/util/context.go @@ -0,0 +1,35 @@ +package util + +import ( + "context" + + log "github.com/Sirupsen/logrus" +) + +// contextKeys is a type alias for string to namespace Context keys per-package. +type contextKeys string + +// ctxValueRequestID is the key to extract the request ID for an HTTP request +const ctxValueRequestID = contextKeys("requestid") + +// GetRequestID returns the request ID associated with this context, or the empty string +// if one is not associated with this context. +func GetRequestID(ctx context.Context) string { + id := ctx.Value(ctxValueRequestID) + if id == nil { + return "" + } + return id.(string) +} + +// ctxValueLogger is the key to extract the logrus Logger. +const ctxValueLogger = contextKeys("logger") + +// GetLogger retrieves the logrus logger from the supplied context. Returns nil if there is no logger. +func GetLogger(ctx context.Context) *log.Entry { + l := ctx.Value(ctxValueLogger) + if l == nil { + return nil + } + return l.(*log.Entry) +} diff --git a/vendor/src/github.com/matrix-org/util/error.go b/vendor/src/github.com/matrix-org/util/error.go deleted file mode 100644 index 9d40c57..0000000 --- a/vendor/src/github.com/matrix-org/util/error.go +++ /dev/null @@ -1,18 +0,0 @@ -package util - -import "fmt" - -// HTTPError An HTTP Error response, which may wrap an underlying native Go Error. -type HTTPError struct { - WrappedError error - Message string - Code int -} - -func (e HTTPError) Error() string { - var wrappedErrMsg string - if e.WrappedError != nil { - wrappedErrMsg = e.WrappedError.Error() - } - return fmt.Sprintf("%s: %d: %s", e.Message, e.Code, wrappedErrMsg) -} diff --git a/vendor/src/github.com/matrix-org/util/json.go b/vendor/src/github.com/matrix-org/util/json.go index 4735bf5..b0834ea 100644 --- a/vendor/src/github.com/matrix-org/util/json.go +++ b/vendor/src/github.com/matrix-org/util/json.go @@ -3,50 +3,75 @@ package util import ( "context" "encoding/json" - "fmt" "math/rand" "net/http" "runtime/debug" + "time" log "github.com/Sirupsen/logrus" ) -// ContextKeys is a type alias for string to namespace Context keys per-package. -type ContextKeys string +// JSONResponse represents an HTTP response which contains a JSON body. +type JSONResponse struct { + // HTTP status code. + Code int + // JSON represents the JSON that should be serialized and sent to the client + JSON interface{} + // Headers represent any headers that should be sent to the client + Headers map[string]string +} -// CtxValueLogger is the key to extract the logrus Logger. -const CtxValueLogger = ContextKeys("logger") +// Is2xx returns true if the Code is between 200 and 299. +func (r JSONResponse) Is2xx() bool { + return r.Code/100 == 2 +} -// JSONRequestHandler represents an interface that must be satisfied in order to respond to incoming -// HTTP requests with JSON. The interface returned will be marshalled into JSON to be sent to the client, -// unless the interface is []byte in which case the bytes are sent to the client unchanged. -// If an error is returned, a JSON error response will also be returned, unless the error code -// is a 302 REDIRECT in which case a redirect is sent based on the Message field. -type JSONRequestHandler interface { - OnIncomingRequest(req *http.Request) (interface{}, *HTTPError) +// RedirectResponse returns a JSONResponse which 302s the client to the given location. +func RedirectResponse(location string) JSONResponse { + headers := make(map[string]string) + headers["Location"] = location + return JSONResponse{ + Code: 302, + JSON: struct{}{}, + Headers: headers, + } +} + +// MessageResponse returns a JSONResponse with a 'message' key containing the given text. +func MessageResponse(code int, msg string) JSONResponse { + return JSONResponse{ + Code: code, + JSON: struct { + Message string `json:"message"` + }{msg}, + } } -// JSONError represents a JSON API error response -type JSONError struct { - Message string `json:"message"` +// ErrorResponse returns an HTTP 500 JSONResponse with the stringified form of the given error. +func ErrorResponse(err error) JSONResponse { + return MessageResponse(500, err.Error()) +} + +// JSONRequestHandler represents an interface that must be satisfied in order to respond to incoming +// HTTP requests with JSON. +type JSONRequestHandler interface { + OnIncomingRequest(req *http.Request) JSONResponse } // Protect panicking HTTP requests from taking down the entire process, and log them using // the correct logger, returning a 500 with a JSON response rather than abruptly closing the -// connection. The http.Request MUST have a CtxValueLogger. +// connection. The http.Request MUST have a ctxValueLogger. func Protect(handler http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { defer func() { if r := recover(); r != nil { - logger := req.Context().Value(CtxValueLogger).(*log.Entry) + logger := req.Context().Value(ctxValueLogger).(*log.Entry) logger.WithFields(log.Fields{ "panic": r, }).Errorf( "Request panicked!\n%s", debug.Stack(), ) - jsonErrorResponse( - w, req, &HTTPError{nil, "Internal Server Error", 500}, - ) + respond(w, req, MessageResponse(500, "Internal Server Error")) } }() handler(w, req) @@ -55,72 +80,67 @@ func Protect(handler http.HandlerFunc) http.HandlerFunc { // MakeJSONAPI creates an HTTP handler which always responds to incoming requests with JSON responses. // Incoming http.Requests will have a logger (with a request ID/method/path logged) attached to the Context. -// This can be accessed via the const CtxValueLogger. The type of the logger is *log.Entry from github.com/Sirupsen/logrus +// This can be accessed via GetLogger(Context). func MakeJSONAPI(handler JSONRequestHandler) http.HandlerFunc { return Protect(func(w http.ResponseWriter, req *http.Request) { - // Set a Logger on the context - ctx := context.WithValue(req.Context(), CtxValueLogger, log.WithFields(log.Fields{ + reqID := RandomString(12) + // Set a Logger and request ID on the context + ctx := context.WithValue(req.Context(), ctxValueLogger, log.WithFields(log.Fields{ "req.method": req.Method, "req.path": req.URL.Path, - "req.id": RandomString(12), + "req.id": reqID, })) + ctx = context.WithValue(ctx, ctxValueRequestID, reqID) req = req.WithContext(ctx) - logger := req.Context().Value(CtxValueLogger).(*log.Entry) + logger := req.Context().Value(ctxValueLogger).(*log.Entry) logger.Print("Incoming request") - res, httpErr := handler.OnIncomingRequest(req) + res := handler.OnIncomingRequest(req) // Set common headers returned regardless of the outcome of the request w.Header().Set("Content-Type", "application/json") SetCORSHeaders(w) - if httpErr != nil { - jsonErrorResponse(w, req, httpErr) - return - } - - // if they've returned bytes as the response, then just return them rather than marshalling as JSON. - // This gives handlers an escape hatch if they want to return cached bytes. - var resBytes []byte - resBytes, ok := res.([]byte) - if !ok { - r, err := json.Marshal(res) - if err != nil { - jsonErrorResponse(w, req, &HTTPError{nil, "Failed to serialise response as JSON", 500}) - return - } - resBytes = r - } - logger.Print(fmt.Sprintf("Responding (%d bytes)", len(resBytes))) - w.Write(resBytes) + respond(w, req, res) }) } -func jsonErrorResponse(w http.ResponseWriter, req *http.Request, httpErr *HTTPError) { - logger := req.Context().Value(CtxValueLogger).(*log.Entry) - if httpErr.Code == 302 { - logger.WithField("err", httpErr.Error()).Print("Redirecting") - http.Redirect(w, req, httpErr.Message, 302) - return - } - logger.WithFields(log.Fields{ - log.ErrorKey: httpErr, - }).Print("Responding with error") +func respond(w http.ResponseWriter, req *http.Request, res JSONResponse) { + logger := req.Context().Value(ctxValueLogger).(*log.Entry) - w.WriteHeader(httpErr.Code) // Set response code + // Set custom headers + if res.Headers != nil { + for h, val := range res.Headers { + w.Header().Set(h, val) + } + } - r, err := json.Marshal(&JSONError{ - Message: httpErr.Message, - }) + // Marshal JSON response into raw bytes to send as the HTTP body + resBytes, err := json.Marshal(res.JSON) if err != nil { - // We should never fail to marshal the JSON error response, but in this event just skip - // marshalling altogether - logger.Warn("Failed to marshal error response") - w.Write([]byte(`{}`)) - return + logger.WithError(err).Error("Failed to marshal JSONResponse") + // this should never fail to be marshalled so drop err to the floor + res = MessageResponse(500, "Internal Server Error") + resBytes, _ = json.Marshal(res.JSON) + } + + // Set status code and write the body + w.WriteHeader(res.Code) + logger.WithField("code", res.Code).Infof("Responding (%d bytes)", len(resBytes)) + w.Write(resBytes) +} + +// WithCORSOptions intercepts all OPTIONS requests and responds with CORS headers. The request handler +// is not invoked when this happens. +func WithCORSOptions(handler http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, req *http.Request) { + if req.Method == "OPTIONS" { + SetCORSHeaders(w) + return + } + handler(w, req) } - w.Write(r) } // SetCORSHeaders sets unrestricted origin Access-Control headers on the response writer @@ -140,3 +160,7 @@ func RandomString(n int) string { } return string(b) } + +func init() { + rand.Seed(time.Now().UTC().UnixNano()) +} diff --git a/vendor/src/github.com/matrix-org/util/json_test.go b/vendor/src/github.com/matrix-org/util/json_test.go index 203fa70..687db27 100644 --- a/vendor/src/github.com/matrix-org/util/json_test.go +++ b/vendor/src/github.com/matrix-org/util/json_test.go @@ -2,6 +2,7 @@ package util import ( "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -10,10 +11,10 @@ import ( ) type MockJSONRequestHandler struct { - handler func(req *http.Request) (interface{}, *HTTPError) + handler func(req *http.Request) JSONResponse } -func (h *MockJSONRequestHandler) OnIncomingRequest(req *http.Request) (interface{}, *HTTPError) { +func (h *MockJSONRequestHandler) OnIncomingRequest(req *http.Request) JSONResponse { return h.handler(req) } @@ -24,22 +25,27 @@ type MockResponse struct { func TestMakeJSONAPI(t *testing.T) { log.SetLevel(log.PanicLevel) // suppress logs in test output tests := []struct { - Return interface{} - Err *HTTPError + Return JSONResponse ExpectCode int ExpectJSON string }{ - {nil, &HTTPError{nil, "Everything is broken", 500}, 500, `{"message":"Everything is broken"}`}, // Error return values - {nil, &HTTPError{nil, "Not here", 404}, 404, `{"message":"Not here"}`}, // With different status codes - {&MockResponse{"yep"}, nil, 200, `{"foo":"yep"}`}, // Success return values - {[]MockResponse{{"yep"}, {"narp"}}, nil, 200, `[{"foo":"yep"},{"foo":"narp"}]`}, // Top-level array success values - {[]byte(`actually bytes`), nil, 200, `actually bytes`}, // raw []byte escape hatch - {func(cannotBe, marshalled string) {}, nil, 500, `{"message":"Failed to serialise response as JSON"}`}, // impossible marshal + // MessageResponse return values + {MessageResponse(500, "Everything is broken"), 500, `{"message":"Everything is broken"}`}, + // interface return values + {JSONResponse{500, MockResponse{"yep"}, nil}, 500, `{"foo":"yep"}`}, + // Error JSON return values which fail to be marshalled should fallback to text + {JSONResponse{500, struct { + Foo interface{} `json:"foo"` + }{func(cannotBe, marshalled string) {}}, nil}, 500, `{"message":"Internal Server Error"}`}, + // With different status codes + {JSONResponse{201, MockResponse{"narp"}, nil}, 201, `{"foo":"narp"}`}, + // Top-level array success values + {JSONResponse{200, []MockResponse{{"yep"}, {"narp"}}, nil}, 200, `[{"foo":"yep"},{"foo":"narp"}]`}, } for _, tst := range tests { - mock := MockJSONRequestHandler{func(req *http.Request) (interface{}, *HTTPError) { - return tst.Return, tst.Err + mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse { + return tst.Return }} mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) mockWriter := httptest.NewRecorder() @@ -55,10 +61,38 @@ func TestMakeJSONAPI(t *testing.T) { } } +func TestMakeJSONAPICustomHeaders(t *testing.T) { + mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse { + headers := make(map[string]string) + headers["Custom"] = "Thing" + headers["X-Custom"] = "Things" + return JSONResponse{ + Code: 200, + JSON: MockResponse{"yep"}, + Headers: headers, + } + }} + mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + mockWriter := httptest.NewRecorder() + handlerFunc := MakeJSONAPI(&mock) + handlerFunc(mockWriter, mockReq) + if mockWriter.Code != 200 { + t.Errorf("TestMakeJSONAPICustomHeaders wanted HTTP status 200, got %d", mockWriter.Code) + } + h := mockWriter.Header().Get("Custom") + if h != "Thing" { + t.Errorf("TestMakeJSONAPICustomHeaders wanted header 'Custom: Thing' , got 'Custom: %s'", h) + } + h = mockWriter.Header().Get("X-Custom") + if h != "Things" { + t.Errorf("TestMakeJSONAPICustomHeaders wanted header 'X-Custom: Things' , got 'X-Custom: %s'", h) + } +} + func TestMakeJSONAPIRedirect(t *testing.T) { log.SetLevel(log.PanicLevel) // suppress logs in test output - mock := MockJSONRequestHandler{func(req *http.Request) (interface{}, *HTTPError) { - return nil, &HTTPError{nil, "https://matrix.org", 302} + mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse { + return RedirectResponse("https://matrix.org") }} mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) mockWriter := httptest.NewRecorder() @@ -73,12 +107,74 @@ func TestMakeJSONAPIRedirect(t *testing.T) { } } +func TestMakeJSONAPIError(t *testing.T) { + log.SetLevel(log.PanicLevel) // suppress logs in test output + mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse { + err := errors.New("oops") + return ErrorResponse(err) + }} + mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + mockWriter := httptest.NewRecorder() + handlerFunc := MakeJSONAPI(&mock) + handlerFunc(mockWriter, mockReq) + if mockWriter.Code != 500 { + t.Errorf("TestMakeJSONAPIError wanted HTTP status 500, got %d", mockWriter.Code) + } + actualBody := mockWriter.Body.String() + expect := `{"message":"oops"}` + if actualBody != expect { + t.Errorf("TestMakeJSONAPIError wanted body '%s', got '%s'", expect, actualBody) + } +} + +func TestIs2xx(t *testing.T) { + tests := []struct { + Code int + Expect bool + }{ + {200, true}, + {201, true}, + {299, true}, + {300, false}, + {199, false}, + {0, false}, + {500, false}, + } + for _, test := range tests { + j := JSONResponse{ + Code: test.Code, + } + actual := j.Is2xx() + if actual != test.Expect { + t.Errorf("TestIs2xx wanted %t, got %t", test.Expect, actual) + } + } +} + +func TestGetLogger(t *testing.T) { + log.SetLevel(log.PanicLevel) // suppress logs in test output + entry := log.WithField("test", "yep") + mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + ctx := context.WithValue(mockReq.Context(), ctxValueLogger, entry) + mockReq = mockReq.WithContext(ctx) + ctxLogger := GetLogger(mockReq.Context()) + if ctxLogger != entry { + t.Errorf("TestGetLogger wanted logger '%v', got '%v'", entry, ctxLogger) + } + + noLoggerInReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + ctxLogger = GetLogger(noLoggerInReq.Context()) + if ctxLogger != nil { + t.Errorf("TestGetLogger wanted nil logger, got '%v'", ctxLogger) + } +} + func TestProtect(t *testing.T) { log.SetLevel(log.PanicLevel) // suppress logs in test output mockWriter := httptest.NewRecorder() mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) mockReq = mockReq.WithContext( - context.WithValue(mockReq.Context(), CtxValueLogger, log.WithField("test", "yep")), + context.WithValue(mockReq.Context(), ctxValueLogger, log.WithField("test", "yep")), ) h := Protect(func(w http.ResponseWriter, req *http.Request) { panic("oh noes!") @@ -97,3 +193,47 @@ func TestProtect(t *testing.T) { t.Errorf("TestProtect wanted body %s, got %s", expectBody, actualBody) } } + +func TestWithCORSOptions(t *testing.T) { + log.SetLevel(log.PanicLevel) // suppress logs in test output + mockWriter := httptest.NewRecorder() + mockReq, _ := http.NewRequest("OPTIONS", "http://example.com/foo", nil) + h := WithCORSOptions(func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(200) + w.Write([]byte("yep")) + }) + h(mockWriter, mockReq) + if mockWriter.Code != 200 { + t.Errorf("TestWithCORSOptions wanted HTTP status 200, got %d", mockWriter.Code) + } + + origin := mockWriter.Header().Get("Access-Control-Allow-Origin") + if origin != "*" { + t.Errorf("TestWithCORSOptions wanted Access-Control-Allow-Origin header '*', got '%s'", origin) + } + + // OPTIONS request shouldn't hit the handler func + expectBody := "" + actualBody := mockWriter.Body.String() + if actualBody != expectBody { + t.Errorf("TestWithCORSOptions wanted body %s, got %s", expectBody, actualBody) + } +} + +func TestGetRequestID(t *testing.T) { + log.SetLevel(log.PanicLevel) // suppress logs in test output + reqID := "alphabetsoup" + mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + ctx := context.WithValue(mockReq.Context(), ctxValueRequestID, reqID) + mockReq = mockReq.WithContext(ctx) + ctxReqID := GetRequestID(mockReq.Context()) + if reqID != ctxReqID { + t.Errorf("TestGetRequestID wanted request ID '%s', got '%s'", reqID, ctxReqID) + } + + noReqIDInReq, _ := http.NewRequest("GET", "http://example.com/foo", nil) + ctxReqID = GetRequestID(noReqIDInReq.Context()) + if ctxReqID != "" { + t.Errorf("TestGetRequestID wanted empty request ID, got '%s'", ctxReqID) + } +} From 302a0238e26f88d8d5c88f9a5eb1ee49f44b0ca5 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 23 Feb 2017 17:37:42 +0000 Subject: [PATCH 2/3] Update go-neb to use util.JSONResponse --- .../matrix-org/go-neb/api/handlers/auth.go | 109 +++++++++++------- .../matrix-org/go-neb/api/handlers/client.go | 22 ++-- .../go-neb/api/handlers/heartbeat.go | 7 +- .../matrix-org/go-neb/api/handlers/service.go | 75 +++++++----- .../go-neb/services/github/webhook/webhook.go | 17 ++- .../matrix-org/go-neb/services/jira/jira.go | 4 +- .../go-neb/services/jira/webhook/webhook.go | 50 ++++---- 7 files changed, 166 insertions(+), 118 deletions(-) diff --git a/src/github.com/matrix-org/go-neb/api/handlers/auth.go b/src/github.com/matrix-org/go-neb/api/handlers/auth.go index e056a1f..8030b09 100644 --- a/src/github.com/matrix-org/go-neb/api/handlers/auth.go +++ b/src/github.com/matrix-org/go-neb/api/handlers/auth.go @@ -40,36 +40,43 @@ type RequestAuthSession struct { // { // // AuthRealm-specific information // } -func (h *RequestAuthSession) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (h *RequestAuthSession) OnIncomingRequest(req *http.Request) util.JSONResponse { + logger := util.GetLogger(req.Context()) if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body api.RequestAuthSessionRequest if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } - log.WithFields(log.Fields{ + logger.WithFields(log.Fields{ "realm_id": body.RealmID, "user_id": body.UserID, }).Print("Incoming auth session request") if err := body.Check(); err != nil { - return nil, &util.HTTPError{err, err.Error(), 400} + logger.WithError(err).Info("Failed Check") + return util.MessageResponse(400, err.Error()) } realm, err := h.Db.LoadAuthRealm(body.RealmID) if err != nil { - return nil, &util.HTTPError{err, "Unknown RealmID", 400} + logger.WithError(err).Info("Failed to LoadAuthRealm") + return util.MessageResponse(400, "Unknown RealmID") } response := realm.RequestAuthSession(body.UserID, body.Config) if response == nil { - return nil, &util.HTTPError{nil, "Failed to request auth session", 500} + logger.WithField("body", body).Error("Failed to RequestAuthSession") + return util.MessageResponse(500, "Failed to request auth session") } metrics.IncrementAuthSession(realm.Type()) - return response, nil + return util.JSONResponse{ + Code: 200, + JSON: response, + } } // RemoveAuthSession represents an HTTP handler capable of processing /admin/removeAuthSession requests. @@ -90,36 +97,41 @@ type RemoveAuthSession struct { // Response: // HTTP/1.1 200 OK // {} -func (h *RemoveAuthSession) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (h *RemoveAuthSession) OnIncomingRequest(req *http.Request) util.JSONResponse { + logger := util.GetLogger(req.Context()) if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body struct { RealmID string UserID string } if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } - log.WithFields(log.Fields{ + logger.WithFields(log.Fields{ "realm_id": body.RealmID, "user_id": body.UserID, }).Print("Incoming remove auth session request") if body.UserID == "" || body.RealmID == "" { - return nil, &util.HTTPError{nil, `Must supply a "UserID", a "RealmID"`, 400} + return util.MessageResponse(400, `Must supply a "UserID", a "RealmID"`) } _, err := h.Db.LoadAuthRealm(body.RealmID) if err != nil { - return nil, &util.HTTPError{err, "Unknown RealmID", 400} + return util.MessageResponse(400, "Unknown RealmID") } if err := h.Db.RemoveAuthSession(body.RealmID, body.UserID); err != nil { - return nil, &util.HTTPError{err, "Failed to remove auth session", 500} + logger.WithError(err).Error("Failed to RemoveAuthSession") + return util.MessageResponse(500, "Failed to remove auth session") } - return []byte(`{}`), nil + return util.JSONResponse{ + Code: 200, + JSON: struct{}{}, + } } // RealmRedirect represents an HTTP handler which can process incoming redirects for auth realms. @@ -186,39 +198,44 @@ type ConfigureAuthRealm struct { // // New auth realm config information // }, // } -func (h *ConfigureAuthRealm) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (h *ConfigureAuthRealm) OnIncomingRequest(req *http.Request) util.JSONResponse { + logger := util.GetLogger(req.Context()) if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body api.ConfigureAuthRealmRequest if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } if err := body.Check(); err != nil { - return nil, &util.HTTPError{err, err.Error(), 400} + return util.MessageResponse(400, err.Error()) } realm, err := types.CreateAuthRealm(body.ID, body.Type, body.Config) if err != nil { - return nil, &util.HTTPError{err, "Error parsing config JSON", 400} + return util.MessageResponse(400, "Error parsing config JSON") } if err = realm.Register(); err != nil { - return nil, &util.HTTPError{err, "Error registering auth realm", 400} + return util.MessageResponse(400, "Error registering auth realm") } oldRealm, err := h.Db.StoreAuthRealm(realm) if err != nil { - return nil, &util.HTTPError{err, "Error storing realm", 500} + logger.WithError(err).Error("Failed to StoreAuthRealm") + return util.MessageResponse(500, "Error storing realm") } - return &struct { - ID string - Type string - OldConfig types.AuthRealm - NewConfig types.AuthRealm - }{body.ID, body.Type, oldRealm, realm}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + ID string + Type string + OldConfig types.AuthRealm + NewConfig types.AuthRealm + }{body.ID, body.Type, oldRealm, realm}, + } } // GetSession represents an HTTP handler capable of processing /admin/getSession requests. @@ -252,35 +269,43 @@ type GetSession struct { // { // "Authenticated": false // } -func (h *GetSession) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (h *GetSession) OnIncomingRequest(req *http.Request) util.JSONResponse { + logger := util.GetLogger(req.Context()) if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body struct { RealmID string UserID string } if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } if body.RealmID == "" || body.UserID == "" { - return nil, &util.HTTPError{nil, `Must supply a "RealmID" and "UserID"`, 400} + return util.MessageResponse(400, `Must supply a "RealmID" and "UserID"`) } session, err := h.Db.LoadAuthSessionByUser(body.RealmID, body.UserID) if err != nil && err != sql.ErrNoRows { - return nil, &util.HTTPError{err, `Failed to load session`, 500} + logger.WithError(err).WithField("body", body).Error("Failed to LoadAuthSessionByUser") + return util.MessageResponse(500, `Failed to load session`) } if err == sql.ErrNoRows { - return &struct { - Authenticated bool - }{false}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + Authenticated bool + }{false}, + } } - return &struct { - ID string - Authenticated bool - Info interface{} - }{session.ID(), session.Authenticated(), session.Info()}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + ID string + Authenticated bool + Info interface{} + }{session.ID(), session.Authenticated(), session.Info()}, + } } diff --git a/src/github.com/matrix-org/go-neb/api/handlers/client.go b/src/github.com/matrix-org/go-neb/api/handlers/client.go index 5327ae8..3e00a20 100644 --- a/src/github.com/matrix-org/go-neb/api/handlers/client.go +++ b/src/github.com/matrix-org/go-neb/api/handlers/client.go @@ -39,27 +39,31 @@ type ConfigureClient struct { // // The new api.ClientConfig // } // } -func (s *ConfigureClient) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (s *ConfigureClient) OnIncomingRequest(req *http.Request) util.JSONResponse { if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body api.ClientConfig if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } if err := body.Check(); err != nil { - return nil, &util.HTTPError{err, "Error parsing client config", 400} + return util.MessageResponse(400, "Error parsing client config") } oldClient, err := s.Clients.Update(body) if err != nil { - return nil, &util.HTTPError{err, "Error storing token", 500} + util.GetLogger(req.Context()).WithError(err).WithField("body", body).Error("Failed to Clients.Update") + return util.MessageResponse(500, "Error storing token") } - return &struct { - OldClient api.ClientConfig - NewClient api.ClientConfig - }{oldClient, body}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + OldClient api.ClientConfig + NewClient api.ClientConfig + }{oldClient, body}, + } } diff --git a/src/github.com/matrix-org/go-neb/api/handlers/heartbeat.go b/src/github.com/matrix-org/go-neb/api/handlers/heartbeat.go index 484814b..c110139 100644 --- a/src/github.com/matrix-org/go-neb/api/handlers/heartbeat.go +++ b/src/github.com/matrix-org/go-neb/api/handlers/heartbeat.go @@ -26,6 +26,9 @@ type Heartbeat struct{} // Response: // HTTP/1.1 200 OK // {} -func (*Heartbeat) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { - return &struct{}{}, nil +func (*Heartbeat) OnIncomingRequest(req *http.Request) util.JSONResponse { + return util.JSONResponse{ + Code: 200, + JSON: struct{}{}, + } } diff --git a/src/github.com/matrix-org/go-neb/api/handlers/service.go b/src/github.com/matrix-org/go-neb/api/handlers/service.go index 6cd3fd4..d088d63 100644 --- a/src/github.com/matrix-org/go-neb/api/handlers/service.go +++ b/src/github.com/matrix-org/go-neb/api/handlers/service.go @@ -76,16 +76,17 @@ func (s *ConfigureService) getMutexForServiceID(serviceID string) *sync.Mutex { // // new service-specific config information // }, // } -func (s *ConfigureService) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (s *ConfigureService) OnIncomingRequest(req *http.Request) util.JSONResponse { if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } service, httpErr := s.createService(req) if httpErr != nil { - return nil, httpErr + return *httpErr } - log.WithFields(log.Fields{ + logger := util.GetLogger(req.Context()) + logger.WithFields(log.Fields{ "service_id": service.ServiceID(), "service_type": service.ServiceType(), "service_user_id": service.ServiceUserID(), @@ -98,32 +99,34 @@ func (s *ConfigureService) OnIncomingRequest(req *http.Request) (interface{}, *u old, err := s.db.LoadService(service.ServiceID()) if err != nil && err != sql.ErrNoRows { - return nil, &util.HTTPError{err, "Error loading old service", 500} + logger.WithError(err).Error("Failed to LoadService") + return util.MessageResponse(500, "Error loading old service") } client, err := s.clients.Client(service.ServiceUserID()) if err != nil { - return nil, &util.HTTPError{err, "Unknown matrix client", 400} + return util.MessageResponse(400, "Unknown matrix client") } if err := checkClientForService(service, client); err != nil { - return nil, &util.HTTPError{err, err.Error(), 400} + return util.MessageResponse(400, err.Error()) } if err = service.Register(old, client); err != nil { - return nil, &util.HTTPError{err, "Failed to register service: " + err.Error(), 500} + return util.MessageResponse(500, "Failed to register service: "+err.Error()) } oldService, err := s.db.StoreService(service) if err != nil { - return nil, &util.HTTPError{err, "Error storing service", 500} + logger.WithError(err).Error("Failed to StoreService") + return util.MessageResponse(500, "Error storing service") } // Start any polling NOW because they may decide to stop it in PostRegister, and we want to make // sure we'll actually stop. if _, ok := service.(types.Poller); ok { if err := polling.StartPolling(service); err != nil { - log.WithFields(log.Fields{ + logger.WithFields(log.Fields{ "service_id": service.ServiceID(), log.ErrorKey: err, }).Error("Failed to start poll loop.") @@ -133,27 +136,33 @@ func (s *ConfigureService) OnIncomingRequest(req *http.Request) (interface{}, *u service.PostRegister(old) metrics.IncrementConfigureService(service.ServiceType()) - return &struct { - ID string - Type string - OldConfig types.Service - NewConfig types.Service - }{service.ServiceID(), service.ServiceType(), oldService, service}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + ID string + Type string + OldConfig types.Service + NewConfig types.Service + }{service.ServiceID(), service.ServiceType(), oldService, service}, + } } -func (s *ConfigureService) createService(req *http.Request) (types.Service, *util.HTTPError) { +func (s *ConfigureService) createService(req *http.Request) (types.Service, *util.JSONResponse) { var body api.ConfigureServiceRequest if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + res := util.MessageResponse(400, "Error parsing request JSON") + return nil, &res } if err := body.Check(); err != nil { - return nil, &util.HTTPError{err, err.Error(), 400} + res := util.MessageResponse(400, err.Error()) + return nil, &res } service, err := types.CreateService(body.ID, body.Type, body.UserID, body.Config) if err != nil { - return nil, &util.HTTPError{err, "Error parsing config JSON", 400} + res := util.MessageResponse(400, "Error parsing config JSON") + return nil, &res } return service, nil } @@ -182,34 +191,38 @@ type GetService struct { // // service-specific config information // } // } -func (h *GetService) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) { +func (h *GetService) OnIncomingRequest(req *http.Request) util.JSONResponse { if req.Method != "POST" { - return nil, &util.HTTPError{nil, "Unsupported Method", 405} + return util.MessageResponse(405, "Unsupported Method") } var body struct { ID string } if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &util.HTTPError{err, "Error parsing request JSON", 400} + return util.MessageResponse(400, "Error parsing request JSON") } if body.ID == "" { - return nil, &util.HTTPError{nil, `Must supply a "ID"`, 400} + return util.MessageResponse(400, `Must supply a "ID"`) } srv, err := h.Db.LoadService(body.ID) if err != nil { if err == sql.ErrNoRows { - return nil, &util.HTTPError{err, `Service not found`, 404} + return util.MessageResponse(404, `Service not found`) } - return nil, &util.HTTPError{err, `Failed to load service`, 500} + util.GetLogger(req.Context()).WithError(err).Error("Failed to LoadService") + return util.MessageResponse(500, `Failed to load service`) } - return &struct { - ID string - Type string - Config types.Service - }{srv.ServiceID(), srv.ServiceType(), srv}, nil + return util.JSONResponse{ + Code: 200, + JSON: struct { + ID string + Type string + Config types.Service + }{srv.ServiceID(), srv.ServiceType(), srv}, + } } func checkClientForService(service types.Service, client *gomatrix.Client) error { diff --git a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go index ed510d4..c7cb5de 100644 --- a/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go +++ b/src/github.com/matrix-org/go-neb/services/github/webhook/webhook.go @@ -21,14 +21,15 @@ import ( // matrix message to send, along with parsed repo information. // The secretToken, if supplied, will be used to verify the request is from // Github. If it isn't, an error is returned. -func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repository, *gomatrix.HTMLMessage, *util.HTTPError) { +func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repository, *gomatrix.HTMLMessage, *util.JSONResponse) { // Verify the HMAC signature if NEB was configured with a secret token eventType := r.Header.Get("X-GitHub-Event") signatureSHA1 := r.Header.Get("X-Hub-Signature") content, err := ioutil.ReadAll(r.Body) if err != nil { log.WithError(err).Print("Failed to read Github webhook body") - return "", nil, nil, &util.HTTPError{nil, "Failed to parse body", 400} + resErr := util.MessageResponse(400, "Failed to parse body") + return "", nil, nil, &resErr } // Verify request if a secret token has been supplied. if secretToken != "" { @@ -38,14 +39,16 @@ func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repo if err != nil { log.WithError(err).WithField("X-Hub-Signature", sigHex).Print( "Failed to decode signature as hex.") - return "", nil, nil, &util.HTTPError{nil, "Failed to decode signature", 400} + resErr := util.MessageResponse(400, "Failed to decode signature") + return "", nil, nil, &resErr } if !checkMAC([]byte(content), sigBytes, []byte(secretToken)) { log.WithFields(log.Fields{ "X-Hub-Signature": signatureSHA1, }).Print("Received Github event which failed MAC check.") - return "", nil, nil, &util.HTTPError{nil, "Bad signature", 403} + resErr := util.MessageResponse(403, "Bad signature") + return "", nil, nil, &resErr } } @@ -58,13 +61,15 @@ func OnReceiveRequest(r *http.Request, secretToken string) (string, *github.Repo // Github will send a "ping" event when the webhook is first created. We need // to return a 200 in order for the webhook to be marked as "up" (this doesn't // affect delivery, just the tick/cross status flag). - return "", nil, nil, &util.HTTPError{nil, "pong", 200} + res := util.MessageResponse(200, "pong") + return "", nil, nil, &res } htmlStr, repo, refinedType, err := parseGithubEvent(eventType, content) if err != nil { log.WithError(err).Print("Failed to parse github event") - return "", nil, nil, &util.HTTPError{nil, "Failed to parse github event", 500} + resErr := util.MessageResponse(500, "Failed to parse github event") + return "", nil, nil, &resErr } msg := gomatrix.GetHTMLMessage("m.notice", htmlStr) diff --git a/src/github.com/matrix-org/go-neb/services/jira/jira.go b/src/github.com/matrix-org/go-neb/services/jira/jira.go index d4e8735..647c9d9 100644 --- a/src/github.com/matrix-org/go-neb/services/jira/jira.go +++ b/src/github.com/matrix-org/go-neb/services/jira/jira.go @@ -271,8 +271,8 @@ func (s *Service) Expansions(cli *gomatrix.Client) []types.Expansion { func (s *Service) OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *gomatrix.Client) { eventProjectKey, event, httpErr := webhook.OnReceiveRequest(req) if httpErr != nil { - log.WithError(httpErr).Print("Failed to handle JIRA webhook") - w.WriteHeader(500) + log.Print("Failed to handle JIRA webhook") + w.WriteHeader(httpErr.Code) return } // grab base jira url diff --git a/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go b/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go index 4aa45a9..ff15f7f 100644 --- a/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go +++ b/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go @@ -65,19 +65,19 @@ func RegisterHook(jrealm *jira.Realm, projects []string, userID, webhookEndpoint logger.WithError(err).Print("No JIRA client exists") return err // no OAuth token on this JIRA endpoint } - wh, httpErr := getWebhook(cli, webhookEndpointURL) - if httpErr != nil { - if httpErr.Code != 403 { - logger.WithError(httpErr).Print("Failed to GET webhook") - return httpErr + wh, forbidden, err := getWebhook(cli, webhookEndpointURL) + if err != nil { + if !forbidden { + logger.WithError(err).Print("Failed to GET webhook") + return err } // User is not a JIRA admin (cannot GET webhooks) // The only way this is going to end well for this request is if all the projects // are PUBLIC. That is, they can be accessed directly without an access token. - httpErr = checkProjectsArePublic(jrealm, projects, userID) - if httpErr != nil { - logger.WithError(httpErr).Print("Failed to assert that all projects are public") - return httpErr + err = checkProjectsArePublic(jrealm, projects, userID) + if err != nil { + logger.WithError(err).Print("Failed to assert that all projects are public") + return err } // All projects that wish to be tracked are public, but the user cannot create @@ -101,17 +101,19 @@ func RegisterHook(jrealm *jira.Realm, projects []string, userID, webhookEndpoint // OnReceiveRequest is called when JIRA hits NEB with an update. // Returns the project key and webhook event, or an error. -func OnReceiveRequest(req *http.Request) (string, *Event, *util.HTTPError) { +func OnReceiveRequest(req *http.Request) (string, *Event, *util.JSONResponse) { // extract the JIRA webhook event JSON defer req.Body.Close() var whe Event err := json.NewDecoder(req.Body).Decode(&whe) if err != nil { - return "", nil, &util.HTTPError{err, "Failed to parse request JSON", 400} + resErr := util.MessageResponse(400, "Failed to parse request JSON") + return "", nil, &resErr } if err != nil { - return "", nil, &util.HTTPError{err, "Failed to parse JIRA URL", 400} + resErr := util.MessageResponse(400, "Failed to parse JIRA URL") + return "", nil, &resErr } projKey := strings.Split(whe.Issue.Key, "-")[0] projKey = strings.ToUpper(projKey) @@ -153,22 +155,18 @@ func createWebhook(jrealm *jira.Realm, webhookEndpointURL, userID string) error return err } -func getWebhook(cli *gojira.Client, webhookEndpointURL string) (*jiraWebhook, *util.HTTPError) { +func getWebhook(cli *gojira.Client, webhookEndpointURL string) (*jiraWebhook, bool, error) { req, err := cli.NewRequest("GET", "rest/webhooks/1.0/webhook", nil) if err != nil { - return nil, &util.HTTPError{err, "Failed to prepare webhook request", 500} + return nil, false, fmt.Errorf("Failed to prepare webhook request") } var webhookList []jiraWebhook res, err := cli.Do(req, &webhookList) if err != nil { - return nil, &util.HTTPError{err, "Failed to query webhooks", 502} + return nil, false, fmt.Errorf("Failed to query webhooks") } if res.StatusCode < 200 || res.StatusCode >= 300 { - return nil, &util.HTTPError{ - err, - fmt.Sprintf("Querying webhook returned HTTP %d", res.StatusCode), - 403, - } + return nil, true, fmt.Errorf("Querying webhook returned HTTP %d", res.StatusCode) } log.Print("Retrieved ", len(webhookList), " webhooks") var nebWH *jiraWebhook @@ -178,26 +176,26 @@ func getWebhook(cli *gojira.Client, webhookEndpointURL string) (*jiraWebhook, *u break } } - return nebWH, nil + return nebWH, false, nil } -func checkProjectsArePublic(jrealm *jira.Realm, projects []string, userID string) *util.HTTPError { +func checkProjectsArePublic(jrealm *jira.Realm, projects []string, userID string) error { publicCli, err := jrealm.JIRAClient("", true) if err != nil { - return &util.HTTPError{err, "Cannot create public JIRA client", 500} + return fmt.Errorf("Cannot create public JIRA client") } for _, projectKey := range projects { // check you can query this project with a public client req, err := publicCli.NewRequest("GET", "rest/api/2/project/"+projectKey, nil) if err != nil { - return &util.HTTPError{err, "Failed to create project URL", 500} + return fmt.Errorf("Failed to create project URL for project %s", projectKey) } res, err := publicCli.Do(req, nil) if err != nil { - return &util.HTTPError{err, fmt.Sprintf("Failed to query project %s", projectKey), 500} + return fmt.Errorf("Failed to query project %s", projectKey) } if res.StatusCode < 200 || res.StatusCode >= 300 { - return &util.HTTPError{err, fmt.Sprintf("Project %s is not public. (HTTP %d)", projectKey, res.StatusCode), 403} + return fmt.Errorf("Project %s is not public. (HTTP %d)", projectKey, res.StatusCode) } } return nil From 7a643a07114726f9bb3177e6fbbd6d663711fcdd Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 24 Feb 2017 10:22:58 +0000 Subject: [PATCH 3/3] Comment the return args on getWebhook --- .../matrix-org/go-neb/services/jira/webhook/webhook.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go b/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go index ff15f7f..edc9417 100644 --- a/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go +++ b/src/github.com/matrix-org/go-neb/services/jira/webhook/webhook.go @@ -155,6 +155,9 @@ func createWebhook(jrealm *jira.Realm, webhookEndpointURL, userID string) error return err } +// Get an existing JIRA webhook. Returns the hook if it exists, or an error along with a bool +// which indicates if the request to retrieve the hook is not 2xx. If it is not 2xx, it is +// forbidden (different JIRA deployments return different codes ranging from 401/403/404/500). func getWebhook(cli *gojira.Client, webhookEndpointURL string) (*jiraWebhook, bool, error) { req, err := cli.NewRequest("GET", "rest/webhooks/1.0/webhook", nil) if err != nil {