[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
This commit is contained in:
Gusted 2024-03-28 21:41:52 +01:00
parent a11116602e
commit 79b7089360
No known key found for this signature in database
GPG Key ID: FD821B732837125F
16 changed files with 167 additions and 26 deletions

View File

@ -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.

View File

@ -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{})
}

View File

@ -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"`

View File

@ -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"`
}

View File

@ -2348,6 +2348,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
@ -2462,6 +2463,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

View File

@ -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)

View File

@ -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
if ctx.userPerm.IsAdmin() {
return
}
// Now if we're not an admin - we can't overwrite protected files so fail now
if changedProtectedfiles {
// It's not allowed t overwrite protected files. Unless if the user is an
// admin and the protected branch rule doesn't apply to admins.
if changedProtectedfiles && (!ctx.user.IsAdmin || protectBranch.ApplyToAdmins) {
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()),

View File

@ -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,

View File

@ -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(),
}

View File

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

View File

@ -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 adminSkipProtectionCheck {
// * if the doer is admin, they could skip the branch protection check,
// 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

View File

@ -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)
func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullRequest, skipProtectedFilesCheck bool) (err error) {
// CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks).
// 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

View File

@ -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}}

View File

@ -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">

View File

@ -17756,6 +17756,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": {
@ -18406,6 +18410,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": {
@ -19577,6 +19585,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": {

View File

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