Skip to content

Commit dd301ca

Browse files
kemzebkemzeb
authored
Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686)
Resolves go-gitea#30675.
1 parent 238eb3f commit dd301ca

File tree

5 files changed

+119
-14
lines changed

5 files changed

+119
-14
lines changed

‎routers/api/v1/repo/pull_review.go

+11-2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package repo
55

66
import (
7+
"errors"
78
"fmt"
89
"net/http"
910
"strings"
@@ -372,7 +373,11 @@ func CreatePullReview(ctx *context.APIContext) {
372373
// create review and associate all pending review comments
373374
review, _, err := pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, opts.CommitID, nil)
374375
if err != nil {
375-
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
376+
if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
377+
ctx.Error(http.StatusUnprocessableEntity, "", err)
378+
} else {
379+
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
380+
}
376381
return
377382
}
378383

@@ -460,7 +465,11 @@ func SubmitPullReview(ctx *context.APIContext) {
460465
// create review and associate all pending review comments
461466
review, _, err = pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, headCommitID, nil)
462467
if err != nil {
463-
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
468+
if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
469+
ctx.Error(http.StatusUnprocessableEntity, "", err)
470+
} else {
471+
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
472+
}
464473
return
465474
}
466475

‎routers/web/repo/pull_review.go

+2
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,8 @@ func SubmitReview(ctx *context.Context) {
264264
if issues_model.IsContentEmptyErr(err) {
265265
ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))
266266
ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
267+
} else if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
268+
ctx.Status(http.StatusUnprocessableEntity)
267269
} else {
268270
ctx.ServerError("SubmitReview", err)
269271
}

‎services/pull/review.go

+8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package pull
66

77
import (
88
"context"
9+
"errors"
910
"fmt"
1011
"io"
1112
"regexp"
@@ -43,6 +44,9 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error {
4344
return util.ErrPermissionDenied
4445
}
4546

47+
// ErrSubmitReviewOnClosedPR represents an error when an user tries to submit an approve or reject review associated to a closed or merged PR.
48+
var ErrSubmitReviewOnClosedPR = errors.New("can't submit review for a closed or merged PR")
49+
4650
// checkInvalidation checks if the line of code comment got changed by another commit.
4751
// If the line got changed the comment is going to be invalidated.
4852
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
293297
if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
294298
stale = false
295299
} else {
300+
if issue.IsClosed {
301+
return nil, nil, ErrSubmitReviewOnClosedPR
302+
}
303+
296304
headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
297305
if err != nil {
298306
return nil, nil, err

‎templates/repo/diff/new_review.tmpl

+16-12
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,24 @@
3030
{{end}}
3131
<div class="divider"></div>
3232
{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
33-
{{if $showSelfTooltip}}
34-
<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
35-
<button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
36-
</span>
37-
{{else}}
38-
<button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
33+
{{if not $.Issue.IsClosed}}
34+
{{if $showSelfTooltip}}
35+
<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
36+
<button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
37+
</span>
38+
{{else}}
39+
<button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
40+
{{end}}
3941
{{end}}
4042
<button type="submit" name="type" value="comment" class="ui submit tiny basic button btn-submit">{{ctx.Locale.Tr "repo.diff.review.comment"}}</button>
41-
{{if $showSelfTooltip}}
42-
<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
43-
<button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
44-
</span>
45-
{{else}}
46-
<button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
43+
{{if not $.Issue.IsClosed}}
44+
{{if $showSelfTooltip}}
45+
<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
46+
<button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
47+
</span>
48+
{{else}}
49+
<button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
50+
{{end}}
4751
{{end}}
4852
</form>
4953
</div>

‎tests/integration/pull_review_test.go

+82
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,15 @@ package integration
55

66
import (
77
"net/http"
8+
"net/http/httptest"
89
"net/url"
10+
"path"
911
"strings"
1012
"testing"
1113

1214
"code.gitea.io/gitea/models/db"
1315
issues_model "code.gitea.io/gitea/models/issues"
16+
repo_model "code.gitea.io/gitea/models/repo"
1417
"code.gitea.io/gitea/models/unittest"
1518
user_model "code.gitea.io/gitea/models/user"
1619
"code.gitea.io/gitea/modules/git"
@@ -176,3 +179,82 @@ func TestPullView_CodeOwner(t *testing.T) {
176179
})
177180
})
178181
}
182+
183+
func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
184+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
185+
user1Session := loginUser(t, "user1")
186+
user2Session := loginUser(t, "user2")
187+
188+
// Have user1 create a fork of repo1.
189+
testRepoFork(t, user1Session, "user2", "repo1", "user1", "repo1")
190+
191+
t.Run("Submit approve/reject review on merged PR", func(t *testing.T) {
192+
// Create a merged PR (made by user1) in the upstream repo1.
193+
testEditFile(t, user1Session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
194+
resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "master", "This is a pull title")
195+
elem := strings.Split(test.RedirectURL(resp), "/")
196+
assert.EqualValues(t, "pulls", elem[3])
197+
testPullMerge(t, user1Session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
198+
199+
// Grab the CSRF token.
200+
req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
201+
resp = user2Session.MakeRequest(t, req, http.StatusOK)
202+
htmlDoc := NewHTMLParser(t, resp.Body)
203+
204+
// Submit an approve review on the PR.
205+
testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity)
206+
207+
// Submit a reject review on the PR.
208+
testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity)
209+
})
210+
211+
t.Run("Submit approve/reject review on closed PR", func(t *testing.T) {
212+
// Created a closed PR (made by user1) in the upstream repo1.
213+
testEditFileToNewBranch(t, user1Session, "user1", "repo1", "master", "a-test-branch", "README.md", "Hello, World (Editied...again)\n")
214+
resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "a-test-branch", "This is a pull title")
215+
elem := strings.Split(test.RedirectURL(resp), "/")
216+
assert.EqualValues(t, "pulls", elem[3])
217+
testIssueClose(t, user1Session, elem[1], elem[2], elem[4])
218+
219+
// Grab the CSRF token.
220+
req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
221+
resp = user2Session.MakeRequest(t, req, http.StatusOK)
222+
htmlDoc := NewHTMLParser(t, resp.Body)
223+
224+
// Submit an approve review on the PR.
225+
testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "approve", http.StatusUnprocessableEntity)
226+
227+
// Submit a reject review on the PR.
228+
testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "reject", http.StatusUnprocessableEntity)
229+
})
230+
})
231+
}
232+
233+
func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder {
234+
options := map[string]string{
235+
"_csrf": csrf,
236+
"commit_id": "",
237+
"content": "test",
238+
"type": reviewType,
239+
}
240+
241+
submitURL := path.Join(owner, repo, "pulls", pullNumber, "files", "reviews", "submit")
242+
req := NewRequestWithValues(t, "POST", submitURL, options)
243+
return session.MakeRequest(t, req, expectedSubmitStatus)
244+
}
245+
246+
func testIssueClose(t *testing.T, session *TestSession, owner, repo, issueNumber string) *httptest.ResponseRecorder {
247+
req := NewRequest(t, "GET", path.Join(owner, repo, "pulls", issueNumber))
248+
resp := session.MakeRequest(t, req, http.StatusOK)
249+
250+
htmlDoc := NewHTMLParser(t, resp.Body)
251+
closeURL := path.Join(owner, repo, "issues", issueNumber, "comments")
252+
253+
options := map[string]string{
254+
"_csrf": htmlDoc.GetCSRF(),
255+
"status": "close",
256+
}
257+
258+
req = NewRequestWithValues(t, "POST", closeURL, options)
259+
return session.MakeRequest(t, req, http.StatusOK)
260+
}

0 commit comments

Comments
 (0)