From 18e0447b3f65cb6aab2eec6b742edf911773097f Mon Sep 17 00:00:00 2001
From: David Svantesson <davidsvantesson@gmail.com>
Date: Thu, 16 Jan 2020 22:01:22 +0100
Subject: [PATCH] Fix admin handling at merge of PR (#9749)

* Admin shall be able to bypass merge checks.

* Repository admin should not bypass if merge whitelist is set.

* Add code comment about checks that PR are ready

* notAllOverrideableChecksOk->notAllOverridableChecksOk

* Fix merge, require signed currently not overridable.

* fix

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
---
 routers/private/hook.go                     | 26 +++++++++++----------
 services/pull/merge.go                      |  3 ---
 templates/repo/issue/view_content/pull.tmpl |  8 +++----
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/routers/private/hook.go b/routers/private/hook.go
index 6a07de15ff9..7044fdac221 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -224,7 +224,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 				canPush = protectBranch.CanUserPush(opts.UserID)
 			}
 			if !canPush && opts.ProtectedBranchID > 0 {
-				// Manual merge
+				// Merge (from UI or API)
 				pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
 				if err != nil {
 					log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
@@ -264,19 +264,21 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 					})
 					return
 				}
-				// Manual merge only allowed if PR is ready (even if admin)
-				if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
-					if models.IsErrNotAllowedToMerge(err) {
-						log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
-						ctx.JSON(http.StatusForbidden, map[string]interface{}{
-							"err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()),
+				// Check all status checks and reviews is ok, unless repo admin which can bypass this.
+				if !perm.IsAdmin() {
+					if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
+						if models.IsErrNotAllowedToMerge(err) {
+							log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
+							ctx.JSON(http.StatusForbidden, map[string]interface{}{
+								"err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()),
+							})
+							return
+						}
+						log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
+						ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
+							"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
 						})
-						return
 					}
-					log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
-					ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
-						"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
-					})
 				}
 			} else if !canPush {
 				log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
diff --git a/services/pull/merge.go b/services/pull/merge.go
index 5e077f6dc02..b423c663ea6 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -487,9 +487,6 @@ func IsSignedIfRequired(pr *models.PullRequest, doer *models.User) (bool, error)
 
 // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
 func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) {
-	if p.IsAdmin() {
-		return true, nil
-	}
 	if !p.CanWrite(models.UnitTypeCode) {
 		return false, nil
 	}
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index 115ea2d119e..f8a82f1a0fc 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -133,9 +133,9 @@
 						{{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
 					</div>
 				{{end}}
-				{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
-				{{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}}
-					{{if $notAllOk}}
+				{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
+				{{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .RequireSigned) .WillSign)}}
+					{{if $notAllOverridableChecksOk}}
 						<div class="item text yellow">
 							<i class="icon icon-octicon"><span class="octicon octicon-primitive-dot"></span></i>
 							{{$.i18n.Tr "repo.pulls.required_status_check_administrator"}}
@@ -233,7 +233,7 @@
 								</form>
 							</div>
 							{{end}}
-							<div class="ui {{if $notAllOk}}red{{else}}green{{end}} buttons merge-button">
+							<div class="ui {{if $notAllOverridableChecksOk}}red{{else}}green{{end}} buttons merge-button">
 								<button class="ui button" data-do="{{.MergeStyle}}">
 									<span class="octicon octicon-git-merge"></span>
 									<span class="button-text">