From dd525f848fb9ee5d9279bc192d9302d4752d4223 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 14:26:17 +0100 Subject: [PATCH 01/10] Allow repo admin to merge even if review is not ok. --- models/branches.go | 21 ------ models/pull.go | 25 ------- models/repo_permission.go | 2 +- modules/auth/repo_form.go | 1 + options/locale/locale_en-US.ini | 2 +- routers/api/v1/repo/pull.go | 29 +++++--- routers/private/hook.go | 25 ++++--- routers/repo/issue.go | 16 ++++- routers/repo/pull.go | 32 ++++++--- services/pull/merge.go | 43 ++++++++++-- templates/repo/issue/view_content/pull.tmpl | 76 ++++++++++++--------- 11 files changed, 160 insertions(+), 112 deletions(-) diff --git a/models/branches.go b/models/branches.go index 385817e4f982b..601a92c1def7b 100644 --- a/models/branches.go +++ b/models/branches.go @@ -343,27 +343,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 9a8777aca3037..94e4c955e14c4 100644 --- a/models/pull.go +++ b/models/pull.go @@ -608,31 +608,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 79d7dd012b66a..374c6f8d56180 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 c87549af92a20..1003ef600f361 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -447,6 +447,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/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 00221573d0dab..32a31cff46684 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1062,7 +1062,7 @@ 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 status of reviews and status checks. 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 d0551320fdbc7..b77535b595663 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -583,6 +583,11 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } pr.Issue.Repo = ctx.Repo.Repository + err = pr.LoadProtectedBranch() + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadProtectedBranch", err) + return + } if ctx.IsSigned { // Update issue-user. @@ -597,20 +602,28 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() { - ctx.Status(http.StatusMethodNotAllowed) + if !pr.ProtectedBranch.CanUserMerge(ctx.User.ID) { + ctx.Error(http.StatusMethodNotAllowed, "Merge", "Not in merge whitelist") return } - isPass, err := pull_service.IsPullCommitStatusPass(pr) - if err != nil { - ctx.Error(http.StatusInternalServerError, "IsPullCommitStatusPass", err) + if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() { + ctx.Status(http.StatusMethodNotAllowed) return } - if !isPass && !ctx.IsUserRepoAdmin() { - ctx.Status(http.StatusMethodNotAllowed) - return + if err := pull_service.CheckPrReadyToMerge(pr); err != nil { + 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 { diff --git a/routers/private/hook.go b/routers/private/hook.go index c1e283f357ecb..76803106f671e 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,19 +108,26 @@ 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) + if !protectBranch.CanUserMerge(opts.UserID) { + log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and 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 does not have enough approvals", branchName, opts.ProtectedBranchID), + "err": fmt.Sprintf("protected branch %s can not be pushed to", branchName), }) 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), + // 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 cannot push to protected branch %s in %-v and pr #%d: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("protected branch %s can not be pushed to 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 PullRequest %d Error: %v", opts.ProtectedBranchID, err), }) - return } } else if !canPush { log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 46020acb6dfbc..763faafd01675 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -931,14 +931,24 @@ func ViewIssue(ctx *context.Context) { return } prConfig := prUnit.PullRequestsConfig() + err = pull.LoadProtectedBranch() + if err != nil { + ctx.ServerError("LoadProtectedBranch", err) + return + } - ctx.Data["AllowMerge"] = ctx.Repo.CanWrite(models.UnitTypeCode) - if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil { + ctx.Data["AllowMerge"] = pull.ProtectedBranch.CanUserMerge(ctx.User.ID) + if err := pull_service.CheckPrReadyToMerge(pull); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError("CheckUserAllowedToMerge", err) return } - ctx.Data["AllowMerge"] = false + if isRepoAdmin, err := models.IsUserRepoAdmin(pull.BaseRepo, ctx.User); err != nil { + ctx.ServerError("IsUserRepoAdmin", err) + return + } else if !isRepoAdmin { + ctx.Data["AllowMerge"] = false + } } // Check correct values and select default diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 418c2e9438bd5..478144af5b990 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -580,6 +580,16 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { } pr := issue.PullRequest + err := pr.LoadProtectedBranch() + if err != nil { + ctx.ServerError("LoadProtectedBranch", err) + return + } + + if !pr.ProtectedBranch.CanUserMerge(ctx.User.ID) { + ctx.NotFound("MergePullRequest", nil) + return + } if !pr.CanAutoMerge() || pr.HasMerged { ctx.NotFound("MergePullRequest", nil) @@ -592,15 +602,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 8aa38bf11eab7..6fdc2cd048fff 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -30,6 +30,7 @@ import ( ) // Merge merges pull request to base repository. +// Caller chall 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} @@ -471,3 +467,40 @@ func getDiffTree(repoPath, baseBranch, headBranch string) (string, error) { return out.String(), 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) + } + } + + isPass, err := IsPullCommitStatusPass(pr) + if err != nil { + return err + } + if !isPass { + return models.ErrNotAllowedToMerge{ + Reason: "Not all required status checks successfull", + } + } + + 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 cec10a620ea62..be2b6fe3deaa2 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -96,35 +96,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"}} @@ -211,7 +207,7 @@
{{end}} -
+
From 4f4dd9fd010cf71dd2797164eef4ecc8641078bc Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 15:49:54 +0100 Subject: [PATCH 02/10] generate swagger --- services/pull/merge.go | 2 +- templates/swagger/v1_json.tmpl | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 6fdc2cd048fff..d7af979b38632 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -487,7 +487,7 @@ func CheckPrReadyToMerge(pr *models.PullRequest) (err error) { } if !isPass { return models.ErrNotAllowedToMerge{ - Reason: "Not all required status checks successfull", + Reason: "Not all required status checks successful", } } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index f31b37cc4527d..c4ee9b9efcb7e 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -10337,6 +10337,10 @@ }, "MergeTitleField": { "type": "string" + }, + "force_merge": { + "type": "boolean", + "x-go-name": "ForceMerge" } }, "x-go-name": "MergePullRequestForm", From a41de7e47f6a81c0e543346aa2d9532382775ada Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 19:41:33 +0100 Subject: [PATCH 03/10] more refactoring and fixes --- models/branches.go | 4 ++-- modules/convert/convert.go | 2 +- routers/api/v1/repo/pull.go | 20 +++++++++++++------- routers/private/hook.go | 26 +++++++++++++++++++++++++- routers/repo/issue.go | 7 ++++++- routers/repo/pull.go | 8 ++++---- services/pull/merge.go | 21 +++++++++++++++++++++ 7 files changed, 72 insertions(+), 16 deletions(-) diff --git a/models/branches.go b/models/branches.go index 601a92c1def7b..251d95b675711 100644 --- a/models/branches.go +++ b/models/branches.go @@ -91,8 +91,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 } diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 4479653174ca7..a69b09a2b703c 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/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index b77535b595663..fe263a0adcd19 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -583,11 +583,6 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } pr.Issue.Repo = ctx.Repo.Repository - err = pr.LoadProtectedBranch() - if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadProtectedBranch", err) - return - } if ctx.IsSigned { // Update issue-user. @@ -602,8 +597,19 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if !pr.ProtectedBranch.CanUserMerge(ctx.User.ID) { - ctx.Error(http.StatusMethodNotAllowed, "Merge", "Not in merge whitelist") + perm, err := models.GetUserRepoPermission(ctx.Repo.Repository, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + return + } + + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err) + return + } + if allowedMerge { + ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR") return } diff --git a/routers/private/hook.go b/routers/private/hook.go index 76803106f671e..29b86fc007f17 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -108,7 +108,31 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } - if !protectBranch.CanUserMerge(opts.UserID) { + 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 + } + 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 cannot push to protected branch: %s in %-v and 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", branchName), diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 763faafd01675..55e97fb3f6393 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 { @@ -922,6 +923,11 @@ func ViewIssue(ctx *context.Context) { ctx.Data["DeleteBranchLink"] = ctx.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index) + "/cleanup" } } + ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.User) + if err != nil { + ctx.ServerError("IsUserAllowedToMerge", err) + return + } } } @@ -937,7 +943,6 @@ func ViewIssue(ctx *context.Context) { return } - ctx.Data["AllowMerge"] = pull.ProtectedBranch.CanUserMerge(ctx.User.ID) if err := pull_service.CheckPrReadyToMerge(pull); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError("CheckUserAllowedToMerge", err) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 478144af5b990..24e0def020577 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -580,13 +580,13 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { } pr := issue.PullRequest - err := pr.LoadProtectedBranch() + + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User) if err != nil { - ctx.ServerError("LoadProtectedBranch", err) + ctx.ServerError("IsUserAllowedToMerge", err) return } - - if !pr.ProtectedBranch.CanUserMerge(ctx.User.ID) { + if !allowedMerge { ctx.NotFound("MergePullRequest", nil) return } diff --git a/services/pull/merge.go b/services/pull/merge.go index d7af979b38632..9540ec0c749e3 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -468,6 +468,27 @@ 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 { From 3799eb723dc14153286306799fd4b77d8b6680c0 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 23:18:59 +0100 Subject: [PATCH 04/10] fix --- routers/api/v1/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index fe263a0adcd19..1de82de15ce1f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -608,7 +608,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err) return } - if allowedMerge { + if !allowedMerge { ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR") return } From c5898e3952179af425152f65a815eee50665cea2 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 23:37:13 +0100 Subject: [PATCH 05/10] fix --- services/pull/merge.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/pull/merge.go b/services/pull/merge.go index 9540ec0c749e3..326c00686b875 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -500,6 +500,9 @@ func CheckPrReadyToMerge(pr *models.PullRequest) (err error) { if err = pr.LoadProtectedBranch(); err != nil { return fmt.Errorf("LoadProtectedBranch: %v", err) } + if pr.ProtectedBranch == nil { + return nil + } } isPass, err := IsPullCommitStatusPass(pr) From 9aeedbbc10c1f13b71ba0dbed9a8e8c8df9af0fe Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 6 Jan 2020 01:20:14 +0100 Subject: [PATCH 06/10] fix --- routers/repo/issue.go | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 55e97fb3f6393..c9c469000a7c9 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -923,11 +923,20 @@ func ViewIssue(ctx *context.Context) { ctx.Data["DeleteBranchLink"] = ctx.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index) + "/cleanup" } } - ctx.Data["AllowMerge"], err = pull_service.IsUserAllowedToMerge(pull, perm, ctx.User) - if err != nil { - ctx.ServerError("IsUserAllowedToMerge", err) - return - } + } + + 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 } } @@ -937,24 +946,6 @@ func ViewIssue(ctx *context.Context) { return } prConfig := prUnit.PullRequestsConfig() - err = pull.LoadProtectedBranch() - if err != nil { - ctx.ServerError("LoadProtectedBranch", err) - return - } - - if err := pull_service.CheckPrReadyToMerge(pull); err != nil { - if !models.IsErrNotAllowedToMerge(err) { - ctx.ServerError("CheckUserAllowedToMerge", err) - return - } - if isRepoAdmin, err := models.IsUserRepoAdmin(pull.BaseRepo, ctx.User); err != nil { - ctx.ServerError("IsUserRepoAdmin", err) - return - } else if !isRepoAdmin { - ctx.Data["AllowMerge"] = false - } - } // Check correct values and select default if ms, ok := ctx.Data["MergeStyle"].(models.MergeStyle); !ok || From 5f91064a2dd45425fcdb315399990cbed03ce93c Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 6 Jan 2020 01:39:12 +0100 Subject: [PATCH 07/10] Show info if user is not authorized to merge. --- options/locale/locale_en-US.ini | 1 + templates/repo/issue/view_content/pull.tmpl | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 32a31cff46684..f67fa886f385b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1063,6 +1063,7 @@ pulls.no_merge_desc = This pull request cannot be merged because all repository 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_not_ready = This pull request is not ready to be merged, check status of reviews 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/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index be2b6fe3deaa2..633281f476694 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -253,6 +253,11 @@ {{$.i18n.Tr "repo.pulls.no_merge_helper"}} {{end}} + {{else}} +
+ + {{$.i18n.Tr "repo.pulls.no_merge_access"}} +
{{end}} {{end}} {{else}} From 453e92ed3c98fb58012bde621fa6f46046d727c7 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 6 Jan 2020 11:41:38 +0100 Subject: [PATCH 08/10] Fix review comments --- routers/private/hook.go | 4 ++-- services/pull/merge.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/routers/private/hook.go b/routers/private/hook.go index 29b86fc007f17..b2d9dec7c6681 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -133,7 +133,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { return } if !allowedMerge { - log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index) + log.Warn("Forbidden: User %d can not push to protected branch: %s in %-v and 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", branchName), }) @@ -142,7 +142,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { // 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 cannot push to protected branch %s in %-v and pr #%d: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) + log.Warn("Forbidden: User %d can not push to protected branch %s in %-v and pr #%d: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) ctx.JSON(http.StatusForbidden, map[string]interface{}{ "err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), }) diff --git a/services/pull/merge.go b/services/pull/merge.go index 326c00686b875..de1dc84f90916 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -30,7 +30,7 @@ import ( ) // Merge merges pull request to base repository. -// Caller chall check PR is ready to be merged (review and status checks) +// 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() From 030371a4cfa57f6e6938bc70cbb07d0913c044c5 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 7 Jan 2020 22:11:45 +0100 Subject: [PATCH 09/10] Reformulate error messages. --- routers/private/hook.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/routers/private/hook.go b/routers/private/hook.go index b2d9dec7c6681..61d8ee197abda 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -133,30 +133,30 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { return } if !allowedMerge { - log.Warn("Forbidden: User %d can not push to protected branch: %s in %-v and not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index) + 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", branchName), + "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 can not push to protected branch %s in %-v and pr #%d: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) + 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("protected branch %s can not be pushed to and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), + "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 PullRequest %d Error: %v", opts.ProtectedBranchID, err), + "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 } From 4d96079adcb25ad5c908124cfb352696992b4ec5 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 9 Jan 2020 07:38:55 +0100 Subject: [PATCH 10/10] Fix review comments --- options/locale/locale_en-US.ini | 2 +- routers/api/v1/repo/pull.go | 6 +++++- routers/private/hook.go | 2 +- routers/repo/pull.go | 2 +- services/pull/merge.go | 4 ++-- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index b71b1c7ad5d6e..47811f76d4238 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1062,7 +1062,7 @@ 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_not_ready = This pull request is not ready to be merged, check status of reviews and status checks. +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 diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1de82de15ce1f..e60b46e7fc59a 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -618,7 +618,11 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if err := pull_service.CheckPrReadyToMerge(pr); err != nil { + 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) diff --git a/routers/private/hook.go b/routers/private/hook.go index 61d8ee197abda..b4626fddf4272 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -140,7 +140,7 @@ 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 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{}{ diff --git a/routers/repo/pull.go b/routers/repo/pull.go index bad266227143f..e95ce2e6449d5 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -621,7 +621,7 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { return } - if err := pull_service.CheckPrReadyToMerge(pr); err != nil { + if err := pull_service.CheckPRReadyToMerge(pr); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError("Merge PR status", err) return diff --git a/services/pull/merge.go b/services/pull/merge.go index de1dc84f90916..b5b4064bdce08 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -489,8 +489,8 @@ func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *mod return false, nil } -// CheckPrReadyToMerge checks whether the PR is ready to be merged (reviews and status checks) -func CheckPrReadyToMerge(pr *models.PullRequest) (err error) { +// 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)