improve protected branch to add whitelist support (go-gitea#2451)

* improve protected branch to add whitelist support

* fix lint

* fix style check

* fix tests

* fix description on UI and import

* fix test

* bug fixed

* fix tests and languages

* move isSliceInt64Eq to util pkg; improve function names & typo

cmd/hook.go

@@ -84,9 +84,10 @@ func runHookPreReceive(c *cli.Context) error {
 	// the environment setted on serv command
 	repoID, _ := strconv.ParseInt(os.Getenv(models.ProtectedBranchRepoID), 10, 64)
 	isWiki := (os.Getenv(models.EnvRepoIsWiki) == "true")
-	//username := os.Getenv(models.EnvRepoUsername)
-	//reponame := os.Getenv(models.EnvRepoName)
-	//repoPath := models.RepoPath(username, reponame)
+	username := os.Getenv(models.EnvRepoUsername)
+	reponame := os.Getenv(models.EnvRepoName)
+	userIDStr := os.Getenv(models.EnvPusherID)
+	repoPath := models.RepoPath(username, reponame)
 
 	buf := bytes.NewBuffer(nil)
 	scanner := bufio.NewScanner(os.Stdin)
@@ -104,36 +105,37 @@ func runHookPreReceive(c *cli.Context) error {
 			continue
 		}
 
-		//oldCommitID := string(fields[0])
+		oldCommitID := string(fields[0])
 		newCommitID := string(fields[1])
 		refFullName := string(fields[2])
 
-		// FIXME: when we add feature to protected branch to deny force push, then uncomment below
-		/*var isForce bool
-		// detect force push
-		if git.EmptySHA != oldCommitID {
-			output, err := git.NewCommand("rev-list", oldCommitID, "^"+newCommitID).RunInDir(repoPath)
-			if err != nil {
-				fail("Internal error", "Fail to detect force push: %v", err)
-			} else if len(output) > 0 {
-				isForce = true
-			}
-		}*/
-
 		branchName := strings.TrimPrefix(refFullName, git.BranchPrefix)
 		protectBranch, err := private.GetProtectedBranchBy(repoID, branchName)
 		if err != nil {
 			log.GitLogger.Fatal(2, "retrieve protected branches information failed")
 		}
 
-		if protectBranch != nil {
-			if !protectBranch.CanPush {
-				// check and deletion
-				if newCommitID == git.EmptySHA {
-					fail(fmt.Sprintf("branch %s is protected from deletion", branchName), "")
-				} else {
+		if protectBranch != nil && protectBranch.IsProtected() {
+			// detect force push
+			if git.EmptySHA != oldCommitID {
+				output, err := git.NewCommand("rev-list", oldCommitID, "^"+newCommitID).RunInDir(repoPath)
+				if err != nil {
+					fail("Internal error", "Fail to detect force push: %v", err)
+				} else if len(output) > 0 {
+					fail(fmt.Sprintf("branch %s is protected from force push", branchName), "")
+				}
+			}
+
+			// check and deletion
+			if newCommitID == git.EmptySHA {
+				fail(fmt.Sprintf("branch %s is protected from deletion", branchName), "")
+			} else {
+				userID, _ := strconv.ParseInt(userIDStr, 10, 64)
+				canPush, err := private.CanUserPush(protectBranch.ID, userID)
+				if err != nil {
+					fail("Internal error", "Fail to detect user can push: %v", err)
+				} else if !canPush {
 					fail(fmt.Sprintf("protected branch %s can not be pushed to", branchName), "")
-					//fail(fmt.Sprintf("branch %s is protected from force push", branchName), "")
 				}
 			}
 		}

integrations/editor_test.go

@@ -43,16 +43,15 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 
 	csrf := GetCSRF(t, session, "/user2/repo1/settings/branches")
 	// Change master branch to protected
-	req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches?action=protected_branch", map[string]string{
-		"_csrf":      csrf,
-		"branchName": "master",
-		"canPush":    "true",
+	req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/master", map[string]string{
+		"_csrf":     csrf,
+		"protected": "on",
 	})
-	resp := session.MakeRequest(t, req, http.StatusOK)
+	resp := session.MakeRequest(t, req, http.StatusFound)
 	// Check if master branch has been locked successfully
 	flashCookie := session.GetCookie("macaron_flash")
 	assert.NotNil(t, flashCookie)
-	assert.EqualValues(t, flashCookie.Value, "success%3Dmaster%2BLocked%2Bsuccessfully")
+	assert.EqualValues(t, "success%3DBranch%2Bmaster%2Bprotect%2Boptions%2Bchanged%2Bsuccessfully.", flashCookie.Value)
 
 	// Request editor page
 	req = NewRequest(t, "GET", "/user2/repo1/_new/master/")
@@ -74,6 +73,20 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	// Check body for error message
 	assert.Contains(t, string(resp.Body), "Can not commit to protected branch 'master'.")
+
+	// remove the protected branch
+	csrf = GetCSRF(t, session, "/user2/repo1/settings/branches")
+	// Change master branch to protected
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/master", map[string]string{
+		"_csrf":     csrf,
+		"protected": "off",
+	})
+	resp = session.MakeRequest(t, req, http.StatusFound)
+	// Check if master branch has been locked successfully
+	flashCookie = session.GetCookie("macaron_flash")
+	assert.NotNil(t, flashCookie)
+	assert.EqualValues(t, "success%3DBranch%2Bmaster%2Bprotect%2Boptions%2Bremoved%2Bsuccessfully", flashCookie.Value)
+
 }
 
 func testEditFile(t *testing.T, session *TestSession, user, repo, branch, filePath string) *TestResponse {

integrations/integration_test.go

@@ -269,7 +269,7 @@ func MakeRequest(t testing.TB, req *http.Request, expectedStatus int) *TestRespo
 	mac.ServeHTTP(respWriter, req)
 	if expectedStatus != NoExpectedStatus {
 		assert.EqualValues(t, expectedStatus, respWriter.HeaderCode,
-			"Request URL: %s", req.URL.String())
+			"Request URL: %s %s", req.URL.String(), buffer.String())
 	}
 	return &TestResponse{
 		HeaderCode: respWriter.HeaderCode,

integrations/internal_test.go

@@ -31,7 +31,7 @@ func assertProtectedBranch(t *testing.T, repoID int64, branchName string, isErr,
 		var branch models.ProtectedBranch
 		t.Log(string(resp.Body))
 		assert.NoError(t, json.Unmarshal(resp.Body, &branch))
-		assert.Equal(t, canPush, branch.CanPush)
+		assert.Equal(t, canPush, !branch.IsProtected())
 	}
 }
 

models/branches.go

@@ -8,6 +8,12 @@ import (
 	"fmt"
 	"strings"
 	"time"
+
+	"code.gitea.io/gitea/modules/base"
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/util"
+
+	"github.com/Unknwon/com"
 )
 
 const (
@@ -17,14 +23,43 @@ const (
 
 // ProtectedBranch struct
 type ProtectedBranch struct {
-	ID          int64  `xorm:"pk autoincr"`
-	RepoID      int64  `xorm:"UNIQUE(s)"`
-	BranchName  string `xorm:"UNIQUE(s)"`
-	CanPush     bool
-	Created     time.Time `xorm:"-"`
-	CreatedUnix int64     `xorm:"created"`
-	Updated     time.Time `xorm:"-"`
-	UpdatedUnix int64     `xorm:"updated"`
+	ID               int64  `xorm:"pk autoincr"`
+	RepoID           int64  `xorm:"UNIQUE(s)"`
+	BranchName       string `xorm:"UNIQUE(s)"`
+	EnableWhitelist  bool
+	WhitelistUserIDs []int64   `xorm:"JSON TEXT"`
+	WhitelistTeamIDs []int64   `xorm:"JSON TEXT"`
+	Created          time.Time `xorm:"-"`
+	CreatedUnix      int64     `xorm:"created"`
+	Updated          time.Time `xorm:"-"`
+	UpdatedUnix      int64     `xorm:"updated"`
+}
+
+// IsProtected returns if the branch is protected
+func (protectBranch *ProtectedBranch) IsProtected() bool {
+	return protectBranch.ID > 0
+}
+
+// CanUserPush returns if some user could push to this protected branch
+func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool {
+	if !protectBranch.EnableWhitelist {
+		return false
+	}
+
+	if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) {
+		return true
+	}
+
+	if len(protectBranch.WhitelistTeamIDs) == 0 {
+		return false
+	}
+
+	in, err := IsUserInTeams(userID, protectBranch.WhitelistTeamIDs)
+	if err != nil {
+		log.Error(1, "IsUserInTeams:", err)
+		return false
+	}
+	return in
 }
 
 // GetProtectedBranchByRepoID getting protected branch by repo ID
@@ -46,14 +81,81 @@ func GetProtectedBranchBy(repoID int64, BranchName string) (*ProtectedBranch, er
 	return rel, nil
 }
 
+// GetProtectedBranchByID getting protected branch by ID
+func GetProtectedBranchByID(id int64) (*ProtectedBranch, error) {
+	rel := &ProtectedBranch{ID: id}
+	has, err := x.Get(rel)
+	if err != nil {
+		return nil, err
+	}
+	if !has {
+		return nil, nil
+	}
+	return rel, nil
+}
+
+// UpdateProtectBranch saves branch protection options of repository.
+// 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) {
+	if err = repo.GetOwner(); err != nil {
+		return fmt.Errorf("GetOwner: %v", err)
+	}
+
+	hasUsersChanged := !util.IsSliceInt64Eq(protectBranch.WhitelistUserIDs, whitelistUserIDs)
+	if hasUsersChanged {
+		protectBranch.WhitelistUserIDs = make([]int64, 0, len(whitelistUserIDs))
+		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)
+		}
+	}
+
+	// if the repo is in an orgniziation
+	hasTeamsChanged := !util.IsSliceInt64Eq(protectBranch.WhitelistTeamIDs, whitelistTeamIDs)
+	if hasTeamsChanged {
+		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)
+			}
+		}
+	}
+
+	// Make sure protectBranch.ID is not 0 for whitelists
+	if protectBranch.ID == 0 {
+		if _, err = x.Insert(protectBranch); err != nil {
+			return fmt.Errorf("Insert: %v", err)
+		}
+		return nil
+	}
+
+	if _, err = x.Id(protectBranch.ID).AllCols().Update(protectBranch); err != nil {
+		return fmt.Errorf("Update: %v", err)
+	}
+
+	return nil
+}
+
 // GetProtectedBranches get all protected branches
 func (repo *Repository) GetProtectedBranches() ([]*ProtectedBranch, error) {
 	protectedBranches := make([]*ProtectedBranch, 0)
 	return protectedBranches, x.Find(&protectedBranches, &ProtectedBranch{RepoID: repo.ID})
 }
 
 // IsProtectedBranch checks if branch is protected
-func (repo *Repository) IsProtectedBranch(branchName string) (bool, error) {
+func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool, error) {
 	protectedBranch := &ProtectedBranch{
 		RepoID:     repo.ID,
 		BranchName: branchName,
@@ -63,70 +165,12 @@ func (repo *Repository) IsProtectedBranch(branchName string) (bool, error) {
 	if err != nil {
 		return true, err
 	} else if has {
-		return true, nil
+		return !protectedBranch.CanUserPush(doer.ID), nil
 	}
 
 	return false, nil
 }
 
-// AddProtectedBranch add protection to branch
-func (repo *Repository) AddProtectedBranch(branchName string, canPush bool) error {
-	protectedBranch := &ProtectedBranch{
-		RepoID:     repo.ID,
-		BranchName: branchName,
-	}
-
-	has, err := x.Get(protectedBranch)
-	if err != nil {
-		return err
-	} else if has {
-		return nil
-	}
-
-	sess := x.NewSession()
-	defer sess.Close()
-	if err = sess.Begin(); err != nil {
-		return err
-	}
-	protectedBranch.CanPush = canPush
-	if _, err = sess.InsertOne(protectedBranch); err != nil {
-		return err
-	}
-
-	return sess.Commit()
-}
-
-// ChangeProtectedBranch access mode sets new access mode for the ProtectedBranch.
-func (repo *Repository) ChangeProtectedBranch(id int64, canPush bool) error {
-	ProtectedBranch := &ProtectedBranch{
-		RepoID: repo.ID,
-		ID:     id,
-	}
-	has, err := x.Get(ProtectedBranch)
-	if err != nil {
-		return fmt.Errorf("get ProtectedBranch: %v", err)
-	} else if !has {
-		return nil
-	}
-
-	if ProtectedBranch.CanPush == canPush {
-		return nil
-	}
-	ProtectedBranch.CanPush = canPush
-
-	sess := x.NewSession()
-	defer sess.Close()
-	if err = sess.Begin(); err != nil {
-		return err
-	}
-
-	if _, err = sess.Id(ProtectedBranch.ID).AllCols().Update(ProtectedBranch); err != nil {
-		return fmt.Errorf("update ProtectedBranch: %v", err)
-	}
-
-	return sess.Commit()
-}
-
 // DeleteProtectedBranch removes ProtectedBranch relation between the user and repository.
 func (repo *Repository) DeleteProtectedBranch(id int64) (err error) {
 	protectedBranch := &ProtectedBranch{
@@ -148,15 +192,3 @@ func (repo *Repository) DeleteProtectedBranch(id int64) (err error) {
 
 	return sess.Commit()
 }
-
-// newProtectedBranch insert one queue
-func newProtectedBranch(protectedBranch *ProtectedBranch) error {
-	_, err := x.InsertOne(protectedBranch)
-	return err
-}
-
-// UpdateProtectedBranch update queue
-func UpdateProtectedBranch(protectedBranch *ProtectedBranch) error {
-	_, err := x.Update(protectedBranch)
-	return err
-}

models/migrations/migrations.go

@@ -128,6 +128,8 @@ var migrations = []Migration{
 	NewMigration("remove commits and settings unit types", removeCommitsUnitType),
 	// v39 -> v40
 	NewMigration("adds time tracking and stopwatches", addTimetracking),
+	// v40 -> v41
+	NewMigration("migrate protected branch struct", migrateProtectedBranchStruct),
 }
 
 // Migrate database to current version