diff --git a/internal/api/roles.go b/internal/api/roles.go index 1bae7cbe..2e9895e3 100644 --- a/internal/api/roles.go +++ b/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: diff --git a/internal/api/roles_v2.go b/internal/api/roles_v2.go index 8b0ff331..27ac65d7 100644 --- a/internal/api/roles_v2.go +++ b/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) } diff --git a/internal/iapl/rbac.go b/internal/iapl/rbac.go index fa802ce6..13c0815e 100644 --- a/internal/iapl/rbac.go +++ b/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 diff --git a/internal/query/errors.go b/internal/query/errors.go index 21b27e1d..6f21394e 100644 --- a/internal/query/errors.go +++ b/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") diff --git a/internal/query/relations.go b/internal/query/relations.go index 28624a7b..ddbfbc7b 100644 --- a/internal/query/relations.go +++ b/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 } diff --git a/internal/query/roles_v2.go b/internal/query/roles_v2.go index dd5fb5cf..e5e08aa8 100644 --- a/internal/query/roles_v2.go +++ b/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