fix(pull): enforce user blocks on pull request reviews (#40194)

Code review never checked user blocks, so a user blocked by the poster
or repository owner could still post reviews, code comments and replies
through the web, API and email.

- `CreateCodeComment` and `SubmitReview` now check blocks, with the repo
admin exemption issue comments have.
- Official reviewers blocked only by the author can still review, so an
author can't keep a maintainer from requesting changes.
- Review requests are handled in
https://github.com/go-gitea/gitea/pull/39296.

---------

Co-authored-by: silverwind <me@silverwind.io>

routers/api/v1/repo/pull_review.go

@@ -273,7 +273,7 @@ func CreatePullReviewCommentReply(ctx *context.APIContext) {
 		"", nil,
 	)
 	if err != nil {
-		ctx.APIErrorInternal(err)
+		ctx.APIErrorAuto(err)
 		return
 	}
 	if err := comment.LoadPoster(ctx); err != nil {
@@ -545,7 +545,7 @@ func CreatePullReview(ctx *context.APIContext) {
 			opts.CommitID,
 			nil,
 		); err != nil {
-			ctx.APIErrorInternal(err)
+			ctx.APIErrorAuto(err)
 			return
 		}
 	}
@@ -556,7 +556,7 @@ func CreatePullReview(ctx *context.APIContext) {
 		if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) || issues_model.IsContentEmptyErr(err) {
 			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
 		} else {
-			ctx.APIErrorInternal(err)
+			ctx.APIErrorAuto(err)
 		}
 		return
 	}
@@ -650,7 +650,7 @@ func SubmitPullReview(ctx *context.APIContext) {
 		if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) || issues_model.IsContentEmptyErr(err) {
 			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
 		} else {
-			ctx.APIErrorInternal(err)
+			ctx.APIErrorAuto(err)
 		}
 		return
 	}

routers/web/repo/pull_review.go

@@ -105,7 +105,7 @@ func CreateCodeComment(ctx *context.Context) {
 		attachments,
 	)
 	if err != nil {
-		ctx.ServerError("CreateCodeComment", err)
+		ctx.JSONErrorAuto(err)
 		return
 	}
 
@@ -280,7 +280,7 @@ func SubmitReview(ctx *context.Context) {
 		} else if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
 			ctx.Status(http.StatusUnprocessableEntity)
 		} else {
-			ctx.ServerError("SubmitReview", err)
+			ctx.JSONErrorAuto(err)
 		}
 		return
 	}

services/pull/review.go

@@ -11,6 +11,7 @@ import (
 
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	access_model "gitea.dev/models/perm/access"
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
@@ -95,6 +96,10 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis
 // * replyReviewID is set: add the comment to the existing submitted review
 // When pendingReview=true: get current pending review or create a new one, and add the comment to it
 func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string, attachments []string) (_ *issues_model.Comment, err error) {
+	if err := checkReviewerNotBlocked(ctx, doer, issue); err != nil {
+		return nil, err
+	}
+
 	replyToSubmittedReview := !pendingReview && replyReviewID != 0
 	if replyToSubmittedReview {
 		// It's not part of a review; maybe a reply to a review comment or a single comment.
@@ -107,10 +112,6 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 	// Comments that are replies don't require a review header to show up in the issue view
 	// Add the code comment to existing submitted review
 	if replyToSubmittedReview {
-		if err = issue.LoadRepo(ctx); err != nil {
-			return nil, err
-		}
-
 		comment, err := createCodeComment(ctx,
 			doer,
 			issue.Repo,
@@ -182,6 +183,21 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 	return comment, nil
 }
 
+func checkReviewerNotBlocked(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) error {
+	if err := issue.LoadRepo(ctx); err != nil {
+		return err
+	}
+	if !user_model.IsUserBlockedBy(ctx, doer, issue.PosterID, issue.Repo.OwnerID) || access_model.IsUserRepoAdmin(ctx, issue.Repo, doer) {
+		return nil
+	}
+	if !user_model.IsUserBlockedBy(ctx, doer, issue.Repo.OwnerID) { // the author's block must not keep official reviewers from requesting changes
+		if official, err := issues_model.IsOfficialReviewer(ctx, issue, doer); err != nil || official {
+			return err
+		}
+	}
+	return util.ErrorWrapTranslatable(user_model.ErrBlockedUser, "repo.issues.comment.blocked_user")
+}
+
 // createCodeComment creates a plain code comment at the specified line / path
 func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) {
 	var commitID, patch string
@@ -280,6 +296,10 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 
 // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
 func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, reviewType issues_model.ReviewType, content, commitID string, attachmentUUIDs []string) (*issues_model.Review, *issues_model.Comment, error) {
+	if err := checkReviewerNotBlocked(ctx, doer, issue); err != nil {
+		return nil, nil, err
+	}
+
 	if err := issue.LoadPullRequest(ctx); err != nil {
 		return nil, nil, err
 	}

tests/integration/pull_review_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"path"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -36,7 +37,7 @@ import (
 func TestPullReview(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	t.Run("ReviewerMissed", testPullReviewReviewerMissed)
-	t.Run("LockedPR", testPullReviewLockedPR)
+	t.Run("LockedOrBlocked", testPullReviewLockedOrBlocked)
 }
 
 func testPullReviewReviewerMissed(t *testing.T) {
@@ -58,24 +59,33 @@ func testPullReviewReviewerMissed(t *testing.T) {
 	assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
 }
 
-func testPullReviewLockedPR(t *testing.T) {
-	const reviewsURL = "/api/v1/repos/user2/repo1/pulls/3/reviews"
-	token := getUserToken(t, "user4", auth_model.AccessTokenScopeWriteRepository)
-	resp := MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, &api.CreatePullReviewOptions{Event: api.ReviewStatePending, Body: "test"}).AddTokenAuth(token), http.StatusOK)
-	pendingReview := DecodeJSON(t, resp, &api.PullReview{})
+func testPullReviewLockedOrBlocked(t *testing.T) {
 	require.NoError(t, issues_model.UpdateIssueCols(t.Context(), &issues_model.Issue{ID: 3, IsLocked: true}, "is_locked"))
-	commentCount := unittest.GetCount(t, &issues_model.Comment{IssueID: 3})
-	reviewOptions := &api.CreatePullReviewOptions{Event: api.ReviewStateComment, Body: "test"}
+	for _, tc := range []struct{ reviewerID, index, replyCommentID int64 }{{4, 3, 7}, {34, 2, 4}} {
+		reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.reviewerID})
+		issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: 1, Index: tc.index})
+		pendingReview, err := issues_model.CreateReview(t.Context(), issues_model.CreateReviewOptions{Type: issues_model.ReviewTypePending, Issue: issue, Reviewer: reviewer})
+		require.NoError(t, err)
+		commentCount := unittest.GetCount(t, &issues_model.Comment{IssueID: issue.ID})
+		pullPath := fmt.Sprintf("user2/repo1/pulls/%d", tc.index)
+		session := loginUser(t, reviewer.Name)
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
 
-	session := loginUser(t, "user4")
-	testSubmitReview(t, session, "user2", "repo1", "3", "", "comment", http.StatusBadRequest)
-	session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{"content": "test"}), http.StatusBadRequest)
-	MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, reviewOptions).AddTokenAuth(token), http.StatusForbidden)
-	MakeRequest(t, NewRequestWithJSON(t, "POST", fmt.Sprintf("%s/%d", reviewsURL, pendingReview.ID), &api.SubmitPullReviewOptions{Event: api.ReviewStateComment, Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
-	MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/pulls/3/comments/7/replies", &api.CreatePullReviewCommentReplyOptions{Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
-	unittest.AssertCount(t, &issues_model.Comment{IssueID: 3}, commentCount)
+		testSubmitReview(t, session, "user2", "repo1", strconv.FormatInt(tc.index, 10), "", "comment", http.StatusBadRequest)
+		session.MakeRequest(t, NewRequestWithValues(t, "POST", "/"+pullPath+"/files/reviews/comments", map[string]string{
+			"origin": "diff", "content": "test", "path": "README.md", "side": "proposed", "line": "1",
+		}), http.StatusBadRequest)
+		MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/"+pullPath+"/reviews", &api.CreatePullReviewOptions{Event: api.ReviewStateComment, Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
+		MakeRequest(t, NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/reviews/%d", pullPath, pendingReview.ID), &api.SubmitPullReviewOptions{Event: api.ReviewStateComment, Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
+		MakeRequest(t, NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/comments/%d/replies", pullPath, tc.replyCommentID), &api.CreatePullReviewCommentReplyOptions{Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
+		unittest.AssertCount(t, &issues_model.Comment{IssueID: issue.ID}, commentCount)
+	}
+	MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/pulls/3/reviews", &api.CreatePullReviewOptions{Event: api.ReviewStateComment, Body: "test"}).AddTokenAuth(getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)), http.StatusOK)
 
-	MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, reviewOptions).AddTokenAuth(getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)), http.StatusOK)
+	for name, status := range map[string]int{"user5": http.StatusBadRequest, "user40": http.StatusOK} {
+		require.NoError(t, db.Insert(t.Context(), &user_model.Blocking{BlockerID: 1, BlockeeID: unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: name}).ID}))
+		testSubmitReview(t, loginUser(t, name), "user2", "repo1", "2", "", "comment", status)
+	}
 }
 
 func TestPullView_CodeOwner(t *testing.T) {