From dd301cae1c40c9ef2805bd13af6b09a81ff4f5d7 Mon Sep 17 00:00:00 2001
From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com>
Date: Sat, 27 Apr 2024 04:55:03 -0700
Subject: [PATCH] Prevent allow/reject reviews on merged/closed PRs (#30686)
Resolves #30675.
---
routers/api/v1/repo/pull_review.go | 13 ++++-
routers/web/repo/pull_review.go | 2 +
services/pull/review.go | 8 +++
templates/repo/diff/new_review.tmpl | 28 +++++----
tests/integration/pull_review_test.go | 82 +++++++++++++++++++++++++++
5 files changed, 119 insertions(+), 14 deletions(-)
diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go
index b527e90f10b..4b481790fb1 100644
--- a/routers/api/v1/repo/pull_review.go
+++ b/routers/api/v1/repo/pull_review.go
@@ -4,6 +4,7 @@
package repo
import (
+ "errors"
"fmt"
"net/http"
"strings"
@@ -372,7 +373,11 @@ func CreatePullReview(ctx *context.APIContext) {
// create review and associate all pending review comments
review, _, err := pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, opts.CommitID, nil)
if err != nil {
- ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+ if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
+ ctx.Error(http.StatusUnprocessableEntity, "", err)
+ } else {
+ ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+ }
return
}
@@ -460,7 +465,11 @@ func SubmitPullReview(ctx *context.APIContext) {
// create review and associate all pending review comments
review, _, err = pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, headCommitID, nil)
if err != nil {
- ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+ if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
+ ctx.Error(http.StatusUnprocessableEntity, "", err)
+ } else {
+ ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+ }
return
}
diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index a65d4866d0a..62f6d71c5e5 100644
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -264,6 +264,8 @@ func SubmitReview(ctx *context.Context) {
if issues_model.IsContentEmptyErr(err) {
ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))
ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
+ } else if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
+ ctx.Status(http.StatusUnprocessableEntity)
} else {
ctx.ServerError("SubmitReview", err)
}
diff --git a/services/pull/review.go b/services/pull/review.go
index 5bf1991d134..e303cd9a9d6 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -6,6 +6,7 @@ package pull
import (
"context"
+ "errors"
"fmt"
"io"
"regexp"
@@ -43,6 +44,9 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error {
return util.ErrPermissionDenied
}
+// ErrSubmitReviewOnClosedPR represents an error when an user tries to submit an approve or reject review associated to a closed or merged PR.
+var ErrSubmitReviewOnClosedPR = errors.New("can't submit review for a closed or merged PR")
+
// checkInvalidation checks if the line of code comment got changed by another commit.
// If the line got changed the comment is going to be invalidated.
func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error {
@@ -293,6 +297,10 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos
if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
stale = false
} else {
+ if issue.IsClosed {
+ return nil, nil, ErrSubmitReviewOnClosedPR
+ }
+
headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil {
return nil, nil, err
diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl
index a2eae007a56..1b74a230f47 100644
--- a/templates/repo/diff/new_review.tmpl
+++ b/templates/repo/diff/new_review.tmpl
@@ -30,20 +30,24 @@
{{end}}
<div class="divider"></div>
{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
- {{if $showSelfTooltip}}
- <span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
- <button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
- </span>
- {{else}}
- <button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+ {{if not $.Issue.IsClosed}}
+ {{if $showSelfTooltip}}
+ <span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
+ <button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+ </span>
+ {{else}}
+ <button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+ {{end}}
{{end}}
<button type="submit" name="type" value="comment" class="ui submit tiny basic button btn-submit">{{ctx.Locale.Tr "repo.diff.review.comment"}}</button>
- {{if $showSelfTooltip}}
- <span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
- <button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
- </span>
- {{else}}
- <button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+ {{if not $.Issue.IsClosed}}
+ {{if $showSelfTooltip}}
+ <span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
+ <button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+ </span>
+ {{else}}
+ <button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+ {{end}}
{{end}}
</form>
</div>
diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go
index 2d8b3cb4ab2..273332a36b4 100644
--- a/tests/integration/pull_review_test.go
+++ b/tests/integration/pull_review_test.go
@@ -5,12 +5,15 @@ package integration
import (
"net/http"
+ "net/http/httptest"
"net/url"
+ "path"
"strings"
"testing"
"code.gitea.io/gitea/models/db"
issues_model "code.gitea.io/gitea/models/issues"
+ repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
@@ -176,3 +179,82 @@ func TestPullView_CodeOwner(t *testing.T) {
})
})
}
+
+func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
+ onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+ user1Session := loginUser(t, "user1")
+ user2Session := loginUser(t, "user2")
+
+ // Have user1 create a fork of repo1.
+ testRepoFork(t, user1Session, "user2", "repo1", "user1", "repo1")
+
+ t.Run("Submit approve/reject review on merged PR", func(t *testing.T) {
+ // Create a merged PR (made by user1) in the upstream repo1.
+ testEditFile(t, user1Session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
+ resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "master", "This is a pull title")
+ elem := strings.Split(test.RedirectURL(resp), "/")
+ assert.EqualValues(t, "pulls", elem[3])
+ testPullMerge(t, user1Session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
+
+ // Grab the CSRF token.
+ req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
+ resp = user2Session.MakeRequest(t, req, http.StatusOK)
+ htmlDoc := NewHTMLParser(t, resp.Body)
+
+ // Submit an approve review on the PR.
+ testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity)
+
+ // Submit a reject review on the PR.
+ testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity)
+ })
+
+ t.Run("Submit approve/reject review on closed PR", func(t *testing.T) {
+ // Created a closed PR (made by user1) in the upstream repo1.
+ testEditFileToNewBranch(t, user1Session, "user1", "repo1", "master", "a-test-branch", "README.md", "Hello, World (Editied...again)\n")
+ resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "a-test-branch", "This is a pull title")
+ elem := strings.Split(test.RedirectURL(resp), "/")
+ assert.EqualValues(t, "pulls", elem[3])
+ testIssueClose(t, user1Session, elem[1], elem[2], elem[4])
+
+ // Grab the CSRF token.
+ req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
+ resp = user2Session.MakeRequest(t, req, http.StatusOK)
+ htmlDoc := NewHTMLParser(t, resp.Body)
+
+ // Submit an approve review on the PR.
+ testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity)
+
+ // Submit a reject review on the PR.
+ testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity)
+ })
+ })
+}
+
+func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder {
+ options := map[string]string{
+ "_csrf": csrf,
+ "commit_id": "",
+ "content": "test",
+ "type": reviewType,
+ }
+
+ submitURL := path.Join(owner, repo, "pulls", pullNumber, "files", "reviews", "submit")
+ req := NewRequestWithValues(t, "POST", submitURL, options)
+ return session.MakeRequest(t, req, expectedSubmitStatus)
+}
+
+func testIssueClose(t *testing.T, session *TestSession, owner, repo, issueNumber string) *httptest.ResponseRecorder {
+ req := NewRequest(t, "GET", path.Join(owner, repo, "pulls", issueNumber))
+ resp := session.MakeRequest(t, req, http.StatusOK)
+
+ htmlDoc := NewHTMLParser(t, resp.Body)
+ closeURL := path.Join(owner, repo, "issues", issueNumber, "comments")
+
+ options := map[string]string{
+ "_csrf": htmlDoc.GetCSRF(),
+ "status": "close",
+ }
+
+ req = NewRequestWithValues(t, "POST", closeURL, options)
+ return session.MakeRequest(t, req, http.StatusOK)
+}