Merge pull request '[FEAT] Configure if protected branch rule should apply to admins' (#2867) from gusted/forgejo-protectedbranch-admins into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2867
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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")
+		})
+	})
+}