Standardize API error return values

core/echo.go

@@ -1,8 +1,11 @@
 package core
 
 import (
+	"errors"
 	"fmt"
 	"github.com/labstack/echo/v4"
+	"github.com/labstack/echo/v4/middleware"
+	"schneider.vip/problem"
 	"strings"
 	"sync"
 )
@@ -103,3 +106,39 @@ func getGroup(path string) string {
 	}
 	return ""
 }
+
+func createEchoServer(cfg HTTPConfig, strictmode bool) (EchoServer, error) {
+	echoServer := echo.New()
+	echoServer.HideBanner = true
+	echoServer.Use(middleware.Logger())
+	echoServer.HTTPErrorHandler = httpErrorHandler
+	if cfg.CORS.Enabled() {
+		if strictmode {
+			for _, origin := range cfg.CORS.Origin {
+				if strings.TrimSpace(origin) == "*" {
+					return nil, errors.New("wildcard CORS origin is not allowed in strict mode")
+				}
+			}
+		}
+		echoServer.Use(middleware.CORSWithConfig(middleware.CORSConfig{AllowOrigins: cfg.CORS.Origin}))
+	}
+	echoServer.Use(DecodeURIPath)
+	return echoServer, nil
+}
+
+func httpErrorHandler(err error, ctx echo.Context) {
+	if prb, ok := err.(*problem.Problem); ok {
+		if !ctx.Response().Committed {
+			if _, err := prb.WriteTo(ctx.Response()); err != nil {
+				ctx.Echo().Logger.Error(err)
+			}
+		}
+	} else {
+		ctx.Echo().DefaultHTTPErrorHandler(err, ctx)
+	}
+}
+
+// NewProblem creates a new problem.Problem
+func NewProblem(title string, status int, detail string) *problem.Problem {
+	return problem.New(problem.Title(title), problem.Status(status), problem.Detail(detail))
+}

core/echo_test.go

@@ -1,10 +1,16 @@
 package core
 
 import (
+	"encoding/json"
+	"errors"
 	"fmt"
 	"github.com/golang/mock/gomock"
+	"github.com/labstack/echo/v4"
 	"github.com/stretchr/testify/assert"
+	"io"
 	"net/http"
+	"net/http/httptest"
+	"schneider.vip/problem"
 	"testing"
 )
 
@@ -102,3 +108,45 @@ func Test_getGroup(t *testing.T) {
 	assert.Equal(t, "", getGroup(""))
 	assert.Equal(t, "", getGroup("/"))
 }
+
+func TestHttpErrorHandler(t *testing.T) {
+	es, _ := createEchoServer(HTTPConfig{}, false)
+	e := es.(*echo.Echo)
+	server := httptest.NewServer(e)
+	client := http.Client{}
+
+	t.Run("Problem return", func(t *testing.T) {
+		prb := NewProblem("problem title", http.StatusInternalServerError, "problem detail")
+		f := func(c echo.Context) error {
+			return prb
+		}
+		e.Add(http.MethodGet, "/problem", f)
+		req, _ := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/problem", server.URL), nil)
+		resp, err := client.Do(req)
+
+		assert.NoError(t, err)
+		assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+		assert.Equal(t, problem.ContentTypeJSON, resp.Header.Get("Content-Type"))
+
+		// Validate response body with expected problem
+		prbBytes, _ := json.Marshal(prb)
+		bodyBytes, err := io.ReadAll(resp.Body)
+		if !assert.NoError(t, err) {
+			return
+		}
+		assert.Equal(t, prbBytes, bodyBytes)
+	})
+
+	t.Run("Error return", func(t *testing.T) {
+		f := func(c echo.Context) error {
+			return errors.New("error")
+		}
+		e.Add(http.MethodGet, "/error", f)
+		req, _ := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/error", server.URL), nil)
+		resp, err := client.Do(req)
+
+		assert.NoError(t, err)
+		assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+	})
+
+}

core/engine.go

@@ -20,12 +20,9 @@
 package core
 
 import (
-	"errors"
 	"fmt"
-	"github.com/labstack/echo/v4/middleware"
 	"net/url"
 	"os"
-	"strings"
 
 	"github.com/labstack/echo/v4"
 	"github.com/spf13/cobra"
@@ -40,26 +37,10 @@ type Routable interface {
 // NewSystem creates a new, empty System.
 func NewSystem() *System {
 	return &System{
-		engines: []Engine{},
-		Config:  NewServerConfig(),
-		Routers: []Routable{},
-		EchoCreator: func(cfg HTTPConfig, strictmode bool) (EchoServer, error) {
-			echoServer := echo.New()
-			echoServer.HideBanner = true
-			echoServer.Use(middleware.Logger())
-			if cfg.CORS.Enabled() {
-				if strictmode {
-					for _, origin := range cfg.CORS.Origin {
-						if strings.TrimSpace(origin) == "*" {
-							return nil, errors.New("wildcard CORS origin is not allowed in strict mode")
-						}
-					}
-				}
-				echoServer.Use(middleware.CORSWithConfig(middleware.CORSConfig{AllowOrigins: cfg.CORS.Origin}))
-			}
-			echoServer.Use(DecodeURIPath)
-			return echoServer, nil
-		},
+		engines:     []Engine{},
+		Config:      NewServerConfig(),
+		Routers:     []Routable{},
+		EchoCreator: createEchoServer,
 	}
 }
 

core/test.go

@@ -18,7 +18,9 @@
 
 package core
 
-import "github.com/spf13/pflag"
+import (
+	"github.com/spf13/pflag"
+)
 
 type TestEngineConfig struct {
 	Key     string `koanf:"key"`

crypto/api/v1/api.go

@@ -55,24 +55,28 @@ func (signRequest SignJwtRequest) validate() error {
 	return nil
 }
 
+const (
+	problemTitleSignJwt   = "SignJWT failed"
+	problemTitlePublicKey = "Failed to get PublicKey"
+)
+
 // SignJwt handles api calls for signing a Jwt
 func (w *Wrapper) SignJwt(ctx echo.Context) error {
 	var signRequest = &SignJwtRequest{}
 	err := ctx.Bind(signRequest)
 	if err != nil {
 		log.Logger().Error(err.Error())
-		return echo.NewHTTPError(http.StatusBadRequest, err.Error())
+		return core.NewProblem(problemTitleSignJwt, http.StatusBadRequest, err.Error())
 	}
 
 	if err := signRequest.validate(); err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, err)
+		return core.NewProblem(problemTitleSignJwt, http.StatusBadRequest, err.Error())
 	}
 
 	sig, err := w.C.SignJWT(signRequest.Claims, signRequest.Kid)
-
 	if err != nil {
 		log.Logger().Error(err.Error())
-		return echo.NewHTTPError(http.StatusBadRequest, err.Error())
+		return core.NewProblem(problemTitleSignJwt, http.StatusBadRequest, err.Error())
 	}
 
 	return ctx.String(http.StatusOK, sig)
@@ -88,25 +92,24 @@ func (w *Wrapper) PublicKey(ctx echo.Context, kid string, params PublicKeyParams
 	if params.At != nil {
 		at, err = time.Parse(time.RFC3339, *params.At)
 		if err != nil {
-			return ctx.String(http.StatusBadRequest, fmt.Sprintf("cannot parse '%s' as RFC3339 time format", *params.At))
+			return core.NewProblem(problemTitlePublicKey, http.StatusBadRequest, fmt.Sprintf("cannot parse '%s' as RFC3339 time format", *params.At))
 		}
 	}
 
 	pubKey, err := w.C.GetPublicKey(kid, at)
 	if err != nil {
 		if errors.Is(err, crypto.ErrKeyNotFound) {
-			return ctx.NoContent(http.StatusNotFound)
+			return core.NewProblem(problemTitlePublicKey, http.StatusNotFound, fmt.Sprintf("no public key found for %s", kid))
 		}
 		log.Logger().Error(err.Error())
-		return err
+		return core.NewProblem(problemTitlePublicKey, http.StatusInternalServerError, err.Error())
 	}
 
-	// starts with so we can ignore any +
 	if ct, _, _ := mime.ParseMediaType(acceptHeader); ct == "application/json" {
 		jwk, err := jwk.New(pubKey)
 		if err != nil {
 			log.Logger().Error(err.Error())
-			return err
+			return core.NewProblem(problemTitlePublicKey, http.StatusInternalServerError, err.Error())
 		}
 
 		return ctx.JSON(http.StatusOK, jwk)
@@ -116,7 +119,7 @@ func (w *Wrapper) PublicKey(ctx echo.Context, kid string, params PublicKeyParams
 	pub, err := util.PublicKeyToPem(pubKey)
 	if err != nil {
 		log.Logger().Error(err.Error())
-		return err
+		return core.NewProblem(problemTitlePublicKey, http.StatusInternalServerError, err.Error())
 	}
 
 	return ctx.String(http.StatusOK, pub)

crypto/api/v1/api_test.go

@@ -32,6 +32,7 @@ import (
 	"github.com/nuts-foundation/nuts-node/crypto"
 	"github.com/nuts-foundation/nuts-node/crypto/test"
 	"github.com/nuts-foundation/nuts-node/mock"
+	test2 "github.com/nuts-foundation/nuts-node/test"
 )
 
 func TestWrapper_SignJwt(t *testing.T) {
@@ -50,8 +51,10 @@ func TestWrapper_SignJwt(t *testing.T) {
 
 		err := ctx.client.SignJwt(ctx.echo)
 
-		assert.NotNil(t, err)
-		assert.Contains(t, err.Error(), "code=400, message=missing claims")
+		test2.AssertErrProblemTitle(t, problemTitleSignJwt, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err)
+		test2.AssertErrProblemDetail(t, "missing claims", err)
+
 	})
 
 	t.Run("Missing kid returns 400", func(t *testing.T) {
@@ -69,8 +72,9 @@ func TestWrapper_SignJwt(t *testing.T) {
 
 		err := ctx.client.SignJwt(ctx.echo)
 
-		assert.NotNil(t, err)
-		assert.Contains(t, err.Error(), "code=400, message=missing kid")
+		test2.AssertErrProblemTitle(t, problemTitleSignJwt, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err)
+		test2.AssertErrProblemDetail(t, "missing kid", err)
 	})
 
 	t.Run("Sign error returns 400", func(t *testing.T) {
@@ -90,8 +94,9 @@ func TestWrapper_SignJwt(t *testing.T) {
 
 		err := ctx.client.SignJwt(ctx.echo)
 
-		assert.NotNil(t, err)
-		assert.Contains(t, err.Error(), "code=400, message=b00m!")
+		test2.AssertErrProblemTitle(t, problemTitleSignJwt, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err)
+		test2.AssertErrProblemDetail(t, "b00m!", err)
 	})
 
 	t.Run("All OK returns 200", func(t *testing.T) {
@@ -124,8 +129,9 @@ func TestWrapper_SignJwt(t *testing.T) {
 
 		err := ctx.client.SignJwt(ctx.echo)
 
-		assert.NotNil(t, err)
-		assert.Contains(t, err.Error(), "code=400, message=missing body in request")
+		test2.AssertErrProblemTitle(t, problemTitleSignJwt, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err)
+		test2.AssertErrProblemDetail(t, "missing body in request", err)
 	})
 }
 
@@ -168,9 +174,12 @@ func TestWrapper_PublicKey(t *testing.T) {
 
 		ctx.echo.EXPECT().Request().Return(&http.Request{})
 		ctx.keyStore.EXPECT().GetPublicKey("kid", at).Return(nil, crypto.ErrKeyNotFound)
-		ctx.echo.EXPECT().NoContent(http.StatusNotFound)
 
-		_ = ctx.client.PublicKey(ctx.echo, "kid", params)
+		err := ctx.client.PublicKey(ctx.echo, "kid", params)
+
+		test2.AssertErrProblemTitle(t, problemTitlePublicKey, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusNotFound, err)
+		test2.AssertErrProblemDetail(t, "no public key found for kid", err)
 	})
 
 	t.Run("PublicKey API call returns 500 for other error", func(t *testing.T) {
@@ -181,18 +190,23 @@ func TestWrapper_PublicKey(t *testing.T) {
 		ctx.keyStore.EXPECT().GetPublicKey("kid", at).Return(nil, errors.New("b00m!"))
 
 		err := ctx.client.PublicKey(ctx.echo, "kid", params)
-		assert.Error(t, err)
+
+		test2.AssertErrProblemTitle(t, problemTitlePublicKey, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusInternalServerError, err)
 	})
 
 	t.Run("error - 400 for incorrect time format", func(t *testing.T) {
 		ctx := newMockContext(t)
 		defer ctx.ctrl.Finish()
 
 		ctx.echo.EXPECT().Request().Return(&http.Request{})
-		ctx.echo.EXPECT().String(http.StatusBadRequest, "cannot parse 'b00m!' as RFC3339 time format")
+
 		notAt := "b00m!"
+		err := ctx.client.PublicKey(ctx.echo, "kid", PublicKeyParams{At: &notAt})
 
-		_ = ctx.client.PublicKey(ctx.echo, "kid", PublicKeyParams{At: &notAt})
+		test2.AssertErrProblemTitle(t, problemTitlePublicKey, err)
+		test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err)
+		test2.AssertErrProblemDetail(t, "cannot parse 'b00m!' as RFC3339 time format", err)
 	})
 }
 

crypto/api/v1/types.go

@@ -0,0 +1,6 @@
+package v1
+
+import "schneider.vip/problem"
+
+// Error is an alias for the internally used problem.Problem
+type Error = problem.Problem

docs/_static/common/error_response.yaml

@@ -0,0 +1,25 @@
+description: |
+  Default return values follow Problem Details for HTTP APIs as specified in [RFC7807](https://tools.ietf.org/html/rfc7807).
+
+  Currently, return values contain the following members of a problem details object:
+  - "title" (string) - A short, human-readable summary of the problem type.
+  - "status" (number) - The HTTP status code generated by the origin server for this occurrence of the problem.
+  - "detail" (string) - A human-readable explanation specific to this occurrence of the problem.
+content:
+  application/problem+json:  # https://tools.ietf.org/html/rfc7807#section-6.1
+    schema:
+      type: object
+      required:
+        - title
+        - status
+        - detail
+      properties:
+        title:
+          type: string
+          description: A short, human-readable summary of the problem type.
+        status:
+          type: number
+          description: HTTP statuscode
+        detail:
+          type: string
+          description: A human-readable explanation specific to this occurrence of the problem.