Prevent allow/reject reviews on merged/closed PRs (#30686)

Resolves #30675.

diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go
index b527e90f10b8cb5a99c471bec0383b8238a16fa8..4b481790fb1a869ffe82533836517e2919dc6fa8 100644 (file)
--- 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 a65d4866d0a48b97844169690f2a8e9253c021fc..62f6d71c5e5cf2ca7528bc5d1b23d4ce1655adb2 100644 (file)
--- 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 5bf1991d134233073a35d192ea275309fbc426c3..e303cd9a9d6eaa6ec41f14fa117166070038d3d9 100644 (file)
--- 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 a2eae007a56c9e95284d367628180734f82fda77..1b74a230f47d4ed7cf8709aa119764a281501362 100644 (file)
--- 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 2d8b3cb4ab2e96070f07d38ab44ecc2efd1ae382..273332a36b416659d53b49449e45c9c195cb44fa 100644 (file)
--- 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)
+}