From d03ee343693aba8156a3cf02b1a660497aec4001 Mon Sep 17 00:00:00 2001 From: Brandon High Date: Tue, 12 Jan 2021 16:32:37 -0800 Subject: [PATCH 1/2] Add permissions check to valid_owner Previously, the valid_owner check used the Repositories List Teams API endpoint to check if a team was valid. Due to an issue in the GitHub API, that endpoint does not return child teams that would inherit a parent team's permissions. This commit changes the initial check for the validity of a team to use the Teams List Teams API endpoint, which lists all teams in a org to check if the team exists at all. If that check fails, it will clearly state that the team does not exist. As a follow up to the initial valid team check, this commit checks the Teams IsTeamRepoBySlug endpoint, which returns an object containing the actual permissions for the team being checked on the repo. By adding this check, we can verify that a user can actually provide their review on PRs that made the CODEOWNERS line. --- internal/check/valid_owner.go | 49 +++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index de9d2e8..4b13755 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -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: @@ -192,7 +192,52 @@ 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 + repo, _, err := v.ghClient.Teams.IsTeamRepoBySlug(ctx, v.orgName, team, org, v.orgRepoName) + 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.Permissions { + if v == false { + continue + } + + switch k { + case + "admin", + "maintain", + "push", + "triage": + return true + case + "pull": + continue + } + } + + return false + } + + if !teamHasWritePermission() { + return newValidateError("Team %q cannot review PRs on %q.", team, v.orgRepoName) } return nil From bd0c80b512b4b7e5551629ffcb993b63a7a1fbd2 Mon Sep 17 00:00:00 2001 From: Jono Spiro Date: Wed, 20 Jan 2021 21:36:16 -0800 Subject: [PATCH 2/2] address comments from PR #62 --- internal/check/valid_owner.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index 4b13755..2fffa97 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -196,14 +196,20 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr } // 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) 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) + 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) + 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) } @@ -215,8 +221,8 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr } teamHasWritePermission := func() bool { - for k, v := range *repo.Permissions { - if v == false { + for k, v := range repo.GetPermissions() { + if !v { continue } @@ -224,12 +230,11 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr case "admin", "maintain", - "push", - "triage": + "push": return true case - "pull": - continue + "pull", + "triage": } } @@ -237,7 +242,9 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr } if !teamHasWritePermission() { - return newValidateError("Team %q cannot review PRs on %q.", team, v.orgRepoName) + 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