Skip to content

Commit cf1a38b

Browse files
authored
Fix get reviewers' bug (#32415) (#32616)
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
1 parent 073ba97 commit cf1a38b

File tree

12 files changed

+227
-158
lines changed

12 files changed

+227
-158
lines changed

models/organization/team_repo.go

+14
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"code.gitea.io/gitea/models/db"
1010
"code.gitea.io/gitea/models/perm"
1111
repo_model "code.gitea.io/gitea/models/repo"
12+
"code.gitea.io/gitea/models/unit"
1213

1314
"xorm.io/builder"
1415
)
@@ -83,3 +84,16 @@ func GetTeamsWithAccessToRepo(ctx context.Context, orgID, repoID int64, mode per
8384
OrderBy("name").
8485
Find(&teams)
8586
}
87+
88+
// GetTeamsWithAccessToRepoUnit returns all teams in an organization that have given access level to the repository special unit.
89+
func GetTeamsWithAccessToRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type) ([]*Team, error) {
90+
teams := make([]*Team, 0, 5)
91+
return teams, db.GetEngine(ctx).Where("team_unit.access_mode >= ?", mode).
92+
Join("INNER", "team_repo", "team_repo.team_id = team.id").
93+
Join("INNER", "team_unit", "team_unit.team_id = team.id").
94+
And("team_repo.org_id = ?", orgID).
95+
And("team_repo.repo_id = ?", repoID).
96+
And("team_unit.type = ?", unitType).
97+
OrderBy("name").
98+
Find(&teams)
99+
}

models/organization/team_repo_test.go

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package organization_test
5+
6+
import (
7+
"testing"
8+
9+
"code.gitea.io/gitea/models/db"
10+
"code.gitea.io/gitea/models/organization"
11+
"code.gitea.io/gitea/models/perm"
12+
"code.gitea.io/gitea/models/repo"
13+
"code.gitea.io/gitea/models/unit"
14+
"code.gitea.io/gitea/models/unittest"
15+
16+
"github.com/stretchr/testify/assert"
17+
)
18+
19+
func TestGetTeamsWithAccessToRepoUnit(t *testing.T) {
20+
assert.NoError(t, unittest.PrepareTestDatabase())
21+
22+
org41 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 41})
23+
repo61 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 61})
24+
25+
teams, err := organization.GetTeamsWithAccessToRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests)
26+
assert.NoError(t, err)
27+
if assert.Len(t, teams, 2) {
28+
assert.EqualValues(t, 21, teams[0].ID)
29+
assert.EqualValues(t, 22, teams[1].ID)
30+
}
31+
}

models/repo/user_repo.go

-52
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"code.gitea.io/gitea/models/unit"
1212
user_model "code.gitea.io/gitea/models/user"
1313
"code.gitea.io/gitea/modules/container"
14-
api "code.gitea.io/gitea/modules/structs"
1514

1615
"xorm.io/builder"
1716
)
@@ -146,57 +145,6 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us
146145
return users, nil
147146
}
148147

149-
// GetReviewers get all users can be requested to review:
150-
// * for private repositories this returns all users that have read access or higher to the repository.
151-
// * for public repositories this returns all users that have read access or higher to the repository,
152-
// all repo watchers and all organization members.
153-
// TODO: may be we should have a busy choice for users to block review request to them.
154-
func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) {
155-
// Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries
156-
if err := repo.LoadOwner(ctx); err != nil {
157-
return nil, err
158-
}
159-
160-
cond := builder.And(builder.Neq{"`user`.id": posterID}).
161-
And(builder.Eq{"`user`.is_active": true})
162-
163-
if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate {
164-
// This a private repository:
165-
// Anyone who can read the repository is a requestable reviewer
166-
167-
cond = cond.And(builder.In("`user`.id",
168-
builder.Select("user_id").From("access").Where(
169-
builder.Eq{"repo_id": repo.ID}.
170-
And(builder.Gte{"mode": perm.AccessModeRead}),
171-
),
172-
))
173-
174-
if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID {
175-
// as private *user* repos don't generate an entry in the `access` table,
176-
// the owner of a private repo needs to be explicitly added.
177-
cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID})
178-
}
179-
} else {
180-
// This is a "public" repository:
181-
// Any user that has read access, is a watcher or organization member can be requested to review
182-
cond = cond.And(builder.And(builder.In("`user`.id",
183-
builder.Select("user_id").From("access").
184-
Where(builder.Eq{"repo_id": repo.ID}.
185-
And(builder.Gte{"mode": perm.AccessModeRead})),
186-
).Or(builder.In("`user`.id",
187-
builder.Select("user_id").From("watch").
188-
Where(builder.Eq{"repo_id": repo.ID}.
189-
And(builder.In("mode", WatchModeNormal, WatchModeAuto))),
190-
).Or(builder.In("`user`.id",
191-
builder.Select("uid").From("org_user").
192-
Where(builder.Eq{"org_id": repo.OwnerID}),
193-
)))))
194-
}
195-
196-
users := make([]*user_model.User, 0, 8)
197-
return users, db.GetEngine(ctx).Where(cond).OrderBy(user_model.GetOrderByName()).Find(&users)
198-
}
199-
200148
// GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository
201149
// If isShowFullName is set to true, also include full name prefix search
202150
func GetIssuePostersWithSearch(ctx context.Context, repo *Repository, isPull bool, search string, isShowFullName bool) ([]*user_model.User, error) {

models/repo/user_repo_test.go

-43
Original file line numberDiff line numberDiff line change
@@ -38,46 +38,3 @@ func TestRepoAssignees(t *testing.T) {
3838
assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15)
3939
}
4040
}
41-
42-
func TestRepoGetReviewers(t *testing.T) {
43-
assert.NoError(t, unittest.PrepareTestDatabase())
44-
45-
// test public repo
46-
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
47-
48-
ctx := db.DefaultContext
49-
reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2)
50-
assert.NoError(t, err)
51-
if assert.Len(t, reviewers, 3) {
52-
assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID})
53-
}
54-
55-
// should include doer if doer is not PR poster.
56-
reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2)
57-
assert.NoError(t, err)
58-
assert.Len(t, reviewers, 3)
59-
60-
// should not include PR poster, if PR poster would be otherwise eligible
61-
reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4)
62-
assert.NoError(t, err)
63-
assert.Len(t, reviewers, 2)
64-
65-
// test private user repo
66-
repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
67-
68-
reviewers, err = repo_model.GetReviewers(ctx, repo2, 2, 4)
69-
assert.NoError(t, err)
70-
assert.Len(t, reviewers, 1)
71-
assert.EqualValues(t, reviewers[0].ID, 2)
72-
73-
// test private org repo
74-
repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
75-
76-
reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 1)
77-
assert.NoError(t, err)
78-
assert.Len(t, reviewers, 2)
79-
80-
reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2)
81-
assert.NoError(t, err)
82-
assert.Len(t, reviewers, 1)
83-
}

routers/api/v1/repo/collaborators.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ import (
1818
"code.gitea.io/gitea/routers/api/v1/utils"
1919
"code.gitea.io/gitea/services/context"
2020
"code.gitea.io/gitea/services/convert"
21+
issue_service "code.gitea.io/gitea/services/issue"
22+
pull_service "code.gitea.io/gitea/services/pull"
2123
repo_service "code.gitea.io/gitea/services/repository"
2224
)
2325

@@ -323,7 +325,13 @@ func GetReviewers(ctx *context.APIContext) {
323325
// "404":
324326
// "$ref": "#/responses/notFound"
325327

326-
reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
328+
canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0)
329+
if !canChooseReviewer {
330+
ctx.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers"))
331+
return
332+
}
333+
334+
reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
327335
if err != nil {
328336
ctx.Error(http.StatusInternalServerError, "ListCollaborators", err)
329337
return

routers/web/repo/issue.go

+3-4
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ import (
5656
"code.gitea.io/gitea/services/forms"
5757
issue_service "code.gitea.io/gitea/services/issue"
5858
pull_service "code.gitea.io/gitea/services/pull"
59-
repo_service "code.gitea.io/gitea/services/repository"
6059
user_service "code.gitea.io/gitea/services/user"
6160
)
6261

@@ -693,13 +692,13 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
693692
posterID = 0
694693
}
695694

696-
reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
695+
reviewers, err = pull_service.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
697696
if err != nil {
698697
ctx.ServerError("GetReviewers", err)
699698
return
700699
}
701700

702-
teamReviewers, err = repo_service.GetReviewerTeams(ctx, repo)
701+
teamReviewers, err = pull_service.GetReviewerTeams(ctx, repo)
703702
if err != nil {
704703
ctx.ServerError("GetReviewerTeams", err)
705704
return
@@ -1536,7 +1535,7 @@ func ViewIssue(ctx *context.Context) {
15361535
if issue.IsPull {
15371536
canChooseReviewer := false
15381537
if ctx.Doer != nil && ctx.IsSigned {
1539-
canChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue)
1538+
canChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue.PosterID)
15401539
}
15411540

15421541
RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer)

services/issue/assignee.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
114114
return err
115115
}
116116

117-
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
117+
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
118118

119119
if isAdd {
120120
if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) {
@@ -173,7 +173,7 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
173173
}
174174
}
175175

176-
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
176+
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
177177

178178
if isAdd {
179179
if issue.Repo.IsPrivate {
@@ -267,9 +267,12 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe
267267
}
268268

269269
// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR
270-
func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue) bool {
270+
func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool {
271+
if repo.IsArchived {
272+
return false
273+
}
271274
// The poster of the PR can change the reviewers
272-
if doer.ID == issue.PosterID {
275+
if doer.ID == posterID {
273276
return true
274277
}
275278

services/pull/reviewer.go

+89
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package pull
5+
6+
import (
7+
"context"
8+
9+
"code.gitea.io/gitea/models/db"
10+
"code.gitea.io/gitea/models/organization"
11+
"code.gitea.io/gitea/models/perm"
12+
repo_model "code.gitea.io/gitea/models/repo"
13+
"code.gitea.io/gitea/models/unit"
14+
user_model "code.gitea.io/gitea/models/user"
15+
"code.gitea.io/gitea/modules/container"
16+
17+
"xorm.io/builder"
18+
)
19+
20+
// GetReviewers get all users can be requested to review:
21+
// - Poster should not be listed
22+
// - For collaborator, all users that have read access or higher to the repository.
23+
// - For repository under organization, users under the teams which have read permission or higher of pull request unit
24+
// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers
25+
func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, posterID int64) ([]*user_model.User, error) {
26+
if err := repo.LoadOwner(ctx); err != nil {
27+
return nil, err
28+
}
29+
30+
e := db.GetEngine(ctx)
31+
uniqueUserIDs := make(container.Set[int64])
32+
33+
collaboratorIDs := make([]int64, 0, 10)
34+
if err := e.Table("collaboration").Where("repo_id=?", repo.ID).
35+
And("mode >= ?", perm.AccessModeRead).
36+
Select("user_id").
37+
Find(&collaboratorIDs); err != nil {
38+
return nil, err
39+
}
40+
uniqueUserIDs.AddMultiple(collaboratorIDs...)
41+
42+
if repo.Owner.IsOrganization() {
43+
additionalUserIDs := make([]int64, 0, 10)
44+
if err := e.Table("team_user").
45+
Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id").
46+
Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id").
47+
Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)",
48+
repo.ID, perm.AccessModeRead, unit.TypePullRequests).
49+
Distinct("`team_user`.uid").
50+
Select("`team_user`.uid").
51+
Find(&additionalUserIDs); err != nil {
52+
return nil, err
53+
}
54+
uniqueUserIDs.AddMultiple(additionalUserIDs...)
55+
}
56+
57+
uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers
58+
59+
// Leave a seat for owner itself to append later, but if owner is an organization
60+
// and just waste 1 unit is cheaper than re-allocate memory once.
61+
users := make([]*user_model.User, 0, len(uniqueUserIDs)+1)
62+
if len(uniqueUserIDs) > 0 {
63+
if err := e.In("id", uniqueUserIDs.Values()).
64+
Where(builder.Eq{"`user`.is_active": true}).
65+
OrderBy(user_model.GetOrderByName()).
66+
Find(&users); err != nil {
67+
return nil, err
68+
}
69+
}
70+
71+
// add owner after all users are loaded because we can avoid load owner twice
72+
if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) {
73+
users = append(users, repo.Owner)
74+
}
75+
76+
return users, nil
77+
}
78+
79+
// GetReviewerTeams get all teams can be requested to review
80+
func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) {
81+
if err := repo.LoadOwner(ctx); err != nil {
82+
return nil, err
83+
}
84+
if !repo.Owner.IsOrganization() {
85+
return nil, nil
86+
}
87+
88+
return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests)
89+
}

0 commit comments

Comments
 (0)