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.

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

Jump to bottom

Check for user write access (also, more speed and less code!) #222

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Check for user write access (also, more speed and less code!) #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc0d42
Simplify and speed up team check
infinisil Apr 25, 2024
dbba5ba
Speed up user check and check for push access
infinisil Apr 25, 2024
a69f70c
Document that write permissions are checked
infinisil Apr 25, 2024
840eeb8
GitHub Apps now instead need Administration repo permissions
infinisil Apr 26, 2024
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ The following checks are enabled by default:
|-------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| duppatterns | **[Duplicated Pattern Checker]** <br /><br /> Reports if CODEOWNERS file contain duplicated lines with the same file pattern. |
| files | **[File Exist Checker]** <br /><br /> Reports if CODEOWNERS file contain lines with the file pattern that do not exist in a given repository. |
| owners | **[Valid Owner Checker]** <br /><br /> Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `[email protected]` <br /> _source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_. <br /> <br /> **Checks:** <br /> &#x09; &nbsp;&nbsp;&nbsp;&nbsp;1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address). <br /><br />&nbsp;&nbsp;&nbsp;&nbsp;2. Check if a GitHub owner has a GitHub account <br /><br />&nbsp;&nbsp;&nbsp;&nbsp;3. Check if a GitHub owner is in a given organization <br /> <br />&nbsp;&nbsp;&nbsp;&nbsp;4. Check if an organization team exists |
| owners | **[Valid Owner Checker]** <br /><br /> Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `[email protected]` <br /> _source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_. <br /> <br /> **Checks:** <br /> &#x09; &nbsp;&nbsp;&nbsp;&nbsp;1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address). <br /><br />&nbsp;&nbsp;&nbsp;&nbsp;2. Check if the owner has write access to the repository (only for users and teams) |
| syntax | **[Valid Syntax Checker]** <br /><br /> Reports if CODEOWNERS file contain invalid syntax definition. It is imported as: <br />&nbsp;&nbsp;&nbsp;&nbsp;"If any line in your CODEOWNERS file contains invalid syntax, the file will not be detected<br />&nbsp;&nbsp;&nbsp;&nbsp;and will not be used to request reviews. Invalid syntax includes inline comments <br />&nbsp;&nbsp;&nbsp;&nbsp;and user or team names that do not exist on GitHub." <br /> <br /> _source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_. |

The experimental checks are disabled by default:
Expand Down
2 changes: 1 addition & 1 deletion docs/gh-auth.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
131 changes: 31 additions & 100 deletions internal/check/valid_owner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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()
}
}
Expand All @@ -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
Expand All @@ -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
}
Expand Down