From 029bcd361a09664d4814d46379c1d82c6a6b9f32 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 28 Mar 2024 21:41:52 +0100 Subject: [PATCH] [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 | 2 + models/forgejo_migrations/v1_22/v9.go | 15 ++++ models/git/protected_branch.go | 1 + modules/structs/repo_branch.go | 3 + options/locale/locale_en-US.ini | 3 + routers/api/v1/repo/branch.go | 5 ++ routers/private/hook_pre_receive.go | 16 ++-- routers/web/repo/setting/protected_branch.go | 1 + services/convert/convert.go | 1 + services/forms/repo_form.go | 1 + services/pull/check.go | 7 +- services/pull/merge.go | 29 ++++--- templates/repo/issue/view_content/pull.tmpl | 2 +- templates/repo/settings/protected_branch.tmpl | 8 ++ templates/swagger/v1_json.tmpl | 12 +++ tests/integration/proctected_branch_test.go | 87 +++++++++++++++++++ 16 files changed, 167 insertions(+), 26 deletions(-) create mode 100644 models/forgejo_migrations/v1_22/v9.go create mode 100644 tests/integration/proctected_branch_test.go 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 b06e864869..97dc28b795 100644 --- a/options/locale/locale_en-US.ini +++ b/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 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 393ea1b9aa..2d657c74ac 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 fec4d7c8d4..a991e68e6f 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -260,6 +260,14 @@

{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}

+
{{ctx.Locale.Tr "repo.settings.event_pull_request_enforcement"}}
+
+
+ + +

{{ctx.Locale.Tr "repo.settings.enforce_on_admins_desc"}}

+
+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 22ada2f023..181c564f4a 100644 --- a/templates/swagger/v1_json.tmpl +++ b/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": { 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") + }) + }) +}