Skip to content

Commit

Permalink
add changes per review
Browse files Browse the repository at this point in the history 
Signed-off-by: Bailin He <[email protected]>
  • Loading branch information
@bailinhe
bailinhe committed Apr 18, 2024
1 parent 34551eb commit c357346
Show file tree
Hide file tree
Showing 12 changed files with 268 additions and 229 deletions.
7 changes: 4 additions & 3 deletions internal/api/assignments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
10 changes: 10 additions & 0 deletions internal/api/errors.go
View file
Original file line number Diff line number Diff line change
@@ -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")
)
8 changes: 7 additions & 1 deletion internal/api/response.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ func (r *Router) errorResponse(basemsg string, err error) *echo.HTTPError {

switch {
case
errors.Is(err, storage.ErrRoleNameTaken),
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
Expand All @@ -47,6 +49,10 @@ func (r *Router) errorResponse(basemsg string, err error) *echo.HTTPError {
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
}
Expand Down
21 changes: 7 additions & 14 deletions internal/api/roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,11 @@ import (
"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")

Expand Down Expand Up @@ -50,7 +43,7 @@ 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
}

Expand Down Expand Up @@ -120,7 +113,7 @@ 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
}

Expand Down Expand Up @@ -185,7 +178,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
}

Expand Down Expand Up @@ -234,7 +227,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
}

Expand Down Expand Up @@ -297,7 +290,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
}

Expand Down Expand Up @@ -351,7 +344,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
}

Expand Down
81 changes: 32 additions & 49 deletions internal/api/roles_v2.go
View file
Original file line number Diff line number Diff line change
@@ -1,35 +1,34 @@
package api

import (
"errors"
"fmt"
"net/http"
"time"

"go.infratographer.com/permissions-api/internal/iapl"

"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/query"
"go.infratographer.com/permissions-api/internal/storage"
)

func (r *Router) roleV2Create(c echo.Context) error {
resourceIDStr := c.Param("id")

ctx, span := tracer.Start(c.Request().Context(), "api.roleCreate", trace.WithAttributes(attribute.String("id", resourceIDStr)))
ctx, span := tracer.Start(c.Request().Context(), "api.roleV2Create", trace.WithAttributes(attribute.String("id", resourceIDStr)))
defer span.End()

resourceID, err := gidx.Parse(resourceIDStr)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error parsing resource ID").SetInternal(err)
return r.errorResponse("error parsing resource ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
}

var reqBody createRoleRequest

err = c.Bind(&reqBody)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error parsing request body").SetInternal(err)
return r.errorResponse(err.Error(), ErrParsingRequestBody)
}

subjectResource, err := r.currentSubject(c)
Expand All @@ -39,23 +38,16 @@ func (r *Router) roleV2Create(c echo.Context) error {

resource, err := r.engine.NewResourceFromID(resourceID)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
return r.errorResponse("error creating resource", 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.CreateRoleV2(ctx, subjectResource, resource, reqBody.Name, reqBody.Actions)

switch {
case err == nil:
case errors.Is(err, query.ErrInvalidAction):
return echo.NewHTTPError(http.StatusBadRequest, "error creating resource: "+err.Error())
case errors.Is(err, storage.ErrRoleAlreadyExists), errors.Is(err, storage.ErrRoleNameTaken):
return echo.NewHTTPError(http.StatusConflict, "error creating resource: "+err.Error())
default:
return echo.NewHTTPError(http.StatusInternalServerError, "error creating resource").SetInternal(err)
if err != nil {
return r.errorResponse("error creating resource", err)
}

resp := roleResponse{
Expand All @@ -75,19 +67,19 @@ func (r *Router) roleV2Create(c echo.Context) error {
func (r *Router) roleV2Update(c echo.Context) error {
roleIDStr := c.Param("role_id")

ctx, span := tracer.Start(c.Request().Context(), "api.roleUpdate", trace.WithAttributes(attribute.String("id", roleIDStr)))
ctx, span := tracer.Start(c.Request().Context(), "api.roleV2Update", trace.WithAttributes(attribute.String("id", roleIDStr)))
defer span.End()

roleID, err := gidx.Parse(roleIDStr)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error parsing role ID").SetInternal(err)
return r.errorResponse("error parsing resource ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
}

var reqBody updateRoleRequest

err = c.Bind(&reqBody)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error parsing request body").SetInternal(err)
return r.errorResponse(err.Error(), ErrParsingRequestBody)
}

subjectResource, err := r.currentSubject(c)
Expand All @@ -99,12 +91,10 @@ func (r *Router) roleV2Update(c echo.Context) error {
// on the roles themselves.
roleResource, err := r.engine.NewResourceFromID(roleID)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
return r.errorResponse("error creating resource", 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, roleResource); err != nil {
if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionGet), roleResource); err != nil {
return err
}

Expand All @@ -130,12 +120,12 @@ func (r *Router) roleV2Update(c echo.Context) error {
func (r *Router) roleV2Get(c echo.Context) error {
roleIDStr := c.Param("role_id")

ctx, span := tracer.Start(c.Request().Context(), "api.roleGet", trace.WithAttributes(attribute.String("id", roleIDStr)))
ctx, span := tracer.Start(c.Request().Context(), "api.roleV2Get", trace.WithAttributes(attribute.String("id", roleIDStr)))
defer span.End()

roleResourceID, err := gidx.Parse(roleIDStr)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
return r.errorResponse("error parsing resource ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
}

subjectResource, err := r.currentSubject(c)
Expand All @@ -147,12 +137,10 @@ func (r *Router) roleV2Get(c echo.Context) error {
// on the roles themselves.
roleResource, err := r.engine.NewResourceFromID(roleResourceID)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
return r.errorResponse("error creating resource", 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, roleResource); err != nil {
if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionGet), roleResource); err != nil {
return err
}

Expand All @@ -178,12 +166,12 @@ func (r *Router) roleV2Get(c echo.Context) error {
func (r *Router) roleV2sList(c echo.Context) error {
resourceIDStr := c.Param("id")

ctx, span := tracer.Start(c.Request().Context(), "api.rolesList", trace.WithAttributes(attribute.String("id", resourceIDStr)))
ctx, span := tracer.Start(c.Request().Context(), "api.roleV2sList", trace.WithAttributes(attribute.String("id", resourceIDStr)))
defer span.End()

resourceID, err := gidx.Parse(resourceIDStr)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error parsing resource ID").SetInternal(err)
return r.errorResponse("error parsing resource ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
}

subjectResource, err := r.currentSubject(c)
Expand All @@ -193,10 +181,10 @@ func (r *Router) roleV2sList(c echo.Context) error {

resource, err := r.engine.NewResourceFromID(resourceID)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err)
return r.errorResponse("error creating resource", 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
}

Expand All @@ -205,19 +193,14 @@ func (r *Router) roleV2sList(c echo.Context) error {
return r.errorResponse("error getting roles", err)
}

resp := listRolesResponse{
Data: []roleResponse{},
resp := listRolesV2Response{
Data: []listRolesV2Role{},
}

for _, role := range roles {
roleResp := roleResponse{
ID: role.ID,
Name: role.Name,
Actions: role.Actions,
CreatedBy: role.CreatedBy,
UpdatedBy: role.UpdatedBy,
CreatedAt: role.CreatedAt.Format(time.RFC3339),
UpdatedAt: role.UpdatedAt.Format(time.RFC3339),
roleResp := listRolesV2Role{
ID: role.ID,
Name: role.Name,
}

resp.Data = append(resp.Data, roleResp)
Expand All @@ -229,12 +212,12 @@ func (r *Router) roleV2sList(c echo.Context) error {
func (r *Router) roleV2Delete(c echo.Context) error {
roleIDStr := c.Param("id")

ctx, span := tracer.Start(c.Request().Context(), "api.roleDelete", trace.WithAttributes(attribute.String("id", roleIDStr)))
ctx, span := tracer.Start(c.Request().Context(), "api.roleV2Delete", trace.WithAttributes(attribute.String("id", roleIDStr)))
defer span.End()

roleResourceID, err := gidx.Parse(roleIDStr)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err)
return r.errorResponse("error parsing resource ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
}

subjectResource, err := r.currentSubject(c)
Expand All @@ -246,10 +229,10 @@ func (r *Router) roleV2Delete(c echo.Context) error {
// on the roles themselves.
roleResource, err := r.engine.NewResourceFromID(roleResourceID)
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
return r.errorResponse("error creating resource", err)
}

if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, roleResource); err != nil {
if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleActionDelete), roleResource); err != nil {
return err
}

Expand Down
9 changes: 9 additions & 0 deletions internal/api/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,12 @@ type assignmentItem struct {
type listAssignmentsResponse struct {
Data []assignmentItem `json:"data"`
}

type listRolesV2Response struct {
Data []listRolesV2Role `json:"data"`
}

type listRolesV2Role struct {
ID gidx.PrefixedID `json:"id"`
Name string `json:"name"`
}
16 changes: 16 additions & 0 deletions internal/iapl/rbac.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,22 @@ const (
GrantRelationship = "grant"
)

// RoleAction is the list of actions that can be performed on a role resource
type RoleAction string

const (
// RoleActionCreate is the action name to create a role
RoleActionCreate RoleAction = "role_create"
// RoleActionGet is the action name to get a role
RoleActionGet RoleAction = "role_get"
// RoleActionList is the action name to list roles
RoleActionList RoleAction = "role_list"
// RoleActionUpdate is the action name to update a role
RoleActionUpdate RoleAction = "role_update"
// RoleActionDelete is the action name to delete a role
RoleActionDelete RoleAction = "role_delete"
)

// RoleBindingAction is the list of actions that can be performed on a role-binding resource
type RoleBindingAction string

Expand Down
Loading

0 comments on commit c357346

Please sign in to comment.