From cdc0d42a9055ddbfa4375ceb84aabe3165785320 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 25 Apr 2024 20:18:26 +0200 Subject: [PATCH 1/4] Simplify and speed up team check This switches from [`Teams.ListTeams`](https://pkg.go.dev/github.com/google/go-github/v41/github#TeamsService.ListTeams) and potentially many calls of [`Teams.IsTeamRepoBySlug`](https://pkg.go.dev/github.com/google/go-github/v41/github#TeamsService.IsTeamRepoBySlug) to just a single [`Repositories.ListTeams`](https://pkg.go.dev/github.com/google/go-github/v41/github#RepositoriesService.ListTeams), which returns all the teams permissions to a specific repository. This does have the minor drawback of not being able to distinguish between teams not existing and teams not being in the organisation, but this does also have the benefit of potentially speeding up the result when there's many teams in the organisation. --- internal/check/valid_owner.go | 85 ++++++----------------------------- 1 file changed, 14 insertions(+), 71 deletions(-) diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index 76b6e8b..2abb69d 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -46,7 +46,7 @@ type ValidOwner struct { checkScopes bool orgMembers *map[string]struct{} orgName string - orgTeams []*github.Team + repoTeams []*github.Team orgRepoName string ignOwners map[string]struct{} allowUnownedPatterns bool @@ -170,18 +170,18 @@ func (v *ValidOwner) selectValidateFn(name string) func(context.Context, string) } } -func (v *ValidOwner) initOrgListTeams(ctx context.Context) *validateError { +func (v *ValidOwner) initRepoTeams(ctx context.Context) *validateError { var teams []*github.Team req := &github.ListOptions{ PerPage: 100, } for { - resultPage, resp, err := v.ghClient.Teams.ListTeams(ctx, v.orgName, req) + resultPage, resp, err := v.ghClient.Repositories.ListTeams(ctx, v.orgName, v.orgRepoName, req) if err != nil { // TODO(mszostok): implement retry? switch err := err.(type) { case *github.ErrorResponse: if err.Response.StatusCode == http.StatusUnauthorized { - return newValidateError("Teams for organization %q could not be queried. Requires GitHub authorization.", v.orgName) + return newValidateError("Teams for repository %q could not be queried. Requires GitHub authorization.", v.orgRepoName) } return newValidateError("HTTP error occurred while calling GitHub: %v", err) case *github.RateLimitError: @@ -197,14 +197,14 @@ func (v *ValidOwner) initOrgListTeams(ctx context.Context) *validateError { req.Page = resp.NextPage } - v.orgTeams = teams + v.repoTeams = teams return nil } func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateError { - if v.orgTeams == nil { - if err := v.initOrgListTeams(ctx); err != nil { + if v.repoTeams == nil { + if err := v.initRepoTeams(ctx); err != nil { return err.AsPermanent() } } @@ -220,74 +220,17 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr return newValidateError("Team %q does not belong to %q organization.", name, v.orgName) } - teamExists := func() bool { - for _, v := range v.orgTeams { - // GitHub normalizes name before comparison - if strings.EqualFold(v.GetSlug(), team) { - return true + for _, t := range v.repoTeams { + // GitHub normalizes name before comparison + if strings.EqualFold(t.GetSlug(), team) { + if t.Permissions["push"] { + return nil } + return newValidateError("Team %q cannot review PRs on %q as neither it nor any parent team has write permissions.", team, v.orgRepoName) } - return false } - if !teamExists() { - return newValidateError("Team %q does not exist in organization %q.", name, 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) - if err != nil { // TODO(mszostok): implement retry? - switch err := err.(type) { - case *github.ErrorResponse: - switch err.Response.StatusCode { - case http.StatusUnauthorized: - return newValidateError( - "Team permissions information for %q/%q could not be queried. Requires GitHub authorization.", - org, v.orgRepoName) - case http.StatusNotFound: - return newValidateError( - "Team %q does not have permissions associated with the repository %q.", - team, v.orgRepoName) - default: - 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 has write permissions.", - team, v.orgRepoName) - } - - return nil + return newValidateError("Team %q does not exist in organization %q or has no access to repository %q", name, org, v.orgRepoName) } func (v *ValidOwner) validateGitHubUser(ctx context.Context, name string) *validateError { From dbba5ba30459c086966a6666eed891dfa9a878a3 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 25 Apr 2024 20:26:06 +0200 Subject: [PATCH 2/4] Speed up user check and check for push access For organizations with many members without write access (e.g. https://github.com/orgs/NixOS/people), this speeds up the user check by switching from [`Organizations.ListMembers`](https://pkg.go.dev/github.com/google/go-github/v41/github#OrganizationsService.ListMembers) to [`Repositories.ListCollaborators`](https://pkg.go.dev/github.com/google/go-github/v41/github#RepositoriesService.ListCollaborators). Furthermore, using `ListCollaborators` we can easily check that the users actually have write access to the repo, the same way as is done for teams since the parent commit. --- internal/check/valid_owner.go | 46 +++++++++++++---------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index 2abb69d..a264bcc 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -44,8 +44,8 @@ type ValidOwnerConfig struct { type ValidOwner struct { ghClient *github.Client checkScopes bool - orgMembers *map[string]struct{} orgName string + repoUsers []*github.User repoTeams []*github.Team orgRepoName string ignOwners map[string]struct{} @@ -234,34 +234,25 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr } func (v *ValidOwner) validateGitHubUser(ctx context.Context, name string) *validateError { - if v.orgMembers == nil { // TODO(mszostok): lazy init, make it more robust. - if err := v.initOrgListMembers(ctx); err != nil { + if v.repoUsers == nil { // TODO(mszostok): lazy init, make it more robust. + if err := v.initRepoUsers(ctx); err != nil { return newValidateError("Cannot initialize organization member list: %v", err).AsPermanent() } } userName := strings.TrimPrefix(name, "@") - _, _, err := v.ghClient.Users.Get(ctx, userName) - if err != nil { // TODO(mszostok): implement retry? - switch err := err.(type) { - case *github.ErrorResponse: - if err.Response.StatusCode == http.StatusNotFound { - return newValidateError("User %q does not have github account", name) + + for _, u := range v.repoUsers { + // GitHub normalizes name before comparison + if strings.EqualFold(u.GetLogin(), userName) { + if u.Permissions["push"] { + return nil } - return newValidateError("HTTP error occurred while calling GitHub: %v", err).AsPermanent() - case *github.RateLimitError: - return newValidateError("GitHub rate limit reached: %v", err.Message).AsPermanent() - default: - return newValidateError("Unknown error occurred while calling GitHub: %v", err).AsPermanent() + return newValidateError("User %q cannot review PRs on %q as they don't have write permissions.", userName, v.orgRepoName) } } - _, isMember := (*v.orgMembers)[userName] - if !isMember { - return newValidateError("User %q is not a member of the organization", name) - } - - return nil + return newValidateError("User %q is not a member of the organization %q or has no access to repository %q", name, v.orgName, v.orgRepoName) } // There is a method to check if user is a org member @@ -270,28 +261,25 @@ func (v *ValidOwner) validateGitHubUser(ctx context.Context, name string) *valid // // But latency is too huge for checking each single user independent // better and faster is to ask for all members and cache them. -func (v *ValidOwner) initOrgListMembers(ctx context.Context) error { - opt := &github.ListMembersOptions{ +func (v *ValidOwner) initRepoUsers(ctx context.Context) error { + var users []*github.User + opt := &github.ListCollaboratorsOptions{ ListOptions: github.ListOptions{PerPage: 100}, } - var allMembers []*github.User for { - users, resp, err := v.ghClient.Organizations.ListMembers(ctx, v.orgName, opt) + resultPage, resp, err := v.ghClient.Repositories.ListCollaborators(ctx, v.orgName, v.orgRepoName, opt) if err != nil { return err } - allMembers = append(allMembers, users...) + users = append(users, resultPage...) if resp.NextPage == 0 { break } opt.Page = resp.NextPage } - v.orgMembers = &map[string]struct{}{} - for _, u := range allMembers { - (*v.orgMembers)[u.GetLogin()] = struct{}{} - } + v.repoUsers = users return nil } From a69f70c0bd8ec168ff695f412afa83c7b7a65413 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 25 Apr 2024 20:36:11 +0200 Subject: [PATCH 3/4] Document that write permissions are checked --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index cc5fd98..3565711 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ The following checks are enabled by default: |-------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | duppatterns | **[Duplicated Pattern Checker]**

Reports if CODEOWNERS file contain duplicated lines with the same file pattern. | | files | **[File Exist Checker]**

Reports if CODEOWNERS file contain lines with the file pattern that do not exist in a given repository. | -| owners | **[Valid Owner Checker]**

Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `user@example.com`
_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_.

**Checks:**
    1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address).

    2. Check if a GitHub owner has a GitHub account

    3. Check if a GitHub owner is in a given organization

    4. Check if an organization team exists | +| owners | **[Valid Owner Checker]**

Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `user@example.com`
_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_.

**Checks:**
    1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address).

    2. Check if the owner has write access to the repository (only for users and teams) | | syntax | **[Valid Syntax Checker]**

Reports if CODEOWNERS file contain invalid syntax definition. It is imported as:
    "If any line in your CODEOWNERS file contains invalid syntax, the file will not be detected
    and will not be used to request reviews. Invalid syntax includes inline comments
    and user or team names that do not exist on GitHub."

_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_. | The experimental checks are disabled by default: From 840eeb88b4da92bda3e13c838f67f6540b9e8529 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 26 Apr 2024 16:32:16 +0200 Subject: [PATCH 4/4] GitHub Apps now instead need Administration repo permissions --- docs/gh-auth.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/gh-auth.md b/docs/gh-auth.md index f41f1b3..57715c4 100644 --- a/docs/gh-auth.md +++ b/docs/gh-auth.md @@ -36,7 +36,7 @@ Here are the steps to create a GitHub App and use it for this tool: 1. [Create a GitHub App](https://docs.github.com/en/developers/apps/building-github-apps/creating-a-github-app). > **Note** > Your app does not need a callback or a webhook URL. -2. Add a read-only permission to the "Members" item of organization permissions. +2. Add a read-only permission to the "Administration" item of repository permissions. 3. [Install the app in your organization](https://docs.github.com/en/developers/apps/managing-github-apps/installing-github-apps). 4. Done! To authenticate with your app, you need: