From 602fe459364fe1da01471a1c766127ec25b77b38 Mon Sep 17 00:00:00 2001
From: Lunny Xiao <xiaolunwen@gmail.com>
Date: Sat, 21 Mar 2020 05:31:01 +0800
Subject: [PATCH] Fix bug on branch API (#10767) (#10775)

* Fix bug on branch API (#10767)

* Fix branch api canPush and canMerge
---
 integrations/api_branch_test.go |  2 ++
 models/branches.go              | 22 ++++++++++++++++++++++
 modules/convert/convert.go      | 28 +++++++++++++++++++++-------
 routers/api/v1/repo/branch.go   | 17 +++++++++++++++--
 4 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/integrations/api_branch_test.go b/integrations/api_branch_test.go
index 037a42dee..2d64e9360 100644
--- a/integrations/api_branch_test.go
+++ b/integrations/api_branch_test.go
@@ -28,6 +28,8 @@ func testAPIGetBranch(t *testing.T, branchName string, exists bool) {
 	var branch api.Branch
 	DecodeJSON(t, resp, &branch)
 	assert.EqualValues(t, branchName, branch.Name)
+	assert.True(t, branch.UserCanPush)
+	assert.True(t, branch.UserCanMerge)
 }
 
 func TestAPIGetBranch(t *testing.T) {
diff --git a/models/branches.go b/models/branches.go
index 385817e4f..715b9d3a3 100644
--- a/models/branches.go
+++ b/models/branches.go
@@ -113,6 +113,28 @@ func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool {
 	return in
 }
 
+// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch
+func (protectBranch *ProtectedBranch) IsUserMergeWhitelisted(userID int64) bool {
+	if !protectBranch.EnableMergeWhitelist {
+		return true
+	}
+
+	if base.Int64sContains(protectBranch.MergeWhitelistUserIDs, userID) {
+		return true
+	}
+
+	if len(protectBranch.MergeWhitelistTeamIDs) == 0 {
+		return false
+	}
+
+	in, err := IsUserInTeams(userID, protectBranch.MergeWhitelistTeamIDs)
+	if err != nil {
+		log.Error("IsUserInTeams: %v", err)
+		return false
+	}
+	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)
diff --git a/modules/convert/convert.go b/modules/convert/convert.go
index 447965317..b8c7dc562 100644
--- a/modules/convert/convert.go
+++ b/modules/convert/convert.go
@@ -30,8 +30,17 @@ func ToEmail(email *models.EmailAddress) *api.Email {
 }
 
 // ToBranch convert a git.Commit and git.Branch to an api.Branch
-func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models.ProtectedBranch, user *models.User) *api.Branch {
+func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models.ProtectedBranch, user *models.User) (*api.Branch, error) {
 	if bp == nil {
+		var hasPerm bool
+		var err error
+		if user != nil {
+			hasPerm, err = models.HasAccessUnit(user, repo, models.UnitTypeCode, models.AccessModeWrite)
+			if err != nil {
+				return nil, err
+			}
+		}
+
 		return &api.Branch{
 			Name:                b.Name,
 			Commit:              ToCommit(repo, c),
@@ -39,20 +48,25 @@ func ToBranch(repo *models.Repository, b *git.Branch, c *git.Commit, bp *models.
 			RequiredApprovals:   0,
 			EnableStatusCheck:   false,
 			StatusCheckContexts: []string{},
-			UserCanPush:         true,
-			UserCanMerge:        true,
-		}
+			UserCanPush:         hasPerm,
+			UserCanMerge:        hasPerm,
+		}, nil
 	}
-	return &api.Branch{
+
+	branch := &api.Branch{
 		Name:                b.Name,
 		Commit:              ToCommit(repo, c),
 		Protected:           true,
 		RequiredApprovals:   bp.RequiredApprovals,
 		EnableStatusCheck:   bp.EnableStatusCheck,
 		StatusCheckContexts: bp.StatusCheckContexts,
-		UserCanPush:         bp.CanUserPush(user.ID),
-		UserCanMerge:        bp.CanUserMerge(user.ID),
 	}
+
+	if user != nil {
+		branch.UserCanPush = bp.CanUserPush(user.ID)
+		branch.UserCanMerge = bp.IsUserMergeWhitelisted(user.ID)
+	}
+	return branch, nil
 }
 
 // ToTag convert a git.Tag to an api.Tag
diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go
index afc4763b5..55dac7d21 100644
--- a/routers/api/v1/repo/branch.go
+++ b/routers/api/v1/repo/branch.go
@@ -70,7 +70,13 @@ func GetBranch(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusOK, convert.ToBranch(ctx.Repo.Repository, branch, c, branchProtection, ctx.User))
+	br, err := convert.ToBranch(ctx.Repo.Repository, branch, c, branchProtection, ctx.User)
+	if err != nil {
+		ctx.Error(http.StatusInternalServerError, "convert.ToBranch", err)
+		return
+	}
+
+	ctx.JSON(http.StatusOK, br)
 }
 
 // ListBranches list all the branches of a repository
@@ -113,7 +119,14 @@ func ListBranches(ctx *context.APIContext) {
 			ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err)
 			return
 		}
-		apiBranches[i] = convert.ToBranch(ctx.Repo.Repository, branches[i], c, branchProtection, ctx.User)
+
+		br, err := convert.ToBranch(ctx.Repo.Repository, branches[i], c, branchProtection, ctx.User)
+		if err != nil {
+			ctx.Error(http.StatusInternalServerError, "convert.ToBranch", err)
+			return
+		}
+
+		apiBranches[i] = br
 	}
 
 	ctx.JSON(http.StatusOK, &apiBranches)