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>

1 files changed, 14 insertions, 12 deletions

diff --git a/routers/private/hook.go b/routers/private/hook.go
index 6a07de15ff..7044fdac22 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)