Skip to content

Update security manager operations to use organization roles REST APIs #3421

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

Closed
just-joshing wants to merge 7 commits into from
Closed

Update security manager operations to use organization roles REST APIs #3421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21855f4
Update ListSecurityManagerTeams to use org roles APIs
just-joshing Jan 6, 2025
6e962d9
Update AddSecurityManagerTeam to use org roles APIs
just-joshing Jan 6, 2025
c59f0d2
Update RemoveSecurityManagerTeam to use org roles APIs
just-joshing Jan 6, 2025
d980320
Fix linting errors
just-joshing Jan 6, 2025
1b33bfc
Fixing meta:operations
just-joshing Jan 6, 2025
a524128
Add test coverage for GetSecurityManagerRole
just-joshing Jan 6, 2025
faf36ea
Merge branch 'master' into master
just-joshing Jan 6, 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
78 changes: 54 additions & 24 deletions github/orgs_security_managers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,57 +7,87 @@ package github

import (
"context"
"fmt"
"errors"
)

// ListSecurityManagerTeams lists all security manager teams for an organization.
// GetSecurityManagerRole retrieves the security manager role for an organization.
//
// GitHub API docs: https://docs.github.com/rest/orgs/security-managers#list-security-manager-teams
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#get-all-organization-roles-for-an-organization
//
//meta:operation GET /orgs/{org}/security-managers
func (s *OrganizationsService) ListSecurityManagerTeams(ctx context.Context, org string) ([]*Team, *Response, error) {
u := fmt.Sprintf("orgs/%v/security-managers", org)

req, err := s.client.NewRequest("GET", u, nil)
//meta:operation GET /orgs/{org}/organization-roles
func (s *OrganizationsService) GetSecurityManagerRole(ctx context.Context, org string) (*CustomOrgRoles, *Response, error) {
roles, resp, err := s.ListRoles(ctx, org)
if err != nil {
return nil, nil, err
return nil, resp, err
}

var teams []*Team
resp, err := s.client.Do(ctx, req, &teams)
for _, role := range roles.CustomRepoRoles {
if *role.Name == "security_manager" {
return role, resp, nil
}
}

return nil, resp, errors.New("security manager role not found")
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 7, 2025

Choose a reason for hiding this comment

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

Please add a unit test that covers this error.

}

// ListSecurityManagerTeams lists all security manager teams for an organization.
//
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#get-all-organization-roles-for-an-organization
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#list-teams-that-are-assigned-to-an-organization-role
//
//meta:operation GET /orgs/{org}/organization-roles
//meta:operation GET /orgs/{org}/organization-roles/{role_id}/teams
func (s *OrganizationsService) ListSecurityManagerTeams(ctx context.Context, org string) ([]*Team, *Response, error) {
securityManagerRole, resp, err := s.GetSecurityManagerRole(ctx, org)
if err != nil {
return nil, resp, err
}

return teams, resp, nil
options := &ListOptions{PerPage: 100}
securityManagerTeams := make([]*Team, 0)
for {
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 7, 2025

Choose a reason for hiding this comment

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

With this change, this will be the first endpoint that handles pagination within the call itself, whereas all other endpoints in this repo expect the caller to handle pagination (which also typically involves checking for rate limit errors).

Honestly, I'm not seeing the need here to introduce pagination in this endpoint.
We have a section in the README.md discussing this issue.
Do you have a strong argument for including pagination here?
If we start doing this, then others will start wanting to add it to their favorite endpoint too, and I'm concerned that this will snowball and one implementation won't solve everyone's needs for pagination.

Copy link
Copy Markdown
Contributor Author

@just-joshing just-joshing Jan 7, 2025

Choose a reason for hiding this comment

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

Hi @gmlewis! The reason I did that is because the previous API did not support pagination, so I didn't want to change the contract here. My goal was for these operations to continue to work the same way they did previously so consumers of the library would not face any interruptions from these when updating to newer versions of the library.

But based on other feedback here as well, it sounds like it might be more desirable to keep the library closer to a 1:1 with the GitHub APIs, so I will mark them as deprecated instead.

teams, resp, err := s.ListTeamsAssignedToOrgRole(ctx, org, securityManagerRole.GetID(), options)
if err != nil {
return nil, resp, err
}

securityManagerTeams = append(securityManagerTeams, teams...)
if resp.NextPage == 0 {
return securityManagerTeams, resp, nil
}

options.Page = resp.NextPage
}
}

// AddSecurityManagerTeam adds a team to the list of security managers for an organization.
//
// GitHub API docs: https://docs.github.com/rest/orgs/security-managers#add-a-security-manager-team
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#assign-an-organization-role-to-a-team
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#get-all-organization-roles-for-an-organization
//
//meta:operation PUT /orgs/{org}/security-managers/teams/{team_slug}
//meta:operation GET /orgs/{org}/organization-roles
//meta:operation PUT /orgs/{org}/organization-roles/teams/{team_slug}/{role_id}
func (s *OrganizationsService) AddSecurityManagerTeam(ctx context.Context, org, team string) (*Response, error) {
u := fmt.Sprintf("orgs/%v/security-managers/teams/%v", org, team)
req, err := s.client.NewRequest("PUT", u, nil)
securityManagerRole, resp, err := s.GetSecurityManagerRole(ctx, org)
if err != nil {
return nil, err
return resp, err
}

return s.client.Do(ctx, req, nil)
return s.AssignOrgRoleToTeam(ctx, org, team, securityManagerRole.GetID())
}

// RemoveSecurityManagerTeam removes a team from the list of security managers for an organization.
//
// GitHub API docs: https://docs.github.com/rest/orgs/security-managers#remove-a-security-manager-team
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#get-all-organization-roles-for-an-organization
// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#remove-an-organization-role-from-a-team
//
//meta:operation DELETE /orgs/{org}/security-managers/teams/{team_slug}
//meta:operation GET /orgs/{org}/organization-roles
//meta:operation DELETE /orgs/{org}/organization-roles/teams/{team_slug}/{role_id}
func (s *OrganizationsService) RemoveSecurityManagerTeam(ctx context.Context, org, team string) (*Response, error) {
u := fmt.Sprintf("orgs/%v/security-managers/teams/%v", org, team)
req, err := s.client.NewRequest("DELETE", u, nil)
securityManagerRole, resp, err := s.GetSecurityManagerRole(ctx, org)
if err != nil {
return nil, err
return resp, err
}

return s.client.Do(ctx, req, nil)
return s.RemoveOrgRoleFromTeam(ctx, org, team, securityManagerRole.GetID())
}
69 changes: 62 additions & 7 deletions github/orgs_security_managers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,53 @@ import (
"github.com/google/go-cmp/cmp"
)

func TestOrganizationService_GetSecurityManagerRole(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

handleGetSecurityManagerRole(t, mux, "o")

ctx := context.Background()
role, _, err := client.Organizations.GetSecurityManagerRole(ctx, "o")
if err != nil {
t.Errorf("Organizations.GetSecurityManagerRole returned error: %v", err)
}

want := &CustomOrgRoles{ID: Ptr(int64(138)), Name: Ptr("security_manager")}
if !cmp.Equal(role, want) {
t.Errorf("Organizations.GetSecurityManagerRole returned %+v, want %+v", role, want)
}

const methodName = "GetSecurityManagerRole"
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Organizations.GetSecurityManagerRole(ctx, "\n")
return err
})

testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
got, resp, err := client.Organizations.GetSecurityManagerRole(ctx, "o")
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
}
return resp, err
})
}

func TestOrganizationsService_GetSecurityManagerRole_invalidOrg(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)

ctx := context.Background()
_, _, err := client.Organizations.GetSecurityManagerRole(ctx, "%")
testURLParseError(t, err)
}

func TestOrganizationsService_ListSecurityManagerTeams(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/orgs/o/security-managers", func(w http.ResponseWriter, r *http.Request) {
handleGetSecurityManagerRole(t, mux, "o")
mux.HandleFunc("/orgs/o/organization-roles/138/teams", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
fmt.Fprint(w, `[{"id":1}]`)
})
Expand Down Expand Up @@ -62,7 +104,8 @@ func TestOrganizationsService_AddSecurityManagerTeam(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/orgs/o/security-managers/teams/t", func(w http.ResponseWriter, r *http.Request) {
handleGetSecurityManagerRole(t, mux, "o")
mux.HandleFunc("/orgs/o/organization-roles/teams/t/138", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "PUT")
})

Expand Down Expand Up @@ -94,18 +137,21 @@ func TestOrganizationsService_AddSecurityManagerTeam_invalidOrg(t *testing.T) {

func TestOrganizationsService_AddSecurityManagerTeam_invalidTeam(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
client, mux, _ := setup(t)

handleGetSecurityManagerRole(t, mux, "o")

ctx := context.Background()
_, err := client.Organizations.AddSecurityManagerTeam(ctx, "%", "t")
_, err := client.Organizations.AddSecurityManagerTeam(ctx, "o", "%")
testURLParseError(t, err)
}

func TestOrganizationsService_RemoveSecurityManagerTeam(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/orgs/o/security-managers/teams/t", func(w http.ResponseWriter, r *http.Request) {
handleGetSecurityManagerRole(t, mux, "o")
mux.HandleFunc("/orgs/o/organization-roles/teams/t/138", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "DELETE")
})

Expand Down Expand Up @@ -137,9 +183,18 @@ func TestOrganizationsService_RemoveSecurityManagerTeam_invalidOrg(t *testing.T)

func TestOrganizationsService_RemoveSecurityManagerTeam_invalidTeam(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)
client, mux, _ := setup(t)

handleGetSecurityManagerRole(t, mux, "o")

ctx := context.Background()
_, err := client.Organizations.RemoveSecurityManagerTeam(ctx, "%", "t")
_, err := client.Organizations.RemoveSecurityManagerTeam(ctx, "o", "%")
testURLParseError(t, err)
}

func handleGetSecurityManagerRole(t *testing.T, mux *http.ServeMux, org string) {
mux.HandleFunc(fmt.Sprintf("/orgs/%s/organization-roles", org), func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
fmt.Fprint(w, `{"roles":[{"id":138,"name":"security_manager"}]}`)
})
}