Increase role http error and test quality (#222)

* check action against policy on access check

This updates access check requests to ensure the requested action exists
in the policy for the provided resource beforce executing the request to
spicedb and failing for this error.

Knowing this we can return a relevant error (ErrInvalidAction) which we
can now handle and return a 400 Bad Request status code instead of the
previous 500 Internal Server Error status code we were returning due to
the spicedb error we were receiving before.

Signed-off-by: Mike Mason <[email protected]>

* handle context canceled request 500 errors

Context Canceled errors from http requests should not produce 500 errors
as those are client requests.
This change adds an error middleware which can capture the canceled
context error and return a 422 status instead.

Signed-off-by: Mike Mason <[email protected]>

* validate create/update role actions requests

This validates create/update role action requests before submitting the
request to spicedb and handles the errors gracefully by returning 400
errors instead of generic 500 errors to the client.

Signed-off-by: Mike Mason <[email protected]>

* handle role http errors better and add http tests

This handles role http endpoint errors better by better handling the
errors returned and setting the response status codes.

Additionally this adds testing to the http api so we may test these http
status codes to ensure they are working as we expect.

Signed-off-by: Mike Mason <[email protected]>

* fix and add tests for role assignment endpoints

This adds http tests and fixes the http status codes for the role
assignment and unassignment endpoints to resolve 500 errors occurring
when provided role ids were didn't exist.

Signed-off-by: Mike Mason <[email protected]>

---------

Signed-off-by: Mike Mason <[email protected]>

.devcontainer/Dockerfile

@@ -1,5 +1,5 @@
 # [Choice] Go version (use -bullseye variants on local arm64/Apple Silicon): 1, 1.18, 1.17, 1-bullseye, 1.18-bullseye, 1.17-bullseye, 1-buster, 1.18-buster, 1.17-buster
-FROM mcr.microsoft.com/vscode/devcontainers/go:1-1.20-bullseye
+FROM mcr.microsoft.com/vscode/devcontainers/go:1-1.22-bullseye
 
 # [Choice] Node.js version: none, lts/*, 16, 14, 12, 10
 ARG NODE_VERSION="none"

go.mod

@@ -1,11 +1,14 @@
 module go.infratographer.com/permissions-api
 
-go 1.20
+go 1.22
 
 require (
 	github.com/authzed/authzed-go v0.10.1
 	github.com/authzed/grpcutil v0.0.0-20240123194739-2ea1e3d2d98b
 	github.com/cockroachdb/cockroach-go/v2 v2.3.6
+	github.com/go-jose/go-jose/v4 v4.0.1
+	github.com/golang-jwt/jwt v3.2.2+incompatible
+	github.com/labstack/echo-jwt/v4 v4.2.0
 	github.com/labstack/echo/v4 v4.11.4
 	github.com/lib/pq v1.10.9
 	github.com/nats-io/nats.go v1.31.0
@@ -22,6 +25,7 @@ require (
 	go.opentelemetry.io/otel/trace v1.16.0
 	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.26.0
+	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
 	google.golang.org/grpc v1.60.1
 	gopkg.in/yaml.v3 v3.0.1
 )
@@ -40,7 +44,6 @@ require (
 	github.com/go-logr/logr v1.2.4 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
 	github.com/gofrs/flock v0.8.1 // indirect
-	github.com/golang-jwt/jwt v3.2.2+incompatible // indirect
 	github.com/golang-jwt/jwt/v5 v5.0.0 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect
@@ -60,7 +63,6 @@ require (
 	github.com/jzelinskie/stringz v0.0.2 // indirect
 	github.com/klauspost/compress v1.17.2 // indirect
 	github.com/labstack/echo-contrib v0.15.0 // indirect
-	github.com/labstack/echo-jwt/v4 v4.2.0 // indirect
 	github.com/labstack/gommon v0.4.2 // indirect
 	github.com/magiconair/properties v1.8.7 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
@@ -99,10 +101,9 @@ require (
 	go.opentelemetry.io/otel/metric v1.16.0 // indirect
 	go.opentelemetry.io/otel/sdk v1.16.0 // indirect
 	go.opentelemetry.io/proto/otlp v0.19.0 // indirect
-	golang.org/x/crypto v0.17.0 // indirect
-	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
+	golang.org/x/crypto v0.19.0 // indirect
 	golang.org/x/net v0.19.0 // indirect
-	golang.org/x/sys v0.15.0 // indirect
+	golang.org/x/sys v0.17.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	golang.org/x/time v0.5.0 // indirect
 	google.golang.org/genproto v0.0.0-20231106174013-bbf56f31fb17 // indirect

internal/api/assignments.go

@@ -1,10 +1,12 @@
 package api
 
 import (
+	"errors"
 	"net/http"
 
 	"go.infratographer.com/x/gidx"
 
+	"go.infratographer.com/permissions-api/internal/query"
 	"go.infratographer.com/permissions-api/internal/types"
 
 	"github.com/labstack/echo/v4"
@@ -37,7 +39,7 @@ func (r *Router) assignmentCreate(c echo.Context) error {
 
 	assigneeResource, err := r.engine.NewResourceFromID(assigneeID)
 	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
+		return echo.NewHTTPError(http.StatusBadRequest, "error assigning subject").SetInternal(err)
 	}
 
 	subjectResource, err := r.currentSubject(c)
@@ -51,8 +53,13 @@ func (r *Router) assignmentCreate(c echo.Context) error {
 	}
 
 	resource, err := r.engine.GetRoleResource(ctx, roleResource)
-	if err != nil {
-		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
+
+	switch {
+	case err == nil:
+	case errors.Is(err, query.ErrRoleNotFound):
+		return echo.NewHTTPError(http.StatusNotFound, "role not found").SetInternal(err)
+	default:
+		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
 	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {
@@ -96,8 +103,13 @@ func (r *Router) assignmentsList(c echo.Context) error {
 	}
 
 	resource, err := r.engine.GetRoleResource(ctx, roleResource)
-	if err != nil {
-		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
+
+	switch {
+	case err == nil:
+	case errors.Is(err, query.ErrRoleNotFound):
+		return echo.NewHTTPError(http.StatusNotFound, "role not found").SetInternal(err)
+	default:
+		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
 	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil {
@@ -169,8 +181,13 @@ func (r *Router) assignmentDelete(c echo.Context) error {
 	}
 
 	resource, err := r.engine.GetRoleResource(ctx, roleResource)
-	if err != nil {
-		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
+
+	switch {
+	case err == nil:
+	case errors.Is(err, query.ErrRoleNotFound):
+		return echo.NewHTTPError(http.StatusNotFound, "role not found").SetInternal(err)
+	default:
+		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
 	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {