From dc74e725d4b6a271509ef2069df927ac255d339e Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 27 Sep 2024 12:24:33 -0400 Subject: [PATCH] Some clarity updates to comments in ACL code (#5136) --- internal/perms/acl.go | 46 +++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 2f6037e001..694c6cff88 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -235,7 +235,7 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option if len(split) == 2 { parentAction = action.Map[split[0]] } - // Now, go through and check the cases indicated above + // Now, go through and check whether grants match for _, grant := range grants { var outputFieldsOnly bool switch { @@ -262,8 +262,9 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option continue } - // We step through all grants, to fetch the full list of output fields. - // However, we shortcut if we find *. + // We step through all grants in order to fetch the full list of output + // fields, even if we find a match. However, we shortcut if we find * + // for output fields, which is the default. // // If the action was not found above but we did find output fields in // patterns that match, we do not authorize the request, but we do build @@ -300,10 +301,11 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option found = true } - // Case 2: - // id=;actions= where ID cannot be a wildcard; or - // id=;output_fields= where fields cannot be a - // wildcard. + // Case 2: id=;actions= where ID cannot be a + // wildcard. Type is optional but if present must match the resource. + // This will also allow matching an id with specific output fields + // (handled later). Cannot be a list or create action as those do not + // operate on specific IDs, only types. case grant.Id == r.Id && grant.Id != "" && grant.Id != "*" && @@ -314,20 +316,19 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option found = true // Case 3: type=;actions= when action is list or - // create (cannot be a wildcard). Must be a top level collection, - // otherwise must be one of the two formats specified in cases 4 or 5. - // Or, type=resource.Type;output_fields= and no action. This is - // more of a semantic difference compared to 4 more than a security - // difference; this type is for clarity as it ties more closely to the - // concept of create and list as actions on a collection, operating on a - // collection directly. The format in case 4 will still work for - // create/list on collections but that's more of a shortcut to allow - // things like id=*;type=*;actions=* for admin flows so that you don't - // need to separate out explicit collection actions into separate typed - // grants for each collection within a role. This does mean there are - // "two ways of doing things" but it's a reasonable UX tradeoff given - // that "all IDs" can reasonably be construed to include "and the one - // I'm making" and "all of them for listing". + // create (cannot be a wildcard). Must be a top level collection; if not + // it's handled in cases 4 or 5. This is more of a semantic difference + // compared to case 4 more than a security difference; this type is for + // clarity as it ties more closely to the concept of create and list as + // actions on a collection, operating on a collection directly. The + // format in case 4 will still work for create/list on collections but + // that's more of a shortcut to allow things like id=*;type=*;actions=* + // for admin flows so that you don't need to separate out explicit + // collection actions into separate typed grants for each collection + // within a role. This does mean there are "two ways of doing things" + // but it's a reasonable UX tradeoff given that "all IDs" can reasonably + // be construed to include "and the one I'm making" and "all of them for + // listing". case grant.Id == "" && r.Id == "" && grant.Type == r.Type && @@ -351,8 +352,7 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option // Case 5: // id=;type=;actions= where type can be a - // wildcard and this this is operating on a non-top-level type. Same for - // output fields only. + // wildcard and this this is operating on a non-top-level type. case grant.Id != "" && grant.Id == r.Pin && grant.Type != resource.Unknown &&