Branch protection: Possibility to not use whitelist but allow anyone with write access (#9055)

* Possibility to not use whitelist but allow anyone with write access

* fix existing test

* rename migration function

* Try to give a better name for migration step

* Clear settings if higher level setting is not set

* Move official reviews to db instead of counting approvals each time

* migration

* fix

* fix migration

* fix migration

* Remove NOT NULL from EnableWhitelist as migration isn't possible

* Fix migration, reviews are connected to issues.

* Fix SQL query issues in GetReviewersByPullID.

* Simplify function GetReviewersByIssueID

* Handle reviewers that has been deleted

* Ensure reviews for test is in a well defined order

* Only clear and set official reviews when it is an approve or reject.

integrations/git_test.go

@@ -383,6 +383,7 @@ func doProtectBranch(ctx APITestContext, branch string, userToWhitelist string)
 			req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/%s", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), url.PathEscape(branch)), map[string]string{
 				"_csrf":            csrf,
 				"protected":        "on",
+				"enable_push":      "whitelist",
 				"enable_whitelist": "on",
 				"whitelist_users":  strconv.FormatInt(user.ID, 10),
 			})

models/branches.go

@@ -39,6 +39,7 @@ type ProtectedBranch struct {
 	MergeWhitelistTeamIDs     []int64            `xorm:"JSON TEXT"`
 	EnableStatusCheck         bool               `xorm:"NOT NULL DEFAULT false"`
 	StatusCheckContexts       []string           `xorm:"JSON TEXT"`
+	EnableApprovalsWhitelist  bool               `xorm:"NOT NULL DEFAULT false"`
 	ApprovalsWhitelistUserIDs []int64            `xorm:"JSON TEXT"`
 	ApprovalsWhitelistTeamIDs []int64            `xorm:"JSON TEXT"`
 	RequiredApprovals         int64              `xorm:"NOT NULL DEFAULT 0"`
@@ -53,10 +54,25 @@ func (protectBranch *ProtectedBranch) IsProtected() bool {
 
 // CanUserPush returns if some user could push to this protected branch
 func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool {
-	if !protectBranch.EnableWhitelist {
+	if !protectBranch.CanPush {
 		return false
 	}
 
+	if !protectBranch.EnableWhitelist {
+		if user, err := GetUserByID(userID); err != nil {
+			log.Error("GetUserByID: %v", err)
+			return false
+		} else if repo, err := GetRepositoryByID(protectBranch.RepoID); err != nil {
+			log.Error("GetRepositoryByID: %v", err)
+			return false
+		} else if writeAccess, err := HasAccessUnit(user, repo, UnitTypeCode, AccessModeWrite); err != nil {
+			log.Error("HasAccessUnit: %v", err)
+			return false
+		} else {
+			return writeAccess
+		}
+	}
+
 	if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) {
 		return true
 	}
@@ -95,6 +111,38 @@ func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool {
 	return in
 }
 
+// IsUserOfficialReviewer check if user is official reviewer for the branch (counts towards required approvals)
+func (protectBranch *ProtectedBranch) IsUserOfficialReviewer(user *User) (bool, error) {
+	return protectBranch.isUserOfficialReviewer(x, user)
+}
+
+func (protectBranch *ProtectedBranch) isUserOfficialReviewer(e Engine, user *User) (bool, error) {
+	repo, err := getRepositoryByID(e, protectBranch.RepoID)
+	if err != nil {
+		return false, err
+	}
+
+	if !protectBranch.EnableApprovalsWhitelist {
+		// Anyone with write access is considered official reviewer
+		writeAccess, err := hasAccessUnit(e, user, repo, UnitTypeCode, AccessModeWrite)
+		if err != nil {
+			return false, err
+		}
+		return writeAccess, nil
+	}
+
+	if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, user.ID) {
+		return true, nil
+	}
+
+	inTeam, err := isUserInTeams(e, user.ID, protectBranch.ApprovalsWhitelistTeamIDs)
+	if err != nil {
+		return false, err
+	}
+
+	return inTeam, nil
+}
+
 // HasEnoughApprovals returns true if pr has enough granted approvals.
 func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool {
 	if protectBranch.RequiredApprovals == 0 {
@@ -105,30 +153,16 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool {
 
 // 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.IssueID)
+	approvals, err := x.Where("issue_id = ?", pr.Issue.ID).
+		And("type = ?", ReviewTypeApprove).
+		And("official = ?", true).
+		Count(new(Review))
 	if err != nil {
-		log.Error("GetReviewersByPullID: %v", err)
+		log.Error("GetGrantedApprovalsCount: %v", 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("UsersInTeamsCount: %v", err)
-		return 0
-	}
-	return approvalTeamCount + approvals
+	return approvals
 }
 
 // GetProtectedBranchByRepoID getting protected branch by repo ID
@@ -139,8 +173,12 @@ func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
 
 // GetProtectedBranchBy getting protected branch by ID/Name
 func GetProtectedBranchBy(repoID int64, branchName string) (*ProtectedBranch, error) {
+	return getProtectedBranchBy(x, repoID, branchName)
+}
+
+func getProtectedBranchBy(e Engine, repoID int64, branchName string) (*ProtectedBranch, error) {
 	rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName}
-	has, err := x.Get(rel)
+	has, err := e.Get(rel)
 	if err != nil {
 		return nil, err
 	}

models/fixtures/review.yml

@@ -44,32 +44,32 @@
   reviewer_id: 2
   issue_id: 3
   content: "New review 3"
-  updated_unix: 946684810
-  created_unix: 946684810
+  updated_unix: 946684811
+  created_unix: 946684811
 -
   id: 7
   type: 3
   reviewer_id: 3
   issue_id: 3
   content: "New review 4"
-  updated_unix: 946684810
-  created_unix: 946684810
+  updated_unix: 946684812
+  created_unix: 946684812
 -
   id: 8
   type: 1
   reviewer_id: 4
   issue_id: 3
   content: "New review 5"
-  updated_unix: 946684810
-  created_unix: 946684810
+  updated_unix: 946684813
+  created_unix: 946684813
 -
   id: 9
   type: 3
   reviewer_id: 2
   issue_id: 3
   content: "New review 3 rejected"
-  updated_unix: 946684810
-  created_unix: 946684810
+  updated_unix: 946684814
+  created_unix: 946684814
 
 -
   id: 10
@@ -77,5 +77,5 @@
   reviewer_id: 100
   issue_id: 3
   content: "a deleted user's review"
-  updated_unix: 946684810
-  created_unix: 946684810
+  updated_unix: 946684815
+  created_unix: 946684815

models/migrations/migrations.go

@@ -276,6 +276,8 @@ var migrations = []Migration{
 	NewMigration("add can_create_org_repo to team", addCanCreateOrgRepoColumnForTeam),
 	// v110 -> v111
 	NewMigration("change review content type to text", changeReviewContentToText),
+	// v111 -> v112
+	NewMigration("update branch protection for can push and whitelist enable", addBranchProtectionCanPushAndEnableWhitelist),
 }
 
 // Migrate database to current version

models/migrations/v111.go

@@ -0,0 +1,87 @@
+// 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 (
+	"code.gitea.io/gitea/models"
+
+	"xorm.io/xorm"
+)
+
+func addBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error {
+
+	type ProtectedBranch struct {
+		CanPush                  bool  `xorm:"NOT NULL DEFAULT false"`
+		EnableApprovalsWhitelist bool  `xorm:"NOT NULL DEFAULT false"`
+		RequiredApprovals        int64 `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	type Review struct {
+		ID       int64 `xorm:"pk autoincr"`
+		Official bool  `xorm:"NOT NULL DEFAULT false"`
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	if err := sess.Sync2(new(ProtectedBranch)); err != nil {
+		return err
+	}
+
+	if err := sess.Sync2(new(Review)); err != nil {
+		return err
+	}
+
+	if _, err := sess.Exec("UPDATE `protected_branch` SET `can_push` = `enable_whitelist`"); err != nil {
+		return err
+	}
+	if _, err := sess.Exec("UPDATE `protected_branch` SET `enable_approvals_whitelist` = ? WHERE `required_approvals` > ?", true, 0); err != nil {
+		return err
+	}
+
+	var pageSize int64 = 20
+	qresult, err := sess.QueryInterface("SELECT max(id) as max_id FROM issue")
+	if err != nil {
+		return err
+	}
+	var totalIssues int64
+	totalIssues, ok := qresult[0]["max_id"].(int64)
+	if !ok {
+		// If there are no issues at all we ignore it
+		return nil
+	}
+	totalPages := totalIssues / pageSize
+
+	// Find latest review of each user in each pull request, and set official field if appropriate
+	reviews := []*models.Review{}
+	var page int64
+	for page = 0; page <= totalPages; page++ {
+		if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id > ? AND issue_id <= ? AND type in (?, ?) GROUP BY issue_id, reviewer_id)",
+			page*pageSize, (page+1)*pageSize, models.ReviewTypeApprove, models.ReviewTypeReject).
+			Find(&reviews); err != nil {
+			return err
+		}
+
+		for _, review := range reviews {
+			if err := review.LoadAttributes(); err != nil {
+				// Error might occur if user or issue doesn't exist, ignore it.
+				continue
+			}
+			official, err := models.IsOfficialReviewer(review.Issue, review.Reviewer)
+			if err != nil {
+				// Branch might not be proteced or other error, ignore it.
+				continue
+			}
+			review.Official = official
+
+			if _, err := sess.ID(review.ID).Cols("official").Update(review); err != nil {
+				return err
+			}
+		}
+
+	}
+
+	return sess.Commit()
+}

models/org_team.go

@@ -914,7 +914,11 @@ func RemoveTeamMember(team *Team, userID int64) error {
 
 // IsUserInTeams returns if a user in some teams
 func IsUserInTeams(userID int64, teamIDs []int64) (bool, error) {
-	return x.Where("uid=?", userID).In("team_id", teamIDs).Exist(new(TeamUser))
+	return isUserInTeams(x, userID, teamIDs)
+}
+
+func isUserInTeams(e Engine, userID int64, teamIDs []int64) (bool, error) {
+	return e.Where("uid=?", userID).In("team_id", teamIDs).Exist(new(TeamUser))
 }
 
 // UsersInTeamsCount counts the number of users which are in userIDs and teamIDs

models/pull.go

@@ -159,16 +159,20 @@ func (pr *PullRequest) loadIssue(e Engine) (err error) {
 
 // LoadProtectedBranch loads the protected branch of the base branch
 func (pr *PullRequest) LoadProtectedBranch() (err error) {
+	return pr.loadProtectedBranch(x)
+}
+
+func (pr *PullRequest) loadProtectedBranch(e Engine) (err error) {
 	if pr.BaseRepo == nil {
 		if pr.BaseRepoID == 0 {
 			return nil
 		}
-		pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID)
+		pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID)
 		if err != nil {
 			return
 		}
 	}
-	pr.ProtectedBranch, err = GetProtectedBranchBy(pr.BaseRepo.ID, pr.BaseBranch)
+	pr.ProtectedBranch, err = getProtectedBranchBy(e, pr.BaseRepo.ID, pr.BaseBranch)
 	return
 }
 

models/review.go

@@ -10,7 +10,6 @@ import (
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"xorm.io/builder"
-	"xorm.io/core"
 )
 
 // ReviewType defines the sort of feedback a review gives
@@ -53,6 +52,8 @@ type Review struct {
 	Issue      *Issue `xorm:"-"`
 	IssueID    int64  `xorm:"index"`
 	Content    string `xorm:"TEXT"`
+	// Official is a review made by an assigned approver (counts towards approval)
+	Official bool `xorm:"NOT NULL DEFAULT false"`
 
 	CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@@ -122,23 +123,6 @@ 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
@@ -180,6 +164,27 @@ type CreateReviewOptions struct {
 	Type     ReviewType
 	Issue    *Issue
 	Reviewer *User
+	Official bool
+}
+
+// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
+func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {
+	return isOfficialReviewer(x, issue, reviewer)
+}
+
+func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
+	pr, err := getPullRequestByIssueID(e, issue.ID)
+	if err != nil {
+		return false, err
+	}
+	if err = pr.loadProtectedBranch(e); err != nil {
+		return false, err
+	}
+	if pr.ProtectedBranch == nil {
+		return false, nil
+	}
+
+	return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
 }
 
 func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
@@ -190,6 +195,7 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
 		Reviewer:   opts.Reviewer,
 		ReviewerID: opts.Reviewer.ID,
 		Content:    opts.Content,
+		Official:   opts.Official,
 	}
 	if _, err := e.Insert(review); err != nil {
 		return nil, err
@@ -255,6 +261,8 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 		return nil, nil, err
 	}
 
+	var official = false
+
 	review, err := getCurrentReview(sess, doer, issue)
 	if err != nil {
 		if !IsErrReviewNotExist(err) {
@@ -265,12 +273,24 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 			return nil, nil, ContentEmptyErr{}
 		}
 
+		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
+			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
+			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
+				return nil, nil, err
+			}
+			official, err = isOfficialReviewer(sess, issue, doer)
+			if err != nil {
+				return nil, nil, err
+			}
+		}
+
 		// No current review. Create a new one!
 		review, err = createReview(sess, CreateReviewOptions{
 			Type:     reviewType,
 			Issue:    issue,
 			Reviewer: doer,
 			Content:  content,
+			Official: official,
 		})
 		if err != nil {
 			return nil, nil, err
@@ -283,10 +303,23 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 			return nil, nil, ContentEmptyErr{}
 		}
 
+		if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
+			// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
+			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
+				return nil, nil, err
+			}
+			official, err = isOfficialReviewer(sess, issue, doer)
+			if err != nil {
+				return nil, nil, err
+			}
+		}
+
+		review.Official = official
 		review.Issue = issue
 		review.Content = content
 		review.Type = reviewType
-		if _, err := sess.ID(review.ID).Cols("content, type").Update(review); err != nil {
+
+		if _, err := sess.ID(review.ID).Cols("content, type, official").Update(review); err != nil {
 			return nil, nil, err
 		}
 	}
@@ -307,46 +340,33 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 	return review, comm, sess.Commit()
 }
 
-// PullReviewersWithType represents the type used to display a review overview
-type PullReviewersWithType struct {
-	User              `xorm:"extends"`
-	Type              ReviewType
-	ReviewUpdatedUnix timeutil.TimeStamp `xorm:"review_updated_unix"`
-}
+// GetReviewersByIssueID gets the latest review of each reviewer for a pull request
+func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
+	reviewsUnfiltered := []*Review{}
 
-// GetReviewersByPullID gets all reviewers for a pull request with the statuses
-func GetReviewersByPullID(pullID int64) (issueReviewers []*PullReviewersWithType, err error) {
-	irs := []*PullReviewersWithType{}
-	if x.Dialect().DBType() == core.MSSQL {
-		err = x.SQL(`SELECT [user].*, review.type, review.review_updated_unix FROM
-(SELECT review.id, review.type, review.reviewer_id, max(review.updated_unix) as review_updated_unix
-FROM review WHERE review.issue_id=? AND (review.type = ? OR review.type = ?)
-GROUP BY review.id, review.type, review.reviewer_id) as review
-INNER JOIN [user] ON review.reviewer_id = [user].id ORDER BY review_updated_unix DESC`,
-			pullID, ReviewTypeApprove, ReviewTypeReject).
-			Find(&irs)
-	} else {
-		err = x.Select("`user`.*, review.type, max(review.updated_unix) as review_updated_unix").
-			Table("review").
-			Join("INNER", "`user`", "review.reviewer_id = `user`.id").
-			Where("review.issue_id = ? AND (review.type = ? OR review.type = ?)",
-				pullID, ReviewTypeApprove, ReviewTypeReject).
-			GroupBy("`user`.id, review.type").
-			OrderBy("review_updated_unix DESC").
-			Find(&irs)
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
 	}
 
-	// We need to group our results by user id _and_ review type, otherwise the query fails when using postgresql.
-	// But becaus we're doing this, we need to manually filter out multiple reviews of different types by the
-	// same person because we only want to show the newest review grouped by user. Thats why we're using a map here.
-	issueReviewers = []*PullReviewersWithType{}
-	usersInArray := make(map[int64]bool)
-	for _, ir := range irs {
-		if !usersInArray[ir.ID] {
-			issueReviewers = append(issueReviewers, ir)
-			usersInArray[ir.ID] = true
+	// Get latest review of each reviwer, sorted in order they were made
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
+		issueID, ReviewTypeApprove, ReviewTypeReject).
+		Find(&reviewsUnfiltered); err != nil {
+		return nil, err
+	}
+
+	// Load reviewer and skip if user is deleted
+	for _, review := range reviewsUnfiltered {
+		if err := review.loadReviewer(sess); err != nil {
+			if !IsErrUserNotExist(err) {
+				return nil, err
+			}
+		} else {
+			reviews = append(reviews, review)
 		}
 	}
 
-	return
+	return reviews, nil
 }

models/review_test.go

@@ -98,7 +98,7 @@ func TestCreateReview(t *testing.T) {
 	AssertExistsAndLoadBean(t, &Review{Content: "New Review"})
 }
 
-func TestGetReviewersByPullID(t *testing.T) {
+func TestGetReviewersByIssueID(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	issue := AssertExistsAndLoadBean(t, &Issue{ID: 3}).(*Issue)
@@ -106,24 +106,29 @@ func TestGetReviewersByPullID(t *testing.T) {
 	user3 := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
 	user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
 
-	expectedReviews := []*PullReviewersWithType{}
-	expectedReviews = append(expectedReviews, &PullReviewersWithType{
-		User:              *user2,
-		Type:              ReviewTypeReject,
-		ReviewUpdatedUnix: 946684810,
-	},
-		&PullReviewersWithType{
-			User:              *user3,
-			Type:              ReviewTypeReject,
-			ReviewUpdatedUnix: 946684810,
+	expectedReviews := []*Review{}
+	expectedReviews = append(expectedReviews,
+		&Review{
+			Reviewer:    user3,
+			Type:        ReviewTypeReject,
+			UpdatedUnix: 946684812,
 		},
-		&PullReviewersWithType{
-			User:              *user4,
-			Type:              ReviewTypeApprove,
-			ReviewUpdatedUnix: 946684810,
+		&Review{
+			Reviewer:    user4,
+			Type:        ReviewTypeApprove,
+			UpdatedUnix: 946684813,
+		},
+		&Review{
+			Reviewer:    user2,
+			Type:        ReviewTypeReject,
+			UpdatedUnix: 946684814,
 		})
 
-	allReviews, err := GetReviewersByPullID(issue.ID)
+	allReviews, err := GetReviewersByIssueID(issue.ID)
 	assert.NoError(t, err)
-	assert.Equal(t, expectedReviews, allReviews)
+	for i, review := range allReviews {
+		assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer)
+		assert.Equal(t, expectedReviews[i].Type, review.Type)
+		assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix)
+	}
 }

modules/auth/repo_form.go

@@ -157,19 +157,20 @@ 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
-	WhitelistDeployKeys     bool
-	EnableMergeWhitelist    bool
-	MergeWhitelistUsers     string
-	MergeWhitelistTeams     string
-	EnableStatusCheck       bool `xorm:"NOT NULL DEFAULT false"`
-	StatusCheckContexts     []string
-	RequiredApprovals       int64
-	ApprovalsWhitelistUsers string
-	ApprovalsWhitelistTeams string
+	Protected                bool
+	EnablePush               string
+	WhitelistUsers           string
+	WhitelistTeams           string
+	WhitelistDeployKeys      bool
+	EnableMergeWhitelist     bool
+	MergeWhitelistUsers      string
+	MergeWhitelistTeams      string
+	EnableStatusCheck        bool `xorm:"NOT NULL DEFAULT false"`
+	StatusCheckContexts      []string
+	RequiredApprovals        int64
+	EnableApprovalsWhitelist bool
+	ApprovalsWhitelistUsers  string
+	ApprovalsWhitelistTeams  string
 }
 
 // Validate validates the fields

options/locale/locale_en-US.ini

@@ -1382,9 +1382,13 @@ settings.protected_branch_can_push_yes = You can push
 settings.protected_branch_can_push_no = You can not push
 settings.branch_protection = Branch Protection for Branch '<b>%s</b>'
 settings.protect_this_branch = Enable Branch Protection
-settings.protect_this_branch_desc = Prevent deletion and disable any Git pushing to the branch.
-settings.protect_whitelist_committers = Enable Push Whitelist
-settings.protect_whitelist_committers_desc = Allow whitelisted users or teams to push to this branch (but not force push).
+settings.protect_this_branch_desc = Prevents deletion and restricts Git pushing and merging to the branch.
+settings.protect_disable_push = Disable Push
+settings.protect_disable_push_desc = No pushing will be allowed to this branch.
+settings.protect_enable_push = Enable Push
+settings.protect_enable_push_desc = Anyone with write access will be allowed to push to this branch (but not force push).
+settings.protect_whitelist_committers = Whitelist Restricted Push
+settings.protect_whitelist_committers_desc = Only whitelisted users or teams will be allowed to push to this branch (but not force push).
 settings.protect_whitelist_deploy_keys = Whitelist deploy keys with write access to push
 settings.protect_whitelist_users = Whitelisted users for pushing:
 settings.protect_whitelist_search_users = Search users…
@@ -1398,7 +1402,9 @@ 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. If no contexts are selected, the last commit must be successful regardless of context.
 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_required_approvals_desc = Allow only to merge pull request with enough positive reviews.
+settings.protect_approvals_whitelist_enabled = Restrict approvals to whitelisted users or teams
+settings.protect_approvals_whitelist_enabled_desc = Only reviews from whitelisted users or teams will count to the required approvals. Without approval whitelist, reviews from anyone with write access count to the required approvals. 
 settings.protect_approvals_whitelist_users = Whitelisted reviewers:
 settings.protect_approvals_whitelist_teams = Whitelisted teams for reviews:
 settings.add_protected_branch = Enable protection

routers/private/hook.go

@@ -98,7 +98,7 @@ func HookPreReceive(ctx *macaron.Context) {
 
 		canPush := false
 		if isDeployKey {
-			canPush = protectBranch.WhitelistDeployKeys
+			canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
 		} else {
 			canPush = protectBranch.CanUserPush(userID)
 		}

routers/repo/issue.go

@@ -935,9 +935,9 @@ func ViewIssue(ctx *context.Context) {
 		}
 		ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
 
-		ctx.Data["PullReviewersWithType"], err = models.GetReviewersByPullID(issue.ID)
+		ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID)
 		if err != nil {
-			ctx.ServerError("GetReviewersByPullID", err)
+			ctx.ServerError("GetReviewersByIssueID", err)
 			return
 		}
 	}

routers/repo/setting_protected_branch.go

@@ -196,32 +196,55 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
 		}
 
 		var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64
-		protectBranch.EnableWhitelist = f.EnableWhitelist
-		if strings.TrimSpace(f.WhitelistUsers) != "" {
-			whitelistUsers, _ = base.StringsToInt64s(strings.Split(f.WhitelistUsers, ","))
-		}
-		if strings.TrimSpace(f.WhitelistTeams) != "" {
-			whitelistTeams, _ = base.StringsToInt64s(strings.Split(f.WhitelistTeams, ","))
+		switch f.EnablePush {
+		case "all":
+			protectBranch.CanPush = true
+			protectBranch.EnableWhitelist = false
+			protectBranch.WhitelistDeployKeys = false
+		case "whitelist":
+			protectBranch.CanPush = true
+			protectBranch.EnableWhitelist = true
+			protectBranch.WhitelistDeployKeys = f.WhitelistDeployKeys
+			if strings.TrimSpace(f.WhitelistUsers) != "" {
+				whitelistUsers, _ = base.StringsToInt64s(strings.Split(f.WhitelistUsers, ","))
+			}
+			if strings.TrimSpace(f.WhitelistTeams) != "" {
+				whitelistTeams, _ = base.StringsToInt64s(strings.Split(f.WhitelistTeams, ","))
+			}
+		default:
+			protectBranch.CanPush = false
+			protectBranch.EnableWhitelist = false
+			protectBranch.WhitelistDeployKeys = false
 		}
+
 		protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist
-		if strings.TrimSpace(f.MergeWhitelistUsers) != "" {
-			mergeWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ","))
-		}
-		if strings.TrimSpace(f.MergeWhitelistTeams) != "" {
-			mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ","))
+		if f.EnableMergeWhitelist {
+			if strings.TrimSpace(f.MergeWhitelistUsers) != "" {
+				mergeWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ","))
+			}
+			if strings.TrimSpace(f.MergeWhitelistTeams) != "" {
+				mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ","))
+			}
 		}
 
 		protectBranch.EnableStatusCheck = f.EnableStatusCheck
-		protectBranch.StatusCheckContexts = f.StatusCheckContexts
-		protectBranch.WhitelistDeployKeys = f.WhitelistDeployKeys
+		if f.EnableStatusCheck {
+			protectBranch.StatusCheckContexts = f.StatusCheckContexts
+		} else {
+			protectBranch.StatusCheckContexts = nil
+		}
 
 		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, ","))
+		protectBranch.EnableApprovalsWhitelist = f.EnableApprovalsWhitelist
+		if f.EnableApprovalsWhitelist {
+			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,

services/pull/review.go

@@ -72,6 +72,7 @@ func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, conte
 			Type:     models.ReviewTypePending,
 			Reviewer: doer,
 			Issue:    issue,
+			Official: false,
 		})
 		if err != nil {
 			return nil, err

templates/repo/issue/view_content/pull.tmpl

@@ -1,10 +1,10 @@
-{{if gt (len .PullReviewersWithType) 0}}
+{{if gt (len .PullReviewers) 0}}
 	<div class="comment box">
 		<div class="content">
 			<div class="ui segment">
 				<h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4>
-				{{range .PullReviewersWithType}}
-					{{ $createdStr:= TimeSinceUnix .ReviewUpdatedUnix $.Lang }}
+				{{range .PullReviewers}}
+					{{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }}
 					<div class="ui divider"></div>
 					<div class="review-item">
 						<span class="type-icon text {{if eq .Type 1}}green
@@ -13,10 +13,10 @@
 							{{else}}grey{{end}}">
 							<span class="octicon octicon-{{.Type.Icon}}"></span>
 						</span>
-						<a class="ui avatar image" href="{{.HomeLink}}">
-							<img src="{{.RelAvatarLink}}">
+						<a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
+							<img src="{{.Reviewer.RelAvatarLink}}">
 						</a>
-						<span class="text grey"><a href="{{.HomeLink}}">{{.Name}}</a>
+						<span class="text grey"><a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
 							{{if eq .Type 1}}
 								{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
 							{{else if eq .Type 2}}

templates/repo/settings/protected_branch.tmpl

@@ -19,8 +19,22 @@
 				</div>
 				<div id="protection_box" class="fields {{if not .Branch.IsProtected}}disabled{{end}}">
 					<div class="field">
-						<div class="ui checkbox">
-							<input class="enable-whitelist" name="enable_whitelist" type="checkbox" data-target="#whitelist_box" {{if .Branch.EnableWhitelist}}checked{{end}}>
+						<div class="ui radio checkbox">
+							<input name="enable_push" type="radio" value="none" class="disable-whitelist" data-target="#whitelist_box" {{if not .Branch.CanPush}}checked{{end}}>
+							<label>{{.i18n.Tr "repo.settings.protect_disable_push"}}</label>
+							<p class="help">{{.i18n.Tr "repo.settings.protect_disable_push_desc"}}</p>
+						</div>
+					</div>
+					<div class="field">
+						<div class="ui radio checkbox">
+							<input name="enable_push" type="radio" value="all" class="disable-whitelist" data-target="#whitelist_box" {{if and (.Branch.CanPush) (not .Branch.EnableWhitelist)}}checked{{end}}>
+							<label>{{.i18n.Tr "repo.settings.protect_enable_push"}}</label>
+							<p class="help">{{.i18n.Tr "repo.settings.protect_enable_push_desc"}}</p>
+						</div>
+					</div>
+					<div class="field">
+						<div class="ui radio checkbox">
+							<input name="enable_push" type="radio" value="whitelist" class="enable-whitelist" data-target="#whitelist_box" {{if and (.Branch.CanPush) (.Branch.EnableWhitelist)}}checked{{end}}>
 							<label>{{.i18n.Tr "repo.settings.protect_whitelist_committers"}}</label>
 							<p class="help">{{.i18n.Tr "repo.settings.protect_whitelist_committers_desc"}}</p>
 						</div>
@@ -148,7 +162,14 @@
 						<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="field">
+						<div class="ui checkbox">
+							<input class="enable-whitelist" name="enable_approvals_whitelist" type="checkbox" data-target="#approvals_whitelist_box" {{if .Branch.EnableApprovalsWhitelist}}checked{{end}}>
+							<label>{{.i18n.Tr "repo.settings.protect_approvals_whitelist_enabled"}}</label>
+							<p class="help">{{.i18n.Tr "repo.settings.protect_approvals_whitelist_enabled_desc"}}</p>
+						</div>
+					</div>
+					<div id="approvals_whitelist_box" class="fields {{if not .Branch.EnableApprovalsWhitelist}}disabled{{end}}">
 						<div class="whitelist field">
 							<label>{{.i18n.Tr "repo.settings.protect_approvals_whitelist_users"}}</label>
 							<div class="ui multiple search selection dropdown">
@@ -164,24 +185,24 @@
 								</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}}
+						{{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>
-								{{end}}
 								</div>
 							</div>
-						</div>
-					{{end}}
+						{{end}}
 					</div>
 				</div>
 

web_src/js/index.js

@@ -1043,6 +1043,11 @@ function initRepository() {
         $($(this).data('target')).addClass('disabled');
       }
     });
+    $('.disable-whitelist').change(function () {
+      if (this.checked) {
+        $($(this).data('target')).addClass('disabled');
+      }
+    });
   }
 }