Add protected branch whitelists for merging (#3689)

* Add database migrations for merge whitelist

* Add merge whitelist settings for protected branches

* Add checks for merge whitelists

models/branches.go

@@ -23,15 +23,18 @@ const (
 
 // ProtectedBranch struct
 type ProtectedBranch struct {
-	ID               int64  `xorm:"pk autoincr"`
+	ID                    int64  `xorm:"pk autoincr"`
-	RepoID           int64  `xorm:"UNIQUE(s)"`
+	RepoID                int64  `xorm:"UNIQUE(s)"`
-	BranchName       string `xorm:"UNIQUE(s)"`
+	BranchName            string `xorm:"UNIQUE(s)"`
-	CanPush          bool   `xorm:"NOT NULL DEFAULT false"`
+	CanPush               bool   `xorm:"NOT NULL DEFAULT false"`
-	EnableWhitelist  bool
+	EnableWhitelist       bool
-	WhitelistUserIDs []int64        `xorm:"JSON TEXT"`
+	WhitelistUserIDs      []int64        `xorm:"JSON TEXT"`
-	WhitelistTeamIDs []int64        `xorm:"JSON TEXT"`
+	WhitelistTeamIDs      []int64        `xorm:"JSON TEXT"`
-	CreatedUnix      util.TimeStamp `xorm:"created"`
+	EnableMergeWhitelist  bool           `xorm:"NOT NULL DEFAULT false"`
-	UpdatedUnix      util.TimeStamp `xorm:"updated"`
+	MergeWhitelistUserIDs []int64        `xorm:"JSON TEXT"`
+	MergeWhitelistTeamIDs []int64        `xorm:"JSON TEXT"`
+	CreatedUnix           util.TimeStamp `xorm:"created"`
+	UpdatedUnix           util.TimeStamp `xorm:"updated"`
 }
 
 // IsProtected returns if the branch is protected
@@ -61,6 +64,28 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool {
 	return in
 }
 
+// CanUserMerge returns if some user could merge a pull request to this protected branch
+func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool {
+	if !protectBranch.EnableMergeWhitelist {
+		return true
+	}
+
+	if base.Int64sContains(protectBranch.MergeWhitelistUserIDs, userID) {
+		return true
+	}
+
+	if len(protectBranch.WhitelistTeamIDs) == 0 {
+		return false
+	}
+
+	in, err := IsUserInTeams(userID, protectBranch.MergeWhitelistTeamIDs)
+	if err != nil {
+		log.Error(1, "IsUserInTeams:", err)
+		return false
+	}
+	return in
+}
+
 // GetProtectedBranchByRepoID getting protected branch by repo ID
 func GetProtectedBranchByRepoID(RepoID int64) ([]*ProtectedBranch, error) {
 	protectedBranches := make([]*ProtectedBranch, 0)
@@ -97,40 +122,35 @@ func GetProtectedBranchByID(id int64) (*ProtectedBranch, error) {
 // 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 []int64) (err error) {
+func UpdateProtectBranch(repo *Repository, protectBranch *ProtectedBranch, whitelistUserIDs, whitelistTeamIDs, mergeWhitelistUserIDs, mergeWhitelistTeamIDs []int64) (err error) {
 	if err = repo.GetOwner(); err != nil {
 		return fmt.Errorf("GetOwner: %v", err)
 	}
 
-	hasUsersChanged := !util.IsSliceInt64Eq(protectBranch.WhitelistUserIDs, whitelistUserIDs)
+	whitelist, err := updateUserWhitelist(repo, protectBranch.WhitelistUserIDs, whitelistUserIDs)
-	if hasUsersChanged {
+	if err != nil {
-		protectBranch.WhitelistUserIDs = make([]int64, 0, len(whitelistUserIDs))
+		return err
-		for _, userID := range whitelistUserIDs {
-			has, err := hasAccess(x, userID, repo, AccessModeWrite)
-			if err != nil {
-				return fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, protectBranch.RepoID, err)
-			} else if !has {
-				continue // Drop invalid user ID
-			}
-
-			protectBranch.WhitelistUserIDs = append(protectBranch.WhitelistUserIDs, userID)
-		}
 	}
+	protectBranch.WhitelistUserIDs = whitelist
 
-	// if the repo is in an orgniziation
+	whitelist, err = updateUserWhitelist(repo, protectBranch.MergeWhitelistUserIDs, mergeWhitelistUserIDs)
-	hasTeamsChanged := !util.IsSliceInt64Eq(protectBranch.WhitelistTeamIDs, whitelistTeamIDs)
+	if err != nil {
-	if hasTeamsChanged {
+		return err
-		teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeWrite)
-		if err != nil {
-			return fmt.Errorf("GetTeamsWithAccessToRepo [org_id: %d, repo_id: %d]: %v", repo.OwnerID, repo.ID, err)
-		}
-		protectBranch.WhitelistTeamIDs = make([]int64, 0, len(teams))
-		for i := range teams {
-			if teams[i].HasWriteAccess() && com.IsSliceContainsInt64(whitelistTeamIDs, teams[i].ID) {
-				protectBranch.WhitelistTeamIDs = append(protectBranch.WhitelistTeamIDs, teams[i].ID)
-			}
-		}
 	}
+	protectBranch.MergeWhitelistUserIDs = whitelist
+
+	// if the repo is in an organization
+	whitelist, err = updateTeamWhitelist(repo, protectBranch.WhitelistTeamIDs, whitelistTeamIDs)
+	if err != nil {
+		return err
+	}
+	protectBranch.WhitelistTeamIDs = whitelist
+
+	whitelist, err = updateTeamWhitelist(repo, protectBranch.MergeWhitelistTeamIDs, mergeWhitelistTeamIDs)
+	if err != nil {
+		return err
+	}
+	protectBranch.MergeWhitelistTeamIDs = whitelist
 
 	// Make sure protectBranch.ID is not 0 for whitelists
 	if protectBranch.ID == 0 {
@@ -174,6 +194,73 @@ func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool,
 	return false, nil
 }
 
+// IsProtectedBranchForMerging checks if branch is protected for merging
+func (repo *Repository) IsProtectedBranchForMerging(branchName string, doer *User) (bool, error) {
+	if doer == nil {
+		return true, nil
+	}
+
+	protectedBranch := &ProtectedBranch{
+		RepoID:     repo.ID,
+		BranchName: branchName,
+	}
+
+	has, err := x.Get(protectedBranch)
+	if err != nil {
+		return true, err
+	} else if has {
+		return !protectedBranch.CanUserMerge(doer.ID), nil
+	}
+
+	return false, nil
+}
+
+// updateUserWhitelist checks whether the user whitelist changed and returns a whitelist with
+// the users from newWhitelist which have write access to the repo.
+func updateUserWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {
+	hasUsersChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist)
+	if !hasUsersChanged {
+		return currentWhitelist, nil
+	}
+
+	whitelist = make([]int64, 0, len(newWhitelist))
+	for _, userID := range newWhitelist {
+		has, err := hasAccess(x, userID, repo, AccessModeWrite)
+		if err != nil {
+			return nil, fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
+		} else if !has {
+			continue // Drop invalid user ID
+		}
+
+		whitelist = append(whitelist, userID)
+	}
+
+	return
+}
+
+// updateTeamWhitelist checks whether the team whitelist changed and returns a whitelist with
+// the teams from newWhitelist which have write access to the repo.
+func updateTeamWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {
+	hasTeamsChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist)
+	if !hasTeamsChanged {
+		return currentWhitelist, nil
+	}
+
+	teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeWrite)
+	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) {
+			whitelist = append(whitelist, teams[i].ID)
+		}
+	}
+
+	return
+}
+
 // DeleteProtectedBranch removes ProtectedBranch relation between the user and repository.
 func (repo *Repository) DeleteProtectedBranch(id int64) (err error) {
 	protectedBranch := &ProtectedBranch{

models/migrations/migrations.go

@@ -170,6 +170,8 @@ var migrations = []Migration{
 	NewMigration("add closed_unix column for issues", addIssueClosedTime),
 	// v58 -> v59
 	NewMigration("add label descriptions", addLabelsDescriptions),
+	// v59 -> v60
+	NewMigration("add merge whitelist for protected branches", addProtectedBranchMergeWhitelist),
 }
 
 // Migrate database to current version

models/migrations/v59.go

@@ -0,0 +1,24 @@
+// 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 (
+	"fmt"
+
+	"github.com/go-xorm/xorm"
+)
+
+func addProtectedBranchMergeWhitelist(x *xorm.Engine) error {
+	type ProtectedBranch struct {
+		EnableMergeWhitelist  bool    `xorm:"NOT NULL DEFAULT false"`
+		MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
+		MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
+	}
+
+	if err := x.Sync2(new(ProtectedBranch)); err != nil {
+		return fmt.Errorf("Sync2: %v", err)
+	}
+	return nil
+}

models/pull.go

@@ -286,7 +286,7 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) {
 		}
 	}
 
-	if protected, err := pr.BaseRepo.IsProtectedBranch(pr.BaseBranch, doer); err != nil {
+	if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr.BaseBranch, doer); err != nil {
 		return fmt.Errorf("IsProtectedBranch: %v", err)
 	} else if protected {
 		return ErrNotAllowedToMerge{

modules/auth/repo_form.go

@@ -129,10 +129,13 @@ func (f *RepoSettingForm) Validate(ctx *macaron.Context, errs binding.Errors) bi
 
 // ProtectBranchForm form for changing protected branch settings
 type ProtectBranchForm struct {
-	Protected       bool
+	Protected            bool
-	EnableWhitelist bool
+	EnableWhitelist      bool
-	WhitelistUsers  string
+	WhitelistUsers       string
-	WhitelistTeams  string
+	WhitelistTeams       string
+	EnableMergeWhitelist bool
+	MergeWhitelistUsers  string
+	MergeWhitelistTeams  string
 }
 
 // Validate validates the fields

options/locale/locale_en-US.ini

@@ -1028,6 +1028,10 @@ settings.protect_whitelist_users = Users who can push to this branch
 settings.protect_whitelist_search_users = Search users
 settings.protect_whitelist_teams = Teams whose members can push to this branch.
 settings.protect_whitelist_search_teams = Search teams
+settings.protect_merge_whitelist_committers = Restrict who can merge pull requests to this branch
+settings.protect_merge_whitelist_committers_desc = Add users or teams to this branch's merge whitelist. Only whitelisted users can merge pull requests to this branch. If not checked, anyone with write permissions can merge pull requests to this branch.
+settings.protect_merge_whitelist_users = Users who can merge pull requests to this branch
+settings.protect_merge_whitelist_teams = Teams whose members can merge pull requests to this branch.
 settings.add_protected_branch=Enable protection
 settings.delete_protected_branch=Disable protection
 settings.update_protect_branch_success = Branch %s protect options changed successfully.

routers/repo/setting_protected_branch.go

@@ -123,6 +123,7 @@ 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), ",")
 
 	if c.Repo.Owner.IsOrganization() {
 		teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeWrite)
@@ -132,6 +133,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["Branch"] = protectBranch
@@ -166,7 +168,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
 		protectBranch.EnableWhitelist = f.EnableWhitelist
 		whitelistUsers, _ := base.StringsToInt64s(strings.Split(f.WhitelistUsers, ","))
 		whitelistTeams, _ := base.StringsToInt64s(strings.Split(f.WhitelistTeams, ","))
-		err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams)
+		protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist
+		mergeWhitelistUsers, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ","))
+		mergeWhitelistTeams, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ","))
+		err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams)
 		if err != nil {
 			ctx.ServerError("UpdateProtectBranch", err)
 			return

templates/repo/settings/protected_branch.tmpl

@@ -60,6 +60,49 @@
 							</div>
 						{{end}}
 					</div>
+
+					<div class="field">
+						<div class="ui checkbox">
+							<input class="enable-whitelist" name="enable_merge_whitelist" type="checkbox" data-target="#merge_whitelist_box" {{if .Branch.EnableMergeWhitelist}}checked{{end}}>
+							<label>{{.i18n.Tr "repo.settings.protect_merge_whitelist_committers"}}</label>
+							<p class="help">{{.i18n.Tr "repo.settings.protect_merge_whitelist_committers_desc"}}</p>
+						</div>
+					</div>
+					<div id="merge_whitelist_box" class="fields {{if not .Branch.EnableMergeWhitelist}}disabled{{end}}">
+						<div class="whitelist field">
+							<label>{{.i18n.Tr "repo.settings.protect_merge_whitelist_users"}}</label>
+							<div class="ui multiple search selection dropdown">
+								<input type="hidden" name="merge_whitelist_users" value="{{.merge_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_merge_whitelist_teams"}}</label>
+							<div class="ui multiple search selection dropdown">
+								<input type="hidden" name="merge_whitelist_teams" value="{{.merge_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>