Skip to content

Commit fe49cb0

Browse files
authored
Fix get reviewers' bug (#32415)
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 #32394 Fix #32394
1 parent bc7d599 commit fe49cb0

File tree

12 files changed

+225
-158
lines changed

12 files changed

+225
-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
@@ -17,6 +17,8 @@ import (
1717
"code.gitea.io/gitea/routers/api/v1/utils"
1818
"code.gitea.io/gitea/services/context"
1919
"code.gitea.io/gitea/services/convert"
20+
issue_service "code.gitea.io/gitea/services/issue"
21+
pull_service "code.gitea.io/gitea/services/pull"
2022
repo_service "code.gitea.io/gitea/services/repository"
2123
)
2224

@@ -320,7 +322,13 @@ func GetReviewers(ctx *context.APIContext) {
320322
// "404":
321323
// "$ref": "#/responses/notFound"
322324

323-
reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
325+
canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0)
326+
if !canChooseReviewer {
327+
ctx.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers"))
328+
return
329+
}
330+
331+
reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
324332
if err != nil {
325333
ctx.Error(http.StatusInternalServerError, "ListCollaborators", err)
326334
return

routers/web/repo/issue_page_meta.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import (
1919
shared_user "code.gitea.io/gitea/routers/web/shared/user"
2020
"code.gitea.io/gitea/services/context"
2121
issue_service "code.gitea.io/gitea/services/issue"
22-
repo_service "code.gitea.io/gitea/services/repository"
22+
pull_service "code.gitea.io/gitea/services/pull"
2323
)
2424

2525
type issueSidebarMilestoneData struct {
@@ -186,7 +186,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) {
186186
if d.Issue == nil {
187187
data.CanChooseReviewer = true
188188
} else {
189-
data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue)
189+
data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID)
190190
}
191191
}
192192

@@ -231,13 +231,13 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) {
231231

232232
if data.CanChooseReviewer {
233233
var err error
234-
reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
234+
reviewers, err = pull_service.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
235235
if err != nil {
236236
ctx.ServerError("GetReviewers", err)
237237
return
238238
}
239239

240-
teamReviewers, err = repo_service.GetReviewerTeams(ctx, repo)
240+
teamReviewers, err = pull_service.GetReviewerTeams(ctx, repo)
241241
if err != nil {
242242
ctx.ServerError("GetReviewerTeams", err)
243243
return

services/issue/assignee.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
119119
return err
120120
}
121121

122-
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
122+
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
123123

124124
if isAdd {
125125
if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) {
@@ -178,7 +178,7 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
178178
}
179179
}
180180

181-
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
181+
canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
182182

183183
if isAdd {
184184
if issue.Repo.IsPrivate {
@@ -276,12 +276,12 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe
276276
}
277277

278278
// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR
279-
func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue) bool {
279+
func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool {
280280
if repo.IsArchived {
281281
return false
282282
}
283283
// The poster of the PR can change the reviewers
284-
if doer.ID == issue.PosterID {
284+
if doer.ID == posterID {
285285
return true
286286
}
287287

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)