Allow non-admin users to delete review requests (#29057)

Fix #14459

The following users can add/remove review requests of a PR
- the poster of the PR
- the owner or collaborators of the repository
- members with read permission on the pull requests unit

(cherry picked from commit c42083a33950be6ee9f822c6d0de3c3a79d1f51b)

Conflicts:
	models/repo/repo_list_test.go
	tests/integration/api_nodeinfo_test.go
	tests/integration/api_repo_test.go
	shared fixture counts

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

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

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

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

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

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

models/fixtures/repo_unit.yml

@@ -708,3 +708,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

models/fixtures/repository.yml

@@ -1720,3 +1720,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

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

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

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

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

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

models/issues/issue_test.go

@@ -381,7 +381,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) {

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: 32,
+			count: 34,
 		},
 		{
 			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: 37,
+			count: 39,
 		},
 		{
 			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: 32,
+			count: 34,
 		},
 		{
 			name:  "AllTemplates",

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})

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},
+			[]int64{22, 21, 17, 16, 15},
-			20,
+			22,
 		},
 	}
 	for _, test := range tests {

routers/web/repo/issue.go

@@ -717,16 +717,12 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 			tmp.ItemID = -review.ReviewerTeamID
 		}
 
-		if ctx.Repo.IsAdmin() {
+		if canChooseReviewer {
-			// Admin can dismiss or re-request any review requests
+			// 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)
@@ -1534,18 +1530,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 = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, issue)
-				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
-				}
-			}
 		}
 
 		RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer)

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 !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) {
-		if !pemResult {
 			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,24 +132,37 @@ 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
+		}
+
+		if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != issues_model.ReviewTypeRequest {
 			return nil
 		}
 
-		pemResult = permDoer.IsAdmin()
-		if !pemResult {
 		return issues_model.ErrNotValidReviewRequest{
-				Reason: "Doer is not admin",
+			Reason: "Doer can't choose reviewer",
 			UserID: doer.ID,
 			RepoID: issue.Repo.ID,
 		}
 	}
+
+	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
 func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error {
 	if doer.IsOrganization() {
@@ -181,11 +173,7 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
 		}
 	}
 
-	permission, err := access_model.GetUserRepoPermission(ctx, issue.Repo, doer)
+	canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
-	if err != nil {
-		log.Error("Unable to GetUserRepoPermission for %-v in %-v#%d", doer, issue.Repo, issue.Index)
-		return err
-	}
 
 	if isAdd {
 		if issue.Repo.IsPrivate {
@@ -200,32 +188,28 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
 			}
 		}
 
-		doerCanWrite := permission.CanAccessAny(perm.AccessModeWrite, unit.TypePullRequests)
+		if canDoerChangeReviewRequests {
-		if !doerCanWrite && doer.ID != issue.PosterID {
+			return nil
-			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 remove reviewer",
 		UserID: doer.ID,
 		RepoID: issue.Repo.ID,
 	}
 }
 
-	return nil
-}
-
 // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it.
 func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) {
 	if isAdd {
@@ -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
+}

tests/gitea-repositories-meta/org41/repo61.git/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/master

tests/gitea-repositories-meta/org41/repo61.git/config

@@ -0,0 +1,6 @@
+[core]
+	repositoryformatversion = 0
+	filemode = false
+	bare = true
+	symlinks = false
+	ignorecase = true

tests/gitea-repositories-meta/org41/repo61.git/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

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]
+# *~

tests/gitea-repositories-meta/user40/repo60.git/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/master

tests/gitea-repositories-meta/user40/repo60.git/config

@@ -0,0 +1,6 @@
+[core]
+	repositoryformatversion = 0
+	filemode = false
+	bare = true
+	symlinks = false
+	ignorecase = true

tests/gitea-repositories-meta/user40/repo60.git/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

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]
+# *~

tests/integration/api_issue_test.go

@@ -368,7 +368,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
 	}
@@ -408,7 +408,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")
@@ -416,7 +416,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"}}
@@ -466,7 +466,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
 	}

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, "forgejo", nodeinfo.Software.Name)
-		assert.Equal(t, 26, nodeinfo.Usage.Users.Total)
+		assert.Equal(t, 29, nodeinfo.Usage.Users.Total)
-		assert.Equal(t, 20, nodeinfo.Usage.LocalPosts)
+		assert.Equal(t, 22, nodeinfo.Usage.LocalPosts)
 		assert.Equal(t, 3, nodeinfo.Usage.LocalComments)
 	})
 }

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)
 }

tests/integration/api_pull_review_test.go

@@ -401,6 +401,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))

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: 34},
+				nil:   {count: 36},
-				user:  {count: 34},
+				user:  {count: 36},
-				user2: {count: 34},
+				user2: {count: 36},
 			},
 		},
 		{

tests/integration/issue_test.go

@@ -458,7 +458,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
 	}
@@ -495,7 +495,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")
@@ -503,7 +503,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"}}
@@ -552,7 +552,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
 	}