Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add permissions check to valid_owner #62

Merged
merged 2 commits into from Jan 21, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 54 additions & 2 deletions internal/check/valid_owner.go
Expand Up @@ -139,7 +139,7 @@ func (v *ValidOwner) initOrgListTeams(ctx context.Context) *validateError {
PerPage: 100,
}
for {
resultPage, resp, err := v.ghClient.Repositories.ListTeams(ctx, v.orgName, v.orgRepoName, req)
resultPage, resp, err := v.ghClient.Teams.ListTeams(ctx, v.orgName, req)
if err != nil { // TODO(mszostok): implement retry?
switch err := err.(type) {
case *github.ErrorResponse:
Expand Down Expand Up @@ -192,7 +192,59 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr
}

if !teamExists() {
return newValidateError("Team %q does not exist in organization %q or has no permissions associated with the repository.", team, org)
return newValidateError("Team %q does not exist in organization %q.", team, org)
}

// repo contains the permissions for the team slug given
// TODO(mszostok): Switch to GraphQL API, see:
// https://github.com/mszostok/codeowners-validator/pull/62#discussion_r561273525
repo, _, err := v.ghClient.Teams.IsTeamRepoBySlug(ctx, v.orgName, team, org, v.orgRepoName)
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding that functionality, it was also requested some time ago (#21)

Unfortunately, this approach increases the number of external call to GitHub, probably in the near feature it will be good to switch to GraphQL which will remove problem with over fetched data, and we will be able to merge the ListTeams and IsTeamRepoBySlug into single query, see: #21 (comment)

but this is for future, for now, it lgtm 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

That is quite true. There are also potentially issues with GraphQL, which we've experienced with another project we contribute to, the GitHub Terraform provider – some calls because extremely, extremely slow in larger orgs (branch protection) and it annihilated the performance compared to REST. I doubt it'd be an issue here, just sharing our experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add a todo.

Copy link
Owner

Choose a reason for hiding this comment

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

thx for the heads-up 👍

if err != nil { // TODO(mszostok): implement retry?
switch err := err.(type) {
case *github.ErrorResponse:
if err.Response.StatusCode == http.StatusUnauthorized {
return newValidateError(
"Team permissions information for %q/%q could not be queried. Requires GitHub authorization.",
org, v.orgRepoName)
} else if err.Response.StatusCode == http.StatusNotFound {
return newValidateError(
"Team %q does not have permissions associated with the repository %q.",
team, v.orgRepoName)
} else {
return newValidateError("HTTP error occurred while calling GitHub: %v", err)
}
case *github.RateLimitError:
return newValidateError("GitHub rate limit reached: %v", err.Message)
default:
return newValidateError("Unknown error occurred while calling GitHub: %v", err)
}
}

teamHasWritePermission := func() bool {
for k, v := range repo.GetPermissions() {
if !v {
continue
}

switch k {
case
"admin",
"maintain",
"push":
return true
case
"pull",
"triage":
}
}

return false
}

if !teamHasWritePermission() {
return newValidateError(
"Team %q cannot review PRs on %q as neither it nor any parent team does not have write permissions.",
team, v.orgRepoName)
}

return nil
Expand Down