From 23676bfea7ccbbe166a554115ea1f5f02800e379 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`: Screenshot 2024-03-25 at 12 39 18 AM (cherry picked from commit 242b331260925e604150346e61329097d5731e77) --- 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 9ccbb57c52..77c0d25e2a 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -787,6 +787,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) @@ -855,6 +857,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 } @@ -1068,7 +1074,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 } @@ -1078,13 +1084,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 7647e89c56..a4e3f7165a 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2494,6 +2494,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 ead7592fd0..afa3b17d31 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -263,6 +263,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 7d698a14f6..6ad931b679 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 80b627c2ed..f4b19f0948 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -59,7 +59,7 @@ {{end}}
- {{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))}} {{svg "octicon-x" 20}} @@ -91,7 +91,7 @@ {{svg "octicon-hourglass" 16}} {{end}} - {{if .CanChange}} + {{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}} {{if .Checked}}{{svg "octicon-trash"}}{{else}}{{svg "octicon-sync"}}{{end}} {{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 6034a8fbce..22ada2f023 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -11422,6 +11422,9 @@ "204": { "$ref": "#/responses/empty" }, + "403": { + "$ref": "#/responses/forbidden" + }, "404": { "$ref": "#/responses/notFound" },