From 242b331260925e604150346e61329097d5731e77 Mon Sep 17 00:00:00 2001
From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com>
Date: Thu, 28 Mar 2024 08:19:24 -0700
Subject: [PATCH] Prevent re-review and dismiss review actions on closed and
merged PRs (#30065)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Resolves #29965.
---
Manually tested this by:
- Following the
[installation](https://docs.gitea.com/next/installation/install-with-docker#basics)
guide (but built a local Docker image instead)
- Creating 2 users, one who is the `Owner` of a newly-created repository
and the other a `Collaborator`
- Had the `Collaborator` create a PR that the `Owner` reviews
- `Collaborator` resolves conversation and `Owner` merges PR
And with this change we see that we can no longer see re-request review
button for the `Owner`:
<img width="1351" alt="Screenshot 2024-03-25 at 12 39 18 AM"
src="https://github.com/go-gitea/gitea/assets/60799661/bcd9c579-3cf7-474f-a51e-b436fe1a39a4">
---
models/issues/review.go | 38 +++++++++++++--
models/issues/review_test.go | 30 ++++++++++++
routers/api/v1/repo/pull_review.go | 17 ++++---
routers/web/repo/issue.go | 4 ++
routers/web/repo/pull_review.go | 4 ++
services/pull/review.go | 33 +++++++++++++
services/pull/review_test.go | 48 +++++++++++++++++++
.../repo/issue/view_content/sidebar.tmpl | 4 +-
templates/swagger/v1_json.tmpl | 3 ++
9 files changed, 170 insertions(+), 11 deletions(-)
create mode 100644 services/pull/review_test.go
diff --git a/models/issues/review.go b/models/issues/review.go
index 455bcda50a..92764db4d1 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -66,6 +66,23 @@ func (err ErrNotValidReviewRequest) Unwrap() error {
return util.ErrInvalidArgument
}
+// ErrReviewRequestOnClosedPR represents an error when an user tries to request a re-review on a closed or merged PR.
+type ErrReviewRequestOnClosedPR struct{}
+
+// IsErrReviewRequestOnClosedPR checks if an error is an ErrReviewRequestOnClosedPR.
+func IsErrReviewRequestOnClosedPR(err error) bool {
+ _, ok := err.(ErrReviewRequestOnClosedPR)
+ return ok
+}
+
+func (err ErrReviewRequestOnClosedPR) Error() string {
+ return "cannot request a re-review on a closed or merged PR"
+}
+
+func (err ErrReviewRequestOnClosedPR) Unwrap() error {
+ return util.ErrPermissionDenied
+}
+
// ReviewType defines the sort of feedback a review gives
type ReviewType int
@@ -618,9 +635,24 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
return nil, err
}
- // skip it when reviewer hase been request to review
- if review != nil && review.Type == ReviewTypeRequest {
- return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
+ if review != nil {
+ // skip it when reviewer hase been request to review
+ if review.Type == ReviewTypeRequest {
+ return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
+ }
+
+ if issue.IsClosed {
+ return nil, ErrReviewRequestOnClosedPR{}
+ }
+
+ if issue.IsPull {
+ if err := issue.LoadPullRequest(ctx); err != nil {
+ return nil, err
+ }
+ if issue.PullRequest.HasMerged {
+ return nil, ErrReviewRequestOnClosedPR{}
+ }
+ }
}
// if the reviewer is an official reviewer,
diff --git a/models/issues/review_test.go b/models/issues/review_test.go
index 1868cb1bfa..ac1b84adeb 100644
--- a/models/issues/review_test.go
+++ b/models/issues/review_test.go
@@ -288,3 +288,33 @@ func TestDeleteDismissedReview(t *testing.T) {
assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review))
unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID})
}
+
+func TestAddReviewRequest(t *testing.T) {
+ assert.NoError(t, unittest.PrepareTestDatabase())
+
+ pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1})
+ assert.NoError(t, pull.LoadIssue(db.DefaultContext))
+ issue := pull.Issue
+ assert.NoError(t, issue.LoadRepo(db.DefaultContext))
+ reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+ _, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{
+ Issue: issue,
+ Reviewer: reviewer,
+ Type: issues_model.ReviewTypeReject,
+ })
+
+ assert.NoError(t, err)
+ pull.HasMerged = false
+ assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
+ issue.IsClosed = true
+ _, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{})
+ assert.Error(t, err)
+ assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
+
+ pull.HasMerged = true
+ assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
+ issue.IsClosed = false
+ _, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{})
+ assert.Error(t, err)
+ assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
+}
diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go
index d314c4e7f7..17bb2085b6 100644
--- a/routers/api/v1/repo/pull_review.go
+++ b/routers/api/v1/repo/pull_review.go
@@ -640,6 +640,8 @@ func DeleteReviewRequests(ctx *context.APIContext) {
// "$ref": "#/responses/empty"
// "422":
// "$ref": "#/responses/validationError"
+ // "403":
+ // "$ref": "#/responses/forbidden"
// "404":
// "$ref": "#/responses/notFound"
opts := web.GetForm(ctx).(*api.PullReviewRequestOptions)
@@ -708,6 +710,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
for _, reviewer := range reviewers {
comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd)
if err != nil {
+ if issues_model.IsErrReviewRequestOnClosedPR(err) {
+ ctx.Error(http.StatusForbidden, "", err)
+ return
+ }
ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
return
}
@@ -874,7 +880,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
ctx.Error(http.StatusForbidden, "", "Must be repo admin")
return
}
- review, pr, isWrong := prepareSingleReview(ctx)
+ review, _, isWrong := prepareSingleReview(ctx)
if isWrong {
return
}
@@ -884,13 +890,12 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
return
}
- if pr.Issue.IsClosed {
- ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed")
- return
- }
-
_, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors)
if err != nil {
+ if pull_service.IsErrDismissRequestOnClosedPR(err) {
+ ctx.Error(http.StatusForbidden, "", err)
+ return
+ }
ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
return
}
diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go
index 12233d0e17..6c2d4a7390 100644
--- a/routers/web/repo/issue.go
+++ b/routers/web/repo/issue.go
@@ -2498,6 +2498,10 @@ func UpdatePullReviewRequest(ctx *context.Context) {
_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach")
if err != nil {
+ if issues_model.IsErrReviewRequestOnClosedPR(err) {
+ ctx.Status(http.StatusForbidden)
+ return
+ }
ctx.ServerError("ReviewRequest", err)
return
}
diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index 5385ebfc97..c8d149a482 100644
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -277,6 +277,10 @@ func DismissReview(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.DismissReviewForm)
comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true)
if err != nil {
+ if pull_service.IsErrDismissRequestOnClosedPR(err) {
+ ctx.Status(http.StatusForbidden)
+ return
+ }
ctx.ServerError("pull_service.DismissReview", err)
return
}
diff --git a/services/pull/review.go b/services/pull/review.go
index de1021c5c0..5bf1991d13 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -20,11 +20,29 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting"
+ "code.gitea.io/gitea/modules/util"
notify_service "code.gitea.io/gitea/services/notify"
)
var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`)
+// ErrDismissRequestOnClosedPR represents an error when an user tries to dismiss a review associated to a closed or merged PR.
+type ErrDismissRequestOnClosedPR struct{}
+
+// IsErrDismissRequestOnClosedPR checks if an error is an ErrDismissRequestOnClosedPR.
+func IsErrDismissRequestOnClosedPR(err error) bool {
+ _, ok := err.(ErrDismissRequestOnClosedPR)
+ return ok
+}
+
+func (err ErrDismissRequestOnClosedPR) Error() string {
+ return "can't dismiss a review associated to a closed or merged PR"
+}
+
+func (err ErrDismissRequestOnClosedPR) 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 {
@@ -382,6 +400,21 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string,
return nil, fmt.Errorf("reviews's repository is not the same as the one we expect")
}
+ issue := review.Issue
+
+ if issue.IsClosed {
+ return nil, ErrDismissRequestOnClosedPR{}
+ }
+
+ if issue.IsPull {
+ if err := issue.LoadPullRequest(ctx); err != nil {
+ return nil, err
+ }
+ if issue.PullRequest.HasMerged {
+ return nil, ErrDismissRequestOnClosedPR{}
+ }
+ }
+
if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil {
return nil, err
}
diff --git a/services/pull/review_test.go b/services/pull/review_test.go
new file mode 100644
index 0000000000..3bce1e523d
--- /dev/null
+++ b/services/pull/review_test.go
@@ -0,0 +1,48 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull_test
+
+import (
+ "testing"
+
+ "code.gitea.io/gitea/models/db"
+ issues_model "code.gitea.io/gitea/models/issues"
+ "code.gitea.io/gitea/models/unittest"
+ user_model "code.gitea.io/gitea/models/user"
+ pull_service "code.gitea.io/gitea/services/pull"
+
+ "github.com/stretchr/testify/assert"
+)
+
+func TestDismissReview(t *testing.T) {
+ assert.NoError(t, unittest.PrepareTestDatabase())
+
+ pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{})
+ assert.NoError(t, pull.LoadIssue(db.DefaultContext))
+ issue := pull.Issue
+ assert.NoError(t, issue.LoadRepo(db.DefaultContext))
+ reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+ review, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{
+ Issue: issue,
+ Reviewer: reviewer,
+ Type: issues_model.ReviewTypeReject,
+ })
+
+ assert.NoError(t, err)
+ issue.IsClosed = true
+ pull.HasMerged = false
+ assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed"))
+ assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
+ _, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false)
+ assert.Error(t, err)
+ assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err))
+
+ pull.HasMerged = true
+ pull.Issue.IsClosed = false
+ assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed"))
+ assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
+ _, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false)
+ assert.Error(t, err)
+ assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err))
+}
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index bc2a841708..5913916ae4 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -59,7 +59,7 @@
{{end}}
</div>
<div class="tw-flex tw-items-center tw-gap-2">
- {{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed))}}
+ {{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged))}}
<a href="#" class="ui muted icon tw-flex tw-items-center show-modal" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.dismiss_review"}}" data-modal="#dismiss-review-modal-{{.Review.ID}}">
{{svg "octicon-x" 20}}
</a>
@@ -91,7 +91,7 @@
{{svg "octicon-hourglass" 16}}
</span>
{{end}}
- {{if .CanChange}}
+ {{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}}
<a href="#" class="ui muted icon re-request-review{{if .Checked}} checked{{end}}" data-tooltip-content="{{if .Checked}}{{ctx.Locale.Tr "repo.issues.remove_request_review"}}{{else}}{{ctx.Locale.Tr "repo.issues.re_request_review"}}{{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">{{if .Checked}}{{svg "octicon-trash"}}{{else}}{{svg "octicon-sync"}}{{end}}</a>
{{end}}
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
index f835df084d..ef6126ff85 100644
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -11276,6 +11276,9 @@
"204": {
"$ref": "#/responses/empty"
},
+ "403": {
+ "$ref": "#/responses/forbidden"
+ },
"404": {
"$ref": "#/responses/notFound"
},