From 5baa9489dcdaf8fa69b7af790f8e2d46108e3ced Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 21:17:26 +0000 Subject: [PATCH] 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 --- internal/api/roles.go | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/internal/api/roles.go b/internal/api/roles.go index e020909c..7c323b25 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -83,7 +83,16 @@ func (r *Router) roleGet(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { + // 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 } @@ -167,7 +176,14 @@ func (r *Router) roleDelete(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, roleResource); err != nil { + // 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 } @@ -204,15 +220,17 @@ func (r *Router) roleGetResource(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { - return 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, }