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.

3 files changed, 44 insertions, 21 deletions

diff --git a/routers/private/hook.go b/routers/private/hook.go
index c9065bceb3..2644302ead 100644
--- a/routers/private/hook.go
+++ b/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)
 		}
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index a2f4022a73..cabb9e63c1 100644
--- a/routers/repo/issue.go
+++ b/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
 		}
 	}
diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go
index bc4d7c3a9e..c279c94b1b 100644
--- a/routers/repo/setting_protected_branch.go
+++ b/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,