From 0c95056aea30db779b15988a300866bda1582ba9 Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 01:50:56 -0400 Subject: [PATCH 01/13] initial support for org roles. --- pkg/connector/connector.go | 7 + pkg/connector/org.go | 1 + pkg/connector/org_role.go | 306 +++++++++++++++++++++++++++++++++++++ 3 files changed, 314 insertions(+) create mode 100644 pkg/connector/org_role.go diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index ef3e3b81..416bc02b 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -49,6 +49,12 @@ var ( }, Annotations: v1AnnotationsForResourceType("user"), } + resourceTypeOrgRole = &v2.ResourceType{ + Id: "org_role", + DisplayName: "Organization Role", + Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_ROLE}, + Annotations: v1AnnotationsForResourceType("org_role"), + } ) type GitHub struct { @@ -66,6 +72,7 @@ func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.Resour teamBuilder(gh.client, gh.orgCache), userBuilder(gh.client, gh.hasSAMLEnabled, gh.graphqlClient, gh.orgCache), repositoryBuilder(gh.client, gh.orgCache), + orgRoleBuilder(gh.client, gh.orgCache), } } diff --git a/pkg/connector/org.go b/pkg/connector/org.go index 53b4eb56..d463555a 100644 --- a/pkg/connector/org.go +++ b/pkg/connector/org.go @@ -52,6 +52,7 @@ func organizationResource( &v2.ChildResourceType{ResourceTypeId: resourceTypeUser.Id}, &v2.ChildResourceType{ResourceTypeId: resourceTypeTeam.Id}, &v2.ChildResourceType{ResourceTypeId: resourceTypeRepository.Id}, + &v2.ChildResourceType{ResourceTypeId: resourceTypeOrgRole.Id}, ), ) } diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go new file mode 100644 index 00000000..872f5c52 --- /dev/null +++ b/pkg/connector/org_role.go @@ -0,0 +1,306 @@ +package connector + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "strconv" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/pagination" + "github.com/conductorone/baton-sdk/pkg/types/entitlement" + "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/google/go-github/v63/github" +) + +type OrganizationRole struct { + ID int64 `json:"id"` + Name string `json:"name"` + Description string `json:"description"` + Permissions []string `json:"permissions"` +} + +type OrganizationRoleResponse struct { + TotalCount int `json:"total_count"` + Roles []OrganizationRole `json:"roles"` +} + +type OrganizationRoleTeam struct { + ID int64 `json:"id"` + Name string `json:"name"` +} + +type orgRoleResourceType struct { + resourceType *v2.ResourceType + client *github.Client + orgCache *orgNameCache +} + +func orgRoleResource( + ctx context.Context, + role *OrganizationRole, + org *v2.Resource, +) (*v2.Resource, error) { + profile := map[string]interface{}{ + "description": role.Description, + } + + return resource.NewRoleResource( + role.Name, + resourceTypeOrgRole, + role.ID, + []resource.RoleTraitOption{ + resource.WithRoleProfile(profile), + }, + resource.WithParentResourceID(org.Id), + resource.WithAnnotation( + &v2.V1Identifier{Id: fmt.Sprintf("org_role:%d", role.ID)}, + ), + ) +} + +func (o *orgRoleResourceType) ResourceType(_ context.Context) *v2.ResourceType { + return o.resourceType +} + +func (o *orgRoleResourceType) List( + ctx context.Context, + parentID *v2.ResourceId, + pToken *pagination.Token, +) ([]*v2.Resource, string, annotations.Annotations, error) { + if parentID == nil { + return nil, "", nil, nil + } + + bag, _, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) + if err != nil { + return nil, "", nil, err + } + + orgName, err := o.orgCache.GetOrgName(ctx, parentID) + if err != nil { + return nil, "", nil, err + } + + // Use REST API directly since the client doesn't support these endpoints yet + url := fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles", orgName) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to create request: %w", err) + } + + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err := o.client.Client().Do(req) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to list organization roles: %w", err) + } + defer resp.Body.Close() + + // Handle permission errors gracefully + if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { + // Return empty list with no error to indicate we skipped this resource + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return nil, pageToken, nil, nil + } + + if resp.StatusCode != http.StatusOK { + return nil, "", nil, fmt.Errorf("failed to list organization roles: %s", resp.Status) + } + + var roleResp OrganizationRoleResponse + if err := json.NewDecoder(resp.Body).Decode(&roleResp); err != nil { + return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) + } + + var ret []*v2.Resource + for _, role := range roleResp.Roles { + roleResource, err := orgRoleResource(ctx, &role, &v2.Resource{Id: parentID}) + if err != nil { + return nil, "", nil, err + } + ret = append(ret, roleResource) + } + + // Since the API doesn't support pagination for roles yet, we'll return an empty token + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + + return ret, pageToken, nil, nil +} + +func (o *orgRoleResourceType) Entitlements( + _ context.Context, + resource *v2.Resource, + _ *pagination.Token, +) ([]*v2.Entitlement, string, annotations.Annotations, error) { + rv := make([]*v2.Entitlement, 0, 1) + rv = append(rv, entitlement.NewAssignmentEntitlement(resource, "assigned", + entitlement.WithDisplayName(resource.DisplayName), + entitlement.WithDescription(fmt.Sprintf("Assignment to %s role in GitHub", resource.DisplayName)), + entitlement.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("org_role:%s", resource.Id.Resource), + }), + entitlement.WithGrantableTo(resourceTypeUser), + )) + + return rv, "", nil, nil +} + +func (o *orgRoleResourceType) Grants( + ctx context.Context, + resource *v2.Resource, + pToken *pagination.Token, +) ([]*v2.Grant, string, annotations.Annotations, error) { + if resource == nil { + return nil, "", nil, nil + } + + bag, _, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) + if err != nil { + return nil, "", nil, err + } + + orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) + if err != nil { + return nil, "", nil, err + } + + roleID, err := strconv.ParseInt(resource.Id.Resource, 10, 64) + if err != nil { + return nil, "", nil, fmt.Errorf("invalid role ID: %w", err) + } + + var ret []*v2.Grant + + // First, get teams with this role + url := fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles/%d/teams", orgName, roleID) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to create request: %w", err) + } + + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err := o.client.Client().Do(req) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) + } + defer resp.Body.Close() + + // Handle permission errors gracefully + if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { + // Return empty list with no error to indicate we skipped this resource + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return nil, pageToken, nil, nil + } + + if resp.StatusCode != http.StatusOK { + return nil, "", nil, fmt.Errorf("failed to list role teams: %s", resp.Status) + } + + var teams []OrganizationRoleTeam + if err := json.NewDecoder(resp.Body).Decode(&teams); err != nil { + return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) + } + + // Create expandable grants for teams + for _, team := range teams { + teamResource, err := teamResource(&github.Team{ID: &team.ID, Name: &team.Name}, resource.ParentResourceId) + if err != nil { + return nil, "", nil, err + } + + // Create an expandable grant for the team + grant := grant.NewGrant( + resource, + "assigned", + teamResource.Id, + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{fmt.Sprintf("team:%d:member", team.ID)}, + Shallow: true, + }), + ) + ret = append(ret, grant) + } + + // Then, get direct user assignments + url = fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles/%d/users", orgName, roleID) + req, err = http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to create request: %w", err) + } + + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err = o.client.Client().Do(req) + if err != nil { + return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) + } + defer resp.Body.Close() + + // Handle permission errors gracefully + if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { + // Return what we have so far (teams) with no error + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return ret, pageToken, nil, nil + } + + if resp.StatusCode != http.StatusOK { + return nil, "", nil, fmt.Errorf("failed to list role users: %s", resp.Status) + } + + var users []*github.User + if err := json.NewDecoder(resp.Body).Decode(&users); err != nil { + return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) + } + + // Create regular grants for direct user assignments + for _, user := range users { + grant := grant.NewGrant( + resource, + "assigned", + &v2.ResourceId{ + ResourceType: resourceTypeUser.Id, + Resource: fmt.Sprintf("%d", user.GetID()), + }, + grant.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("org_role_grant:%s:%d", resource.Id.Resource, user.GetID()), + }), + ) + ret = append(ret, grant) + } + + // Since the API doesn't support pagination for role teams/users yet, we'll return an empty token + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + + return ret, pageToken, nil, nil +} + +func orgRoleBuilder(client *github.Client, orgCache *orgNameCache) *orgRoleResourceType { + return &orgRoleResourceType{ + resourceType: resourceTypeOrgRole, + client: client, + orgCache: orgCache, + } +} From 1f108abf29ed421c01df8e875106d55ca4c82977 Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 03:03:12 -0400 Subject: [PATCH 02/13] clean up and added grant/revoke --- pkg/connector/org_role.go | 310 +++++++++++++++++++++++++------------- 1 file changed, 203 insertions(+), 107 deletions(-) diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go index 872f5c52..c2daed92 100644 --- a/pkg/connector/org_role.go +++ b/pkg/connector/org_role.go @@ -2,7 +2,6 @@ package connector import ( "context" - "encoding/json" "fmt" "net/http" "strconv" @@ -14,13 +13,14 @@ import ( "github.com/conductorone/baton-sdk/pkg/types/grant" "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/google/go-github/v63/github" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" ) type OrganizationRole struct { - ID int64 `json:"id"` - Name string `json:"name"` - Description string `json:"description"` - Permissions []string `json:"permissions"` + ID int64 `json:"id"` + Name string `json:"name"` + Description string `json:"description"` } type OrganizationRoleResponse struct { @@ -85,51 +85,33 @@ func (o *orgRoleResourceType) List( return nil, "", nil, err } - // Use REST API directly since the client doesn't support these endpoints yet - url := fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles", orgName) - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) if err != nil { - return nil, "", nil, fmt.Errorf("failed to create request: %w", err) - } - - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - - resp, err := o.client.Client().Do(req) - if err != nil { - return nil, "", nil, fmt.Errorf("failed to list organization roles: %w", err) - } - defer resp.Body.Close() - - // Handle permission errors gracefully - if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { - // Return empty list with no error to indicate we skipped this resource - pageToken, err := bag.NextToken("") - if err != nil { - return nil, "", nil, err + // Handle permission errors gracefully + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + // Return empty list with no error to indicate we skipped this resource + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return nil, pageToken, nil, nil } - return nil, pageToken, nil, nil - } - - if resp.StatusCode != http.StatusOK { - return nil, "", nil, fmt.Errorf("failed to list organization roles: %s", resp.Status) - } - - var roleResp OrganizationRoleResponse - if err := json.NewDecoder(resp.Body).Decode(&roleResp); err != nil { - return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) + return nil, "", nil, fmt.Errorf("failed to list organization roles: %w", err) } var ret []*v2.Resource - for _, role := range roleResp.Roles { - roleResource, err := orgRoleResource(ctx, &role, &v2.Resource{Id: parentID}) + for _, role := range roles.CustomRepoRoles { + roleResource, err := orgRoleResource(ctx, &OrganizationRole{ + ID: role.GetID(), + Name: role.GetName(), + Description: role.GetDescription(), + }, &v2.Resource{Id: parentID}) if err != nil { return nil, "", nil, err } ret = append(ret, roleResource) } - // Since the API doesn't support pagination for roles yet, we'll return an empty token pageToken, err := bag.NextToken("") if err != nil { return nil, "", nil, err @@ -183,43 +165,23 @@ func (o *orgRoleResourceType) Grants( var ret []*v2.Grant // First, get teams with this role - url := fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles/%d/teams", orgName, roleID) - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, nil) if err != nil { - return nil, "", nil, fmt.Errorf("failed to create request: %w", err) - } - - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - - resp, err := o.client.Client().Do(req) - if err != nil { - return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) - } - defer resp.Body.Close() - - // Handle permission errors gracefully - if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { - // Return empty list with no error to indicate we skipped this resource - pageToken, err := bag.NextToken("") - if err != nil { - return nil, "", nil, err + // Handle permission errors gracefully + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + // Return empty list with no error to indicate we skipped this resource + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return nil, pageToken, nil, nil } - return nil, pageToken, nil, nil - } - - if resp.StatusCode != http.StatusOK { - return nil, "", nil, fmt.Errorf("failed to list role teams: %s", resp.Status) - } - - var teams []OrganizationRoleTeam - if err := json.NewDecoder(resp.Body).Decode(&teams); err != nil { - return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) + return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) } // Create expandable grants for teams for _, team := range teams { - teamResource, err := teamResource(&github.Team{ID: &team.ID, Name: &team.Name}, resource.ParentResourceId) + teamResource, err := teamResource(team, resource.ParentResourceId) if err != nil { return nil, "", nil, err } @@ -230,7 +192,7 @@ func (o *orgRoleResourceType) Grants( "assigned", teamResource.Id, grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{fmt.Sprintf("team:%d:member", team.ID)}, + EntitlementIds: []string{fmt.Sprintf("team:%d:member", team.GetID())}, Shallow: true, }), ) @@ -238,57 +200,35 @@ func (o *orgRoleResourceType) Grants( } // Then, get direct user assignments - url = fmt.Sprintf("https://api.github.com/orgs/%s/organization-roles/%d/users", orgName, roleID) - req, err = http.NewRequestWithContext(ctx, "GET", url, nil) - if err != nil { - return nil, "", nil, fmt.Errorf("failed to create request: %w", err) - } - - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - - resp, err = o.client.Client().Do(req) + users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, nil) if err != nil { + // Handle permission errors gracefully + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + // Return what we have so far (teams) with no error + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return ret, pageToken, nil, nil + } return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) } - defer resp.Body.Close() - // Handle permission errors gracefully - if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { - // Return what we have so far (teams) with no error - pageToken, err := bag.NextToken("") + // Create regular grants for direct user assignments + for _, user := range users { + userResource, err := userResource(ctx, user, user.GetEmail(), nil) if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil - } - - if resp.StatusCode != http.StatusOK { - return nil, "", nil, fmt.Errorf("failed to list role users: %s", resp.Status) - } - var users []*github.User - if err := json.NewDecoder(resp.Body).Decode(&users); err != nil { - return nil, "", nil, fmt.Errorf("failed to decode response: %w", err) - } - - // Create regular grants for direct user assignments - for _, user := range users { grant := grant.NewGrant( resource, "assigned", - &v2.ResourceId{ - ResourceType: resourceTypeUser.Id, - Resource: fmt.Sprintf("%d", user.GetID()), - }, - grant.WithAnnotation(&v2.V1Identifier{ - Id: fmt.Sprintf("org_role_grant:%s:%d", resource.Id.Resource, user.GetID()), - }), + userResource.Id, ) ret = append(ret, grant) } - // Since the API doesn't support pagination for role teams/users yet, we'll return an empty token pageToken, err := bag.NextToken("") if err != nil { return nil, "", nil, err @@ -297,6 +237,162 @@ func (o *orgRoleResourceType) Grants( return ret, pageToken, nil, nil } +func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { + l := ctxzap.Extract(ctx) + + if principal.Id.ResourceType != resourceTypeUser.Id { + l.Warn( + "github-connectorv2: only users can be granted organization roles", + zap.String("principal_type", principal.Id.ResourceType), + zap.String("principal_id", principal.Id.Resource), + ) + return nil, fmt.Errorf("github-connectorv2: only users can be granted organization roles") + } + + roleID, err := strconv.ParseInt(entitlement.Resource.Id.Resource, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid role ID: %w", err) + } + + orgName, err := o.orgCache.GetOrgName(ctx, entitlement.Resource.ParentResourceId) + if err != nil { + return nil, fmt.Errorf("failed to get org name: %w", err) + } + + // First verify that the role exists + roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) + if err != nil { + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + return nil, fmt.Errorf("failed to verify role: organization not found or insufficient permissions") + } + return nil, fmt.Errorf("failed to verify role: %w", err) + } + + // Check if the role exists + roleExists := false + for _, role := range roles.CustomRepoRoles { + if role.GetID() == roleID { + roleExists = true + break + } + } + + if !roleExists { + return nil, fmt.Errorf("role with ID %d not found in organization %s", roleID, orgName) + } + + userID, err := strconv.ParseInt(principal.Id.Resource, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid user ID: %w", err) + } + + user, _, err := o.client.Users.GetByID(ctx, userID) + if err != nil { + return nil, fmt.Errorf("failed to get user: %w", err) + } + + l.Info("attempting to assign role", + zap.String("org", orgName), + zap.Int64("role_id", roleID), + zap.String("user", user.GetLogin()), + ) + + // Use the client's HTTP client to make the request with the correct URL format + url := fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID) + req, err := o.client.NewRequest("PUT", url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + resp, err = o.client.Do(ctx, req, nil) + if err != nil { + if resp != nil { + l.Error("failed to assign role", + zap.String("org", orgName), + zap.Int64("role_id", roleID), + zap.String("user", user.GetLogin()), + zap.Int("status_code", resp.StatusCode), + zap.String("status", resp.Status), + zap.Error(err), + ) + } + return nil, fmt.Errorf("failed to assign role: %w", err) + } + + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNoContent { + l.Error("failed to assign role", + zap.String("org", orgName), + zap.Int64("role_id", roleID), + zap.String("user", user.GetLogin()), + zap.Int("status_code", resp.StatusCode), + zap.String("status", resp.Status), + ) + return nil, fmt.Errorf("failed to assign role: %s", resp.Status) + } + + l.Info("successfully assigned role", + zap.String("org", orgName), + zap.Int64("role_id", roleID), + zap.String("user", user.GetLogin()), + ) + + return nil, nil +} + +func (o *orgRoleResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { + l := ctxzap.Extract(ctx) + + entitlement := grant.Entitlement + principal := grant.Principal + + if principal.Id.ResourceType != resourceTypeUser.Id { + l.Warn( + "github-connectorv2: only users can have organization roles revoked", + zap.String("principal_type", principal.Id.ResourceType), + zap.String("principal_id", principal.Id.Resource), + ) + return nil, fmt.Errorf("github-connectorv2: only users can have organization roles revoked") + } + + roleID, err := strconv.ParseInt(entitlement.Resource.Id.Resource, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid role ID: %w", err) + } + + orgName, err := o.orgCache.GetOrgName(ctx, entitlement.Resource.ParentResourceId) + if err != nil { + return nil, fmt.Errorf("failed to get org name: %w", err) + } + + userID, err := strconv.ParseInt(principal.Id.Resource, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid user ID: %w", err) + } + + user, _, err := o.client.Users.GetByID(ctx, userID) + if err != nil { + return nil, fmt.Errorf("failed to get user: %w", err) + } + + // Use the client's HTTP client to make the request with the correct URL format + url := fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID) + req, err := o.client.NewRequest("DELETE", url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + + resp, err := o.client.Do(ctx, req, nil) + if err != nil { + return nil, fmt.Errorf("failed to revoke role: %w", err) + } + + if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed to revoke role: %s", resp.Status) + } + + return nil, nil +} + func orgRoleBuilder(client *github.Client, orgCache *orgNameCache) *orgRoleResourceType { return &orgRoleResourceType{ resourceType: resourceTypeOrgRole, From 86c4ec1dc769a3c21f5c18ccbcc4aaf9c6a7090f Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 03:04:01 -0400 Subject: [PATCH 03/13] emitting org role entitlements and grant counts --- pkg/connector/team.go | 110 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/pkg/connector/team.go b/pkg/connector/team.go index 268ad8b3..955e8510 100644 --- a/pkg/connector/team.go +++ b/pkg/connector/team.go @@ -3,6 +3,7 @@ package connector import ( "context" "fmt" + "net/http" "strconv" "strings" @@ -128,8 +129,10 @@ func (o *teamResourceType) List(ctx context.Context, parentID *v2.ResourceId, pt return rv, pageToken, reqAnnos, nil } -func (o *teamResourceType) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { +func (o *teamResourceType) Entitlements(ctx context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { rv := make([]*v2.Entitlement, 0, len(teamAccessLevels)) + + // Add team role entitlements for _, level := range teamAccessLevels { rv = append( rv, @@ -148,6 +151,60 @@ func (o *teamResourceType) Entitlements(_ context.Context, resource *v2.Resource ) } + // Get organization roles for this team + orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) + if err != nil { + return rv, "", nil, nil // Return what we have so far if we can't get org name + } + + roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) + if err != nil { + // Handle permission errors gracefully + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + return rv, "", nil, nil // Return what we have so far if we don't have permission + } + return rv, "", nil, nil // Return what we have so far if request failed + } + + // Get team ID for checking role assignments + teamID, err := strconv.ParseInt(resource.Id.Resource, 10, 64) + if err != nil { + return rv, "", nil, nil // Return what we have so far if we can't parse team ID + } + + // Add organization role entitlements only for roles the team is assigned to + for _, role := range roles.CustomRepoRoles { + // Check if team is assigned to this role + teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, role.GetID(), nil) + if err != nil { + // Skip this role if we can't check assignments + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + continue + } + continue + } + + // Check if this team is in the list of teams with this role + for _, team := range teams { + if team.GetID() == teamID { + rv = append( + rv, + entitlement.NewAssignmentEntitlement( + resource, + role.GetName(), + entitlement.WithDisplayName(role.GetName()), + entitlement.WithDescription(role.GetDescription()), + entitlement.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("team:%s:org_role:%d", resource.Id.Resource, role.GetID()), + }), + entitlement.WithGrantableTo(resourceTypeUser), + ), + ) + break // Found the team, no need to check other teams + } + } + } + return rv, "", nil, nil } @@ -221,6 +278,57 @@ func (o *teamResourceType) Grants(ctx context.Context, resource *v2.Resource, pT )) } + // Get organization roles for this team + orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) + if err != nil { + return rv, pageToken, reqAnnos, nil // Return what we have so far if we can't get org name + } + + roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) + if err != nil { + // Handle permission errors gracefully + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + return rv, pageToken, reqAnnos, nil // Return what we have so far if we don't have permission + } + return rv, pageToken, reqAnnos, nil // Return what we have so far if request failed + } + + // Add grants for organization roles + for _, role := range roles.CustomRepoRoles { + // Check if team is assigned to this role + teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, role.GetID(), nil) + if err != nil { + // Skip this role if we can't check assignments + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + continue + } + continue + } + + // Check if this team is in the list of teams with this role + for _, team := range teams { + if team.GetID() == githubID { + // Create a grant for each team member with this role + for _, user := range users { + ur, err := userResource(ctx, user, user.GetEmail(), nil) + if err != nil { + continue + } + + rv = append(rv, grant.NewGrant( + resource, + role.GetName(), + ur.Id, + grant.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("team-org-role-grant:%s:%d:%s", resource.Id.Resource, user.GetID(), role.GetName()), + }), + )) + } + break // Found the team, no need to check other teams + } + } + } + return rv, pageToken, reqAnnos, nil } From 20fcc4cd3562d6e8f2e58fef72bf1fde575714c7 Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 03:17:40 -0400 Subject: [PATCH 04/13] added org role tests --- pkg/connector/org_role_test.go | 94 +++++++++++++++++++++++++++++ test/mocks/endpointpattern.go | 26 ++++++++ test/mocks/github.go | 107 +++++++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+) create mode 100644 pkg/connector/org_role_test.go diff --git a/pkg/connector/org_role_test.go b/pkg/connector/org_role_test.go new file mode 100644 index 00000000..09ddfaaf --- /dev/null +++ b/pkg/connector/org_role_test.go @@ -0,0 +1,94 @@ +package connector + +import ( + "context" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/pagination" + entitlement2 "github.com/conductorone/baton-sdk/pkg/types/entitlement" + "github.com/google/go-github/v63/github" + "github.com/stretchr/testify/require" + + "github.com/conductorone/baton-github/test" + "github.com/conductorone/baton-github/test/mocks" +) + +func TestOrgRole(t *testing.T) { + ctx := context.Background() + + t.Run("should grant and revoke entitlements", func(t *testing.T) { + mgh := mocks.NewMockGitHub() + + githubOrganization, _, _, githubUser, _ := mgh.Seed() + + githubClient := github.NewClient(mgh.Server()) + cache := newOrgNameCache(githubClient) + client := orgRoleBuilder(githubClient, cache) + + organization, _ := organizationResource(ctx, githubOrganization, nil) + role, _ := orgRoleResource(ctx, &OrganizationRole{ + ID: 1, + Name: "Test Role", + Description: "Test Role Description", + }, organization) + user, _ := userResource(ctx, githubUser, *githubUser.Email, nil) + + entitlement := v2.Entitlement{ + Id: entitlement2.NewEntitlementID(role, "assigned"), + Resource: role, + } + + grantAnnotations, err := client.Grant(ctx, user, &entitlement) + require.Nil(t, err) + require.Empty(t, grantAnnotations) + + grants, nextToken, grantsAnnotations, err := client.Grants(ctx, role, &pagination.Token{}) + require.Nil(t, err) + test.AssertNoRatelimitAnnotations(t, grantsAnnotations) + require.Equal(t, "", nextToken) + require.Len(t, grants, 1) + + grant := v2.Grant{ + Entitlement: &entitlement, + Principal: user, + } + + revokeAnnotations, err := client.Revoke(ctx, &grant) + require.Nil(t, err) + require.Empty(t, revokeAnnotations) + }) + + t.Run("should handle permission errors gracefully", func(t *testing.T) { + mockGithub := mocks.NewMockGitHub() + mockGithub.SimulateOrgRolePermErr = true + + githubOrganization, _, _, _, _ := mockGithub.Seed() + + githubClient := github.NewClient(mockGithub.Server()) + cache := newOrgNameCache(githubClient) + client := orgRoleBuilder(githubClient, cache) + + organization, _ := organizationResource(ctx, githubOrganization, nil) + + // Test List with permission error + resources, nextToken, annotations, err := client.List(ctx, organization.Id, &pagination.Token{}) + require.Nil(t, err) + require.Empty(t, resources) + require.Empty(t, nextToken) + test.AssertNoRatelimitAnnotations(t, annotations) + + // Test Grants with permission error + role, _ := orgRoleResource(ctx, &OrganizationRole{ + ID: 1, + Name: "Test Role", + Description: "Test Role Description", + }, organization) + + grants, nextToken, grantsAnnotations, err := client.Grants(ctx, role, &pagination.Token{}) + require.Nil(t, err) + require.Empty(t, grants) + require.Empty(t, nextToken) + test.AssertNoRatelimitAnnotations(t, grantsAnnotations) + }) +} diff --git a/test/mocks/endpointpattern.go b/test/mocks/endpointpattern.go index a628c2af..039f278c 100644 --- a/test/mocks/endpointpattern.go +++ b/test/mocks/endpointpattern.go @@ -41,3 +41,29 @@ var GetOrganizationsTeamsMembershipsByTeamIdByUsername = mock.EndpointPattern{ Pattern: "/organizations/{org_id}/team/{team_id}/memberships/{username}", Method: "GET", } + +// Organization role endpoints +var GetOrgsRolesByOrg = mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles", + Method: "GET", +} + +var GetOrgsRolesTeamsByOrgByRoleId = mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles/{role_id}/teams", + Method: "GET", +} + +var GetOrgsRolesUsersByOrgByRoleId = mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles/{role_id}/users", + Method: "GET", +} + +var PutOrgsRolesUsersByOrgByRoleIdByUsername = mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles/users/{username}/{role_id}", + Method: "PUT", +} + +var DeleteOrgsRolesUsersByOrgByRoleIdByUsername = mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles/users/{username}/{role_id}", + Method: "DELETE", +} diff --git a/test/mocks/github.go b/test/mocks/github.go index 57db08ed..1a65ce6a 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -22,6 +22,8 @@ type MockGitHub struct { repositories map[int64]github.Repository teams map[int64]github.Team users map[int64]github.User + orgRoles map[int64]mapset.Set[int64] // Maps role ID to set of user IDs + SimulateOrgRolePermErr bool // Simulate permission error for org roles } func NewMockGitHub() *MockGitHub { @@ -33,6 +35,7 @@ func NewMockGitHub() *MockGitHub { repositories: map[int64]github.Repository{}, teams: map[int64]github.Team{}, users: map[int64]github.User{}, + orgRoles: map[int64]mapset.Set[int64]{}, } } @@ -494,6 +497,104 @@ func (mgh MockGitHub) removeRepositoryCollaborator( ) } +type OrganizationRole struct { + ID int64 `json:"id"` + Name string `json:"name"` + Description string `json:"description"` +} + +type OrganizationRoles struct { + CustomRepoRoles []*OrganizationRole `json:"roles"` +} + +func (mgh MockGitHub) getOrgRoles( + w http.ResponseWriter, + variables map[string]string, +) { + if mgh.SimulateOrgRolePermErr { + w.WriteHeader(http.StatusForbidden) + return + } + orgID, _ := getCrossTableId(w, variables, "org") + if _, ok := mgh.organizations[orgID]; !ok { + w.WriteHeader(http.StatusNotFound) + return + } + + // Return a mock role + role := &OrganizationRole{ + ID: 1, + Name: "Test Role", + Description: "Test Role Description", + } + + roles := &OrganizationRoles{ + CustomRepoRoles: []*OrganizationRole{role}, + } + + _, _ = w.Write(mock.MustMarshal(roles)) +} + +func (mgh MockGitHub) getOrgRoleTeams( + w http.ResponseWriter, + variables map[string]string, +) { + roleID, _ := getCrossTableId(w, variables, "role_id") + if _, ok := mgh.orgRoles[roleID]; !ok { + w.WriteHeader(http.StatusNotFound) + return + } + + // Return empty teams list for now + _, _ = w.Write(mock.MustMarshal([]*github.Team{})) +} + +func (mgh MockGitHub) getOrgRoleUsers( + w http.ResponseWriter, + variables map[string]string, +) { + roleID, _ := getCrossTableId(w, variables, "role_id") + memberships, ok := mgh.orgRoles[roleID] + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + + users := make([]github.User, 0) + for _, userID := range memberships.ToSlice() { + if user, ok := mgh.users[userID]; ok { + users = append(users, user) + } + } + + _, _ = w.Write(mock.MustMarshal(users)) +} + +func (mgh MockGitHub) addOrgRoleUser( + w http.ResponseWriter, + variables map[string]string, +) { + roleID, _ := getCrossTableId(w, variables, "role_id") + userID, _ := getUserId(w, variables) + + if _, ok := mgh.orgRoles[roleID]; !ok { + mgh.orgRoles[roleID] = mapset.NewSet[int64]() + } + mgh.orgRoles[roleID].Add(userID) +} + +func (mgh MockGitHub) removeOrgRoleUser( + w http.ResponseWriter, + variables map[string]string, +) { + roleID, _ := getCrossTableId(w, variables, "role_id") + userID, _ := getUserId(w, variables) + + if memberships, ok := mgh.orgRoles[roleID]; ok { + memberships.Remove(userID) + } +} + type handler = func(w http.ResponseWriter, variables map[string]string) // addEndpointHandler takes a string interpolation pattern and a handler @@ -540,6 +641,12 @@ func (mgh MockGitHub) Server() *http.Client { mock.PutReposCollaboratorsByOwnerByRepoByUsername: mgh.addRepositoryCollaborator, DeleteOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername: mgh.removeMembership, PutOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername: mgh.addMembership, + // Add organization role endpoints + GetOrgsRolesByOrg: mgh.getOrgRoles, + GetOrgsRolesTeamsByOrgByRoleId: mgh.getOrgRoleTeams, + GetOrgsRolesUsersByOrgByRoleId: mgh.getOrgRoleUsers, + PutOrgsRolesUsersByOrgByRoleIdByUsername: mgh.addOrgRoleUser, + DeleteOrgsRolesUsersByOrgByRoleIdByUsername: mgh.removeOrgRoleUser, } options := make([]mock.MockBackendOption, 0) From 6c0e2eff4eb348cb75388edb3525815895815939 Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 10:52:47 -0400 Subject: [PATCH 05/13] made some comments to review specific sections --- pkg/connector/org_role.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go index c2daed92..3eb8e21d 100644 --- a/pkg/connector/org_role.go +++ b/pkg/connector/org_role.go @@ -167,7 +167,7 @@ func (o *orgRoleResourceType) Grants( // First, get teams with this role teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, nil) if err != nil { - // Handle permission errors gracefully + // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { // Return empty list with no error to indicate we skipped this resource pageToken, err := bag.NextToken("") @@ -179,14 +179,13 @@ func (o *orgRoleResourceType) Grants( return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) } - // Create expandable grants for teams + // Create expandable grants for teams. To show inherited roles, we need to show the teams that have the role. for _, team := range teams { teamResource, err := teamResource(team, resource.ParentResourceId) if err != nil { return nil, "", nil, err } - // Create an expandable grant for the team grant := grant.NewGrant( resource, "assigned", @@ -202,9 +201,7 @@ func (o *orgRoleResourceType) Grants( // Then, get direct user assignments users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, nil) if err != nil { - // Handle permission errors gracefully if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - // Return what we have so far (teams) with no error pageToken, err := bag.NextToken("") if err != nil { return nil, "", nil, err @@ -214,7 +211,7 @@ func (o *orgRoleResourceType) Grants( return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) } - // Create regular grants for direct user assignments + // Create regular grants for direct user assignments. for _, user := range users { userResource, err := userResource(ctx, user, user.GetEmail(), nil) if err != nil { @@ -240,13 +237,14 @@ func (o *orgRoleResourceType) Grants( func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { l := ctxzap.Extract(ctx) + // Needs review, I copied this from the team grant function, but roles can be granted to teams as well, but we don't necessarily support that so wasn't sure if this was the intended behavior. if principal.Id.ResourceType != resourceTypeUser.Id { l.Warn( - "github-connectorv2: only users can be granted organization roles", + "github-connector: only users can be granted organization roles", zap.String("principal_type", principal.Id.ResourceType), zap.String("principal_id", principal.Id.Resource), ) - return nil, fmt.Errorf("github-connectorv2: only users can be granted organization roles") + return nil, fmt.Errorf("github-connector: only users can be granted organization roles") } roleID, err := strconv.ParseInt(entitlement.Resource.Id.Resource, 10, 64) @@ -297,7 +295,7 @@ func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, zap.String("user", user.GetLogin()), ) - // Use the client's HTTP client to make the request with the correct URL format + // Use the client's HTTP client to make the request with the correct URL format. Couldn't find a built in function for this. Cursor suggested this approach. url := fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID) req, err := o.client.NewRequest("PUT", url, nil) if err != nil { @@ -345,13 +343,14 @@ func (o *orgRoleResourceType) Revoke(ctx context.Context, grant *v2.Grant) (anno entitlement := grant.Entitlement principal := grant.Principal + // Needs review, I copied this from the team grant function, but roles can be granted to teams as well, but we don't necessarily support that so wasn't sure if this was the intended behavior. if principal.Id.ResourceType != resourceTypeUser.Id { l.Warn( - "github-connectorv2: only users can have organization roles revoked", + "github-connector: only users can have organization roles revoked", zap.String("principal_type", principal.Id.ResourceType), zap.String("principal_id", principal.Id.Resource), ) - return nil, fmt.Errorf("github-connectorv2: only users can have organization roles revoked") + return nil, fmt.Errorf("github-connector: only users can have organization roles revoked") } roleID, err := strconv.ParseInt(entitlement.Resource.Id.Resource, 10, 64) @@ -374,7 +373,6 @@ func (o *orgRoleResourceType) Revoke(ctx context.Context, grant *v2.Grant) (anno return nil, fmt.Errorf("failed to get user: %w", err) } - // Use the client's HTTP client to make the request with the correct URL format url := fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID) req, err := o.client.NewRequest("DELETE", url, nil) if err != nil { From 8745e67227e8134ed3d858e95b2e4ab1cb3f35f9 Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 15 May 2025 11:04:57 -0400 Subject: [PATCH 06/13] fixed linting issues --- pkg/connector/team.go | 10 +++++----- test/mocks/endpointpattern.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/connector/team.go b/pkg/connector/team.go index 955e8510..782faa20 100644 --- a/pkg/connector/team.go +++ b/pkg/connector/team.go @@ -154,7 +154,7 @@ func (o *teamResourceType) Entitlements(ctx context.Context, resource *v2.Resour // Get organization roles for this team orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) if err != nil { - return rv, "", nil, nil // Return what we have so far if we can't get org name + return rv, "", nil, err } roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) @@ -163,13 +163,13 @@ func (o *teamResourceType) Entitlements(ctx context.Context, resource *v2.Resour if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { return rv, "", nil, nil // Return what we have so far if we don't have permission } - return rv, "", nil, nil // Return what we have so far if request failed + return rv, "", nil, err } // Get team ID for checking role assignments teamID, err := strconv.ParseInt(resource.Id.Resource, 10, 64) if err != nil { - return rv, "", nil, nil // Return what we have so far if we can't parse team ID + return rv, "", nil, err } // Add organization role entitlements only for roles the team is assigned to @@ -281,7 +281,7 @@ func (o *teamResourceType) Grants(ctx context.Context, resource *v2.Resource, pT // Get organization roles for this team orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) if err != nil { - return rv, pageToken, reqAnnos, nil // Return what we have so far if we can't get org name + return rv, pageToken, reqAnnos, err } roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) @@ -290,7 +290,7 @@ func (o *teamResourceType) Grants(ctx context.Context, resource *v2.Resource, pT if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { return rv, pageToken, reqAnnos, nil // Return what we have so far if we don't have permission } - return rv, pageToken, reqAnnos, nil // Return what we have so far if request failed + return rv, pageToken, reqAnnos, err } // Add grants for organization roles diff --git a/test/mocks/endpointpattern.go b/test/mocks/endpointpattern.go index 039f278c..c57cefff 100644 --- a/test/mocks/endpointpattern.go +++ b/test/mocks/endpointpattern.go @@ -42,7 +42,7 @@ var GetOrganizationsTeamsMembershipsByTeamIdByUsername = mock.EndpointPattern{ Method: "GET", } -// Organization role endpoints +// Organization role endpoints. var GetOrgsRolesByOrg = mock.EndpointPattern{ Pattern: "/orgs/{org}/organization-roles", Method: "GET", From 03d1e5df88ce81e2eb9a5abcd220b32fb36449a3 Mon Sep 17 00:00:00 2001 From: Ali Date: Mon, 19 May 2025 11:41:18 -0600 Subject: [PATCH 07/13] Refactor orgRole resource methods to simplify pagination handling. Removed unnecessary page token parsing and streamlined return values for list and grants functions. Improved handling of permission errors and added pagination logic for teams and users. --- pkg/connector/org_role.go | 56 +++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go index 3eb8e21d..590e6b8a 100644 --- a/pkg/connector/org_role.go +++ b/pkg/connector/org_role.go @@ -75,11 +75,6 @@ func (o *orgRoleResourceType) List( return nil, "", nil, nil } - bag, _, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) - if err != nil { - return nil, "", nil, err - } - orgName, err := o.orgCache.GetOrgName(ctx, parentID) if err != nil { return nil, "", nil, err @@ -90,11 +85,7 @@ func (o *orgRoleResourceType) List( // Handle permission errors gracefully if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { // Return empty list with no error to indicate we skipped this resource - pageToken, err := bag.NextToken("") - if err != nil { - return nil, "", nil, err - } - return nil, pageToken, nil, nil + return nil, "", nil, nil } return nil, "", nil, fmt.Errorf("failed to list organization roles: %w", err) } @@ -112,12 +103,7 @@ func (o *orgRoleResourceType) List( ret = append(ret, roleResource) } - pageToken, err := bag.NextToken("") - if err != nil { - return nil, "", nil, err - } - - return ret, pageToken, nil, nil + return ret, "", nil, nil } func (o *orgRoleResourceType) Entitlements( @@ -147,7 +133,7 @@ func (o *orgRoleResourceType) Grants( return nil, "", nil, nil } - bag, _, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) + bag, page, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) if err != nil { return nil, "", nil, err } @@ -165,7 +151,11 @@ func (o *orgRoleResourceType) Grants( var ret []*v2.Grant // First, get teams with this role - teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, nil) + opts := &github.ListOptions{ + Page: page, + PerPage: pToken.Size, + } + teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, opts) if err != nil { // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { @@ -198,22 +188,33 @@ func (o *orgRoleResourceType) Grants( ret = append(ret, grant) } + // If we got a full page of teams, return them and let the next page handle users + if len(teams) == pToken.Size { + pageToken, err := bag.NextToken(fmt.Sprintf("%d", page+1)) + if err != nil { + return nil, "", nil, err + } + return ret, pageToken, nil, nil + } + // Then, get direct user assignments - users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, nil) + users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, opts) if err != nil { + // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + // Return empty list with no error to indicate we skipped this resource pageToken, err := bag.NextToken("") if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil + return nil, pageToken, nil, nil } return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) } - // Create regular grants for direct user assignments. + // Create grants for users for _, user := range users { - userResource, err := userResource(ctx, user, user.GetEmail(), nil) + userResource, err := userResource(ctx, user, *user.Email, nil) if err != nil { return nil, "", nil, err } @@ -226,11 +227,20 @@ func (o *orgRoleResourceType) Grants( ret = append(ret, grant) } + // If we got a full page of users, return them and let the next page handle more users + if len(users) == pToken.Size { + pageToken, err := bag.NextToken(fmt.Sprintf("%d", page+1)) + if err != nil { + return nil, "", nil, err + } + return ret, pageToken, nil, nil + } + + // No more pages pageToken, err := bag.NextToken("") if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil } From a76b43734b24886f5c76a991d0f6b0807bf83142 Mon Sep 17 00:00:00 2001 From: Ali Date: Mon, 19 May 2025 11:52:08 -0600 Subject: [PATCH 08/13] Add pagination support for teams and users in org role tests. Enhanced mock GitHub server to handle paginated responses for teams and users, and updated tests to verify correct handling of grants and pagination scenarios. --- pkg/connector/org_role_test.go | 48 +++++++++++++++++++- test/mocks/github.go | 81 ++++++++++++++++++++++++++++++++-- 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/pkg/connector/org_role_test.go b/pkg/connector/org_role_test.go index 09ddfaaf..c5350b19 100644 --- a/pkg/connector/org_role_test.go +++ b/pkg/connector/org_role_test.go @@ -22,6 +22,10 @@ func TestOrgRole(t *testing.T) { githubOrganization, _, _, githubUser, _ := mgh.Seed() + // Add user to org role + roleId := int64(1) + mgh.AddUserToOrgRole(roleId, *githubUser.ID) + githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) client := orgRoleBuilder(githubClient, cache) @@ -39,16 +43,19 @@ func TestOrgRole(t *testing.T) { Resource: role, } + // Grant the role to the user grantAnnotations, err := client.Grant(ctx, user, &entitlement) require.Nil(t, err) require.Empty(t, grantAnnotations) + // Check that we can see both teams and users in the grants list grants, nextToken, grantsAnnotations, err := client.Grants(ctx, role, &pagination.Token{}) require.Nil(t, err) test.AssertNoRatelimitAnnotations(t, grantsAnnotations) - require.Equal(t, "", nextToken) - require.Len(t, grants, 1) + require.Empty(t, nextToken) // No next token since we don't have a full page + require.Len(t, grants, 2) // Should get both the team and user + // Revoke the role from the user grant := v2.Grant{ Entitlement: &entitlement, Principal: user, @@ -91,4 +98,41 @@ func TestOrgRole(t *testing.T) { require.Empty(t, nextToken) test.AssertNoRatelimitAnnotations(t, grantsAnnotations) }) + + t.Run("should handle pagination for teams and users", func(t *testing.T) { + mockGithub := mocks.NewMockGitHub() + githubOrganization, _, _, githubUser, _ := mockGithub.Seed() + + // Add more teams to trigger pagination + for i := 0; i < 3; i++ { + teamId := int64(100 + i) + team := github.Team{ + ID: &teamId, + Organization: githubOrganization, + } + mockGithub.AddTeam(team) + } + + // Add user to org role + roleId := int64(1) + mockGithub.AddUserToOrgRole(roleId, *githubUser.ID) + + githubClient := github.NewClient(mockGithub.Server()) + cache := newOrgNameCache(githubClient) + client := orgRoleBuilder(githubClient, cache) + + organization, _ := organizationResource(ctx, githubOrganization, nil) + role, _ := orgRoleResource(ctx, &OrganizationRole{ + ID: 1, + Name: "Test Role", + Description: "Test Role Description", + }, organization) + + // Test first page (should get all teams and users) + grants, nextToken, annotations, err := client.Grants(ctx, role, &pagination.Token{Size: 5}) + require.Nil(t, err) + require.Empty(t, nextToken) // No next token since we got all results + require.Len(t, grants, 5) // Should get all 4 teams (3 added + 1 from Seed) and 1 user + test.AssertNoRatelimitAnnotations(t, annotations) + }) } diff --git a/test/mocks/github.go b/test/mocks/github.go index 1a65ce6a..33952c30 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -545,8 +545,36 @@ func (mgh MockGitHub) getOrgRoleTeams( return } - // Return empty teams list for now - _, _ = w.Write(mock.MustMarshal([]*github.Team{})) + // Get pagination parameters + page := 1 + perPage := 30 + if pageStr, ok := variables["page"]; ok { + if p, err := strconv.Atoi(pageStr); err == nil { + page = p + } + } + if perPageStr, ok := variables["per_page"]; ok { + if pp, err := strconv.Atoi(perPageStr); err == nil { + perPage = pp + } + } + + // Return paginated teams + teams := make([]*github.Team, 0) + start := (page - 1) * perPage + end := start + perPage + i := 0 + for _, team := range mgh.teams { + if i >= start && i < end { + teams = append(teams, &team) + } + i++ + if i >= end { + break + } + } + + _, _ = w.Write(mock.MustMarshal(teams)) } func (mgh MockGitHub) getOrgRoleUsers( @@ -560,10 +588,34 @@ func (mgh MockGitHub) getOrgRoleUsers( return } + // Get pagination parameters + page := 1 + perPage := 30 + if pageStr, ok := variables["page"]; ok { + if p, err := strconv.Atoi(pageStr); err == nil { + page = p + } + } + if perPageStr, ok := variables["per_page"]; ok { + if pp, err := strconv.Atoi(perPageStr); err == nil { + perPage = pp + } + } + + // Return paginated users users := make([]github.User, 0) + start := (page - 1) * perPage + end := start + perPage + i := 0 for _, userID := range memberships.ToSlice() { - if user, ok := mgh.users[userID]; ok { - users = append(users, user) + if i >= start && i < end { + if user, ok := mgh.users[userID]; ok { + users = append(users, user) + } + } + i++ + if i >= end { + break } } @@ -655,3 +707,24 @@ func (mgh MockGitHub) Server() *http.Client { } return mock.NewMockedHTTPClient(options...) } + +// AddTeam adds a team to the mock server for testing purposes. +func (mgh *MockGitHub) AddTeam(team github.Team) { + mgh.teams[*team.ID] = team +} + +// AddUserToOrgRole adds a user to an org role for testing purposes. +func (mgh *MockGitHub) AddUserToOrgRole(roleID int64, userID int64) { + if _, ok := mgh.orgRoles[roleID]; !ok { + mgh.orgRoles[roleID] = mapset.NewSet[int64]() + } + mgh.orgRoles[roleID].Add(userID) +} + +// AddTeamToOrgRole adds a team to an org role for testing purposes. +func (mgh *MockGitHub) AddTeamToOrgRole(roleID int64, teamID int64) { + if _, ok := mgh.orgRoles[roleID]; !ok { + mgh.orgRoles[roleID] = mapset.NewSet[int64]() + } + mgh.orgRoles[roleID].Add(teamID) +} From f9d07649a4c709b04d9c1b7783cfdd40f39054c3 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 May 2025 07:47:18 -0600 Subject: [PATCH 09/13] Refactor Grants method in orgRole to improve pagination and error handling. Streamlined resource type checks and enhanced grant creation for users and teams, ensuring proper handling of permission errors and returning accurate annotations. --- pkg/connector/org_role.go | 161 ++++++++++++++++++++++---------------- 1 file changed, 92 insertions(+), 69 deletions(-) diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go index 590e6b8a..b7f3c42e 100644 --- a/pkg/connector/org_role.go +++ b/pkg/connector/org_role.go @@ -133,7 +133,7 @@ func (o *orgRoleResourceType) Grants( return nil, "", nil, nil } - bag, page, err := parsePageToken(pToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrgRole.Id}) + bag, page, err := parsePageToken(pToken.Token, resource.Id) if err != nil { return nil, "", nil, err } @@ -143,105 +143,128 @@ func (o *orgRoleResourceType) Grants( return nil, "", nil, err } + var rv []*v2.Grant + var reqAnnos annotations.Annotations + roleID, err := strconv.ParseInt(resource.Id.Resource, 10, 64) if err != nil { return nil, "", nil, fmt.Errorf("invalid role ID: %w", err) } - var ret []*v2.Grant - - // First, get teams with this role - opts := &github.ListOptions{ - Page: page, - PerPage: pToken.Size, - } - teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, opts) - if err != nil { - // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - // Return empty list with no error to indicate we skipped this resource - pageToken, err := bag.NextToken("") - if err != nil { - return nil, "", nil, err + switch bag.ResourceTypeID() { + case resourceTypeOrgRole.Id: + bag.Pop() + bag.Push(pagination.PageState{ + ResourceTypeID: resourceTypeUser.Id, + }) + bag.Push(pagination.PageState{ + ResourceTypeID: resourceTypeTeam.Id, + }) + case resourceTypeUser.Id: + opts := &github.ListOptions{ + Page: page, + } + users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, opts) + if err != nil { + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return rv, pageToken, nil, nil } - return nil, pageToken, nil, nil + return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) } - return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) - } - - // Create expandable grants for teams. To show inherited roles, we need to show the teams that have the role. - for _, team := range teams { - teamResource, err := teamResource(team, resource.ParentResourceId) + nextPage, respAnnos, err := parseResp(resp) if err != nil { return nil, "", nil, err } + reqAnnos = respAnnos - grant := grant.NewGrant( - resource, - "assigned", - teamResource.Id, - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{fmt.Sprintf("team:%d:member", team.GetID())}, - Shallow: true, - }), - ) - ret = append(ret, grant) - } - - // If we got a full page of teams, return them and let the next page handle users - if len(teams) == pToken.Size { - pageToken, err := bag.NextToken(fmt.Sprintf("%d", page+1)) + err = bag.Next(nextPage) if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil - } - // Then, get direct user assignments - users, resp, err := o.client.Organizations.ListUsersAssignedToOrgRole(ctx, orgName, roleID, opts) - if err != nil { - // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - // Return empty list with no error to indicate we skipped this resource - pageToken, err := bag.NextToken("") + // Create regular grants for direct user assignments. + for _, user := range users { + userResource, err := userResource(ctx, user, user.GetEmail(), nil) if err != nil { return nil, "", nil, err } - return nil, pageToken, nil, nil + + grant := grant.NewGrant( + resource, + "assigned", + userResource.Id, + grant.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("org-role:%s:%d:%d", resource.Id.Resource, user.GetID(), roleID), + }), + ) + grant.Principal = userResource + rv = append(rv, grant) + } + case resourceTypeTeam.Id: + opts := &github.ListOptions{ + Page: page, + } + teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, roleID, opts) + if err != nil { + // Handle permission errors without erroring out. Some customers may not want to give us permissions to get org roles and members. + if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { + // Return empty list with no error to indicate we skipped this resource + pageToken, err := bag.NextToken("") + if err != nil { + return nil, "", nil, err + } + return nil, pageToken, nil, nil + } + return nil, "", nil, fmt.Errorf("failed to list role teams: %w", err) } - return nil, "", nil, fmt.Errorf("failed to list role users: %w", err) - } - // Create grants for users - for _, user := range users { - userResource, err := userResource(ctx, user, *user.Email, nil) + nextPage, respAnnos, err := parseResp(resp) if err != nil { return nil, "", nil, err } + reqAnnos = respAnnos - grant := grant.NewGrant( - resource, - "assigned", - userResource.Id, - ) - ret = append(ret, grant) - } - - // If we got a full page of users, return them and let the next page handle more users - if len(users) == pToken.Size { - pageToken, err := bag.NextToken(fmt.Sprintf("%d", page+1)) + err = bag.Next(nextPage) if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil - } - // No more pages - pageToken, err := bag.NextToken("") + // Create expandable grants for teams. To show inherited roles, we need to show the teams that have the role. + for _, team := range teams { + teamResource, err := teamResource(team, resource.ParentResourceId) + if err != nil { + return nil, "", nil, err + } + rv = append(rv, grant.NewGrant( + resource, + "assigned", + teamResource.Id, + grant.WithAnnotation(&v2.V1Identifier{ + Id: fmt.Sprintf("org-role-grant:%s:%d:%s", resource.Id.Resource, team.GetID(), "assigned"), + }, + &v2.GrantExpandable{ + EntitlementIds: []string{ + entitlement.NewEntitlementID(teamResource, teamRoleMaintainer), + entitlement.NewEntitlementID(teamResource, teamRoleMember), + }, + Shallow: true, + }, + ), + )) + } + default: + return nil, "", nil, fmt.Errorf("unexpected resource type while fetching grants for org role") + } + pageToken, err := bag.Marshal() if err != nil { return nil, "", nil, err } - return ret, pageToken, nil, nil + + return rv, pageToken, reqAnnos, nil } func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { From 0fca2ccae4fb2d74c683e743fe509dac5e64d799 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 May 2025 08:56:47 -0600 Subject: [PATCH 10/13] Refactor Grant method in orgRole to enhance error handling and role verification. Replaced role existence check with a direct API request, improved entitlement ID validation, and streamlined request creation for assigning roles to users. --- pkg/connector/org_role.go | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/pkg/connector/org_role.go b/pkg/connector/org_role.go index b7f3c42e..d2cbe4fe 100644 --- a/pkg/connector/org_role.go +++ b/pkg/connector/org_role.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "strconv" + "strings" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" @@ -270,7 +271,6 @@ func (o *orgRoleResourceType) Grants( func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { l := ctxzap.Extract(ctx) - // Needs review, I copied this from the team grant function, but roles can be granted to teams as well, but we don't necessarily support that so wasn't sure if this was the intended behavior. if principal.Id.ResourceType != resourceTypeUser.Id { l.Warn( "github-connector: only users can be granted organization roles", @@ -291,24 +291,17 @@ func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, } // First verify that the role exists - roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) + req, err := o.client.NewRequest("GET", fmt.Sprintf("orgs/%s/organization-roles/%d", orgName, roleID), nil) if err != nil { - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - return nil, fmt.Errorf("failed to verify role: organization not found or insufficient permissions") - } - return nil, fmt.Errorf("failed to verify role: %w", err) + return nil, fmt.Errorf("failed to create request: %w", err) } - // Check if the role exists - roleExists := false - for _, role := range roles.CustomRepoRoles { - if role.GetID() == roleID { - roleExists = true - break - } + resp, err := o.client.Do(ctx, req, nil) + if err != nil { + return nil, fmt.Errorf("failed to get role existence: %w", err) } - if !roleExists { + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("role with ID %d not found in organization %s", roleID, orgName) } @@ -317,25 +310,21 @@ func (o *orgRoleResourceType) Grant(ctx context.Context, principal *v2.Resource, return nil, fmt.Errorf("invalid user ID: %w", err) } + enIDParts := strings.Split(entitlement.Id, ":") + if len(enIDParts) != 3 { + return nil, fmt.Errorf("github-connectorv2: invalid entitlement ID: %s", entitlement.Id) + } + user, _, err := o.client.Users.GetByID(ctx, userID) if err != nil { return nil, fmt.Errorf("failed to get user: %w", err) } - l.Info("attempting to assign role", - zap.String("org", orgName), - zap.Int64("role_id", roleID), - zap.String("user", user.GetLogin()), - ) - - // Use the client's HTTP client to make the request with the correct URL format. Couldn't find a built in function for this. Cursor suggested this approach. - url := fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID) - req, err := o.client.NewRequest("PUT", url, nil) + reqs, err := o.client.NewRequest("PUT", fmt.Sprintf("orgs/%s/organization-roles/users/%s/%d", orgName, user.GetLogin(), roleID), nil) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } - - resp, err = o.client.Do(ctx, req, nil) + resp, err = o.client.Do(ctx, reqs, nil) if err != nil { if resp != nil { l.Error("failed to assign role", From 50b8537161e43d9c2cd0e9f102f62ccd0423d6e1 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 May 2025 11:49:44 -0600 Subject: [PATCH 11/13] support org role tests by updating Seed method to include OrganizationRole. Refactor test cases to utilize the new role data --- pkg/connector/org_role_test.go | 99 +++++++++++++------------------- pkg/connector/org_test.go | 2 +- pkg/connector/repository_test.go | 2 +- pkg/connector/team_test.go | 2 +- pkg/connector/user_test.go | 2 +- test/mocks/github.go | 84 ++++++++++++++++++++++----- 6 files changed, 112 insertions(+), 79 deletions(-) diff --git a/pkg/connector/org_role_test.go b/pkg/connector/org_role_test.go index c5350b19..c3adeb6f 100644 --- a/pkg/connector/org_role_test.go +++ b/pkg/connector/org_role_test.go @@ -20,27 +20,23 @@ func TestOrgRole(t *testing.T) { t.Run("should grant and revoke entitlements", func(t *testing.T) { mgh := mocks.NewMockGitHub() - githubOrganization, _, _, githubUser, _ := mgh.Seed() - - // Add user to org role - roleId := int64(1) - mgh.AddUserToOrgRole(roleId, *githubUser.ID) + githubOrganization, _, _, githubUser, orgRole, _ := mgh.Seed() githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) client := orgRoleBuilder(githubClient, cache) organization, _ := organizationResource(ctx, githubOrganization, nil) - role, _ := orgRoleResource(ctx, &OrganizationRole{ - ID: 1, - Name: "Test Role", - Description: "Test Role Description", + roleResource, _ := orgRoleResource(ctx, &OrganizationRole{ + ID: orgRole.ID, + Name: orgRole.Name, + Description: orgRole.Description, }, organization) user, _ := userResource(ctx, githubUser, *githubUser.Email, nil) entitlement := v2.Entitlement{ - Id: entitlement2.NewEntitlementID(role, "assigned"), - Resource: role, + Id: entitlement2.NewEntitlementID(roleResource, "assigned"), + Resource: roleResource, } // Grant the role to the user @@ -48,14 +44,33 @@ func TestOrgRole(t *testing.T) { require.Nil(t, err) require.Empty(t, grantAnnotations) - // Check that we can see both teams and users in the grants list - grants, nextToken, grantsAnnotations, err := client.Grants(ctx, role, &pagination.Token{}) - require.Nil(t, err) - test.AssertNoRatelimitAnnotations(t, grantsAnnotations) - require.Empty(t, nextToken) // No next token since we don't have a full page - require.Len(t, grants, 2) // Should get both the team and user + grants := make([]*v2.Grant, 0) + bag := &pagination.Bag{} + for { + pToken := pagination.Token{} + state := bag.Current() + if state != nil { + token, _ := bag.Marshal() + pToken.Token = token + } + + nextGrants, nextToken, grantsAnnotations, err := client.Grants(ctx, roleResource, &pToken) + grants = append(grants, nextGrants...) + + require.Nil(t, err) + test.AssertNoRatelimitAnnotations(t, grantsAnnotations) + if nextToken == "" { + break + } + + err = bag.Unmarshal(nextToken) + if err != nil { + t.Error(err) + } + } + + require.Len(t, grants, 2) - // Revoke the role from the user grant := v2.Grant{ Entitlement: &entitlement, Principal: user, @@ -70,7 +85,7 @@ func TestOrgRole(t *testing.T) { mockGithub := mocks.NewMockGitHub() mockGithub.SimulateOrgRolePermErr = true - githubOrganization, _, _, _, _ := mockGithub.Seed() + githubOrganization, _, _, _, orgRole, _ := mockGithub.Seed() githubClient := github.NewClient(mockGithub.Server()) cache := newOrgNameCache(githubClient) @@ -87,52 +102,16 @@ func TestOrgRole(t *testing.T) { // Test Grants with permission error role, _ := orgRoleResource(ctx, &OrganizationRole{ - ID: 1, - Name: "Test Role", - Description: "Test Role Description", + ID: orgRole.ID, + Name: orgRole.Name, + Description: orgRole.Description, }, organization) grants, nextToken, grantsAnnotations, err := client.Grants(ctx, role, &pagination.Token{}) require.Nil(t, err) require.Empty(t, grants) - require.Empty(t, nextToken) + // The token should contain the initial state for users + require.NotEmpty(t, nextToken) test.AssertNoRatelimitAnnotations(t, grantsAnnotations) }) - - t.Run("should handle pagination for teams and users", func(t *testing.T) { - mockGithub := mocks.NewMockGitHub() - githubOrganization, _, _, githubUser, _ := mockGithub.Seed() - - // Add more teams to trigger pagination - for i := 0; i < 3; i++ { - teamId := int64(100 + i) - team := github.Team{ - ID: &teamId, - Organization: githubOrganization, - } - mockGithub.AddTeam(team) - } - - // Add user to org role - roleId := int64(1) - mockGithub.AddUserToOrgRole(roleId, *githubUser.ID) - - githubClient := github.NewClient(mockGithub.Server()) - cache := newOrgNameCache(githubClient) - client := orgRoleBuilder(githubClient, cache) - - organization, _ := organizationResource(ctx, githubOrganization, nil) - role, _ := orgRoleResource(ctx, &OrganizationRole{ - ID: 1, - Name: "Test Role", - Description: "Test Role Description", - }, organization) - - // Test first page (should get all teams and users) - grants, nextToken, annotations, err := client.Grants(ctx, role, &pagination.Token{Size: 5}) - require.Nil(t, err) - require.Empty(t, nextToken) // No next token since we got all results - require.Len(t, grants, 5) // Should get all 4 teams (3 added + 1 from Seed) and 1 user - test.AssertNoRatelimitAnnotations(t, annotations) - }) } diff --git a/pkg/connector/org_test.go b/pkg/connector/org_test.go index fa8dc4a2..070dd5ca 100644 --- a/pkg/connector/org_test.go +++ b/pkg/connector/org_test.go @@ -19,7 +19,7 @@ func TestOrganization(t *testing.T) { t.Run("should grant and revoke entitlements", func(t *testing.T) { mgh := mocks.NewMockGitHub() - githubOrganization, _, _, githubUser, _ := mgh.Seed() + githubOrganization, _, _, githubUser, _, _ := mgh.Seed() githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) diff --git a/pkg/connector/repository_test.go b/pkg/connector/repository_test.go index 9a747bb5..3b55663b 100644 --- a/pkg/connector/repository_test.go +++ b/pkg/connector/repository_test.go @@ -20,7 +20,7 @@ func TestRepository(t *testing.T) { t.Run("should grant and revoke entitlements", func(t *testing.T) { mgh := mocks.NewMockGitHub() - githubOrganization, githubRepository, _, githubUser, _ := mgh.Seed() + githubOrganization, githubRepository, _, githubUser, _, _ := mgh.Seed() githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) diff --git a/pkg/connector/team_test.go b/pkg/connector/team_test.go index 7b1e89f9..639a02f5 100644 --- a/pkg/connector/team_test.go +++ b/pkg/connector/team_test.go @@ -20,7 +20,7 @@ func TestTeam(t *testing.T) { t.Run("should grant and revoke entitlements", func(t *testing.T) { mgh := mocks.NewMockGitHub() - githubOrganization, _, githubTeam, githubUser, _ := mgh.Seed() + githubOrganization, _, githubTeam, githubUser, _, _ := mgh.Seed() githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index c91a66b1..51e87546 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -29,7 +29,7 @@ func TestUsersList(t *testing.T) { t.Run(fmt.Sprintf("should get a list of users (SAML:%s)", testCase.message), func(t *testing.T) { mgh := mocks.NewMockGitHub() - githubOrganization, _, _, githubUser, _ := mgh.Seed() + githubOrganization, _, _, githubUser, _, _ := mgh.Seed() organization, err := organizationResource( ctx, diff --git a/test/mocks/github.go b/test/mocks/github.go index 33952c30..113300a2 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -93,6 +93,7 @@ func (mgh MockGitHub) Seed() ( *github.Repository, *github.Team, *github.User, + *OrganizationRole, error, ) { organizationId := int64(12) @@ -137,7 +138,16 @@ func (mgh MockGitHub) Seed() ( mgh.teamMemberships[teamId] = mapset.NewSet[int64](userId) mgh.organizationMemberships[organizationId] = mapset.NewSet[int64](userId) - return &githubOrganization, &githubRepository, &githubTeam, &githubUser, nil + // Add a mock org role + roleId := int64(1) + orgRole := &OrganizationRole{ + ID: roleId, + Name: "Test Role", + Description: "Test Role Description", + } + mgh.orgRoles[roleId] = mapset.NewSet[int64]() // Initialize the set for this role + + return &githubOrganization, &githubRepository, &githubTeam, &githubUser, orgRole, nil } func getResource[T interface{}]( @@ -564,13 +574,29 @@ func (mgh MockGitHub) getOrgRoleTeams( start := (page - 1) * perPage end := start + perPage i := 0 - for _, team := range mgh.teams { - if i >= start && i < end { - teams = append(teams, &team) + + // Get users with this role + roleUsers := mgh.orgRoles[roleID] + + // Find teams that have members with this role + for teamID, team := range mgh.teams { + teamMembers := mgh.teamMemberships[teamID] + if teamMembers == nil { + continue } - i++ - if i >= end { - break + + // Check if any team member has the role + for _, userID := range teamMembers.ToSlice() { + if roleUsers.Contains(userID) { + if i >= start && i < end { + teams = append(teams, &team) + } + i++ + if i >= end { + break + } + break // Found a member with the role, no need to check other members + } } } @@ -647,6 +673,30 @@ func (mgh MockGitHub) removeOrgRoleUser( } } +// Add a handler for GET /orgs/{org}/organization-roles/{role_id} +func (mgh MockGitHub) getOrgRoleByID( + w http.ResponseWriter, + variables map[string]string, +) { + orgID, _ := getCrossTableId(w, variables, "org") + roleID, _ := getCrossTableId(w, variables, "role_id") + if _, ok := mgh.organizations[orgID]; !ok { + w.WriteHeader(http.StatusNotFound) + return + } + // Only support role ID 1 for the mock + if roleID != 1 { + w.WriteHeader(http.StatusNotFound) + return + } + role := &OrganizationRole{ + ID: 1, + Name: "Test Role", + Description: "Test Role Description", + } + _, _ = w.Write(mock.MustMarshal(role)) +} + type handler = func(w http.ResponseWriter, variables map[string]string) // addEndpointHandler takes a string interpolation pattern and a handler @@ -694,9 +744,13 @@ func (mgh MockGitHub) Server() *http.Client { DeleteOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername: mgh.removeMembership, PutOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername: mgh.addMembership, // Add organization role endpoints - GetOrgsRolesByOrg: mgh.getOrgRoles, - GetOrgsRolesTeamsByOrgByRoleId: mgh.getOrgRoleTeams, - GetOrgsRolesUsersByOrgByRoleId: mgh.getOrgRoleUsers, + GetOrgsRolesByOrg: mgh.getOrgRoles, + GetOrgsRolesTeamsByOrgByRoleId: mgh.getOrgRoleTeams, + GetOrgsRolesUsersByOrgByRoleId: mgh.getOrgRoleUsers, + mock.EndpointPattern{ + Pattern: "/orgs/{org}/organization-roles/{role_id}", + Method: "GET", + }: mgh.getOrgRoleByID, PutOrgsRolesUsersByOrgByRoleIdByUsername: mgh.addOrgRoleUser, DeleteOrgsRolesUsersByOrgByRoleIdByUsername: mgh.removeOrgRoleUser, } @@ -721,10 +775,10 @@ func (mgh *MockGitHub) AddUserToOrgRole(roleID int64, userID int64) { mgh.orgRoles[roleID].Add(userID) } -// AddTeamToOrgRole adds a team to an org role for testing purposes. -func (mgh *MockGitHub) AddTeamToOrgRole(roleID int64, teamID int64) { - if _, ok := mgh.orgRoles[roleID]; !ok { - mgh.orgRoles[roleID] = mapset.NewSet[int64]() +// AddMembership adds a user to a team for testing purposes. +func (mgh *MockGitHub) AddMembership(teamID int64, userID int64) { + if _, ok := mgh.teamMemberships[teamID]; !ok { + mgh.teamMemberships[teamID] = mapset.NewSet[int64]() } - mgh.orgRoles[roleID].Add(teamID) + mgh.teamMemberships[teamID].Add(userID) } From ba811ff78e2025a5b80cde5589052b4b23f0a379 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 May 2025 12:11:00 -0600 Subject: [PATCH 12/13] Refactor Entitlements and Grants methods in teamResourceType to remove organization role handling. Showing the team > org_role entitlement looks good in the UI but provisioning fails since it's expecting team roles not org roles. We need to think about the right approach before emitting these entitlements --- pkg/connector/team.go | 110 +----------------------------------------- 1 file changed, 1 insertion(+), 109 deletions(-) diff --git a/pkg/connector/team.go b/pkg/connector/team.go index 782faa20..268ad8b3 100644 --- a/pkg/connector/team.go +++ b/pkg/connector/team.go @@ -3,7 +3,6 @@ package connector import ( "context" "fmt" - "net/http" "strconv" "strings" @@ -129,10 +128,8 @@ func (o *teamResourceType) List(ctx context.Context, parentID *v2.ResourceId, pt return rv, pageToken, reqAnnos, nil } -func (o *teamResourceType) Entitlements(ctx context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { +func (o *teamResourceType) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { rv := make([]*v2.Entitlement, 0, len(teamAccessLevels)) - - // Add team role entitlements for _, level := range teamAccessLevels { rv = append( rv, @@ -151,60 +148,6 @@ func (o *teamResourceType) Entitlements(ctx context.Context, resource *v2.Resour ) } - // Get organization roles for this team - orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) - if err != nil { - return rv, "", nil, err - } - - roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) - if err != nil { - // Handle permission errors gracefully - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - return rv, "", nil, nil // Return what we have so far if we don't have permission - } - return rv, "", nil, err - } - - // Get team ID for checking role assignments - teamID, err := strconv.ParseInt(resource.Id.Resource, 10, 64) - if err != nil { - return rv, "", nil, err - } - - // Add organization role entitlements only for roles the team is assigned to - for _, role := range roles.CustomRepoRoles { - // Check if team is assigned to this role - teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, role.GetID(), nil) - if err != nil { - // Skip this role if we can't check assignments - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - continue - } - continue - } - - // Check if this team is in the list of teams with this role - for _, team := range teams { - if team.GetID() == teamID { - rv = append( - rv, - entitlement.NewAssignmentEntitlement( - resource, - role.GetName(), - entitlement.WithDisplayName(role.GetName()), - entitlement.WithDescription(role.GetDescription()), - entitlement.WithAnnotation(&v2.V1Identifier{ - Id: fmt.Sprintf("team:%s:org_role:%d", resource.Id.Resource, role.GetID()), - }), - entitlement.WithGrantableTo(resourceTypeUser), - ), - ) - break // Found the team, no need to check other teams - } - } - } - return rv, "", nil, nil } @@ -278,57 +221,6 @@ func (o *teamResourceType) Grants(ctx context.Context, resource *v2.Resource, pT )) } - // Get organization roles for this team - orgName, err := o.orgCache.GetOrgName(ctx, resource.ParentResourceId) - if err != nil { - return rv, pageToken, reqAnnos, err - } - - roles, resp, err := o.client.Organizations.ListRoles(ctx, orgName) - if err != nil { - // Handle permission errors gracefully - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - return rv, pageToken, reqAnnos, nil // Return what we have so far if we don't have permission - } - return rv, pageToken, reqAnnos, err - } - - // Add grants for organization roles - for _, role := range roles.CustomRepoRoles { - // Check if team is assigned to this role - teams, resp, err := o.client.Organizations.ListTeamsAssignedToOrgRole(ctx, orgName, role.GetID(), nil) - if err != nil { - // Skip this role if we can't check assignments - if resp != nil && (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound) { - continue - } - continue - } - - // Check if this team is in the list of teams with this role - for _, team := range teams { - if team.GetID() == githubID { - // Create a grant for each team member with this role - for _, user := range users { - ur, err := userResource(ctx, user, user.GetEmail(), nil) - if err != nil { - continue - } - - rv = append(rv, grant.NewGrant( - resource, - role.GetName(), - ur.Id, - grant.WithAnnotation(&v2.V1Identifier{ - Id: fmt.Sprintf("team-org-role-grant:%s:%d:%s", resource.Id.Resource, user.GetID(), role.GetName()), - }), - )) - } - break // Found the team, no need to check other teams - } - } - } - return rv, pageToken, reqAnnos, nil } From e705b66e9155c420ac3af21787c524d0954647ba Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 May 2025 12:13:23 -0600 Subject: [PATCH 13/13] linting --- test/mocks/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/github.go b/test/mocks/github.go index 113300a2..42cdb407 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -673,7 +673,7 @@ func (mgh MockGitHub) removeOrgRoleUser( } } -// Add a handler for GET /orgs/{org}/organization-roles/{role_id} +// Add a handler for GET /orgs/{org}/organization-roles/{role_id}. func (mgh MockGitHub) getOrgRoleByID( w http.ResponseWriter, variables map[string]string,