Apply suggestions from code review

Co-authored-by: John Schaeffer <[email protected]>
Signed-off-by: Bailin He <[email protected]>

internal/api/roles.go

@@ -3,6 +3,7 @@ package api
 import (
 	"errors"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/labstack/echo/v4"
@@ -47,7 +48,10 @@ func (r *Router) roleCreate(c echo.Context) error {
 		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:
@@ -117,7 +121,10 @@ func (r *Router) roleUpdate(c echo.Context) error {
 		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:

internal/api/roles_v2.go

@@ -3,6 +3,7 @@ package api
 import (
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"go.infratographer.com/permissions-api/internal/iapl"
@@ -45,7 +46,10 @@ func (r *Router) roleV2Create(c echo.Context) error {
 		return err
 	}
 
-	role, err := r.engine.CreateRoleV2(ctx, subjectResource, resource, reqBody.Name, reqBody.Actions)
+	role, err := r.engine.CreateRoleV2(
+		ctx, subjectResource, resource,
+		strings.TrimSpace(reqBody.Name), reqBody.Actions,
+	)
 	if err != nil {
 		return r.errorResponse("error creating resource", err)
 	}
@@ -98,7 +102,10 @@ func (r *Router) roleV2Update(c echo.Context) error {
 		return err
 	}
 
-	role, err := r.engine.UpdateRoleV2(ctx, subjectResource, roleResource, reqBody.Name, reqBody.Actions)
+	role, err := r.engine.UpdateRoleV2(
+		ctx, subjectResource, roleResource,
+		strings.TrimSpace(reqBody.Name), reqBody.Actions,
+	)
 	if err != nil {
 		return r.errorResponse("error updating role", err)
 	}

internal/iapl/rbac.go

@@ -47,15 +47,15 @@ type RoleBindingAction string
 
 const (
 	// RoleBindingActionCreate is the action name to create a role binding
-	RoleBindingActionCreate RoleBindingAction = "rolebinding_create"
+	RoleBindingActionCreate RoleBindingAction = "iam_rolebinding_create"
 	// RoleBindingActionUpdate is the action name to update a role binding
-	RoleBindingActionUpdate RoleBindingAction = "rolebinding_update"
+	RoleBindingActionUpdate RoleBindingAction = "iam_rolebinding_update"
 	// RoleBindingActionDelete is the action name to delete a role binding
-	RoleBindingActionDelete RoleBindingAction = "rolebinding_delete"
+	RoleBindingActionDelete RoleBindingAction = "iam_rolebinding_delete"
 	// RoleBindingActionGet is the action name to get a role binding
-	RoleBindingActionGet RoleBindingAction = "rolebinding_get"
+	RoleBindingActionGet RoleBindingAction = "iam_rolebinding_get"
 	// RoleBindingActionList is the action name to list role bindings
-	RoleBindingActionList RoleBindingAction = "rolebinding_list"
+	RoleBindingActionList RoleBindingAction = "iam_rolebinding_list"
 )
 
 // ResourceRoleBindingV2 describes the relationships that will be created

internal/query/errors.go

@@ -31,8 +31,8 @@ var (
 	// ErrResourceNotFound represents an error when no matching resource was found
 	ErrResourceNotFound = errors.New("resource not found")
 
-	// ErrRoleBindingNotFound represents an error when no matching role-binding was found
-	ErrRoleBindingNotFound = errors.New("role-binding not found")
+	// ErrRoleBindingNotFound represents an error when no matching role binding was found
+	ErrRoleBindingNotFound = errors.New("role binding not found")
 
 	// ErrRoleHasTooManyResources represents an error which a role has too many resources
 	ErrRoleHasTooManyResources = errors.New("role has too many resources")

internal/query/relations.go

@@ -313,8 +313,6 @@ func (e *engine) CreateRole(ctx context.Context, actor, res types.Resource, role
 		return types.Role{}, err
 	}
 
-	roleName = strings.TrimSpace(roleName)
-
 	role := newRole(roleName, actions)
 	roleRels := e.roleRelationships(role, res)
 
@@ -443,8 +441,6 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou
 		return types.Role{}, err
 	}
 
-	newName = strings.TrimSpace(newName)
-
 	if newName == "" {
 		newName = role.Name
 	}

internal/query/roles_v2.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"strings"
 
 	pb "github.com/authzed/authzed-go/proto/authzed/api/v1"
 	"go.infratographer.com/x/gidx"
@@ -28,8 +27,6 @@ func (e *engine) CreateRoleV2(ctx context.Context, actor, owner types.Resource,
 
 	defer span.End()
 
-	roleName = strings.TrimSpace(roleName)
-
 	role, err := newRoleWithPrefix(e.schemaTypeMap[e.rbac.RoleResource.Name].IDPrefix, roleName, actions)
 	if err != nil {
 		return types.Role{}, err
@@ -142,9 +139,7 @@ func (e *engine) ListRolesV2(ctx context.Context, owner types.Resource) ([]types
 			break
 		}
 
-		subj := lookup.GetSubject()
-
-		id, err := gidx.Parse(subj.SubjectObjectId)
+		id, err := gidx.Parse(lookup.Subject.SubjectObjectId)
 		if err != nil {
 			span.RecordError(err)
 			span.SetStatus(codes.Error, err.Error())
@@ -179,7 +174,7 @@ func (e *engine) GetRoleV2(ctx context.Context, role types.Resource) (types.Role
 	ctx, span := e.tracer.Start(
 		ctx,
 		"engine.GetRoleV2",
-		trace.WithAttributes(attribute.Stringer("role", role.ID)),
+		trace.WithAttributes(attribute.Stringer("permissions.role_id", role.ID)),
 	)
 	defer span.End()
 
@@ -193,33 +188,24 @@ func (e *engine) GetRoleV2(ctx context.Context, role types.Resource) (types.Role
 	}
 
 	// 1. Get role actions from spice DB
-	spicedbctx, listRoleV2ActionsSpan := e.tracer.Start(ctx, "listRoleV2Actions")
-	defer listRoleV2ActionsSpan.End()
 
-	actions, err := e.listRoleV2Actions(spicedbctx, types.Role{ID: role.ID})
+	actions, err := e.listRoleV2Actions(ctx, types.Role{ID: role.ID})
 	if err != nil {
-		listRoleV2ActionsSpan.RecordError(err)
-		listRoleV2ActionsSpan.SetStatus(codes.Error, err.Error())
+		span.RecordError(err)
+		span.SetStatus(codes.Error, err.Error())
 
 		return types.Role{}, err
 	}
 
-	listRoleV2ActionsSpan.End()
-
 	// 2. Get role info (name, created_by, etc.) from permissions API DB
-	apidbctx, getRoleFromPermissionAPISpan := e.tracer.Start(ctx, "getRoleFromPermissionAPI")
-	defer getRoleFromPermissionAPISpan.End()
-
-	dbrole, err := e.store.GetRoleByID(apidbctx, role.ID)
+	dbrole, err := e.store.GetRoleByID(ctx, role.ID)
 	if err != nil {
-		getRoleFromPermissionAPISpan.RecordError(err)
-		getRoleFromPermissionAPISpan.SetStatus(codes.Error, err.Error())
+		span.RecordError(err)
+		span.SetStatus(codes.Error, err.Error())
 
 		return types.Role{}, err
 	}
 
-	getRoleFromPermissionAPISpan.End()
-
 	resp := types.Role{
 		ID:      dbrole.ID,
 		Name:    dbrole.Name,
@@ -266,8 +252,6 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 		return types.Role{}, err
 	}
 
-	newName = strings.TrimSpace(newName)
-
 	if newName == "" {
 		newName = role.Name
 	}
@@ -279,12 +263,14 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 		if err = e.store.CommitContext(dbCtx); err != nil {
 			span.RecordError(err)
 			span.SetStatus(codes.Error, err.Error())
+
+			return types.Role{}, err
 		}
 
 		return role, nil
 	}
 
-	// 1. update role in permission-api DB
+	// 1. update role in permissions-api DB
 	dbRole, err := e.store.UpdateRole(dbCtx, actor.ID, role.ID, newName)
 	if err != nil {
 		span.RecordError(err)
@@ -295,7 +281,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 		return types.Role{}, err
 	}
 
-	// 2. update permissions relationships in spice db
+	// 2. update permissions relationships in SpiceDB
 	updates := []*pb.RelationshipUpdate{}
 	roleRef := resourceToSpiceDBRef(e.namespace, roleResource)
 
@@ -321,7 +307,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 		)
 	}
 
-	// 2.c write updates to spicedb
+	// 2.c write updates to SpiceDB
 	request := &pb.WriteRelationshipsRequest{Updates: updates}
 
 	if _, err := e.client.WriteRelationships(ctx, request); err != nil {
@@ -339,10 +325,10 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
 
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
-		// At this point, spicedb changes have already been applied.
+		// At this point, SpiceDB changes have already been applied.
 		// Attempting to rollback could result in failures that could result in the same situation.
 		//
-		// TODO: add spicedb rollback logic along with rollback failure scenarios.
+		// TODO: add SpiceDB rollback logic along with rollback failure scenarios.
 
 		return types.Role{}, err
 	}
@@ -462,6 +448,12 @@ func (e *engine) DeleteRoleV2(ctx context.Context, roleResource types.Resource)
 		// At this point, spicedb changes have already been applied.
 		// Attempting to rollback could result in failures that could result in the same situation.
 		//
+		// In this particular case, in the absence of spiceDB rollback, the expected
+		// behavior in this case would be that the role only exists in the permissions-api
+		// DB and not in spiceDB. This would result in the role being "orphaned"
+		// (lack of ownership in spiceDB will prevent this role from being accessed or bind
+		// by any user), and would need to be cleaned up manually.
+		//
 		// TODO: add spicedb rollback logic along with rollback failure scenarios.
 
 		return err