From aa570f4e48318a2b1cf74eb9c75d84c6581b1fe6 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 24 Mar 2021 15:33:04 -0400 Subject: [PATCH] Honor child scope list permissions when recursing (#1016) Current behavior is that you need list grant on the parent scope when recursively listing, and this is sufficient. This is good for administrative workflows but can be unintuitive for resources that only exist at a project level. This change allows permissions on the child scope to also be taken into account, so that you can start a recursive list from a parent scope without direct list support on that scope but still get back child scopes for which list access is granted. This also fixes a bug where visibility of a scope for recursive listing would be based on whether the user had list resources for roles within that scope instead of for the resource type in question. --- CHANGELOG.md | 10 +- internal/auth/auth.go | 24 +- internal/iam/options.go | 9 - internal/perms/acl.go | 11 +- internal/perms/acl_test.go | 110 +++--- internal/perms/grants.go | 2 +- .../controller/common/scopeids/scope_ids.go | 197 ++++++++--- .../common/scopeids/scope_ids_test.go | 315 ++++++++++++++++++ .../authmethods/authmethod_service.go | 18 +- .../handlers/authtokens/authtoken_service.go | 18 +- .../servers/controller/handlers/errors.go | 32 +- .../handlers/groups/group_service.go | 18 +- .../host_catalogs/host_catalog_service.go | 18 +- .../controller/handlers/roles/role_service.go | 18 +- .../handlers/scopes/scope_service.go | 18 +- .../handlers/sessions/session_service.go | 18 +- .../handlers/targets/target_service.go | 18 +- .../controller/handlers/users/user_service.go | 18 +- 18 files changed, 699 insertions(+), 173 deletions(-) create mode 100644 internal/servers/controller/common/scopeids/scope_ids_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e574110a4..47e23c809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,15 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. -## Pending +## Next + +### New and Improved + +* server: When performing recursive listing, `list` action is not longer + required to be granted to the calling user. Instead, the given scope acts as + the root point (so only results under that scope will be shown), and `list` + grant is evaluated per-scope. + [PR](https://github.com/hashicorp/boundary/pull/1016) ### Bug Fixes diff --git a/internal/auth/auth.go b/internal/auth/auth.go index fe11159dc..89ce07cb7 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -63,10 +63,11 @@ type RequestInfo struct { } type VerifyResults struct { - UserId string - AuthTokenId string - Error error - Scope *scopes.ScopeInfo + UserId string + AuthTokenId string + Error error + Scope *scopes.ScopeInfo + Authenticated bool // RoundTripValue can be set to allow the function performing authentication // (often accompanied by lookup(s)) to return a result of that lookup to the @@ -175,7 +176,8 @@ func Verify(ctx context.Context, opt ...Option) (ret VerifyResults) { } ret.AuthTokenId = v.requestInfo.PublicId - if !authResults.Allowed { + ret.Authenticated = authResults.Authenticated + if !authResults.Authorized { if v.requestInfo.DisableAuthzFailures { ret.Error = nil // TODO: Decide whether to remove this @@ -407,7 +409,8 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string // At this point we don't need to look up grants since it's automatically allowed if v.requestInfo.TokenFormat == AuthTokenTypeRecoveryKms { - aclResults.Allowed = true + aclResults.Authenticated = true + aclResults.Authorized = true retErr = nil return } @@ -442,6 +445,11 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string retAcl = perms.NewACL(parsedGrants...) aclResults = retAcl.Allowed(*v.res, v.act) + // We don't set authenticated above because setting this but not authorized + // is used for further permissions checks, such as during recursive listing. + // So we want to make sure any code relying on that has the full set of + // grants successfully loaded. + aclResults.Authenticated = true retErr = nil return } @@ -469,9 +477,11 @@ func (r *VerifyResults) fetchActions(ctx context.Context, id string, typ resourc opts := getOpts(opt...) res := opts.withResource + // If not passed in, use what's already been populated through verification if res == nil { res = r.v.res } + // If this is being called directly we may not have a resource yet if res == nil { res = new(perms.Resource) } @@ -484,7 +494,7 @@ func (r *VerifyResults) fetchActions(ctx context.Context, id string, typ resourc ret := make(action.ActionSet, 0, len(availableActions)) for _, act := range availableActions { - if r.v.acl.Allowed(*res, act).Allowed { + if r.v.acl.Allowed(*res, act).Authorized { ret = append(ret, act) } } diff --git a/internal/iam/options.go b/internal/iam/options.go index eb8d50132..ae0238e6e 100644 --- a/internal/iam/options.go +++ b/internal/iam/options.go @@ -19,7 +19,6 @@ type options struct { withPublicId string withName string withDescription string - withGroupGrants bool withLimit int withAutoVivify bool withGrantScopeId string @@ -36,7 +35,6 @@ func getDefaultOptions() options { withPublicId: "", withName: "", withDescription: "", - withGroupGrants: false, withLimit: 0, withAutoVivify: false, withGrantScopeId: "", @@ -44,13 +42,6 @@ func getDefaultOptions() options { } } -// WithGroupGrants provides and option to include group grants -func WithGroupGrants(enable bool) Option { - return func(o *options) { - o.withGroupGrants = enable - } -} - // WithPublicId provides an optional public id func WithPublicId(id string) Option { return func(o *options) { diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 59081e940..83c417153 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -30,7 +30,8 @@ type ACL struct { // pass more detailed information along in the future if we want. It was useful // in Vault, may be useful here. type ACLResults struct { - Allowed bool + Authenticated bool + Authorized bool // This is included but unexported for testing/debugging scopeMap map[string][]Grant @@ -102,7 +103,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { aType != action.List && aType != action.Create: - results.Allowed = true + results.Authorized = true return // type=;actions= when action is list(:self) or @@ -116,7 +117,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (aType == action.List || aType == action.Create): - results.Allowed = true + results.Authorized = true return // id=*;type=;actions= where type cannot be @@ -126,7 +127,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (grant.typ == r.Type || grant.typ == resource.All): - results.Allowed = true + results.Authorized = true return // id=;type=;actions= where type can be a @@ -137,7 +138,7 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { (grant.typ == r.Type || grant.typ == resource.All) && !topLevelType(r.Type): - results.Allowed = true + results.Authorized = true return } } diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index 8573f3ff1..2a1d96562 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -16,17 +16,17 @@ func Test_ACLAllowed(t *testing.T) { scope string grants []string } - type actionAllowed struct { - action action.Type - allowed bool + type actionAuthorized struct { + action action.Type + authorized bool } type input struct { - name string - scopeGrants []scopeGrant - resource Resource - actionsAllowed []actionAllowed - userId string - accountId string + name string + scopeGrants []scopeGrant + resource Resource + actionsAuthorized []actionAuthorized + userId string + accountId string } // A set of common grants to use in the following tests @@ -71,7 +71,7 @@ func Test_ACLAllowed(t *testing.T) { { name: "no grants", resource: Resource{ScopeId: "foo", Id: "bar", Type: resource.HostCatalog}, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Create}, {action: action.Read}, }, @@ -80,7 +80,7 @@ func Test_ACLAllowed(t *testing.T) { name: "no overlap", resource: Resource{ScopeId: "foo", Id: "bar", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Create}, {action: action.Read}, }, @@ -89,8 +89,8 @@ func Test_ACLAllowed(t *testing.T) { name: "top level create with type only", resource: Resource{ScopeId: "o_a", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Create, authorized: true}, {action: action.Delete}, }, }, @@ -98,7 +98,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id no matching action", resource: Resource{ScopeId: "o_a", Id: "a_foo", Type: resource.Role}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, }, @@ -107,9 +107,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and matching action", resource: Resource{ScopeId: "o_a", Id: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, {action: action.Delete}, }, }, @@ -117,17 +117,17 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and type and all action with valid pin", resource: Resource{ScopeId: "o_b", Pin: "mypin", Type: resource.Host}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, - {action: action.Delete, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, + {action: action.Delete, authorized: true}, }, }, { name: "matching scope and type and all action but bad pin", resource: Resource{ScopeId: "o_b", Pin: "notmypin", Type: resource.Host}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, {action: action.Update}, {action: action.Delete}, @@ -137,9 +137,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and some action", resource: Resource{ScopeId: "o_b", Id: "myhost", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, + {action: action.Create, authorized: true}, {action: action.AddHosts}, }, }, @@ -147,7 +147,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and id and all action but bad specifier", resource: Resource{ScopeId: "o_b", Id: "id_g"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, {action: action.Update}, {action: action.Delete}, @@ -157,7 +157,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and not matching type", resource: Resource{ScopeId: "o_a", Type: resource.HostCatalog}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, }, @@ -166,9 +166,9 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope and matching type", resource: Resource{ScopeId: "o_a", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, - {action: action.Create, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, + {action: action.Create, authorized: true}, {action: action.Delete}, }, }, @@ -176,7 +176,7 @@ func Test_ACLAllowed(t *testing.T) { name: "matching scope, type, action, random id and bad pin", resource: Resource{ScopeId: "o_a", Id: "anything", Type: resource.HostCatalog, Pin: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Update}, {action: action.Delete}, {action: action.Read}, @@ -186,7 +186,7 @@ func Test_ACLAllowed(t *testing.T) { name: "wrong scope and matching type", resource: Resource{ScopeId: "o_bad", Type: resource.HostSet}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Create}, {action: action.Delete}, @@ -196,9 +196,9 @@ func Test_ACLAllowed(t *testing.T) { name: "any id", resource: Resource{ScopeId: "o_b", Type: resource.AuthMethod}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, - {action: action.Authenticate, allowed: true}, + {action: action.Authenticate, authorized: true}, {action: action.Delete}, }, }, @@ -206,7 +206,7 @@ func Test_ACLAllowed(t *testing.T) { name: "bad templated user id", resource: Resource{ScopeId: "o_c"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Authenticate}, {action: action.Delete}, @@ -217,9 +217,9 @@ func Test_ACLAllowed(t *testing.T) { name: "good templated user id", resource: Resource{ScopeId: "o_c", Id: "u_abcd1234"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.Update, authorized: true}, }, userId: "u_abcd1234", }, @@ -227,7 +227,7 @@ func Test_ACLAllowed(t *testing.T) { name: "bad templated account id", resource: Resource{ScopeId: "o_c"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.List}, {action: action.Authenticate}, {action: action.Delete}, @@ -238,8 +238,8 @@ func Test_ACLAllowed(t *testing.T) { name: "good templated user id", resource: Resource{ScopeId: "o_c", Id: "apw_1234567890"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.ChangePassword, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.ChangePassword, authorized: true}, {action: action.Update}, }, accountId: "apw_1234567890", @@ -248,9 +248,9 @@ func Test_ACLAllowed(t *testing.T) { name: "all type", resource: Resource{ScopeId: "o_d", Type: resource.Account}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Create, allowed: true}, - {action: action.Update, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Create, authorized: true}, + {action: action.Update, authorized: true}, }, userId: "u_abcd1234", }, @@ -258,34 +258,34 @@ func Test_ACLAllowed(t *testing.T) { name: "list with top level list", resource: Resource{ScopeId: "o_a", Type: resource.Target}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, }, }, { name: "list sessions with wildcard actions", resource: Resource{ScopeId: "o_d", Type: resource.Session}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.List, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.List, authorized: true}, }, }, { name: "read self with top level read", resource: Resource{ScopeId: "o_a", Id: "a_bar"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Read, allowed: true}, - {action: action.ReadSelf, allowed: true}, + actionsAuthorized: []actionAuthorized{ + {action: action.Read, authorized: true}, + {action: action.ReadSelf, authorized: true}, }, }, { name: "read self only", resource: Resource{ScopeId: "o_a", Id: "a_baz"}, scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ + actionsAuthorized: []actionAuthorized{ {action: action.Read}, - {action: action.ReadSelf, allowed: true}, + {action: action.ReadSelf, authorized: true}, }, }, } @@ -301,8 +301,8 @@ func Test_ACLAllowed(t *testing.T) { } } acl := NewACL(grants...) - for _, aa := range test.actionsAllowed { - assert.True(t, acl.Allowed(test.resource, aa.action).Allowed == aa.allowed, "action: %s, acl allowed: %t, test action allowed: %t", aa.action, acl.Allowed(test.resource, aa.action).Allowed, aa.allowed) + for _, aa := range test.actionsAuthorized { + assert.True(t, acl.Allowed(test.resource, aa.action).Authorized == aa.authorized, "action: %s, acl authorized: %t, test action authorized: %t", aa.action, acl.Allowed(test.resource, aa.action).Authorized, aa.authorized) } }) } diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 7b98b734e..58f0e050b 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -362,7 +362,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) { var allowed bool for k := range grant.actions { results := acl.Allowed(r, k) - if results.Allowed { + if results.Authorized { allowed = true } } diff --git a/internal/servers/controller/common/scopeids/scope_ids.go b/internal/servers/controller/common/scopeids/scope_ids.go index b0e7446df..e634fcb2b 100644 --- a/internal/servers/controller/common/scopeids/scope_ids.go +++ b/internal/servers/controller/common/scopeids/scope_ids.go @@ -4,14 +4,18 @@ import ( "context" "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" + "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/servers/controller/common" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" + "github.com/hashicorp/boundary/internal/types/scope" ) -// GetScopeIds, given common parameters for List calls, returns the set of scope +// GetListingScopeIds, given common parameters for List calls, returns the set of scope // IDs in which to search for resources. It also returns a memoized map of the // scopes to their info for populating returned values. // @@ -19,7 +23,7 @@ import ( // tests in the other service handlers test this function extensively as it // forms the basis for all recursive listing tests; see those tests for list // functionality in the various service handlers. -func GetScopeIds( +func GetListingScopeIds( // The context to use when listing in the DB, if required ctx context.Context, // An IAM repo function to use for a listing call, if required @@ -29,63 +33,146 @@ func GetScopeIds( // The scope ID to use, or to use as the starting point for a recursive // search rootScopeId string, + // The type of resource we are listing + typ resource.Type, // Whether or not the search should be recursive - recursive bool) ([]string, map[string]*scopes.ScopeInfo, error) { - // Get the list of scope IDs. If it's recursive, check ACLs. - var scopeIds []string + recursive bool, + // Whether to only return scopes with exact permissions, or whether parent + // scopes with appropriate permissions are sufficient + directOnly bool) ([]string, map[string]*scopes.ScopeInfo, error) { + const op = "GetListingScopeIds" + + // Validation + switch { + case typ == resource.Unknown: + return nil, nil, errors.New(errors.InvalidParameter, op, "unknown resource") + case repoFn == nil: + return nil, nil, errors.New(errors.InvalidParameter, op, "nil iam repo") + case rootScopeId == "": + return nil, nil, errors.New(errors.InvalidParameter, op, "missing root scope id") + case authResults.Scope == nil: + return nil, nil, errors.New(errors.InvalidParameter, op, "nil scope in auth results") + } + + // Base case: if not recursive, return the scope we were given and the + // already-looked-up info + if !recursive { + return []string{rootScopeId}, map[string]*scopes.ScopeInfo{rootScopeId: authResults.Scope}, nil + } + // This will be used to memoize scope info so we can put the right scope // info for each returned value - scopeInfoMap := map[string]*scopes.ScopeInfo{ - rootScopeId: authResults.Scope, - } - switch recursive { - case true: - repo, err := repoFn() - if err != nil { - return nil, nil, err - } - // Get all scopes recursively - scps, err := repo.ListScopesRecursively(ctx, rootScopeId) - if err != nil { - return nil, nil, err - } - scopeIds = make([]string, 0, len(scps)) - res := perms.Resource{ - Type: resource.Role, - } - // For each scope, see if we have permission to list on that scope - for _, scp := range scps { - scpId := scp.GetPublicId() - // We already checked the incoming ID so we can definitely add it - if scpId == authResults.Scope.Id { - scopeIds = append(scopeIds, scp.GetPublicId()) - continue - } - res.ScopeId = scpId - aSet := authResults.FetchActionSetForType(ctx, - resource.Role, - action.ActionSet{action.List}, - auth.WithResource(&res), - ) - // We only passed one action in, so anything other than that one - // action back should not be included. Assuming it's correct, add - // the scope ID for lookup and memoize the scope info if needed. - if len(aSet) == 1 && aSet[0] == action.List { - scopeIds = append(scopeIds, scpId) - if scopeInfoMap[scpId] == nil { - scopeInfo := &scopes.ScopeInfo{ - Id: scp.GetPublicId(), - Type: scp.GetType(), - Name: scp.GetName(), - Description: scp.GetDescription(), - } - scopeInfoMap[scpId] = scopeInfo - } - } - } + scopeInfoMap := map[string]*scopes.ScopeInfo{} - default: - scopeIds = []string{rootScopeId} + repo, err := repoFn() + if err != nil { + return nil, nil, err + } + // Get all scopes recursively. Start at global because we need to take into + // account permissions in parent scopes even if they want to scale back the + // returned information to a child scope and its children. + scps, err := repo.ListScopesRecursively(ctx, scope.Global.String()) + if err != nil { + return nil, nil, err + } + + res := perms.Resource{ + Type: typ, + } + // For each scope, see if we have permission to list that type in that + // scope + var deferredScopes []*iam.Scope + // Store whether global has list permission + var globalHasList bool + for _, scp := range scps { + scpId := scp.GetPublicId() + res.ScopeId = scpId + aSet := authResults.FetchActionSetForType(ctx, + // This is overridden by WithResource + resource.Unknown, + action.ActionSet{action.List}, + auth.WithResource(&res), + ) + switch len(aSet) { + case 0: + // Defer until we've read all scopes. We do this because if the + // ordering coming back isn't in parent-first ording our map + // lookup might fail. + if !directOnly { + deferredScopes = append(deferredScopes, scp) + } + case 1: + if aSet[0] != action.List { + return nil, nil, errors.New(errors.Internal, op, "unexpected action in set") + } + if scopeInfoMap[scpId] == nil { + scopeInfo := &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + scopeInfoMap[scpId] = scopeInfo + } + if scpId == scope.Global.String() { + globalHasList = true + } + default: + return nil, nil, errors.New(errors.Internal, op, "unexpected number of actions back in set") + } + } + + // Now go through these and see if a parent matches + for _, scp := range deferredScopes { + // If they had list on global scope anything else is automatically + // included; otherwise if they had list on the parent scope, this + // scope is included in the map and is sufficient here. + if globalHasList || scopeInfoMap[scp.GetParentId()] != nil { + scpId := scp.GetPublicId() + if scopeInfoMap[scpId] == nil { + scopeInfo := &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + scopeInfoMap[scpId] = scopeInfo + } + } + } + + // If we have nothing in scopeInfoMap at this point, we aren't authorized + // anywhere so return 403. + if len(scopeInfoMap) == 0 { + return nil, nil, handlers.ForbiddenError() + } + + // Now elide out any that aren't under the root scope ID + elideScopes := make([]string, 0, len(scopeInfoMap)) + for scpId, scp := range scopeInfoMap { + switch rootScopeId { + // If the root is global, it matches + case scope.Global.String(): + // If the current scope matches the root, it matches + case scpId: + // Or if the parent of this scope is the root (for orgs that would mean + // a root scope ID which is covered in the case above, so this is really + // projects matching an org used as the root) + case scp.GetParentScopeId(): + default: + elideScopes = append(elideScopes, scpId) + } + } + + for _, scpId := range elideScopes { + delete(scopeInfoMap, scpId) + } + + scopeIds := make([]string, 0, len(scopeInfoMap)) + for k := range scopeInfoMap { + scopeIds = append(scopeIds, k) } return scopeIds, scopeInfoMap, nil diff --git a/internal/servers/controller/common/scopeids/scope_ids_test.go b/internal/servers/controller/common/scopeids/scope_ids_test.go new file mode 100644 index 000000000..71a9b940b --- /dev/null +++ b/internal/servers/controller/common/scopeids/scope_ids_test.go @@ -0,0 +1,315 @@ +package scopeids_test + +import ( + "context" + "testing" + + "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/authtoken" + "github.com/hashicorp/boundary/internal/db" + pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers" + "github.com/hashicorp/boundary/internal/servers/controller/handlers" + "github.com/hashicorp/boundary/internal/servers/controller/handlers/groups" + "github.com/hashicorp/boundary/internal/servers/controller/handlers/sessions" + "github.com/hashicorp/boundary/internal/session" + "github.com/hashicorp/boundary/internal/types/scope" + "github.com/stretchr/testify/require" +) + +// This test validates that we can perform recursive listing when there are no +// permissions on the parent scope against which the query is being run, but +// there are on child scopes. We use groups because groups are (one type of +// resource) valid in projects and we want to validate that the initial bugged +// behavior that used role permissions instead of the resource type under test +// is fixed. +func TestListingScopeIds(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrap := db.TestWrapper(t) + kms := kms.TestKms(t, conn, wrap) + iamRepo := iam.TestRepo(t, conn, wrap) + iamRepoFn := func() (*iam.Repository, error) { + return iamRepo, nil + } + authTokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*servers.Repository, error) { + return servers.NewRepository(rw, rw, kms) + } + s, err := groups.NewService(iamRepoFn) + require.NoError(t, err) + + sessionsRepoFn := func() (*session.Repository, error) { + return session.NewRepository(rw, rw, kms) + } + sess, err := sessions.NewService(sessionsRepoFn, iamRepoFn) + require.NoError(t, err) + + tcs := []struct { + name string + globalGrants []string + orgGrants []string + projGrants []string + globalGroups int + orgGroups int + projGroups int + users []string + wantErr error + expCount int + recurseFrom string + }{ + { + name: "no perms, start at global", + recurseFrom: "global", + wantErr: handlers.ForbiddenError(), + }, + { + name: "no perms, start at org", + recurseFrom: "org", + wantErr: handlers.ForbiddenError(), + }, + { + name: "no perms, start at project", + recurseFrom: "project", + wantErr: handlers.ForbiddenError(), + }, + { + name: "perms on global, no groups", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + recurseFrom: "global", + }, + { + name: "perms on org, no groups", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + recurseFrom: "org", + }, + { + name: "perms on project, no groups", + projGrants: []string{"id=*;type=group;actions=list,read"}, + recurseFrom: "project", + }, + { + name: "perms on global, none in org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 6, + recurseFrom: "global", + }, + { + name: "perms on global, none in org, start at org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "org", + }, + { + name: "perms on global, with org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 9, + recurseFrom: "global", + }, + { + name: "perms on global, with org, start at org", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "org", + }, + { + name: "perms on global, start at project", + globalGrants: []string{"id=*;type=group;actions=list,read"}, + orgGrants: []string{"id=*;type=group;actions=read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on org, start at global, no read on org", + orgGrants: []string{"id=*;type=group;actions=list"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "global", + }, + { + name: "perms on org, start at global, read on org", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "global", + }, + { + name: "perms on org, start at org", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 8, + recurseFrom: "org", + }, + { + name: "perms on org, start at project", + orgGrants: []string{"id=*;type=group;actions=list,read"}, + projGrants: []string{"id=*;type=group;actions=read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on proj", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "project", + }, + { + name: "perms on proj, start at org", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "org", + }, + { + name: "perms on proj, start at global", + projGrants: []string{"id=*;type=group;actions=list,read"}, + globalGroups: 1, + orgGroups: 3, + projGroups: 5, + expCount: 5, + recurseFrom: "global", + }, + } + // Each test starts with a new set of scopes and new users/roles, which + // ensures we don't clash and doesn't require us to deal with cleanup. Plus + // it means subsequent tests are ensuring that only the scopes that the + // current user should see are seen. + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + o, p := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, o.GetPublicId()) + ctx := auth.NewVerifierContext(context.Background(), + nil, + iamRepoFn, + authTokenRepoFn, + serversRepoFn, + kms, + auth.RequestInfo{ + Token: at.GetToken(), + TokenFormat: auth.AuthTokenTypeBearer, + PublicId: at.GetPublicId(), + }) + + // Clean up scopes between tests + defer func() { + _, err := iamRepo.DeleteScope(ctx, o.GetPublicId()) + require.NoError(err) + }() + + for i := 0; i < tc.globalGroups; i++ { + g := iam.TestGroup(t, conn, scope.Global.String()) + defer func() { + _, err := iamRepo.DeleteGroup(ctx, g.GetPublicId()) + require.NoError(err) + }() + } + for i := 0; i < tc.orgGroups; i++ { + iam.TestGroup(t, conn, o.GetPublicId()) + } + for i := 0; i < tc.projGroups; i++ { + iam.TestGroup(t, conn, p.GetPublicId()) + } + + for i, grants := range [][]string{tc.globalGrants, tc.orgGrants, tc.projGrants} { + if len(grants) > 0 { + pubId := scope.Global.String() + switch i { + case 1: + pubId = o.GetPublicId() + case 2: + pubId = p.GetPublicId() + } + role := iam.TestRole(t, conn, pubId) + // Clean up global between tests + if pubId == scope.Global.String() { + defer func() { + _, err := iamRepo.DeleteRole(ctx, role.GetPublicId()) + require.NoError(err) + }() + } + for _, grant := range grants { + iam.TestRoleGrant(t, conn, role.GetPublicId(), grant) + } + iam.TestUserRole(t, conn, role.GetPublicId(), at.GetIamUserId()) + } + } + var startScope string + switch tc.recurseFrom { + case "global": + startScope = scope.Global.String() + case "org": + startScope = o.GetPublicId() + case "project": + startScope = p.GetPublicId() + default: + t.Fatal("unknown start scope") + } + out, err := s.ListGroups(ctx, &pbs.ListGroupsRequest{ + Recursive: true, + ScopeId: startScope, + }) + if tc.wantErr != nil { + require.Error(err) + require.Nil(out) + require.Equal(tc.wantErr.Error(), err.Error()) + } else { + require.NoError(err) + require.NotNil(out) + require.Len(out.Items, tc.expCount) + } + + sessOut, err := sess.ListSessions(ctx, &pbs.ListSessionsRequest{ + Recursive: true, + ScopeId: startScope, + }) + require.NoError(err) + require.NotNil(sessOut) + }) + } +} diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index 73ad77007..7e9296e3c 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -128,14 +128,26 @@ func (s Service) ListAuthMethods(ctx context.Context, req *pbs.ListAuthMethodsRe } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.AuthMethod, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListAuthMethodsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/authtokens/authtoken_service.go b/internal/servers/controller/handlers/authtokens/authtoken_service.go index 4fcd1f1d7..671b82b29 100644 --- a/internal/servers/controller/handlers/authtokens/authtoken_service.go +++ b/internal/servers/controller/handlers/authtokens/authtoken_service.go @@ -61,14 +61,26 @@ func (s Service) ListAuthTokens(ctx context.Context, req *pbs.ListAuthTokensRequ } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.AuthToken, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListAuthTokensResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go index 8ea725c1b..357981e94 100644 --- a/internal/servers/controller/handlers/errors.go +++ b/internal/servers/controller/handlers/errors.go @@ -90,24 +90,28 @@ func NotFoundErrorf(msg string, a ...interface{}) error { } } +var unauthorizedError = &apiError{ + status: http.StatusForbidden, + inner: &pb.Error{ + Kind: codes.PermissionDenied.String(), + Message: "Forbidden.", + }, +} + func ForbiddenError() error { - return &apiError{ - status: http.StatusForbidden, - inner: &pb.Error{ - Kind: codes.PermissionDenied.String(), - Message: "Forbidden.", - }, - } + return unauthorizedError +} + +var unauthenticatedError = &apiError{ + status: http.StatusUnauthorized, + inner: &pb.Error{ + Kind: codes.Unauthenticated.String(), + Message: "Unauthenticated, or invalid token.", + }, } func UnauthenticatedError() error { - return &apiError{ - status: http.StatusUnauthorized, - inner: &pb.Error{ - Kind: codes.Unauthenticated.String(), - Message: "Unauthenticated, or invalid token.", - }, - } + return unauthenticatedError } func InvalidArgumentErrorf(msg string, fields map[string]string) error { diff --git a/internal/servers/controller/handlers/groups/group_service.go b/internal/servers/controller/handlers/groups/group_service.go index 5e7c4264d..7298fead4 100644 --- a/internal/servers/controller/handlers/groups/group_service.go +++ b/internal/servers/controller/handlers/groups/group_service.go @@ -75,14 +75,26 @@ func (s Service) ListGroups(ctx context.Context, req *pbs.ListGroupsRequest) (*p } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Group, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListGroupsResponse{}, nil + } gl, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go index 38ed3641b..0acd93c35 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go @@ -111,14 +111,26 @@ func (s Service) ListHostCatalogs(ctx context.Context, req *pbs.ListHostCatalogs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.HostCatalog, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListHostCatalogsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index df43e7a0e..a6f8f09f2 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -78,14 +78,26 @@ func (s Service) ListRoles(ctx context.Context, req *pbs.ListRolesRequest) (*pbs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Role, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListRolesResponse{}, nil + } items, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/scopes/scope_service.go b/internal/servers/controller/handlers/scopes/scope_service.go index 19e0f6f54..fbf37023a 100644 --- a/internal/servers/controller/handlers/scopes/scope_service.go +++ b/internal/servers/controller/handlers/scopes/scope_service.go @@ -133,14 +133,26 @@ func (s Service) ListScopes(ctx context.Context, req *pbs.ListScopesRequest) (*p } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.Scope, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListScopesResponse{}, nil + } pl, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/sessions/session_service.go b/internal/servers/controller/handlers/sessions/session_service.go index 4e09633f8..d89352a1a 100644 --- a/internal/servers/controller/handlers/sessions/session_service.go +++ b/internal/servers/controller/handlers/sessions/session_service.go @@ -100,12 +100,26 @@ func (s Service) ListSessions(ctx context.Context, req *pbs.ListSessionsRequest) authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds(ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds(ctx, + s.iamRepoFn, authResults, req.GetScopeId(), resource.Session, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListSessionsResponse{}, nil + } seslist, err := s.listFromRepo(ctx, session.WithScopeIds(scopeIds)) if err != nil { diff --git a/internal/servers/controller/handlers/targets/target_service.go b/internal/servers/controller/handlers/targets/target_service.go index 716f9f110..e7b6f6dde 100644 --- a/internal/servers/controller/handlers/targets/target_service.go +++ b/internal/servers/controller/handlers/targets/target_service.go @@ -122,14 +122,26 @@ func (s Service) ListTargets(ctx context.Context, req *pbs.ListTargetsRequest) ( } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.iamRepoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.iamRepoFn, authResults, req.GetScopeId(), resource.Target, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListTargetsResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil { diff --git a/internal/servers/controller/handlers/users/user_service.go b/internal/servers/controller/handlers/users/user_service.go index 3f5a41a71..15a68f5ae 100644 --- a/internal/servers/controller/handlers/users/user_service.go +++ b/internal/servers/controller/handlers/users/user_service.go @@ -76,14 +76,26 @@ func (s Service) ListUsers(ctx context.Context, req *pbs.ListUsersRequest) (*pbs } authResults := s.authResult(ctx, req.GetScopeId(), action.List) if authResults.Error != nil { - return nil, authResults.Error + // If it's forbidden, and it's a recursive request, and they're + // successfully authenticated but just not authorized, keep going as we + // may have authorization on downstream scopes. + if authResults.Error == handlers.ForbiddenError() && + req.GetRecursive() && + authResults.Authenticated { + } else { + return nil, authResults.Error + } } - scopeIds, scopeInfoMap, err := scopeids.GetScopeIds( - ctx, s.repoFn, authResults, req.GetScopeId(), req.GetRecursive()) + scopeIds, scopeInfoMap, err := scopeids.GetListingScopeIds( + ctx, s.repoFn, authResults, req.GetScopeId(), resource.User, req.GetRecursive(), false) if err != nil { return nil, err } + // If no scopes match, return an empty response + if len(scopeIds) == 0 { + return &pbs.ListUsersResponse{}, nil + } ul, err := s.listFromRepo(ctx, scopeIds) if err != nil {