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

1 files changed, 88 insertions, 57 deletions

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