Skip to content

[INC-300] baton-github: optimize Grants() for org Resource #97

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.

Sign up for GitHub

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

Merged
Bencheng21 merged 3 commits into from
Jul 24, 2025
Merged

[INC-300] baton-github: optimize Grants() for org Resource #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29251a5
use bag
Bencheng21 Jul 24, 2025
95cd8a9
add bag to team grants
Bencheng21 Jul 24, 2025
ad3faeb
fix test
Bencheng21 Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 56 additions & 50 deletions pkg/connector/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,72 +187,78 @@ func (o *orgResourceType) Grants(
return nil, "", nil, err
}
Copy link
Contributor Author

@Bencheng21 Bencheng21 Jul 24, 2025

Choose a reason for hiding this comment

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

I want to use bag. But looks like we need to push resourceTypes to bag. bag.push(resourceTypes)
How to handle pagination nicely? in this case.

Copy link
Contributor

@ggreer ggreer Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The resource.Id should contain the resource type, right? You should be able to push page tokens for different resources and resource types, then pop stuff off and switch based on the resource type. See https://github.com/ConductorOne/baton-microsoft-entra/blob/main/pkg/connector/enterprise_applications.go#L267 for an example.

Copy link
Contributor

@laurenleach laurenleach Jul 24, 2025

Choose a reason for hiding this comment

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

yes need to use pagination bag

Copy link
Contributor Author

@Bencheng21 Bencheng21 Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

image retested it.

it's a little confusing. I push admin and member as resourceTypeId, but it works.
Thanks!


opts := github.ListMembersOptions{
ListOptions: github.ListOptions{
Page: page,
PerPage: maxPageSize,
},
}
var (
reqAnnos annotations.Annotations
pageToken string
rv = []*v2.Grant{}
)

orgName, err := o.orgCache.GetOrgName(ctx, resource.Id)
if err != nil {
return nil, "", nil, err
}
switch rId := bag.ResourceTypeID(); rId {
case resourceTypeOrg.Id:
bag.Pop()
bag.Push(pagination.PageState{
ResourceTypeID: orgRoleAdmin,
})
bag.Push(pagination.PageState{
ResourceTypeID: orgRoleMember,
})
case orgRoleAdmin, orgRoleMember:

users, resp, err := o.client.Organizations.ListMembers(ctx, orgName, &opts)
if err != nil {
if isNotFoundError(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("org: %s not found", orgName))
orgName, err := o.orgCache.GetOrgName(ctx, resource.Id)
if err != nil {
return nil, "", nil, err
}
errMsg := "github-connectorv2: failed to list org members"
if isRatelimited(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.Unavailable, "too many requests", err)
opts := github.ListMembersOptions{
Role: rId,
ListOptions: github.ListOptions{
Page: page,
PerPage: maxPageSize,
},
}
return nil, "", nil, fmt.Errorf("%s: %w", errMsg, err)
}

nextPage, reqAnnos, err := parseResp(resp)
if err != nil {
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to parse response: %w", err)
}

pageToken, err := bag.NextToken(nextPage)
if err != nil {
return nil, "", nil, err
}

var rv []*v2.Grant
for _, user := range users {
membership, _, err := o.client.Organizations.GetOrgMembership(ctx, user.GetLogin(), orgName)
users, resp, err := o.client.Organizations.ListMembers(ctx, orgName, &opts)
if err != nil {
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to get org memberships for user: %w", err)
if isNotFoundError(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("org: %s not found", orgName))
}
errMsg := "github-connectorv2: failed to list org members"
if isRatelimited(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.Unavailable, "too many requests", err)
}
return nil, "", nil, fmt.Errorf("%s: %w", errMsg, err)
}
if membership.GetState() == "pending" {
continue

var nextPage string
nextPage, reqAnnos, err = parseResp(resp)
if err != nil {
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to parse response: %w", err)
}

ur, err := userResource(ctx, user, user.GetEmail(), nil)
err = bag.Next(nextPage)
if err != nil {
return nil, "", nil, err
}

roleName := strings.ToLower(membership.GetRole())
switch roleName {
case orgRoleAdmin:
rv = append(rv, o.orgRoleGrant(orgRoleAdmin, resource, ur.Id, user.GetID()))
rv = append(rv, o.orgRoleGrant(orgRoleMember, resource, ur.Id, user.GetID()))

case orgRoleMember:
rv = append(rv, o.orgRoleGrant(orgRoleMember, resource, ur.Id, user.GetID()))
for _, user := range users {
ur, err := userResource(ctx, user, user.GetEmail(), nil)
if err != nil {
return nil, "", nil, err
}

default:
ctxzap.Extract(ctx).Warn("Unknown GitHub Role Name",
zap.String("role_name", roleName),
zap.String("github_username", user.GetLogin()),
)
if rId == orgRoleAdmin {
rv = append(rv, o.orgRoleGrant(orgRoleMember, resource, ur.Id, user.GetID()))
}
rv = append(rv, o.orgRoleGrant(rId, resource, ur.Id, user.GetID()))
Copy link

@cursor cursor bot Jul 24, 2025

Choose a reason for hiding this comment

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

Bug: Pending Memberships Included in Grants

The optimization removed the check for pending organization memberships. Previously, users with pending invitations were explicitly skipped. The new code, using role-filtered ListMembers() API calls, no longer performs this check, leading to pending members being incorrectly included in grants.

Locations (1)

Fix in Cursor • Fix in Web

}
default:
ctxzap.Extract(ctx).Warn("Unknown GitHub Role Name",
zap.String("role_name", rId),
)
}

pageToken, err = bag.Marshal()
if err != nil {
return nil, "", nil, err
}
return rv, pageToken, reqAnnos, nil
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/connector/org_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ func TestOrganization(t *testing.T) {
require.Nil(t, err)
require.Empty(t, grantAnnotations)

grants, nextToken, grantsAnnotations, err := client.Grants(ctx, organization, &pagination.Token{})
_, nextToken, grantsAnnotations, err := client.Grants(ctx, organization, &pagination.Token{})
require.Nil(t, err)
test.AssertHasRatelimitAnnotations(t, grantsAnnotations)
require.Equal(t, "", nextToken)
require.Len(t, grants, 2)
require.Equal(t, "{\"states\":[{\"type\":\"admin\"}],\"current_state\":{\"type\":\"member\"}}", nextToken)

grant := v2.Grant{
Entitlement: &entitlement,
Expand Down
90 changes: 53 additions & 37 deletions pkg/connector/team.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,56 +174,72 @@ func (o *teamResourceType) Grants(ctx context.Context, resource *v2.Resource, pT
return nil, "", nil, err
}

opts := github.TeamListTeamMembersOptions{
ListOptions: github.ListOptions{
Page: page,
PerPage: maxPageSize,
},
}

users, resp, err := o.client.Teams.ListTeamMembersByID(ctx, org.GetID(), githubID, &opts)
if err != nil {
if isNotFoundError(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("org: %d not found", org.GetID()))
}
if isRatelimited(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.Unavailable, "too many requests", err)
var (
reqAnnos annotations.Annotations
pageToken string
rv = []*v2.Grant{}
)
switch rId := bag.ResourceTypeID(); rId {
case resourceTypeTeam.Id:
bag.Pop()
bag.Push(pagination.PageState{
ResourceTypeID: teamRoleMember,
})
bag.Push(pagination.PageState{
ResourceTypeID: teamRoleMaintainer,
})
case teamRoleMember, teamRoleMaintainer:
opts := github.TeamListTeamMembersOptions{
ListOptions: github.ListOptions{
Page: page,
PerPage: maxPageSize,
},
Role: rId,
}
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to fetch team members: %w", err)
}

nextPage, reqAnnos, err := parseResp(resp)
if err != nil {
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to parse response: %w", err)
}

pageToken, err := bag.NextToken(nextPage)
if err != nil {
return nil, "", nil, err
}

var rv []*v2.Grant
for _, user := range users {
membership, _, err := o.client.Teams.GetTeamMembershipByID(ctx, org.GetID(), githubID, user.GetLogin())
users, resp, err := o.client.Teams.ListTeamMembersByID(ctx, org.GetID(), githubID, &opts)
if err != nil {
if isNotFoundError(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("user: %s not found", user.GetLogin()))
return nil, "", nil, uhttp.WrapErrors(codes.NotFound, fmt.Sprintf("org: %d not found", org.GetID()))
}
if isRatelimited(resp) {
return nil, "", nil, uhttp.WrapErrors(codes.Unavailable, "too many requests", err)
}
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to get team membership for user: %w", err)
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to fetch team members: %w", err)
}

var nextPage string
nextPage, reqAnnos, err = parseResp(resp)
if err != nil {
return nil, "", nil, fmt.Errorf("github-connectorv2: failed to parse response: %w", err)
}

ur, err := userResource(ctx, user, user.GetEmail(), nil)
err = bag.Next(nextPage)
if err != nil {
return nil, "", nil, err
}

rv = append(rv, grant.NewGrant(resource, membership.GetRole(), ur.Id,
grant.WithAnnotation(&v2.V1Identifier{
Id: fmt.Sprintf("team-grant:%s:%d:%s", resource.Id.Resource, user.GetID(), membership.GetRole()),
}),
))
for _, user := range users {
ur, err := userResource(ctx, user, user.GetEmail(), nil)
if err != nil {
return nil, "", nil, err
}
rv = append(rv, grant.NewGrant(resource, rId, ur.Id,
grant.WithAnnotation(&v2.V1Identifier{
Id: fmt.Sprintf("team-grant:%s:%d:%s", resource.Id.Resource, user.GetID(), rId),
}),
))
}
default:
ctxzap.Extract(ctx).Warn("Unknown GitHub Role Name",
zap.String("role_name", rId),
)
}

pageToken, err = bag.Marshal()
if err != nil {
return nil, "", nil, err
}
return rv, pageToken, reqAnnos, nil
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/connector/team_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,10 @@ func TestTeam(t *testing.T) {
require.Nil(t, err)
require.Empty(t, grantAnnotations)

grants, nextToken, grantsAnnotations, err := client.Grants(ctx, team, &pagination.Token{})
_, nextToken, grantsAnnotations, err := client.Grants(ctx, team, &pagination.Token{})
require.Nil(t, err)
test.AssertHasRatelimitAnnotations(t, grantsAnnotations)
require.Equal(t, "", nextToken)
require.Len(t, grants, 1)
require.Equal(t, "{\"states\":[{\"type\":\"member\"}],\"current_state\":{\"type\":\"maintainer\"}}", nextToken)

grant := v2.Grant{
Entitlement: &entitlement,
Expand Down
Loading