Skip to content

PR reviewers list contains invalid options #32394

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

Closed
jackHay22 opened this issue Oct 31, 2024 · 0 comments · Fixed by #32415
Closed

PR reviewers list contains invalid options #32394

jackHay22 opened this issue Oct 31, 2024 · 0 comments · Fixed by #32415
Labels
topic/pr Issues related to pull requests type/bug
Milestone

Comments

@jackHay22
Copy link
Contributor

jackHay22 commented Oct 31, 2024

Description

Steps to reproduce:

  1. Create a repository owned by an organization.
  2. Create a team in the org with the following permissions. Note: the team has read permission for at least one unit but not for pull requests
Screen Shot 2024-10-31 at 1 35 54 PM
  1. Add a user to this team which is not a repo collaborator or part of any other team in the org. Notably, they do not have read permission for pull requests
  2. They will show up as an option in the reviewers list in a pull request within this org:
Screen Shot 2024-10-31 at 1 35 33 PM 5. If selected, they will not be added and there is no error message
2024/10/31 13:34:51 ...rs/web/repo/issue.go:2532:UpdatePullReviewRequest() [W] UpdatePullReviewRequest: refusing to add invalid review request for <User 12:testsamluser> to <Repository 4:AuditOrg/Test-Repo>#1: Error: Reviewer can't read [user_id: 1, repo_id: 4]

Notes:

func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) {
        ...
	cond = cond.And(builder.In("`user`.id",
		builder.Select("user_id").From("access").Where(
			builder.Eq{"repo_id": repo.ID}.
				And(builder.Gte{"mode": perm.AccessModeRead}),
		),
	))

The GetReviewers function checks the access table to determine review eligibility. However, this table explicit stores the highest level of access for a user within a repository:

// Access represents the highest access level of a user to the repository. The only access type
// that is not in this table is the real owner of a repository. In case of an organization
// repository, the members of the owners team are in this table.
type Access struct {
	ID     int64 `xorm:"pk autoincr"`
	UserID int64 `xorm:"UNIQUE(s)"`
	RepoID int64 `xorm:"UNIQUE(s)"`
	Mode   perm.AccessMode
}

In this case, the access table will show the user as having read permission incorrectly.

Gitea Version

main

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

command-lin

Database

None

@kemzeb kemzeb added the topic/pr Issues related to pull requests label Nov 1, 2024
@lunny lunny added this to the 1.22.4 milestone Nov 4, 2024
lunny added a commit to lunny/gitea that referenced this issue Nov 22, 2024
This PR rewrites `GetReviewer` function and move it to service layer.

Reviewers should not be watchers, so that this PR removed all watchers
from reviewers. When the repository is under an organization, the pull
request unit read permission will be checked to resolve the bug of

Fix go-gitea#32394
lunny added a commit that referenced this issue Nov 23, 2024
This PR rewrites `GetReviewer` function and move it to service layer.

Reviewers should not be watchers, so that this PR removed all watchers
from reviewers. When the repository is under an organization, the pull
request unit read permission will be checked to resolve the bug of

Fix #32394
Backport #32415
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/pr Issues related to pull requests type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants