From de2ad2e1b177ed1c3412761c54b28579f8ecbb00 Mon Sep 17 00:00:00 2001 From: Tim Date: Wed, 23 Oct 2024 06:39:43 +0200 Subject: [PATCH] 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 Co-authored-by: Giteabot --- models/git/protected_branch.go | 1 + models/migrations/migrations.go | 2 + models/migrations/v1_23/v306.go | 13 +++++ modules/structs/repo_branch.go | 3 ++ options/locale/locale_en-US.ini | 2 + routers/api/v1/repo/branch.go | 5 ++ routers/web/repo/setting/protected_branch.go | 1 + services/convert/convert.go | 1 + services/forms/repo_form.go | 1 + services/pull/check.go | 25 ++++++---- templates/repo/issue/view_content/pull.tmpl | 2 +- templates/repo/settings/protected_branch.tmpl | 7 +++ templates/swagger/v1_json.tmpl | 12 +++++ tests/integration/pull_merge_test.go | 47 +++++++++++++++++++ 14 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 models/migrations/v1_23/v306.go diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index bde6057375..0033a42d4e 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -63,6 +63,7 @@ type ProtectedBranch struct { RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` ProtectedFilePatterns string `xorm:"TEXT"` UnprotectedFilePatterns string `xorm:"TEXT"` + BlockAdminMergeOverride bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"created"` UpdatedUnix timeutil.TimeStamp `xorm:"updated"` diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index f99718ead2..f0651ddbfa 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -603,6 +603,8 @@ var migrations = []Migration{ NewMigration("Add index for release sha1", v1_23.AddIndexForReleaseSha1), // v305 -> v306 NewMigration("Add Repository Licenses", v1_23.AddRepositoryLicenses), + // v306 -> v307 + NewMigration("Add BlockAdminMergeOverride to ProtectedBranch", v1_23.AddBlockAdminMergeOverrideBranchProtection), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_23/v306.go b/models/migrations/v1_23/v306.go new file mode 100644 index 0000000000..276b438e95 --- /dev/null +++ b/models/migrations/v1_23/v306.go @@ -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)) +} diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 0f2cf482fd..12a8344e87 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -52,6 +52,7 @@ type BranchProtection struct { RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` UnprotectedFilePatterns string `json:"unprotected_file_patterns"` + BlockAdminMergeOverride bool `json:"block_admin_merge_override"` // swagger:strfmt date-time Created time.Time `json:"created_at"` // swagger:strfmt date-time @@ -90,6 +91,7 @@ type CreateBranchProtectionOption struct { RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` UnprotectedFilePatterns string `json:"unprotected_file_patterns"` + BlockAdminMergeOverride bool `json:"block_admin_merge_override"` } // EditBranchProtectionOption options for editing a branch protection @@ -121,4 +123,5 @@ type EditBranchProtectionOption struct { RequireSignedCommits *bool `json:"require_signed_commits"` ProtectedFilePatterns *string `json:"protected_file_patterns"` UnprotectedFilePatterns *string `json:"unprotected_file_patterns"` + BlockAdminMergeOverride *bool `json:"block_admin_merge_override"` } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index a02d939b79..06bf57fc62 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -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_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_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.merge_style_desc = Merge Styles settings.default_merge_style_desc = Default Merge Style diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 63de4b8b6a..bb16858c81 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -642,6 +642,7 @@ func CreateBranchProtection(ctx *context.APIContext) { ProtectedFilePatterns: form.ProtectedFilePatterns, UnprotectedFilePatterns: form.UnprotectedFilePatterns, BlockOnOutdatedBranch: form.BlockOnOutdatedBranch, + BlockAdminMergeOverride: form.BlockAdminMergeOverride, } 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 } + if form.BlockAdminMergeOverride != nil { + protectBranch.BlockAdminMergeOverride = *form.BlockAdminMergeOverride + } + var whitelistUsers, forcePushAllowlistUsers, mergeWhitelistUsers, approvalsWhitelistUsers []int64 if form.PushWhitelistUsernames != nil { whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false) diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 7d943fd434..940a138aff 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -256,6 +256,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch + protectBranch.BlockAdminMergeOverride = f.BlockAdminMergeOverride err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ UserIDs: whitelistUsers, diff --git a/services/convert/convert.go b/services/convert/convert.go index 041d553e98..8dc311dae9 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -185,6 +185,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo RequireSignedCommits: bp.RequireSignedCommits, ProtectedFilePatterns: bp.ProtectedFilePatterns, UnprotectedFilePatterns: bp.UnprotectedFilePatterns, + BlockAdminMergeOverride: bp.BlockAdminMergeOverride, Created: bp.CreatedUnix.AsTime(), Updated: bp.UpdatedUnix.AsTime(), } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 988e479a48..ddd07a64cb 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -219,6 +219,7 @@ type ProtectBranchForm struct { RequireSignedCommits bool ProtectedFilePatterns string UnprotectedFilePatterns string + BlockAdminMergeOverride bool } // Validate validates the fields diff --git a/services/pull/check.go b/services/pull/check.go index ce212f7d83..736be4611b 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -68,7 +68,7 @@ const ( ) // 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 { if pr.HasMerged { return ErrHasMerged @@ -118,13 +118,22 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc err = nil } - // * if the doer is admin, they could skip the branch protection check - if adminSkipProtectionCheck { - if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil { - log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin) - return errCheckAdmin - } else if isRepoAdmin { - err = nil // repo admin can skip the check, so clear the error + // * if admin tries to "Force Merge", they could sometimes skip the branch protection check + if adminForceMerge { + isRepoAdmin, errForceMerge := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer) + if errForceMerge != nil { + return fmt.Errorf("IsUserRepoAdmin failed, repo: %v, doer: %v, err: %w", pr.BaseRepoID, doer.ID, errForceMerge) + } + + 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 } } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 1ce658ed00..5353357e81 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -164,7 +164,7 @@ {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}} {{/* 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 */}} {{if $canMergeNow}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 6fab4a625a..61cc6077a1 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -323,6 +323,13 @@

{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}

+
+
+ + +

{{ctx.Locale.Tr "repo.settings.block_admin_merge_override_desc"}}

+
+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index a2b75bd873..4cbf511aac 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -18771,6 +18771,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_admin_merge_override": { + "type": "boolean", + "x-go-name": "BlockAdminMergeOverride" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -19466,6 +19470,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_admin_merge_override": { + "type": "boolean", + "x-go-name": "BlockAdminMergeOverride" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -20685,6 +20693,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_admin_merge_override": { + "type": "boolean", + "x-go-name": "BlockAdminMergeOverride" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index c1c8a8bf4e..43210e852e 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -976,3 +976,50 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. 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) + }) +} -- 2.39.5