mirror of https://github.com/go-gitea/gitea.git
Make admins adhere to branch protection rules (#32248)
This introduces a new flag `BlockAdminMergeOverride` on the branch protection rules that prevents admins/repo owners from bypassing branch protection rules and merging without approvals or failing status checks. Fixes #17131 --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Giteabot <teabot@gitea.io>
This commit is contained in:
parent
620f19610e
commit
de2ad2e1b1
|
@ -63,6 +63,7 @@ type ProtectedBranch struct {
|
||||||
RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"`
|
RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"`
|
||||||
ProtectedFilePatterns string `xorm:"TEXT"`
|
ProtectedFilePatterns string `xorm:"TEXT"`
|
||||||
UnprotectedFilePatterns string `xorm:"TEXT"`
|
UnprotectedFilePatterns string `xorm:"TEXT"`
|
||||||
|
BlockAdminMergeOverride bool `xorm:"NOT NULL DEFAULT false"`
|
||||||
|
|
||||||
CreatedUnix timeutil.TimeStamp `xorm:"created"`
|
CreatedUnix timeutil.TimeStamp `xorm:"created"`
|
||||||
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
|
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
|
||||||
|
|
|
@ -603,6 +603,8 @@ var migrations = []Migration{
|
||||||
NewMigration("Add index for release sha1", v1_23.AddIndexForReleaseSha1),
|
NewMigration("Add index for release sha1", v1_23.AddIndexForReleaseSha1),
|
||||||
// v305 -> v306
|
// v305 -> v306
|
||||||
NewMigration("Add Repository Licenses", v1_23.AddRepositoryLicenses),
|
NewMigration("Add Repository Licenses", v1_23.AddRepositoryLicenses),
|
||||||
|
// v306 -> v307
|
||||||
|
NewMigration("Add BlockAdminMergeOverride to ProtectedBranch", v1_23.AddBlockAdminMergeOverrideBranchProtection),
|
||||||
}
|
}
|
||||||
|
|
||||||
// GetCurrentDBVersion returns the current db version
|
// GetCurrentDBVersion returns the current db version
|
||||||
|
|
|
@ -0,0 +1,13 @@
|
||||||
|
// Copyright 2024 The Gitea Authors. All rights reserved.
|
||||||
|
// SPDX-License-Identifier: MIT
|
||||||
|
|
||||||
|
package v1_23 //nolint
|
||||||
|
|
||||||
|
import "xorm.io/xorm"
|
||||||
|
|
||||||
|
func AddBlockAdminMergeOverrideBranchProtection(x *xorm.Engine) error {
|
||||||
|
type ProtectedBranch struct {
|
||||||
|
BlockAdminMergeOverride bool `xorm:"NOT NULL DEFAULT false"`
|
||||||
|
}
|
||||||
|
return x.Sync(new(ProtectedBranch))
|
||||||
|
}
|
|
@ -52,6 +52,7 @@ type BranchProtection struct {
|
||||||
RequireSignedCommits bool `json:"require_signed_commits"`
|
RequireSignedCommits bool `json:"require_signed_commits"`
|
||||||
ProtectedFilePatterns string `json:"protected_file_patterns"`
|
ProtectedFilePatterns string `json:"protected_file_patterns"`
|
||||||
UnprotectedFilePatterns string `json:"unprotected_file_patterns"`
|
UnprotectedFilePatterns string `json:"unprotected_file_patterns"`
|
||||||
|
BlockAdminMergeOverride bool `json:"block_admin_merge_override"`
|
||||||
// swagger:strfmt date-time
|
// swagger:strfmt date-time
|
||||||
Created time.Time `json:"created_at"`
|
Created time.Time `json:"created_at"`
|
||||||
// swagger:strfmt date-time
|
// swagger:strfmt date-time
|
||||||
|
@ -90,6 +91,7 @@ type CreateBranchProtectionOption struct {
|
||||||
RequireSignedCommits bool `json:"require_signed_commits"`
|
RequireSignedCommits bool `json:"require_signed_commits"`
|
||||||
ProtectedFilePatterns string `json:"protected_file_patterns"`
|
ProtectedFilePatterns string `json:"protected_file_patterns"`
|
||||||
UnprotectedFilePatterns string `json:"unprotected_file_patterns"`
|
UnprotectedFilePatterns string `json:"unprotected_file_patterns"`
|
||||||
|
BlockAdminMergeOverride bool `json:"block_admin_merge_override"`
|
||||||
}
|
}
|
||||||
|
|
||||||
// EditBranchProtectionOption options for editing a branch protection
|
// EditBranchProtectionOption options for editing a branch protection
|
||||||
|
@ -121,4 +123,5 @@ type EditBranchProtectionOption struct {
|
||||||
RequireSignedCommits *bool `json:"require_signed_commits"`
|
RequireSignedCommits *bool `json:"require_signed_commits"`
|
||||||
ProtectedFilePatterns *string `json:"protected_file_patterns"`
|
ProtectedFilePatterns *string `json:"protected_file_patterns"`
|
||||||
UnprotectedFilePatterns *string `json:"unprotected_file_patterns"`
|
UnprotectedFilePatterns *string `json:"unprotected_file_patterns"`
|
||||||
|
BlockAdminMergeOverride *bool `json:"block_admin_merge_override"`
|
||||||
}
|
}
|
||||||
|
|
|
@ -2461,6 +2461,8 @@ settings.block_on_official_review_requests = Block merge on official review requ
|
||||||
settings.block_on_official_review_requests_desc = Merging will not be possible when it has official review requests, even if there are enough approvals.
|
settings.block_on_official_review_requests_desc = Merging will not be possible when it has official review requests, even if there are enough approvals.
|
||||||
settings.block_outdated_branch = Block merge if pull request is outdated
|
settings.block_outdated_branch = Block merge if pull request is outdated
|
||||||
settings.block_outdated_branch_desc = Merging will not be possible when head branch is behind base branch.
|
settings.block_outdated_branch_desc = Merging will not be possible when head branch is behind base branch.
|
||||||
|
settings.block_admin_merge_override = Administrators must follow branch protection rules
|
||||||
|
settings.block_admin_merge_override_desc = Administrators must follow branch protection rules and can not circumvent it.
|
||||||
settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
|
settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
|
||||||
settings.merge_style_desc = Merge Styles
|
settings.merge_style_desc = Merge Styles
|
||||||
settings.default_merge_style_desc = Default Merge Style
|
settings.default_merge_style_desc = Default Merge Style
|
||||||
|
|
|
@ -642,6 +642,7 @@ func CreateBranchProtection(ctx *context.APIContext) {
|
||||||
ProtectedFilePatterns: form.ProtectedFilePatterns,
|
ProtectedFilePatterns: form.ProtectedFilePatterns,
|
||||||
UnprotectedFilePatterns: form.UnprotectedFilePatterns,
|
UnprotectedFilePatterns: form.UnprotectedFilePatterns,
|
||||||
BlockOnOutdatedBranch: form.BlockOnOutdatedBranch,
|
BlockOnOutdatedBranch: form.BlockOnOutdatedBranch,
|
||||||
|
BlockAdminMergeOverride: form.BlockAdminMergeOverride,
|
||||||
}
|
}
|
||||||
|
|
||||||
err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
|
err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
|
||||||
|
@ -852,6 +853,10 @@ func EditBranchProtection(ctx *context.APIContext) {
|
||||||
protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch
|
protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if form.BlockAdminMergeOverride != nil {
|
||||||
|
protectBranch.BlockAdminMergeOverride = *form.BlockAdminMergeOverride
|
||||||
|
}
|
||||||
|
|
||||||
var whitelistUsers, forcePushAllowlistUsers, mergeWhitelistUsers, approvalsWhitelistUsers []int64
|
var whitelistUsers, forcePushAllowlistUsers, mergeWhitelistUsers, approvalsWhitelistUsers []int64
|
||||||
if form.PushWhitelistUsernames != nil {
|
if form.PushWhitelistUsernames != nil {
|
||||||
whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false)
|
whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false)
|
||||||
|
|
|
@ -256,6 +256,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
|
||||||
protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns
|
protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns
|
||||||
protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns
|
protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns
|
||||||
protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch
|
protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch
|
||||||
|
protectBranch.BlockAdminMergeOverride = f.BlockAdminMergeOverride
|
||||||
|
|
||||||
err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
|
err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
|
||||||
UserIDs: whitelistUsers,
|
UserIDs: whitelistUsers,
|
||||||
|
|
|
@ -185,6 +185,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo
|
||||||
RequireSignedCommits: bp.RequireSignedCommits,
|
RequireSignedCommits: bp.RequireSignedCommits,
|
||||||
ProtectedFilePatterns: bp.ProtectedFilePatterns,
|
ProtectedFilePatterns: bp.ProtectedFilePatterns,
|
||||||
UnprotectedFilePatterns: bp.UnprotectedFilePatterns,
|
UnprotectedFilePatterns: bp.UnprotectedFilePatterns,
|
||||||
|
BlockAdminMergeOverride: bp.BlockAdminMergeOverride,
|
||||||
Created: bp.CreatedUnix.AsTime(),
|
Created: bp.CreatedUnix.AsTime(),
|
||||||
Updated: bp.UpdatedUnix.AsTime(),
|
Updated: bp.UpdatedUnix.AsTime(),
|
||||||
}
|
}
|
||||||
|
|
|
@ -219,6 +219,7 @@ type ProtectBranchForm struct {
|
||||||
RequireSignedCommits bool
|
RequireSignedCommits bool
|
||||||
ProtectedFilePatterns string
|
ProtectedFilePatterns string
|
||||||
UnprotectedFilePatterns string
|
UnprotectedFilePatterns string
|
||||||
|
BlockAdminMergeOverride bool
|
||||||
}
|
}
|
||||||
|
|
||||||
// Validate validates the fields
|
// Validate validates the fields
|
||||||
|
|
|
@ -68,7 +68,7 @@ const (
|
||||||
)
|
)
|
||||||
|
|
||||||
// CheckPullMergeable check if the pull mergeable based on all conditions (branch protection, merge options, ...)
|
// CheckPullMergeable check if the pull mergeable based on all conditions (branch protection, merge options, ...)
|
||||||
func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool) error {
|
func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminForceMerge bool) error {
|
||||||
return db.WithTx(stdCtx, func(ctx context.Context) error {
|
return db.WithTx(stdCtx, func(ctx context.Context) error {
|
||||||
if pr.HasMerged {
|
if pr.HasMerged {
|
||||||
return ErrHasMerged
|
return ErrHasMerged
|
||||||
|
@ -118,13 +118,22 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc
|
||||||
err = nil
|
err = nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// * if the doer is admin, they could skip the branch protection check
|
// * if admin tries to "Force Merge", they could sometimes skip the branch protection check
|
||||||
if adminSkipProtectionCheck {
|
if adminForceMerge {
|
||||||
if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
|
isRepoAdmin, errForceMerge := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer)
|
||||||
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
|
if errForceMerge != nil {
|
||||||
return errCheckAdmin
|
return fmt.Errorf("IsUserRepoAdmin failed, repo: %v, doer: %v, err: %w", pr.BaseRepoID, doer.ID, errForceMerge)
|
||||||
} else if isRepoAdmin {
|
}
|
||||||
err = nil // repo admin can skip the check, so clear the error
|
|
||||||
|
protectedBranchRule, errForceMerge := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
|
||||||
|
if errForceMerge != nil {
|
||||||
|
return fmt.Errorf("GetFirstMatchProtectedBranchRule failed, repo: %v, base branch: %v, err: %w", pr.BaseRepoID, pr.BaseBranch, errForceMerge)
|
||||||
|
}
|
||||||
|
|
||||||
|
// if doer is admin and the "Force Merge" is not blocked, then clear the branch protection check error
|
||||||
|
blockAdminForceMerge := protectedBranchRule != nil && protectedBranchRule.BlockAdminMergeOverride
|
||||||
|
if isRepoAdmin && !blockAdminForceMerge {
|
||||||
|
err = nil
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -164,7 +164,7 @@
|
||||||
{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}}
|
{{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}}
|
||||||
|
|
||||||
{{/* admin can merge without checks, writer can merge when checks succeed */}}
|
{{/* admin can merge without checks, writer can merge when checks succeed */}}
|
||||||
{{$canMergeNow := and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}}
|
{{$canMergeNow := and (or (and (not $.ProtectedBranch.BlockAdminMergeOverride) $.IsRepoAdmin) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}}
|
||||||
{{/* admin and writer both can make an auto merge schedule */}}
|
{{/* admin and writer both can make an auto merge schedule */}}
|
||||||
|
|
||||||
{{if $canMergeNow}}
|
{{if $canMergeNow}}
|
||||||
|
|
|
@ -323,6 +323,13 @@
|
||||||
<p class="help">{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}</p>
|
<p class="help">{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}</p>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
<div class="field">
|
||||||
|
<div class="ui checkbox">
|
||||||
|
<input name="block_admin_merge_override" type="checkbox" {{if .Rule.BlockAdminMergeOverride}}checked{{end}}>
|
||||||
|
<label>{{ctx.Locale.Tr "repo.settings.block_admin_merge_override"}}</label>
|
||||||
|
<p class="help">{{ctx.Locale.Tr "repo.settings.block_admin_merge_override_desc"}}</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
<div class="divider"></div>
|
<div class="divider"></div>
|
||||||
|
|
||||||
<div class="field">
|
<div class="field">
|
||||||
|
|
|
@ -18771,6 +18771,10 @@
|
||||||
},
|
},
|
||||||
"x-go-name": "ApprovalsWhitelistUsernames"
|
"x-go-name": "ApprovalsWhitelistUsernames"
|
||||||
},
|
},
|
||||||
|
"block_admin_merge_override": {
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "BlockAdminMergeOverride"
|
||||||
|
},
|
||||||
"block_on_official_review_requests": {
|
"block_on_official_review_requests": {
|
||||||
"type": "boolean",
|
"type": "boolean",
|
||||||
"x-go-name": "BlockOnOfficialReviewRequests"
|
"x-go-name": "BlockOnOfficialReviewRequests"
|
||||||
|
@ -19466,6 +19470,10 @@
|
||||||
},
|
},
|
||||||
"x-go-name": "ApprovalsWhitelistUsernames"
|
"x-go-name": "ApprovalsWhitelistUsernames"
|
||||||
},
|
},
|
||||||
|
"block_admin_merge_override": {
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "BlockAdminMergeOverride"
|
||||||
|
},
|
||||||
"block_on_official_review_requests": {
|
"block_on_official_review_requests": {
|
||||||
"type": "boolean",
|
"type": "boolean",
|
||||||
"x-go-name": "BlockOnOfficialReviewRequests"
|
"x-go-name": "BlockOnOfficialReviewRequests"
|
||||||
|
@ -20685,6 +20693,10 @@
|
||||||
},
|
},
|
||||||
"x-go-name": "ApprovalsWhitelistUsernames"
|
"x-go-name": "ApprovalsWhitelistUsernames"
|
||||||
},
|
},
|
||||||
|
"block_admin_merge_override": {
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "BlockAdminMergeOverride"
|
||||||
|
},
|
||||||
"block_on_official_review_requests": {
|
"block_on_official_review_requests": {
|
||||||
"type": "boolean",
|
"type": "boolean",
|
||||||
"x-go-name": "BlockOnOfficialReviewRequests"
|
"x-go-name": "BlockOnOfficialReviewRequests"
|
||||||
|
|
|
@ -976,3 +976,50 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
|
||||||
unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID})
|
unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID})
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
|
||||||
|
onGiteaRun(t, func(t *testing.T, u *url.URL) {
|
||||||
|
// create a pull request
|
||||||
|
session := loginUser(t, "user1")
|
||||||
|
forkedName := "repo1-1"
|
||||||
|
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
|
||||||
|
defer testDeleteRepository(t, session, "user1", forkedName)
|
||||||
|
|
||||||
|
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
|
||||||
|
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
|
||||||
|
|
||||||
|
baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
|
||||||
|
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName})
|
||||||
|
unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
|
||||||
|
BaseRepoID: baseRepo.ID,
|
||||||
|
BaseBranch: "master",
|
||||||
|
HeadRepoID: forkedRepo.ID,
|
||||||
|
HeadBranch: "master",
|
||||||
|
})
|
||||||
|
|
||||||
|
// add protected branch for commit status
|
||||||
|
csrf := GetUserCSRFToken(t, session)
|
||||||
|
// Change master branch to protected
|
||||||
|
pbCreateReq := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{
|
||||||
|
"_csrf": csrf,
|
||||||
|
"rule_name": "master",
|
||||||
|
"enable_push": "true",
|
||||||
|
"enable_status_check": "true",
|
||||||
|
"status_check_contexts": "gitea/actions",
|
||||||
|
"block_admin_merge_override": "true",
|
||||||
|
})
|
||||||
|
session.MakeRequest(t, pbCreateReq, http.StatusSeeOther)
|
||||||
|
|
||||||
|
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
|
||||||
|
mergeReq := NewRequestWithValues(t, "POST", "/api/v1/repos/user2/repo1/pulls/6/merge", map[string]string{
|
||||||
|
"_csrf": csrf,
|
||||||
|
"head_commit_id": "",
|
||||||
|
"merge_when_checks_succeed": "false",
|
||||||
|
"force_merge": "true",
|
||||||
|
"do": "rebase",
|
||||||
|
}).AddTokenAuth(token)
|
||||||
|
|
||||||
|
session.MakeRequest(t, mergeReq, http.StatusMethodNotAllowed)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
Loading…
Reference in New Issue