Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(grants): GrantsForUsers tests for Group resource #5443

Open
wants to merge 4 commits into
base: llb-normalized-grants
Choose a base branch
from

Conversation

dkanney
Copy link
Collaborator

@dkanney dkanney commented Jan 21, 2025

Goal: Write direct & indirect tests for the GrantsForUser function against the Group resource

  • Direct: Directly associating a user to a role
  • Indirect: Associating a user to a role via one of the following:
    1. Group
    2. Managed Group (e.g. OIDC, LDAP)

I created one test for each case: one direct test, one group test, and one managed group test (performed via OIDC). All tests contain grants against the Group resource (and its associated actions)

@dkanney dkanney force-pushed the dkanney-test-grants-for-user-group-associations branch 3 times, most recently from 6bdc21d to 34114df Compare January 22, 2025 21:52
@dkanney dkanney marked this pull request as ready for review January 22, 2025 21:54
@dkanney dkanney requested a review from a team as a code owner January 22, 2025 21:54
@dkanney dkanney requested review from tmessi, elimt and bosorawis January 22, 2025 22:56
repo := iam.TestRepo(t, conn, wrap)
kmsCache := kms.TestKms(t, conn, wrap)

grant := "ids=*;type=*;actions=*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the grant, if we're testing against the groups resource, we should set set the grant to look like ids=*;type=group;actions=*.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we should have multiple grants and multiple roles so we can have multiple grantTuples and assert it's what we expect

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated these tests to only test against the Groups resource

internal/iam/repository_role_grant_test.go Show resolved Hide resolved
res: perms.Resource{
ScopeId: directGrantProj1a.PublicId,
Id: "cs_abcd1234",
Type: resource.Credential,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we hae dedicated tests for Groups resource

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do now 👍

Copy link
Collaborator Author

@dkanney dkanney Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have tests against the Groups resource here: Test_MarshalingAndCloning

@dkanney dkanney force-pushed the dkanney-test-grants-for-user-group-associations branch from 34114df to 79b9462 Compare January 24, 2025 18:48
@dkanney dkanney force-pushed the dkanney-test-grants-for-user-group-associations branch 3 times, most recently from e7cb846 to 0b6ee1a Compare January 28, 2025 20:22
@dkanney dkanney changed the title test(grants): tests for group associations test(grants): GrantsForUsers tests for Group resource Jan 29, 2025
@dkanney dkanney requested a review from mgaffney January 29, 2025 17:55
directGrantOrg1, directGrantProj1a, directGrantProj1b := iam.SetupDirectGrantScopes(t, conn, repo)
directGrantOrg1Role := iam.TestRole(t, conn, directGrantOrg1.PublicId)
iam.TestUserRole(t, conn, directGrantOrg1Role.PublicId, user.PublicId)
directGrantOrg1RoleGrant1 := "ids=*;type=group;actions=*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this direct association only test group resource? The title of the test just says TestGrantsForUser_DirectAssociation and the grantString is has type set to "ids=*;type=group;actions=*". When we want to test other resource, are you going to use the same test which builds a grantString for each resource or will they be separated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - only the group resource for now. This function should probably be renamed to something Group-specific if we don't plan on adding subtests for the other differently scoped resources (e.g. Auth Methods, Targets).

I figured we could create a separate function for each differently scoped resource since the scope combinations and actions will be different. For instance - Auth Methods apply to global and org scopes only, so we don't need to setup any project scopes like we do here.

RoleScopeId: directGrantOrg1.PublicId,
RoleParentScopeId: scope.Global.String(),
GrantScopeIds: globals.GrantScopeThis,
Grants: strings.Join([]string{directGrantOrg1RoleGrant1, directGrantOrg1RoleGrant2}, "^"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does ^ do or signify?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ^ is used as a delimiter to split grant strings/grant scope ids.

The grantsForUser query pulls in multiple grant strings/grant scope ids, aggregating them into a single string using ^ as a delimiter. In the code, we use strings.Split(..., "^") to split grant strings/grant scope ids into atomic pieces.

iam.WithGrantScopeIds([]string{
globals.GrantScopeChildren,
}))
iam.TestUserRole(t, conn, childGrantGlobalRole.PublicId, globals.AnyAuthenticatedUserId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not set the userId here instead of assigning the role to u_auth?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, I didn't notice I was assigning u_auth to the role here instead of the test user I created.

Pushed a change to use user.PublicId instead of u_auth

Grants: childGrantGlobalRoleGrant,
},
}
for i, tuple := range expMultiGrantTuples {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we assign any roles to a different user to user it does not get returned?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that did not make sense. I mean do we assign roles to a different user to ensure the only the grants for that 1 particular user gets returned?

In other words, how do we ensure do we have other non-applicable grants that are created but should not be returned because they are not applicable to the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good test case - I'll add another user with different grants to each test

@dkanney dkanney force-pushed the dkanney-test-grants-for-user-group-associations branch from 0b6ee1a to 884f33a Compare February 3, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants