Skip to content

Commit

Permalink
Bug: fix groups in children scopes being filtered out by grants (#5418)
Browse files Browse the repository at this point in the history
* add utility functions for grants tests

* use passed in scopeID to create user

* add test for groups list

* groups: set ParentScopeId before FetchActionSetForId

* add comment to TestRoleGrantsForToken

* lint and ran make gen

* fix import groups

* add an additional test case

* remove print

* changelog

* fix(alias): set parent scope id for alias resource (#5434)

set the `ParentScopeId` before fetching authorized actions for alias

* fix(worker): set parent scope id for worker resource (#5435)

set the `ParentScopeId` before fetching authorized actions for worker

* fix(user): children scopes being filtered out by grants for user (#5436)

* fix(user): children scopes being filtered out by grants for user

Resources are being filtered out due to missing ParentScopeId when constructing Resource to pass into authResults.FetchActionSetForId.

* fix(scope): set parent scope id for worker resource (#5439)

set the `ParentScopeId` before fetching authorized actions for worker

* fix(target): set parent scope id for target resource (#5447)

set the `ParentScopeId` before fetching authorized actions for target

* fix(roles): set parent scope id for roles resource (#5452)

* fix(roles): set parent scope id for roles resource

set the `ParentScopeId` before fetching authorized actions for role resource

* test(managed-group): add grants test coverage (#5453)

* fix(managed-group): set parent scope id for managed-group resource

set the ParentScopeId before fetching authorized actions for managed resource

* fix(host): set parent scope id for host resource (#5455)

set the `ParentScopeId` before fetching authorized actions for host resource

* fix(host-set): set parent scope id for host-set resource (#5456)

set the ParentScopeId before fetching authorized actions for host-set resource

* fix(host-catalog): set parent scope id for host-catalog resource (#5457)

set the ParentScopeId before fetching authorized actions for host-catalog resource

* fix(credential-store): set parent scope id for credential-store resource (#5458)

set the ParentScopeId before fetching authorized actions for credential-store resource

* fix(authmethods): set parent scope ID for auth methods resource (#5448)

* handlers/authmethods: fix children permission

* documentation

* formatting

* fix(credential): set parent scope id for credential resource (#5459)

set the `ParentScopeId` before fetching authorized actions for credential resource

* fix(accounts): bug grants filter children accounts (#5431)

* handlers/accounts: fix children grants filtering out results unexpectedly

* make gen

* fix(authtokens): set parent scope ID for auth token resource (#5451)

* fix authtokens not passing children scope ID

* make gen

* fix(credential-libraries): set parent scope ID (#5463)

* fix(common) set parent ID before fetching action setsparent ID before fetching action sets (#5467)

* fix(common) set parent ID before fetching action sets

* make gen

* additional test

* rename test

* more tests

* remove duplicate test

* make gen

* Update CHANGELOG.md

Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

---------

Co-authored-by: Elim Tsiagbey <elim.tsiagbey@hashicorp.com>
Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
  • Loading branch information
3 people authored Jan 24, 2025
1 parent 80c9f07 commit 7c4451d
Show file tree
Hide file tree
Showing 39 changed files with 2,676 additions and 30 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ maintainability of worker queries, and improve DB performance. ([PR](https://git
([PR](https://github.com/hashicorp/boundary/pull/5221)).
* Fix an issue where, when starting a session, the connection limit always displays 0.
([PR](https://github.com/hashicorp/boundary/pull/5396)).
* Fix bug which caused the `children` keyword not to apply the appropriate
permissions for a number of resources.
([PR](https://github.com/hashicorp/boundary/pull/5418)).

## 0.18.2 (2024/12/12)
### Bug fixes
Expand Down
36 changes: 36 additions & 0 deletions internal/authtoken/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/hashicorp/boundary/internal/db"
"github.com/hashicorp/boundary/internal/iam"
"github.com/hashicorp/boundary/internal/kms"
"github.com/hashicorp/go-uuid"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -46,3 +47,38 @@ func TestAuthToken(t testing.TB, conn *db.DB, kms *kms.Kms, scopeId string, opt
require.NoError(t, err)
return at
}

// TestRoleGrantsForToken contains information used by TestAuthTokenWithRoles to create
// roles and their associated grants (with grant scopes)
type TestRoleGrantsForToken struct {
RoleScopeID string
GrantStrings []string
GrantScopes []string
}

// TestAuthTokenWithRoles creates auth token associated with roles as requested by the caller along
// with any required resources to achieve said token
func TestAuthTokenWithRoles(t testing.TB, conn *db.DB, kms *kms.Kms, scopeId string, roles []TestRoleGrantsForToken) *AuthToken {
t.Helper()
ctx := context.Background()
rw := db.New(conn)
atRepo, err := NewRepository(ctx, rw, rw, kms)
require.NoError(t, err)

iamRepo, err := iam.NewRepository(ctx, rw, rw, kms)
require.NoError(t, err)

authMethod := password.TestAuthMethods(t, conn, scopeId, 1)[0]

loginName, err := uuid.GenerateUUID()
require.NoError(t, err)
acct := password.TestAccount(t, conn, authMethod.GetPublicId(), loginName)
user := iam.TestUser(t, iamRepo, scopeId, iam.WithAccountIds(acct.GetPublicId()))
for _, r := range roles {
role := iam.TestRoleWithGrants(t, conn, r.RoleScopeID, r.GrantScopes, r.GrantStrings)
_ = iam.TestUserRole(t, conn, role.PublicId, user.PublicId)
}
fullGrantToken, err := atRepo.CreateAuthToken(ctx, user, acct.GetPublicId())
require.NoError(t, err)
return fullGrantToken
}
35 changes: 35 additions & 0 deletions internal/daemon/controller/auth/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,19 @@ package auth

import (
"context"
"testing"

"github.com/hashicorp/boundary/globals"
"github.com/hashicorp/boundary/internal/authtoken"
"github.com/hashicorp/boundary/internal/daemon/controller/common"
"github.com/hashicorp/boundary/internal/db"
authpb "github.com/hashicorp/boundary/internal/gen/controller/auth"
"github.com/hashicorp/boundary/internal/iam"
"github.com/hashicorp/boundary/internal/kms"
"github.com/hashicorp/boundary/internal/requests"
"github.com/hashicorp/boundary/internal/server"
wrapping "github.com/hashicorp/go-kms-wrapping/v2"
"github.com/stretchr/testify/require"
)

// DisabledAuthTestContext is meant for testing, and uses a context that has
Expand All @@ -30,3 +38,30 @@ func DisabledAuthTestContext(iamRepoFn common.IamRepoFactory, scopeId string, op
requestContext := context.WithValue(context.Background(), requests.ContextRequestInformationKey, &requests.RequestContext{})
return NewVerifierContext(requestContext, iamRepoFn, nil, nil, opts.withKms, &reqInfo)
}

// TestAuthContextFromToken creates an auth context with provided token
// This is used in conjunction with TestAuthTokenWithRoles which creates a test token
func TestAuthContextFromToken(t *testing.T, conn *db.DB, wrap wrapping.Wrapper, token *authtoken.AuthToken, iamRepo *iam.Repository) context.Context {
t.Helper()
ctx := context.Background()
rw := db.New(conn)
kmsCache := kms.TestKms(t, conn, wrap)
atRepo, err := authtoken.NewRepository(ctx, rw, rw, kmsCache)
require.NoError(t, err)
serversRepoFn := func() (*server.Repository, error) {
return server.NewRepository(ctx, rw, rw, kmsCache)
}
iamRepoFn := func() (*iam.Repository, error) {
return iamRepo, nil
}
atRepoFn := func() (*authtoken.Repository, error) {
return atRepo, nil
}
fullGrantAuthCtx := NewVerifierContext(requests.NewRequestContext(ctx, requests.WithUserId(token.GetIamUserId())),
iamRepoFn, atRepoFn, serversRepoFn, kmsCache, &authpb.RequestInfo{
PublicId: token.PublicId,
Token: token.GetToken(),
TokenFormat: uint32(AuthTokenTypeBearer),
})
return fullGrantAuthCtx
}
1 change: 1 addition & 0 deletions internal/daemon/controller/common/scopeids/scope_ids.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ func GetListingResourceInformation(
for _, scp := range scps {
scpId := scp.GetPublicId()
res.ScopeId = scpId
res.ParentScopeId = scp.GetParentId()
aSet := input.AuthResults.FetchActionSetForType(ctx,
// This is overridden by WithResource
resource.Unknown,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1488,9 +1488,10 @@ func validateSetPasswordRequest(ctx context.Context, req *pbs.SetPasswordRequest

func newOutputOpts(ctx context.Context, item auth.Account, authMethodId string, authResults requestauth.VerifyResults) ([]handlers.Option, bool) {
res := perms.Resource{
ScopeId: authResults.Scope.Id,
Type: resource.Account,
Pin: authMethodId,
ScopeId: authResults.Scope.Id,
ParentScopeId: authResults.Scope.ParentScopeId,
Type: resource.Account,
Pin: authMethodId,
}
res.Id = item.GetPublicId()
authorizedActions := authResults.FetchActionSetForId(ctx, item.GetPublicId(), IdActions[globals.ResourceInfoFromPrefix(item.GetPublicId()).Subtype], requestauth.WithResource(&res)).Strings()
Expand Down
111 changes: 111 additions & 0 deletions internal/daemon/controller/handlers/accounts/grants_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package accounts_test

import (
"context"
"testing"

"github.com/hashicorp/boundary/globals"
"github.com/hashicorp/boundary/internal/auth/ldap"
"github.com/hashicorp/boundary/internal/auth/oidc"
"github.com/hashicorp/boundary/internal/auth/password"
"github.com/hashicorp/boundary/internal/authtoken"
"github.com/hashicorp/boundary/internal/daemon/controller/auth"
"github.com/hashicorp/boundary/internal/daemon/controller/handlers/accounts"
"github.com/hashicorp/boundary/internal/db"
pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services"
"github.com/hashicorp/boundary/internal/iam"
"github.com/hashicorp/boundary/internal/kms"
"github.com/stretchr/testify/require"
)

func TestListPassword_Grants(t *testing.T) {
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
rw := db.New(conn)
wrap := db.TestWrapper(t)
kms := kms.TestKms(t, conn, wrap)
iamRepo, err := iam.NewRepository(ctx, rw, rw, kms)
require.NoError(t, err)
pwRepoFn := func() (*password.Repository, error) {
return password.NewRepository(ctx, rw, rw, kms)
}
oidcRepoFn := func() (*oidc.Repository, error) {
return oidc.NewRepository(ctx, rw, rw, kms)
}

ldapRepoFn := func() (*ldap.Repository, error) {
return ldap.NewRepository(ctx, rw, rw, kms)
}

org, proj := iam.TestScopes(t, iam.TestRepo(t, conn, wrap))
orgAM := password.TestAuthMethod(t, conn, org.GetPublicId())
projAM := password.TestAuthMethod(t, conn, proj.GetPublicId())
orgPWAccounts := password.TestMultipleAccounts(t, conn, orgAM.GetPublicId(), 3)
projPWAccounts := password.TestMultipleAccounts(t, conn, projAM.GetPublicId(), 3)

testcases := []struct {
name string
roleRequest []authtoken.TestRoleGrantsForToken
input *pbs.ListAccountsRequest
wantAccountIDs []string
wantErr error
}{
{
name: "children grants global scope list org accounts return org accounts",
input: &pbs.ListAccountsRequest{
AuthMethodId: orgAM.PublicId,
PageSize: 100,
},
roleRequest: []authtoken.TestRoleGrantsForToken{
{
RoleScopeID: globals.GlobalPrefix,
GrantStrings: []string{"ids=*;type=*;actions=list,read"},
GrantScopes: []string{globals.GrantScopeChildren},
},
},
wantAccountIDs: []string{orgPWAccounts[0].PublicId, orgPWAccounts[1].PublicId, orgPWAccounts[2].PublicId},
wantErr: nil,
},
{
name: "children grants org scope list project accounts return project accounts",
input: &pbs.ListAccountsRequest{
AuthMethodId: projAM.PublicId,
PageSize: 100,
},
roleRequest: []authtoken.TestRoleGrantsForToken{
{
RoleScopeID: org.GetPublicId(),
GrantStrings: []string{"ids=*;type=*;actions=list,read"},
GrantScopes: []string{globals.GrantScopeChildren},
},
},
wantAccountIDs: []string{projPWAccounts[0].PublicId, projPWAccounts[1].PublicId, projPWAccounts[2].PublicId},
wantErr: nil,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
s, err := accounts.NewService(ctx, pwRepoFn, oidcRepoFn, ldapRepoFn, 1000)
require.NoError(t, err, "Couldn't create new user service.")
tok := authtoken.TestAuthTokenWithRoles(t, conn, kms, globals.GlobalPrefix, tc.roleRequest)
fullGrantAuthCtx := auth.TestAuthContextFromToken(t, conn, wrap, tok, iamRepo)
got, gErr := s.ListAccounts(fullGrantAuthCtx, tc.input)
if tc.wantErr != nil {
require.Error(t, err)
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, gErr)
require.Len(t, got.Items, len(orgPWAccounts))
var gotIDs []string
for _, item := range got.Items {
gotIDs = append(gotIDs, item.Id)
}
require.ElementsMatch(t, tc.wantAccountIDs, gotIDs)
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,7 @@ func newOutputOpts(ctx context.Context, item *target.Alias, scopeInfoMap map[str
}
res.Id = item.GetPublicId()
res.ScopeId = item.GetScopeId()
res.ParentScopeId = scopeInfoMap[item.GetScopeId()].GetParentScopeId()
authorizedActions := authResults.FetchActionSetForId(ctx, item.GetPublicId(), IdActions, auth.WithResource(&res))
if len(authorizedActions) == 0 {
return nil, false
Expand Down
107 changes: 107 additions & 0 deletions internal/daemon/controller/handlers/aliases/grants_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package aliases_test

import (
"context"
"testing"

"github.com/hashicorp/boundary/globals"
"github.com/hashicorp/boundary/internal/alias/target"
"github.com/hashicorp/boundary/internal/authtoken"
"github.com/hashicorp/boundary/internal/daemon/controller/auth"
"github.com/hashicorp/boundary/internal/daemon/controller/handlers"
"github.com/hashicorp/boundary/internal/daemon/controller/handlers/aliases"
"github.com/hashicorp/boundary/internal/db"
pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services"
"github.com/hashicorp/boundary/internal/iam"
"github.com/hashicorp/boundary/internal/kms"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
)

// TestGrants_ReadActions tests read actions to assert that grants are being applied properly
//
// Role - which scope the role is created in
// - global level
// Scopes [resource]:
// - globalAlias1 [globalAlias]
// - globalAlias2 [globalAlias]
func TestGrants_ReadActions(t *testing.T) {
ctx := context.Background()
conn, _ := db.TestSetup(t, "postgres")
rw := db.New(conn)
wrap := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrap)
iamRepo := iam.TestRepo(t, conn, wrap)
iamRepoFn := func() (*iam.Repository, error) {
return iamRepo, nil
}
repoFn := func() (*target.Repository, error) {
return target.NewRepository(ctx, rw, rw, kmsCache)
}
s, err := aliases.NewService(ctx, repoFn, iamRepoFn, 1000)
require.NoError(t, err)
globalAlias1 := target.TestAlias(t, rw, "test.alias.one", target.WithDescription("alias_1"), target.WithName("alias_one"))
globalAlias2 := target.TestAlias(t, rw, "test.alias.two", target.WithDescription("alias_2"), target.WithName("alias_two"))
t.Run("List", func(t *testing.T) {
testcases := []struct {
name string
input *pbs.ListAliasesRequest
rolesToCreate []authtoken.TestRoleGrantsForToken
wantErr error
wantIDs []string
}{
{
name: "global role grant this returns all created aliases",
input: &pbs.ListAliasesRequest{
ScopeId: globals.GlobalPrefix,
Recursive: true,
},
rolesToCreate: []authtoken.TestRoleGrantsForToken{
{
RoleScopeID: globals.GlobalPrefix,
GrantStrings: []string{"ids=*;type=alias;actions=list,read"},
GrantScopes: []string{globals.GrantScopeThis},
},
},
wantErr: nil,
wantIDs: []string{globalAlias1.PublicId, globalAlias2.PublicId},
},
{
name: "global role grant this with a non-applicable type throws an error",
input: &pbs.ListAliasesRequest{
ScopeId: globals.GlobalPrefix,
Recursive: true,
},
rolesToCreate: []authtoken.TestRoleGrantsForToken{
{
RoleScopeID: globals.GlobalPrefix,
GrantStrings: []string{"ids=*;type=group;actions=list,read"},
GrantScopes: []string{globals.GrantScopeThis},
},
},
wantErr: handlers.ApiErrorWithCode(codes.PermissionDenied),
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
tok := authtoken.TestAuthTokenWithRoles(t, conn, kmsCache, globals.GlobalPrefix, tc.rolesToCreate)
fullGrantAuthCtx := auth.TestAuthContextFromToken(t, conn, wrap, tok, iamRepo)
got, finalErr := s.ListAliases(fullGrantAuthCtx, tc.input)
if tc.wantErr != nil {
require.ErrorIs(t, finalErr, tc.wantErr)
return
}
require.NoError(t, finalErr)
var gotIDs []string
for _, g := range got.Items {
gotIDs = append(gotIDs, g.GetId())
}
require.ElementsMatch(t, tc.wantIDs, gotIDs)
})
}
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -1577,6 +1577,7 @@ func newOutputOpts(ctx context.Context, item auth.AuthMethod, scopeInfoMap map[s
}
res.Id = item.GetPublicId()
res.ScopeId = item.GetScopeId()
res.ParentScopeId = scopeInfoMap[item.GetScopeId()].GetParentScopeId()
authorizedActions := authResults.FetchActionSetForId(ctx, item.GetPublicId(), IdActions[globals.ResourceInfoFromPrefix(item.GetPublicId()).Subtype], requestauth.WithResource(&res)).Strings()
if len(authorizedActions) == 0 {
return nil, false, nil
Expand Down
Loading

0 comments on commit 7c4451d

Please sign in to comment.