From bf263fbd7db2cfbe2c98868bc6547e086fe31d5d Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Tue, 23 Aug 2022 17:32:36 +0100 Subject: [PATCH] perf(target): Use new Permissions object to determine resource access Using the new Permissions object, we can build a list of the level of access a user should have based on the grants they've been given, meaining we don't need to fetch all resources from the database to then remove the ones the user shouldn't be able to see. This commit also removes some unused options in `ListTargets` for the sake of simplicity. --- .../handlers/targets/target_service.go | 53 +++--- internal/target/repository.go | 93 ++++------ internal/target/repository_test.go | 172 ++++++++++++++++++ internal/target/tcp/repository_test.go | 131 +++---------- 4 files changed, 259 insertions(+), 190 deletions(-) diff --git a/internal/daemon/controller/handlers/targets/target_service.go b/internal/daemon/controller/handlers/targets/target_service.go index f5c45dfc2f..f2b603e83a 100644 --- a/internal/daemon/controller/handlers/targets/target_service.go +++ b/internal/daemon/controller/handlers/targets/target_service.go @@ -15,7 +15,6 @@ import ( "github.com/hashicorp/boundary/internal/credential/vault" "github.com/hashicorp/boundary/internal/daemon/controller/auth" "github.com/hashicorp/boundary/internal/daemon/controller/common" - "github.com/hashicorp/boundary/internal/daemon/controller/common/scopeids" "github.com/hashicorp/boundary/internal/daemon/controller/handlers" "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/errors" @@ -33,6 +32,7 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "github.com/hashicorp/boundary/internal/types/subtypes" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/scopes" pb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/targets" "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-secure-stdlib/strutil" @@ -183,34 +183,24 @@ func (s Service) ListTargets(ctx context.Context, req *pbs.ListTargetsRequest) ( } } - repo, err := s.repoFn() - if err != nil { - return nil, errors.Wrap(ctx, err, op) + var err error + var authzScopes map[string]*scopes.ScopeInfo + if req.GetRecursive() { + authzScopes, err = authResults.ScopesAuthorizedForList(ctx, req.GetScopeId(), resource.Target) + } else { + authzScopes = map[string]*scopes.ScopeInfo{authResults.Scope.Id: authResults.Scope} } - - scopeResourceInfo, err := scopeids.GetListingResourceInformation( - ctx, - scopeids.GetListingResourceInformationInput{ - IamRepoFn: s.iamRepoFn, - AuthResults: authResults, - RootScopeId: req.GetScopeId(), - Type: resource.Target, - Recursive: req.GetRecursive(), - AuthzProtectedEntityProvider: repo, - ActionSet: IdActions, - }, - ) if err != nil { return nil, err } - // If no scopes match, return an empty response - if len(scopeResourceInfo.ScopeIds) == 0 || - len(scopeResourceInfo.ResourceIds) == 0 { + // Get all user permissions for the requested scope(s). + userPerms := authResults.ACL().ListPermissions(authzScopes, resource.Target, IdActions) + if len(userPerms) == 0 { return &pbs.ListTargetsResponse{}, nil } - tl, err := s.listFromRepo(ctx, scopeResourceInfo.ResourceIds) + tl, err := s.listFromRepo(ctx, userPerms) if err != nil { return nil, err } @@ -222,19 +212,21 @@ func (s Service) ListTargets(ctx context.Context, req *pbs.ListTargetsRequest) ( if err != nil { return nil, err } + finalItems := make([]*pb.Target, 0, len(tl)) - res := perms.Resource{ - Type: resource.Target, - } for _, item := range tl { - outputFields := authResults.FetchOutputFields(res, action.List).SelfOrDefaults(authResults.UserId) + pr := perms.Resource{Id: item.GetPublicId(), ScopeId: item.GetProjectId(), Type: resource.Target} + outputFields := authResults.FetchOutputFields(pr, action.List).SelfOrDefaults(authResults.UserId) + outputOpts := make([]handlers.Option, 0, 3) outputOpts = append(outputOpts, handlers.WithOutputFields(&outputFields)) + if outputFields.Has(globals.ScopeField) { - outputOpts = append(outputOpts, handlers.WithScope(scopeResourceInfo.ScopeResourceMap[item.GetProjectId()].ScopeInfo)) + outputOpts = append(outputOpts, handlers.WithScope(authzScopes[item.GetProjectId()])) } if outputFields.Has(globals.AuthorizedActionsField) { - outputOpts = append(outputOpts, handlers.WithAuthorizedActions(scopeResourceInfo.ScopeResourceMap[item.GetProjectId()].Resources[item.GetPublicId()].AuthorizedActions.Strings())) + authorizedActions := authResults.FetchActionSetForId(ctx, item.GetPublicId(), IdActions, auth.WithResource(&pr)).Strings() + outputOpts = append(outputOpts, handlers.WithAuthorizedActions(authorizedActions)) } item, err := toProto(ctx, item, nil, nil, outputOpts...) @@ -250,6 +242,7 @@ func (s Service) ListTargets(ctx context.Context, req *pbs.ListTargetsRequest) ( finalItems = append(finalItems, item) } } + return &pbs.ListTargetsResponse{Items: finalItems}, nil } @@ -1233,12 +1226,12 @@ func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) { return rows > 0, nil } -func (s Service) listFromRepo(ctx context.Context, targetIds []string) ([]target.Target, error) { - repo, err := s.repoFn() +func (s Service) listFromRepo(ctx context.Context, perms []perms.Permission) ([]target.Target, error) { + repo, err := s.repoFn(target.WithPermissions(perms)) if err != nil { return nil, err } - ul, err := repo.ListTargets(ctx, target.WithTargetIds(targetIds)) + ul, err := repo.ListTargets(ctx) if err != nil { return nil, err } diff --git a/internal/target/repository.go b/internal/target/repository.go index 07594e92d3..7ec5d1b2df 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" "github.com/hashicorp/boundary/internal/perms" + "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "github.com/hashicorp/go-dbw" @@ -212,59 +213,36 @@ func (r *Repository) FetchAuthzProtectedEntitiesByScope(ctx context.Context, pro return targetsMap, nil } -// ListTargets in targets in a project. Supports the WithProjectId, WithLimit, WithType options. +// ListTargets lists targets in a project based on the data in the WithPermissions option +// provided to the Repository constructor. If no permissions are available, this function +// is a no-op. +// Supports WithLimit which overrides the limit set in the Repository object. func (r *Repository) ListTargets(ctx context.Context, opt ...Option) ([]Target, error) { const op = "target.(Repository).ListTargets" - opts := GetOpts(opt...) - if len(opts.WithProjectIds) == 0 && opts.WithUserId == "" && len(opts.WithTargetIds) == 0 { - return nil, errors.New(ctx, errors.InvalidParameter, op, "must specify either project ids, target ids, or user id") - } - // TODO (jimlambrt 8/2020) - implement WithUserId() optional filtering. - var where []string - var args []interface{} - inClauseCnt := 0 - switch len(opts.WithProjectIds) { - case 0: - case 1: - inClauseCnt += 1 - where, args = append(where, fmt.Sprintf("project_id = @%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), opts.WithProjectIds[0])) - default: - idsInClause := make([]string, 0, len(opts.WithProjectIds)) - for _, id := range opts.WithProjectIds { - inClauseCnt += 1 - idsInClause, args = append(idsInClause, fmt.Sprintf("@%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), id)) - } - where = append(where, fmt.Sprintf("project_id in (%s)", strings.Join(idsInClause, ","))) + if len(r.permissions) == 0 { + return []Target{}, nil } - switch len(opts.WithTargetIds) { - case 0: - case 1: - inClauseCnt += 1 - where, args = append(where, fmt.Sprintf("public_id = @%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), opts.WithTargetIds[0])) - default: - idsInClause := make([]string, 0, len(opts.WithTargetIds)) - for _, id := range opts.WithTargetIds { - inClauseCnt += 1 - idsInClause, args = append(idsInClause, fmt.Sprintf("@%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), id)) - } - where = append(where, fmt.Sprintf("public_id in (%s)", strings.Join(idsInClause, ","))) + where, args := r.listPermissionWhereClauses() + if len(where) == 0 { + return []Target{}, nil } - if opts.WithType != "" { - inClauseCnt += 1 - where, args = append(where, fmt.Sprintf("type = @%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), opts.WithType.String())) + opts := GetOpts(opt...) + limit := r.defaultLimit + if opts.WithLimit != 0 { + limit = opts.WithLimit } var foundTargets []*targetView - err := r.list(ctx, &foundTargets, strings.Join(where, " and "), args, opt...) + err := r.reader.SearchWhere(ctx, &foundTargets, strings.Join(where, " or "), args, + db.WithLimit(limit)) if err != nil { return nil, errors.Wrap(ctx, err, op) } targets := make([]Target, 0, len(foundTargets)) - for _, t := range foundTargets { subtype, err := t.targetSubtype(ctx) if err != nil { @@ -272,25 +250,34 @@ func (r *Repository) ListTargets(ctx context.Context, opt ...Option) ([]Target, } targets = append(targets, subtype) } + return targets, nil } -// list will return a listing of resources and honor the WithLimit option or the -// repo defaultLimit -func (r *Repository) list(ctx context.Context, resources interface{}, where string, args []interface{}, opt ...Option) error { - const op = "target.(Repository).list" - opts := GetOpts(opt...) - limit := r.defaultLimit - var dbOpts []db.Option - if opts.WithLimit != 0 { - // non-zero signals an override of the default limit for the repo. - limit = opts.WithLimit - } - dbOpts = append(dbOpts, db.WithLimit(limit)) - if err := r.reader.SearchWhere(ctx, resources, where, args, dbOpts...); err != nil { - return errors.Wrap(ctx, err, op) +func (r *Repository) listPermissionWhereClauses() ([]string, []interface{}) { + var where []string + var args []interface{} + + inClauseCnt := 0 + for _, p := range r.permissions { + if p.Action != action.List { + continue + } + inClauseCnt++ + + var clauses []string + clauses = append(clauses, fmt.Sprintf("project_id = @project_id_%d", inClauseCnt)) + args = append(args, sql.Named(fmt.Sprintf("project_id_%d", inClauseCnt), p.ScopeId)) + + if len(p.ResourceIds) > 0 { + clauses = append(clauses, fmt.Sprintf("public_id = any(@public_id_%d)", inClauseCnt)) + args = append(args, sql.Named(fmt.Sprintf("public_id_%d", inClauseCnt), "{"+strings.Join(p.ResourceIds, ",")+"}")) + } + + where = append(where, fmt.Sprintf("(%s)", strings.Join(clauses, " and "))) } - return nil + + return where, args } // DeleteTarget will delete a target from the repository. diff --git a/internal/target/repository_test.go b/internal/target/repository_test.go index e26b06c4f5..8468756f24 100644 --- a/internal/target/repository_test.go +++ b/internal/target/repository_test.go @@ -1,11 +1,13 @@ package target import ( + "database/sql" "testing" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/perms" + "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -136,3 +138,173 @@ func TestNewRepository(t *testing.T) { }) } } + +func TestRepositoryListPermissionWhereClauses(t *testing.T) { + tests := []struct { + name string + perms []perms.Permission + expWhere []string + expArgs []any + }{ + { + name: "nilPerms", + perms: nil, + expWhere: []string{}, + expArgs: []any{}, + }, + { + name: "emptyPerms", + perms: []perms.Permission{}, + expWhere: []string{}, + expArgs: []any{}, + }, + { + name: "noListActionPerms", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.Create, + }, + { + ScopeId: "scope_b", + Action: action.Read, + }, + { + ScopeId: "scope_c", + Action: action.Delete, + }, + }, + expWhere: []string{}, + expArgs: []any{}, + }, + { + name: "onePermissionAllResources", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.List, + }, + }, + expWhere: []string{"(project_id = @project_id_1)"}, + expArgs: []any{sql.Named("project_id_1", "scope_a")}, + }, + { + name: "onePermissionAllResourcesNonListIgnored", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.List, + }, + { + ScopeId: "scope_b", + Action: action.Create, + }, + }, + expWhere: []string{"(project_id = @project_id_1)"}, + expArgs: []any{sql.Named("project_id_1", "scope_a")}, + }, + { + name: "onePermissionResourceIds", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.List, + ResourceIds: []string{"resourceid1", "resourceid2"}, + }, + }, + expWhere: []string{"(project_id = @project_id_1 and public_id = any(@public_id_1))"}, + expArgs: []any{ + sql.Named("project_id_1", "scope_a"), + sql.Named("public_id_1", "{resourceid1,resourceid2}"), + }, + }, + { + name: "multiplePermissionsAllResources", + perms: []perms.Permission{ + {ScopeId: "scope_a", Action: action.List}, + {ScopeId: "scope_b", Action: action.List}, + {ScopeId: "scope_c", Action: action.List}, + {ScopeId: "scope_d", Action: action.List}, + }, + expWhere: []string{ + "(project_id = @project_id_1)", + "(project_id = @project_id_2)", + "(project_id = @project_id_3)", + "(project_id = @project_id_4)", + }, + expArgs: []any{ + sql.Named("project_id_1", "scope_a"), + sql.Named("project_id_2", "scope_b"), + sql.Named("project_id_3", "scope_c"), + sql.Named("project_id_4", "scope_d"), + }, + }, + { + name: "multiplePermissionsResourceIds", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.List, + ResourceIds: []string{"resourceid1", "resourceid2"}, + }, + { + ScopeId: "scope_b", + Action: action.List, + ResourceIds: []string{"resourceid3", "resourceid4"}, + }, + }, + expWhere: []string{ + "(project_id = @project_id_1 and public_id = any(@public_id_1))", + "(project_id = @project_id_2 and public_id = any(@public_id_2))", + }, + expArgs: []any{ + sql.Named("project_id_1", "scope_a"), + sql.Named("project_id_2", "scope_b"), + sql.Named("public_id_1", "{resourceid1,resourceid2}"), + sql.Named("public_id_2", "{resourceid3,resourceid4}"), + }, + }, + { + name: "multiplePermissionsMix", + perms: []perms.Permission{ + { + ScopeId: "scope_a", + Action: action.List, + ResourceIds: []string{"resourceid1", "resourceid2"}, + }, + { + ScopeId: "scope_b", + Action: action.List, + ResourceIds: []string{"resourceid3", "resourceid4"}, + }, + {ScopeId: "scope_c", Action: action.List}, + {ScopeId: "scope_d", Action: action.List}, + }, + expWhere: []string{ + "(project_id = @project_id_1 and public_id = any(@public_id_1))", + "(project_id = @project_id_2 and public_id = any(@public_id_2))", + "(project_id = @project_id_3)", + "(project_id = @project_id_4)", + }, + expArgs: []any{ + sql.Named("project_id_1", "scope_a"), + sql.Named("public_id_1", "{resourceid1,resourceid2}"), + sql.Named("project_id_2", "scope_b"), + sql.Named("public_id_2", "{resourceid3,resourceid4}"), + sql.Named("project_id_3", "scope_c"), + sql.Named("project_id_4", "scope_d"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := Repository{} + repo.permissions = tt.perms + + where, args := repo.listPermissionWhereClauses() + require.ElementsMatch(t, tt.expWhere, where) + require.ElementsMatch(t, tt.expArgs, args) + }) + } +} diff --git a/internal/target/tcp/repository_test.go b/internal/target/tcp/repository_test.go index d5ca11008a..b7c8e3c60f 100644 --- a/internal/target/tcp/repository_test.go +++ b/internal/target/tcp/repository_test.go @@ -3,7 +3,6 @@ package tcp_test import ( "context" "fmt" - "strconv" "testing" "time" @@ -12,8 +11,11 @@ import ( "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" + "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/target" "github.com/hashicorp/boundary/internal/target/tcp" + "github.com/hashicorp/boundary/internal/types/action" + "github.com/hashicorp/boundary/internal/types/resource" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -135,119 +137,15 @@ func TestRepository_LookupTarget(t *testing.T) { } } -func TestRepository_ListTargets(t *testing.T) { - t.Parallel() - conn, _ := db.TestSetup(t, "postgres") - const testLimit = 10 - wrapper := db.TestWrapper(t) - testKms := kms.TestKms(t, conn, wrapper) - iamRepo := iam.TestRepo(t, conn, wrapper) - _, proj := iam.TestScopes(t, iamRepo) - rw := db.New(conn) - repo, err := target.NewRepository(rw, rw, testKms, target.WithLimit(testLimit)) - require.NoError(t, err) - - type args struct { - opt []target.Option - } - tests := []struct { - name string - createCnt int - createProjectId string - createProjectId2 string - grantUserId string - args args - wantCnt int - wantErr bool - }{ - { - name: "tcp-target", - createCnt: 5, - createProjectId: proj.PublicId, - args: args{ - opt: []target.Option{target.WithType(tcp.Subtype), target.WithProjectIds([]string{proj.PublicId})}, - }, - wantCnt: 5, - wantErr: false, - }, - { - name: "no-limit", - createCnt: testLimit + 1, - createProjectId: proj.PublicId, - args: args{ - opt: []target.Option{target.WithLimit(-1), target.WithProjectIds([]string{proj.PublicId})}, - }, - wantCnt: testLimit + 1, - wantErr: false, - }, - { - name: "default-limit", - createCnt: testLimit + 1, - createProjectId: proj.PublicId, - args: args{ - opt: []target.Option{target.WithProjectIds([]string{proj.PublicId})}, - }, - wantCnt: testLimit, - wantErr: false, - }, - { - name: "custom-limit", - createCnt: testLimit + 1, - createProjectId: proj.PublicId, - args: args{ - opt: []target.Option{target.WithLimit(3), target.WithProjectIds([]string{proj.PublicId})}, - }, - wantCnt: 3, - wantErr: false, - }, - { - name: "bad-org", - createCnt: 1, - createProjectId: proj.PublicId, - args: args{ - opt: []target.Option{target.WithProjectIds([]string{"bad-id"})}, - }, - wantCnt: 0, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert, require := assert.New(t), require.New(t) - ctx := context.Background() - db.TestDeleteWhere(t, conn, tcp.NewTestTarget(""), "1=1") - testGroups := []target.Target{} - for i := 0; i < tt.createCnt; i++ { - switch { - case tt.createProjectId2 != "" && i%2 == 0: - testGroups = append(testGroups, tcp.TestTarget(ctx, t, conn, tt.createProjectId2, strconv.Itoa(i))) - default: - testGroups = append(testGroups, tcp.TestTarget(ctx, t, conn, tt.createProjectId, strconv.Itoa(i))) - } - } - assert.Equal(tt.createCnt, len(testGroups)) - got, err := repo.ListTargets(ctx, tt.args.opt...) - if tt.wantErr { - require.Error(err) - return - } - require.NoError(err) - assert.Equal(tt.wantCnt, len(got)) - }) - } -} - func TestRepository_ListRoles_Multiple_Scopes(t *testing.T) { t.Parallel() conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) + _, proj1 := iam.TestScopes(t, iamRepo) _, proj2 := iam.TestScopes(t, iamRepo) - rw := db.New(conn) - repo, err := target.NewRepository(rw, rw, testKms) - require.NoError(t, err) db.TestDeleteWhere(t, conn, tcp.NewTestTarget(""), "1=1") @@ -261,7 +159,26 @@ func TestRepository_ListRoles_Multiple_Scopes(t *testing.T) { total++ } - got, err := repo.ListTargets(ctx, target.WithProjectIds([]string{"global", proj1.GetPublicId(), proj2.GetPublicId()})) + rw := db.New(conn) + repo, err := target.NewRepository(rw, rw, testKms, + target.WithPermissions([]perms.Permission{ + { + ScopeId: proj1.PublicId, + Resource: resource.Target, + Action: action.List, + All: true, + }, + { + ScopeId: proj2.PublicId, + Resource: resource.Target, + Action: action.List, + All: true, + }, + }), + ) + require.NoError(t, err) + + got, err := repo.ListTargets(ctx) require.NoError(t, err) assert.Equal(t, total, len(got)) }