Merge pull request '[v7.0/forgejo] [FEAT] Configure if protected branch rule should apply to admins' (#2956) from bp-v7.0/forgejo-79b7089 into v7.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2956
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
This commit is contained in:
Earl Warren 2024-04-02 05:51:43 +00:00
commit 7db4e374ca
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

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

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

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

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