From 04ca7f004710de2b408f558f6f148894aa61ba57 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 18 Sep 2019 13:39:45 +0800 Subject: [PATCH] Refuse merge until all required status checks success (#7481) * refuse merge until ci successfully * deny merge request when required status checkes not succeed on merge Post and API * add database migration for added columns on protected_branch * fix migration * fix protected branch check bug * fix protected branch settings * remove duplicated code on check pull request's required commit statuses pass * remove unused codes * fix migration * add newline for template file * fix go mod * rename function name and some other fixes * fix template * fix bug pull view * remove go1.12 wrong dependencies * add administrator bypass when protected branch status check enabled * fix bug * improve the codes --- go.mod | 1 + models/branches.go | 2 + models/commit_status.go | 22 ++ models/migrations/migrations.go | 2 + models/migrations/v94.go | 24 ++ models/pull.go | 14 + modules/auth/repo_form.go | 2 + modules/pull/commit_status.go | 70 +++++ options/locale/locale_en-US.ini | 6 + routers/api/v1/repo/pull.go | 12 + routers/repo/pull.go | 30 ++ routers/repo/setting_protected_branch.go | 28 ++ templates/repo/issue/view_content/pull.tmpl | 261 ++++++++++-------- templates/repo/pulls/status.tmpl | 9 +- templates/repo/settings/protected_branch.tmpl | 32 +++ 15 files changed, 393 insertions(+), 122 deletions(-) create mode 100644 models/migrations/v94.go create mode 100644 modules/pull/commit_status.go diff --git a/go.mod b/go.mod index f5d49294d4..0b5dd381de 100644 --- a/go.mod +++ b/go.mod @@ -80,6 +80,7 @@ require ( github.com/nfnt/resize v0.0.0-20160724205520-891127d8d1b5 github.com/oliamb/cutter v0.2.2 github.com/philhofer/fwd v1.0.0 // indirect + github.com/pkg/errors v0.8.1 github.com/pquerna/otp v0.0.0-20160912161815-54653902c20e github.com/prometheus/client_golang v1.1.0 github.com/prometheus/procfs v0.0.4 // indirect diff --git a/models/branches.go b/models/branches.go index 2a99d98955..9daaa487e7 100644 --- a/models/branches.go +++ b/models/branches.go @@ -36,6 +36,8 @@ type ProtectedBranch struct { EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` + StatusCheckContexts []string `xorm:"JSON TEXT"` ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` diff --git a/models/commit_status.go b/models/commit_status.go index 9f0a32cdfb..6f6cbc387f 100644 --- a/models/commit_status.go +++ b/models/commit_status.go @@ -9,6 +9,7 @@ import ( "crypto/sha1" "fmt" "strings" + "time" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -205,6 +206,27 @@ func GetLatestCommitStatus(repo *Repository, sha string, page int) ([]*CommitSta return statuses, x.In("id", ids).Find(&statuses) } +// FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts +func FindRepoRecentCommitStatusContexts(repoID int64, before time.Duration) ([]string, error) { + start := timeutil.TimeStampNow().AddDuration(-before) + ids := make([]int64, 0, 10) + if err := x.Table("commit_status"). + Where("repo_id = ?", repoID). + And("updated_unix >= ?", start). + Select("max( id ) as id"). + GroupBy("context_hash").OrderBy("max( id ) desc"). + Find(&ids); err != nil { + return nil, err + } + + var contexts = make([]string, 0, len(ids)) + if len(ids) == 0 { + return contexts, nil + } + return contexts, x.Select("context").Table("commit_status").In("id", ids).Find(&contexts) + +} + // NewCommitStatusOptions holds options for creating a CommitStatus type NewCommitStatusOptions struct { Repo *Repository diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 15e021c05a..885043dce5 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -242,6 +242,8 @@ var migrations = []Migration{ NewMigration("remove orphaned repository index statuses", removeLingeringIndexStatus), // v93 -> v94 NewMigration("add email notification enabled preference to user", addEmailNotificationEnabledToUser), + // v94 -> v95 + NewMigration("add enable_status_check, status_check_contexts to protected_branch", addStatusCheckColumnsForProtectedBranches), } // Migrate database to current version diff --git a/models/migrations/v94.go b/models/migrations/v94.go new file mode 100644 index 0000000000..5fe8c3fa12 --- /dev/null +++ b/models/migrations/v94.go @@ -0,0 +1,24 @@ +// Copyright 2019 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 "github.com/go-xorm/xorm" + +func addStatusCheckColumnsForProtectedBranches(x *xorm.Engine) error { + type ProtectedBranch struct { + EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` + StatusCheckContexts []string `xorm:"JSON TEXT"` + } + + if err := x.Sync2(new(ProtectedBranch)); err != nil { + return err + } + + _, err := x.Cols("enable_status_check", "status_check_contexts").Update(&ProtectedBranch{ + EnableStatusCheck: false, + StatusCheckContexts: []string{}, + }) + return err +} diff --git a/models/pull.go b/models/pull.go index ecb5c1345e..2f7218f415 100644 --- a/models/pull.go +++ b/models/pull.go @@ -99,6 +99,20 @@ func (pr *PullRequest) LoadAttributes() error { return pr.loadAttributes(x) } +// LoadBaseRepo loads pull request base repository from database +func (pr *PullRequest) LoadBaseRepo() error { + if pr.BaseRepo == nil { + var repo Repository + if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil { + return err + } else if !has { + return ErrRepoNotExist{ID: pr.BaseRepoID} + } + pr.BaseRepo = &repo + } + return nil +} + // LoadIssue loads issue information from database func (pr *PullRequest) LoadIssue() (err error) { return pr.loadIssue(x) diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 56ae77a7f7..8d10fc1570 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -155,6 +155,8 @@ type ProtectBranchForm struct { EnableMergeWhitelist bool MergeWhitelistUsers string MergeWhitelistTeams string + EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` + StatusCheckContexts []string RequiredApprovals int64 ApprovalsWhitelistUsers string ApprovalsWhitelistTeams string diff --git a/modules/pull/commit_status.go b/modules/pull/commit_status.go new file mode 100644 index 0000000000..bdadc329d6 --- /dev/null +++ b/modules/pull/commit_status.go @@ -0,0 +1,70 @@ +// Copyright 2019 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 pull + +import ( + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + + "github.com/pkg/errors" +) + +// IsCommitStatusContextSuccess returns true if all required status check contexts succeed. +func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, requiredContexts []string) bool { + for _, ctx := range requiredContexts { + var found bool + for _, commitStatus := range commitStatuses { + if commitStatus.Context == ctx { + if commitStatus.State != models.CommitStatusSuccess { + return false + } + + found = true + break + } + } + if !found { + return false + } + } + return true +} + +// IsPullCommitStatusPass returns if all required status checks PASS +func IsPullCommitStatusPass(pr *models.PullRequest) (bool, error) { + if err := pr.LoadProtectedBranch(); err != nil { + return false, errors.Wrap(err, "GetLatestCommitStatus") + } + if pr.ProtectedBranch == nil || !pr.ProtectedBranch.EnableStatusCheck { + return true, nil + } + + // check if all required status checks are successful + headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + return false, errors.Wrap(err, "OpenRepository") + } + + if !headGitRepo.IsBranchExist(pr.HeadBranch) { + return false, errors.New("Head branch does not exist, can not merge") + } + + sha, err := headGitRepo.GetBranchCommitID(pr.HeadBranch) + if err != nil { + return false, errors.Wrap(err, "GetBranchCommitID") + } + + if err := pr.LoadBaseRepo(); err != nil { + return false, errors.Wrap(err, "LoadBaseRepo") + } + + commitStatuses, err := models.GetLatestCommitStatus(pr.BaseRepo, sha, 0) + if err != nil { + return false, errors.Wrap(err, "GetLatestCommitStatus") + } + + return IsCommitStatusContextSuccess(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 8a1dc95b9f..843252dc34 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -981,6 +981,8 @@ pulls.cannot_merge_work_in_progress = This pull request is marked as a work in p pulls.data_broken = This pull request is broken due to missing fork information. pulls.files_conflicted = This pull request has changes conflicting with the target branch. pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments." +pulls.required_status_check_failed = Some required checks were not successful. +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.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. @@ -988,6 +990,7 @@ pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled. pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually. pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress. +pulls.no_merge_status_check = This pull request cannot be merged because not all required status checkes are successful. pulls.merge_pull_request = Merge Pull Request pulls.rebase_merge_pull_request = Rebase and Merge pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff) @@ -1311,6 +1314,9 @@ settings.protect_merge_whitelist_committers = Enable Merge Whitelist settings.protect_merge_whitelist_committers_desc = Allow only whitelisted users or teams to merge pull requests into this branch. settings.protect_merge_whitelist_users = Whitelisted users for merging: settings.protect_merge_whitelist_teams = Whitelisted teams for merging: +settings.protect_check_status_contexts = Enable Status Check +settings.protect_check_status_contexts_desc = Require status checks to pass before merging Choose which status checks must pass before branches can be merged into a branch that matches this rule. When enabled, commits must first be pushed to another branch, then merged or pushed directly to a branch that matches this rule after status checks have passed. +settings.protect_check_status_contexts_list = Status checks found in the last week for this repository settings.protect_required_approvals = Required approvals: settings.protect_required_approvals_desc = Allow only to merge pull request with enough positive reviews of whitelisted users or teams. settings.protect_approvals_whitelist_users = Whitelisted reviewers: diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 7be34c656e..0f9eab2f50 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" "code.gitea.io/gitea/modules/pull" + pull_service "code.gitea.io/gitea/modules/pull" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" milestone_service "code.gitea.io/gitea/services/milestone" @@ -571,6 +572,17 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } + isPass, err := pull_service.IsPullCommitStatusPass(pr) + if err != nil { + ctx.Error(500, "IsPullCommitStatusPass", err) + return + } + + if !isPass && !ctx.IsUserRepoAdmin() { + ctx.Status(405) + return + } + if len(form.Do) == 0 { form.Do = string(models.MergeStyleMerge) } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 14b8670a20..180d592e3d 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" "code.gitea.io/gitea/modules/pull" + pull_service "code.gitea.io/gitea/modules/pull" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/gitdiff" @@ -322,6 +323,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare setMergeTarget(ctx, pull) + if err = pull.LoadProtectedBranch(); err != nil { + ctx.ServerError("GetLatestCommitStatus", err) + return nil + } + ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck + var headGitRepo *git.Repository var headBranchExist bool // HeadRepo may be missing @@ -350,6 +357,18 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare ctx.Data["LatestCommitStatuses"] = commitStatuses ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(commitStatuses) } + + if pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck { + ctx.Data["is_context_required"] = func(context string) bool { + for _, c := range pull.ProtectedBranch.StatusCheckContexts { + if c == context { + return true + } + } + return false + } + ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts) + } } } @@ -608,6 +627,17 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { return } + isPass, err := pull_service.IsPullCommitStatusPass(pr) + if err != nil { + ctx.ServerError("IsPullCommitStatusPass", err) + return + } + if !isPass && !ctx.IsUserRepoAdmin() { + ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_status_check")) + ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) + return + } + if ctx.HasError() { ctx.Flash.Error(ctx.Data["ErrorMsg"].(string)) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index b5a115b6a4..80f44ead99 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -7,6 +7,7 @@ package repo import ( "fmt" "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" @@ -125,6 +126,29 @@ func SettingsProtectedBranch(c *context.Context) { c.Data["whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.WhitelistUserIDs), ",") c.Data["merge_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.MergeWhitelistUserIDs), ",") c.Data["approvals_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.ApprovalsWhitelistUserIDs), ",") + contexts, _ := models.FindRepoRecentCommitStatusContexts(c.Repo.Repository.ID, 7*24*time.Hour) // Find last week status check contexts + for _, context := range protectBranch.StatusCheckContexts { + var found bool + for _, ctx := range contexts { + if ctx == context { + found = true + break + } + } + if !found { + contexts = append(contexts, context) + } + } + + c.Data["branch_status_check_contexts"] = contexts + c.Data["is_context_required"] = func(context string) bool { + for _, c := range protectBranch.StatusCheckContexts { + if c == context { + return true + } + } + return false + } if c.Repo.Owner.IsOrganization() { teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeRead) @@ -186,6 +210,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) if strings.TrimSpace(f.MergeWhitelistTeams) != "" { mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) } + + protectBranch.EnableStatusCheck = f.EnableStatusCheck + protectBranch.StatusCheckContexts = f.StatusCheckContexts + protectBranch.RequiredApprovals = f.RequiredApprovals if strings.TrimSpace(f.ApprovalsWhitelistUsers) != "" { approvalsWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistUsers, ",")) diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index f0e39d5e37..f5ce8e0886 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -41,6 +41,7 @@ {{else if .IsFilesConflicted}}grey {{else if .IsPullRequestBroken}}red {{else if .IsBlockedByApprovals}}red + {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red {{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.CanAutoMerge}}green {{else}}red{{end}}"> @@ -104,131 +105,151 @@ {{$.i18n.Tr "repo.pulls.is_checking"}} - {{else if .Issue.PullRequest.CanAutoMerge}} -
- - {{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}} + {{else if and (not .Issue.PullRequest.CanAutoMerge) .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} +
+ + {{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
- {{if .AllowMerge}} - {{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}} - {{if or $prUnit.PullRequestsConfig.AllowMerge $prUnit.PullRequestsConfig.AllowRebase $prUnit.PullRequestsConfig.AllowRebaseMerge $prUnit.PullRequestsConfig.AllowSquash}} -
- {{if $prUnit.PullRequestsConfig.AllowMerge}} - - {{end}} - {{if $prUnit.PullRequestsConfig.AllowRebase}} - - {{end}} - {{if $prUnit.PullRequestsConfig.AllowRebaseMerge}} - - {{end}} - {{if $prUnit.PullRequestsConfig.AllowSquash}} - - {{end}} -
- -
- -
- {{if $prUnit.PullRequestsConfig.AllowMerge}} -
{{$.i18n.Tr "repo.pulls.merge_pull_request"}}
- {{end}} - {{if $prUnit.PullRequestsConfig.AllowRebase}} -
{{$.i18n.Tr "repo.pulls.rebase_merge_pull_request"}}
- {{end}} - {{if $prUnit.PullRequestsConfig.AllowRebaseMerge}} -
{{$.i18n.Tr "repo.pulls.rebase_merge_commit_pull_request"}}
- {{end}} - {{if $prUnit.PullRequestsConfig.AllowSquash}} -
{{$.i18n.Tr "repo.pulls.squash_merge_pull_request"}}
- {{end}} -
-
+ {{else if .Issue.PullRequest.CanAutoMerge}} + {{if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} +
+ + {{$.i18n.Tr "repo.pulls.required_status_check_failed"}} +
+ {{end}} + {{if or $.IsRepoAdmin (not .EnableStatusCheck) .IsRequiredStatusCheckSuccess}} + {{if and $.IsRepoAdmin .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}} +
+ + {{$.i18n.Tr "repo.pulls.required_status_check_administrator"}}
{{else}} -
- - {{$.i18n.Tr "repo.pulls.no_merge_desc"}} -
-
- - {{$.i18n.Tr "repo.pulls.no_merge_helper"}} +
+ + {{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}}
{{end}} + {{if .AllowMerge}} + {{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}} + {{if or $prUnit.PullRequestsConfig.AllowMerge $prUnit.PullRequestsConfig.AllowRebase $prUnit.PullRequestsConfig.AllowRebaseMerge $prUnit.PullRequestsConfig.AllowSquash}} +
+ {{if $prUnit.PullRequestsConfig.AllowMerge}} + + {{end}} + {{if $prUnit.PullRequestsConfig.AllowRebase}} + + {{end}} + {{if $prUnit.PullRequestsConfig.AllowRebaseMerge}} + + {{end}} + {{if $prUnit.PullRequestsConfig.AllowSquash}} + + {{end}} +
+ +
+ +
+ {{if $prUnit.PullRequestsConfig.AllowMerge}} +
{{$.i18n.Tr "repo.pulls.merge_pull_request"}}
+ {{end}} + {{if $prUnit.PullRequestsConfig.AllowRebase}} +
{{$.i18n.Tr "repo.pulls.rebase_merge_pull_request"}}
+ {{end}} + {{if $prUnit.PullRequestsConfig.AllowRebaseMerge}} +
{{$.i18n.Tr "repo.pulls.rebase_merge_commit_pull_request"}}
+ {{end}} + {{if $prUnit.PullRequestsConfig.AllowSquash}} +
{{$.i18n.Tr "repo.pulls.squash_merge_pull_request"}}
+ {{end}} +
+
+
+ {{else}} +
+ + {{$.i18n.Tr "repo.pulls.no_merge_desc"}} +
+
+ + {{$.i18n.Tr "repo.pulls.no_merge_helper"}} +
+ {{end}} + {{end}} {{end}} {{else}}
diff --git a/templates/repo/pulls/status.tmpl b/templates/repo/pulls/status.tmpl index e1401aa8bb..76a4eb5423 100644 --- a/templates/repo/pulls/status.tmpl +++ b/templates/repo/pulls/status.tmpl @@ -15,7 +15,12 @@
{{template "repo/commit_status" .}} {{.Context}} {{.Description}} -
{{if .TargetURL}}Details{{end}}
+
+ {{if $.is_context_required}} + {{if (call $.is_context_required .Context)}}
Required
{{end}} + {{end}} + {{if .TargetURL}}Details{{end}} +
{{end}} -{{end}} \ No newline at end of file +{{end}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 066350f97a..067d1d9761 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -104,6 +104,38 @@ {{end}}
+
+
+ + +

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

+
+
+ +
+
+ + + + + + + {{range $.branch_status_check_contexts}} + + {{end}} + +
+ {{.i18n.Tr "repo.settings.protect_check_status_contexts_list"}} +
+ + + + {{.}} + {{if $.is_context_required}}{{if call $.is_context_required .}}
Required
{{end}}{{end}} +
+
+
+