From ab7cb9509deaa062033fb7f232be6d4975384c40 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 3 Nov 2024 22:50:10 -0800 Subject: [PATCH 1/7] Fix get reviewers' bug --- models/repo/user_repo.go | 86 +++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 41 deletions(-) diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index c305603e02070..c74023511d90c 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -11,7 +11,6 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" - api "code.gitea.io/gitea/modules/structs" "xorm.io/builder" ) @@ -145,54 +144,59 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us } // GetReviewers get all users can be requested to review: -// * for private repositories this returns all users that have read access or higher to the repository. -// * for public repositories this returns all users that have read access or higher to the repository, -// all repo watchers and all organization members. -// TODO: may be we should have a busy choice for users to block review request to them. +// - Poster should not be listed +// - For collabrators, all users that have read access or higher to the repository. +// - For repository under orgnization, users under the teams which have read permission or higher of pull request unit +// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) { - // Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries if err := repo.LoadOwner(ctx); err != nil { return nil, err } - cond := builder.And(builder.Neq{"`user`.id": posterID}). - And(builder.Eq{"`user`.is_active": true}) - - if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { - // This a private repository: - // Anyone who can read the repository is a requestable reviewer - - 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}), - ), - )) + e := db.GetEngine(ctx) + uniqueUserIDs := make(container.Set[int64]) - if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID { - // as private *user* repos don't generate an entry in the `access` table, - // the owner of a private repo needs to be explicitly added. - cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID}) + if repo.Owner.IsOrganization() { + additionalUserIDs := make([]int64, 0, 10) + if err := e.Table("team_user"). + Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). + Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). + Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)", + repo.ID, perm.AccessModeRead, unit.TypePullRequests). + Distinct("`team_user`.uid"). + Select("`team_user`.uid"). + Find(&additionalUserIDs); err != nil { + return nil, err } + uniqueUserIDs.AddMultiple(additionalUserIDs...) } else { - // This is a "public" repository: - // Any user that has read access, is a watcher or organization member can be requested to review - cond = cond.And(builder.And(builder.In("`user`.id", - builder.Select("user_id").From("access"). - Where(builder.Eq{"repo_id": repo.ID}. - And(builder.Gte{"mode": perm.AccessModeRead})), - ).Or(builder.In("`user`.id", - builder.Select("user_id").From("watch"). - Where(builder.Eq{"repo_id": repo.ID}. - And(builder.In("mode", WatchModeNormal, WatchModeAuto))), - ).Or(builder.In("`user`.id", - builder.Select("uid").From("org_user"). - Where(builder.Eq{"org_id": repo.OwnerID}), - ))))) - } - - users := make([]*user_model.User, 0, 8) - return users, db.GetEngine(ctx).Where(cond).OrderBy(user_model.GetOrderByName()).Find(&users) + userIDs := make([]int64, 0, 10) + if err := e.Table("access"). + Where("repo_id = ? AND mode >= ?", repo.ID, perm.AccessModeRead). + Select("user_id"). + Find(&userIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(userIDs...) + } + uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers + + // Leave a seat for owner itself to append later, but if owner is an organization + // and just waste 1 unit is cheaper than re-allocate memory once. + users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) + if len(uniqueUserIDs) > 0 { + if err := e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { + return nil, err + } + } + if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) { + users = append(users, repo.Owner) + } + + return users, nil } // GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository From 672e98e531bfd7f36d6f5c4a72fc0a9e8d3ef9a2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 4 Nov 2024 19:58:43 -0800 Subject: [PATCH 2/7] Fix test --- models/repo/user_repo.go | 56 ---------------- models/repo/user_repo_test.go | 43 ------------- routers/api/v1/repo/collaborators.go | 3 +- services/pull/reviewer.go | 95 ++++++++++++++++++++++++++++ services/pull/reviewer_test.go | 58 +++++++++++++++++ 5 files changed, 155 insertions(+), 100 deletions(-) create mode 100644 services/pull/reviewer.go create mode 100644 services/pull/reviewer_test.go diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index c74023511d90c..17bcb83516bb0 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -143,62 +143,6 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us return users, nil } -// GetReviewers get all users can be requested to review: -// - Poster should not be listed -// - For collabrators, all users that have read access or higher to the repository. -// - For repository under orgnization, users under the teams which have read permission or higher of pull request unit -// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers -func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) { - if err := repo.LoadOwner(ctx); err != nil { - return nil, err - } - - e := db.GetEngine(ctx) - uniqueUserIDs := make(container.Set[int64]) - - if repo.Owner.IsOrganization() { - additionalUserIDs := make([]int64, 0, 10) - if err := e.Table("team_user"). - Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). - Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). - Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)", - repo.ID, perm.AccessModeRead, unit.TypePullRequests). - Distinct("`team_user`.uid"). - Select("`team_user`.uid"). - Find(&additionalUserIDs); err != nil { - return nil, err - } - uniqueUserIDs.AddMultiple(additionalUserIDs...) - } else { - userIDs := make([]int64, 0, 10) - if err := e.Table("access"). - Where("repo_id = ? AND mode >= ?", repo.ID, perm.AccessModeRead). - Select("user_id"). - Find(&userIDs); err != nil { - return nil, err - } - uniqueUserIDs.AddMultiple(userIDs...) - } - uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers - - // Leave a seat for owner itself to append later, but if owner is an organization - // and just waste 1 unit is cheaper than re-allocate memory once. - users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) - if len(uniqueUserIDs) > 0 { - if err := e.In("id", uniqueUserIDs.Values()). - Where(builder.Eq{"`user`.is_active": true}). - OrderBy(user_model.GetOrderByName()). - Find(&users); err != nil { - return nil, err - } - } - if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) { - users = append(users, repo.Owner) - } - - return users, nil -} - // GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository // If isShowFullName is set to true, also include full name prefix search func GetIssuePostersWithSearch(ctx context.Context, repo *Repository, isPull bool, search string, isShowFullName bool) ([]*user_model.User, error) { diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index d2bf6dc9121c9..f2abc2ffa01b9 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -38,46 +38,3 @@ func TestRepoAssignees(t *testing.T) { assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) } } - -func TestRepoGetReviewers(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - // test public repo - repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - - ctx := db.DefaultContext - reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) - assert.NoError(t, err) - if assert.Len(t, reviewers, 3) { - assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) - } - - // should include doer if doer is not PR poster. - reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) - assert.NoError(t, err) - assert.Len(t, reviewers, 3) - - // should not include PR poster, if PR poster would be otherwise eligible - reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4) - assert.NoError(t, err) - assert.Len(t, reviewers, 2) - - // test private user repo - repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) - - reviewers, err = repo_model.GetReviewers(ctx, repo2, 2, 4) - assert.NoError(t, err) - assert.Len(t, reviewers, 1) - assert.EqualValues(t, reviewers[0].ID, 2) - - // test private org repo - repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - - reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 1) - assert.NoError(t, err) - assert.Len(t, reviewers, 2) - - reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2) - assert.NoError(t, err) - assert.Len(t, reviewers, 1) -} diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 39c9ba527dfae..a4610bd6c265c 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -323,7 +324,7 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) + reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) if err != nil { ctx.Error(http.StatusInternalServerError, "ListCollaborators", err) return diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go new file mode 100644 index 0000000000000..80b5267aee309 --- /dev/null +++ b/services/pull/reviewer.go @@ -0,0 +1,95 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + "fmt" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/perm" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" + + "xorm.io/builder" +) + +// GetReviewers get all users can be requested to review: +// - Poster should not be listed +// - For collaborator, all users that have read access or higher to the repository. +// - For repository under organization, users under the teams which have read permission or higher of pull request unit +// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers +func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, posterID int64) ([]*user_model.User, error) { + if err := repo.LoadOwner(ctx); err != nil { + return nil, err + } + + e := db.GetEngine(ctx) + uniqueUserIDs := make(container.Set[int64]) + + collaboratorIDs := make([]int64, 0, 10) + if err := e.Table("collaboration").Where("repo_id=?", repo.ID). + And("mode >= ?", perm.AccessModeRead). + Select("user_id"). + Find(&collaboratorIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(collaboratorIDs...) + + if repo.Owner.IsOrganization() { + additionalUserIDs := make([]int64, 0, 10) + if err := e.Table("team_user"). + Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). + Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). + Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)", + repo.ID, perm.AccessModeRead, unit.TypePullRequests). + Distinct("`team_user`.uid"). + Select("`team_user`.uid"). + Find(&additionalUserIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(additionalUserIDs...) + + if repo.Owner.Visibility.IsLimited() && doerID == 0 { + return nil, fmt.Errorf("permission denied") + } + + if (repo.IsPrivate || repo.Owner.Visibility.IsPrivate()) && !uniqueUserIDs.Contains(doerID) { + return nil, fmt.Errorf("permission denied") + } + } else { + userIDs := make([]int64, 0, 10) + if err := e.Table("access"). + Where("repo_id = ? AND mode >= ?", repo.ID, perm.AccessModeRead). + Select("user_id"). + Find(&userIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(userIDs...) + if repo.IsPrivate && !uniqueUserIDs.Contains(doerID) && doerID != repo.OwnerID { + return nil, fmt.Errorf("permission denied") + } + } + + uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers + + // Leave a seat for owner itself to append later, but if owner is an organization + // and just waste 1 unit is cheaper than re-allocate memory once. + users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) + if len(uniqueUserIDs) > 0 { + if err := e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { + return nil, err + } + } + if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) { + users = append(users, repo.Owner) + } + + return users, nil +} diff --git a/services/pull/reviewer_test.go b/services/pull/reviewer_test.go new file mode 100644 index 0000000000000..f394923d30db2 --- /dev/null +++ b/services/pull/reviewer_test.go @@ -0,0 +1,58 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + pull_service "code.gitea.io/gitea/services/pull" + + "github.com/stretchr/testify/assert" +) + +func TestRepoGetReviewers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // test public repo + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + ctx := db.DefaultContext + reviewers, err := pull_service.GetReviewers(ctx, repo1, 2, 0) + assert.NoError(t, err) + if assert.Len(t, reviewers, 1) { + assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID}) + } + + // should not include doer and remove the poster + reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 2) + assert.NoError(t, err) + assert.Len(t, reviewers, 0) + + // should not include PR poster, if PR poster would be otherwise eligible + reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 4) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) + + // test private user repo + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + + reviewers, err = pull_service.GetReviewers(ctx, repo2, 2, 4) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) + assert.EqualValues(t, reviewers[0].ID, 2) + + // test private org repo + repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + + reviewers, err = pull_service.GetReviewers(ctx, repo3, 2, 1) + assert.NoError(t, err) + assert.Len(t, reviewers, 2) + + reviewers, err = pull_service.GetReviewers(ctx, repo3, 2, 2) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) +} From bcc78c380e2777fe2d42b75895d3506830934528 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 5 Nov 2024 00:08:31 -0800 Subject: [PATCH 3/7] Move getreviewerteams to pull service --- routers/web/repo/issue.go | 5 ++--- services/pull/reviewer.go | 13 +++++++++++++ services/pull/reviewer_test.go | 14 ++++++++++++++ services/repository/review.go | 24 ------------------------ services/repository/review_test.go | 28 ---------------------------- 5 files changed, 29 insertions(+), 55 deletions(-) delete mode 100644 services/repository/review.go delete mode 100644 services/repository/review_test.go diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 507b5af9d904a..73e66926dd064 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -57,7 +57,6 @@ import ( "code.gitea.io/gitea/services/forms" issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" - repo_service "code.gitea.io/gitea/services/repository" user_service "code.gitea.io/gitea/services/user" ) @@ -699,13 +698,13 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is posterID = 0 } - reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID) + reviewers, err = pull_service.GetReviewers(ctx, repo, ctx.Doer.ID, posterID) if err != nil { ctx.ServerError("GetReviewers", err) return } - teamReviewers, err = repo_service.GetReviewerTeams(ctx, repo) + teamReviewers, err = pull_service.GetReviewerTeams(ctx, repo) if err != nil { ctx.ServerError("GetReviewerTeams", err) return diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index 80b5267aee309..27d127486f4c4 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -8,6 +8,7 @@ import ( "fmt" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -93,3 +94,15 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return users, nil } + +// GetReviewerTeams get all teams can be requested to review +func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) { + if err := repo.LoadOwner(ctx); err != nil { + return nil, err + } + if !repo.Owner.IsOrganization() { + return nil, nil + } + + return organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) +} diff --git a/services/pull/reviewer_test.go b/services/pull/reviewer_test.go index f394923d30db2..1ff373bafb721 100644 --- a/services/pull/reviewer_test.go +++ b/services/pull/reviewer_test.go @@ -56,3 +56,17 @@ func TestRepoGetReviewers(t *testing.T) { assert.NoError(t, err) assert.Len(t, reviewers, 1) } + +func TestRepoGetReviewerTeams(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + teams, err := pull_service.GetReviewerTeams(db.DefaultContext, repo2) + assert.NoError(t, err) + assert.Empty(t, teams) + + repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + teams, err = pull_service.GetReviewerTeams(db.DefaultContext, repo3) + assert.NoError(t, err) + assert.Len(t, teams, 2) +} diff --git a/services/repository/review.go b/services/repository/review.go deleted file mode 100644 index 40513e6bc67ba..0000000000000 --- a/services/repository/review.go +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package repository - -import ( - "context" - - "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - repo_model "code.gitea.io/gitea/models/repo" -) - -// GetReviewerTeams get all teams can be requested to review -func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) { - if err := repo.LoadOwner(ctx); err != nil { - return nil, err - } - if !repo.Owner.IsOrganization() { - return nil, nil - } - - return organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) -} diff --git a/services/repository/review_test.go b/services/repository/review_test.go deleted file mode 100644 index 2db56d4e8a9cc..0000000000000 --- a/services/repository/review_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package repository - -import ( - "testing" - - "code.gitea.io/gitea/models/db" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unittest" - - "github.com/stretchr/testify/assert" -) - -func TestRepoGetReviewerTeams(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) - teams, err := GetReviewerTeams(db.DefaultContext, repo2) - assert.NoError(t, err) - assert.Empty(t, teams) - - repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - teams, err = GetReviewerTeams(db.DefaultContext, repo3) - assert.NoError(t, err) - assert.Len(t, teams, 2) -} From 6fccf000352ab5873accf89928cc1fd7640cc0b3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 5 Nov 2024 21:57:10 -0800 Subject: [PATCH 4/7] Fix test --- tests/integration/api_repo_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 93c9ca0920d49..122afbfa08fb1 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -718,8 +718,8 @@ func TestAPIRepoGetReviewers(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var reviewers []*api.User DecodeJSON(t, resp, &reviewers) - if assert.Len(t, reviewers, 3) { - assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + if assert.Len(t, reviewers, 1) { + assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID}) } } From fc1e61c845765e580b1a07cab467cfc7be48bc31 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 6 Nov 2024 20:49:52 -0800 Subject: [PATCH 5/7] Use GetTeamsWithAccessToRepoUnit when get review teams and add test --- models/organization/team_repo.go | 14 ++++++++++++ models/organization/team_repo_test.go | 31 +++++++++++++++++++++++++++ services/pull/reviewer.go | 2 +- 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 models/organization/team_repo_test.go diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go index 1184e39263635..c90dfdeda04dc 100644 --- a/models/organization/team_repo.go +++ b/models/organization/team_repo.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" "xorm.io/builder" ) @@ -83,3 +84,16 @@ func GetTeamsWithAccessToRepo(ctx context.Context, orgID, repoID int64, mode per OrderBy("name"). Find(&teams) } + +// GetTeamsWithAccessToRepoUnit returns all teams in an organization that have given access level to the repository special unit. +func GetTeamsWithAccessToRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type) ([]*Team, error) { + teams := make([]*Team, 0, 5) + return teams, db.GetEngine(ctx).Where("team_unit.access_mode >= ?", mode). + Join("INNER", "team_repo", "team_repo.team_id = team.id"). + Join("INNER", "team_unit", "team_unit.team_id = team.id"). + And("team_repo.org_id = ?", orgID). + And("team_repo.repo_id = ?", repoID). + And("team_unit.type = ?", unitType). + OrderBy("name"). + Find(&teams) +} diff --git a/models/organization/team_repo_test.go b/models/organization/team_repo_test.go new file mode 100644 index 0000000000000..c0d6750df90cb --- /dev/null +++ b/models/organization/team_repo_test.go @@ -0,0 +1,31 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package organization_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" +) + +func TestGetTeamsWithAccessToRepoUnit(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + org41 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 41}) + repo61 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 61}) + + teams, err := organization.GetTeamsWithAccessToRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests) + assert.NoError(t, err) + if assert.Len(t, teams, 2) { + assert.EqualValues(t, 21, teams[0].ID) + assert.EqualValues(t, 22, teams[1].ID) + } +} diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index 27d127486f4c4..78b60c54a4f8e 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -104,5 +104,5 @@ func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*orga return nil, nil } - return organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) } From 7e073ec81195dba8df11e4e0700d673166bc7530 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 13 Nov 2024 10:02:36 -0800 Subject: [PATCH 6/7] Move permission check to router layer --- routers/api/v1/repo/collaborators.go | 7 +++++++ routers/web/repo/issue_page_meta.go | 2 +- services/issue/assignee.go | 8 ++++---- services/pull/reviewer.go | 12 ------------ 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 126fa57774458..0bbf5a1ea49e8 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" + issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -321,6 +322,12 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" + canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) + if !canChooseReviewer { + ctx.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers")) + return + } + reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) if err != nil { ctx.Error(http.StatusInternalServerError, "ListCollaborators", err) diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index 84cefc437adb0..e04d76b287d8f 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -186,7 +186,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { if d.Issue == nil { data.CanChooseReviewer = true } else { - data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue) + data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) } } diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 52ee9f2b22cbf..c7e24955687f9 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -119,7 +119,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return err } - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) if isAdd { if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { @@ -178,7 +178,7 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } } - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) if isAdd { if issue.Repo.IsPrivate { @@ -276,12 +276,12 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe } // CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR -func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue) bool { +func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { if repo.IsArchived { return false } // The poster of the PR can change the reviewers - if doer.ID == issue.PosterID { + if doer.ID == posterID { return true } diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index 78b60c54a4f8e..1e673f4b77493 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -5,7 +5,6 @@ package pull import ( "context" - "fmt" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" @@ -53,14 +52,6 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return nil, err } uniqueUserIDs.AddMultiple(additionalUserIDs...) - - if repo.Owner.Visibility.IsLimited() && doerID == 0 { - return nil, fmt.Errorf("permission denied") - } - - if (repo.IsPrivate || repo.Owner.Visibility.IsPrivate()) && !uniqueUserIDs.Contains(doerID) { - return nil, fmt.Errorf("permission denied") - } } else { userIDs := make([]int64, 0, 10) if err := e.Table("access"). @@ -70,9 +61,6 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return nil, err } uniqueUserIDs.AddMultiple(userIDs...) - if repo.IsPrivate && !uniqueUserIDs.Contains(doerID) && doerID != repo.OwnerID { - return nil, fmt.Errorf("permission denied") - } } uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers From d5a0c12f9de6c0ae46250acbcb21cbc2d48896a2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 21 Nov 2024 11:24:19 -0800 Subject: [PATCH 7/7] Remove unnecessary code and add some comments --- services/pull/reviewer.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index 1e673f4b77493..bf0d8cb298c8b 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -52,15 +52,6 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return nil, err } uniqueUserIDs.AddMultiple(additionalUserIDs...) - } else { - userIDs := make([]int64, 0, 10) - if err := e.Table("access"). - Where("repo_id = ? AND mode >= ?", repo.ID, perm.AccessModeRead). - Select("user_id"). - Find(&userIDs); err != nil { - return nil, err - } - uniqueUserIDs.AddMultiple(userIDs...) } uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers @@ -76,6 +67,8 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return nil, err } } + + // add owner after all users are loaded because we can avoid load owner twice if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) { users = append(users, repo.Owner) }