diff options
33 files changed, 656 insertions, 103 deletions
diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 1bb6a9a8ac..641c453eb7 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -135,3 +135,27 @@ user_id: 31 repo_id: 28 mode: 4 + +- + id: 24 + user_id: 38 + repo_id: 60 + mode: 2 + +- + id: 25 + user_id: 38 + repo_id: 61 + mode: 1 + +- + id: 26 + user_id: 39 + repo_id: 61 + mode: 1 + +- + id: 27 + user_id: 40 + repo_id: 61 + mode: 4 diff --git a/models/fixtures/collaboration.yml b/models/fixtures/collaboration.yml index ef77d22b24..7603bdad32 100644 --- a/models/fixtures/collaboration.yml +++ b/models/fixtures/collaboration.yml @@ -45,3 +45,9 @@ repo_id: 22 user_id: 18 mode: 2 # write + +- + id: 9 + repo_id: 60 + user_id: 38 + mode: 2 # write diff --git a/models/fixtures/email_address.yml b/models/fixtures/email_address.yml index 67a99f43e2..b2a0432635 100644 --- a/models/fixtures/email_address.yml +++ b/models/fixtures/email_address.yml @@ -293,3 +293,27 @@ lower_email: user37@example.com is_activated: true is_primary: true + +- + id: 38 + uid: 38 + email: user38@example.com + lower_email: user38@example.com + is_activated: true + is_primary: true + +- + id: 39 + uid: 39 + email: user39@example.com + lower_email: user39@example.com + is_activated: true + is_primary: true + +- + id: 40 + uid: 40 + email: user40@example.com + lower_email: user40@example.com + is_activated: true + is_primary: true diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index 0c9b6ff406..ca5b1c6cd1 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -338,3 +338,37 @@ created_unix: 978307210 updated_unix: 978307210 is_locked: false + +- + id: 21 + repo_id: 60 + index: 1 + poster_id: 39 + original_author_id: 0 + name: repo60 pull1 + content: content for the 1st issue + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 1707270422 + updated_unix: 1707270422 + is_locked: false + +- + id: 22 + repo_id: 61 + index: 1 + poster_id: 40 + original_author_id: 0 + name: repo61 pull1 + content: content for the 1st issue + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 1707270422 + updated_unix: 1707270422 + is_locked: false diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index 8d58169a32..a7fbcb2c5a 100644 --- a/models/fixtures/org_user.yml +++ b/models/fixtures/org_user.yml @@ -99,3 +99,21 @@ uid: 5 org_id: 36 is_public: true + +- + id: 18 + uid: 38 + org_id: 41 + is_public: true + +- + id: 19 + uid: 39 + org_id: 41 + is_public: true + +- + id: 20 + uid: 40 + org_id: 41 + is_public: true diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml index 54590fb830..3fc8ce630d 100644 --- a/models/fixtures/pull_request.yml +++ b/models/fixtures/pull_request.yml @@ -99,3 +99,21 @@ index: 1 head_repo_id: 23 base_repo_id: 23 + +- + id: 9 + type: 0 # gitea pull request + status: 2 # mergable + issue_id: 21 + index: 1 + head_repo_id: 60 + base_repo_id: 60 + +- + id: 10 + type: 0 # gitea pull request + status: 2 # mergable + issue_id: 22 + index: 1 + head_repo_id: 61 + base_repo_id: 61 diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index e6c59f527a..4b26674990 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -676,3 +676,45 @@ type: 1 config: "{}" created_unix: 946684810 + +- + id: 102 + repo_id: 60 + type: 1 + config: "{}" + created_unix: 946684810 + +- + id: 103 + repo_id: 60 + type: 2 + config: "{\"EnableTimetracker\":true,\"AllowOnlyContributorsToTrackTime\":true}" + created_unix: 946684810 + +- + id: 104 + repo_id: 60 + type: 3 + config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowRebaseMerge\":true,\"AllowSquash\":true}" + created_unix: 946684810 + +- + id: 105 + repo_id: 61 + type: 1 + config: "{}" + created_unix: 946684810 + +- + id: 106 + repo_id: 61 + type: 2 + config: "{\"EnableTimetracker\":true,\"AllowOnlyContributorsToTrackTime\":true}" + created_unix: 946684810 + +- + id: 107 + repo_id: 61 + type: 3 + config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowRebaseMerge\":true,\"AllowSquash\":true}" + created_unix: 946684810 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index f4e8376735..d094fe82d8 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -1706,3 +1706,65 @@ is_private: true status: 0 num_issues: 0 + +- + id: 60 + owner_id: 40 + owner_name: user40 + lower_name: repo60 + name: repo60 + default_branch: main + num_watches: 0 + num_stars: 0 + num_forks: 0 + num_issues: 0 + num_closed_issues: 0 + num_pulls: 1 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_projects: 0 + num_closed_projects: 0 + is_private: false + is_empty: false + is_archived: false + is_mirror: false + status: 0 + is_fork: false + fork_id: 0 + is_template: false + template_id: 0 + size: 0 + is_fsck_enabled: true + close_issues_via_commit_in_any_branch: false + +- + id: 61 + owner_id: 41 + owner_name: org41 + lower_name: repo61 + name: repo61 + default_branch: main + num_watches: 0 + num_stars: 0 + num_forks: 0 + num_issues: 0 + num_closed_issues: 0 + num_pulls: 1 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_projects: 0 + num_closed_projects: 0 + is_private: false + is_empty: false + is_archived: false + is_mirror: false + status: 0 + is_fork: false + fork_id: 0 + is_template: false + template_id: 0 + size: 0 + is_fsck_enabled: true + close_issues_via_commit_in_any_branch: false diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index 295e51e39c..149fe90888 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -217,3 +217,25 @@ num_members: 1 includes_all_repositories: false can_create_org_repo: true + +- + id: 21 + org_id: 41 + lower_name: owners + name: Owners + authorize: 4 # owner + num_repos: 1 + num_members: 1 + includes_all_repositories: true + can_create_org_repo: true + +- + id: 22 + org_id: 41 + lower_name: team1 + name: Team1 + authorize: 1 # read + num_repos: 1 + num_members: 2 + includes_all_repositories: false + can_create_org_repo: false diff --git a/models/fixtures/team_repo.yml b/models/fixtures/team_repo.yml index 8497720892..a29078107e 100644 --- a/models/fixtures/team_repo.yml +++ b/models/fixtures/team_repo.yml @@ -63,3 +63,15 @@ org_id: 17 team_id: 9 repo_id: 24 + +- + id: 12 + org_id: 41 + team_id: 21 + repo_id: 61 + +- + id: 13 + org_id: 41 + team_id: 22 + repo_id: 61 diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index c5531aa57a..de0e8d738b 100644 --- a/models/fixtures/team_unit.yml +++ b/models/fixtures/team_unit.yml @@ -286,3 +286,39 @@ team_id: 2 type: 8 access_mode: 2 + +- + id: 49 + team_id: 21 + type: 1 + access_mode: 4 + +- + id: 50 + team_id: 21 + type: 2 + access_mode: 4 + +- + id: 51 + team_id: 21 + type: 3 + access_mode: 4 + +- + id: 52 + team_id: 22 + type: 1 + access_mode: 1 + +- + id: 53 + team_id: 22 + type: 2 + access_mode: 1 + +- + id: 54 + team_id: 22 + type: 3 + access_mode: 1 diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index 9142fe609a..02d57ae644 100644 --- a/models/fixtures/team_user.yml +++ b/models/fixtures/team_user.yml @@ -129,3 +129,21 @@ org_id: 17 team_id: 9 uid: 15 + +- + id: 23 + org_id: 41 + team_id: 21 + uid: 40 + +- + id: 24 + org_id: 41 + team_id: 22 + uid: 38 + +- + id: 25 + org_id: 41 + team_id: 22 + uid: 39 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index aa0daedd85..16b687ae04 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -1369,3 +1369,151 @@ repo_admin_change_team_access: false theme: "" keep_activity_private: false + +- + id: 38 + lower_name: user38 + name: user38 + full_name: User38 + email: user38@example.com + keep_email_private: false + email_notifications_preference: enabled + passwd: ZogKvWdyEx:password + passwd_hash_algo: dummy + must_change_password: false + login_source: 0 + login_name: user38 + type: 0 + salt: ZogKvWdyEx + max_repo_creation: -1 + is_active: true + is_admin: false + is_restricted: false + allow_git_hook: false + allow_import_local: false + allow_create_organization: true + prohibit_login: false + avatar: avatar38 + avatar_email: user38@example.com + use_custom_avatar: false + num_followers: 0 + num_following: 0 + num_stars: 0 + num_repos: 0 + num_teams: 0 + num_members: 0 + visibility: 0 + repo_admin_change_team_access: false + theme: "" + keep_activity_private: false + +- + id: 39 + lower_name: user39 + name: user39 + full_name: User39 + email: user39@example.com + keep_email_private: false + email_notifications_preference: enabled + passwd: ZogKvWdyEx:password + passwd_hash_algo: dummy + must_change_password: false + login_source: 0 + login_name: user39 + type: 0 + salt: ZogKvWdyEx + max_repo_creation: -1 + is_active: true + is_admin: false + is_restricted: false + allow_git_hook: false + allow_import_local: false + allow_create_organization: true + prohibit_login: false + avatar: avatar39 + avatar_email: user39@example.com + use_custom_avatar: false + num_followers: 0 + num_following: 0 + num_stars: 0 + num_repos: 0 + num_teams: 0 + num_members: 0 + visibility: 0 + repo_admin_change_team_access: false + theme: "" + keep_activity_private: false + +- + id: 40 + lower_name: user40 + name: user40 + full_name: User40 + email: user40@example.com + keep_email_private: false + email_notifications_preference: onmention + passwd: ZogKvWdyEx:password + passwd_hash_algo: dummy + must_change_password: false + login_source: 0 + login_name: user40 + type: 0 + salt: ZogKvWdyEx + max_repo_creation: -1 + is_active: true + is_admin: false + is_restricted: false + allow_git_hook: false + allow_import_local: false + allow_create_organization: true + prohibit_login: false + avatar: avatar40 + avatar_email: user40@example.com + use_custom_avatar: false + num_followers: 0 + num_following: 0 + num_stars: 0 + num_repos: 1 + num_teams: 0 + num_members: 0 + visibility: 0 + repo_admin_change_team_access: false + theme: "" + keep_activity_private: false + +- + id: 41 + lower_name: org41 + name: org41 + full_name: Org41 + email: org41@example.com + keep_email_private: false + email_notifications_preference: onmention + passwd: ZogKvWdyEx:password + passwd_hash_algo: dummy + must_change_password: false + login_source: 0 + login_name: org41 + type: 1 + salt: ZogKvWdyEx + max_repo_creation: -1 + is_active: false + is_admin: false + is_restricted: false + allow_git_hook: false + allow_import_local: false + allow_create_organization: true + prohibit_login: false + avatar: avatar41 + avatar_email: org41@example.com + use_custom_avatar: false + num_followers: 0 + num_following: 0 + num_stars: 0 + num_repos: 1 + num_teams: 2 + num_members: 3 + visibility: 0 + repo_admin_change_team_access: false + theme: "" + keep_activity_private: false diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index cc363d2fae..1bbc0eee56 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -379,7 +379,7 @@ func TestCountIssues(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) count, err := issues_model.CountIssues(db.DefaultContext, &issues_model.IssuesOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 20, count) + assert.EqualValues(t, 22, count) } func TestIssueLoadAttributes(t *testing.T) { diff --git a/models/repo/repo_list_test.go b/models/repo/repo_list_test.go index 8a1799aac0..83e37a27fd 100644 --- a/models/repo/repo_list_test.go +++ b/models/repo/repo_list_test.go @@ -138,12 +138,12 @@ func getTestCases() []struct { { name: "AllPublic/PublicRepositoriesOfUserIncludingCollaborative", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: util.OptionalBoolFalse}, - count: 31, + count: 33, }, { name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: util.OptionalBoolFalse}, - count: 36, + count: 38, }, { name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", @@ -158,7 +158,7 @@ func getTestCases() []struct { { name: "AllPublic/PublicRepositoriesOfOrganization", opts: &repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: util.OptionalBoolFalse, Template: util.OptionalBoolFalse}, - count: 31, + count: 33, }, { name: "AllTemplates", diff --git a/models/user/user_test.go b/models/user/user_test.go index f3e5a95b1e..68cee9cdbd 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -89,7 +89,7 @@ func TestSearchUsers(t *testing.T) { []int64{19, 25}) testOrgSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 4, PageSize: 2}}, - []int64{26}) + []int64{26, 41}) testOrgSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 5, PageSize: 2}}, []int64{}) @@ -101,13 +101,13 @@ func TestSearchUsers(t *testing.T) { } testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}}, - []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37}) + []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40}) testUserSuccess(&user_model.SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsActive: util.OptionalBoolFalse}, []int64{9}) testUserSuccess(&user_model.SearchUserOptions{OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: util.OptionalBoolTrue}, - []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37}) + []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30, 32, 34, 37, 38, 39, 40}) testUserSuccess(&user_model.SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: db.ListOptions{Page: 1}, IsActive: util.OptionalBoolTrue}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) diff --git a/modules/indexer/issues/indexer_test.go b/modules/indexer/issues/indexer_test.go index da4fc9b878..3b96686d98 100644 --- a/modules/indexer/issues/indexer_test.go +++ b/modules/indexer/issues/indexer_test.go @@ -218,7 +218,7 @@ func searchIssueIsPull(t *testing.T) { SearchOptions{ IsPull: util.OptionalBoolTrue, }, - []int64{12, 11, 20, 19, 9, 8, 3, 2}, + []int64{22, 21, 12, 11, 20, 19, 9, 8, 3, 2}, }, } for _, test := range tests { @@ -239,7 +239,7 @@ func searchIssueIsClosed(t *testing.T) { SearchOptions{ IsClosed: util.OptionalBoolFalse, }, - []int64{17, 16, 15, 14, 13, 12, 11, 20, 6, 19, 18, 10, 7, 9, 8, 3, 2, 1}, + []int64{22, 21, 17, 16, 15, 14, 13, 12, 11, 20, 6, 19, 18, 10, 7, 9, 8, 3, 2, 1}, }, { SearchOptions{ @@ -305,7 +305,7 @@ func searchIssueByLabelID(t *testing.T) { SearchOptions{ ExcludedLabelIDs: []int64{1}, }, - []int64{17, 16, 15, 14, 13, 12, 11, 20, 6, 5, 19, 18, 10, 7, 4, 9, 8, 3}, + []int64{22, 21, 17, 16, 15, 14, 13, 12, 11, 20, 6, 5, 19, 18, 10, 7, 4, 9, 8, 3}, }, } for _, test := range tests { @@ -329,7 +329,7 @@ func searchIssueByTime(t *testing.T) { SearchOptions{ UpdatedAfterUnix: int64Pointer(0), }, - []int64{17, 16, 15, 14, 13, 12, 11, 20, 6, 5, 19, 18, 10, 7, 4, 9, 8, 3, 2, 1}, + []int64{22, 21, 17, 16, 15, 14, 13, 12, 11, 20, 6, 5, 19, 18, 10, 7, 4, 9, 8, 3, 2, 1}, }, } for _, test := range tests { @@ -350,7 +350,7 @@ func searchIssueWithOrder(t *testing.T) { SearchOptions{ SortBy: internal.SortByCreatedAsc, }, - []int64{1, 2, 3, 8, 9, 4, 7, 10, 18, 19, 5, 6, 20, 11, 12, 13, 14, 15, 16, 17}, + []int64{1, 2, 3, 8, 9, 4, 7, 10, 18, 19, 5, 6, 20, 11, 12, 13, 14, 15, 16, 17, 21, 22}, }, } for _, test := range tests { @@ -410,8 +410,8 @@ func searchIssueWithPaginator(t *testing.T) { PageSize: 5, }, }, - []int64{17, 16, 15, 14, 13}, - 20, + []int64{22, 21, 17, 16, 15}, + 22, }, } for _, test := range tests { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index d5e49960a1..9f08607642 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -711,16 +711,12 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is tmp.ItemID = -review.ReviewerTeamID } - if ctx.Repo.IsAdmin() { - // Admin can dismiss or re-request any review requests + if canChooseReviewer { + // Users who can choose reviewers can also remove review requests tmp.CanChange = true } else if ctx.Doer != nil && ctx.Doer.ID == review.ReviewerID && review.Type == issues_model.ReviewTypeRequest { // A user can refuse review requests tmp.CanChange = true - } else if (canChooseReviewer || (ctx.Doer != nil && ctx.Doer.ID == issue.PosterID)) && review.Type != issues_model.ReviewTypeRequest && - ctx.Doer.ID != review.ReviewerID { - // The poster of the PR, a manager, or official reviewers can re-request review from other reviewers - tmp.CanChange = true } pullReviews = append(pullReviews, tmp) @@ -1525,18 +1521,9 @@ func ViewIssue(ctx *context.Context) { } if issue.IsPull { - canChooseReviewer := ctx.Repo.CanWrite(unit.TypePullRequests) + canChooseReviewer := false if ctx.Doer != nil && ctx.IsSigned { - if !canChooseReviewer { - canChooseReviewer = ctx.Doer.ID == issue.PosterID - } - if !canChooseReviewer { - canChooseReviewer, err = issues_model.IsOfficialReviewer(ctx, issue, ctx.Doer) - if err != nil { - ctx.ServerError("IsOfficialReviewer", err) - return - } - } + canChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue) } RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer) diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 27fc695533..b5f472ba53 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" + 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/log" @@ -113,10 +114,10 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return err } - var pemResult bool + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) + if isAdd { - pemResult = permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) - if !pemResult { + if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { return issues_model.ErrNotValidReviewRequest{ Reason: "Reviewer can't read", UserID: doer.ID, @@ -124,28 +125,6 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } - if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != issues_model.ReviewTypeRequest { - return nil - } - - pemResult = doer.ID == issue.PosterID - if !pemResult { - pemResult = permDoer.CanAccessAny(perm.AccessModeWrite, unit.TypePullRequests) - } - if !pemResult { - pemResult, err = issues_model.IsOfficialReviewer(ctx, issue, doer) - if err != nil { - return err - } - if !pemResult { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - } - if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { return issues_model.ErrNotValidReviewRequest{ Reason: "poster of pr can't be reviewer", @@ -153,22 +132,35 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, RepoID: issue.Repo.ID, } } - } else { - if lastreview != nil && lastreview.Type == issues_model.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { + + if canDoerChangeReviewRequests { return nil } - pemResult = permDoer.IsAdmin() - if !pemResult { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer is not admin", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } + if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != issues_model.ReviewTypeRequest { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't choose reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, } } - return nil + if canDoerChangeReviewRequests { + return nil + } + + if lastreview != nil && lastreview.Type == issues_model.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } } // IsValidTeamReviewRequest Check permission for ReviewRequest Team @@ -181,11 +173,7 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } } - permission, err := access_model.GetUserRepoPermission(ctx, issue.Repo, doer) - if err != nil { - log.Error("Unable to GetUserRepoPermission for %-v in %-v#%d", doer, issue.Repo, issue.Index) - return err - } + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) if isAdd { if issue.Repo.IsPrivate { @@ -200,30 +188,26 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } } - doerCanWrite := permission.CanAccessAny(perm.AccessModeWrite, unit.TypePullRequests) - if !doerCanWrite && doer.ID != issue.PosterID { - official, err := issues_model.IsOfficialReviewer(ctx, issue, doer) - if err != nil { - log.Error("Unable to Check if IsOfficialReviewer for %-v in %-v#%d", doer, issue.Repo, issue.Index) - return err - } - if !official { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } + if canDoerChangeReviewRequests { + return nil } - } else if !permission.IsAdmin() { + return issues_model.ErrNotValidReviewRequest{ - Reason: "Only admin users can remove team requests. Doer is not admin", + Reason: "Doer can't choose reviewer", UserID: doer.ID, RepoID: issue.Repo.ID, } } - return nil + if canDoerChangeReviewRequests { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } } // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. @@ -264,3 +248,50 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use return comment, err } + +// 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 { + // The poster of the PR can change the reviewers + if doer.ID == issue.PosterID { + return true + } + + // The owner of the repo can change the reviewers + if doer.ID == repo.OwnerID { + return true + } + + // Collaborators of the repo can change the reviewers + isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, doer.ID) + if err != nil { + log.Error("IsCollaborator: %v", err) + return false + } + if isCollaborator { + return true + } + + // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers + if repo.Owner.IsOrganization() { + teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + if err != nil { + log.Error("GetTeamsWithAccessToRepo: %v", err) + return false + } + for _, team := range teams { + if !team.UnitEnabled(ctx, unit.TypePullRequests) { + continue + } + isMember, err := organization.IsTeamMember(ctx, repo.OwnerID, team.ID, doer.ID) + if err != nil { + log.Error("IsTeamMember: %v", err) + continue + } + if isMember { + return true + } + } + } + + return false +} diff --git a/tests/gitea-repositories-meta/org41/repo61.git/HEAD b/tests/gitea-repositories-meta/org41/repo61.git/HEAD new file mode 100644 index 0000000000..cb089cd89a --- /dev/null +++ b/tests/gitea-repositories-meta/org41/repo61.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/master diff --git a/tests/gitea-repositories-meta/org41/repo61.git/config b/tests/gitea-repositories-meta/org41/repo61.git/config new file mode 100644 index 0000000000..64280b806c --- /dev/null +++ b/tests/gitea-repositories-meta/org41/repo61.git/config @@ -0,0 +1,6 @@ +[core] + repositoryformatversion = 0 + filemode = false + bare = true + symlinks = false + ignorecase = true diff --git a/tests/gitea-repositories-meta/org41/repo61.git/description b/tests/gitea-repositories-meta/org41/repo61.git/description new file mode 100644 index 0000000000..498b267a8c --- /dev/null +++ b/tests/gitea-repositories-meta/org41/repo61.git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/tests/gitea-repositories-meta/org41/repo61.git/info/exclude b/tests/gitea-repositories-meta/org41/repo61.git/info/exclude new file mode 100644 index 0000000000..a5196d1be8 --- /dev/null +++ b/tests/gitea-repositories-meta/org41/repo61.git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/tests/gitea-repositories-meta/user40/repo60.git/HEAD b/tests/gitea-repositories-meta/user40/repo60.git/HEAD new file mode 100644 index 0000000000..cb089cd89a --- /dev/null +++ b/tests/gitea-repositories-meta/user40/repo60.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/master diff --git a/tests/gitea-repositories-meta/user40/repo60.git/config b/tests/gitea-repositories-meta/user40/repo60.git/config new file mode 100644 index 0000000000..64280b806c --- /dev/null +++ b/tests/gitea-repositories-meta/user40/repo60.git/config @@ -0,0 +1,6 @@ +[core] + repositoryformatversion = 0 + filemode = false + bare = true + symlinks = false + ignorecase = true diff --git a/tests/gitea-repositories-meta/user40/repo60.git/description b/tests/gitea-repositories-meta/user40/repo60.git/description new file mode 100644 index 0000000000..498b267a8c --- /dev/null +++ b/tests/gitea-repositories-meta/user40/repo60.git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/tests/gitea-repositories-meta/user40/repo60.git/info/exclude b/tests/gitea-repositories-meta/user40/repo60.git/info/exclude new file mode 100644 index 0000000000..a5196d1be8 --- /dev/null +++ b/tests/gitea-repositories-meta/user40/repo60.git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index f025806868..650bac2e32 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -217,7 +217,7 @@ func TestAPISearchIssues(t *testing.T) { defer tests.PrepareTestEnv(t)() // as this API was used in the frontend, it uses UI page size - expectedIssueCount := 18 // from the fixtures + expectedIssueCount := 20 // from the fixtures if expectedIssueCount > setting.UI.IssuePagingNum { expectedIssueCount = setting.UI.IssuePagingNum } @@ -257,7 +257,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()).AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.EqualValues(t, "20", resp.Header().Get("X-Total-Count")) + assert.EqualValues(t, "22", resp.Header().Get("X-Total-Count")) assert.Len(t, apiIssues, 20) query.Add("limit", "10") @@ -265,7 +265,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()).AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.EqualValues(t, "20", resp.Header().Get("X-Total-Count")) + assert.EqualValues(t, "22", resp.Header().Get("X-Total-Count")) assert.Len(t, apiIssues, 10) query = url.Values{"assigned": {"true"}, "state": {"all"}} @@ -315,7 +315,7 @@ func TestAPISearchIssuesWithLabels(t *testing.T) { defer tests.PrepareTestEnv(t)() // as this API was used in the frontend, it uses UI page size - expectedIssueCount := 18 // from the fixtures + expectedIssueCount := 20 // from the fixtures if expectedIssueCount > setting.UI.IssuePagingNum { expectedIssueCount = setting.UI.IssuePagingNum } diff --git a/tests/integration/api_nodeinfo_test.go b/tests/integration/api_nodeinfo_test.go index 876fb5ac13..75f8dbb4ba 100644 --- a/tests/integration/api_nodeinfo_test.go +++ b/tests/integration/api_nodeinfo_test.go @@ -32,8 +32,8 @@ func TestNodeinfo(t *testing.T) { DecodeJSON(t, resp, &nodeinfo) assert.True(t, nodeinfo.OpenRegistrations) assert.Equal(t, "gitea", nodeinfo.Software.Name) - assert.Equal(t, 26, nodeinfo.Usage.Users.Total) - assert.Equal(t, 20, nodeinfo.Usage.LocalPosts) + assert.Equal(t, 29, nodeinfo.Usage.Users.Total) + assert.Equal(t, 22, nodeinfo.Usage.LocalPosts) assert.Equal(t, 3, nodeinfo.Usage.LocalComments) }) } diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index 1cd82fe4e0..70d3a446f7 100644 --- a/tests/integration/api_org_test.go +++ b/tests/integration/api_org_test.go @@ -177,7 +177,7 @@ func TestAPIGetAll(t *testing.T) { var apiOrgList []*api.Organization DecodeJSON(t, resp, &apiOrgList) - assert.Len(t, apiOrgList, 11) + assert.Len(t, apiOrgList, 12) assert.Equal(t, "Limited Org 36", apiOrgList[1].FullName) assert.Equal(t, "limited", apiOrgList[1].Visibility) @@ -186,7 +186,7 @@ func TestAPIGetAll(t *testing.T) { resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiOrgList) - assert.Len(t, apiOrgList, 7) + assert.Len(t, apiOrgList, 8) assert.Equal(t, "org 17", apiOrgList[0].FullName) assert.Equal(t, "public", apiOrgList[0].Visibility) } diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index ab6d33cd5b..bc544a30b5 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -279,6 +279,49 @@ func TestAPIPullReviewRequest(t *testing.T) { }).AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) + // a collaborator can add/remove a review request + pullIssue21 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 21}) + assert.NoError(t, pullIssue21.LoadAttributes(db.DefaultContext)) + pull21Repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: pullIssue21.RepoID}) // repo60 + user38Session := loginUser(t, "user38") + user38Token := getTokenForLoggedInUser(t, user38Session, auth_model.AccessTokenScopeWriteRepository) + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user4@example.com"}, + }).AddTokenAuth(user38Token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user4@example.com"}, + }).AddTokenAuth(user38Token) + MakeRequest(t, req, http.StatusNoContent) + + // the poster of the PR can add/remove a review request + user39Session := loginUser(t, "user39") + user39Token := getTokenForLoggedInUser(t, user39Session, auth_model.AccessTokenScopeWriteRepository) + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user8"}, + }).AddTokenAuth(user39Token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user8"}, + }).AddTokenAuth(user39Token) + MakeRequest(t, req, http.StatusNoContent) + + // user with read permission on pull requests unit can add/remove a review request + pullIssue22 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 22}) + assert.NoError(t, pullIssue22.LoadAttributes(db.DefaultContext)) + pull22Repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: pullIssue22.RepoID}) // repo61 + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull22Repo.OwnerName, pull22Repo.Name, pullIssue22.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user38"}, + }).AddTokenAuth(user39Token) // user39 is from a team with read permission on pull requests unit + MakeRequest(t, req, http.StatusCreated) + + req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull22Repo.OwnerName, pull22Repo.Name, pullIssue22.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user38"}, + }).AddTokenAuth(user39Token) // user39 is from a team with read permission on pull requests unit + MakeRequest(t, req, http.StatusNoContent) + // Test team review request pullIssue12 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12}) assert.NoError(t, pullIssue12.LoadAttributes(db.DefaultContext)) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 90f84c794e..481732f8df 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -93,9 +93,9 @@ func TestAPISearchRepo(t *testing.T) { }{ { name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{ - nil: {count: 33}, - user: {count: 33}, - user2: {count: 33}, + nil: {count: 35}, + user: {count: 35}, + user2: {count: 35}, }, }, { diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 4b3f581c2b..44d362d9c7 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -407,7 +407,7 @@ func TestSearchIssues(t *testing.T) { session := loginUser(t, "user2") - expectedIssueCount := 18 // from the fixtures + expectedIssueCount := 20 // from the fixtures if expectedIssueCount > setting.UI.IssuePagingNum { expectedIssueCount = setting.UI.IssuePagingNum } @@ -444,7 +444,7 @@ func TestSearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.EqualValues(t, "20", resp.Header().Get("X-Total-Count")) + assert.EqualValues(t, "22", resp.Header().Get("X-Total-Count")) assert.Len(t, apiIssues, 20) query.Add("limit", "5") @@ -452,7 +452,7 @@ func TestSearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.EqualValues(t, "20", resp.Header().Get("X-Total-Count")) + assert.EqualValues(t, "22", resp.Header().Get("X-Total-Count")) assert.Len(t, apiIssues, 5) query = url.Values{"assigned": {"true"}, "state": {"all"}} @@ -501,7 +501,7 @@ func TestSearchIssues(t *testing.T) { func TestSearchIssuesWithLabels(t *testing.T) { defer tests.PrepareTestEnv(t)() - expectedIssueCount := 18 // from the fixtures + expectedIssueCount := 20 // from the fixtures if expectedIssueCount > setting.UI.IssuePagingNum { expectedIssueCount = setting.UI.IssuePagingNum } |