From ea707f5a77d83d023cb02ce8e44c4b95ac83ef30 Mon Sep 17 00:00:00 2001
From: David Svantesson <davidsvantesson@gmail.com>
Date: Fri, 3 Jan 2020 18:47:10 +0100
Subject: [PATCH] Add branch protection option to block merge on requested
 changes. (#9592)

* Add branch protection option to block merge on requested changes.

* Add migration step

* Fix check to correct negation

* Apply suggestions from code review

Language improvement.

Co-Authored-By: John Olheiser <42128690+jolheiser@users.noreply.github.com>

* Copyright year.

Co-authored-by: John Olheiser <42128690+jolheiser@users.noreply.github.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
---
 models/branches.go                            | 20 ++++++++++++++++++-
 models/migrations/migrations.go               |  2 ++
 models/migrations/v117.go                     | 17 ++++++++++++++++
 modules/auth/repo_form.go                     |  1 +
 options/locale/locale_en-US.ini               |  3 +++
 routers/private/hook.go                       |  7 +++++++
 routers/repo/issue.go                         |  3 ++-
 routers/repo/setting_protected_branch.go      |  1 +
 templates/repo/issue/view_content/pull.tmpl   |  6 ++++++
 templates/repo/settings/protected_branch.tmpl |  7 +++++++
 10 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 models/migrations/v117.go

diff --git a/models/branches.go b/models/branches.go
index 21b23c75d9..385817e4f9 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -44,6 +44,7 @@ type ProtectedBranch struct {
 	ApprovalsWhitelistUserIDs []int64            `xorm:"JSON TEXT"`
 	ApprovalsWhitelistTeamIDs []int64            `xorm:"JSON TEXT"`
 	RequiredApprovals         int64              `xorm:"NOT NULL DEFAULT 0"`
+	BlockOnRejectedReviews    bool               `xorm:"NOT NULL DEFAULT false"`
 	CreatedUnix               timeutil.TimeStamp `xorm:"created"`
 	UpdatedUnix               timeutil.TimeStamp `xorm:"updated"`
 }
@@ -166,6 +167,23 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
 	return approvals
 }
 
+// MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews
+func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool {
+	if !protectBranch.BlockOnRejectedReviews {
+		return false
+	}
+	rejectExist, err := x.Where("issue_id = ?", pr.IssueID).
+		And("type = ?", ReviewTypeReject).
+		And("official = ?", true).
+		Exist(new(Review))
+	if err != nil {
+		log.Error("MergeBlockedByRejectedReview: %v", err)
+		return true
+	}
+
+	return rejectExist
+}
+
 // GetProtectedBranchByRepoID getting protected branch by repo ID
 func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
 	protectedBranches := make([]*ProtectedBranch, 0)
@@ -340,7 +358,7 @@ func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName
 	if err != nil {
 		return true, err
 	} else if has {
-		return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr), nil
+		return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil
 	}
 
 	return false, nil
diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index e8bb3f16d4..73c9bc1138 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -288,6 +288,8 @@ var migrations = []Migration{
 	NewMigration("add user_id prefix to existing user avatar name", renameExistingUserAvatarName),
 	// v116 -> v117
 	NewMigration("Extend TrackedTimes", extendTrackedTimes),
+	// v117 -> v118
+	NewMigration("Add block on rejected reviews branch protection", addBlockOnRejectedReviews),
 }
 
 // Migrate database to current version
diff --git a/models/migrations/v117.go b/models/migrations/v117.go
new file mode 100644
index 0000000000..662d6c7b46
--- /dev/null
+++ b/models/migrations/v117.go
@@ -0,0 +1,17 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func addBlockOnRejectedReviews(x *xorm.Engine) error {
+	type ProtectedBranch struct {
+		BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"`
+	}
+
+	return x.Sync2(new(ProtectedBranch))
+}
diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go
index 26ed2bb692..c87549af92 100644
--- a/modules/auth/repo_form.go
+++ b/modules/auth/repo_form.go
@@ -171,6 +171,7 @@ type ProtectBranchForm struct {
 	EnableApprovalsWhitelist bool
 	ApprovalsWhitelistUsers  string
 	ApprovalsWhitelistTeams  string
+	BlockOnRejectedReviews   bool
 }
 
 // Validate validates the fields
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 6911904271..712cd7ca41 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1054,6 +1054,7 @@ pulls.is_checking = "Merge conflict checking is in progress. Try again in few mo
 pulls.required_status_check_failed = Some required checks were not successful.
 pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
 pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
+pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer."
 pulls.can_auto_merge_desc = This pull request can be merged automatically.
 pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts.
 pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
@@ -1417,6 +1418,8 @@ settings.update_protect_branch_success = Branch protection for branch '%s' has b
 settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled.
 settings.protected_branch_deletion = Disable Branch Protection
 settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue?
+settings.block_rejected_reviews = Block merge on rejected reviews
+settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, even if there are enough approvals.
 settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
 settings.choose_branch = Choose a branch…
 settings.no_protected_branch = There are no protected branches.
diff --git a/routers/private/hook.go b/routers/private/hook.go
index dc5001ad4e..c1e283f357 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -113,6 +113,13 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 					})
 					return
 				}
+				if protectBranch.MergeBlockedByRejectedReview(pr) {
+					log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d has requested changes", opts.UserID, branchName, repo, pr.Index)
+					ctx.JSON(http.StatusForbidden, map[string]interface{}{
+						"err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d has requested changes", branchName, opts.ProtectedBranchID),
+					})
+					return
+				}
 			} else if !canPush {
 				log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo)
 				ctx.JSON(http.StatusForbidden, map[string]interface{}{
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 925903fee4..46020acb6d 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -962,7 +962,8 @@ func ViewIssue(ctx *context.Context) {
 		}
 		if pull.ProtectedBranch != nil {
 			cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
-			ctx.Data["IsBlockedByApprovals"] = pull.ProtectedBranch.RequiredApprovals > 0 && cnt < pull.ProtectedBranch.RequiredApprovals
+			ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull)
+			ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull)
 			ctx.Data["GrantedApprovals"] = cnt
 		}
 		ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go
index c279c94b1b..8872c7471f 100644
--- a/routers/repo/setting_protected_branch.go
+++ b/routers/repo/setting_protected_branch.go
@@ -244,6 +244,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
 				approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ","))
 			}
 		}
+		protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews
 
 		err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
 			UserIDs:          whitelistUsers,
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index 3503a51742..cec10a620e 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -41,6 +41,7 @@
 	{{else if .IsFilesConflicted}}grey
 	{{else if .IsPullRequestBroken}}red
 	{{else if .IsBlockedByApprovals}}red
+	{{else if .IsBlockedByRejection}}red
 	{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red
 	{{else if .Issue.PullRequest.IsChecking}}yellow
 	{{else if .Issue.PullRequest.CanAutoMerge}}green
@@ -100,6 +101,11 @@
 					<span class="octicon octicon-x"></span>
 				{{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}}
 				</div>
+			{{else if .IsBlockedByRejection}}
+				<div class="item text red">
+					<span class="octicon octicon-x"></span>
+				{{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
+				</div>			
 			{{else if .Issue.PullRequest.IsChecking}}
 				<div class="item text yellow">
 					<span class="octicon octicon-sync"></span>
diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl
index b6305861af..bf74a12330 100644
--- a/templates/repo/settings/protected_branch.tmpl
+++ b/templates/repo/settings/protected_branch.tmpl
@@ -204,6 +204,13 @@
 							</div>
 						{{end}}
 					</div>
+					<div class="field">
+						<div class="ui checkbox">
+							<input name="block_on_rejected_reviews" type="checkbox" {{if .Branch.BlockOnRejectedReviews}}checked{{end}}>
+							<label for="block_on_rejected_reviews">{{.i18n.Tr "repo.settings.block_rejected_reviews"}}</label>
+							<p class="help">{{.i18n.Tr "repo.settings.block_rejected_reviews_desc"}}</p>
+						</div>
+					</div>					
 				</div>
 
 				<div class="ui divider"></div>