From c52d48aae46af879fdfcfd94d03b7072878b5441 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 17 Apr 2020 03:00:36 +0200 Subject: [PATCH] Prevent merge of outdated PRs on protected branches (#11012) * Block PR on Outdated Branch * finalize * cleanup * fix typo and sentences thanks @guillep2k Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> Co-authored-by: Lauris BH --- models/branches.go | 6 ++++++ models/migrations/migrations.go | 6 ++++-- models/migrations/v137.go | 16 ++++++++++++++++ modules/auth/repo_form.go | 1 + modules/convert/convert.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.go | 1 + routers/repo/issue.go | 1 + routers/repo/setting_protected_branch.go | 1 + services/pull/merge.go | 10 ++++++++-- templates/repo/issue/view_content/pull.tmpl | 13 ++++++++++++- templates/repo/settings/protected_branch.tmpl | 7 +++++++ templates/swagger/v1_json.tmpl | 12 ++++++++++++ 15 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 models/migrations/v137.go diff --git a/models/branches.go b/models/branches.go index 44cfb41403..e6b8d61a70 100644 --- a/models/branches.go +++ b/models/branches.go @@ -47,6 +47,7 @@ type ProtectedBranch struct { ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` + BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"` DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` ProtectedFilePatterns string `xorm:"TEXT"` @@ -194,6 +195,11 @@ func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullReque return rejectExist } +// MergeBlockedByOutdatedBranch returns true if merge is blocked by an outdated head branch +func (protectBranch *ProtectedBranch) MergeBlockedByOutdatedBranch(pr *PullRequest) bool { + return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0 +} + // GetProtectedFilePatterns parses a semicolon separated list of protected file patterns and returns a glob.Glob slice func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob { extarr := make([]glob.Glob, 0, 10) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index e1d46236a9..fe72d0f630 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -202,10 +202,12 @@ var migrations = []Migration{ NewMigration("Add EmailHash Table", addEmailHashTable), // v134 -> v135 NewMigration("Refix merge base for merged pull requests", refixMergeBase), - // v135 -> 136 + // v135 -> v136 NewMigration("Add OrgID column to Labels table", addOrgIDLabelColumn), - // v136 -> 137 + // v136 -> v137 NewMigration("Add CommitsAhead and CommitsBehind Column to PullRequest Table", addCommitDivergenceToPulls), + // v137 -> v138 + NewMigration("Add Branch Protection Block Outdated Branch", addBlockOnOutdatedBranch), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v137.go b/models/migrations/v137.go new file mode 100644 index 0000000000..f175cf8a80 --- /dev/null +++ b/models/migrations/v137.go @@ -0,0 +1,16 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addBlockOnOutdatedBranch(x *xorm.Engine) error { + type ProtectedBranch struct { + BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"` + } + return x.Sync2(new(ProtectedBranch)) +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 80ed314196..6c3421e4f7 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -173,6 +173,7 @@ type ProtectBranchForm struct { ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string BlockOnRejectedReviews bool + BlockOnOutdatedBranch bool DismissStaleApprovals bool RequireSignedCommits bool ProtectedFilePatterns string diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 30d240f809..ec18b13056 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -118,6 +118,7 @@ func ToBranchProtection(bp *models.ProtectedBranch) *api.BranchProtection { ApprovalsWhitelistUsernames: approvalsWhitelistUsernames, ApprovalsWhitelistTeams: approvalsWhitelistTeams, BlockOnRejectedReviews: bp.BlockOnRejectedReviews, + BlockOnOutdatedBranch: bp.BlockOnOutdatedBranch, DismissStaleApprovals: bp.DismissStaleApprovals, RequireSignedCommits: bp.RequireSignedCommits, ProtectedFilePatterns: bp.ProtectedFilePatterns, diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 886018c858..490c63070a 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -39,6 +39,7 @@ type BranchProtection struct { ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"` ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"` + BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"` DismissStaleApprovals bool `json:"dismiss_stale_approvals"` RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` @@ -66,6 +67,7 @@ type CreateBranchProtectionOption struct { ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"` ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"` + BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"` DismissStaleApprovals bool `json:"dismiss_stale_approvals"` RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` @@ -88,6 +90,7 @@ type EditBranchProtectionOption struct { ApprovalsWhitelistUsernames []string `json:"approvals_whitelist_username"` ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews *bool `json:"block_on_rejected_reviews"` + BlockOnOutdatedBranch *bool `json:"block_on_outdated_branch"` DismissStaleApprovals *bool `json:"dismiss_stale_approvals"` RequireSignedCommits *bool `json:"require_signed_commits"` ProtectedFilePatterns *string `json:"protected_file_patterns"` diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index af2aec7e98..9653784f91 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1102,6 +1102,7 @@ pulls.required_status_check_missing = Some required checks are missing. pulls.required_status_check_administrator = As an administrator, you may still merge this pull request. pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted." pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer." +pulls.blocked_by_outdated_branch = "This Pull Request is blocked because it's outdated." pulls.can_auto_merge_desc = This pull request can be merged automatically. pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts. pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. @@ -1528,6 +1529,8 @@ settings.protected_branch_deletion = Disable Branch Protection settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue? settings.block_rejected_reviews = Block merge on rejected reviews settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, 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.default_branch_desc = Select a default repository branch for pull requests and code commits: settings.choose_branch = Choose a branch… settings.no_protected_branch = There are no protected branches. diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 56a9b5ec25..07c6159501 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -340,6 +340,7 @@ func CreateBranchProtection(ctx *context.APIContext, form api.CreateBranchProtec DismissStaleApprovals: form.DismissStaleApprovals, RequireSignedCommits: form.RequireSignedCommits, ProtectedFilePatterns: form.ProtectedFilePatterns, + BlockOnOutdatedBranch: form.BlockOnOutdatedBranch, } err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ @@ -475,6 +476,10 @@ func EditBranchProtection(ctx *context.APIContext, form api.EditBranchProtection protectBranch.ProtectedFilePatterns = *form.ProtectedFilePatterns } + if form.BlockOnOutdatedBranch != nil { + protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch + } + var whitelistUsers []int64 if form.PushWhitelistUsernames != nil { whitelistUsers, err = models.GetUserIDsByNames(form.PushWhitelistUsernames, false) diff --git a/routers/private/hook.go b/routers/private/hook.go index 846d9b6070..de2b03e0b2 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -263,6 +263,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { } } + // Detect Protected file pattern globs := protectBranch.GetProtectedFilePatterns() if len(globs) > 0 { err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 3655de7ed4..9ad379684a 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1065,6 +1065,7 @@ func ViewIssue(ctx *context.Context) { cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull) ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) + ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull) ctx.Data["GrantedApprovals"] = cnt ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits } diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index af49aefcec..ab0fd77eee 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -248,6 +248,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) protectBranch.DismissStaleApprovals = f.DismissStaleApprovals protectBranch.RequireSignedCommits = f.RequireSignedCommits protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns + protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ UserIDs: whitelistUsers, diff --git a/services/pull/merge.go b/services/pull/merge.go index a20a214014..47521ce147 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -574,16 +574,22 @@ func CheckPRReadyToMerge(pr *models.PullRequest) (err error) { } } - if enoughApprovals := pr.ProtectedBranch.HasEnoughApprovals(pr); !enoughApprovals { + if !pr.ProtectedBranch.HasEnoughApprovals(pr) { return models.ErrNotAllowedToMerge{ Reason: "Does not have enough approvals", } } - if rejected := pr.ProtectedBranch.MergeBlockedByRejectedReview(pr); rejected { + if pr.ProtectedBranch.MergeBlockedByRejectedReview(pr) { return models.ErrNotAllowedToMerge{ Reason: "There are requested changes", } } + if pr.ProtectedBranch.MergeBlockedByOutdatedBranch(pr) { + return models.ErrNotAllowedToMerge{ + Reason: "The head branch is behind the base branch", + } + } + return nil } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 73a6d695ff..e66c2f6a9a 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -66,6 +66,7 @@ {{- else if .IsPullRequestBroken}}red {{- else if .IsBlockedByApprovals}}red {{- else if .IsBlockedByRejection}}red + {{- else if .IsBlockedByOutdatedBranch}}red {{- else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsFailure .RequiredStatusCheckState.IsError)}}red {{- else if and .EnableStatusCheck (or (not $.LatestCommitStatus) .RequiredStatusCheckState.IsPending .RequiredStatusCheckState.IsWarning)}}yellow {{- else if and .RequireSigned (not .WillSign)}}}red @@ -138,6 +139,11 @@ {{svg "octicon-x" 16}} {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}} + {{else if .IsBlockedByOutdatedBranch}} +
+ {{svg "octicon-x" 16}} + {{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}} +
{{else if and .EnableStatusCheck (or .RequiredStatusCheckState.IsError .RequiredStatusCheckState.IsFailure)}}
{{svg "octicon-x" 16}} @@ -158,7 +164,7 @@ {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{end}} - {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}} + {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOutdatedBranch (and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess))}} {{if and (or $.IsRepoAdmin (not $notAllOverridableChecksOk)) (or (not .RequireSigned) .WillSign)}} {{if $notAllOverridableChecksOk}}
@@ -342,6 +348,11 @@ {{svg "octicon-x" 16}} {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
+ {{else if .IsBlockedByOutdatedBranch}} +
+ {{svg "octicon-x" 16}} + {{$.i18n.Tr "repo.pulls.blocked_by_outdated_branch"}} +
{{else if and .EnableStatusCheck (not .RequiredStatusCheckState.IsSuccess)}}
{{svg "octicon-x" 16}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index f46e84d3bc..0496eb189d 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -225,6 +225,13 @@

{{.i18n.Tr "repo.settings.require_signed_commits_desc"}}

+
+
+ + +

{{.i18n.Tr "repo.settings.block_outdated_branch_desc"}}

+
+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 8cb7dbfcc0..e9368a7d2a 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -10072,6 +10072,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_on_outdated_branch": { + "type": "boolean", + "x-go-name": "BlockOnOutdatedBranch" + }, "block_on_rejected_reviews": { "type": "boolean", "x-go-name": "BlockOnRejectedReviews" @@ -10392,6 +10396,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_on_outdated_branch": { + "type": "boolean", + "x-go-name": "BlockOnOutdatedBranch" + }, "block_on_rejected_reviews": { "type": "boolean", "x-go-name": "BlockOnRejectedReviews" @@ -11204,6 +11212,10 @@ }, "x-go-name": "ApprovalsWhitelistUsernames" }, + "block_on_outdated_branch": { + "type": "boolean", + "x-go-name": "BlockOnOutdatedBranch" + }, "block_on_rejected_reviews": { "type": "boolean", "x-go-name": "BlockOnRejectedReviews"