Make admins adhere to branch protection rules

enko · enko · commit 952bc01b4f04 · 2024-10-17T21:58:23.000+02: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
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/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/web/repo/issue.go b/routers/web/repo/issue.go
@@ -1943,6 +1943,7 @@ func ViewIssue(ctx *context.Context) {
 			ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0
 			ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)
 			ctx.Data["RequireApprovalsWhitelist"] = pb.EnableApprovalsWhitelist
+			ctx.Data["BlockAdminMergeOverride"] = pb.BlockAdminMergeOverride
 		}
 		ctx.Data["WillSign"] = false
 		if ctx.Doer != nil {
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/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
@@ -118,13 +118,26 @@ 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 the doer is admin, they could sometimes skip the branch protection check
 			if adminSkipProtectionCheck {
-				if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
+				pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
+				if err != nil {
+					return fmt.Errorf("LoadProtectedBranch: %v", err)
+				}
+
+				isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer)
+
+				/**
+				 * Checks are only skipable if there is no branch protection available or BlockAdminMergeOverride
+				 * of branch protection is set to false
+				 */
+				if 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
+				} else if isRepoAdmin && pb != nil && !pb.BlockAdminMergeOverride {
+					err = nil
+				} else if isRepoAdmin && pb == nil {
+					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 $.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,52 @@ 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")
+		// user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+		forkedName := "repo1-1"
+		testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
+		defer func() {
+			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
`@@ -1943,6 +1943,7 @@ func ViewIssue(ctx *context.Context) {`
`1943`	`1943`	`ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0`
`1944`	`1944`	`ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)`
`1945`	`1945`	`ctx.Data["RequireApprovalsWhitelist"] = pb.EnableApprovalsWhitelist`
	`1946`	`+ ctx.Data["BlockAdminMergeOverride"] = pb.BlockAdminMergeOverride`
`1946`	`1947`	`}`
`1947`	`1948`	`ctx.Data["WillSign"] = false`
`1948`	`1949`	`if ctx.Doer != nil {`
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`