Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Apr 27, 2024
12 changes: 10 additions & 2 deletions routers/api/v1/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,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 pull_service.IsErrSubmitReviewOnClosedPR(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
} else {
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
}
return
}

Expand Down Expand Up @@ -460,7 +464,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 pull_service.IsErrSubmitReviewOnClosedPR(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
} else {
ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
}
return
}

Expand Down
2 changes: 2 additions & 0 deletions routers/web/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 pull_service.IsErrSubmitReviewOnClosedPR(err) {
ctx.Status(http.StatusUnprocessableEntity)
} else {
ctx.ServerError("SubmitReview", err)
}
Expand Down
21 changes: 21 additions & 0 deletions services/pull/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ 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.
type ErrSubmitReviewOnClosedPR struct{}

// IsErrSubmitReviewOnClosedPR checks if an error is an ErrSubmitReviewOnClosedPR.
func IsErrSubmitReviewOnClosedPR(err error) bool {
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
_, ok := err.(ErrSubmitReviewOnClosedPR)
return ok
}

func (err ErrSubmitReviewOnClosedPR) Error() string {
return "can't submit review for a closed or merged PR"
}

func (err ErrSubmitReviewOnClosedPR) Unwrap() error {
return util.ErrPermissionDenied
}

// 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 {
Expand Down Expand Up @@ -293,6 +310,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 || pr.HasMerged {
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
return nil, nil, ErrSubmitReviewOnClosedPR{}
}

headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil {
return nil, nil, err
Expand Down
29 changes: 17 additions & 12 deletions templates/repo/diff/new_review.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,25 @@
{{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>
{{$isOpen := (not (or $.Issue.IsClosed (and $.Issue.IsPull $.Issue.PullRequest.HasMerged)))}}
{{if $isOpen}}
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
{{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 $isOpen}}
{{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>
Expand Down
82 changes: 82 additions & 0 deletions tests/integration/pull_review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}