RBAC V2 - Roles APIs (#245)

* add roles query and APIs Signed-off-by: Bailin He <[email protected]> * add changes per review Signed-off-by: Bailin He <[email protected]> * Apply suggestions from code review Co-authored-by: John Schaeffer <[email protected]> Signed-off-by: Bailin He <[email protected]> --------- Signed-off-by: Bailin He <[email protected]> Signed-off-by: Bailin He <[email protected]> Co-authored-by: John Schaeffer <[email protected]>
infratographer · Apr 22, 2024 · ca13cbc · ca13cbc
1 parent c71c129
commit ca13cbc
Show file tree

Hide file tree

Showing 25 changed files with 1,985 additions and 111 deletions.
diff --git a/Makefile b/Makefile
@@ -52,12 +52,12 @@ ci: | golint test coverage  ## Setup dev database and run tests.
 .PHONY: test
 test:  ## Runs unit tests.
 	@echo Running unit tests...
-	@go test -v -timeout 30s -cover -short -tags testtools ./...
+	@go test -v -timeout 120s -cover -short -tags testtools ./...
 
 .PHONY: coverage
 coverage:  ## Generates a test coverage report.
 	@echo Generating coverage report...
-	@go test -timeout 30s -tags testtools ./... -coverprofile=coverage.out -covermode=atomic
+	@go test -timeout 120s -tags testtools ./... -coverprofile=coverage.out -covermode=atomic
 	@go tool cover -func=coverage.out
 	@go tool cover -html=coverage.out
 

diff --git a/cmd/server.go b/cmd/server.go
@@ -21,9 +21,7 @@ import (
 	"go.infratographer.com/permissions-api/internal/storage"
 )
 
-var (
-	apiDefaultListen = "0.0.0.0:7602"
-)
+var apiDefaultListen = "0.0.0.0:7602"
 
 var serverCmd = &cobra.Command{
 	Use:   "server",
@@ -89,7 +87,7 @@ func serve(_ context.Context, cfg *config.AppConfig) {
 		logger.Fatalw("invalid spicedb policy", "error", err)
 	}
 
-	engine, err := query.NewEngine("infratographer", spiceClient, kv, store, query.WithPolicy(policy))
+	engine, err := query.NewEngine("infratographer", spiceClient, kv, store, query.WithPolicy(policy), query.WithLogger(logger))
 	if err != nil {
 		logger.Fatalw("error creating engine", "error", err)
 	}

diff --git a/internal/api/assignments.go b/internal/api/assignments.go
@@ -6,6 +6,7 @@ import (
 
 	"go.infratographer.com/x/gidx"
 
+	"go.infratographer.com/permissions-api/internal/iapl"
 	"go.infratographer.com/permissions-api/internal/query"
 	"go.infratographer.com/permissions-api/internal/types"
 
@@ -62,7 +63,7 @@ func (r *Router) assignmentCreate(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionUpdate), resource); err != nil {
 		return err
 	}
 
@@ -112,7 +113,7 @@ func (r *Router) assignmentsList(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionGet), resource); err != nil {
 		return err
 	}
 
@@ -190,7 +191,7 @@ func (r *Router) assignmentDelete(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionUpdate), resource); err != nil {
 		return err
 	}
 

diff --git a/internal/api/assignments_test.go b/internal/api/assignments_test.go
@@ -42,7 +42,7 @@ func TestAssignmentCreate(t *testing.T) {
 					"subject_id": "bad-id",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -67,7 +67,7 @@ func TestAssignmentCreate(t *testing.T) {
 					"subject_id": "idntusr-abc123",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -92,7 +92,7 @@ func TestAssignmentCreate(t *testing.T) {
 					"subject_id": "idntusr-def456",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -119,7 +119,7 @@ func TestAssignmentCreate(t *testing.T) {
 					"subject_id": "idntusr-def456",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -211,7 +211,7 @@ func TestAssignmentsList(t *testing.T) {
 		{
 			Name:  "RoleResourceNotFound",
 			Input: "/api/v1/roles/permrol-abc123/assignments",
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -233,7 +233,7 @@ func TestAssignmentsList(t *testing.T) {
 		{
 			Name:  "AssignmentsRetrieved",
 			Input: "/api/v1/roles/permrol-abc123/assignments",
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -327,7 +327,7 @@ func TestAssignmentDelete(t *testing.T) {
 					"subject_id": "bad-id",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -352,7 +352,7 @@ func TestAssignmentDelete(t *testing.T) {
 					"subject_id": "idntusr-abc123",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -377,7 +377,7 @@ func TestAssignmentDelete(t *testing.T) {
 					"subject_id": "idntusr-def456",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}
@@ -404,7 +404,7 @@ func TestAssignmentDelete(t *testing.T) {
 					"subject_id": "idntusr-def456",
 				},
 			},
-			SetupFn: func(ctx context.Context, t *testing.T) context.Context {
+			SetupFn: func(ctx context.Context, _ *testing.T) context.Context {
 				engine := mock.Engine{
 					Namespace: "test",
 				}

diff --git a/internal/api/errors.go b/internal/api/errors.go
@@ -0,0 +1,10 @@
+package api
+
+import "errors"
+
+var (
+	// ErrInvalidID is returned when the ID is invalid
+	ErrInvalidID = errors.New("invalid ID")
+	// ErrParsingRequestBody is returned when failing to parse the request body
+	ErrParsingRequestBody = errors.New("error parsing request body")
+)
diff --git a/internal/api/response.go b/internal/api/response.go
@@ -2,6 +2,15 @@ package api
 
 import (
 	"errors"
+	"fmt"
+	"net/http"
+
+	"github.com/labstack/echo/v4"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
+	"go.infratographer.com/permissions-api/internal/query"
+	"go.infratographer.com/permissions-api/internal/storage"
 )
 
 // ErrorResponse represents the data that the server will return on any given call
@@ -20,3 +29,33 @@ var (
 	// ErrResourceAlreadyExists is returned when the resource already exists
 	ErrResourceAlreadyExists = errors.New("resource already exists")
 )
+
+func (r *Router) errorResponse(basemsg string, err error) *echo.HTTPError {
+	msg := fmt.Sprintf("%s: %s", basemsg, err.Error())
+	httpstatus := http.StatusInternalServerError
+
+	switch {
+	case
+		errors.Is(err, query.ErrInvalidType),
+		errors.Is(err, query.ErrInvalidArgument),
+		errors.Is(err, query.ErrInvalidAction),
+		errors.Is(err, query.ErrInvalidNamespace),
+		errors.Is(err, ErrInvalidID),
+		status.Code(err) == codes.InvalidArgument,
+		status.Code(err) == codes.FailedPrecondition:
+		httpstatus = http.StatusBadRequest
+	case
+		errors.Is(err, storage.ErrNoRoleFound),
+		errors.Is(err, query.ErrRoleNotFound),
+		errors.Is(err, query.ErrRoleBindingNotFound):
+		httpstatus = http.StatusNotFound
+	case
+		errors.Is(err, storage.ErrRoleAlreadyExists),
+		errors.Is(err, storage.ErrRoleNameTaken):
+		httpstatus = http.StatusConflict
+	default:
+		msg = basemsg
+	}
+
+	return echo.NewHTTPError(httpstatus, msg).SetInternal(err)
+}
diff --git a/internal/api/roles.go b/internal/api/roles.go
@@ -3,25 +3,19 @@ package api
 import (
 	"errors"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/labstack/echo/v4"
 	"go.infratographer.com/x/gidx"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 
+	"go.infratographer.com/permissions-api/internal/iapl"
 	"go.infratographer.com/permissions-api/internal/query"
 	"go.infratographer.com/permissions-api/internal/storage"
 )
 
-const (
-	actionRoleCreate = "role_create"
-	actionRoleGet    = "role_get"
-	actionRoleList   = "role_list"
-	actionRoleUpdate = "role_update"
-	actionRoleDelete = "role_delete"
-)
-
 func (r *Router) roleCreate(c echo.Context) error {
 	resourceIDStr := c.Param("id")
 
@@ -50,11 +44,14 @@ func (r *Router) roleCreate(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleCreate, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionCreate), resource); err != nil {
 		return err
 	}
 
-	role, err := r.engine.CreateRole(ctx, subjectResource, resource, reqBody.Name, reqBody.Actions)
+	role, err := r.engine.CreateRole(
+		ctx, subjectResource, resource,
+		strings.TrimSpace(reqBody.Name), reqBody.Actions,
+	)
 
 	switch {
 	case err == nil:
@@ -120,11 +117,14 @@ func (r *Router) roleUpdate(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionUpdate), resource); err != nil {
 		return err
 	}
 
-	role, err := r.engine.UpdateRole(ctx, subjectResource, roleResource, reqBody.Name, reqBody.Actions)
+	role, err := r.engine.UpdateRole(
+		ctx, subjectResource, roleResource,
+		strings.TrimSpace(reqBody.Name), reqBody.Actions,
+	)
 
 	switch {
 	case err == nil:
@@ -185,7 +185,7 @@ func (r *Router) roleGet(c echo.Context) error {
 
 	// TODO: This shows an error for the role's resource, not the role. Determine if that
 	// matters.
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionGet), resource); err != nil {
 		return err
 	}
 
@@ -234,7 +234,7 @@ func (r *Router) rolesList(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleList, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionList), resource); err != nil {
 		return err
 	}
 
@@ -297,7 +297,7 @@ func (r *Router) roleDelete(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionDelete), resource); err != nil {
 		return err
 	}
 
@@ -351,7 +351,7 @@ func (r *Router) roleGetResource(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
 	}
 
-	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil {
+	if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionGet), resource); err != nil {
 		return err
 	}