diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index e8bc2398a29..dec78dc7797 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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 } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 2fd841eb18a..83c055dab2b 100644 --- a/routers/web/repo/pull_review.go +++ b/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 } diff --git a/services/pull/review.go b/services/pull/review.go index 7526e0d0359..b3160f2430f 100644 --- a/services/pull/review.go +++ b/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 } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 6f9c28d76ad..fc292239fd7 100644 --- a/tests/integration/pull_review_test.go +++ b/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) {