From 9350ba7947d8caa6e7338d7c9e54df2f3aef2146 Mon Sep 17 00:00:00 2001 From: Chri-s Date: Sun, 25 Mar 2018 12:01:32 +0200 Subject: [PATCH] Add protected branch whitelists for merging (#3689) * Add database migrations for merge whitelist * Add merge whitelist settings for protected branches * Add checks for merge whitelists --- models/branches.go | 159 ++++++++++++++---- models/migrations/migrations.go | 2 + models/migrations/v59.go | 24 +++ models/pull.go | 2 +- modules/auth/repo_form.go | 11 +- options/locale/locale_en-US.ini | 4 + routers/repo/setting_protected_branch.go | 7 +- templates/repo/settings/protected_branch.tmpl | 43 +++++ 8 files changed, 210 insertions(+), 42 deletions(-) create mode 100644 models/migrations/v59.go diff --git a/models/branches.go b/models/branches.go index 0a3d19858..faa3ba6af 100644 --- a/models/branches.go +++ b/models/branches.go @@ -23,15 +23,18 @@ const ( // ProtectedBranch struct type ProtectedBranch struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"UNIQUE(s)"` - BranchName string `xorm:"UNIQUE(s)"` - CanPush bool `xorm:"NOT NULL DEFAULT false"` - EnableWhitelist bool - WhitelistUserIDs []int64 `xorm:"JSON TEXT"` - WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` - CreatedUnix util.TimeStamp `xorm:"created"` - UpdatedUnix util.TimeStamp `xorm:"updated"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + BranchName string `xorm:"UNIQUE(s)"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` + EnableWhitelist bool + WhitelistUserIDs []int64 `xorm:"JSON TEXT"` + WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + CreatedUnix util.TimeStamp `xorm:"created"` + UpdatedUnix util.TimeStamp `xorm:"updated"` } // IsProtected returns if the branch is protected @@ -61,6 +64,28 @@ func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool { return in } +// CanUserMerge returns if some user could merge a pull request to this protected branch +func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool { + if !protectBranch.EnableMergeWhitelist { + return true + } + + if base.Int64sContains(protectBranch.MergeWhitelistUserIDs, userID) { + return true + } + + if len(protectBranch.WhitelistTeamIDs) == 0 { + return false + } + + in, err := IsUserInTeams(userID, protectBranch.MergeWhitelistTeamIDs) + if err != nil { + log.Error(1, "IsUserInTeams:", err) + return false + } + return in +} + // GetProtectedBranchByRepoID getting protected branch by repo ID func GetProtectedBranchByRepoID(RepoID int64) ([]*ProtectedBranch, error) { protectedBranches := make([]*ProtectedBranch, 0) @@ -97,40 +122,35 @@ func GetProtectedBranchByID(id int64) (*ProtectedBranch, error) { // 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) { +func UpdateProtectBranch(repo *Repository, protectBranch *ProtectedBranch, whitelistUserIDs, whitelistTeamIDs, mergeWhitelistUserIDs, mergeWhitelistTeamIDs []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) - } + whitelist, err := updateUserWhitelist(repo, protectBranch.WhitelistUserIDs, whitelistUserIDs) + if err != nil { + return err } + protectBranch.WhitelistUserIDs = whitelist - // 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) - } - } + whitelist, err = updateUserWhitelist(repo, protectBranch.MergeWhitelistUserIDs, mergeWhitelistUserIDs) + if err != nil { + return err } + protectBranch.MergeWhitelistUserIDs = whitelist + + // if the repo is in an organization + whitelist, err = updateTeamWhitelist(repo, protectBranch.WhitelistTeamIDs, whitelistTeamIDs) + if err != nil { + return err + } + protectBranch.WhitelistTeamIDs = whitelist + + whitelist, err = updateTeamWhitelist(repo, protectBranch.MergeWhitelistTeamIDs, mergeWhitelistTeamIDs) + if err != nil { + return err + } + protectBranch.MergeWhitelistTeamIDs = whitelist // Make sure protectBranch.ID is not 0 for whitelists if protectBranch.ID == 0 { @@ -174,6 +194,73 @@ func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool, return false, nil } +// IsProtectedBranchForMerging checks if branch is protected for merging +func (repo *Repository) IsProtectedBranchForMerging(branchName string, doer *User) (bool, error) { + if doer == nil { + return true, nil + } + + protectedBranch := &ProtectedBranch{ + RepoID: repo.ID, + BranchName: branchName, + } + + has, err := x.Get(protectedBranch) + if err != nil { + return true, err + } else if has { + return !protectedBranch.CanUserMerge(doer.ID), nil + } + + return false, nil +} + +// updateUserWhitelist checks whether the user whitelist changed and returns a whitelist with +// the users from newWhitelist which have write access to the repo. +func updateUserWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { + hasUsersChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist) + if !hasUsersChanged { + return currentWhitelist, nil + } + + whitelist = make([]int64, 0, len(newWhitelist)) + for _, userID := range newWhitelist { + has, err := hasAccess(x, userID, repo, AccessModeWrite) + if err != nil { + return nil, fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) + } else if !has { + continue // Drop invalid user ID + } + + whitelist = append(whitelist, userID) + } + + return +} + +// updateTeamWhitelist checks whether the team whitelist changed and returns a whitelist with +// the teams from newWhitelist which have write access to the repo. +func updateTeamWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { + hasTeamsChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist) + if !hasTeamsChanged { + return currentWhitelist, nil + } + + teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeWrite) + if err != nil { + return nil, fmt.Errorf("GetTeamsWithAccessToRepo [org_id: %d, repo_id: %d]: %v", repo.OwnerID, repo.ID, err) + } + + whitelist = make([]int64, 0, len(teams)) + for i := range teams { + if teams[i].HasWriteAccess() && com.IsSliceContainsInt64(newWhitelist, teams[i].ID) { + whitelist = append(whitelist, teams[i].ID) + } + } + + return +} + // DeleteProtectedBranch removes ProtectedBranch relation between the user and repository. func (repo *Repository) DeleteProtectedBranch(id int64) (err error) { protectedBranch := &ProtectedBranch{ diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index cc5d0bae8..6c3404bcd 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -170,6 +170,8 @@ var migrations = []Migration{ NewMigration("add closed_unix column for issues", addIssueClosedTime), // v58 -> v59 NewMigration("add label descriptions", addLabelsDescriptions), + // v59 -> v60 + NewMigration("add merge whitelist for protected branches", addProtectedBranchMergeWhitelist), } // Migrate database to current version diff --git a/models/migrations/v59.go b/models/migrations/v59.go new file mode 100644 index 000000000..0a05495e7 --- /dev/null +++ b/models/migrations/v59.go @@ -0,0 +1,24 @@ +// Copyright 2018 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 ( + "fmt" + + "github.com/go-xorm/xorm" +) + +func addProtectedBranchMergeWhitelist(x *xorm.Engine) error { + type ProtectedBranch struct { + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` + } + + if err := x.Sync2(new(ProtectedBranch)); err != nil { + return fmt.Errorf("Sync2: %v", err) + } + return nil +} diff --git a/models/pull.go b/models/pull.go index 137900bee..6862d11a1 100644 --- a/models/pull.go +++ b/models/pull.go @@ -286,7 +286,7 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) { } } - if protected, err := pr.BaseRepo.IsProtectedBranch(pr.BaseBranch, doer); err != nil { + if protected, err := pr.BaseRepo.IsProtectedBranchForMerging(pr.BaseBranch, doer); err != nil { return fmt.Errorf("IsProtectedBranch: %v", err) } else if protected { return ErrNotAllowedToMerge{ diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index abe25866b..565428c35 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -129,10 +129,13 @@ 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 + Protected bool + EnableWhitelist bool + WhitelistUsers string + WhitelistTeams string + EnableMergeWhitelist bool + MergeWhitelistUsers string + MergeWhitelistTeams string } // Validate validates the fields diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 40cb20942..0bc8309ff 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1028,6 +1028,10 @@ settings.protect_whitelist_users = Users who can push to this branch settings.protect_whitelist_search_users = Search users settings.protect_whitelist_teams = Teams whose members can push to this branch. settings.protect_whitelist_search_teams = Search teams +settings.protect_merge_whitelist_committers = Restrict who can merge pull requests to this branch +settings.protect_merge_whitelist_committers_desc = Add users or teams to this branch's merge whitelist. Only whitelisted users can merge pull requests to this branch. If not checked, anyone with write permissions can merge pull requests to this branch. +settings.protect_merge_whitelist_users = Users who can merge pull requests to this branch +settings.protect_merge_whitelist_teams = Teams whose members can merge pull requests to this branch. settings.add_protected_branch=Enable protection settings.delete_protected_branch=Disable protection settings.update_protect_branch_success = Branch %s protect options changed successfully. diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index ac1017b91..db12f0afc 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -123,6 +123,7 @@ func SettingsProtectedBranch(c *context.Context) { } c.Data["Users"] = users c.Data["whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.WhitelistUserIDs), ",") + c.Data["merge_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.MergeWhitelistUserIDs), ",") if c.Repo.Owner.IsOrganization() { teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeWrite) @@ -132,6 +133,7 @@ func SettingsProtectedBranch(c *context.Context) { } c.Data["Teams"] = teams c.Data["whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.WhitelistTeamIDs), ",") + c.Data["merge_whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.MergeWhitelistTeamIDs), ",") } c.Data["Branch"] = protectBranch @@ -166,7 +168,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) protectBranch.EnableWhitelist = f.EnableWhitelist whitelistUsers, _ := base.StringsToInt64s(strings.Split(f.WhitelistUsers, ",")) whitelistTeams, _ := base.StringsToInt64s(strings.Split(f.WhitelistTeams, ",")) - err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams) + protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist + mergeWhitelistUsers, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ",")) + mergeWhitelistTeams, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) + err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams) if err != nil { ctx.ServerError("UpdateProtectBranch", err) return diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index a3a153eb3..6c892b662 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -60,6 +60,49 @@ {{end}} + +
+
+ + +

{{.i18n.Tr "repo.settings.protect_merge_whitelist_committers_desc"}}

+
+
+
+
+ +
+ +
{{.i18n.Tr "repo.settings.protect_whitelist_search_users"}}
+
+ {{range .Users}} +
+ + {{.Name}} +
+ {{end}} +
+
+
+ {{if .Owner.IsOrganization}} +
+
+ +
+ +
{{.i18n.Tr "repo.settings.protect_whitelist_search_teams"}}
+
+ {{range .Teams}} +
+ + {{.Name}} +
+ {{end}} +
+
+
+ {{end}} +