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.
This commit is contained in:
Jeff Mitchell authored and 
GitHub committed
1 parent
79283d93ac
commit
aa570f4e48
18 files changed
+699
-173
No files matched your search
@@ -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
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user