Make admins adhere to branch protection rules (#32248)

enko · wxiaoguang · GiteaBot · web-flow · commit de2ad2e1b177 · 2024-10-23T12:39:43.000+08:00
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>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -323,6 +323,13 @@
 						<p class="help">{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}</p>
 					</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="field">
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
diff --git 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)
+	})
+}

Original file line number	Diff line number	Diff line change
`@@ -603,6 +603,8 @@ var migrations = []Migration{`
`603`	`603`	`NewMigration("Add index for release sha1", v1_23.AddIndexForReleaseSha1),`
`604`	`604`	`// v305 -> v306`
`605`	`605`	`NewMigration("Add Repository Licenses", v1_23.AddRepositoryLicenses),`
	`606`	`+ // v306 -> v307`
	`607`	`+ NewMigration("Add BlockAdminMergeOverride to ProtectedBranch", v1_23.AddBlockAdminMergeOverrideBranchProtection),`
`606`	`608`	`}`
`607`	`609`
`608`	`610`	`// GetCurrentDBVersion returns the current db version`
Original file line number	Diff line number	Diff line change
`@@ -185,6 +185,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo`
`185`	`185`	`RequireSignedCommits: bp.RequireSignedCommits,`
`186`	`186`	`ProtectedFilePatterns: bp.ProtectedFilePatterns,`
`187`	`187`	`UnprotectedFilePatterns: bp.UnprotectedFilePatterns,`
	`188`	`+ BlockAdminMergeOverride: bp.BlockAdminMergeOverride,`
`188`	`189`	`Created: bp.CreatedUnix.AsTime(),`
`189`	`190`	`Updated: bp.UpdatedUnix.AsTime(),`
`190`	`191`	`}`
Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,7 @@ type ProtectBranchForm struct {`
`219`	`219`	`RequireSignedCommits bool`
`220`	`220`	`ProtectedFilePatterns string`
`221`	`221`	`UnprotectedFilePatterns string`
	`222`	`+ BlockAdminMergeOverride bool`
`222`	`223`	`}`
`223`	`224`
`224`	`225`	`// Validate validates the fields`