diff options
| -rw-r--r-- | models/forgejo_migrations/migrate.go | 2 | ||||
| -rw-r--r-- | models/forgejo_migrations/v1_22/v9.go | 15 | ||||
| -rw-r--r-- | models/git/protected_branch.go | 1 | ||||
| -rw-r--r-- | modules/structs/repo_branch.go | 3 | ||||
| -rw-r--r-- | options/locale/locale_en-US.ini | 3 | ||||
| -rw-r--r-- | routers/api/v1/repo/branch.go | 5 | ||||
| -rw-r--r-- | routers/private/hook_pre_receive.go | 16 | ||||
| -rw-r--r-- | routers/web/repo/setting/protected_branch.go | 1 | ||||
| -rw-r--r-- | services/convert/convert.go | 1 | ||||
| -rw-r--r-- | services/forms/repo_form.go | 1 | ||||
| -rw-r--r-- | services/pull/check.go | 7 | ||||
| -rw-r--r-- | services/pull/merge.go | 29 | ||||
| -rw-r--r-- | templates/repo/issue/view_content/pull.tmpl | 2 | ||||
| -rw-r--r-- | templates/repo/settings/protected_branch.tmpl | 8 | ||||
| -rw-r--r-- | templates/swagger/v1_json.tmpl | 12 | ||||
| -rw-r--r-- | tests/integration/proctected_branch_test.go | 87 |
16 files changed, 167 insertions, 26 deletions
diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index c2ffda5eb7..965b748ac9 100644 --- a/models/forgejo_migrations/migrate.go +++ b/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. diff --git a/models/forgejo_migrations/v1_22/v9.go b/models/forgejo_migrations/v1_22/v9.go new file mode 100644 index 0000000000..34c2844c39 --- /dev/null +++ b/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{}) +} diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index e0ff4d1542..a8b8c81bbe 100644 --- a/models/git/protected_branch.go +++ b/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"` diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index e96d276b29..0b3b0bb030 100644 --- a/modules/structs/repo_branch.go +++ b/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"` } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 17a8180ec9..5042e5467e 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -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 diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 5e6b6a8658..c33beee0ae 100644 --- a/routers/api/v1/repo/branch.go +++ b/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) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index f45e57b9e3..0613492845 100644 --- a/routers/private/hook_pre_receive.go +++ b/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 - 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()), diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 7ee67e5925..25146779de 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/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, diff --git a/services/convert/convert.go b/services/convert/convert.go index ca3ec32a40..dd2239458e 100644 --- a/services/convert/convert.go +++ b/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(), } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 0f7665804d..b5ff031f4b 100644 --- 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 + ApplyToAdmins bool } // Validate validates the fields diff --git a/services/pull/check.go b/services/pull/check.go index f4dd332b14..9aab3c94f3 100644 --- a/services/pull/check.go +++ b/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 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 diff --git a/services/pull/merge.go b/services/pull/merge.go index df8d66e2d4..7f79eca2aa 100644 --- a/services/pull/merge.go +++ b/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) -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 diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 19f1d3f91d..08f666d210 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/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}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index e95dd831c9..e1ee7b36f5 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/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"> diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 6034a8fbce..ee1ed64eaf 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -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": { diff --git a/tests/integration/proctected_branch_test.go b/tests/integration/proctected_branch_test.go new file mode 100644 index 0000000000..9c6e5e3cae --- /dev/null +++ b/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") + }) + }) +} |