test(groups): add grants tests for groups API #5403

bosorawis · 2025-01-03T17:47:21Z

Add tests that validate covering grants in preparation for grants system rework

johanbrandhorst

Very cool, thanks for this!

internal/daemon/controller/handlers/groups/grants_test.go

johanbrandhorst · 2025-01-04T00:18:34Z

Looks like you need a "go mod tidy"

elimt · 2025-01-06T15:49:28Z

internal/daemon/controller/handlers/groups/grants_test.go

+// genAuthTokenCtx creates an auth.VerifierContext which contains a valid auth token
+// for a user which is associated with roles in the roles parameter
+// this function creates an authMethod, account, user at global scope
+func genAuthTokenCtx(t *testing.T,


We should add a prefix of Test to the function since it's only used for testing. So nobody accidentally calls it. I think we have a pattern of this in Boundary

Since the function takes *testing.T as its first parameter so I don't think this can be accidentally called outside of test but I'll add test... prefix to its name 👍

I would also note that this function is defined in an _test.go file, which means it does not get compiled into the final binary. As such it can only be called via the tests. The pattern of using a Test prefix is usually for things defined in testing.go.

elimt · 2025-01-06T16:09:56Z

internal/daemon/controller/handlers/groups/grants_test.go

+				wantIDs: []string{globalGroup.PublicId, org1Group.PublicId, org2Group.PublicId, proj1Group.PublicId, proj2Group.PublicId, proj3Group.PublicId},
+			},
+			{
+				name: "org role grant children IDs only org children",


So descendants will also include items in the current scope which is org2Group. I expected only proj2Group and proj3Group to be returned just like GetGroup works with descendants

Oops, this is a bad test name. The role is in global with descendant access - calling list at org2 will return org2, proj2, proj3. AFAIK, there's no way to make a list exclude the "listing scope". You always get all the groups in ScopeId of the list call.

elimt · 2025-01-06T16:15:25Z

internal/daemon/controller/handlers/groups/grants_test.go

+	proj2Group := iam.TestGroup(t, conn, proj2.GetPublicId(), iam.WithDescription("proj2"), iam.WithName("proj2"))
+	proj3Group := iam.TestGroup(t, conn, proj3.GetPublicId(), iam.WithDescription("proj3"), iam.WithName("proj3"))
+
+	t.Run("List", func(t *testing.T) {


We could add test coverage for:

List with grant string with specific scope id

Test permission error with type that is not a group

added tests to cover both cases

elimt · 2025-01-06T16:26:39Z

internal/daemon/controller/handlers/groups/grants_test.go

+				},
+			},
+			{
+				name: "global_role_grant_all_specific_permissions",


This test and the previous are the same or? Unless I am missing the difference

another test I forgot to rename 🤦

elimt · 2025-01-06T16:29:22Z

internal/daemon/controller/handlers/groups/grants_test.go

+				},
+			},
+			{
+				name: "global_role_grant_all_specific_permissions",


The test name here is also the same as the last 2 tests. Not distinct enough

good catch! my bad, will push a fix soon

elimt · 2025-01-06T17:01:24Z

internal/daemon/controller/handlers/groups/grants_test.go

+	t.Run("update", func(t *testing.T) {
+		testcases := []struct {
+			name                      string
+			setupScopesResourcesRoles func(t *testing.T, conn *db.DB, iamRepo *iam.Repository) (*iam.Group, []roleRequest)


Is there a reason why we can't pass only roleRequest and allow the for loop to create a test group to follow the pattern of other tests? is it because we want to create groups in different scopes?

because in some cases we need ID of the group for testing resource-specific grant string

internal/daemon/controller/handlers/groups/grants_test.go

elimt · 2025-02-01T02:48:09Z

internal/daemon/controller/handlers/groups/grants_test.go

+		}
+
+		for _, tc := range testcases {
+			t.Run(tc.name, func(t *testing.T) {


Can we use the Get call to test the behaviour of output_fields? We could also create a separate test that does this

IMO output_fields should be tested across all CRUDL methods, not just the the read operations. I'm still working on adding a specific output_fields tests which encompasses all the method types.

elimt · 2025-02-01T03:02:14Z

internal/daemon/controller/handlers/groups/grants_test.go

+		testcases := []struct {
+			name              string
+			userFunc          func() *iam.User
+			canCreateInScopes []string


For consistency, why don't we follow what we do for Get between tests where we attempt an action and evaluate the result for each group. Makes it easier add to it and to follow

inputWantErrMap: map[*pbs.CreateGroupRequest]error{ {Id: globalGroup.PublicId}: nil, {Id: org1Group.PublicId}: nil, {Id: proj1Group.PublicId}: nil, {Id: org2Group.PublicId}: handlers.ForbiddenError(), {Id: proj2Group.PublicId}: handlers.ForbiddenError(), }

elimt · 2025-02-01T03:17:53Z

internal/iam/testing.go

+		ctx := context.Background()
+		rw := db.New(conn)
+		repo, err := NewRepository(ctx, rw, rw, kmsCache)
+		require.NoError(t, err)
+		u, err := NewUser(ctx, scopeID)
+		require.NoError(t, err)
+		user, err := repo.CreateUser(ctx, u)
+		require.NoError(t, err)


This part of it can be done with iam.TestUser

elimt · 2025-02-01T03:40:55Z

internal/iam/testing.go

+		require.NoError(t, err)
+		user, err := repo.CreateUser(ctx, u)
+		require.NoError(t, err)
+		for _, trg := range testRoleGrants {


What do you think about splitting this up some more. Have functions that only creates the target resource(direct,group or managed_group) and another function that does the associating of the target resources to roles set in TestRoleGrantsRequest by type

IMO that makes writing the test table more complicated since they cannot be normalized across all types of grants associations.

I wanted to incorporate all grants type to under one setup interface (func() *iam.User) so the table-driven tests can use the same table so tests don't need to be repeated. I'm happy to make them more distinct though but I'm afraid people might ended up ignoring the more complicated test setup (like LDAP/OIDC)

bosorawis · 2025-02-12T03:02:47Z

internal/iam/testing.go

+// TestRoleWithGrants creates a role suitable for testing along with grants
+// Functional options for GrantScopes aren't used to express that
+// this function does not provide any default grant scope unlike TestRole
+func TestRoleWithGrants(t testing.TB, conn *db.DB, scopeId string, grantScopeIDs []string, grants []string) *Role {


this will be moved to an unexported function if the setup pattern is accepted

bosorawis requested a review from a team as a code owner January 3, 2025 17:47

bosorawis added the api/groups label Jan 3, 2025

github-actions bot added core/iam core core/daemon labels Jan 3, 2025

bosorawis force-pushed the bosorawis-prototype-grant-test branch from ffbb1c6 to e18100e Compare January 3, 2025 17:48

bosorawis marked this pull request as draft January 3, 2025 17:49

johanbrandhorst approved these changes Jan 3, 2025

View reviewed changes

internal/daemon/controller/handlers/groups/grants_test.go Outdated Show resolved Hide resolved

bosorawis changed the base branch from main to llb-normalized-grants January 3, 2025 19:19

bosorawis force-pushed the bosorawis-prototype-grant-test branch from fa1bac8 to 8a31f36 Compare January 3, 2025 19:19

elimt reviewed Jan 6, 2025

View reviewed changes

bosorawis marked this pull request as ready for review January 6, 2025 23:34

johanbrandhorst approved these changes Jan 6, 2025

View reviewed changes

bosorawis force-pushed the bosorawis-prototype-grant-test branch from e44604f to 8e8eceb Compare January 7, 2025 01:21

dkanney reviewed Jan 7, 2025

View reviewed changes

internal/daemon/controller/handlers/groups/grants_test.go Outdated Show resolved Hide resolved

vercel bot had a problem deploying to Preview January 8, 2025 23:47 Failure

bosorawis requested a review from elimt January 8, 2025 23:47

elimt approved these changes Jan 10, 2025

View reviewed changes

bosorawis changed the title ~~add grants tests for groups API~~ test(groups): add grants tests for groups API Jan 30, 2025

elimt reviewed Feb 1, 2025

View reviewed changes

bosorawis added 8 commits February 11, 2025 18:35

first test with all the required setup

9deb35a

v1 of test

b62c58c

add primitive func and more test

2847974

refactor read tests into a single top level

1fb540f

move token generation to a function

62a3112

add test for creates

bda6a33

add delete tests

1bc4bb9

add update test

99552af

bosorawis added 24 commits February 11, 2025 18:36

only check for version and update_time

5137e63

move setup resource into testcase to support grants with specific ID

df1eea1

add member tests

a11f366

add group-member test example with multiple actions

f38198e

remove duplicate group membership tests

8f24be7

ran make gen

515013f

fix missing parentID bug

567190e

fix typo

def3900

fix test names and add test cases

a32b74d

switch from google/uuid to hashicorp/go-uuid

401a9eb

add comment to groupmember tests

4dc5344

small comment change

8b5e8d3

pull shared test utility code from PR #5418

a457a9c

refactor role grants out of authtoken package

05f9a59

unexport utility function

fb7bda7

Remove dead code

224f2be

lint and make gen

5eddf3f

fix role cration logic

4670161

fix password TestAccountFunc implementation

11b9664

implement TestAccountFunc for LDAP

6428eb9

implement TestAccountFunc for OIDC

e992244

implement TestUserFunc for managed groups

9acb2f4

use managed groups in grants test

7291ccb

undo removal of authtoken.TestAuthTokenWithRoles for future refactor

4c13c6a

bosorawis force-pushed the bosorawis-prototype-grant-test branch from 44f89b4 to 4c13c6a Compare February 12, 2025 02:47

github-actions bot added the core/auth label Feb 12, 2025

bosorawis added 3 commits February 11, 2025 18:58

switch from list to map based test case for create tests

9592fdb

undo merge mistakes

743bb0b

another merge mistake

52b0dec

bosorawis commented Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(groups): add grants tests for groups API #5403

test(groups): add grants tests for groups API #5403

bosorawis commented Jan 3, 2025

johanbrandhorst left a comment

johanbrandhorst commented Jan 4, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

tmessi Jan 10, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

elimt Jan 6, 2025

bosorawis Jan 6, 2025

elimt Feb 1, 2025

bosorawis Feb 12, 2025

elimt Feb 1, 2025

elimt Feb 1, 2025

elimt Feb 1, 2025 •

edited

Loading

bosorawis Feb 12, 2025

bosorawis Feb 12, 2025

test(groups): add grants tests for groups API #5403

Are you sure you want to change the base?

test(groups): add grants tests for groups API #5403

Conversation

bosorawis commented Jan 3, 2025

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elimt Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elimt Feb 1, 2025 •

edited

Loading