From 32fb813133c74ddc3af1964e81fff72fea4f24f1 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 11 Jan 2020 08:29:34 +0100 Subject: [PATCH] Allow repo admin to merge PR regardless of review status (#9611) * Allow repo admin to merge even if review is not ok. --- models/branches.go | 25 +------ models/pull.go | 25 ------- models/repo_permission.go | 2 +- modules/auth/repo_form.go | 1 + modules/convert/convert.go | 2 +- options/locale/locale_en-US.ini | 3 +- routers/api/v1/repo/pull.go | 33 +++++++-- routers/private/hook.go | 51 ++++++++++--- routers/repo/issue.go | 24 +++--- routers/repo/pull.go | 32 +++++--- services/pull/merge.go | 67 +++++++++++++++-- templates/repo/issue/view_content/pull.tmpl | 81 +++++++++++++-------- templates/swagger/v1_json.tmpl | 4 + 13 files changed, 231 insertions(+), 119 deletions(-) diff --git a/models/branches.go b/models/branches.go index 1932e06db..b6398f569 100644 --- a/models/branches.go +++ b/models/branches.go @@ -93,8 +93,8 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool { return in } -// CanUserMerge returns if some user could merge a pull request to this protected branch -func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool { +// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch +func (protectBranch *ProtectedBranch) IsUserMergeWhitelisted(userID int64) bool { if !protectBranch.EnableMergeWhitelist { return true } @@ -348,27 +348,6 @@ func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User) return false, nil } -// IsProtectedBranchForMerging checks if branch is protected for merging -func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName string, doer *User) (bool, error) { - if doer == nil { - return true, nil - } - - protectedBranch := &ProtectedBranch{ - RepoID: repo.ID, - BranchName: branchName, - } - - has, err := x.Get(protectedBranch) - if err != nil { - return true, err - } else if has { - return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil - } - - return false, nil -} - // updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with // the users from newWhitelist which have explicit read or write access to the repo. func updateApprovalWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { diff --git a/models/pull.go b/models/pull.go index e2a35c592..bf2527679 100644 --- a/models/pull.go +++ b/models/pull.go @@ -479,31 +479,6 @@ const ( MergeStyleSquash MergeStyle = "squash" ) -// CheckUserAllowedToMerge checks whether the user is allowed to merge -func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) { - if doer == nil { - return ErrNotAllowedToMerge{ - "Not signed in", - } - } - - if pr.BaseRepo == nil { - if err = pr.GetBaseRepo(); err != nil { - return fmt.Errorf("GetBaseRepo: %v", err) - } - } - - if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr, pr.BaseBranch, doer); err != nil { - return fmt.Errorf("IsProtectedBranch: %v", err) - } else if protected { - return ErrNotAllowedToMerge{ - "The branch is protected", - } - } - - return nil -} - // SetMerged sets a pull request to merged and closes the corresponding issue func (pr *PullRequest) SetMerged() (err error) { if pr.HasMerged { diff --git a/models/repo_permission.go b/models/repo_permission.go index 79d7dd012..374c6f8d5 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -271,7 +271,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss return } -// IsUserRepoAdmin return ture if user has admin right of a repo +// IsUserRepoAdmin return true if user has admin right of a repo func IsUserRepoAdmin(repo *Repository, user *User) (bool, error) { return isUserRepoAdmin(x, repo, user) } diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 28615ebbc..0086419b6 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -448,6 +448,7 @@ type MergePullRequestForm struct { Do string `binding:"Required;In(merge,rebase,rebase-merge,squash)"` MergeTitleField string MergeMessageField string + ForceMerge *bool `json:"force_merge,omitempty"` } // Validate validates the fields diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 447965317..a69b09a2b 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -51,7 +51,7 @@ func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models. EnableStatusCheck: bp.EnableStatusCheck, StatusCheckContexts: bp.StatusCheckContexts, UserCanPush: bp.CanUserPush(user.ID), - UserCanMerge: bp.CanUserMerge(user.ID), + UserCanMerge: bp.IsUserMergeWhitelisted(user.ID), } } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index a00df07d9..4dc0b9223 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1062,7 +1062,8 @@ pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled. pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually. pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress. -pulls.no_merge_status_check = This pull request cannot be merged because not all required status checkes are successful. +pulls.no_merge_not_ready = This pull request is not ready to be merged, check review status and status checks. +pulls.no_merge_access = You are not authorized to merge this pull request. pulls.merge_pull_request = Merge Pull Request pulls.rebase_merge_pull_request = Rebase and Merge pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 8e6677116..6b643371e 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -600,22 +600,45 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() { - ctx.Status(http.StatusMethodNotAllowed) + perm, err := models.GetUserRepoPermission(ctx.Repo.Repository, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) return } - isPass, err := pull_service.IsPullCommitStatusPass(pr) + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, ctx.User) if err != nil { - ctx.Error(http.StatusInternalServerError, "IsPullCommitStatusPass", err) + ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err) + return + } + if !allowedMerge { + ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR") return } - if !isPass && !ctx.IsUserRepoAdmin() { + if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() { ctx.Status(http.StatusMethodNotAllowed) return } + if err := pull_service.CheckPRReadyToMerge(pr); err != nil { + if !models.IsErrNotAllowedToMerge(err) { + ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err) + return + } + if form.ForceMerge != nil && *form.ForceMerge { + if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil { + ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err) + return + } else if !isRepoAdmin { + ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)") + } + } else { + ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err) + return + } + } + if len(form.Do) == 0 { form.Do = string(models.MergeStyleMerge) } diff --git a/routers/private/hook.go b/routers/private/hook.go index c1e283f35..b4626fddf 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/modules/private" "code.gitea.io/gitea/modules/repofiles" "code.gitea.io/gitea/modules/util" + pull_service "code.gitea.io/gitea/services/pull" "gitea.com/macaron/macaron" ) @@ -98,6 +99,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { canPush = protectBranch.CanUserPush(opts.UserID) } if !canPush && opts.ProtectedBranchID > 0 { + // Manual merge pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) if err != nil { log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) @@ -106,24 +108,55 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } - if !protectBranch.HasEnoughApprovals(pr) { - log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d does not have enough approvals", 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 does not have enough approvals", branchName, opts.ProtectedBranchID), + user, err := models.GetUserByID(opts.UserID) + if err != nil { + log.Error("Unable to get User id %d Error: %v", opts.UserID, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err), + }) + return + } + perm, err := models.GetUserRepoPermission(repo, user) + if err != nil { + log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err), }) 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) + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user) + if err != nil { + log.Error("Error calculating if allowed to merge: %v", err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err), + }) + return + } + if !allowedMerge { + log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", 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), + "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), }) 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()), + }) + 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 cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo) + log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo) ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("protected branch %s can not be pushed to", branchName), + "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), }) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 0a78e06b4..ce3eb5bd2 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -903,6 +903,7 @@ func ViewIssue(ctx *context.Context) { pull := issue.PullRequest pull.Issue = issue canDelete := false + ctx.Data["AllowMerge"] = false if ctx.IsSigned { if err := pull.GetHeadRepo(); err != nil { @@ -923,6 +924,20 @@ func ViewIssue(ctx *context.Context) { } } } + + if err := pull.GetBaseRepo(); err != nil { + log.Error("GetBaseRepo: %v", err) + } + perm, err := models.GetUserRepoPermission(pull.BaseRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return + } + ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.User) + if err != nil { + ctx.ServerError("IsUserAllowedToMerge", err) + return + } } prUnit, err := repo.GetUnit(models.UnitTypePullRequests) @@ -932,15 +947,6 @@ func ViewIssue(ctx *context.Context) { } prConfig := prUnit.PullRequestsConfig() - ctx.Data["AllowMerge"] = ctx.Repo.CanWrite(models.UnitTypeCode) - if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil { - if !models.IsErrNotAllowedToMerge(err) { - ctx.ServerError("CheckUserAllowedToMerge", err) - return - } - ctx.Data["AllowMerge"] = false - } - // Check correct values and select default if ms, ok := ctx.Data["MergeStyle"].(models.MergeStyle); !ok || !prConfig.IsMergeStyleAllowed(ms) { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 93e98e0c3..901ab4885 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -600,6 +600,16 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { pr := issue.PullRequest + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User) + if err != nil { + ctx.ServerError("IsUserAllowedToMerge", err) + return + } + if !allowedMerge { + ctx.NotFound("MergePullRequest", nil) + return + } + if !pr.CanAutoMerge() || pr.HasMerged { ctx.NotFound("MergePullRequest", nil) return @@ -611,15 +621,19 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { return } - isPass, err := pull_service.IsPullCommitStatusPass(pr) - if err != nil { - ctx.ServerError("IsPullCommitStatusPass", err) - return - } - if !isPass && !ctx.IsUserRepoAdmin() { - ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_status_check")) - ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) - return + if err := pull_service.CheckPRReadyToMerge(pr); err != nil { + if !models.IsErrNotAllowedToMerge(err) { + ctx.ServerError("Merge PR status", err) + return + } + if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil { + ctx.ServerError("IsUserRepoAdmin", err) + return + } else if !isRepoAdmin { + ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) + ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) + return + } } if ctx.HasError() { diff --git a/services/pull/merge.go b/services/pull/merge.go index 7aec7cef3..e825c3fdd 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -30,6 +30,7 @@ import ( ) // Merge merges pull request to base repository. +// Caller should check PR is ready to be merged (review and status checks) // FIXME: add repoWorkingPull make sure two merges does not happen at same time. func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repository, mergeStyle models.MergeStyle, message string) (err error) { binVersion, err := git.BinVersion() @@ -53,11 +54,6 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } prConfig := prUnit.PullRequestsConfig() - if err := pr.CheckUserAllowedToMerge(doer); err != nil { - log.Error("CheckUserAllowedToMerge(%v): %v", doer, err) - return fmt.Errorf("CheckUserAllowedToMerge: %v", err) - } - // Check if merge style is correct and allowed if !prConfig.IsMergeStyleAllowed(mergeStyle) { return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} @@ -473,3 +469,64 @@ func getDiffTree(repoPath, baseBranch, headBranch string) (string, error) { return out.String(), nil } + +// 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 + } + + err := pr.LoadProtectedBranch() + if err != nil { + return false, err + } + + if pr.ProtectedBranch == nil || pr.ProtectedBranch.IsUserMergeWhitelisted(user.ID) { + return true, nil + } + + return false, nil +} + +// CheckPRReadyToMerge checks whether the PR is ready to be merged (reviews and status checks) +func CheckPRReadyToMerge(pr *models.PullRequest) (err error) { + if pr.BaseRepo == nil { + if err = pr.GetBaseRepo(); err != nil { + return fmt.Errorf("GetBaseRepo: %v", err) + } + } + if pr.ProtectedBranch == nil { + if err = pr.LoadProtectedBranch(); err != nil { + return fmt.Errorf("LoadProtectedBranch: %v", err) + } + if pr.ProtectedBranch == nil { + return nil + } + } + + isPass, err := IsPullCommitStatusPass(pr) + if err != nil { + return err + } + if !isPass { + return models.ErrNotAllowedToMerge{ + Reason: "Not all required status checks successful", + } + } + + if enoughApprovals := pr.ProtectedBranch.HasEnoughApprovals(pr); !enoughApprovals { + return models.ErrNotAllowedToMerge{ + Reason: "Does not have enough approvals", + } + } + if rejected := pr.ProtectedBranch.MergeBlockedByRejectedReview(pr); rejected { + return models.ErrNotAllowedToMerge{ + Reason: "There are requested changes", + } + } + + return nil +} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 9dedc7dad..7073312b1 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -101,35 +101,31 @@ {{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}} - {{else if .IsBlockedByApprovals}} -
- - {{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} -
- {{else if .IsBlockedByRejection}} -
- - {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}} -
{{else if .Issue.PullRequest.IsChecking}}
{{$.i18n.Tr "repo.pulls.is_checking"}}
- {{else if and (not .Issue.PullRequest.CanAutoMerge) .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} -
- - {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} -
{{else if .Issue.PullRequest.CanAutoMerge}} - {{if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} -
- - {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} -
+ {{if .IsBlockedByApprovals}} +
+ + {{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} +
+ {{else if .IsBlockedByRejection}} +
+ + {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}} +
+ {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} +
+ + {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} +
{{end}} - {{if or $.IsRepoAdmin (not .EnableStatusCheck) .IsRequiredStatusCheckSuccess}} - {{if and $.IsRepoAdmin .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} + {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} + {{if or $.IsRepoAdmin (not $notAllOk)}} + {{if $notAllOk}}
{{$.i18n.Tr "repo.pulls.required_status_check_administrator"}} @@ -216,7 +212,7 @@
{{end}} -
+
{{end}} + {{else}} +
+ + {{$.i18n.Tr "repo.pulls.no_merge_access"}} +
{{end}} {{end}} {{else}} -
- - {{$.i18n.Tr "repo.pulls.cannot_auto_merge_desc"}} -
-
- - {{$.i18n.Tr "repo.pulls.cannot_auto_merge_helper"}} -
+ {{/* Merge conflict without specific file. Suggest manual merge, only if all reviews and status checks OK. */}} + {{if .IsBlockedByApprovals}} +
+ + {{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} +
+ {{else if .IsBlockedByRejection}} +
+ + {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}} +
+ {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} +
+ + {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} +
+ {{else}} +
+ + {{$.i18n.Tr "repo.pulls.cannot_auto_merge_desc"}} +
+
+ + {{$.i18n.Tr "repo.pulls.cannot_auto_merge_helper"}} +
+ {{end}} {{end}}
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index d0e23a460..ddf144066 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -10745,6 +10745,10 @@ }, "MergeTitleField": { "type": "string" + }, + "force_merge": { + "type": "boolean", + "x-go-name": "ForceMerge" } }, "x-go-name": "MergePullRequestForm",