Approvals at Branch Protection (#5350)

* Add branch protection for approvals

Signed-off-by: Jonas Franz <info@jonasfranz.software>

* Add required approvals

Signed-off-by: Jonas Franz <info@jonasfranz.software>

* Add missing comments and fmt

Signed-off-by: Jonas Franz <info@jonasfranz.software>

* Add type = approval and group by reviewer_id to review

* Prevent users from adding negative review limits

* Add migration for approval whitelists

Signed-off-by: Jonas Franz <info@jonasfranz.software>

models/branches.go

@@ -33,6 +33,9 @@ type ProtectedBranch struct {
 	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"`
 }
@@ -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)
 		}
 	}

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

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))
+}

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
+}
+
 // ___________                  __________
 // \__    ___/___ _____    _____\______   \ ____ ______   ____
 //   |    |_/ __ \\__  \  /     \|       _// __ \\____ \ /  _ \

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)
+}

models/pull.go

@@ -67,6 +67,7 @@ type PullRequest struct {
 	HeadUserName    string
 	HeadBranch      string
 	BaseBranch      string
+	ProtectedBranch *ProtectedBranch `xorm:"-"`
 	MergeBase       string           `xorm:"VARCHAR(40)"`
 
 	HasMerged      bool           `xorm:"INDEX"`
@@ -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{

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

modules/auth/repo_form.go

@@ -142,6 +142,9 @@ type ProtectBranchForm struct {
 	EnableMergeWhitelist    bool
 	MergeWhitelistUsers     string
 	MergeWhitelistTeams     string
+	RequiredApprovals       int64
+	ApprovalsWhitelistUsers string
+	ApprovalsWhitelistTeams string
 }
 
 // Validate validates the fields

options/locale/locale_en-US.ini

@@ -859,6 +859,7 @@ pulls.title_wip_desc = `<a href="#">Start the title with <strong>%s</strong></a>
 pulls.cannot_merge_work_in_progress = This pull request is marked as a work in progress. Remove the <strong>%s</strong> 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

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)

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

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}}"><span class="mega-octicon octicon-git-merge"></span></a>
@@ -68,6 +69,11 @@
 					<span class="octicon octicon-x"></span>
 					{{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}}
 				</div>
+			{{else if .IsBlockedByApprovals}}
+				<div class="item text red">
+					<span class="octicon octicon-x"></span>
+				{{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}}
+				</div>
 			{{else if .Issue.PullRequest.IsChecking}}
 				<div class="item text yellow">
 					<span class="octicon octicon-sync"></span>

templates/repo/settings/protected_branch.tmpl

@@ -103,6 +103,47 @@
 						</div>
 					{{end}}
 					</div>
+
+					<div class="field">
+						<label for="required-approvals">{{.i18n.Tr "repo.settings.protect_required_approvals"}}</label>
+						<input name="required_approvals" id="required-approvals" type="number" value="{{.Branch.RequiredApprovals}}">
+						<p class="help">{{.i18n.Tr "repo.settings.protect_required_approvals_desc"}}</p>
+					</div>
+					<div class="fields">
+						<div class="whitelist field">
+							<label>{{.i18n.Tr "repo.settings.protect_approvals_whitelist_users"}}</label>
+							<div class="ui multiple search selection dropdown">
+								<input type="hidden" name="approvals_whitelist_users" value="{{.approvals_whitelist_users}}">
+								<div class="default text">{{.i18n.Tr "repo.settings.protect_whitelist_search_users"}}</div>
+								<div class="menu">
+								{{range .Users}}
+									<div class="item" data-value="{{.ID}}">
+										<img class="ui mini image" src="{{.RelAvatarLink}}">
+									{{.Name}}
+									</div>
+								{{end}}
+								</div>
+							</div>
+						</div>
+					{{if .Owner.IsOrganization}}
+						<br>
+						<div class="whitelist field">
+							<label>{{.i18n.Tr "repo.settings.protect_approvals_whitelist_teams"}}</label>
+							<div class="ui multiple search selection dropdown">
+								<input type="hidden" name="approvals_whitelist_teams" value="{{.approvals_whitelist_teams}}">
+								<div class="default text">{{.i18n.Tr "repo.settings.protect_whitelist_search_teams"}}</div>
+								<div class="menu">
+								{{range .Teams}}
+									<div class="item" data-value="{{.ID}}">
+										<i class="octicon octicon-jersey"></i>
+									{{.Name}}
+									</div>
+								{{end}}
+								</div>
+							</div>
+						</div>
+					{{end}}
+					</div>
 				</div>
 
 				<div class="ui divider"></div>