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: 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: diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index 76b6e8b..a264bcc 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -44,9 +44,9 @@ type ValidOwnerConfig struct { type ValidOwner struct { ghClient *github.Client checkScopes bool - orgMembers *map[string]struct{} orgName string - orgTeams []*github.Team + repoUsers []*github.User + 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,105 +220,39 @@ 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 { - 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 @@ -327,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 }