Role permissions checks (#170)

* Refactor logic for getting current subject and checking permissions

Some functions, like currentSubject and checkAction, have logic that
is either duplicated or could be reused elsewhere for things like
checking access to actions in permissions-api itself. This commit
moves some of that logic around so it is easier for other
handlers (such as the handlers for roles) to use.

Signed-off-by: John Schaeffer <[email protected]>

* Add permissions checks for role creation

This commit adds permissions checks for role creation, as well as the
action and bindings to the example policy.

Signed-off-by: John Schaeffer <[email protected]>

* Add create-role command to bootstrap permissions-api

This commit adds a command to create roles directly in SpiceDB,
bypassing permissions checks. The intent of this command is to
bootstrap a new permissions-api deployment with enough access to start
provisioning roles using some subject.

Signed-off-by: John Schaeffer <[email protected]>

* Add permissions checks for other role operations

This commit adds permissions checks for getting, listing, updating,
and deleting roles.

Signed-off-by: John Schaeffer <[email protected]>

* Fix linting whitespace issue in checkActionWithResponse

Signed-off-by: John Schaeffer <[email protected]>

* Check role permissions based on the resource, not the role

One of the quirks of our current role model is that roles don't belong
to a resource outright - instead, their binding to a resource is
inferred by the actions that can be performed. This means that we
can't use the role itself to make authorization decisions. This commit
updates permissions checks for roles to use the role's resource rather
than the role itself for checking permissions.

Signed-off-by: John Schaeffer <[email protected]>

---------

Signed-off-by: John Schaeffer <[email protected]>

cmd/createrole.go

@@ -0,0 +1,111 @@
+package cmd
+
+import (
+	"context"
+
+	"github.com/spf13/cobra"
+	"github.com/spf13/viper"
+	"go.infratographer.com/permissions-api/internal/config"
+	"go.infratographer.com/permissions-api/internal/iapl"
+	"go.infratographer.com/permissions-api/internal/query"
+	"go.infratographer.com/permissions-api/internal/spicedbx"
+	"go.infratographer.com/x/gidx"
+	"go.infratographer.com/x/viperx"
+)
+
+const (
+	createRoleFlagSubject  = "subject"
+	createRoleFlagResource = "resource"
+	createRoleFlagActions  = "actions"
+)
+
+var (
+	createRoleCmd = &cobra.Command{
+		Use:   "create-role",
+		Short: "create role in SpiceDB directly",
+		Run: func(cmd *cobra.Command, args []string) {
+			createRole(cmd.Context(), globalCfg)
+		},
+	}
+)
+
+func init() {
+	rootCmd.AddCommand(createRoleCmd)
+
+	flags := createRoleCmd.Flags()
+	flags.String(createRoleFlagSubject, "", "subject to assign to created role")
+	flags.StringSlice(createRoleFlagActions, []string{}, "actions to assign to created role")
+	flags.String(createRoleFlagResource, "", "resource to bind to created role")
+
+	v := viper.GetViper()
+
+	viperx.MustBindFlag(v, createRoleFlagSubject, flags.Lookup(createRoleFlagSubject))
+	viperx.MustBindFlag(v, createRoleFlagActions, flags.Lookup(createRoleFlagActions))
+	viperx.MustBindFlag(v, createRoleFlagResource, flags.Lookup(createRoleFlagResource))
+}
+
+func createRole(ctx context.Context, cfg *config.AppConfig) {
+	subjectIDStr := viper.GetString(createRoleFlagSubject)
+	actions := viper.GetStringSlice(createRoleFlagActions)
+	resourceIDStr := viper.GetString(createRoleFlagResource)
+
+	if subjectIDStr == "" || len(actions) == 0 || resourceIDStr == "" {
+		logger.Fatal("invalid config")
+	}
+
+	spiceClient, err := spicedbx.NewClient(cfg.SpiceDB, cfg.Tracing.Enabled)
+	if err != nil {
+		logger.Fatalw("unable to initialize spicedb client", "error", err)
+	}
+
+	var policy iapl.Policy
+
+	if cfg.SpiceDB.PolicyFile != "" {
+		policy, err = iapl.NewPolicyFromFile(cfg.SpiceDB.PolicyFile)
+		if err != nil {
+			logger.Fatalw("unable to load new policy from schema file", "policy_file", cfg.SpiceDB.PolicyFile, "error", err)
+		}
+	} else {
+		logger.Warn("no spicedb policy file defined, using default policy")
+
+		policy = iapl.DefaultPolicy()
+	}
+
+	if err = policy.Validate(); err != nil {
+		logger.Fatalw("invalid spicedb policy", "error", err)
+	}
+
+	resourceID, err := gidx.Parse(resourceIDStr)
+	if err != nil {
+		logger.Fatalw("error parsing resource ID", "error", err)
+	}
+
+	subjectID, err := gidx.Parse(subjectIDStr)
+	if err != nil {
+		logger.Fatalw("error parsing subject ID", "error", err)
+	}
+
+	engine := query.NewEngine("infratographer", spiceClient, query.WithPolicy(policy), query.WithLogger(logger))
+
+	resource, err := engine.NewResourceFromID(resourceID)
+	if err != nil {
+		logger.Fatalw("error creating resource", "error", err)
+	}
+
+	subjectResource, err := engine.NewResourceFromID(subjectID)
+	if err != nil {
+		logger.Fatalw("error creating subject resource", "error", err)
+	}
+
+	role, _, err := engine.CreateRole(ctx, resource, actions)
+	if err != nil {
+		logger.Fatalw("error creating role", "error", err)
+	}
+
+	_, err = engine.AssignSubjectRole(ctx, subjectResource, role)
+	if err != nil {
+		logger.Fatalw("error creating role", "error", err)
+	}
+
+	logger.Infow("role successfully created", "role_id", role.ID)
+}

internal/api/permissions.go

@@ -67,28 +67,37 @@ func (r *Router) checkAction(c echo.Context) error {
 	}
 
 	// Subject validation
-	subject, err := currentSubject(c)
+	subjectResource, err := r.currentSubject(c)
 	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err)
+		return err
 	}
 
-	subjectResource, err := r.engine.NewResourceFromID(subject)
-	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err)
+	// Check the permissions
+	if err := r.checkActionWithResponse(ctx, subjectResource, action, resource); err != nil {
+		return err
 	}
 
-	// Check the permissions
-	err = r.engine.SubjectHasPermission(ctx, subjectResource, action, resource)
-	if err != nil && errors.Is(err, query.ErrActionNotAssigned) {
-		msg := fmt.Sprintf("subject '%s' does not have permission to perform action '%s' on resource '%s'",
-			subject, action, resourceIDStr)
+	return c.JSON(http.StatusOK, echo.Map{})
+}
+
+func (r *Router) checkActionWithResponse(ctx context.Context, subjectResource types.Resource, action string, resource types.Resource) error {
+	err := r.engine.SubjectHasPermission(ctx, subjectResource, action, resource)
+
+	switch {
+	case errors.Is(err, query.ErrActionNotAssigned):
+		msg := fmt.Sprintf(
+			"subject '%s' does not have permission to perform action '%s' on resource '%s'",
+			subjectResource.ID.String(),
+			action,
+			resource.ID.String(),
+		)
 
 		return echo.NewHTTPError(http.StatusForbidden, msg).SetInternal(err)
-	} else if err != nil {
+	case err != nil:
 		return echo.NewHTTPError(http.StatusInternalServerError, "an error occurred checking permissions").SetInternal(err)
+	default:
+		return nil
 	}
-
-	return c.JSON(http.StatusOK, echo.Map{})
 }
 
 type checkPermissionsRequest struct {
@@ -124,14 +133,9 @@ func (r *Router) checkAllActions(c echo.Context) error {
 	defer span.End()
 
 	// Subject validation
-	subject, err := currentSubject(c)
-	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err)
-	}
-
-	subjectResource, err := r.engine.NewResourceFromID(subject)
+	subjectResource, err := r.currentSubject(c)
 	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err)
+		return err
 	}
 
 	var reqBody checkPermissionsRequest
@@ -223,8 +227,13 @@ func (r *Router) checkAllActions(c echo.Context) error {
 		case result := <-resultsCh:
 			if result.Error != nil {
 				if errors.Is(result.Error, query.ErrActionNotAssigned) {
-					err := fmt.Errorf("%w: subject '%s' does not have permission to perform action '%s' on resource '%s'",
-						ErrAccessDenied, subject, result.Request.Action, result.Request.Resource.ID.String())
+					err := fmt.Errorf(
+						"%w: subject '%s' does not have permission to perform action '%s' on resource '%s'",
+						ErrAccessDenied,
+						subjectResource.ID,
+						result.Request.Action,
+						result.Request.Resource.ID,
+					)
 
 					unauthorizedErrors++
 
@@ -254,7 +263,10 @@ func (r *Router) checkAllActions(c echo.Context) error {
 	}
 
 	if unauthorizedErrors != 0 {
-		msg := fmt.Sprintf("subject '%s' does not have permission to the requested resource actions", subject)
+		msg := fmt.Sprintf(
+			"subject '%s' does not have permission to the requested resource actions",
+			subjectResource.ID,
+		)
 
 		return echo.NewHTTPError(http.StatusForbidden, msg).SetInternal(multierr.Combine(allErrors...))
 	}

internal/api/roles.go

@@ -9,6 +9,14 @@ import (
 	"go.opentelemetry.io/otel/trace"
 )
 
+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")
 
@@ -27,11 +35,20 @@ func (r *Router) roleCreate(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error parsing request body").SetInternal(err)
 	}
 
+	subjectResource, err := r.currentSubject(c)
+	if err != nil {
+		return err
+	}
+
 	resource, err := r.engine.NewResourceFromID(resourceID)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
 	}
 
+	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleCreate, resource); err != nil {
+		return err
+	}
+
 	role, _, err := r.engine.CreateRole(ctx, resource, reqBody.Actions)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error creating resource").SetInternal(err)
@@ -56,11 +73,29 @@ func (r *Router) roleGet(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
 	}
 
+	subjectResource, err := r.currentSubject(c)
+	if err != nil {
+		return err
+	}
+
 	roleResource, err := r.engine.NewResourceFromID(roleResourceID)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
 	}
 
+	// Roles belong to resources by way of the actions they can perform; do the permissions
+	// check on the role resource.
+	resource, err := r.engine.GetRoleResource(ctx, roleResource, "")
+	if err != nil {
+		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
+	}
+
+	// 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 {
+		return err
+	}
+
 	role, err := r.engine.GetRole(ctx, roleResource, "")
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
@@ -85,11 +120,20 @@ func (r *Router) rolesList(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error parsing resource ID").SetInternal(err)
 	}
 
+	subjectResource, err := r.currentSubject(c)
+	if err != nil {
+		return err
+	}
+
 	resource, err := r.engine.NewResourceFromID(resourceID)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
 	}
 
+	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleList, resource); err != nil {
+		return err
+	}
+
 	roles, err := r.engine.ListRoles(ctx, resource, "")
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err)
@@ -122,11 +166,27 @@ func (r *Router) roleDelete(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err)
 	}
 
+	subjectResource, err := r.currentSubject(c)
+	if err != nil {
+		return err
+	}
+
 	roleResource, err := r.engine.NewResourceFromID(roleResourceID)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err)
 	}
 
+	// Roles belong to resources by way of the actions they can perform; do the permissions
+	// check on the role resource.
+	resource, err := r.engine.GetRoleResource(ctx, roleResource, "")
+	if err != nil {
+		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
+	}
+
+	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, resource); err != nil {
+		return err
+	}
+
 	_, err = r.engine.DeleteRole(ctx, roleResource, "")
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error deleting resource").SetInternal(err)
@@ -150,16 +210,27 @@ func (r *Router) roleGetResource(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
 	}
 
+	subjectResource, err := r.currentSubject(c)
+	if err != nil {
+		return err
+	}
+
 	roleResource, err := r.engine.NewResourceFromID(roleResourceID)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
 	}
 
+	// There's a little irony here in that getting a role's resource here is required to actually
+	// do the permissions check.
 	resource, err := r.engine.GetRoleResource(ctx, roleResource, "")
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
 	}
 
+	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil {
+		return err
+	}
+
 	resp := resourceResponse{
 		ID: resource.ID,
 	}

internal/api/router.go

@@ -2,9 +2,11 @@ package api
 
 import (
 	"context"
+	"net/http"
 
 	"github.com/labstack/echo/v4"
 	"go.infratographer.com/permissions-api/internal/query"
+	"go.infratographer.com/permissions-api/internal/types"
 	"go.infratographer.com/x/echojwtx"
 	"go.infratographer.com/x/gidx"
 	"go.opentelemetry.io/otel"
@@ -97,8 +99,18 @@ func WithCheckConcurrency(count int) Option {
 	}
 }
 
-func currentSubject(c echo.Context) (gidx.PrefixedID, error) {
-	subject := echojwtx.Actor(c)
+func (r *Router) currentSubject(c echo.Context) (types.Resource, error) {
+	subjectStr := echojwtx.Actor(c)
 
-	return gidx.Parse(subject)
+	subject, err := gidx.Parse(subjectStr)
+	if err != nil {
+		return types.Resource{}, echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err)
+	}
+
+	subjectResource, err := r.engine.NewResourceFromID(subject)
+	if err != nil {
+		return types.Resource{}, echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err)
+	}
+
+	return subjectResource, nil
 }

permissions-api.example.yaml

@@ -1,3 +1,4 @@
 oidc:
   issuer: http://mock-oauth2-server:8081/default
-
+spicedb:
+  policyFile: /workspace/policy.example.yaml