[FEAT] Configure if protected branch rule should apply to admins

- Currently protected branch rules do not apply to admins, however in
some cases (like in the case of Forgejo project) you might also want to
apply these rules to admins to avoid accidental merges.
- Add new option to configure this on a per-rule basis.
- Adds integration tests.
- Resolves #65

models/forgejo_migrations/migrate.go

@@ -54,6 +54,8 @@ var migrations = []*Migration{
 	NewMigration("Add the `enable_repo_unit_hints` column to the `user` table", forgejo_v1_22.AddUserRepoUnitHintsSetting),
 	// v7 -> v8
 	NewMigration("Modify the `release`.`note` content to remove SSH signatures", forgejo_v1_22.RemoveSSHSignaturesFromReleaseNotes),
+	// v8 -> v9
+	NewMigration("Add the `apply_to_admins` column to the `protected_branch` table", forgejo_v1_22.AddApplyToAdminsSetting),
 }
 
 // GetCurrentDBVersion returns the current Forgejo database version.

models/forgejo_migrations/v1_22/v9.go

@@ -0,0 +1,15 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import "xorm.io/xorm"
+
+func AddApplyToAdminsSetting(x *xorm.Engine) error {
+	type ProtectedBranch struct {
+		ID            int64 `xorm:"pk autoincr"`
+		ApplyToAdmins bool  `xorm:"NOT NULL DEFAULT false"`
+	}
+
+	return x.Sync(&ProtectedBranch{})
+}

models/git/protected_branch.go

@@ -58,6 +58,7 @@ type ProtectedBranch struct {
 	RequireSignedCommits          bool     `xorm:"NOT NULL DEFAULT false"`
 	ProtectedFilePatterns         string   `xorm:"TEXT"`
 	UnprotectedFilePatterns       string   `xorm:"TEXT"`
+	ApplyToAdmins                 bool     `xorm:"NOT NULL DEFAULT false"`
 
 	CreatedUnix timeutil.TimeStamp `xorm:"created"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"updated"`

modules/structs/repo_branch.go

@@ -47,6 +47,7 @@ type BranchProtection struct {
 	RequireSignedCommits          bool     `json:"require_signed_commits"`
 	ProtectedFilePatterns         string   `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       string   `json:"unprotected_file_patterns"`
+	ApplyToAdmins                 bool     `json:"apply_to_admins"`
 	// swagger:strfmt date-time
 	Created time.Time `json:"created_at"`
 	// swagger:strfmt date-time
@@ -80,6 +81,7 @@ type CreateBranchProtectionOption struct {
 	RequireSignedCommits          bool     `json:"require_signed_commits"`
 	ProtectedFilePatterns         string   `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       string   `json:"unprotected_file_patterns"`
+	ApplyToAdmins                 bool     `json:"apply_to_admins"`
 }
 
 // EditBranchProtectionOption options for editing a branch protection
@@ -106,4 +108,5 @@ type EditBranchProtectionOption struct {
 	RequireSignedCommits          *bool    `json:"require_signed_commits"`
 	ProtectedFilePatterns         *string  `json:"protected_file_patterns"`
 	UnprotectedFilePatterns       *string  `json:"unprotected_file_patterns"`
+	ApplyToAdmins                 *bool    `json:"apply_to_admins"`
 }

options/locale/locale_en-US.ini

@@ -2343,6 +2343,7 @@ settings.event_pull_request_review_request = Pull request review requested
 settings.event_pull_request_review_request_desc = Pull request review requested or review request removed.
 settings.event_pull_request_approvals = Pull request approvals
 settings.event_pull_request_merge = Pull request merge
+settings.event_pull_request_enforcement = Enforcement
 settings.event_package = Package
 settings.event_package_desc = Package created or deleted in a repository.
 settings.branch_filter = Branch filter
@@ -2457,6 +2458,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.enforce_on_admins = Enforce this rule for repository admins
+settings.enforce_on_admins_desc = Repository admins cannot bypass this rule.
 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

routers/api/v1/repo/branch.go

@@ -621,6 +621,7 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		ProtectedFilePatterns:         form.ProtectedFilePatterns,
 		UnprotectedFilePatterns:       form.UnprotectedFilePatterns,
 		BlockOnOutdatedBranch:         form.BlockOnOutdatedBranch,
+		ApplyToAdmins:                 form.ApplyToAdmins,
 	}
 
 	err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
@@ -808,6 +809,10 @@ func EditBranchProtection(ctx *context.APIContext) {
 		protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch
 	}
 
+	if form.ApplyToAdmins != nil {
+		protectBranch.ApplyToAdmins = *form.ApplyToAdmins
+	}
+
 	var whitelistUsers []int64
 	if form.PushWhitelistUsernames != nil {
 		whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false)

routers/private/hook_pre_receive.go

@@ -337,13 +337,9 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
 			return
 		}
 
-		// If we're an admin for the repository we can ignore status checks, reviews and override protected files
+		// It's not allowed t overwrite protected files. Unless if the user is an
-		if ctx.userPerm.IsAdmin() {
+		// admin and the protected branch rule doesn't apply to admins.
-			return
+		if changedProtectedfiles && (!ctx.user.IsAdmin || protectBranch.ApplyToAdmins) {
-		}
-
-		// Now if we're not an admin - we can't overwrite protected files so fail now
-		if changedProtectedfiles {
 			log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
 			ctx.JSON(http.StatusForbidden, private.Response{
 				UserMsg: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
@@ -352,8 +348,12 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
 		}
 
 		// Check all status checks and reviews are ok
-		if err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil {
+		if pb, err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil {
 			if models.IsErrDisallowedToMerge(err) {
+				// Allow this if the rule doesn't apply to admins and the user is an admin.
+				if ctx.user.IsAdmin && !pb.ApplyToAdmins {
+					return
+				}
 				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", ctx.opts.UserID, branchName, repo, pr.Index, err.Error())
 				ctx.JSON(http.StatusForbidden, private.Response{
 					UserMsg: fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, ctx.opts.PullRequestID, err.Error()),

routers/web/repo/setting/protected_branch.go

@@ -237,6 +237,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 	protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns
 	protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns
 	protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch
+	protectBranch.ApplyToAdmins = f.ApplyToAdmins
 
 	err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
 		UserIDs:          whitelistUsers,

services/convert/convert.go

@@ -162,6 +162,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch) *api
 		RequireSignedCommits:          bp.RequireSignedCommits,
 		ProtectedFilePatterns:         bp.ProtectedFilePatterns,
 		UnprotectedFilePatterns:       bp.UnprotectedFilePatterns,
+		ApplyToAdmins:                 bp.ApplyToAdmins,
 		Created:                       bp.CreatedUnix.AsTime(),
 		Updated:                       bp.UpdatedUnix.AsTime(),
 	}

services/forms/repo_form.go

@@ -219,6 +219,7 @@ type ProtectBranchForm struct {
 	RequireSignedCommits          bool
 	ProtectedFilePatterns         string
 	UnprotectedFilePatterns       string
+	ApplyToAdmins                 bool
 }
 
 // Validate validates the fields

services/pull/check.go

@@ -104,7 +104,7 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
 			return ErrIsChecking
 		}
 
-		if err := CheckPullBranchProtections(ctx, pr, false); err != nil {
+		if pb, err := CheckPullBranchProtections(ctx, pr, false); err != nil {
 			if !models.IsErrDisallowedToMerge(err) {
 				log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err)
 				return err
@@ -117,8 +117,9 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
 				err = nil
 			}
 
-			// * if the doer is admin, they could skip the branch protection check
+			// * if the doer is admin, they could skip the branch protection check,
-			if adminSkipProtectionCheck {
+			// if that's allowed by the protected branch rule.
+			if adminSkipProtectionCheck && !pb.ApplyToAdmins {
 				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

services/pull/merge.go

@@ -424,63 +424,64 @@ func IsUserAllowedToMerge(ctx context.Context, pr *issues_model.PullRequest, p a
 	return false, nil
 }
 
-// CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks)
+// CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks).
-func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullRequest, skipProtectedFilesCheck bool) (err error) {
+// Returns the protected branch rule when `ErrDisallowedToMerge` is returned as error.
+func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullRequest, skipProtectedFilesCheck bool) (protectedBranchRule *git_model.ProtectedBranch, err error) {
 	if err = pr.LoadBaseRepo(ctx); err != nil {
-		return fmt.Errorf("LoadBaseRepo: %w", err)
+		return nil, fmt.Errorf("LoadBaseRepo: %w", err)
 	}
 
 	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
 	if err != nil {
-		return fmt.Errorf("LoadProtectedBranch: %v", err)
+		return nil, fmt.Errorf("LoadProtectedBranch: %v", err)
 	}
 	if pb == nil {
-		return nil
+		return nil, nil
 	}
 
 	isPass, err := IsPullCommitStatusPass(ctx, pr)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if !isPass {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "Not all required status checks successful",
 		}
 	}
 
 	if !issues_model.HasEnoughApprovals(ctx, pb, pr) {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "Does not have enough approvals",
 		}
 	}
 	if issues_model.MergeBlockedByRejectedReview(ctx, pb, pr) {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "There are requested changes",
 		}
 	}
 	if issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pr) {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "There are official review requests",
 		}
 	}
 
 	if issues_model.MergeBlockedByOutdatedBranch(pb, pr) {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "The head branch is behind the base branch",
 		}
 	}
 
 	if skipProtectedFilesCheck {
-		return nil
+		return nil, nil
 	}
 
 	if pb.MergeBlockedByProtectedFiles(pr.ChangedProtectedFiles) {
-		return models.ErrDisallowedToMerge{
+		return pb, models.ErrDisallowedToMerge{
 			Reason: "Changed protected files",
 		}
 	}
 
-	return nil
+	return nil, nil
 }
 
 // MergedManually mark pr as merged manually

templates/repo/issue/view_content/pull.tmpl

@@ -158,7 +158,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 $.IsRepoAdmin (not .ProtectedBranch.ApplyToAdmins)) (not $notAllOverridableChecksOk)) (or (not .AllowMerge) (not .RequireSigned) .WillSign)}}
 				{{/* admin and writer both can make an auto merge schedule */}}
 
 				{{if $canMergeNow}}

templates/repo/settings/protected_branch.tmpl

@@ -260,6 +260,14 @@
 						<p class="help">{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}</p>
 					</div>
 				</div>
+				<h5 class="ui dividing header">{{ctx.Locale.Tr "repo.settings.event_pull_request_enforcement"}}</h5>
+				<div class="field">
+					<div class="ui checkbox">
+						<input name="apply_to_admins" type="checkbox" {{if .Rule.ApplyToAdmins}}checked{{end}}>
+						<label>{{ctx.Locale.Tr "repo.settings.enforce_on_admins"}}</label>
+						<p class="help">{{ctx.Locale.Tr "repo.settings.enforce_on_admins_desc"}}</p>
+					</div>
+				</div>
 				<div class="divider"></div>
 
 				<div class="field">

templates/swagger/v1_json.tmpl

@@ -17759,6 +17759,10 @@
       "description": "BranchProtection represents a branch protection for a repository",
       "type": "object",
       "properties": {
+        "apply_to_admins": {
+          "type": "boolean",
+          "x-go-name": "ApplyToAdmins"
+        },
         "approvals_whitelist_teams": {
           "type": "array",
           "items": {
@@ -18409,6 +18413,10 @@
       "description": "CreateBranchProtectionOption options for creating a branch protection",
       "type": "object",
       "properties": {
+        "apply_to_admins": {
+          "type": "boolean",
+          "x-go-name": "ApplyToAdmins"
+        },
         "approvals_whitelist_teams": {
           "type": "array",
           "items": {
@@ -19580,6 +19588,10 @@
       "description": "EditBranchProtectionOption options for editing a branch protection",
       "type": "object",
       "properties": {
+        "apply_to_admins": {
+          "type": "boolean",
+          "x-go-name": "ApplyToAdmins"
+        },
         "approvals_whitelist_teams": {
           "type": "array",
           "items": {

tests/integration/proctected_branch_test.go

@@ -0,0 +1,87 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"net/url"
+	"strconv"
+	"strings"
+	"testing"
+
+	git_model "code.gitea.io/gitea/models/git"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestProtectedBranch_AdminEnforcement(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		session := loginUser(t, "user1")
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
+		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "add-readme", "README.md", "WIP")
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: 1, Name: "repo1"})
+
+		req := NewRequestWithValues(t, "POST", "user1/repo1/compare/master...add-readme", map[string]string{
+			"_csrf": GetCSRF(t, session, "user1/repo1/compare/master...add-readme"),
+			"title": "pull request",
+		})
+		session.MakeRequest(t, req, http.StatusOK)
+
+		t.Run("No protected branch", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req = NewRequest(t, "GET", "/user1/repo1/pulls/1")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			doc := NewHTMLParser(t, resp.Body)
+
+			text := strings.TrimSpace(doc.doc.Find(".merge-section").Text())
+			assert.Contains(t, text, "This pull request can be merged automatically.")
+			assert.Contains(t, text, "'canMergeNow':  true")
+		})
+
+		t.Run("Without admin enforcement", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequestWithValues(t, "POST", "/user1/repo1/settings/branches/edit", map[string]string{
+				"_csrf":              GetCSRF(t, session, "/user1/repo1/settings/branches/edit"),
+				"rule_name":          "master",
+				"required_approvals": "1",
+			})
+			session.MakeRequest(t, req, http.StatusSeeOther)
+
+			req = NewRequest(t, "GET", "/user1/repo1/pulls/1")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			doc := NewHTMLParser(t, resp.Body)
+
+			text := strings.TrimSpace(doc.doc.Find(".merge-section").Text())
+			assert.Contains(t, text, "This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.")
+			assert.Contains(t, text, "'canMergeNow':  true")
+		})
+
+		t.Run("With admin enforcement", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			protectedBranch := unittest.AssertExistsAndLoadBean(t, &git_model.ProtectedBranch{RuleName: "master", RepoID: repo.ID})
+			req := NewRequestWithValues(t, "POST", "/user1/repo1/settings/branches/edit", map[string]string{
+				"_csrf":              GetCSRF(t, session, "/user1/repo1/settings/branches/edit"),
+				"rule_name":          "master",
+				"rule_id":            strconv.FormatInt(protectedBranch.ID, 10),
+				"required_approvals": "1",
+				"apply_to_admins":    "true",
+			})
+			session.MakeRequest(t, req, http.StatusSeeOther)
+
+			req = NewRequest(t, "GET", "/user1/repo1/pulls/1")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			doc := NewHTMLParser(t, resp.Body)
+
+			text := strings.TrimSpace(doc.doc.Find(".merge-section").Text())
+			assert.Contains(t, text, "This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.")
+			assert.Contains(t, text, "'canMergeNow':  false")
+		})
+	})
+}