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.

Author: davidsvantesson

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,38 +44,38 @@
   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
   type: 3
   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
 }