From 9ee5a9f713e0a0f64f6bafd525f7c17c7e3b50d6 Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Thu, 4 Aug 2022 12:30:40 +0100 Subject: [PATCH 01/16] feat(perms): Derive permissions list from user grants --- internal/perms/acl.go | 75 ++++++++ internal/perms/acl_test.go | 376 ++++++++++++++++++++++++++++++++++++- 2 files changed, 447 insertions(+), 4 deletions(-) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index b8e2f3c0d..589480bb0 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/scopes" ) const AnonymousUserId = "u_anon" @@ -27,6 +28,18 @@ type ACLResults struct { scopeMap map[string][]Grant } +// Permission provides information about the specific +// resources that a user has been granted access to for a given scope, resource, and action. +type Permission struct { + ScopeId string // The scope id for which the permission applies. + Resource resource.Type + Action action.Type + + ResourceIds []string // Any specific resource ids that have been referred in the grant's `id` field, if applicable. + OnlySelf bool // The grant only allows actions against the user's own resources. + All bool // We got a wildcard in the grant string's `id` field. +} + // Resource defines something within boundary that requires authorization // capabilities. Resources must have a ScopeId. type Resource struct { @@ -209,6 +222,68 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option return } +// ListPermissions builds a set of Permissions based on the grants in the ACL for the List action, for the requested scopes and resource type. +func (a ACL) ListPermissions(requestedScopes map[string]*scopes.ScopeInfo, requestedType resource.Type, requestedActions action.ActionSet) []Permission { + perms := make([]Permission, 0, len(requestedScopes)) + for scopeId := range requestedScopes { + p := Permission{ + ScopeId: scopeId, + Resource: requestedType, + Action: action.List, + } + + // Get grants for a specific scope id from the source of truth. + grants := a.scopeMap[scopeId] + for _, grant := range grants { + // This grant doesn't match what we're looking for, ignore. + if grant.typ != requestedType && grant.typ != resource.All { + continue + } + + // We found a grant that matches the requested resource type: + // Search to see if one or all actions in the action set have been granted. + found := false + if ok := grant.actions[action.All]; ok { + found = true + } else { + for _, a := range requestedActions { + if ok := grant.actions[a]; ok { + found = true + break + } + } + } + if !found { // In this case, none of the requested actions were granted for the given scope id. + continue + } + + actions, _ := grant.Actions() + excludeList := make(action.ActionSet, 0, len(actions)) + for _, aa := range actions { + if aa != action.List { + excludeList = append(excludeList, aa) + } + } + p.OnlySelf = excludeList.OnlySelf() + + switch grant.id { + case "*": + p.All = true + case "": + continue + default: + p.ResourceIds = append(p.ResourceIds, grant.id) + } + } + + if p.All || len(p.ResourceIds) > 0 { + perms = append(perms, p) + } + } + + return perms +} + func topLevelType(typ resource.Type) bool { switch typ { case resource.AuthMethod, diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index 8e3e31a6f..b4b86e6d5 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -9,17 +9,19 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/scopes" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type scopeGrant struct { + scope string + grants []string +} + func Test_ACLAllowed(t *testing.T) { t.Parallel() - type scopeGrant struct { - scope string - grants []string - } type actionAuthorized struct { action action.Type authorized bool @@ -372,6 +374,372 @@ func Test_ACLAllowed(t *testing.T) { } } +func TestACL_ListPermissions(t *testing.T) { + tests := []struct { + name string + aclGrants []scopeGrant + scopes map[string]*scopes.ScopeInfo // *scopes.ScopeInfo isn't used at the moment. + resourceType resource.Type + actionSet action.ActionSet + expPermissions []Permission + }{ + { + name: "Requested scope(s) not present in ACL scope map", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=list,read"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{ + "o_non_existent_scope": nil, + "o_this_one_too": nil, + }, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{}, + }, + { + name: "Requested resource mismatch", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=target;actions=list,read"}, // List & Read for all Targets + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, // We're requesting sessions. + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{}, + }, + { + name: "Requested actions not available for the requested scope id", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=delete"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{}, + }, + { + name: "No specific id or wildcard provided for `id` field", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"type=*;actions=list,read"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{}, + }, + { + name: "Allow all ids", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=list,read"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: false, + All: true, + }, + }, + }, + { + name: "Allow all ids, :self actions", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=list,read:self"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: true, + All: true, + }, + }, + }, + { + name: "Allow specific IDs", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{ + "id=s_1;type=session;actions=list,read", + "id=s_2;type=session;actions=list,read", + "id=s_3;type=session;actions=list,read", + }, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: []string{"s_1", "s_2", "s_3"}, + OnlySelf: false, + All: false, + }, + }, + }, + { + name: "No specific type 1", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=*;actions=list,read:self"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: true, + All: true, + }, + }, + }, + { + name: "List + No-op action with id wildcard", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=list,no-op"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: false, + All: true, + }, + }, + }, + { + name: "List + No-op action with id wildcard, read requested", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=session;actions=list,no-op"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.Read}, + expPermissions: []Permission{}, + }, + { + name: "List + No-op action with specific ids", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{ + "id=s_1;type=session;actions=list,no-op", + "id=s_2;type=session;actions=list,no-op", + "id=s_3;type=session;actions=list,no-op", + }, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: []string{"s_1", "s_2", "s_3"}, + OnlySelf: false, + All: false, + }, + }, + }, + { + name: "No specific type 2", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=*;actions=list,read:self"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Host, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Host, + Action: action.List, + ResourceIds: nil, + OnlySelf: true, + All: true, + }, + }, + }, + { + name: "Grant hierarchy is respected", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{ + "id=*;type=session;actions=read:self", + "id=*;type=*;actions=list,read", + }, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Target, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Target, + Action: action.List, + ResourceIds: nil, + OnlySelf: false, + All: true, + }, + }, + }, + { + name: "Full access 1", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=*;actions=*"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read, action.Create, action.Delete}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: false, + All: true, + }, + }, + }, + { + name: "Full access 2", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{"id=*;type=*;actions=*"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil}, + resourceType: resource.Host, + actionSet: action.ActionSet{action.List, action.Read, action.Create, action.Delete}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Host, + Action: action.List, + ResourceIds: nil, + OnlySelf: false, + All: true, + }, + }, + }, + { + name: "Multiple scopes", + aclGrants: []scopeGrant{ + { + scope: "o_1", + grants: []string{ + "id=s_1;type=session;actions=list,read", + "id=s_2;type=session;actions=list,read", + "id=s_3;type=session;actions=list,read", + }, + }, + { + scope: "o_2", + grants: []string{"id=*;type=session;actions=list,read:self"}, + }, + }, + scopes: map[string]*scopes.ScopeInfo{"o_1": nil, "o_2": nil}, + resourceType: resource.Session, + actionSet: action.ActionSet{action.List, action.Read}, + expPermissions: []Permission{ + { + ScopeId: "o_1", + Resource: resource.Session, + Action: action.List, + ResourceIds: []string{"s_1", "s_2", "s_3"}, + OnlySelf: false, + All: false, + }, + { + ScopeId: "o_2", + Resource: resource.Session, + Action: action.List, + ResourceIds: nil, + OnlySelf: true, + All: true, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var grants []Grant + for _, sg := range tt.aclGrants { + for _, g := range sg.grants { + grant, err := Parse(sg.scope, g, WithSkipFinalValidation(true)) + require.NoError(t, err) + grants = append(grants, grant) + } + } + + acl := NewACL(grants...) + perms := acl.ListPermissions(tt.scopes, tt.resourceType, tt.actionSet) + require.ElementsMatch(t, tt.expPermissions, perms) + }) + } +} + func TestJsonMarshal(t *testing.T) { res := &Resource{ ScopeId: "scope", From ccb17df01aea93a87328c6ef8646f344784c2c46 Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Fri, 5 Aug 2022 14:03:44 +0100 Subject: [PATCH 02/16] feat(auth): Get all authorised scope info for a given list request --- internal/daemon/controller/auth/auth.go | 121 ++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/internal/daemon/controller/auth/auth.go b/internal/daemon/controller/auth/auth.go index a762a9d94..053e1c6d9 100644 --- a/internal/daemon/controller/auth/auth.go +++ b/internal/daemon/controller/auth/auth.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/boundary/internal/errors" authpb "github.com/hashicorp/boundary/internal/gen/controller/auth" "github.com/hashicorp/boundary/internal/gen/controller/tokens" + "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/daemon/controller/common" "github.com/hashicorp/boundary/internal/daemon/controller/handlers" @@ -694,3 +695,123 @@ func GetTokenFromRequest(ctx context.Context, kmsCache *kms.Kms, req *http.Reque return publicId, encryptedToken, uint32(receivedTokenType) } + +// ScopesAuthorizedForList retrieves and returns all scopes where a user is authorized +// to perform a *list* action on. It looks recursively from `rootScopeId`. +func (r *VerifyResults) ScopesAuthorizedForList(ctx context.Context, rootScopeId string, resourceType resource.Type) (map[string]*scopes.ScopeInfo, error) { + const op = "auth.(VerifyResults).ScopesAuthorizedForList" + + // Validation + switch { + case resourceType == resource.Unknown: + return nil, errors.New(ctx, errors.InvalidParameter, op, "unknown resource") + case r.v.iamRepoFn == nil: + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil iam repo") + case rootScopeId == "": + return nil, errors.New(ctx, errors.InvalidParameter, op, "missing root scope id") + case r.Scope == nil: + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil scope in auth results") + } + + repo, err := r.v.iamRepoFn() + if err != nil { + return 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, err + } + + var deferredScopes []*iam.Scope + var globalHasList bool // Store whether global has list permission + scopeResourceMap := make(map[string]*scopes.ScopeInfo) // Scope data per scope id + + // For each scope, see if we have permission to list that resource in that scope + for _, scp := range scps { + scpId := scp.GetPublicId() + aSet := r.FetchActionSetForType(ctx, + resource.Unknown, // This is overridden by `WithResource` option. + action.ActionSet{action.List}, + WithResource(&perms.Resource{Type: resourceType, ScopeId: scpId}), + ) + + // We only expect the action set to be nothing, or list. In case + // this is not the case, we bail out. + 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 ordering our map + // lookup might fail. + deferredScopes = append(deferredScopes, scp) + case 1: + if aSet[0] != action.List { + return nil, errors.New(ctx, errors.Internal, op, "unexpected action in set") + } + if scopeResourceMap[scpId] == nil { + scopeResourceMap[scpId] = &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + } + if scpId == scope.Global.String() { + globalHasList = true + } + default: + return nil, errors.New(ctx, errors.Internal, op, "unexpected number of actions back in set") + } + } + + // Now go through the deferred scopes 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 || scopeResourceMap[scp.GetParentId()] != nil { + scpId := scp.GetPublicId() + if scopeResourceMap[scpId] == nil { + scopeResourceMap[scpId] = &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + } + } + } + + // Elide out any that aren't under the root scope id + elideScopes := make([]string, 0, len(scopeResourceMap)) + for scpId, scp := range scopeResourceMap { + 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.ParentScopeId: + default: + elideScopes = append(elideScopes, scpId) + } + } + for _, scpId := range elideScopes { + delete(scopeResourceMap, scpId) + } + + // If we have nothing in scopeInfoMap at this point, we aren't authorized + // anywhere so return 403. + if len(scopeResourceMap) == 0 { + return nil, handlers.ForbiddenError() + } + + return scopeResourceMap, nil +} From 02cef3d8a83a066aac02750debc8c88f759e28ef Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Mon, 11 Jul 2022 20:54:09 +0000 Subject: [PATCH 03/16] test(sessions): Remove auth.DisabledAuthTestContext from tests Disabling the auth was resulting in some unrealistic tests setup and expectations. It also makes it difficult to refactor any behavior in the authn/authz flow since this option can bypass most of it. --- .../handlers/sessions/session_service_test.go | 166 ++++++++++++------ 1 file changed, 108 insertions(+), 58 deletions(-) diff --git a/internal/daemon/controller/handlers/sessions/session_service_test.go b/internal/daemon/controller/handlers/sessions/session_service_test.go index 158a05993..c68aba5f4 100644 --- a/internal/daemon/controller/handlers/sessions/session_service_test.go +++ b/internal/daemon/controller/handlers/sessions/session_service_test.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/boundary/internal/host/static" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/requests" "github.com/hashicorp/boundary/internal/server" "github.com/hashicorp/boundary/internal/session" "github.com/hashicorp/boundary/internal/target" @@ -32,7 +33,7 @@ import ( "google.golang.org/protobuf/testing/protocmp" ) -var testAuthorizedActions = []string{"no-op", "read", "read:self", "cancel", "cancel:self"} +var testAuthorizedActions = []string{"read:self", "cancel:self"} func TestGetSession(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") @@ -42,6 +43,7 @@ func TestGetSession(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrap) rw := db.New(conn) + sessRepo, err := session.NewRepository(rw, rw, kms) require.NoError(t, err) @@ -51,6 +53,12 @@ func TestGetSession(t *testing.T) { sessRepoFn := func() (*session.Repository, error) { return sessRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } o, p := iam.TestScopes(t, iamRepo) at := authtoken.TestAuthToken(t, conn, kms, o.GetPublicId()) @@ -106,7 +114,7 @@ func TestGetSession(t *testing.T) { res: &pbs.GetSessionResponse{Item: wireSess}, }, { - name: "Get a non existant Session", + name: "Get a non existent Session", req: &pbs.GetSessionRequest{Id: session.SessionPrefix + "_DoesntExis"}, res: nil, err: handlers.ApiErrorWithCode(codes.NotFound), @@ -131,7 +139,15 @@ func TestGetSession(t *testing.T) { s, err := sessions.NewService(sessRepoFn, iamRepoFn) require.NoError(err, "Couldn't create new session service.") - got, gErr := s.GetSession(auth.DisabledAuthTestContext(iamRepoFn, tc.scopeId), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + + got, gErr := s.GetSession(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -140,7 +156,7 @@ func TestGetSession(t *testing.T) { assert.True(got.GetItem().GetExpirationTime().AsTime().Sub(tc.res.GetItem().GetExpirationTime().AsTime()) < 10*time.Millisecond) tc.res.GetItem().ExpirationTime = got.GetItem().GetExpirationTime() } - assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetSession(%q) got response\n%q, wanted\n%q", tc.req, got, tc.res) + assert.Empty(cmp.Diff(tc.res, got, protocmp.Transform()), "GetSession(%q) got response\n%q, wanted\n%q", tc.req, got, tc.res) }) } } @@ -246,6 +262,12 @@ func TestList(t *testing.T) { sessRepoFn := func() (*session.Repository, error) { return sessRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } _, pNoSessions := iam.TestScopes(t, iamRepo) o, pWithSessions := iam.TestScopes(t, iamRepo) @@ -270,7 +292,7 @@ func TestList(t *testing.T) { tarOther := tcp.TestTarget(ctx, t, conn, pWithOtherSessions.GetPublicId(), "test", target.WithHostSources([]string{hsOther.GetPublicId()})) var wantSession []*pb.Session - var totalSession []*pb.Session + var wantOtherSession []*pb.Session var wantIncludeTerminatedSessions []*pb.Session for i := 0; i < 10; i++ { sess := session.TestSession(t, conn, wrap, session.ComposedOf{ @@ -309,7 +331,6 @@ func TestList(t *testing.T) { Connections: []*pb.Connection{}, // connections should not be returned for list }) - totalSession = append(totalSession, wantSession[i]) wantIncludeTerminatedSessions = append(wantIncludeTerminatedSessions, wantSession[i]) sess = session.TestSession(t, conn, wrap, session.ComposedOf{ @@ -326,11 +347,11 @@ func TestList(t *testing.T) { status, states = convertStates(sess.States) - totalSession = append(totalSession, &pb.Session{ + wantOtherSession = append(wantOtherSession, &pb.Session{ Id: sess.GetPublicId(), - ScopeId: pWithOtherSessions.GetPublicId(), - AuthTokenId: atOther.GetPublicId(), - UserId: atOther.GetIamUserId(), + ScopeId: pWithSessions.GetPublicId(), + AuthTokenId: at.GetPublicId(), + UserId: at.GetIamUserId(), TargetId: sess.TargetId, Endpoint: sess.Endpoint, HostSetId: sess.HostSetId, @@ -339,7 +360,7 @@ func TestList(t *testing.T) { UpdatedTime: sess.UpdateTime.GetTimestamp(), CreatedTime: sess.CreateTime.GetTimestamp(), ExpirationTime: sess.ExpirationTime.GetTimestamp(), - Scope: &scopes.ScopeInfo{Id: pWithOtherSessions.GetPublicId(), Type: scope.Project.String(), ParentScopeId: oOther.GetPublicId()}, + Scope: &scopes.ScopeInfo{Id: pWithSessions.GetPublicId(), Type: scope.Project.String(), ParentScopeId: o.GetPublicId()}, Status: status, States: states, Certificate: sess.Certificate, @@ -397,35 +418,41 @@ func TestList(t *testing.T) { } cases := []struct { - name string - req *pbs.ListSessionsRequest - res *pbs.ListSessionsResponse - err error + name string + req *pbs.ListSessionsRequest + res *pbs.ListSessionsResponse + otherRes *pbs.ListSessionsResponse + err error }{ { - name: "List Many Sessions", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId()}, - res: &pbs.ListSessionsResponse{Items: wantSession}, + name: "List Many Sessions", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId()}, + res: &pbs.ListSessionsResponse{Items: wantSession}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { - name: "List Many Include Terminated", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), IncludeTerminated: true}, - res: &pbs.ListSessionsResponse{Items: wantIncludeTerminatedSessions}, + name: "List Many Include Terminated", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), IncludeTerminated: true}, + res: &pbs.ListSessionsResponse{Items: wantIncludeTerminatedSessions}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { - name: "List No Sessions", - req: &pbs.ListSessionsRequest{ScopeId: pNoSessions.GetPublicId()}, - res: &pbs.ListSessionsResponse{}, + name: "List No Sessions", + req: &pbs.ListSessionsRequest{ScopeId: pNoSessions.GetPublicId()}, + res: &pbs.ListSessionsResponse{}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { - name: "List Sessions Recursively", - req: &pbs.ListSessionsRequest{ScopeId: scope.Global.String(), Recursive: true}, - res: &pbs.ListSessionsResponse{Items: totalSession}, + name: "List Sessions Recursively", + req: &pbs.ListSessionsRequest{ScopeId: scope.Global.String(), Recursive: true}, + res: &pbs.ListSessionsResponse{Items: wantSession}, + otherRes: &pbs.ListSessionsResponse{Items: wantOtherSession}, }, { - name: "Filter To Single Sessions", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: fmt.Sprintf(`"/item/id"==%q`, totalSession[4].Id)}, - res: &pbs.ListSessionsResponse{Items: totalSession[4:5]}, + name: "Filter To Single Sessions", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: fmt.Sprintf(`"/item/id"==%q`, wantSession[4].Id)}, + res: &pbs.ListSessionsResponse{Items: wantSession[4:5]}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { name: "Filter To Many Sessions", @@ -433,17 +460,20 @@ func TestList(t *testing.T) { ScopeId: scope.Global.String(), Recursive: true, Filter: fmt.Sprintf(`"/item/scope/id" matches "^%s"`, pWithSessions.GetPublicId()[:8]), }, - res: &pbs.ListSessionsResponse{Items: wantSession}, + res: &pbs.ListSessionsResponse{Items: wantSession}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { - name: "Filter To Nothing", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: `"/item/id" == ""`}, - res: &pbs.ListSessionsResponse{}, + name: "Filter To Nothing", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: `"/item/id" == ""`}, + res: &pbs.ListSessionsResponse{}, + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, { - name: "Filter Bad Format", - req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: `//badformat/`}, - err: handlers.InvalidArgumentErrorf("bad format", nil), + name: "Filter Bad Format", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), Filter: `//badformat/`}, + err: handlers.InvalidArgumentErrorf("bad format", nil), + otherRes: &pbs.ListSessionsResponse{Items: []*pb.Session{}}, }, } for _, tc := range cases { @@ -453,7 +483,14 @@ func TestList(t *testing.T) { require.NoError(err, "Couldn't create new session service.") // Test without anon user - got, gErr := s.ListSessions(auth.DisabledAuthTestContext(iamRepoFn, tc.req.GetScopeId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := s.ListSessions(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "ListSessions(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -470,22 +507,21 @@ func TestList(t *testing.T) { } assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "ListSessions(%q) got response %q, wanted %q", tc.req, got, tc.res) - // Test with anon user - got, gErr = s.ListSessions(auth.DisabledAuthTestContext(iamRepoFn, tc.req.GetScopeId(), auth.WithUserId(auth.AnonymousUserId)), tc.req) + // Test with other user + otherRequestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: atOther.GetPublicId(), + Token: atOther.GetToken(), + } + otherRequestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + otherCtx := auth.NewVerifierContext(otherRequestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &otherRequestInfo) + got, gErr = s.ListSessions(otherCtx, tc.req) require.NoError(gErr) - assert.Len(got.Items, len(tc.res.Items)) - for _, item := range got.GetItems() { - require.Empty(item.Version) - require.Empty(item.UserId) - require.Empty(item.HostId) - require.Empty(item.HostSetId) - require.Empty(item.AuthTokenId) - require.Empty(item.Endpoint) - require.Nil(item.CreatedTime) - require.Nil(item.ExpirationTime) - require.Nil(item.UpdatedTime) - require.Empty(item.Certificate) - require.Empty(item.TerminationReason) + assert.Len(got.Items, len(tc.otherRes.Items)) + for i, wantSess := range tc.otherRes.GetItems() { + assert.True(got.GetItems()[i].GetExpirationTime().AsTime().Sub(wantSess.GetExpirationTime().AsTime()) < 10*time.Millisecond) + assert.Equal(0, len(wantSess.GetConnections())) // no connections on list + wantSess.ExpirationTime = got.GetItems()[i].GetExpirationTime() } }) } @@ -525,6 +561,12 @@ func TestCancel(t *testing.T) { sessRepoFn := func() (*session.Repository, error) { return sessRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } o, p := iam.TestScopes(t, iamRepo) at := authtoken.TestAuthToken(t, conn, kms, o.GetPublicId()) @@ -613,16 +655,23 @@ func TestCancel(t *testing.T) { tc.req.Version = version - got, gErr := s.CancelSession(auth.DisabledAuthTestContext(iamRepoFn, tc.scopeId), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := s.CancelSession(ctx, tc.req) if tc.err != nil { require.Error(gErr) // It's hard to mix and match api/error package errors right now // so use old/new behavior depending on the type. If validate // gets updated this can be standardized. if errors.Match(errors.T(errors.InvalidSessionState), gErr) { - assert.True(errors.Match(errors.T(tc.err), gErr), "GetSession(%+v) got error %#v, wanted %#v", tc.req, gErr, tc.err) + assert.True(errors.Match(errors.T(tc.err), gErr), "CancelSession(%+v) got error %#v, wanted %#v", tc.req, gErr, tc.err) } else { - assert.True(errors.Is(gErr, tc.err), "GetSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) + assert.True(errors.Is(gErr, tc.err), "CancelSession(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) } } @@ -630,6 +679,7 @@ func TestCancel(t *testing.T) { require.Nil(got) return } + require.NotNil(got) tc.res.GetItem().Version = got.GetItem().Version // Compare the new canceling state and then remove it for the rest of the proto comparison @@ -647,7 +697,7 @@ func TestCancel(t *testing.T) { EndTime: got.GetItem().GetUpdatedTime(), }, } - assert.Empty(cmp.Diff(got.GetItem().GetStates(), wantState, protocmp.Transform()), "GetSession(%q) states") + assert.Empty(cmp.Diff(got.GetItem().GetStates(), wantState, protocmp.Transform()), "CancelSession(%q) states") got.GetItem().States = nil got.GetItem().UpdatedTime = nil @@ -656,7 +706,7 @@ func TestCancel(t *testing.T) { tc.res.GetItem().ExpirationTime = got.GetItem().GetExpirationTime() } - assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "GetSession(%q) got response\n%q, wanted\n%q", tc.req, got, tc.res) + assert.Empty(cmp.Diff(got, tc.res, protocmp.Transform()), "CancelSession(%q) got response\n%q, wanted\n%q", tc.req, got, tc.res) if tc.req != nil { require.NotNil(got) From 39fedd843c343d290ca195124b42ef75c591dde5 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Fri, 29 Jul 2022 17:24:39 +0000 Subject: [PATCH 04/16] test(targets): Remove auth.DisabledAuthTestContext from tests Disabling the auth was resulting in some unrealistic tests setup and expectations. It also makes it difficult to refactor any behavior in the authn/authz flow since this option can bypass most of it. --- .../targets/tcp/target_service_test.go | 477 ++++++++++++++++-- 1 file changed, 426 insertions(+), 51 deletions(-) diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index 21434c70f..359d32d66 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -109,12 +109,24 @@ func TestGet(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } o, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, o.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") hc := static.TestCatalogs(t, conn, proj.GetPublicId(), 1)[0] hs := static.TestSets(t, conn, hc.GetPublicId(), 2) @@ -179,7 +191,14 @@ func TestGet(t *testing.T) { s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(err, "Couldn't create a new host set service.") - got, gErr := s.GetTarget(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := s.GetTarget(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "GetTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -198,14 +217,34 @@ func TestList(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } _, projNoTar := iam.TestScopes(t, iamRepo) org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") + + ar := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, ar.GetPublicId(), auth.AnonymousUserId) + _ = iam.TestRoleGrant(t, conn, ar.GetPublicId(), "id=*;type=target;actions=*") + otherOrg, otherProj := iam.TestScopes(t, iamRepo) + r = iam.TestRole(t, conn, otherProj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") hc := static.TestCatalogs(t, conn, proj.GetPublicId(), 1)[0] otherHc := static.TestCatalogs(t, conn, otherProj.GetPublicId(), 1)[0] hss := static.TestSets(t, conn, hc.GetPublicId(), 2) @@ -292,8 +331,14 @@ func TestList(t *testing.T) { s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(err, "Couldn't create new host set service.") - // Test with non-anon user - got, gErr := s.ListTargets(auth.DisabledAuthTestContext(iamRepoFn, tc.req.GetScopeId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := s.ListTargets(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "ListTargets(%q) got error %v, wanted %v", tc.req.GetScopeId(), gErr, tc.err) @@ -312,17 +357,20 @@ func TestList(t *testing.T) { } // Test with anon user - got, gErr = s.ListTargets(auth.DisabledAuthTestContext(iamRepoFn, tc.req.GetScopeId(), auth.WithUserId(auth.AnonymousUserId)), tc.req) - require.NoError(gErr) - assert.Len(got.Items, len(tc.res.Items)) - for _, item := range got.GetItems() { - require.Empty(item.Version) - require.Nil(item.CreatedTime) - require.Nil(item.UpdatedTime) - require.Nil(item.SessionMaxSeconds) - require.Nil(item.SessionConnectionLimit) - require.Empty(item.WorkerFilter) - require.Nil(item.Attrs) + requestInfo = authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeUnknown), + } + requestContext = context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr = s.ListTargets(ctx, tc.req) + require.Error(gErr) + + // For now, due to how recursive checks the additional scopes, + // it gets a 403 while a non-recursive expects a 401 + if tc.req.GetRecursive() { + assert.ErrorIs(gErr, handlers.ForbiddenError()) + } else { + assert.ErrorIs(gErr, handlers.UnauthenticatedError()) } }) } @@ -334,13 +382,26 @@ func TestDelete(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } ctx := context.Background() - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") + tar := tcp.TestTarget(ctx, t, conn, proj.GetPublicId(), "test") s, err := testService(t, context.Background(), conn, kms, wrapper) @@ -380,7 +441,14 @@ func TestDelete(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - got, gErr := s.DeleteTarget(auth.DisabledAuthTestContext(iamRepoFn, tc.scopeId), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := s.DeleteTarget(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "DeleteTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -397,13 +465,25 @@ func TestDelete_twice(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + ctx := context.Background() iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") tar := tcp.TestTarget(ctx, t, conn, proj.GetPublicId(), "test") @@ -412,7 +492,13 @@ func TestDelete_twice(t *testing.T) { req := &pbs.DeleteTargetRequest{ Id: tar.GetPublicId(), } - ctx = auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) _, gErr := s.DeleteTarget(ctx, req) assert.NoError(gErr, "First attempt") _, gErr = s.DeleteTarget(ctx, req) @@ -426,12 +512,24 @@ func TestCreate(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") cases := []struct { name string @@ -555,7 +653,15 @@ func TestCreate(t *testing.T) { s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(err, "Failed to create a new host set service.") - got, gErr := s.CreateTarget(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + + got, gErr := s.CreateTarget(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "CreateTarget(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -586,14 +692,24 @@ func TestUpdate(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") - rw := db.New(conn) repoFn := func() (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } @@ -960,7 +1076,14 @@ func TestUpdate(t *testing.T) { req := proto.Clone(toMerge).(*pbs.UpdateTargetRequest) proto.Merge(req, tc.req) - got, gErr := tested.UpdateTarget(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, gErr := tested.UpdateTarget(ctx, req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "UpdateTarget(%+v) got error %v, wanted %v", req, gErr, tc.err) @@ -992,14 +1115,25 @@ func TestUpdate_BadVersion(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") - rw := db.New(conn) repoFn := func() (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } @@ -1017,7 +1151,14 @@ func TestUpdate_BadVersion(t *testing.T) { tested, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Failed to create a new host set service.") - upTar, err := tested.UpdateTarget(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), &pbs.UpdateTargetRequest{ + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + upTar, err := tested.UpdateTarget(ctx, &pbs.UpdateTargetRequest{ Id: gtar.GetPublicId(), Item: &pb.Target{ Description: wrapperspb.String("updated"), @@ -1036,12 +1177,24 @@ func TestAddTargetHostSets(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new target service.") @@ -1102,7 +1255,14 @@ func TestAddTargetHostSets(t *testing.T) { HostSetIds: tc.addHostSets, } - got, err := s.AddTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.AddTargetHostSets(ctx, req) s, ok := status.FromError(err) require.True(t, ok) require.NoError(t, err, "Got error: %v", s) @@ -1157,7 +1317,14 @@ func TestAddTargetHostSets(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.AddTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.AddTargetHostSets(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "AddTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1172,12 +1339,24 @@ func TestSetTargetHostSets(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new host set service.") @@ -1243,7 +1422,14 @@ func TestSetTargetHostSets(t *testing.T) { HostSetIds: tc.setHostSets, } - got, err := s.SetTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.SetTargetHostSets(ctx, req) require.NoError(t, err, "Got error: %v", s) assert.ElementsMatch(t, tc.resultHostSets, got.GetItem().GetHostSetIds()) }) @@ -1287,7 +1473,14 @@ func TestSetTargetHostSets(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.SetTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.SetTargetHostSets(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "SetTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1302,12 +1495,24 @@ func TestRemoveTargetHostSets(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new host set service.") @@ -1369,7 +1574,14 @@ func TestRemoveTargetHostSets(t *testing.T) { HostSetIds: tc.removeHosts, } - got, err := s.RemoveTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.RemoveTargetHostSets(ctx, req) if tc.wantErr { assert.Error(t, err) return @@ -1429,7 +1641,14 @@ func TestRemoveTargetHostSets(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.RemoveTargetHostSets(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.RemoveTargetHostSets(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "RemoveTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1444,12 +1663,24 @@ func TestAddTargetHostSources(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new target service.") @@ -1510,7 +1741,14 @@ func TestAddTargetHostSources(t *testing.T) { HostSourceIds: tc.addHostSources, } - got, err := s.AddTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.AddTargetHostSources(ctx, req) s, ok := status.FromError(err) require.True(t, ok) require.NoError(t, err, "Got error: %v", s) @@ -1565,7 +1803,14 @@ func TestAddTargetHostSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.AddTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.AddTargetHostSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "AddTargetHostSources(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1580,12 +1825,24 @@ func TestSetTargetHostSources(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new host set service.") @@ -1645,7 +1902,14 @@ func TestSetTargetHostSources(t *testing.T) { HostSourceIds: tc.setHostSources, } - got, err := s.SetTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.SetTargetHostSources(ctx, req) require.NoError(t, err, "Got error: %v", s) assert.ElementsMatch(t, tc.resultHostSources, got.GetItem().GetHostSourceIds()) }) @@ -1689,7 +1953,14 @@ func TestSetTargetHostSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.SetTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.SetTargetHostSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "SetTargetHostSources(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1704,12 +1975,24 @@ func TestRemoveTargetHostSources(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new host set service.") @@ -1771,7 +2054,14 @@ func TestRemoveTargetHostSources(t *testing.T) { HostSourceIds: tc.removeHostSources, } - got, err := s.RemoveTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.RemoveTargetHostSources(ctx, req) if tc.wantErr { assert.Error(t, err) return @@ -1831,7 +2121,14 @@ func TestRemoveTargetHostSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.RemoveTargetHostSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx = auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.RemoveTargetHostSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "RemoveTargetHostSets(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -1845,12 +2142,24 @@ func TestAddTargetCredentialSources(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new target service.") @@ -1926,7 +2235,14 @@ func TestAddTargetCredentialSources(t *testing.T) { BrokeredCredentialSourceIds: tc.addSources, } - got, err := s.AddTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.AddTargetCredentialSources(ctx, req) require.NoError(t, err, "Got error: %v", s) assert.ElementsMatch(t, tc.resultSourceIds, got.GetItem().GetBrokeredCredentialSourceIds()) @@ -1987,7 +2303,14 @@ func TestAddTargetCredentialSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.AddTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.AddTargetCredentialSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "AddTargetCredentialSources(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -2001,12 +2324,24 @@ func TestSetTargetCredentialSources(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new target service.") @@ -2089,7 +2424,14 @@ func TestSetTargetCredentialSources(t *testing.T) { BrokeredCredentialSourceIds: tc.setCredentialSources, } - got, err := s.SetTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.SetTargetCredentialSources(ctx, req) require.NoError(t, err, "Got error: %v", s) assert.ElementsMatch(t, tc.resultCredentialSourceIds, got.GetItem().GetBrokeredCredentialSourceIds()) assert.Equal(t, len(tc.resultCredentialSourceIds), len(got.GetItem().GetBrokeredCredentialSources())) @@ -2137,7 +2479,14 @@ func TestSetTargetCredentialSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.SetTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.SetTargetCredentialSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "SetTargetCredentialSources(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) @@ -2151,12 +2500,24 @@ func TestRemoveTargetCredentialSources(t *testing.T) { wrapper := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } + tokenRepoFn := func() (*authtoken.Repository, error) { + return authtoken.NewRepository(rw, rw, kms) + } + serversRepoFn := func() (*server.Repository, error) { + return server.NewRepository(rw, rw, kms) + } - _, proj := iam.TestScopes(t, iamRepo) + org, proj := iam.TestScopes(t, iamRepo) + at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) + r := iam.TestRole(t, conn, proj.GetPublicId()) + _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) + _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") s, err := testService(t, context.Background(), conn, kms, wrapper) require.NoError(t, err, "Error when getting new target service.") @@ -2276,7 +2637,14 @@ func TestRemoveTargetCredentialSources(t *testing.T) { BrokeredCredentialSourceIds: tc.removeCredentialSources, } - got, err := s.RemoveTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + got, err := s.RemoveTargetCredentialSources(ctx, req) if tc.wantErr { assert.Error(t, err) return @@ -2344,7 +2712,14 @@ func TestRemoveTargetCredentialSources(t *testing.T) { for _, tc := range failCases { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - _, gErr := s.RemoveTargetCredentialSources(auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId()), tc.req) + requestInfo := authpb.RequestInfo{ + TokenFormat: uint32(auth.AuthTokenTypeBearer), + PublicId: at.GetPublicId(), + Token: at.GetToken(), + } + requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{}) + ctx := auth.NewVerifierContext(requestContext, iamRepoFn, tokenRepoFn, serversRepoFn, kms, &requestInfo) + _, gErr := s.RemoveTargetCredentialSources(ctx, tc.req) if tc.err != nil { require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "RemoveTargetCredentialSources(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) From 05b3d2917babb73429d63fbc90dedddb75f47474 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Tue, 9 Aug 2022 12:40:02 +0000 Subject: [PATCH 05/16] feat(perms): Add UserPermissions This is used to associate a user with a set of permissions which is necessary for repositories that need honor the OnlySelf aspect of a Permission. --- internal/perms/acl.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 589480bb0..47ab170df 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -40,6 +40,12 @@ type Permission struct { All bool // We got a wildcard in the grant string's `id` field. } +// UserPermissions is a set of Permissions for a User. +type UserPermissions struct { + UserId string + Permissions []Permission +} + // Resource defines something within boundary that requires authorization // capabilities. Resources must have a ScopeId. type Resource struct { From e4bb847ad1adc14ba1b7fa7caa5d18097fa458d7 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Tue, 9 Aug 2022 12:42:01 +0000 Subject: [PATCH 06/16] feat(auth): Expose ACL from VerifyResults --- internal/daemon/controller/auth/auth.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/daemon/controller/auth/auth.go b/internal/daemon/controller/auth/auth.go index 053e1c6d9..350bfc91b 100644 --- a/internal/daemon/controller/auth/auth.go +++ b/internal/daemon/controller/auth/auth.go @@ -641,6 +641,15 @@ func (r *VerifyResults) FetchOutputFields(res perms.Resource, act action.Type) p return r.v.acl.Allowed(res, act, r.UserId).OutputFields } +// ACL returns the perms.ACL of the verifier. +func (r *VerifyResults) ACL() perms.ACL { + if r.v == nil { + return perms.ACL{} + } + + return r.v.acl +} + // GetTokenFromRequest pulls the token from either the Authorization header or // split cookies and parses it. If it cannot be parsed successfully, the issue // is logged and we return blank, so logic will continue as the anonymous user. From 2fbdcf6ce096002e339ab59b563fac1b5dc3adc8 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Tue, 9 Aug 2022 12:42:58 +0000 Subject: [PATCH 07/16] feat(session): Use permissions for limiting list results The session repository can now be passed an optional set of permissions. The permissions are used to restrict results from the repository. Currently this only impacts the List method. This enabled the sessions service to use a single query to retrieve sessions and replaces the previous behavior where the session service would first request all sessions for a given set of scopes, evaluate the permissions, and then request sessions based on a set of session ids. --- .../postgres/50/01_session_list_index.up.sql | 11 + internal/session/options.go | 10 + internal/session/query.go | 15 - internal/session/repository.go | 57 +++ internal/session/repository_session.go | 97 +---- internal/session/repository_session_test.go | 385 +++++------------- .../session/service_list_for_authz_check.go | 23 -- 7 files changed, 184 insertions(+), 414 deletions(-) create mode 100644 internal/db/schema/migrations/oss/postgres/50/01_session_list_index.up.sql delete mode 100644 internal/session/service_list_for_authz_check.go diff --git a/internal/db/schema/migrations/oss/postgres/50/01_session_list_index.up.sql b/internal/db/schema/migrations/oss/postgres/50/01_session_list_index.up.sql new file mode 100644 index 000000000..b92cf056f --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/50/01_session_list_index.up.sql @@ -0,0 +1,11 @@ +begin; + -- Partial index to aid session list requests + -- + -- If a session list request is made using the default list request options + -- and using the standard grants created by boundary by default, + -- it will include where clauses that: + -- * include a project_id paired with a user_id + -- * and where termination_reason is null + create index session_list_pix on session (project_id, user_id, termination_reason) where termination_reason is null; + analyze session; +end; diff --git a/internal/session/options.go b/internal/session/options.go index fa1e90285..4e90a396a 100644 --- a/internal/session/options.go +++ b/internal/session/options.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/db/timestamp" + "github.com/hashicorp/boundary/internal/perms" ) // getOpts - iterate the inbound Options and return a struct @@ -32,6 +33,7 @@ type options struct { withDbOpts []db.Option withWorkerStateDelay time.Duration withTerminated bool + withPermissions *perms.UserPermissions } func getDefaultOptions() options { @@ -120,3 +122,11 @@ func WithTerminated(withTerminated bool) Option { o.withTerminated = withTerminated } } + +// WithPermissions is used to include user permissions when constructing a +// Repository. +func WithPermissions(p *perms.UserPermissions) Option { + return func(o *options) { + o.withPermissions = p + } +} diff --git a/internal/session/query.go b/internal/session/query.go index 8cbfb570c..21c001547 100644 --- a/internal/session/query.go +++ b/internal/session/query.go @@ -117,21 +117,6 @@ session_connection_limit(expiration_time, connection_limit) as ( select expiration_time, connection_limit, current_connection_count from session_connection_limit, session_connection_count; -` - nonTerminatedSessionPublicIdList = ` -select public_id, project_id, user_id from session -where - session.termination_reason is null -and - project_id = any(@project_ids) -; -` - - sessionPublicIdList = ` -select public_id, project_id, user_id from session -where - project_id = any(@project_ids) -; ` sessionList = ` diff --git a/internal/session/repository.go b/internal/session/repository.go index 27957382a..8080ae56f 100644 --- a/internal/session/repository.go +++ b/internal/session/repository.go @@ -2,12 +2,18 @@ package session import ( "context" + "database/sql" + "fmt" "sort" + "strings" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/errors" "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" ) // Clonable provides a cloning interface @@ -23,8 +29,13 @@ type Repository struct { // defaultLimit provides a default for limiting the number of results returned from the repo defaultLimit int + + permissions *perms.UserPermissions } +// RepositoryFactory is a function that creates a Repository. +type RepositoryFactory func(opt ...Option) (*Repository, error) + // NewRepository creates a new session Repository. Supports the options: WithLimit // which sets a default limit on results returned by repo operations. func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { @@ -43,14 +54,60 @@ func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repo // zero signals the boundary defaults should be used. opts.withLimit = db.DefaultLimit } + + if opts.withPermissions != nil { + for _, p := range opts.withPermissions.Permissions { + if p.Resource != resource.Session { + return nil, errors.NewDeprecated(errors.InvalidParameter, op, "permission for incorrect resource") + } + } + } + return &Repository{ reader: r, writer: w, kms: kms, defaultLimit: opts.withLimit, + permissions: opts.withPermissions, }, nil } +func (r *Repository) listPermissionWhereClauses() ([]string, []interface{}) { + var where []string + var args []interface{} + + if r.permissions == nil { + return where, args + } + + inClauseCnt := 0 + for _, p := range r.permissions.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, ",")+"}")) + } + + if p.OnlySelf { + inClauseCnt++ + clauses = append(clauses, fmt.Sprintf("user_id = @user_id_%d", inClauseCnt)) + args = append(args, sql.Named(fmt.Sprintf("user_id_%d", inClauseCnt), r.permissions.UserId)) + } + + where = append(where, fmt.Sprintf("(%s)", strings.Join(clauses, " and "))) + } + return where, args +} + // list will return a listing of resources and honor the WithLimit option or the // repo defaultLimit. Supports WithOrder option. func (r *Repository) list(ctx context.Context, resources interface{}, where string, args []interface{}, opt ...Option) error { diff --git a/internal/session/repository_session.go b/internal/session/repository_session.go index a9c0228f2..f0aa64751 100644 --- a/internal/session/repository_session.go +++ b/internal/session/repository_session.go @@ -9,7 +9,6 @@ import ( "strings" "time" - "github.com/hashicorp/boundary/internal/boundary" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/errors" @@ -229,89 +228,28 @@ func (r *Repository) LookupSession(ctx context.Context, sessionId string, _ ...O return &session, authzSummary, nil } -// fetchAuthzProtectedSessionsByProject fetches sessions for the given projects. -// Note that the sessions are not fully populated, and only contain the -// necessary information to implement the boundary.AuthzProtectedEntity -// interface. Supports the WithTerminated option. -func (r *Repository) fetchAuthzProtectedSessionsByProject( - ctx context.Context, projectIds []string, opt ...Option, -) (map[string][]boundary.AuthzProtectedEntity, error) { - const op = "session.(Repository).fetchAuthzProtectedSessionsByProject" - - opts := getOpts(opt...) - - if len(projectIds) == 0 { - return nil, errors.New(ctx, errors.InvalidParameter, op, "no project ids given") - } - - args := []interface{}{ - sql.Named("project_ids", "{"+strings.Join(projectIds, ",")+"}"), - } - - var query string - if opts.withTerminated { - query = sessionPublicIdList - } else { - query = nonTerminatedSessionPublicIdList - } - - rows, err := r.reader.Query(ctx, query, args) - if err != nil { - return nil, errors.Wrap(ctx, err, op) - } - defer rows.Close() - - sessionsMap := map[string][]boundary.AuthzProtectedEntity{} - for rows.Next() { - var ses Session - if err := r.reader.ScanRows(ctx, rows, &ses); err != nil { - return nil, errors.Wrap(ctx, err, op, errors.WithMsg("scan row failed")) - } - sessionsMap[ses.GetProjectId()] = append(sessionsMap[ses.GetProjectId()], ses) - } - - return sessionsMap, nil -} - -// ListSessions lists sessions. Supports the WithLimit, WithProjectId, and WithSessionIds options. +// ListSessions lists sessions. Sessions returned will be limited by the list +// permissions of the repository. Supports the WithTerminated, WithLimit, +// WithOrderByCreateTime options. func (r *Repository) ListSessions(ctx context.Context, opt ...Option) ([]*Session, error) { const op = "session.(Repository).ListSessions" opts := getOpts(opt...) - 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, ","))) + where, args := r.listPermissionWhereClauses() + if len(where) == 0 { + return nil, nil } - if opts.withUserId != "" { - inClauseCnt += 1 - where, args = append(where, fmt.Sprintf("user_id = @%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), opts.withUserId)) - } - - switch len(opts.withSessionIds) { - case 0: - case 1: - inClauseCnt += 1 - where, args = append(where, fmt.Sprintf("s.public_id = @%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), opts.withSessionIds[0])) - default: - idsInClause := make([]string, 0, len(opts.withSessionIds)) - for _, id := range opts.withSessionIds { - inClauseCnt += 1 - idsInClause, args = append(idsInClause, fmt.Sprintf("@%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), id)) + var whereClause string + if len(where) > 0 { + whereClause = " where (" + strings.Join(where, " or ") + ")" + if !opts.withTerminated { + whereClause += "and termination_reason is null" + } + } else { + if !opts.withTerminated { + whereClause = "where termination_reason is null" } - where = append(where, fmt.Sprintf("s.public_id in (%s)", strings.Join(idsInClause, ","))) } var limit string @@ -323,7 +261,6 @@ func (r *Repository) ListSessions(ctx context.Context, opt ...Option) ([]*Sessio // non-zero signals an override of the default limit for the repo. limit = fmt.Sprintf("limit %d", opts.withLimit) } - var withOrder string switch opts.withOrderByCreateTime { case db.AscendingOrderBy: @@ -334,10 +271,6 @@ func (r *Repository) ListSessions(ctx context.Context, opt ...Option) ([]*Sessio withOrder = "order by create_time" } - var whereClause string - if len(where) > 0 { - whereClause = " where " + strings.Join(where, " and ") - } q := sessionList query := fmt.Sprintf(q, whereClause, withOrder, limit, withOrder) diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index 0bcf6e055..30ef301a2 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -21,9 +21,12 @@ import ( iamStore "github.com/hashicorp/boundary/internal/iam/store" "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" tcpStore "github.com/hashicorp/boundary/internal/target/tcp/store" + "github.com/hashicorp/boundary/internal/types/action" + "github.com/hashicorp/boundary/internal/types/resource" wrapping "github.com/hashicorp/go-kms-wrapping/v2" "github.com/jackc/pgconn" "github.com/stretchr/testify/assert" @@ -39,10 +42,18 @@ func TestRepository_ListSession(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit)) - require.NoError(t, err) composedOf := TestSessionParams(t, conn, wrapper, iamRepo) + listPerms := &perms.UserPermissions{ + UserId: composedOf.UserId, + Permissions: []perms.Permission{ + { + ScopeId: composedOf.ProjectId, + Resource: resource.Session, + Action: action.List, + }, + }, + } type args struct { opt []Option } @@ -50,58 +61,85 @@ func TestRepository_ListSession(t *testing.T) { name string createCnt int args args + perms *perms.UserPermissions wantCnt int wantErr bool withConnections int }{ { name: "no-limit", - createCnt: repo.defaultLimit + 1, + createCnt: testLimit + 1, args: args{ opt: []Option{WithLimit(-1)}, }, - wantCnt: repo.defaultLimit + 1, + perms: listPerms, + wantCnt: testLimit + 1, wantErr: false, }, { name: "default-limit", - createCnt: repo.defaultLimit + 1, + createCnt: testLimit + 1, args: args{}, - wantCnt: repo.defaultLimit, + perms: listPerms, + wantCnt: testLimit, wantErr: false, }, { name: "custom-limit", - createCnt: repo.defaultLimit + 1, + createCnt: testLimit + 1, args: args{ opt: []Option{WithLimit(3)}, }, + perms: listPerms, wantCnt: 3, wantErr: false, }, { - name: "withProjectIds", - createCnt: repo.defaultLimit + 1, - args: args{ - opt: []Option{WithProjectIds([]string{composedOf.ProjectId})}, + name: "withNoPerms", + createCnt: testLimit + 1, + args: args{}, + perms: &perms.UserPermissions{}, + wantCnt: 0, + wantErr: false, + }, + { + name: "withPermsDifferentScopeId", + createCnt: testLimit + 1, + args: args{}, + perms: &perms.UserPermissions{ + Permissions: []perms.Permission{ + { + ScopeId: "o_thisIsNotValid", + Resource: resource.Session, + Action: action.List, + }, + }, }, - wantCnt: repo.defaultLimit, + wantCnt: 0, wantErr: false, }, { - name: "bad-withProjectId", - createCnt: repo.defaultLimit + 1, - args: args{ - opt: []Option{WithProjectIds([]string{"o_thisIsNotValid"})}, + name: "withPermsNonListAction", + createCnt: testLimit + 1, + args: args{}, + perms: &perms.UserPermissions{ + Permissions: []perms.Permission{ + { + ScopeId: composedOf.ProjectId, + Resource: resource.Session, + Action: action.Read, + }, + }, }, wantCnt: 0, wantErr: false, }, { name: "multiple-connections", - createCnt: repo.defaultLimit + 1, + createCnt: testLimit + 1, args: args{}, - wantCnt: repo.defaultLimit, + perms: listPerms, + wantCnt: testLimit, wantErr: false, withConnections: 3, }, @@ -109,6 +147,10 @@ func TestRepository_ListSession(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) + + repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(tt.perms)) + require.NoError(err) + db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") testSessions := []*Session{} for i := 0; i < tt.createCnt; i++ { @@ -150,6 +192,10 @@ func TestRepository_ListSession(t *testing.T) { for i := 0; i < wantCnt; i++ { _ = TestSession(t, conn, wrapper, composedOf) } + + repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(listPerms)) + require.NoError(err) + got, err := repo.ListSessions(context.Background(), WithOrderByCreateTime(db.AscendingOrderBy)) require.NoError(err) assert.Equal(wantCnt, len(got)) @@ -160,7 +206,7 @@ func TestRepository_ListSession(t *testing.T) { assert.True(first.Before(second)) } }) - t.Run("withUserId", func(t *testing.T) { + t.Run("onlySelf", func(t *testing.T) { assert, require := assert.New(t), require.New(t) db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") wantCnt := 5 @@ -168,51 +214,25 @@ func TestRepository_ListSession(t *testing.T) { _ = TestSession(t, conn, wrapper, composedOf) } s := TestDefaultSession(t, conn, wrapper, iamRepo) + + p := &perms.UserPermissions{ + UserId: s.UserId, + Permissions: []perms.Permission{ + { + ScopeId: s.ProjectId, + Resource: resource.Session, + Action: action.List, + OnlySelf: true, + }, + }, + } + repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(p)) + require.NoError(err) got, err := repo.ListSessions(context.Background(), WithUserId(s.UserId)) require.NoError(err) assert.Equal(1, len(got)) assert.Equal(s.UserId, got[0].UserId) }) - t.Run("withUserIdAndwithScopeId", func(t *testing.T) { - assert, require := assert.New(t), require.New(t) - db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") - wantCnt := 5 - for i := 0; i < wantCnt; i++ { - // Scope 1 User 1 - _ = TestSession(t, conn, wrapper, composedOf) - } - // Scope 2 User 2 - s := TestDefaultSession(t, conn, wrapper, iamRepo) - - // Scope 1 User 2 - coDiffUser := composedOf - coDiffUser.AuthTokenId = s.AuthTokenId - coDiffUser.UserId = s.UserId - wantS := TestSession(t, conn, wrapper, coDiffUser) - - got, err := repo.ListSessions(context.Background(), WithUserId(coDiffUser.UserId), WithProjectIds([]string{coDiffUser.ProjectId})) - require.NoError(err) - assert.Equal(1, len(got)) - assert.Equal(wantS.UserId, got[0].UserId) - assert.Equal(wantS.ProjectId, got[0].ProjectId) - }) - t.Run("WithSessionIds", func(t *testing.T) { - assert, require := assert.New(t), require.New(t) - db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") - testSessions := []*Session{} - for i := 0; i < 10; i++ { - s := TestSession(t, conn, wrapper, composedOf) - _ = TestState(t, conn, s.PublicId, StatusActive) - testSessions = append(testSessions, s) - } - assert.Equal(10, len(testSessions)) - withIds := []string{testSessions[0].PublicId, testSessions[1].PublicId} - got, err := repo.ListSessions(context.Background(), WithSessionIds(withIds...), WithOrderByCreateTime(db.AscendingOrderBy)) - require.NoError(err) - assert.Equal(2, len(got)) - assert.Equal(StatusActive, got[0].States[0].Status) - assert.Equal(StatusPending, got[0].States[1].Status) - }) } func TestRepository_ListSessions_Multiple_Scopes(t *testing.T) { @@ -222,23 +242,29 @@ func TestRepository_ListSessions_Multiple_Scopes(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) - require.NoError(t, err) db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") const numPerScope = 10 - var projs []string + var p []perms.Permission for i := 0; i < numPerScope; i++ { composedOf := TestSessionParams(t, conn, wrapper, iamRepo) - projs = append(projs, composedOf.ProjectId) + p = append(p, perms.Permission{ + ScopeId: composedOf.ProjectId, + Resource: resource.Session, + Action: action.List, + }) s := TestSession(t, conn, wrapper, composedOf) _ = TestState(t, conn, s.PublicId, StatusActive) } - got, err := repo.ListSessions(context.Background(), WithProjectIds(projs)) + repo, err := NewRepository(rw, rw, kms, WithPermissions(&perms.UserPermissions{ + Permissions: p, + })) require.NoError(t, err) - assert.Equal(t, len(projs), len(got)) + got, err := repo.ListSessions(context.Background()) + require.NoError(t, err) + assert.Equal(t, len(p), len(got)) } func TestRepository_CreateSession(t *testing.T) { @@ -1623,232 +1649,3 @@ func TestRepository_deleteTerminated(t *testing.T) { }) } } - -func TestFetchAuthzProtectedSessionsByScopes(t *testing.T) { - conn, _ := db.TestSetup(t, "postgres") - ctx := context.Background() - wrapper := db.TestWrapper(t) - iamRepo := iam.TestRepo(t, conn, wrapper) - rw := db.New(conn) - kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) - require.NoError(t, err) - composedOf := TestSessionParams(t, conn, wrapper, iamRepo) - - _, pWithOtherSessions := iam.TestScopes(t, iamRepo) - - hcOther := static.TestCatalogs(t, conn, pWithOtherSessions.GetPublicId(), 1)[0] - hsOther := static.TestSets(t, conn, hcOther.GetPublicId(), 1)[0] - hOther := static.TestHosts(t, conn, hcOther.GetPublicId(), 1)[0] - static.TestSetMembers(t, conn, hsOther.GetPublicId(), []*static.Host{hOther}) - tarOther := tcp.TestTarget(ctx, t, conn, pWithOtherSessions.GetPublicId(), "test", target.WithHostSources([]string{hsOther.GetPublicId()})) - - composedOfOther := ComposedOf{ - UserId: composedOf.UserId, - HostId: hOther.GetPublicId(), - TargetId: tarOther.GetPublicId(), - HostSetId: hsOther.GetPublicId(), - AuthTokenId: composedOf.AuthTokenId, - ProjectId: pWithOtherSessions.GetPublicId(), - Endpoint: "tcp://127.0.0.1:22", - } - - type testCase struct { - name string - createCnt int - terminateCnt int - otherCnt int - otherTerminateCnt int - opts []Option - reqScopes []string - wantCnt int - wantOtherCnt int - wantErr bool - } - - cases := []testCase{ - { - name: "NonTerminated/none", - createCnt: 0, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 0, - wantErr: false, - }, - { - name: "NonTerminated/one", - createCnt: 1, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 1, - wantErr: false, - }, - { - name: "NonTerminated/many", - createCnt: 5, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 5, - wantErr: false, - }, - { - name: "NonTerminated/many one terminated", - createCnt: 5, - terminateCnt: 1, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 4, - wantErr: false, - }, - { - name: "NonTerminated/many terminated", - createCnt: 5, - terminateCnt: 3, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 2, - wantErr: false, - }, - { - name: "NonTerminated/many multiple projects", - createCnt: 5, - terminateCnt: 3, - reqScopes: []string{composedOf.ProjectId, composedOfOther.ProjectId}, - wantCnt: 2, - wantErr: false, - }, - { - name: "NonTerminated/many multiple projects", - createCnt: 5, - terminateCnt: 3, - otherCnt: 3, - otherTerminateCnt: 1, - reqScopes: []string{composedOf.ProjectId, composedOfOther.ProjectId}, - wantCnt: 2, - wantOtherCnt: 2, - wantErr: false, - }, - { - name: "NonTerminated/no projects", - createCnt: 2, - terminateCnt: 1, - otherCnt: 2, - otherTerminateCnt: 1, - reqScopes: []string{}, - wantErr: true, - }, - { - name: "none", - opts: []Option{WithTerminated(true)}, - createCnt: 0, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 0, - wantErr: false, - }, - { - name: "one", - opts: []Option{WithTerminated(true)}, - createCnt: 1, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 1, - wantErr: false, - }, - { - name: "many", - opts: []Option{WithTerminated(true)}, - createCnt: 5, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 5, - wantErr: false, - }, - { - name: "many one terminated", - opts: []Option{WithTerminated(true)}, - createCnt: 5, - terminateCnt: 1, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 5, - wantErr: false, - }, - { - name: "many terminated", - opts: []Option{WithTerminated(true)}, - createCnt: 5, - terminateCnt: 3, - reqScopes: []string{composedOf.ProjectId}, - wantCnt: 5, - wantErr: false, - }, - { - name: "many multiple projects", - opts: []Option{WithTerminated(true)}, - createCnt: 5, - terminateCnt: 3, - reqScopes: []string{composedOf.ProjectId, composedOfOther.ProjectId}, - wantCnt: 5, - wantErr: false, - }, - { - name: "many multiple projects", - opts: []Option{WithTerminated(true)}, - createCnt: 5, - terminateCnt: 3, - otherCnt: 3, - otherTerminateCnt: 1, - reqScopes: []string{composedOf.ProjectId, composedOfOther.ProjectId}, - wantCnt: 5, - wantOtherCnt: 3, - wantErr: false, - }, - { - name: "no projects", - opts: []Option{WithTerminated(true)}, - createCnt: 2, - terminateCnt: 1, - otherCnt: 2, - otherTerminateCnt: 1, - reqScopes: []string{}, - wantErr: true, - }, - } - for _, tt := range cases { - t.Run(tt.name, func(t *testing.T) { - assert, require := assert.New(t), require.New(t) - db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") - - testSessions := []*Session{} - for i := 0; i < tt.createCnt; i++ { - s := TestSession(t, conn, wrapper, composedOf) - _ = TestState(t, conn, s.PublicId, StatusActive) - testSessions = append(testSessions, s) - } - for i := 0; i < tt.terminateCnt; i++ { - _, err := repo.CancelSession(ctx, testSessions[i].PublicId, testSessions[i].Version) - require.NoError(err) - } - terminated, err := repo.TerminateCompletedSessions(ctx) - require.NoError(err) - require.Equal(tt.terminateCnt, terminated) - - otherTestSessions := []*Session{} - for i := 0; i < tt.otherCnt; i++ { - s := TestSession(t, conn, wrapper, composedOfOther) - _ = TestState(t, conn, s.PublicId, StatusActive) - otherTestSessions = append(otherTestSessions, s) - } - for i := 0; i < tt.otherTerminateCnt; i++ { - _, err := repo.CancelSession(ctx, otherTestSessions[i].PublicId, otherTestSessions[i].Version) - require.NoError(err) - } - terminated, err = repo.TerminateCompletedSessions(ctx) - require.NoError(err) - require.Equal(tt.otherTerminateCnt, terminated) - - assert.Equal(tt.otherCnt, len(otherTestSessions)) - - got, err := repo.fetchAuthzProtectedSessionsByProject(ctx, tt.reqScopes, tt.opts...) - if tt.wantErr { - require.Error(err) - return - } - require.NoError(err) - assert.Equal(tt.wantCnt, len(got[composedOf.ProjectId])) - assert.Equal(tt.wantOtherCnt, len(got[composedOfOther.ProjectId])) - }) - } -} diff --git a/internal/session/service_list_for_authz_check.go b/internal/session/service_list_for_authz_check.go deleted file mode 100644 index 98687c7bb..000000000 --- a/internal/session/service_list_for_authz_check.go +++ /dev/null @@ -1,23 +0,0 @@ -package session - -import ( - "context" - - "github.com/hashicorp/boundary/internal/boundary" -) - -type listForAuthzCheckFunc func(ctx context.Context, projectIds []string) (map[string][]boundary.AuthzProtectedEntity, error) - -func (a listForAuthzCheckFunc) FetchAuthzProtectedEntitiesByScope(ctx context.Context, projectIds []string) (map[string][]boundary.AuthzProtectedEntity, error) { - return a(ctx, projectIds) -} - -// ListForAuthzCheck returns a functions that fetches sessions for the given -// projects. Note that the sessions are not fully populated, and only contain the -// necessary information to implement the boundary.AuthzProtectedEntity -// interface. Supports the WithTerminated option. -func ListForAuthzCheck(repo *Repository, opt ...Option) listForAuthzCheckFunc { - return func(ctx context.Context, projectIds []string) (map[string][]boundary.AuthzProtectedEntity, error) { - return repo.fetchAuthzProtectedSessionsByProject(ctx, projectIds, opt...) - } -} From 68568828e6a1509c639be5ae64b125fbd4aaaccd Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Tue, 9 Aug 2022 12:43:15 +0000 Subject: [PATCH 08/16] perf(sessions): Use session repository with permissions This builds on previous performance improvements to the list behavior by reducing the number of queries against the database and the amount of data transfered in order to retrieve a list of sessions. Previously, a query was performed to retrieve a tuple of `(session_id, scope_id, user_id)` for the requested scope, including all child scopes if the request was recursive. These tuples were evaluated against the requesting user's grants to identify the sessions that they were authorized to list. In the case of clusters with a large number of sessions, this would result in a large amount of data needing to be transfered for sessions that would not be used. Then a second query would be used to retrieve the full session information for the session ids that were authorized. This changes the list behavior to evaluate a set of permissions from the requesting user's grants, use these permissions when constructing a session repository, and retrieve the list of sessions the user is authorized to list in a signle query. Ref: d741034829df9367728bdf54727509be70f6d82e 32070678dc712812a42a511bb68f668658bd327a --- .../sessions/session_list_benchmarks_test.go | 7 +- .../handlers/sessions/session_service.go | 65 ++++++++----------- .../handlers/sessions/session_service_test.go | 21 +++--- 3 files changed, 36 insertions(+), 57 deletions(-) diff --git a/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go b/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go index 6f926d8b8..eeddc12e6 100644 --- a/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go +++ b/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go @@ -84,9 +84,6 @@ func BenchmarkSessionList(b *testing.B) { iamRepo, err := iam.NewRepository(rw, rw, kmsThing) require.NoError(b, err) - sessRepo, err := session.NewRepository(rw, rw, kmsThing) - require.NoError(b, err) - authTokenRepo, err := authtoken.NewRepository(rw, rw, kmsThing) require.NoError(b, err) @@ -99,8 +96,8 @@ func BenchmarkSessionList(b *testing.B) { iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } - sessRepoFn := func() (*session.Repository, error) { - return sessRepo, nil + sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kmsThing, opt...) } authTokenRepoFn := func() (*authtoken.Repository, error) { return authTokenRepo, nil diff --git a/internal/daemon/controller/handlers/sessions/session_service.go b/internal/daemon/controller/handlers/sessions/session_service.go index b8e530f5b..cbed06d4a 100644 --- a/internal/daemon/controller/handlers/sessions/session_service.go +++ b/internal/daemon/controller/handlers/sessions/session_service.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/boundary/globals" "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/errors" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" @@ -19,6 +18,7 @@ import ( "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/scopes" pb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/sessions" "google.golang.org/grpc/codes" ) @@ -139,34 +139,26 @@ func (s Service) ListSessions(ctx context.Context, req *pbs.ListSessionsRequest) } } - repo, err := s.repoFn() + var scopeIds map[string]*scopes.ScopeInfo + var err error + + if !req.GetRecursive() { + scopeIds = map[string]*scopes.ScopeInfo{authResults.Scope.Id: authResults.Scope} + } else { + scopeIds, err = authResults.ScopesAuthorizedForList(ctx, req.GetScopeId(), resource.Session) + } + + listPerms := authResults.ACL().ListPermissions(scopeIds, resource.Session, IdActions) + + repo, err := s.repoFn(session.WithPermissions(&perms.UserPermissions{ + UserId: authResults.UserId, + Permissions: listPerms, + })) if err != nil { return nil, errors.Wrap(ctx, err, op) } - scopeResourceInfo, err := scopeids.GetListingResourceInformation( - ctx, - scopeids.GetListingResourceInformationInput{ - IamRepoFn: s.iamRepoFn, - AuthResults: authResults, - RootScopeId: req.GetScopeId(), - Type: resource.Session, - Recursive: req.GetRecursive(), - AuthzProtectedEntityProvider: session.ListForAuthzCheck(repo, session.WithTerminated(req.IncludeTerminated)), - ActionSet: IdActions, - }, - ) - if err != nil { - return nil, err - } - // If no scopes match or we match scopes but there are no resources in them - // that we are authorized to see, return an empty response - if len(scopeResourceInfo.ScopeIds) == 0 || - len(scopeResourceInfo.ResourceIds) == 0 { - return &pbs.ListSessionsResponse{}, nil - } - - sesList, err := s.listFromRepo(ctx, scopeResourceInfo.ResourceIds) + sesList, err := repo.ListSessions(ctx, session.WithTerminated(req.GetIncludeTerminated())) if err != nil { return nil, err } @@ -183,14 +175,21 @@ func (s Service) ListSessions(ctx context.Context, req *pbs.ListSessionsRequest) Type: resource.Session, } for _, item := range sesList { + res.Id = item.GetPublicId() + res.ScopeId = item.GetProjectId() + authorizedActions := authResults.FetchActionSetForId(ctx, item.GetPublicId(), IdActions, auth.WithResource(&res)).Strings() + if len(authorizedActions) == 0 { + continue + } + outputFields := authResults.FetchOutputFields(res, 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.ProjectId].ScopeInfo)) + outputOpts = append(outputOpts, handlers.WithScope(scopeIds[item.ProjectId])) } if outputFields.Has(globals.AuthorizedActionsField) { - outputOpts = append(outputOpts, handlers.WithAuthorizedActions(scopeResourceInfo.ScopeResourceMap[item.ProjectId].Resources[item.PublicId].AuthorizedActions.Strings())) + outputOpts = append(outputOpts, handlers.WithAuthorizedActions(authorizedActions)) } item, err := toProto(ctx, item, outputOpts...) @@ -295,18 +294,6 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*session.Session, return sess, nil } -func (s Service) listFromRepo(ctx context.Context, sessionIds []string) ([]*session.Session, error) { - repo, err := s.repoFn() - if err != nil { - return nil, err - } - sesList, err := repo.ListSessions(ctx, session.WithSessionIds(sessionIds...)) - if err != nil { - return nil, err - } - return sesList, nil -} - func (s Service) cancelInRepo(ctx context.Context, id string, version uint32) (*session.Session, error) { const op = "sessions.(Service).cancelInRepo" repo, err := s.repoFn() diff --git a/internal/daemon/controller/handlers/sessions/session_service_test.go b/internal/daemon/controller/handlers/sessions/session_service_test.go index c68aba5f4..241e830f4 100644 --- a/internal/daemon/controller/handlers/sessions/session_service_test.go +++ b/internal/daemon/controller/handlers/sessions/session_service_test.go @@ -44,14 +44,11 @@ func TestGetSession(t *testing.T) { rw := db.New(conn) - sessRepo, err := session.NewRepository(rw, rw, kms) - require.NoError(t, err) - iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } - sessRepoFn := func() (*session.Repository, error) { - return sessRepo, nil + sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -172,8 +169,8 @@ func TestList_Self(t *testing.T) { iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } - sessRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -259,8 +256,8 @@ func TestList(t *testing.T) { iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } - sessRepoFn := func() (*session.Repository, error) { - return sessRepo, nil + sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -552,14 +549,12 @@ func TestCancel(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrap) rw := db.New(conn) - sessRepo, err := session.NewRepository(rw, rw, kms) - require.NoError(t, err) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } - sessRepoFn := func() (*session.Repository, error) { - return sessRepo, nil + sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) From febf0b998444ec63f04ce69a8be8785c80e8e94e Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Tue, 9 Aug 2022 12:43:38 +0000 Subject: [PATCH 09/16] refact(controller): Use session repo factory that takes options --- internal/daemon/controller/common/common.go | 2 +- .../controller/common/scopeids/scope_ids_test.go | 4 ++-- internal/daemon/controller/controller.go | 4 ++-- .../handlers/targets/tcp/target_service_test.go | 16 ++++++++-------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/daemon/controller/common/common.go b/internal/daemon/controller/common/common.go index 75c175f77..82635347d 100644 --- a/internal/daemon/controller/common/common.go +++ b/internal/daemon/controller/common/common.go @@ -25,7 +25,7 @@ type ( StaticRepoFactory func() (*static.Repository, error) PluginHostRepoFactory func() (*pluginhost.Repository, error) HostPluginRepoFactory func() (*hostplugin.Repository, error) - SessionRepoFactory func() (*session.Repository, error) + SessionRepoFactory session.RepositoryFactory // TODO refactor and remove this ConnectionRepoFactory func() (*session.ConnectionRepository, error) TargetRepoFactory func() (*target.Repository, error) WorkerAuthRepoStorageFactory func() (*server.WorkerAuthRepositoryStorage, error) diff --git a/internal/daemon/controller/common/scopeids/scope_ids_test.go b/internal/daemon/controller/common/scopeids/scope_ids_test.go index 77e35b8ed..e65b9e9ae 100644 --- a/internal/daemon/controller/common/scopeids/scope_ids_test.go +++ b/internal/daemon/controller/common/scopeids/scope_ids_test.go @@ -44,8 +44,8 @@ func TestListingScopeIds(t *testing.T) { s, err := groups.NewService(iamRepoFn) require.NoError(t, err) - sessionsRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionsRepoFn := func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opt...) } sess, err := sessions.NewService(sessionsRepoFn, iamRepoFn) require.NoError(t, err) diff --git a/internal/daemon/controller/controller.go b/internal/daemon/controller/controller.go index 637c6ec12..5cfbcd0a4 100644 --- a/internal/daemon/controller/controller.go +++ b/internal/daemon/controller/controller.go @@ -336,8 +336,8 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { c.TargetRepoFn = func() (*target.Repository, error) { return target.NewRepository(dbase, dbase, c.kms) } - c.SessionRepoFn = func() (*session.Repository, error) { - return session.NewRepository(dbase, dbase, c.kms) + c.SessionRepoFn = func(opt ...session.Option) (*session.Repository, error) { + return session.NewRepository(dbase, dbase, c.kms, opt...) } c.ConnectionRepoFn = func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, dbase, dbase, c.kms) diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index 359d32d66..676f0f728 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -85,8 +85,8 @@ func testService(t *testing.T, ctx context.Context, conn *db.DB, kms *kms.Kms, w serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kms) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) @@ -2749,8 +2749,8 @@ func TestAuthorizeSession(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kms) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opts...) } staticRepo, err := static.NewRepository(rw, rw, kms) require.NoError(t, err) @@ -3009,8 +3009,8 @@ func TestAuthorizeSessionTypedCredentials(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kms) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) @@ -3588,8 +3588,8 @@ func TestAuthorizeSession_Errors(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kms) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) From eb61ac63045d6690f50565c9d48b36ef1669b78a Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Wed, 24 Aug 2022 13:22:03 +0000 Subject: [PATCH 10/16] refact(session): Add context to session factory This adds a context to the session.RepositoryFactory, allowing errors.NewDeprecated to be replaced with errors.New. It also removes the common.SessionRepoFactory, instead packages just directly import from the session package. --- .../vault/repository_credentials_test.go | 2 +- .../daemon/cluster/handlers/worker_service.go | 4 +- .../handlers/worker_service_status_test.go | 16 ++++---- .../cluster/handlers/worker_service_test.go | 8 ++-- internal/daemon/controller/common/common.go | 1 - .../common/scopeids/scope_ids_test.go | 3 +- internal/daemon/controller/controller.go | 4 +- .../sessions/session_list_benchmarks_test.go | 2 +- .../handlers/sessions/session_service.go | 4 +- .../handlers/sessions/session_service_test.go | 13 ++++--- .../handlers/targets/target_service.go | 4 +- .../targets/tcp/target_service_test.go | 8 ++-- internal/server/repository_worker_test.go | 2 +- .../job_delete_terminated_sessions_test.go | 2 +- internal/session/job_session_cleanup_test.go | 6 +-- internal/session/jobs.go | 2 +- internal/session/repository.go | 10 ++--- .../session/repository_connection_test.go | 8 ++-- .../session/repository_credential_test.go | 6 ++- internal/session/repository_session_test.go | 37 ++++++++++++------- internal/session/repository_test.go | 6 ++- .../service_authorize_connection_test.go | 2 +- .../session/service_close_connections_test.go | 2 +- .../service_worker_status_report_test.go | 2 +- ...on_list_benchmarks_dump_generation_test.go | 2 +- 25 files changed, 86 insertions(+), 70 deletions(-) diff --git a/internal/credential/vault/repository_credentials_test.go b/internal/credential/vault/repository_credentials_test.go index 44b3c6849..732ba5f0d 100644 --- a/internal/credential/vault/repository_credentials_test.go +++ b/internal/credential/vault/repository_credentials_test.go @@ -640,7 +640,7 @@ group by session_id, status; cc[id].active = credentialCount } - sessionRepo, err := session.NewRepository(rw, rw, kms) + sessionRepo, err := session.NewRepository(ctx, rw, rw, kms) require.NoError(err) count, err := sessionRepo.TerminateCompletedSessions(ctx) diff --git a/internal/daemon/cluster/handlers/worker_service.go b/internal/daemon/cluster/handlers/worker_service.go index c486e11a0..50fc98af8 100644 --- a/internal/daemon/cluster/handlers/worker_service.go +++ b/internal/daemon/cluster/handlers/worker_service.go @@ -29,7 +29,7 @@ type workerServiceServer struct { pbs.UnsafeSessionServiceServer serversRepoFn common.ServersRepoFactory - sessionRepoFn common.SessionRepoFactory + sessionRepoFn session.RepositoryFactory connectionRepoFn common.ConnectionRepoFactory updateTimes *sync.Map kms *kms.Kms @@ -42,7 +42,7 @@ var ( func NewWorkerServiceServer( serversRepoFn common.ServersRepoFactory, - sessionRepoFn common.SessionRepoFactory, + sessionRepoFn session.RepositoryFactory, connectionRepoFn common.ConnectionRepoFactory, updateTimes *sync.Map, kms *kms.Kms, diff --git a/internal/daemon/cluster/handlers/worker_service_status_test.go b/internal/daemon/cluster/handlers/worker_service_status_test.go index 2d839b7ab..39f422b0d 100644 --- a/internal/daemon/cluster/handlers/worker_service_status_test.go +++ b/internal/daemon/cluster/handlers/worker_service_status_test.go @@ -47,8 +47,8 @@ func TestStatus(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return serverRepo, nil } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kms, opts...) } connRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kms) @@ -233,8 +233,8 @@ func TestStatusSessionClosed(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return serverRepo, nil } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kms, opts...) } connRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kms) @@ -414,8 +414,8 @@ func TestStatusDeadConnection(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return serverRepo, nil } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kms, opts...) } connRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kms, session.WithWorkerStateDelay(0)) @@ -554,8 +554,8 @@ func TestStatusWorkerWithKeyId(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return serverRepo, nil } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kms, opts...) } connRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kms) diff --git a/internal/daemon/cluster/handlers/worker_service_test.go b/internal/daemon/cluster/handlers/worker_service_test.go index 629159355..44382f4cc 100644 --- a/internal/daemon/cluster/handlers/worker_service_test.go +++ b/internal/daemon/cluster/handlers/worker_service_test.go @@ -39,8 +39,8 @@ func TestLookupSession(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kms) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kms) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kms, opts...) } connectionRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kms) @@ -214,8 +214,8 @@ func TestHcpbWorkers(t *testing.T) { serversRepoFn := func() (*server.Repository, error) { return server.NewRepository(rw, rw, kmsCache) } - sessionRepoFn := func() (*session.Repository, error) { - return session.NewRepository(rw, rw, kmsCache) + sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { + return session.NewRepository(ctx, rw, rw, kmsCache, opts...) } connectionRepoFn := func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, rw, rw, kmsCache) diff --git a/internal/daemon/controller/common/common.go b/internal/daemon/controller/common/common.go index 82635347d..20744a222 100644 --- a/internal/daemon/controller/common/common.go +++ b/internal/daemon/controller/common/common.go @@ -25,7 +25,6 @@ type ( StaticRepoFactory func() (*static.Repository, error) PluginHostRepoFactory func() (*pluginhost.Repository, error) HostPluginRepoFactory func() (*hostplugin.Repository, error) - SessionRepoFactory session.RepositoryFactory // TODO refactor and remove this ConnectionRepoFactory func() (*session.ConnectionRepository, error) TargetRepoFactory func() (*target.Repository, error) WorkerAuthRepoStorageFactory func() (*server.WorkerAuthRepositoryStorage, error) diff --git a/internal/daemon/controller/common/scopeids/scope_ids_test.go b/internal/daemon/controller/common/scopeids/scope_ids_test.go index e65b9e9ae..5c424e8df 100644 --- a/internal/daemon/controller/common/scopeids/scope_ids_test.go +++ b/internal/daemon/controller/common/scopeids/scope_ids_test.go @@ -44,8 +44,9 @@ func TestListingScopeIds(t *testing.T) { s, err := groups.NewService(iamRepoFn) require.NoError(t, err) + ctx := context.Background() sessionsRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opt...) + return session.NewRepository(ctx, rw, rw, kms, opt...) } sess, err := sessions.NewService(sessionsRepoFn, iamRepoFn) require.NoError(t, err) diff --git a/internal/daemon/controller/controller.go b/internal/daemon/controller/controller.go index 5cfbcd0a4..01935b3bf 100644 --- a/internal/daemon/controller/controller.go +++ b/internal/daemon/controller/controller.go @@ -110,7 +110,7 @@ type Controller struct { OidcRepoFn common.OidcAuthRepoFactory PasswordAuthRepoFn common.PasswordAuthRepoFactory ServersRepoFn common.ServersRepoFactory - SessionRepoFn common.SessionRepoFactory + SessionRepoFn session.RepositoryFactory ConnectionRepoFn common.ConnectionRepoFactory StaticHostRepoFn common.StaticRepoFactory PluginHostRepoFn common.PluginHostRepoFactory @@ -337,7 +337,7 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { return target.NewRepository(dbase, dbase, c.kms) } c.SessionRepoFn = func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(dbase, dbase, c.kms, opt...) + return session.NewRepository(ctx, dbase, dbase, c.kms, opt...) } c.ConnectionRepoFn = func() (*session.ConnectionRepository, error) { return session.NewConnectionRepository(ctx, dbase, dbase, c.kms) diff --git a/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go b/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go index eeddc12e6..81c079f25 100644 --- a/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go +++ b/internal/daemon/controller/handlers/sessions/session_list_benchmarks_test.go @@ -97,7 +97,7 @@ func BenchmarkSessionList(b *testing.B) { return iamRepo, nil } sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kmsThing, opt...) + return session.NewRepository(ctx, rw, rw, kmsThing, opt...) } authTokenRepoFn := func() (*authtoken.Repository, error) { return authTokenRepo, nil diff --git a/internal/daemon/controller/handlers/sessions/session_service.go b/internal/daemon/controller/handlers/sessions/session_service.go index cbed06d4a..9a7375058 100644 --- a/internal/daemon/controller/handlers/sessions/session_service.go +++ b/internal/daemon/controller/handlers/sessions/session_service.go @@ -45,14 +45,14 @@ var ( type Service struct { pbs.UnsafeSessionServiceServer - repoFn common.SessionRepoFactory + repoFn session.RepositoryFactory iamRepoFn common.IamRepoFactory } var _ pbs.SessionServiceServer = (*Service)(nil) // NewService returns a session service which handles session related requests to boundary. -func NewService(repoFn common.SessionRepoFactory, iamRepoFn common.IamRepoFactory) (Service, error) { +func NewService(repoFn session.RepositoryFactory, iamRepoFn common.IamRepoFactory) (Service, error) { const op = "sessions.NewService" if repoFn == nil { return Service{}, errors.NewDeprecated(errors.InvalidParameter, op, "missing session repository") diff --git a/internal/daemon/controller/handlers/sessions/session_service_test.go b/internal/daemon/controller/handlers/sessions/session_service_test.go index 241e830f4..5034d6b43 100644 --- a/internal/daemon/controller/handlers/sessions/session_service_test.go +++ b/internal/daemon/controller/handlers/sessions/session_service_test.go @@ -44,11 +44,12 @@ func TestGetSession(t *testing.T) { rw := db.New(conn) + ctx := context.Background() iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opt...) + return session.NewRepository(ctx, rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -166,11 +167,12 @@ func TestList_Self(t *testing.T) { rw := db.New(conn) + ctx := context.Background() iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opt...) + return session.NewRepository(ctx, rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -250,14 +252,14 @@ func TestList(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrap) rw := db.New(conn) - sessRepo, err := session.NewRepository(rw, rw, kms) + sessRepo, err := session.NewRepository(ctx, rw, rw, kms) require.NoError(t, err) iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opt...) + return session.NewRepository(ctx, rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) @@ -550,11 +552,12 @@ func TestCancel(t *testing.T) { rw := db.New(conn) + ctx := context.Background() iamRepoFn := func() (*iam.Repository, error) { return iamRepo, nil } sessRepoFn := func(opt ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opt...) + return session.NewRepository(ctx, rw, rw, kms, opt...) } tokenRepoFn := func() (*authtoken.Repository, error) { return authtoken.NewRepository(rw, rw, kms) diff --git a/internal/daemon/controller/handlers/targets/target_service.go b/internal/daemon/controller/handlers/targets/target_service.go index 2db701764..0b1a3dce7 100644 --- a/internal/daemon/controller/handlers/targets/target_service.go +++ b/internal/daemon/controller/handlers/targets/target_service.go @@ -101,7 +101,7 @@ type Service struct { repoFn common.TargetRepoFactory iamRepoFn common.IamRepoFactory serversRepoFn common.ServersRepoFactory - sessionRepoFn common.SessionRepoFactory + sessionRepoFn session.RepositoryFactory pluginHostRepoFn common.PluginHostRepoFactory staticHostRepoFn common.StaticRepoFactory vaultCredRepoFn common.VaultCredentialRepoFactory @@ -118,7 +118,7 @@ func NewService( repoFn common.TargetRepoFactory, iamRepoFn common.IamRepoFactory, serversRepoFn common.ServersRepoFactory, - sessionRepoFn common.SessionRepoFactory, + sessionRepoFn session.RepositoryFactory, pluginHostRepoFn common.PluginHostRepoFactory, staticHostRepoFn common.StaticRepoFactory, vaultCredRepoFn common.VaultCredentialRepoFactory, diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index 676f0f728..bbbbf1c29 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -86,7 +86,7 @@ func testService(t *testing.T, ctx context.Context, conn *db.DB, kms *kms.Kms, w return server.NewRepository(rw, rw, kms) } sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opts...) + return session.NewRepository(ctx, rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) @@ -2750,7 +2750,7 @@ func TestAuthorizeSession(t *testing.T) { return server.NewRepository(rw, rw, kms) } sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opts...) + return session.NewRepository(ctx, rw, rw, kms, opts...) } staticRepo, err := static.NewRepository(rw, rw, kms) require.NoError(t, err) @@ -3010,7 +3010,7 @@ func TestAuthorizeSessionTypedCredentials(t *testing.T) { return server.NewRepository(rw, rw, kms) } sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opts...) + return session.NewRepository(ctx, rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) @@ -3589,7 +3589,7 @@ func TestAuthorizeSession_Errors(t *testing.T) { return server.NewRepository(rw, rw, kms) } sessionRepoFn := func(opts ...session.Option) (*session.Repository, error) { - return session.NewRepository(rw, rw, kms, opts...) + return session.NewRepository(ctx, rw, rw, kms, opts...) } staticHostRepoFn := func() (*static.Repository, error) { return static.NewRepository(rw, rw, kms) diff --git a/internal/server/repository_worker_test.go b/internal/server/repository_worker_test.go index eb125d4e7..f4e8100c3 100644 --- a/internal/server/repository_worker_test.go +++ b/internal/server/repository_worker_test.go @@ -189,7 +189,7 @@ func TestLookupWorker(t *testing.T) { server.WithWorkerTags(&server.Tag{"key", "val"})) require.NoError(t, err) - sessRepo, err := session.NewRepository(rw, rw, kms) + sessRepo, err := session.NewRepository(ctx, rw, rw, kms) require.NoError(t, err) connRepo, err := session.NewConnectionRepository(ctx, rw, rw, kms, session.WithWorkerStateDelay(0)) require.NoError(t, err) diff --git a/internal/session/job_delete_terminated_sessions_test.go b/internal/session/job_delete_terminated_sessions_test.go index 40fc58d92..ae4835ab0 100644 --- a/internal/session/job_delete_terminated_sessions_test.go +++ b/internal/session/job_delete_terminated_sessions_test.go @@ -20,7 +20,7 @@ func TestDeleteTermiantedSessionsJob(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) composedOf := TestSessionParams(t, conn, wrapper, iamRepo) require.NoError(t, err) diff --git a/internal/session/job_session_cleanup_test.go b/internal/session/job_session_cleanup_test.go index 8c64c3c64..8017c24f5 100644 --- a/internal/session/job_session_cleanup_test.go +++ b/internal/session/job_session_cleanup_test.go @@ -37,7 +37,7 @@ func TestSessionConnectionCleanupJob(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) serversRepo, err := server.NewRepository(rw, rw, kms) require.NoError(err) - sessionRepo, err := NewRepository(rw, rw, kms) + sessionRepo, err := NewRepository(ctx, rw, rw, kms) connectionRepo, err := NewConnectionRepository(ctx, rw, rw, kms) require.NoError(err) @@ -180,7 +180,7 @@ func TestCloseConnectionsForDeadWorkers(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(err) gracePeriod := 1 * time.Second connRepo, err := NewConnectionRepository(ctx, rw, rw, kms) @@ -443,7 +443,7 @@ func TestCloseWorkerlessConnections(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(err) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms) require.NoError(err) diff --git a/internal/session/jobs.go b/internal/session/jobs.go index 70f1e113b..cb4b38246 100644 --- a/internal/session/jobs.go +++ b/internal/session/jobs.go @@ -24,7 +24,7 @@ func RegisterJobs(ctx context.Context, scheduler *scheduler.Scheduler, w db.Writ return fmt.Errorf("error registering session cleanup job: %w", err) } - repo, err := NewRepository(r, w, k) + repo, err := NewRepository(ctx, r, w, k) if err != nil { return fmt.Errorf("error creating repository: %w", err) } diff --git a/internal/session/repository.go b/internal/session/repository.go index 8080ae56f..f43a48ba5 100644 --- a/internal/session/repository.go +++ b/internal/session/repository.go @@ -38,16 +38,16 @@ type RepositoryFactory func(opt ...Option) (*Repository, error) // NewRepository creates a new session Repository. Supports the options: WithLimit // which sets a default limit on results returned by repo operations. -func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { +func NewRepository(ctx context.Context, r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { const op = "session.NewRepository" if r == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil reader") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil reader") } if w == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil writer") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil writer") } if kms == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil kms") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil kms") } opts := getOpts(opt...) if opts.withLimit == 0 { @@ -58,7 +58,7 @@ func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repo if opts.withPermissions != nil { for _, p := range opts.withPermissions.Permissions { if p.Resource != resource.Session { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "permission for incorrect resource") + return nil, errors.New(ctx, errors.InvalidParameter, op, "permission for incorrect resource") } } } diff --git a/internal/session/repository_connection_test.go b/internal/session/repository_connection_test.go index 8ee6f7b2c..732175306 100644 --- a/internal/session/repository_connection_test.go +++ b/internal/session/repository_connection_test.go @@ -25,7 +25,7 @@ func TestRepository_ListConnection(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit)) + repo, err := NewRepository(ctx, rw, rw, kms, WithLimit(testLimit)) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms, WithLimit(testLimit)) require.NoError(t, err) @@ -141,7 +141,7 @@ func TestRepository_ConnectConnection(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms) require.NoError(t, err) @@ -351,7 +351,7 @@ func TestRepository_orphanedConnections(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(err) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms, WithWorkerStateDelay(0)) require.NoError(err) @@ -446,7 +446,7 @@ func TestRepository_CloseConnections(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms) require.NoError(t, err) diff --git a/internal/session/repository_credential_test.go b/internal/session/repository_credential_test.go index 832f8298b..c9829eb47 100644 --- a/internal/session/repository_credential_test.go +++ b/internal/session/repository_credential_test.go @@ -19,7 +19,8 @@ func TestRepository_AddSessionCredentials(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) s := TestSession(t, conn, wrapper, TestSessionParams(t, conn, wrapper, iamRepo)) @@ -137,7 +138,8 @@ func TestRepository_ListSessionCredentials(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) s1 := TestSession(t, conn, wrapper, TestSessionParams(t, conn, wrapper, iamRepo)) s2 := TestSession(t, conn, wrapper, TestSessionParams(t, conn, wrapper, iamRepo)) diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index 30ef301a2..79103dcda 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -42,6 +42,7 @@ func TestRepository_ListSession(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) + ctx := context.Background() composedOf := TestSessionParams(t, conn, wrapper, iamRepo) listPerms := &perms.UserPermissions{ @@ -148,7 +149,7 @@ func TestRepository_ListSession(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(tt.perms)) + repo, err := NewRepository(ctx, rw, rw, kms, WithLimit(testLimit), WithPermissions(tt.perms)) require.NoError(err) db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") @@ -193,7 +194,7 @@ func TestRepository_ListSession(t *testing.T) { _ = TestSession(t, conn, wrapper, composedOf) } - repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(listPerms)) + repo, err := NewRepository(ctx, rw, rw, kms, WithLimit(testLimit), WithPermissions(listPerms)) require.NoError(err) got, err := repo.ListSessions(context.Background(), WithOrderByCreateTime(db.AscendingOrderBy)) @@ -226,7 +227,7 @@ func TestRepository_ListSession(t *testing.T) { }, }, } - repo, err := NewRepository(rw, rw, kms, WithLimit(testLimit), WithPermissions(p)) + repo, err := NewRepository(ctx, rw, rw, kms, WithLimit(testLimit), WithPermissions(p)) require.NoError(err) got, err := repo.ListSessions(context.Background(), WithUserId(s.UserId)) require.NoError(err) @@ -242,6 +243,7 @@ func TestRepository_ListSessions_Multiple_Scopes(t *testing.T) { iamRepo := iam.TestRepo(t, conn, wrapper) rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) + ctx := context.Background() db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") @@ -258,7 +260,7 @@ func TestRepository_ListSessions_Multiple_Scopes(t *testing.T) { _ = TestState(t, conn, s.PublicId, StatusActive) } - repo, err := NewRepository(rw, rw, kms, WithPermissions(&perms.UserPermissions{ + repo, err := NewRepository(ctx, rw, rw, kms, WithPermissions(&perms.UserPermissions{ Permissions: p, })) require.NoError(t, err) @@ -274,7 +276,8 @@ func TestRepository_CreateSession(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) workerAddresses := []string{"1.2.3.4"} @@ -469,7 +472,8 @@ func TestRepository_updateState(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) tests := []struct { @@ -583,7 +587,8 @@ func TestRepository_transitionState(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) tofu := TestTofu(t) @@ -680,7 +685,7 @@ func TestRepository_TerminateCompletedSessions(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) connRepo, err := NewConnectionRepository(ctx, rw, rw, kms) require.NoError(t, err) @@ -919,7 +924,7 @@ func TestRepository_CancelSession(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) testKms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, testKms) + repo, err := NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, testKms) require.NoError(t, err) @@ -1097,7 +1102,8 @@ func TestRepository_CancelSessionViaFKNull(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) setupFn := func() *Session { session := TestDefaultSession(t, conn, wrapper, iamRepo) @@ -1276,7 +1282,8 @@ func TestRepository_ActivateSession(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) tofu := TestTofu(t) @@ -1399,7 +1406,8 @@ func TestRepository_DeleteSession(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) require.NoError(t, err) type args struct { @@ -1520,7 +1528,8 @@ func TestRepository_deleteTargetFKey(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + ctx := context.Background() + repo, err := NewRepository(ctx, rw, rw, kms) targetRepo, err := target.NewRepository(rw, rw, kms) require.NoError(t, err) @@ -1569,7 +1578,7 @@ func TestRepository_deleteTerminated(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) kms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kms) + repo, err := NewRepository(ctx, rw, rw, kms) composedOf := TestSessionParams(t, conn, wrapper, iamRepo) cases := []struct { diff --git a/internal/session/repository_test.go b/internal/session/repository_test.go index 4a2ed7878..dc9983e07 100644 --- a/internal/session/repository_test.go +++ b/internal/session/repository_test.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/boundary/internal/kms" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/net/context" ) func TestNewRepository(t *testing.T) { @@ -15,6 +16,7 @@ func TestNewRepository(t *testing.T) { rw := db.New(conn) wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) + ctx := context.Background() type args struct { r db.Reader @@ -36,7 +38,7 @@ func TestNewRepository(t *testing.T) { k: testKms, }, want: func() *Repository { - ret, err := NewRepository(rw, rw, testKms) + ret, err := NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) return ret }(), @@ -66,7 +68,7 @@ func TestNewRepository(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - got, err := NewRepository(tt.args.r, tt.args.w, tt.args.k) + got, err := NewRepository(ctx, tt.args.r, tt.args.w, tt.args.k) if tt.wantErr { require.Error(err) assert.Equal(tt.wantErrString, err.Error()) diff --git a/internal/session/service_authorize_connection_test.go b/internal/session/service_authorize_connection_test.go index 39582a086..0f254e3e6 100644 --- a/internal/session/service_authorize_connection_test.go +++ b/internal/session/service_authorize_connection_test.go @@ -29,7 +29,7 @@ func TestService_AuthorizeConnection(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) testKms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, testKms) + repo, err := NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, testKms) require.NoError(t, err) diff --git a/internal/session/service_close_connections_test.go b/internal/session/service_close_connections_test.go index 8ccbad1f1..5e378c2a2 100644 --- a/internal/session/service_close_connections_test.go +++ b/internal/session/service_close_connections_test.go @@ -27,7 +27,7 @@ func TestServiceCloseConnections(t *testing.T) { wrapper := db.TestWrapper(t) iamRepo := iam.TestRepo(t, conn, wrapper) testKms := kms.TestKms(t, conn, wrapper) - repo, err := NewRepository(rw, rw, testKms) + repo, err := NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) connRepo, err := NewConnectionRepository(ctx, rw, rw, testKms) require.NoError(t, err) diff --git a/internal/session/service_worker_status_report_test.go b/internal/session/service_worker_status_report_test.go index 20a4c8e7c..5d89a6165 100644 --- a/internal/session/service_worker_status_report_test.go +++ b/internal/session/service_worker_status_report_test.go @@ -33,7 +33,7 @@ func TestWorkerStatusReport(t *testing.T) { }) require.NoError(t, err) - repo, err := session.NewRepository(rw, rw, kms) + repo, err := session.NewRepository(ctx, rw, rw, kms) require.NoError(t, err) connRepo, err := session.NewConnectionRepository(ctx, rw, rw, kms, session.WithWorkerStateDelay(0)) require.NoError(t, err) diff --git a/testing/dbtest/session_list_benchmarks_dump_generation_test.go b/testing/dbtest/session_list_benchmarks_dump_generation_test.go index 6ef81f3ad..60c6aa2d7 100644 --- a/testing/dbtest/session_list_benchmarks_dump_generation_test.go +++ b/testing/dbtest/session_list_benchmarks_dump_generation_test.go @@ -126,7 +126,7 @@ func TestGenerateSessionBenchmarkTemplateDumps(t *testing.T) { require.NoError(err) pwRepo, err := password.NewRepository(rw, rw, kms) require.NoError(err) - sessRepo, err := session.NewRepository(rw, rw, kms) + sessRepo, err := session.NewRepository(ctx, rw, rw, kms) require.NoError(err) connRepo, err := session.NewConnectionRepository(ctx, rw, rw, kms) require.NoError(err) From e65f1f32a79153ce6672302dca0f894e7fae4ec4 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Wed, 24 Aug 2022 13:18:08 +0000 Subject: [PATCH 11/16] chore(session): Remove repository method that was only used in tests This unexported method was only used by a single test, and was taking advantage of its implmentation to populate Connections rather than Sessions. This repository should only interact with Sessions, since there is a separate repository for Connections. --- internal/session/job_session_cleanup_test.go | 2 +- internal/session/repository.go | 24 -------------------- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/internal/session/job_session_cleanup_test.go b/internal/session/job_session_cleanup_test.go index 8017c24f5..c65b45443 100644 --- a/internal/session/job_session_cleanup_test.go +++ b/internal/session/job_session_cleanup_test.go @@ -306,7 +306,7 @@ func TestCloseConnectionsForDeadWorkers(t *testing.T) { t.Helper() var conns []*Connection - require.NoError(repo.list(ctx, &conns, "", nil)) + require.NoError(rw.SearchWhere(ctx, &conns, "", nil)) for i, connId := range connIds { var expected ConnectionStatus switch { diff --git a/internal/session/repository.go b/internal/session/repository.go index f43a48ba5..c09ed4b33 100644 --- a/internal/session/repository.go +++ b/internal/session/repository.go @@ -108,30 +108,6 @@ func (r *Repository) listPermissionWhereClauses() ([]string, []interface{}) { return where, args } -// list will return a listing of resources and honor the WithLimit option or the -// repo defaultLimit. Supports WithOrder option. -func (r *Repository) list(ctx context.Context, resources interface{}, where string, args []interface{}, opt ...Option) error { - const op = "session.(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)) - switch opts.withOrderByCreateTime { - case db.AscendingOrderBy: - dbOpts = append(dbOpts, db.WithOrder("create_time asc")) - case db.DescendingOrderBy: - dbOpts = append(dbOpts, db.WithOrder("create_time")) - } - if err := r.reader.SearchWhere(ctx, resources, where, args, dbOpts...); err != nil { - return errors.Wrap(ctx, err, op) - } - return nil -} - func (r *Repository) convertToSessions(ctx context.Context, sessionList []*sessionListView, opt ...Option) ([]*Session, error) { const op = "session.(Repository).convertToSessions" opts := getOpts(opt...) From 1e1e5a5ff5ebae5a365596b2f7dc7e2e6dbf89e1 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Fri, 26 Aug 2022 17:55:14 +0000 Subject: [PATCH 12/16] doc(perms): Clarify some details in godoc for ListPermissions --- internal/perms/acl.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 47ab170df..d64cf9a26 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -228,8 +228,13 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option return } -// ListPermissions builds a set of Permissions based on the grants in the ACL for the List action, for the requested scopes and resource type. -func (a ACL) ListPermissions(requestedScopes map[string]*scopes.ScopeInfo, requestedType resource.Type, requestedActions action.ActionSet) []Permission { +// ListPermissions builds a set of Permissions based on the grants in the ACL. +// Permissions are determined for the given resource for each of the provided scopes. +// There must be a grant for a given resource for one of the provided "id actions" +// or for action.All in order for a Permission to be created for the scope. +// The set of "id actions" is resource dependant, but will generally include all +// actions that can be taken on an individual resource. +func (a ACL) ListPermissions(requestedScopes map[string]*scopes.ScopeInfo, requestedType resource.Type, idActions action.ActionSet) []Permission { perms := make([]Permission, 0, len(requestedScopes)) for scopeId := range requestedScopes { p := Permission{ @@ -252,7 +257,7 @@ func (a ACL) ListPermissions(requestedScopes map[string]*scopes.ScopeInfo, reque if ok := grant.actions[action.All]; ok { found = true } else { - for _, a := range requestedActions { + for _, a := range idActions { if ok := grant.actions[a]; ok { found = true break From 224f8d2d4ece315b14845518355c5fb8f474566a Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Tue, 23 Aug 2022 14:57:38 +0100 Subject: [PATCH 13/16] refact(controller): Move Target Repository Factory to the target package --- internal/daemon/controller/common/common.go | 2 -- internal/daemon/controller/controller.go | 2 +- internal/daemon/controller/handlers/targets/target_service.go | 4 ++-- internal/target/repository.go | 4 ++++ 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/daemon/controller/common/common.go b/internal/daemon/controller/common/common.go index 20744a222..d0b1d9d00 100644 --- a/internal/daemon/controller/common/common.go +++ b/internal/daemon/controller/common/common.go @@ -11,7 +11,6 @@ import ( hostplugin "github.com/hashicorp/boundary/internal/plugin/host" "github.com/hashicorp/boundary/internal/server" "github.com/hashicorp/boundary/internal/session" - "github.com/hashicorp/boundary/internal/target" ) type ( @@ -26,6 +25,5 @@ type ( PluginHostRepoFactory func() (*pluginhost.Repository, error) HostPluginRepoFactory func() (*hostplugin.Repository, error) ConnectionRepoFactory func() (*session.ConnectionRepository, error) - TargetRepoFactory func() (*target.Repository, error) WorkerAuthRepoStorageFactory func() (*server.WorkerAuthRepositoryStorage, error) ) diff --git a/internal/daemon/controller/controller.go b/internal/daemon/controller/controller.go index 01935b3bf..500b2d709 100644 --- a/internal/daemon/controller/controller.go +++ b/internal/daemon/controller/controller.go @@ -115,7 +115,7 @@ type Controller struct { StaticHostRepoFn common.StaticRepoFactory PluginHostRepoFn common.PluginHostRepoFactory HostPluginRepoFn common.HostPluginRepoFactory - TargetRepoFn common.TargetRepoFactory + TargetRepoFn target.RepositoryFactory WorkerAuthRepoStorageFn common.WorkerAuthRepoStorageFactory scheduler *scheduler.Scheduler diff --git a/internal/daemon/controller/handlers/targets/target_service.go b/internal/daemon/controller/handlers/targets/target_service.go index 0b1a3dce7..f5c45dfc2 100644 --- a/internal/daemon/controller/handlers/targets/target_service.go +++ b/internal/daemon/controller/handlers/targets/target_service.go @@ -98,7 +98,7 @@ var ( type Service struct { pbs.UnsafeTargetServiceServer - repoFn common.TargetRepoFactory + repoFn target.RepositoryFactory iamRepoFn common.IamRepoFactory serversRepoFn common.ServersRepoFactory sessionRepoFn session.RepositoryFactory @@ -115,7 +115,7 @@ var _ pbs.TargetServiceServer = (*Service)(nil) func NewService( ctx context.Context, kmsCache *kms.Kms, - repoFn common.TargetRepoFactory, + repoFn target.RepositoryFactory, iamRepoFn common.IamRepoFactory, serversRepoFn common.ServersRepoFactory, sessionRepoFn session.RepositoryFactory, diff --git a/internal/target/repository.go b/internal/target/repository.go index 2f65552f5..874b9de89 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -15,6 +15,10 @@ import ( "github.com/hashicorp/go-dbw" ) +// RepositoryFactory enables `target.Repository` object instantiation, +// and is used by the various service packages/controller object to do so. +type RepositoryFactory func() (*Repository, error) + // Cloneable provides a cloning interface type Cloneable interface { Clone() Target From 63196ee4caa0907677cb92f0f494a73398042f61 Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Tue, 23 Aug 2022 17:27:56 +0100 Subject: [PATCH 14/16] feat(target): Implement new WithPermissions option Also changes the Target RepositoryFactory to allow for passing options at construction time. --- internal/daemon/controller/controller.go | 4 +- .../targets/tcp/target_service_test.go | 14 ++--- internal/target/options.go | 11 ++++ internal/target/options_test.go | 8 +++ internal/target/repository.go | 26 +++++++-- internal/target/repository_test.go | 53 +++++++++++++++++-- 6 files changed, 100 insertions(+), 16 deletions(-) diff --git a/internal/daemon/controller/controller.go b/internal/daemon/controller/controller.go index 500b2d709..b4427bcb8 100644 --- a/internal/daemon/controller/controller.go +++ b/internal/daemon/controller/controller.go @@ -333,8 +333,8 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { c.PasswordAuthRepoFn = func() (*password.Repository, error) { return password.NewRepository(dbase, dbase, c.kms) } - c.TargetRepoFn = func() (*target.Repository, error) { - return target.NewRepository(dbase, dbase, c.kms) + c.TargetRepoFn = func(o ...target.Option) (*target.Repository, error) { + return target.NewRepository(dbase, dbase, c.kms, o...) } c.SessionRepoFn = func(opt ...session.Option) (*session.Repository, error) { return session.NewRepository(ctx, dbase, dbase, c.kms, opt...) diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index bbbbf1c29..98b5c50c5 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -76,8 +76,8 @@ var testAuthorizedActions = []string{ func testService(t *testing.T, ctx context.Context, conn *db.DB, kms *kms.Kms, wrapper wrapping.Wrapper) (targets.Service, error) { rw := db.New(conn) sche := scheduler.TestScheduler(t, conn, wrapper) - repoFn := func() (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + repoFn := func(o ...target.Option) (*target.Repository, error) { + return target.NewRepository(rw, rw, kms, o...) } iamRepoFn := func() (*iam.Repository, error) { return iam.TestRepo(t, conn, wrapper), nil @@ -710,7 +710,7 @@ func TestUpdate(t *testing.T) { _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") - repoFn := func() (*target.Repository, error) { + repoFn := func(o ...target.Option) (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } repo, err := repoFn() @@ -1134,7 +1134,7 @@ func TestUpdate_BadVersion(t *testing.T) { _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") - repoFn := func() (*target.Repository, error) { + repoFn := func(o ...target.Option) (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } repo, err := repoFn() @@ -2739,7 +2739,7 @@ func TestAuthorizeSession(t *testing.T) { err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) require.NoError(t, err) - repoFn := func() (*target.Repository, error) { + repoFn := func(o ...target.Option) (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) @@ -2999,7 +2999,7 @@ func TestAuthorizeSessionTypedCredentials(t *testing.T) { err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) require.NoError(t, err) - repoFn := func() (*target.Repository, error) { + repoFn := func(o ...target.Option) (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) @@ -3578,7 +3578,7 @@ func TestAuthorizeSession_Errors(t *testing.T) { err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) require.NoError(t, err) - repoFn := func() (*target.Repository, error) { + repoFn := func(o ...target.Option) (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) diff --git a/internal/target/options.go b/internal/target/options.go index cdfb6dd8c..18c1c8c08 100644 --- a/internal/target/options.go +++ b/internal/target/options.go @@ -3,6 +3,7 @@ package target import ( "time" + "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/types/subtypes" ) @@ -34,6 +35,7 @@ type options struct { WithStaticCredentials []*StaticCredential WithSessionMaxSeconds uint32 WithSessionConnectionLimit int32 + WithPermissions []perms.Permission WithPublicId string WithWorkerFilter string WithTargetIds []string @@ -55,6 +57,7 @@ func getDefaultOptions() options { WithStaticCredentials: nil, WithSessionMaxSeconds: uint32((8 * time.Hour).Seconds()), WithSessionConnectionLimit: -1, + WithPermissions: nil, WithPublicId: "", WithWorkerFilter: "", } @@ -178,3 +181,11 @@ func WithTargetIds(with []string) Option { o.WithTargetIds = with } } + +// WithPermissions is used by this repo to restrict a list +// request's results based on the given set of permissions. +func WithPermissions(perms []perms.Permission) Option { + return func(o *options) { + o.WithPermissions = perms + } +} diff --git a/internal/target/options_test.go b/internal/target/options_test.go index d88d8d6b1..c95c68443 100644 --- a/internal/target/options_test.go +++ b/internal/target/options_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/hashicorp/boundary/internal/credential" + "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/target/store" "github.com/hashicorp/boundary/internal/types/subtypes" "github.com/stretchr/testify/assert" @@ -108,6 +109,13 @@ func Test_GetOpts(t *testing.T) { testOpts.WithWorkerFilter = `"/foo" == "bar"` assert.Equal(opts, testOpts) }) + t.Run("WithPermissions", func(t *testing.T) { + assert := assert.New(t) + opts := GetOpts(WithPermissions([]perms.Permission{{ScopeId: "test1"}, {ScopeId: "test2"}})) + testOpts := getDefaultOptions() + testOpts.WithPermissions = []perms.Permission{{ScopeId: "test1"}, {ScopeId: "test2"}} + assert.Equal(opts, testOpts) + }) t.Run("WithCredentialLibraries", func(t *testing.T) { assert := assert.New(t) opts := GetOpts(WithCredentialLibraries([]*CredentialLibrary{ diff --git a/internal/target/repository.go b/internal/target/repository.go index 874b9de89..07594e92d 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -11,13 +11,15 @@ import ( "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" + "github.com/hashicorp/boundary/internal/perms" + "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "github.com/hashicorp/go-dbw" ) // RepositoryFactory enables `target.Repository` object instantiation, // and is used by the various service packages/controller object to do so. -type RepositoryFactory func() (*Repository, error) +type RepositoryFactory func(...Option) (*Repository, error) // Cloneable provides a cloning interface type Cloneable interface { @@ -32,10 +34,19 @@ type Repository struct { // defaultLimit provides a default for limiting the number of results returned from the repo defaultLimit int + + // permissions provides a set of user permissions - these directly correlate to what the user + // has access to in terms of actions and resources and we use it to build queries. + // These are passed in on the repository constructor using `WithPermissions`, meaning the + // `Repository` object is contextualized to whatever the request context is. + permissions []perms.Permission } -// NewRepository creates a new target Repository. Supports the options: WithLimit -// which sets a default limit on results returned by repo operations. +// NewRepository creates a new target Repository. +// Supports the following options: +// - WithLimit: sets a limit on the number of results returned by various repo operations. +// - WithPermissions: defines the permissions the user has to perform different +// actions and access resources within the created repo object. func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { const op = "target.NewRepository" if r == nil { @@ -47,16 +58,25 @@ func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repo if kms == nil { return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil kms") } + opts := GetOpts(opt...) if opts.WithLimit == 0 { // zero signals the boundary defaults should be used. opts.WithLimit = db.DefaultLimit } + + for _, p := range opts.WithPermissions { + if p.Resource != resource.Target { + return nil, errors.New(context.Background(), errors.InvalidParameter, op, "permission for incorrect resource found") + } + } + return &Repository{ reader: r, writer: w, kms: kms, defaultLimit: opts.WithLimit, + permissions: opts.WithPermissions, }, nil } diff --git a/internal/target/repository_test.go b/internal/target/repository_test.go index 6187421a3..e26b06c4f 100644 --- a/internal/target/repository_test.go +++ b/internal/target/repository_test.go @@ -5,6 +5,8 @@ import ( "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/perms" + "github.com/hashicorp/boundary/internal/types/resource" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -16,9 +18,10 @@ func TestNewRepository(t *testing.T) { wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) type args struct { - r db.Reader - w db.Writer - kms *kms.Kms + r db.Reader + w db.Writer + kms *kms.Kms + opts []Option } tests := []struct { name string @@ -75,11 +78,53 @@ func TestNewRepository(t *testing.T) { wantErr: true, wantErrString: "target.NewRepository: nil reader: parameter violation: error #100", }, + { + name: "WithPermissions sets object to `permissions`", + args: args{ + r: rw, + w: rw, + kms: testKms, + opts: []Option{ + WithPermissions([]perms.Permission{ + {ScopeId: "test1", Resource: resource.Target}, + {ScopeId: "test2", Resource: resource.Target}, + }), + }, + }, + want: &Repository{ + reader: rw, + writer: rw, + kms: testKms, + defaultLimit: db.DefaultLimit, + permissions: []perms.Permission{ + {ScopeId: "test1", Resource: resource.Target}, + {ScopeId: "test2", Resource: resource.Target}, + }, + }, + wantErr: false, + }, + { + name: "Don't accept permissions that aren't for the Target resource", + args: args{ + r: rw, + w: rw, + kms: testKms, + opts: []Option{ + WithPermissions([]perms.Permission{ + {ScopeId: "test1", Resource: resource.Target}, + {ScopeId: "test2", Resource: resource.Host}, + }), + }, + }, + want: nil, + wantErr: true, + wantErrString: "target.NewRepository: permission for incorrect resource found: parameter violation: error #100", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - got, err := NewRepository(tt.args.r, tt.args.w, tt.args.kms) + got, err := NewRepository(tt.args.r, tt.args.w, tt.args.kms, tt.args.opts...) if tt.wantErr { require.Error(err) assert.Equal(tt.wantErrString, err.Error()) From bf263fbd7db2cfbe2c98868bc6547e086fe31d5d Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Tue, 23 Aug 2022 17:32:36 +0100 Subject: [PATCH 15/16] 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 | 101 +++++----- internal/target/repository_test.go | 172 ++++++++++++++++++ internal/target/tcp/repository_test.go | 131 +++---------- 4 files changed, 263 insertions(+), 194 deletions(-) diff --git a/internal/daemon/controller/handlers/targets/target_service.go b/internal/daemon/controller/handlers/targets/target_service.go index f5c45dfc2..f2b603e83 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 07594e92d..7ec5d1b2d 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" + + if len(r.permissions) == 0 { + return []Target{}, nil + } + + where, args := r.listPermissionWhereClauses() + if len(where) == 0 { + return []Target{}, nil + } + 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, ","))) - } - - 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, ","))) - } - - 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())) + 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 +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 "))) } - dbOpts = append(dbOpts, db.WithLimit(limit)) - if err := r.reader.SearchWhere(ctx, resources, where, args, dbOpts...); err != nil { - return errors.Wrap(ctx, err, op) - } - 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 e26b06c4f..8468756f2 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 d5ca11008..b7c8e3c60 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)) } From bf1486f75efcde4568b2197379c54a7e94eb62b8 Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Tue, 23 Aug 2022 18:44:10 +0100 Subject: [PATCH 16/16] refact(target): Add context to RepositoryFactory constructor `errors.New` requires a context, so I've made that available from the Controller, and updated all `NewDeprecated` functions to `New` while I was at it --- internal/cmd/base/initial_resources.go | 2 +- internal/daemon/controller/controller.go | 2 +- .../handlers/targets/tcp/target_service_test.go | 16 ++++++++-------- internal/session/repository_session_test.go | 6 +++--- .../session/service_authorize_connection_test.go | 2 +- .../session/service_close_connections_test.go | 2 +- internal/session/testing.go | 2 +- internal/target/repository.go | 10 +++++----- .../target/repository_credential_source_test.go | 2 +- internal/target/repository_test.go | 3 ++- internal/target/tcp/immutable_fields_test.go | 5 +++-- .../tcp/repository_credential_source_test.go | 6 +++--- .../target/tcp/repository_host_source_test.go | 12 ++++++------ .../target/tcp/repository_tcp_target_test.go | 8 ++++---- internal/target/tcp/repository_test.go | 9 +++++---- internal/target/tcp/testing_test.go | 10 ++++++---- 16 files changed, 51 insertions(+), 46 deletions(-) diff --git a/internal/cmd/base/initial_resources.go b/internal/cmd/base/initial_resources.go index e32f27611..38218c293 100644 --- a/internal/cmd/base/initial_resources.go +++ b/internal/cmd/base/initial_resources.go @@ -478,7 +478,7 @@ func (b *Server) CreateInitialTarget(ctx context.Context) (target.Target, error) } }() - targetRepo, err := target.NewRepository(rw, rw, kmsCache) + targetRepo, err := target.NewRepository(ctx, rw, rw, kmsCache) if err != nil { return nil, fmt.Errorf("error creating target repository: %w", err) } diff --git a/internal/daemon/controller/controller.go b/internal/daemon/controller/controller.go index b4427bcb8..ef2123fdc 100644 --- a/internal/daemon/controller/controller.go +++ b/internal/daemon/controller/controller.go @@ -334,7 +334,7 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { return password.NewRepository(dbase, dbase, c.kms) } c.TargetRepoFn = func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(dbase, dbase, c.kms, o...) + return target.NewRepository(ctx, dbase, dbase, c.kms, o...) } c.SessionRepoFn = func(opt ...session.Option) (*session.Repository, error) { return session.NewRepository(ctx, dbase, dbase, c.kms, opt...) diff --git a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go index 98b5c50c5..5a3e26fff 100644 --- a/internal/daemon/controller/handlers/targets/tcp/target_service_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/target_service_test.go @@ -77,7 +77,7 @@ func testService(t *testing.T, ctx context.Context, conn *db.DB, kms *kms.Kms, w rw := db.New(conn) sche := scheduler.TestScheduler(t, conn, wrapper) repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms, o...) + return target.NewRepository(ctx, rw, rw, kms, o...) } iamRepoFn := func() (*iam.Repository, error) { return iam.TestRepo(t, conn, wrapper), nil @@ -710,8 +710,9 @@ func TestUpdate(t *testing.T) { _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") + ctx := context.Background() repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + return target.NewRepository(ctx, rw, rw, kms) } repo, err := repoFn() require.NoError(t, err, "Couldn't create new target repo.") @@ -729,7 +730,6 @@ func TestUpdate(t *testing.T) { {Id: hs[1].GetPublicId(), HostCatalogId: hs[1].GetCatalogId()}, } - ctx := context.Background() ttar, err := target.New(ctx, tcp.Subtype, proj.GetPublicId(), target.WithName("default"), target.WithDescription("default"), @@ -1134,13 +1134,13 @@ func TestUpdate_BadVersion(t *testing.T) { _ = iam.TestUserRole(t, conn, r.GetPublicId(), at.GetIamUserId()) _ = iam.TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") + ctx := context.Background() repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + return target.NewRepository(ctx, rw, rw, kms) } repo, err := repoFn() require.NoError(t, err, "Couldn't create new target repo.") - ctx := context.Background() ttar, err := target.New(ctx, tcp.Subtype, proj.GetPublicId(), target.WithName("default"), target.WithDescription("default")) tar := ttar.(*tcp.Target) tar.DefaultPort = 2 @@ -2740,7 +2740,7 @@ func TestAuthorizeSession(t *testing.T) { require.NoError(t, err) repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + return target.NewRepository(ctx, rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { @@ -3000,7 +3000,7 @@ func TestAuthorizeSessionTypedCredentials(t *testing.T) { require.NoError(t, err) repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + return target.NewRepository(ctx, rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { @@ -3579,7 +3579,7 @@ func TestAuthorizeSession_Errors(t *testing.T) { require.NoError(t, err) repoFn := func(o ...target.Option) (*target.Repository, error) { - return target.NewRepository(rw, rw, kms) + return target.NewRepository(ctx, rw, rw, kms) } iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index 79103dcda..4191fdcf1 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -963,7 +963,7 @@ func TestRepository_CancelSession(t *testing.T) { // is terminated when the one connection is closed. tcpTarget := tcp.TestTarget(ctx, t, conn, proj.PublicId, "test target", target.WithSessionConnectionLimit(1)) - targetRepo, err := target.NewRepository(rw, rw, testKms) + targetRepo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) _, _, _, err = targetRepo.AddTargetHostSources(ctx, tcpTarget.GetPublicId(), tcpTarget.GetVersion(), []string{sets[0].PublicId}) require.NoError(t, err) @@ -1491,7 +1491,7 @@ func testSessionCredentialParams(t *testing.T, conn *db.DB, wrapper wrapping.Wra ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - targetRepo, err := target.NewRepository(rw, rw, kms) + targetRepo, err := target.NewRepository(ctx, rw, rw, kms) require.NoError(err) tar, _, _, err := targetRepo.LookupTarget(ctx, params.TargetId) require.NoError(err) @@ -1530,7 +1530,7 @@ func TestRepository_deleteTargetFKey(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) ctx := context.Background() repo, err := NewRepository(ctx, rw, rw, kms) - targetRepo, err := target.NewRepository(rw, rw, kms) + targetRepo, err := target.NewRepository(ctx, rw, rw, kms) require.NoError(t, err) tests := []struct { diff --git a/internal/session/service_authorize_connection_test.go b/internal/session/service_authorize_connection_test.go index 0f254e3e6..3e6df2937 100644 --- a/internal/session/service_authorize_connection_test.go +++ b/internal/session/service_authorize_connection_test.go @@ -90,7 +90,7 @@ func TestService_AuthorizeConnection(t *testing.T) { // is terminated when the one connection is closed. tcpTarget := tcp.TestTarget(ctx, t, conn, proj.PublicId, "test target", target.WithSessionConnectionLimit(1)) - targetRepo, err := target.NewRepository(rw, rw, testKms) + targetRepo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) _, _, _, err = targetRepo.AddTargetHostSources(ctx, tcpTarget.GetPublicId(), tcpTarget.GetVersion(), []string{sets[0].PublicId}) require.NoError(t, err) diff --git a/internal/session/service_close_connections_test.go b/internal/session/service_close_connections_test.go index 5e378c2a2..cff2a8577 100644 --- a/internal/session/service_close_connections_test.go +++ b/internal/session/service_close_connections_test.go @@ -51,7 +51,7 @@ func TestServiceCloseConnections(t *testing.T) { // is terminated when the one connection is closed. tcpTarget := tcp.TestTarget(ctx, t, conn, proj.PublicId, "test target", target.WithSessionConnectionLimit(1)) - targetRepo, err := target.NewRepository(rw, rw, testKms) + targetRepo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) _, _, _, err = targetRepo.AddTargetHostSources(ctx, tcpTarget.GetPublicId(), tcpTarget.GetVersion(), []string{sets[0].PublicId}) require.NoError(t, err) diff --git a/internal/session/testing.go b/internal/session/testing.go index 65b4dc3a2..09857b634 100644 --- a/internal/session/testing.go +++ b/internal/session/testing.go @@ -144,7 +144,7 @@ func TestSessionParams(t testing.TB, conn *db.DB, wrapper wrapping.Wrapper, iamR tcpTarget := tcp.TestTarget(ctx, t, conn, proj.PublicId, "test target") kms := kms.TestKms(t, conn, wrapper) - targetRepo, err := target.NewRepository(rw, rw, kms) + targetRepo, err := target.NewRepository(ctx, rw, rw, kms) require.NoError(err) _, _, _, err = targetRepo.AddTargetHostSources(ctx, tcpTarget.GetPublicId(), tcpTarget.GetVersion(), []string{sets[0].PublicId}) require.NoError(err) diff --git a/internal/target/repository.go b/internal/target/repository.go index 7ec5d1b2d..c7175b347 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -48,16 +48,16 @@ type Repository struct { // - WithLimit: sets a limit on the number of results returned by various repo operations. // - WithPermissions: defines the permissions the user has to perform different // actions and access resources within the created repo object. -func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { +func NewRepository(ctx context.Context, r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repository, error) { const op = "target.NewRepository" if r == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil reader") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil reader") } if w == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil writer") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil writer") } if kms == nil { - return nil, errors.NewDeprecated(errors.InvalidParameter, op, "nil kms") + return nil, errors.New(ctx, errors.InvalidParameter, op, "nil kms") } opts := GetOpts(opt...) @@ -68,7 +68,7 @@ func NewRepository(r db.Reader, w db.Writer, kms *kms.Kms, opt ...Option) (*Repo for _, p := range opts.WithPermissions { if p.Resource != resource.Target { - return nil, errors.New(context.Background(), errors.InvalidParameter, op, "permission for incorrect resource found") + return nil, errors.New(ctx, errors.InvalidParameter, op, "permission for incorrect resource found") } } diff --git a/internal/target/repository_credential_source_test.go b/internal/target/repository_credential_source_test.go index 049a05a47..2f4f950e0 100644 --- a/internal/target/repository_credential_source_test.go +++ b/internal/target/repository_credential_source_test.go @@ -48,7 +48,7 @@ func TestRepository_SetTargetCredentialSources(t *testing.T) { rw := db.New(conn) wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) diff --git a/internal/target/repository_test.go b/internal/target/repository_test.go index 8468756f2..04a70d33b 100644 --- a/internal/target/repository_test.go +++ b/internal/target/repository_test.go @@ -1,6 +1,7 @@ package target import ( + "context" "database/sql" "testing" @@ -126,7 +127,7 @@ func TestNewRepository(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - got, err := NewRepository(tt.args.r, tt.args.w, tt.args.kms, tt.args.opts...) + got, err := NewRepository(context.Background(), tt.args.r, tt.args.w, tt.args.kms, tt.args.opts...) if tt.wantErr { require.Error(err) assert.Equal(tt.wantErrString, err.Error()) diff --git a/internal/target/tcp/immutable_fields_test.go b/internal/target/tcp/immutable_fields_test.go index fb2871f95..ae23a025c 100644 --- a/internal/target/tcp/immutable_fields_test.go +++ b/internal/target/tcp/immutable_fields_test.go @@ -158,14 +158,15 @@ func TestTargetHostSet_ImmutableFields(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) + + ctx := context.Background() rw := db.New(conn) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) ts := timestamp.Timestamp{Timestamp: ×tamppb.Timestamp{Seconds: 0, Nanos: 0}} _, proj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) - ctx := context.Background() projTarget := tcp.TestTarget(ctx, t, conn, proj.PublicId, tcp.TestId(t)) testCats := static.TestCatalogs(t, conn, proj.PublicId, 1) hsets := static.TestSets(t, conn, testCats[0].GetPublicId(), 2) diff --git a/internal/target/tcp/repository_credential_source_test.go b/internal/target/tcp/repository_credential_source_test.go index 79c76d9bc..65fe022c2 100644 --- a/internal/target/tcp/repository_credential_source_test.go +++ b/internal/target/tcp/repository_credential_source_test.go @@ -29,7 +29,7 @@ func TestRepository_AddTargetCredentialSources(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, staticProj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) csVault := vault.TestCredentialStores(t, conn, wrapper, staticProj.GetPublicId(), 1)[0] @@ -336,7 +336,7 @@ func TestRepository_DeleteTargetCredentialSources(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, proj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) type args struct { @@ -563,7 +563,7 @@ func TestRepository_SetTargetCredentialSources(t *testing.T) { rw := db.New(conn) wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) diff --git a/internal/target/tcp/repository_host_source_test.go b/internal/target/tcp/repository_host_source_test.go index 8767820fd..be2fa10f8 100644 --- a/internal/target/tcp/repository_host_source_test.go +++ b/internal/target/tcp/repository_host_source_test.go @@ -34,7 +34,7 @@ func TestRepository_AddTargetHostSets(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, staticProj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) createHostSetsFn := func(projects []string) []string { @@ -199,10 +199,10 @@ func TestRepository_DeleteTargetHosts(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, proj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) - require.NoError(t, err) ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) + require.NoError(t, err) type args struct { target target.Target @@ -404,7 +404,9 @@ func TestRepository_SetTargetHostSets(t *testing.T) { rw := db.New(conn) wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) - repo, err := target.NewRepository(rw, rw, testKms) + + ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) @@ -427,8 +429,6 @@ func TestRepository_SetTargetHostSets(t *testing.T) { return results } - ctx := context.Background() - setupFn := func(target target.Target) []target.HostSource { hs := createHostSetsFn() _, created, _, err := repo.AddTargetHostSources(context.Background(), target.GetPublicId(), 1, hs) diff --git a/internal/target/tcp/repository_tcp_target_test.go b/internal/target/tcp/repository_tcp_target_test.go index 109daf05a..afaa21a79 100644 --- a/internal/target/tcp/repository_tcp_target_test.go +++ b/internal/target/tcp/repository_tcp_target_test.go @@ -28,7 +28,9 @@ func TestRepository_CreateTarget(t *testing.T) { rw := db.New(conn) wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) - repo, err := target.NewRepository(rw, rw, testKms) + + ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) _, proj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) @@ -38,8 +40,6 @@ func TestRepository_CreateTarget(t *testing.T) { cs := vault.TestCredentialStores(t, conn, wrapper, proj.GetPublicId(), 1)[0] vault.TestCredentialLibraries(t, conn, wrapper, cs.GetPublicId(), 2) - ctx := context.Background() - type args struct { target target.Target opt []target.Option @@ -165,7 +165,7 @@ func TestRepository_UpdateTcpTarget(t *testing.T) { wrapper := db.TestWrapper(t) testKms := kms.TestKms(t, conn, wrapper) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(context.Background(), rw, rw, testKms) require.NoError(t, err) id := tcp.TestId(t) diff --git a/internal/target/tcp/repository_test.go b/internal/target/tcp/repository_test.go index b7c8e3c60..5df9d664b 100644 --- a/internal/target/tcp/repository_test.go +++ b/internal/target/tcp/repository_test.go @@ -32,7 +32,7 @@ func TestRepository_LookupTarget(t *testing.T) { _, _, err := iamRepo.UpdateScope(ctx, proj, 1, []string{"name"}) require.NoError(t, err) rw := db.New(conn) - repo, err := target.NewRepository(rw, rw, testKms) + repo, err := target.NewRepository(ctx, rw, rw, testKms) require.NoError(t, err) tgt := tcp.TestTarget(ctx, t, conn, proj.PublicId, "target-name") @@ -160,7 +160,7 @@ func TestRepository_ListRoles_Multiple_Scopes(t *testing.T) { } rw := db.New(conn) - repo, err := target.NewRepository(rw, rw, testKms, + repo, err := target.NewRepository(ctx, rw, rw, testKms, target.WithPermissions([]perms.Permission{ { ScopeId: proj1.PublicId, @@ -191,10 +191,11 @@ func TestRepository_DeleteTarget(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, proj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) - require.NoError(t, err) ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) + require.NoError(t, err) + type args struct { target target.Target opt []target.Option diff --git a/internal/target/tcp/testing_test.go b/internal/target/tcp/testing_test.go index bcc402f5a..785135ac2 100644 --- a/internal/target/tcp/testing_test.go +++ b/internal/target/tcp/testing_test.go @@ -23,9 +23,10 @@ func Test_TestTcpTarget(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, proj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) - require.NoError(err) + ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) + require.NoError(err) cats := static.TestCatalogs(t, conn, proj.PublicId, 1) hsets := static.TestSets(t, conn, cats[0].GetPublicId(), 2) @@ -56,9 +57,10 @@ func Test_TestCredentialLibrary(t *testing.T) { testKms := kms.TestKms(t, conn, wrapper) iamRepo := iam.TestRepo(t, conn, wrapper) _, proj := iam.TestScopes(t, iamRepo) - repo, err := target.NewRepository(rw, rw, testKms) - require.NoError(err) + ctx := context.Background() + repo, err := target.NewRepository(ctx, rw, rw, testKms) + require.NoError(err) tar := tcp.TestTarget(ctx, t, conn, proj.PublicId, t.Name()) store := vault.TestCredentialStores(t, conn, wrapper, proj.GetPublicId(), 1)[0]