diff options
| -rw-r--r-- | models/organization/team_repo.go | 14 | ||||
| -rw-r--r-- | models/organization/team_repo_test.go | 31 | ||||
| -rw-r--r-- | models/repo/user_repo.go | 52 | ||||
| -rw-r--r-- | models/repo/user_repo_test.go | 43 | ||||
| -rw-r--r-- | routers/api/v1/repo/collaborators.go | 10 | ||||
| -rw-r--r-- | routers/web/repo/issue.go | 7 | ||||
| -rw-r--r-- | services/issue/assignee.go | 11 | ||||
| -rw-r--r-- | services/pull/reviewer.go | 89 | ||||
| -rw-r--r-- | services/pull/reviewer_test.go | 72 | ||||
| -rw-r--r-- | services/repository/review.go | 24 | ||||
| -rw-r--r-- | services/repository/review_test.go | 28 | ||||
| -rw-r--r-- | tests/integration/api_repo_test.go | 4 |
12 files changed, 227 insertions, 158 deletions
diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go index 1184e39263..c90dfdeda0 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 0000000000..c0d6750df9 --- /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/models/repo/user_repo.go b/models/repo/user_repo.go index ecc9216950..a9b1360df1 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" ) @@ -146,57 +145,6 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us return users, nil } -// 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. -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}), - ), - )) - - 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}) - } - } 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) -} - // 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 d2bf6dc912..f2abc2ffa0 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 4ce14f7d01..74be5688ba 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -18,6 +18,8 @@ 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" ) @@ -323,7 +325,13 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) + 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) return diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 47333396c7..3fdf594045 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -56,7 +56,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" ) @@ -693,13 +692,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 @@ -1536,7 +1535,7 @@ func ViewIssue(ctx *context.Context) { if issue.IsPull { canChooseReviewer := false if ctx.Doer != nil && ctx.IsSigned { - canChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue) + canChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue.PosterID) } RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer) diff --git a/services/issue/assignee.go b/services/issue/assignee.go index a0aa5a339b..352ea6fe58 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -114,7 +114,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) { @@ -173,7 +173,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 { @@ -267,9 +267,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 new file mode 100644 index 0000000000..bf0d8cb298 --- /dev/null +++ b/services/pull/reviewer.go @@ -0,0 +1,89 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + "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" + 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...) + } + + 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 + } + } + + // 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) + } + + 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.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) +} diff --git a/services/pull/reviewer_test.go b/services/pull/reviewer_test.go new file mode 100644 index 0000000000..1ff373bafb --- /dev/null +++ b/services/pull/reviewer_test.go @@ -0,0 +1,72 @@ +// 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) +} + +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 40513e6bc6..0000000000 --- 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 2db56d4e8a..0000000000 --- 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) -} diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 716da762e5..732d5fc094 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}) } } |