linden123/githaven-fork
1
1
Fork 0
forked from Shiloh/githaven
Code Pull Requests Activity

Fix admin handling at merge of PR (#9749)

Browse Source 
* 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>
This commit is contained in:
David Svantesson 2020-01-16 22:01:22 +01:00 committed by Lauris BH
parent d3468ed79f
commit 18e0447b3f
3 changed files with 18 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
routers/private/hook.go
View File

@ -224,7 +224,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
canPush = protectBranch.CanUserPush(opts.UserID) canPush = protectBranch.CanUserPush(opts.UserID)
} }
if !canPush && opts.ProtectedBranchID > 0 { if !canPush && opts.ProtectedBranchID > 0 {
// Manual merge // Merge (from UI or API)
pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
if err != nil { if err != nil {
log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) 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 return
} }
// Manual merge only allowed if PR is ready (even if admin) // Check all status checks and reviews is ok, unless repo admin which can bypass this.
if err := pull_service.CheckPRReadyToMerge(pr); err != nil { if !perm.IsAdmin() {
if models.IsErrNotAllowedToMerge(err) { if err := pull_service.CheckPRReadyToMerge(pr); err != nil {
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()) if models.IsErrNotAllowedToMerge(err) {
ctx.JSON(http.StatusForbidden, map[string]interface{}{ 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())
"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()), 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 { } else if !canPush {
log.Warn("Forbidden: User %d is not allowed to 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)

3
services/pull/merge.go
View File

@ -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 // 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) { func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) {
if p.IsAdmin() {
return true, nil
}
if !p.CanWrite(models.UnitTypeCode) { if !p.CanWrite(models.UnitTypeCode) {
return false, nil return false, nil
} }

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

@ -133,9 +133,9 @@
{{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
</div> </div>
{{end}} {{end}}
{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
{{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}} {{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .RequireSigned) .WillSign)}}
{{if $notAllOk}} {{if $notAllOverridableChecksOk}}
<div class="item text yellow"> <div class="item text yellow">
<i class="icon icon-octicon"><span class="octicon octicon-primitive-dot"></span></i> <i class="icon icon-octicon"><span class="octicon octicon-primitive-dot"></span></i>
{{$.i18n.Tr "repo.pulls.required_status_check_administrator"}} {{$.i18n.Tr "repo.pulls.required_status_check_administrator"}}
@ -233,7 +233,7 @@
</form> </form>
</div> </div>
{{end}} {{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}}"> <button class="ui button" data-do="{{.MergeStyle}}">
<span class="octicon octicon-git-merge"></span> <span class="octicon octicon-git-merge"></span>
<span class="button-text"> <span class="button-text">