p85947160
/
gitea
1
0
Fork 1
Code Issues Pull Requests 1 Projects Releases Wiki Activity

Allow repo admin to merge PR regardless of review status (#9611) 

* Allow repo admin to merge even if review is not ok.
This commit is contained in:
David Svantesson 2020-01-11 08:29:34 +01:00 committed by techknowlogick
parent 4d06d10dba
commit 32fb813133
13 changed files with 236 additions and 124 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
models/branches.go
View File

@ -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) {

25
models/pull.go
View File

@ -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 {

2
models/repo_permission.go
View File

@ -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)
}

1
modules/auth/repo_form.go
View File

@ -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

2
modules/convert/convert.go
View File

@ -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),
}
}

3
options/locale/locale_en-US.ini
View File

@ -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)

41
routers/api/v1/repo/pull.go
View File

@ -600,20 +600,43 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
return
}
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
}
if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
ctx.Status(http.StatusMethodNotAllowed)
return
}
isPass, err := pull_service.IsPullCommitStatusPass(pr)
if err != nil {
ctx.Error(http.StatusInternalServerError, "IsPullCommitStatusPass", err)
return
}
if !isPass && !ctx.IsUserRepoAdmin() {
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 {

53
routers/private/hook.go
View File

@ -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
}
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),
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 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("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
}

24
routers/repo/issue.go
View File

@ -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) {

32
routers/repo/pull.go
View File

@ -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() {

67
services/pull/merge.go
View File

@ -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
}

81
templates/repo/issue/view_content/pull.tmpl
View File

@ -101,35 +101,31 @@
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}}
</div>
{{else if .IsBlockedByApprovals}}
<div class="item text red">
<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>
{{$.i18n.Tr "repo.pulls.is_checking"}}
</div>
{{else if and (not .Issue.PullRequest.CanAutoMerge) .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
</div>
{{else if .Issue.PullRequest.CanAutoMerge}}
{{if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
</div>
{{if .IsBlockedByApprovals}}
<div class="item text red">
<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 and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
</div>
{{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}}
<div class="item text yellow">
<span class="octicon octicon-primitive-dot"></span>
{{$.i18n.Tr "repo.pulls.required_status_check_administrator"}}
@ -216,7 +212,7 @@
</form>
</div>
{{end}}
<div class="ui green buttons merge-button">
<div class="ui {{if $notAllOk}}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">
@ -262,17 +258,40 @@
{{$.i18n.Tr "repo.pulls.no_merge_helper"}}
</div>
{{end}}
{{else}}
<div class="item text grey">
<span class="octicon octicon-info"></span>
{{$.i18n.Tr "repo.pulls.no_merge_access"}}
</div>
{{end}}
{{end}}
{{else}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.cannot_auto_merge_desc"}}
</div>
<div class="item text grey">
<span class="octicon octicon-info"></span>
{{$.i18n.Tr "repo.pulls.cannot_auto_merge_helper"}}
</div>
{{/* Merge conflict without specific file. Suggest manual merge, only if all reviews and status checks OK. */}}
{{if .IsBlockedByApprovals}}
<div class="item text red">
<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 and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
</div>
{{else}}
<div class="item text red">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.cannot_auto_merge_desc"}}
</div>
<div class="item text grey">
<span class="octicon octicon-info"></span>
{{$.i18n.Tr "repo.pulls.cannot_auto_merge_helper"}}
</div>
{{end}}
{{end}}
</div>
</div>

4
templates/swagger/v1_json.tmpl
View File

@ -10745,6 +10745,10 @@
},
"MergeTitleField": {
"type": "string"
},
"force_merge": {
"type": "boolean",
"x-go-name": "ForceMerge"
}
},
"x-go-name": "MergePullRequestForm",