diff --git a/models/branches.go b/models/branches.go index bbcd342ba..f2bf9a80a 100644 --- a/models/branches.go +++ b/models/branches.go @@ -23,18 +23,21 @@ const ( // ProtectedBranch struct type ProtectedBranch struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"UNIQUE(s)"` - BranchName string `xorm:"UNIQUE(s)"` - CanPush bool `xorm:"NOT NULL DEFAULT false"` - EnableWhitelist bool - WhitelistUserIDs []int64 `xorm:"JSON TEXT"` - WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` - MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - CreatedUnix util.TimeStamp `xorm:"created"` - UpdatedUnix util.TimeStamp `xorm:"updated"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + BranchName string `xorm:"UNIQUE(s)"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` + EnableWhitelist bool + WhitelistUserIDs []int64 `xorm:"JSON TEXT"` + WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` + CreatedUnix util.TimeStamp `xorm:"created"` + UpdatedUnix util.TimeStamp `xorm:"updated"` } // IsProtected returns if the branch is protected @@ -86,6 +89,41 @@ func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool { return in } +// HasEnoughApprovals returns true if pr has enough granted approvals. +func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { + if protectBranch.RequiredApprovals == 0 { + return true + } + return protectBranch.GetGrantedApprovalsCount(pr) >= protectBranch.RequiredApprovals +} + +// GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist. +func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 { + reviews, err := GetReviewersByPullID(pr.ID) + if err != nil { + log.Error(1, "GetUniqueApprovalsByPullRequestID:", err) + return 0 + } + approvals := int64(0) + userIDs := make([]int64, 0) + for _, review := range reviews { + if review.Type != ReviewTypeApprove { + continue + } + if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, review.ID) { + approvals++ + continue + } + userIDs = append(userIDs, review.ID) + } + approvalTeamCount, err := UsersInTeamsCount(userIDs, protectBranch.ApprovalsWhitelistTeamIDs) + if err != nil { + log.Error(1, "UsersInTeamsCount:", err) + return 0 + } + return approvalTeamCount + approvals +} + // GetProtectedBranchByRepoID getting protected branch by repo ID func GetProtectedBranchByRepoID(RepoID int64) ([]*ProtectedBranch, error) { protectedBranches := make([]*ProtectedBranch, 0) @@ -118,40 +156,64 @@ func GetProtectedBranchByID(id int64) (*ProtectedBranch, error) { return rel, nil } +// WhitelistOptions represent all sorts of whitelists used for protected branches +type WhitelistOptions struct { + UserIDs []int64 + TeamIDs []int64 + + MergeUserIDs []int64 + MergeTeamIDs []int64 + + ApprovalsUserIDs []int64 + ApprovalsTeamIDs []int64 +} + // UpdateProtectBranch saves branch protection options of repository. // If ID is 0, it creates a new record. Otherwise, updates existing record. // This function also performs check if whitelist user and team's IDs have been changed // to avoid unnecessary whitelist delete and regenerate. -func UpdateProtectBranch(repo *Repository, protectBranch *ProtectedBranch, whitelistUserIDs, whitelistTeamIDs, mergeWhitelistUserIDs, mergeWhitelistTeamIDs []int64) (err error) { +func UpdateProtectBranch(repo *Repository, protectBranch *ProtectedBranch, opts WhitelistOptions) (err error) { if err = repo.GetOwner(); err != nil { return fmt.Errorf("GetOwner: %v", err) } - whitelist, err := updateUserWhitelist(repo, protectBranch.WhitelistUserIDs, whitelistUserIDs) + whitelist, err := updateUserWhitelist(repo, protectBranch.WhitelistUserIDs, opts.UserIDs) if err != nil { return err } protectBranch.WhitelistUserIDs = whitelist - whitelist, err = updateUserWhitelist(repo, protectBranch.MergeWhitelistUserIDs, mergeWhitelistUserIDs) + whitelist, err = updateUserWhitelist(repo, protectBranch.MergeWhitelistUserIDs, opts.MergeUserIDs) if err != nil { return err } protectBranch.MergeWhitelistUserIDs = whitelist + whitelist, err = updateUserWhitelist(repo, protectBranch.ApprovalsWhitelistUserIDs, opts.ApprovalsUserIDs) + if err != nil { + return err + } + protectBranch.ApprovalsWhitelistUserIDs = whitelist + // if the repo is in an organization - whitelist, err = updateTeamWhitelist(repo, protectBranch.WhitelistTeamIDs, whitelistTeamIDs) + whitelist, err = updateTeamWhitelist(repo, protectBranch.WhitelistTeamIDs, opts.TeamIDs) if err != nil { return err } protectBranch.WhitelistTeamIDs = whitelist - whitelist, err = updateTeamWhitelist(repo, protectBranch.MergeWhitelistTeamIDs, mergeWhitelistTeamIDs) + whitelist, err = updateTeamWhitelist(repo, protectBranch.MergeWhitelistTeamIDs, opts.MergeTeamIDs) if err != nil { return err } protectBranch.MergeWhitelistTeamIDs = whitelist + whitelist, err = updateTeamWhitelist(repo, protectBranch.ApprovalsWhitelistTeamIDs, opts.ApprovalsTeamIDs) + if err != nil { + return err + } + protectBranch.ApprovalsWhitelistTeamIDs = whitelist + // Make sure protectBranch.ID is not 0 for whitelists if protectBranch.ID == 0 { if _, err = x.Insert(protectBranch); err != nil { @@ -213,7 +275,7 @@ func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User) } // IsProtectedBranchForMerging checks if branch is protected for merging -func (repo *Repository) IsProtectedBranchForMerging(branchName string, doer *User) (bool, error) { +func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName string, doer *User) (bool, error) { if doer == nil { return true, nil } @@ -227,7 +289,7 @@ func (repo *Repository) IsProtectedBranchForMerging(branchName string, doer *Use if err != nil { return true, err } else if has { - return !protectedBranch.CanUserMerge(doer.ID), nil + return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr), nil } return false, nil @@ -270,14 +332,14 @@ func updateTeamWhitelist(repo *Repository, currentWhitelist, newWhitelist []int6 return currentWhitelist, nil } - teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeWrite) + teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead) if err != nil { return nil, fmt.Errorf("GetTeamsWithAccessToRepo [org_id: %d, repo_id: %d]: %v", repo.OwnerID, repo.ID, err) } whitelist = make([]int64, 0, len(teams)) for i := range teams { - if teams[i].HasWriteAccess() && com.IsSliceContainsInt64(newWhitelist, teams[i].ID) { + if com.IsSliceContainsInt64(newWhitelist, teams[i].ID) { whitelist = append(whitelist, teams[i].ID) } } diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 6ac5004eb..6c28989c2 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -200,6 +200,8 @@ var migrations = []Migration{ NewMigration("add review", addReview), // v73 -> v74 NewMigration("add must_change_password column for users table", addMustChangePassword), + // v74 -> v75 + NewMigration("add approval whitelists to protected branches", addApprovalWhitelistsToProtectedBranches), } // Migrate database to current version diff --git a/models/migrations/v74.go b/models/migrations/v74.go new file mode 100644 index 000000000..66e958c7f --- /dev/null +++ b/models/migrations/v74.go @@ -0,0 +1,16 @@ +// Copyright 2018 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 addApprovalWhitelistsToProtectedBranches(x *xorm.Engine) error { + type ProtectedBranch struct { + ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` + } + return x.Sync2(new(ProtectedBranch)) +} diff --git a/models/org_team.go b/models/org_team.go index cad4af250..34e1b4db8 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -700,6 +700,14 @@ func IsUserInTeams(userID int64, teamIDs []int64) (bool, error) { return x.Where("uid=?", userID).In("team_id", teamIDs).Exist(new(TeamUser)) } +// UsersInTeamsCount counts the number of users which are in userIDs and teamIDs +func UsersInTeamsCount(userIDs []int64, teamIDs []int64) (count int64, err error) { + if count, err = x.In("uid", userIDs).In("team_id", teamIDs).Count(new(TeamUser)); err != nil { + return 0, err + } + return +} + // ___________ __________ // \__ ___/___ _____ _____\______ \ ____ ______ ____ // | |_/ __ \\__ \ / \| _// __ \\____ \ / _ \ diff --git a/models/org_team_test.go b/models/org_team_test.go index f9f1f289e..87bfbb484 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -346,3 +346,17 @@ func TestHasTeamRepo(t *testing.T) { test(2, 3, true) test(2, 5, false) } + +func TestUsersInTeamsCount(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + test := func(teamIDs []int64, userIDs []int64, expected int64) { + count, err := UsersInTeamsCount(teamIDs, userIDs) + assert.NoError(t, err) + assert.Equal(t, expected, count) + } + + test([]int64{2}, []int64{1, 2, 3, 4}, 2) + test([]int64{1, 2, 3, 4, 5}, []int64{2, 5}, 2) + test([]int64{1, 2, 3, 4, 5}, []int64{2, 3, 5}, 3) +} diff --git a/models/pull.go b/models/pull.go index e97faa8f5..0041f83dc 100644 --- a/models/pull.go +++ b/models/pull.go @@ -60,14 +60,15 @@ type PullRequest struct { Issue *Issue `xorm:"-"` Index int64 - HeadRepoID int64 `xorm:"INDEX"` - HeadRepo *Repository `xorm:"-"` - BaseRepoID int64 `xorm:"INDEX"` - BaseRepo *Repository `xorm:"-"` - HeadUserName string - HeadBranch string - BaseBranch string - MergeBase string `xorm:"VARCHAR(40)"` + HeadRepoID int64 `xorm:"INDEX"` + HeadRepo *Repository `xorm:"-"` + BaseRepoID int64 `xorm:"INDEX"` + BaseRepo *Repository `xorm:"-"` + HeadUserName string + HeadBranch string + BaseBranch string + ProtectedBranch *ProtectedBranch `xorm:"-"` + MergeBase string `xorm:"VARCHAR(40)"` HasMerged bool `xorm:"INDEX"` MergedCommitID string `xorm:"VARCHAR(40)"` @@ -110,6 +111,12 @@ func (pr *PullRequest) loadIssue(e Engine) (err error) { return err } +// LoadProtectedBranch loads the protected branch of the base branch +func (pr *PullRequest) LoadProtectedBranch() (err error) { + pr.ProtectedBranch, err = GetProtectedBranchBy(pr.BaseRepo.ID, pr.BaseBranch) + return +} + // GetDefaultMergeMessage returns default message used when merging pull request func (pr *PullRequest) GetDefaultMergeMessage() string { if pr.HeadRepo == nil { @@ -288,7 +295,7 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) { } } - if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr.BaseBranch, doer); err != nil { + if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr, pr.BaseBranch, doer); err != nil { return fmt.Errorf("IsProtectedBranch: %v", err) } else if protected { return ErrNotAllowedToMerge{ diff --git a/models/review.go b/models/review.go index 3ae1dd457..91b6d6dbb 100644 --- a/models/review.go +++ b/models/review.go @@ -161,6 +161,23 @@ func GetReviewByID(id int64) (*Review, error) { return getReviewByID(x, id) } +func getUniqueApprovalsByPullRequestID(e Engine, prID int64) (reviews []*Review, err error) { + reviews = make([]*Review, 0) + if err := e. + Where("issue_id = ? AND type = ?", prID, ReviewTypeApprove). + OrderBy("updated_unix"). + GroupBy("reviewer_id"). + Find(&reviews); err != nil { + return nil, err + } + return +} + +// GetUniqueApprovalsByPullRequestID returns all reviews submitted for a specific pull request +func GetUniqueApprovalsByPullRequestID(prID int64) ([]*Review, error) { + return getUniqueApprovalsByPullRequestID(x, prID) +} + // FindReviewOptions represent possible filters to find reviews type FindReviewOptions struct { Type ReviewType diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index a4a00d53b..f86506137 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -135,13 +135,16 @@ func (f *RepoSettingForm) Validate(ctx *macaron.Context, errs binding.Errors) bi // ProtectBranchForm form for changing protected branch settings type ProtectBranchForm struct { - Protected bool - EnableWhitelist bool - WhitelistUsers string - WhitelistTeams string - EnableMergeWhitelist bool - MergeWhitelistUsers string - MergeWhitelistTeams string + Protected bool + EnableWhitelist bool + WhitelistUsers string + WhitelistTeams string + EnableMergeWhitelist bool + MergeWhitelistUsers string + MergeWhitelistTeams string + RequiredApprovals int64 + ApprovalsWhitelistUsers string + ApprovalsWhitelistTeams string } // Validate validates the fields diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index bf4c4964f..1a748e8b2 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -859,6 +859,7 @@ pulls.title_wip_desc = `Start the title with %s pulls.cannot_merge_work_in_progress = This pull request is marked as a work in progress. Remove the %s prefix from the title when it's ready pulls.data_broken = This pull request is broken due to missing fork information. pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments." +pulls.blocked_by_approvals = "This Pull Request hasn't 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. pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. @@ -1149,6 +1150,10 @@ 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_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: +settings.protect_approvals_whitelist_teams = Whitelisted teams for reviews: settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection settings.update_protect_branch_success = Branch protection for branch '%s' has been updated. @@ -1159,6 +1164,7 @@ settings.default_branch_desc = Select a default repository branch for pull reque settings.choose_branch = Choose a branch… settings.no_protected_branch = There are no protected branches. settings.edit_protected_branch = Edit +settings.protected_branch_required_approvals_min = Required approvals cannot be negative. diff.browse_source = Browse Source diff.parent = parent diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 34a01617e..dd1c5cfa5 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -828,6 +828,14 @@ func ViewIssue(ctx *context.Context) { ctx.Data["MergeStyle"] = "" } } + if err = pull.LoadProtectedBranch(); err != nil { + ctx.ServerError("LoadProtectedBranch", err) + return + } + if pull.ProtectedBranch != nil { + ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) + ctx.Data["GrantedApprovals"] = pull.ProtectedBranch.GetGrantedApprovalsCount(pull) + } ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) ctx.Data["PullReviewersWithType"], err = models.GetReviewersByPullID(issue.ID) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index c8f6f843d..def27753d 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -124,9 +124,10 @@ func SettingsProtectedBranch(c *context.Context) { c.Data["Users"] = users 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), ",") if c.Repo.Owner.IsOrganization() { - teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeWrite) + teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeRead) if err != nil { c.ServerError("Repo.Owner.TeamsWithAccessToRepo", err) return @@ -134,6 +135,7 @@ func SettingsProtectedBranch(c *context.Context) { c.Data["Teams"] = teams c.Data["whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.WhitelistTeamIDs), ",") c.Data["merge_whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.MergeWhitelistTeamIDs), ",") + c.Data["approvals_whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.ApprovalsWhitelistTeamIDs), ",") } c.Data["Branch"] = protectBranch @@ -164,8 +166,12 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) BranchName: branch, } } + if f.RequiredApprovals < 0 { + ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min")) + ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, branch)) + } - var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams []int64 + var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64 protectBranch.EnableWhitelist = f.EnableWhitelist if strings.TrimSpace(f.WhitelistUsers) != "" { whitelistUsers, _ = base.StringsToInt64s(strings.Split(f.WhitelistUsers, ",")) @@ -180,7 +186,21 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) if strings.TrimSpace(f.MergeWhitelistTeams) != "" { mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) } - err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams) + protectBranch.RequiredApprovals = f.RequiredApprovals + if strings.TrimSpace(f.ApprovalsWhitelistUsers) != "" { + approvalsWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistUsers, ",")) + } + if strings.TrimSpace(f.ApprovalsWhitelistTeams) != "" { + approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ",")) + } + err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ + UserIDs: whitelistUsers, + TeamIDs: whitelistTeams, + MergeUserIDs: mergeWhitelistUsers, + MergeTeamIDs: mergeWhitelistTeams, + ApprovalsUserIDs: approvalsWhitelistUsers, + ApprovalsTeamIDs: approvalsWhitelistTeams, + }) if err != nil { ctx.ServerError("UpdateProtectBranch", err) return diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 174937f90..5edde9051 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -39,6 +39,7 @@ {{else if .Issue.IsClosed}}grey {{else if .IsPullWorkInProgress}}grey {{else if .IsPullRequestBroken}}red + {{else if .IsBlockedByApprovals}}red {{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.CanAutoMerge}}green {{else}}red{{end}}"> @@ -68,6 +69,11 @@ {{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}} + {{else if .IsBlockedByApprovals}} +
{{.i18n.Tr "repo.settings.protect_required_approvals_desc"}}
+