From 9f9f4882f92fb8f015663dca5726937fa7830b70 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 17 Aug 2022 18:10:58 +0800 Subject: [PATCH 01/34] Make database supports glob branch name protected branch --- models/git/branches.go | 423 +-------------------------- models/git/protected_branch.go | 436 ++++++++++++++++++++++++++++ models/git/protected_branch_list.go | 38 +++ models/issues/pull.go | 33 +-- models/issues/review.go | 17 +- modules/context/repo.go | 5 +- routers/api/v1/repo/branch.go | 16 +- routers/private/hook_pre_receive.go | 2 +- routers/web/repo/issue.go | 21 +- routers/web/repo/pull.go | 20 +- services/asymkey/sign.go | 2 +- services/pull/check.go | 6 +- services/pull/commit_status.go | 12 +- services/pull/merge.go | 19 +- services/pull/patch.go | 13 +- services/pull/update.go | 7 +- services/repository/files/patch.go | 2 +- services/repository/files/update.go | 2 +- 18 files changed, 573 insertions(+), 501 deletions(-) create mode 100644 models/git/protected_branch.go create mode 100644 models/git/protected_branch_list.go diff --git a/models/git/branches.go b/models/git/branches.go index 7f05a566769b..8556bb4a7e6d 100644 --- a/models/git/branches.go +++ b/models/git/branches.go @@ -7,433 +7,15 @@ package git import ( "context" "fmt" - "strings" "time" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" - "code.gitea.io/gitea/modules/util" - - "github.com/gobwas/glob" ) -// 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"` - EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` - WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` - MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` - 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"` - BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` - BlockOnOfficialReviewRequests bool `xorm:"NOT NULL DEFAULT false"` - BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"` - DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` - RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` - ProtectedFilePatterns string `xorm:"TEXT"` - UnprotectedFilePatterns string `xorm:"TEXT"` - - CreatedUnix timeutil.TimeStamp `xorm:"created"` - UpdatedUnix timeutil.TimeStamp `xorm:"updated"` -} - -func init() { - db.RegisterModel(new(ProtectedBranch)) - db.RegisterModel(new(DeletedBranch)) - db.RegisterModel(new(RenamedBranch)) -} - -// 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.CanPush { - return false - } - - if !protectBranch.EnableWhitelist { - if user, err := user_model.GetUserByID(userID); err != nil { - log.Error("GetUserByID: %v", err) - return false - } else if repo, err := repo_model.GetRepositoryByID(protectBranch.RepoID); err != nil { - log.Error("repo_model.GetRepositoryByID: %v", err) - return false - } else if writeAccess, err := access_model.HasAccessUnit(db.DefaultContext, user, repo, unit.TypeCode, perm.AccessModeWrite); err != nil { - log.Error("HasAccessUnit: %v", err) - return false - } else { - return writeAccess - } - } - - if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) { - return true - } - - if len(protectBranch.WhitelistTeamIDs) == 0 { - return false - } - - in, err := organization.IsUserInTeams(db.DefaultContext, userID, protectBranch.WhitelistTeamIDs) - if err != nil { - log.Error("IsUserInTeams: %v", err) - return false - } - return in -} - -// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch -func IsUserMergeWhitelisted(ctx context.Context, protectBranch *ProtectedBranch, userID int64, permissionInRepo access_model.Permission) bool { - if !protectBranch.EnableMergeWhitelist { - // Then we need to fall back on whether the user has write permission - return permissionInRepo.CanWrite(unit.TypeCode) - } - - if base.Int64sContains(protectBranch.MergeWhitelistUserIDs, userID) { - return true - } - - if len(protectBranch.MergeWhitelistTeamIDs) == 0 { - return false - } - - in, err := organization.IsUserInTeams(ctx, 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 IsUserOfficialReviewer(protectBranch *ProtectedBranch, user *user_model.User) (bool, error) { - return IsUserOfficialReviewerCtx(db.DefaultContext, protectBranch, user) -} - -// IsUserOfficialReviewerCtx check if user is official reviewer for the branch (counts towards required approvals) -func IsUserOfficialReviewerCtx(ctx context.Context, protectBranch *ProtectedBranch, user *user_model.User) (bool, error) { - repo, err := repo_model.GetRepositoryByIDCtx(ctx, protectBranch.RepoID) - if err != nil { - return false, err - } - - if !protectBranch.EnableApprovalsWhitelist { - // Anyone with write access is considered official reviewer - writeAccess, err := access_model.HasAccessUnit(ctx, user, repo, unit.TypeCode, perm.AccessModeWrite) - if err != nil { - return false, err - } - return writeAccess, nil - } - - if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, user.ID) { - return true, nil - } - - inTeam, err := organization.IsUserInTeams(ctx, user.ID, protectBranch.ApprovalsWhitelistTeamIDs) - if err != nil { - return false, err - } - - return inTeam, nil -} - -// GetProtectedFilePatterns parses a semicolon separated list of protected file patterns and returns a glob.Glob slice -func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob { - return getFilePatterns(protectBranch.ProtectedFilePatterns) -} - -// GetUnprotectedFilePatterns parses a semicolon separated list of unprotected file patterns and returns a glob.Glob slice -func (protectBranch *ProtectedBranch) GetUnprotectedFilePatterns() []glob.Glob { - return getFilePatterns(protectBranch.UnprotectedFilePatterns) -} - -func getFilePatterns(filePatterns string) []glob.Glob { - extarr := make([]glob.Glob, 0, 10) - for _, expr := range strings.Split(strings.ToLower(filePatterns), ";") { - expr = strings.TrimSpace(expr) - if expr != "" { - if g, err := glob.Compile(expr, '.', '/'); err != nil { - log.Info("Invalid glob expression '%s' (skipped): %v", expr, err) - } else { - extarr = append(extarr, g) - } - } - } - return extarr -} - -// MergeBlockedByProtectedFiles returns true if merge is blocked by protected files change -func (protectBranch *ProtectedBranch) MergeBlockedByProtectedFiles(changedProtectedFiles []string) bool { - glob := protectBranch.GetProtectedFilePatterns() - if len(glob) == 0 { - return false - } - - return len(changedProtectedFiles) > 0 -} - -// IsProtectedFile return if path is protected -func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path string) bool { - if len(patterns) == 0 { - patterns = protectBranch.GetProtectedFilePatterns() - if len(patterns) == 0 { - return false - } - } - - lpath := strings.ToLower(strings.TrimSpace(path)) - - r := false - for _, pat := range patterns { - if pat.Match(lpath) { - r = true - break - } - } - - return r -} - -// IsUnprotectedFile return if path is unprotected -func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, path string) bool { - if len(patterns) == 0 { - patterns = protectBranch.GetUnprotectedFilePatterns() - if len(patterns) == 0 { - return false - } - } - - lpath := strings.ToLower(strings.TrimSpace(path)) - - r := false - for _, pat := range patterns { - if pat.Match(lpath) { - r = true - break - } - } - - return r -} - -// GetProtectedBranchBy getting protected branch by ID/Name -func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { - rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName} - has, err := db.GetByBean(ctx, rel) - if err != nil { - return nil, err - } - if !has { - return nil, nil - } - return rel, nil -} - -// WhitelistOptions represent all sorts of whitelists used for protected branches -type WhitelistOptions struct { - UserIDs []int64 - TeamIDs []int64 - - MergeUserIDs []int64 - MergeTeamIDs []int64 - - ApprovalsUserIDs []int64 - ApprovalsTeamIDs []int64 -} - -// 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(ctx context.Context, repo *repo_model.Repository, protectBranch *ProtectedBranch, opts WhitelistOptions) (err error) { - if err = repo.GetOwner(ctx); err != nil { - return fmt.Errorf("GetOwner: %v", err) - } - - whitelist, err := updateUserWhitelist(ctx, repo, protectBranch.WhitelistUserIDs, opts.UserIDs) - if err != nil { - return err - } - protectBranch.WhitelistUserIDs = whitelist - - whitelist, err = updateUserWhitelist(ctx, repo, protectBranch.MergeWhitelistUserIDs, opts.MergeUserIDs) - if err != nil { - return err - } - protectBranch.MergeWhitelistUserIDs = whitelist - - whitelist, err = updateApprovalWhitelist(ctx, repo, protectBranch.ApprovalsWhitelistUserIDs, opts.ApprovalsUserIDs) - if err != nil { - return err - } - protectBranch.ApprovalsWhitelistUserIDs = whitelist - - // if the repo is in an organization - whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.WhitelistTeamIDs, opts.TeamIDs) - if err != nil { - return err - } - protectBranch.WhitelistTeamIDs = whitelist - - whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.MergeWhitelistTeamIDs, opts.MergeTeamIDs) - if err != nil { - return err - } - protectBranch.MergeWhitelistTeamIDs = whitelist - - whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.ApprovalsWhitelistTeamIDs, opts.ApprovalsTeamIDs) - if err != nil { - return err - } - protectBranch.ApprovalsWhitelistTeamIDs = whitelist - - // Make sure protectBranch.ID is not 0 for whitelists - if protectBranch.ID == 0 { - if _, err = db.GetEngine(ctx).Insert(protectBranch); err != nil { - return fmt.Errorf("Insert: %v", err) - } - return nil - } - - if _, err = db.GetEngine(ctx).ID(protectBranch.ID).AllCols().Update(protectBranch); err != nil { - return fmt.Errorf("Update: %v", err) - } - - return nil -} - -// GetProtectedBranches get all protected branches -func GetProtectedBranches(repoID int64) ([]*ProtectedBranch, error) { - protectedBranches := make([]*ProtectedBranch, 0) - return protectedBranches, db.GetEngine(db.DefaultContext).Find(&protectedBranches, &ProtectedBranch{RepoID: repoID}) -} - -// IsProtectedBranch checks if branch is protected -func IsProtectedBranch(repoID int64, branchName string) (bool, error) { - protectedBranch := &ProtectedBranch{ - RepoID: repoID, - BranchName: branchName, - } - - has, err := db.GetEngine(db.DefaultContext).Exist(protectedBranch) - if err != nil { - return true, err - } - return has, nil -} - -// updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with -// the users from newWhitelist which have explicit read or write access to the repo. -func updateApprovalWhitelist(ctx context.Context, repo *repo_model.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 { - if reader, err := access_model.IsRepoReader(ctx, repo, userID); err != nil { - return nil, err - } else if !reader { - continue - } - whitelist = append(whitelist, userID) - } - - return whitelist, err -} - -// 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(ctx context.Context, repo *repo_model.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 { - user, err := user_model.GetUserByIDCtx(ctx, userID) - if err != nil { - return nil, fmt.Errorf("GetUserByID [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) - } - perm, err := access_model.GetUserRepoPermission(ctx, repo, user) - if err != nil { - return nil, fmt.Errorf("GetUserRepoPermission [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) - } - - if !perm.CanWrite(unit.TypeCode) { - continue // Drop invalid user ID - } - - whitelist = append(whitelist, userID) - } - - return whitelist, err -} - -// 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(ctx context.Context, repo *repo_model.Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { - hasTeamsChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist) - if !hasTeamsChanged { - return currentWhitelist, nil - } - - teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) - 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 util.IsInt64InSlice(teams[i].ID, newWhitelist) { - whitelist = append(whitelist, teams[i].ID) - } - } - - return whitelist, err -} - -// DeleteProtectedBranch removes ProtectedBranch relation between the user and repository. -func DeleteProtectedBranch(repoID, id int64) (err error) { - protectedBranch := &ProtectedBranch{ - RepoID: repoID, - ID: id, - } - - if affected, err := db.GetEngine(db.DefaultContext).Delete(protectedBranch); err != nil { - return err - } else if affected != 1 { - return fmt.Errorf("delete protected branch ID(%v) failed", id) - } - - return nil -} - // DeletedBranch struct type DeletedBranch struct { ID int64 `xorm:"pk autoincr"` @@ -445,6 +27,11 @@ type DeletedBranch struct { DeletedUnix timeutil.TimeStamp `xorm:"INDEX created"` } +func init() { + db.RegisterModel(new(DeletedBranch)) + db.RegisterModel(new(RenamedBranch)) +} + // AddDeletedBranch adds a deleted branch to the database func AddDeletedBranch(repoID int64, branchName, commit string, deletedByID int64) error { deletedBranch := &DeletedBranch{ diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go new file mode 100644 index 000000000000..16108999aea1 --- /dev/null +++ b/models/git/protected_branch.go @@ -0,0 +1,436 @@ +// Copyright 2022 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 git + +import ( + "context" + "fmt" + "strings" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" + + "github.com/gobwas/glob" +) + +// 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"` + EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"` + WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"` + MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"` + 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"` + BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` + BlockOnOfficialReviewRequests bool `xorm:"NOT NULL DEFAULT false"` + BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"` + DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` + RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` + ProtectedFilePatterns string `xorm:"TEXT"` + UnprotectedFilePatterns string `xorm:"TEXT"` + + CreatedUnix timeutil.TimeStamp `xorm:"created"` + UpdatedUnix timeutil.TimeStamp `xorm:"updated"` +} + +func init() { + db.RegisterModel(new(ProtectedBranch)) +} + +// IsProtected returns if the branch is protected +func (protectBranch *ProtectedBranch) IsProtected() bool { + return protectBranch.ID > 0 +} + +// Match tests if branchName matches the rule +func (protectBranch *ProtectedBranch) Match(branchName string) bool { + if strings.EqualFold(protectBranch.BranchName, branchName) { + return true + } + return glob.MustCompile(protectBranch.BranchName).Match(branchName) +} + +// CanUserPush returns if some user could push to this protected branch +func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool { + if !protectBranch.CanPush { + return false + } + + if !protectBranch.EnableWhitelist { + if user, err := user_model.GetUserByID(userID); err != nil { + log.Error("GetUserByID: %v", err) + return false + } else if repo, err := repo_model.GetRepositoryByID(protectBranch.RepoID); err != nil { + log.Error("repo_model.GetRepositoryByID: %v", err) + return false + } else if writeAccess, err := access_model.HasAccessUnit(db.DefaultContext, user, repo, unit.TypeCode, perm.AccessModeWrite); err != nil { + log.Error("HasAccessUnit: %v", err) + return false + } else { + return writeAccess + } + } + + if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) { + return true + } + + if len(protectBranch.WhitelistTeamIDs) == 0 { + return false + } + + in, err := organization.IsUserInTeams(db.DefaultContext, userID, protectBranch.WhitelistTeamIDs) + if err != nil { + log.Error("IsUserInTeams: %v", err) + return false + } + return in +} + +// IsUserMergeWhitelisted checks if some user is whitelisted to merge to this branch +func IsUserMergeWhitelisted(ctx context.Context, protectBranch *ProtectedBranch, userID int64, permissionInRepo access_model.Permission) bool { + if !protectBranch.EnableMergeWhitelist { + // Then we need to fall back on whether the user has write permission + return permissionInRepo.CanWrite(unit.TypeCode) + } + + if base.Int64sContains(protectBranch.MergeWhitelistUserIDs, userID) { + return true + } + + if len(protectBranch.MergeWhitelistTeamIDs) == 0 { + return false + } + + in, err := organization.IsUserInTeams(ctx, 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 IsUserOfficialReviewer(ctx context.Context, protectBranch *ProtectedBranch, user *user_model.User) (bool, error) { + repo, err := repo_model.GetRepositoryByIDCtx(ctx, protectBranch.RepoID) + if err != nil { + return false, err + } + + if !protectBranch.EnableApprovalsWhitelist { + // Anyone with write access is considered official reviewer + writeAccess, err := access_model.HasAccessUnit(ctx, user, repo, unit.TypeCode, perm.AccessModeWrite) + if err != nil { + return false, err + } + return writeAccess, nil + } + + if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, user.ID) { + return true, nil + } + + inTeam, err := organization.IsUserInTeams(ctx, user.ID, protectBranch.ApprovalsWhitelistTeamIDs) + if err != nil { + return false, err + } + + return inTeam, nil +} + +// GetProtectedFilePatterns parses a semicolon separated list of protected file patterns and returns a glob.Glob slice +func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob { + return getFilePatterns(protectBranch.ProtectedFilePatterns) +} + +// GetUnprotectedFilePatterns parses a semicolon separated list of unprotected file patterns and returns a glob.Glob slice +func (protectBranch *ProtectedBranch) GetUnprotectedFilePatterns() []glob.Glob { + return getFilePatterns(protectBranch.UnprotectedFilePatterns) +} + +func getFilePatterns(filePatterns string) []glob.Glob { + extarr := make([]glob.Glob, 0, 10) + for _, expr := range strings.Split(strings.ToLower(filePatterns), ";") { + expr = strings.TrimSpace(expr) + if expr != "" { + if g, err := glob.Compile(expr, '.', '/'); err != nil { + log.Info("Invalid glob expression '%s' (skipped): %v", expr, err) + } else { + extarr = append(extarr, g) + } + } + } + return extarr +} + +// MergeBlockedByProtectedFiles returns true if merge is blocked by protected files change +func (protectBranch *ProtectedBranch) MergeBlockedByProtectedFiles(changedProtectedFiles []string) bool { + glob := protectBranch.GetProtectedFilePatterns() + if len(glob) == 0 { + return false + } + + return len(changedProtectedFiles) > 0 +} + +// IsProtectedFile return if path is protected +func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path string) bool { + if len(patterns) == 0 { + patterns = protectBranch.GetProtectedFilePatterns() + if len(patterns) == 0 { + return false + } + } + + lpath := strings.ToLower(strings.TrimSpace(path)) + + r := false + for _, pat := range patterns { + if pat.Match(lpath) { + r = true + break + } + } + + return r +} + +// IsUnprotectedFile return if path is unprotected +func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, path string) bool { + if len(patterns) == 0 { + patterns = protectBranch.GetUnprotectedFilePatterns() + if len(patterns) == 0 { + return false + } + } + + lpath := strings.ToLower(strings.TrimSpace(path)) + + r := false + for _, pat := range patterns { + if pat.Match(lpath) { + r = true + break + } + } + + return r +} + +// GetProtectedBranchBy getting protected branch by ID/Name +func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { + rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName} + has, err := db.GetByBean(ctx, rel) + if err != nil { + return nil, err + } + if !has { + return nil, nil + } + return rel, nil +} + + +// WhitelistOptions represent all sorts of whitelists used for protected branches +type WhitelistOptions struct { + UserIDs []int64 + TeamIDs []int64 + + MergeUserIDs []int64 + MergeTeamIDs []int64 + + ApprovalsUserIDs []int64 + ApprovalsTeamIDs []int64 +} + +// 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(ctx context.Context, repo *repo_model.Repository, protectBranch *ProtectedBranch, opts WhitelistOptions) (err error) { + if err = repo.GetOwner(ctx); err != nil { + return fmt.Errorf("GetOwner: %v", err) + } + + whitelist, err := updateUserWhitelist(ctx, repo, protectBranch.WhitelistUserIDs, opts.UserIDs) + if err != nil { + return err + } + protectBranch.WhitelistUserIDs = whitelist + + whitelist, err = updateUserWhitelist(ctx, repo, protectBranch.MergeWhitelistUserIDs, opts.MergeUserIDs) + if err != nil { + return err + } + protectBranch.MergeWhitelistUserIDs = whitelist + + whitelist, err = updateApprovalWhitelist(ctx, repo, protectBranch.ApprovalsWhitelistUserIDs, opts.ApprovalsUserIDs) + if err != nil { + return err + } + protectBranch.ApprovalsWhitelistUserIDs = whitelist + + // if the repo is in an organization + whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.WhitelistTeamIDs, opts.TeamIDs) + if err != nil { + return err + } + protectBranch.WhitelistTeamIDs = whitelist + + whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.MergeWhitelistTeamIDs, opts.MergeTeamIDs) + if err != nil { + return err + } + protectBranch.MergeWhitelistTeamIDs = whitelist + + whitelist, err = updateTeamWhitelist(ctx, repo, protectBranch.ApprovalsWhitelistTeamIDs, opts.ApprovalsTeamIDs) + if err != nil { + return err + } + protectBranch.ApprovalsWhitelistTeamIDs = whitelist + + // Make sure protectBranch.ID is not 0 for whitelists + if protectBranch.ID == 0 { + if _, err = db.GetEngine(ctx).Insert(protectBranch); err != nil { + return fmt.Errorf("Insert: %v", err) + } + return nil + } + + if _, err = db.GetEngine(ctx).ID(protectBranch.ID).AllCols().Update(protectBranch); err != nil { + return fmt.Errorf("Update: %v", err) + } + + return nil +} + +// GetProtectedBranches get all protected branches +func GetProtectedBranches(repoID int64) ([]*ProtectedBranch, error) { + protectedBranches := make([]*ProtectedBranch, 0) + return protectedBranches, db.GetEngine(db.DefaultContext).Find(&protectedBranches, &ProtectedBranch{RepoID: repoID}) +} + +// IsProtectedBranch checks if branch is protected +func IsProtectedBranch(repoID int64, branchName string) (bool, error) { + protectedBranch := &ProtectedBranch{ + RepoID: repoID, + BranchName: branchName, + } + + has, err := db.GetEngine(db.DefaultContext).Exist(protectedBranch) + if err != nil { + return true, err + } + return has, nil +} + +// updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with +// the users from newWhitelist which have explicit read or write access to the repo. +func updateApprovalWhitelist(ctx context.Context, repo *repo_model.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 { + if reader, err := access_model.IsRepoReader(ctx, repo, userID); err != nil { + return nil, err + } else if !reader { + continue + } + whitelist = append(whitelist, userID) + } + + return whitelist, err +} + +// 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(ctx context.Context, repo *repo_model.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 { + user, err := user_model.GetUserByIDCtx(ctx, userID) + if err != nil { + return nil, fmt.Errorf("GetUserByID [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) + } + perm, err := access_model.GetUserRepoPermission(ctx, repo, user) + if err != nil { + return nil, fmt.Errorf("GetUserRepoPermission [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) + } + + if !perm.CanWrite(unit.TypeCode) { + continue // Drop invalid user ID + } + + whitelist = append(whitelist, userID) + } + + return whitelist, err +} + +// 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(ctx context.Context, repo *repo_model.Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { + hasTeamsChanged := !util.IsSliceInt64Eq(currentWhitelist, newWhitelist) + if !hasTeamsChanged { + return currentWhitelist, nil + } + + teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + 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 util.IsInt64InSlice(teams[i].ID, newWhitelist) { + whitelist = append(whitelist, teams[i].ID) + } + } + + return whitelist, err +} + +// DeleteProtectedBranch removes ProtectedBranch relation between the user and repository. +func DeleteProtectedBranch(repoID, id int64) (err error) { + protectedBranch := &ProtectedBranch{ + RepoID: repoID, + ID: id, + } + + if affected, err := db.GetEngine(db.DefaultContext).Delete(protectedBranch); err != nil { + return err + } else if affected != 1 { + return fmt.Errorf("delete protected branch ID(%v) failed", id) + } + + return nil +} diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go new file mode 100644 index 000000000000..fce049da0c79 --- /dev/null +++ b/models/git/protected_branch_list.go @@ -0,0 +1,38 @@ +// Copyright 2022 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 git + +import ( + "context" + + "code.gitea.io/gitea/models/db" +) + +type ProtectedBranchRules []*ProtectedBranch + +func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedBranch { + for _, rule := range rules { + if rule.Match(branchName) { + return rule + } + } + return nil +} + +// FindMatchedProtectedBranchRules load all repository's protected rules +func FindMatchedProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedBranchRules, error) { + var rules []*ProtectedBranch + err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Asc("created_unix").Find(&rules) + return rules, err +} + +// GetFirstMatchProtectedBranchRule returns the first matched rules +func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { + rules, err := FindMatchedProtectedBranchRules(ctx, repoID) + if err != nil { + return nil, err + } + return rules.GetFirstMatched(branchName), nil +} diff --git a/models/issues/pull.go b/models/issues/pull.go index f96b03445e91..fd86ceed68d1 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -150,16 +150,16 @@ type PullRequest struct { Issue *Issue `xorm:"-"` Index int64 - HeadRepoID int64 `xorm:"INDEX"` - HeadRepo *repo_model.Repository `xorm:"-"` - BaseRepoID int64 `xorm:"INDEX"` - BaseRepo *repo_model.Repository `xorm:"-"` - HeadBranch string - HeadCommitID string `xorm:"-"` - BaseBranch string - ProtectedBranch *git_model.ProtectedBranch `xorm:"-"` - MergeBase string `xorm:"VARCHAR(40)"` - AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` + HeadRepoID int64 `xorm:"INDEX"` + HeadRepo *repo_model.Repository `xorm:"-"` + BaseRepoID int64 `xorm:"INDEX"` + BaseRepo *repo_model.Repository `xorm:"-"` + HeadBranch string + HeadCommitID string `xorm:"-"` + BaseBranch string + // ProtectedBranch *git_model.ProtectedBranch `xorm:"-"` + MergeBase string `xorm:"VARCHAR(40)"` + AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` HasMerged bool `xorm:"INDEX"` MergedCommitID string `xorm:"VARCHAR(40)"` @@ -305,13 +305,8 @@ func (pr *PullRequest) LoadIssueCtx(ctx context.Context) (err error) { return err } -// LoadProtectedBranch loads the protected branch of the base branch -func (pr *PullRequest) LoadProtectedBranch() (err error) { - return pr.LoadProtectedBranchCtx(db.DefaultContext) -} - -// LoadProtectedBranchCtx loads the protected branch of the base branch -func (pr *PullRequest) LoadProtectedBranchCtx(ctx context.Context) (err error) { +// LoadProtectedBranchRules loads the protected branch of the base branch +/*func (pr *PullRequest) LoadProtectedBranchRules(ctx context.Context) (err error) { if pr.ProtectedBranch == nil { if pr.BaseRepo == nil { if pr.BaseRepoID == 0 { @@ -322,10 +317,10 @@ func (pr *PullRequest) LoadProtectedBranchCtx(ctx context.Context) (err error) { return } } - pr.ProtectedBranch, err = git_model.GetProtectedBranchBy(ctx, pr.BaseRepo.ID, pr.BaseBranch) + pr.ProtectedBranch, err = git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepo.ID, pr.BaseBranch) } return err -} +}*/ // ReviewCount represents a count of Reviews type ReviewCount struct { diff --git a/models/issues/review.go b/models/issues/review.go index 583590080177..7df926efe16a 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -264,15 +264,17 @@ func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewers ...*user_mo if err != nil { return false, err } - if err = pr.LoadProtectedBranchCtx(ctx); err != nil { + + rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return false, err } - if pr.ProtectedBranch == nil { + if rule == nil { return false, nil } for _, reviewer := range reviewers { - official, err := git_model.IsUserOfficialReviewerCtx(ctx, pr.ProtectedBranch, reviewer) + official, err := git_model.IsUserOfficialReviewer(ctx, rule, reviewer) if official || err != nil { return official, err } @@ -287,18 +289,19 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio if err != nil { return false, err } - if err = pr.LoadProtectedBranchCtx(ctx); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return false, err } - if pr.ProtectedBranch == nil { + if pb == nil { return false, nil } - if !pr.ProtectedBranch.EnableApprovalsWhitelist { + if !pb.EnableApprovalsWhitelist { return team.UnitAccessModeCtx(ctx, unit.TypeCode) >= perm.AccessModeWrite, nil } - return base.Int64sContains(pr.ProtectedBranch.ApprovalsWhitelistTeamIDs, team.ID), nil + return base.Int64sContains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil } // CreateReview creates a new review based on opts diff --git a/modules/context/repo.go b/modules/context/repo.go index ea4054206999..185e7c47a191 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -118,9 +118,10 @@ type CanCommitToBranchResults struct { } // CanCommitToBranch returns true if repository is editable and user has proper access level -// and branch is not protected for push +// +// and branch is not protected for push func (r *Repository) CanCommitToBranch(ctx context.Context, doer *user_model.User) (CanCommitToBranchResults, error) { - protectedBranch, err := git_model.GetProtectedBranchBy(ctx, r.Repository.ID, r.BranchName) + protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, r.Repository.ID, r.BranchName) if err != nil { return CanCommitToBranchResults{}, err } diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 84a172e92bf0..2923502c9531 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -71,7 +71,7 @@ func GetBranch(ctx *context.APIContext) { return } - branchProtection, err := git_model.GetProtectedBranchBy(ctx, ctx.Repo.Repository.ID, branchName) + branchProtection, err := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, branchName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err) return @@ -207,7 +207,7 @@ func CreateBranch(ctx *context.APIContext) { return } - branchProtection, err := git_model.GetProtectedBranchBy(ctx, ctx.Repo.Repository.ID, branch.Name) + branchProtection, err := git_model.GetFirstMatchProtectedBranchRule(ctx, ctx.Repo.Repository.ID, branch.Name) if err != nil { ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err) return @@ -260,6 +260,12 @@ func ListBranches(ctx *context.APIContext) { return } + rules, err := git_model.FindMatchedProtectedBranchRules(ctx, ctx.Repo.Repository.ID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindMatchedProtectedBranchRules", err) + return + } + apiBranches := make([]*api.Branch, 0, len(branches)) for i := range branches { c, err := branches[i].GetCommit() @@ -272,11 +278,7 @@ func ListBranches(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "GetCommit", err) return } - branchProtection, err := git_model.GetProtectedBranchBy(ctx, ctx.Repo.Repository.ID, branches[i].Name) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err) - return - } + branchProtection := rules.GetFirstMatched(branches[i].Name) apiBranch, err := convert.ToBranch(ctx.Repo.Repository, branches[i], c, branchProtection, ctx.Doer, ctx.Repo.IsAdmin()) if err != nil { ctx.Error(http.StatusInternalServerError, "convert.ToBranch", err) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 3e7d1fe9ef69..53c0a03fa1b8 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -157,7 +157,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN return } - protectBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, branchName) + protectBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, branchName) if err != nil { log.Error("Unable to get protected branch: %s in %-v Error: %v", branchName, repo, err) ctx.JSON(http.StatusInternalServerError, private.Response{ diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index ad25a94e13b1..baa4ad9f5a70 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1630,22 +1630,23 @@ func ViewIssue(ctx *context.Context) { } ctx.Data["DefaultSquashMergeMessage"] = defaultSquashMergeMessage - if err = pull.LoadProtectedBranch(); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) + if err != nil { ctx.ServerError("LoadProtectedBranch", err) return } ctx.Data["ShowMergeInstructions"] = true - if pull.ProtectedBranch != nil { + if pb != nil { var showMergeInstructions bool if ctx.Doer != nil { - showMergeInstructions = pull.ProtectedBranch.CanUserPush(ctx.Doer.ID) - } - ctx.Data["IsBlockedByApprovals"] = !issues_model.HasEnoughApprovals(ctx, pull.ProtectedBranch, pull) - ctx.Data["IsBlockedByRejection"] = issues_model.MergeBlockedByRejectedReview(ctx, pull.ProtectedBranch, pull) - ctx.Data["IsBlockedByOfficialReviewRequests"] = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pull.ProtectedBranch, pull) - ctx.Data["IsBlockedByOutdatedBranch"] = issues_model.MergeBlockedByOutdatedBranch(pull.ProtectedBranch, pull) - ctx.Data["GrantedApprovals"] = issues_model.GetGrantedApprovalsCount(ctx, pull.ProtectedBranch, pull) - ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits + showMergeInstructions = pb.CanUserPush(ctx.Doer.ID) + } + ctx.Data["IsBlockedByApprovals"] = !issues_model.HasEnoughApprovals(ctx, pb, pull) + ctx.Data["IsBlockedByRejection"] = issues_model.MergeBlockedByRejectedReview(ctx, pb, pull) + ctx.Data["IsBlockedByOfficialReviewRequests"] = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pull) + ctx.Data["IsBlockedByOutdatedBranch"] = issues_model.MergeBlockedByOutdatedBranch(pb, pull) + ctx.Data["GrantedApprovals"] = issues_model.GetGrantedApprovalsCount(ctx, pb, pull) + ctx.Data["RequireSigned"] = pb.RequireSignedCommits ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0 ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7c140a4e5991..6ac2e082a998 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -414,11 +414,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C setMergeTarget(ctx, pull) - if err := pull.LoadProtectedBranch(); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, pull.BaseBranch) + if err != nil { ctx.ServerError("LoadProtectedBranch", err) return nil } - ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck + ctx.Data["EnableStatusCheck"] = pb != nil && pb.EnableStatusCheck var baseGitRepo *git.Repository if pull.BaseRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { @@ -544,16 +545,16 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["LatestCommitStatus"] = git_model.CalcCommitStatus(commitStatuses) } - if pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck { + if pb != nil && pb.EnableStatusCheck { ctx.Data["is_context_required"] = func(context string) bool { - for _, c := range pull.ProtectedBranch.StatusCheckContexts { + for _, c := range pb.StatusCheckContexts { if c == context { return true } } return false } - ctx.Data["RequiredStatusCheckState"] = pull_service.MergeRequiredContextsCommitStatus(commitStatuses, pull.ProtectedBranch.StatusCheckContexts) + ctx.Data["RequiredStatusCheckState"] = pull_service.MergeRequiredContextsCommitStatus(commitStatuses, pb.StatusCheckContexts) } ctx.Data["HeadBranchMovedOn"] = headBranchSha != sha @@ -726,16 +727,17 @@ func ViewPullFiles(ctx *context.Context) { return } - if err = pull.LoadProtectedBranch(); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) + if err != nil { ctx.ServerError("LoadProtectedBranch", err) return } - if pull.ProtectedBranch != nil { - glob := pull.ProtectedBranch.GetProtectedFilePatterns() + if pb != nil { + glob := pb.GetProtectedFilePatterns() if len(glob) != 0 { for _, file := range diff.Files { - file.IsProtected = pull.ProtectedBranch.IsProtectedFile(glob, file.Name) + file.IsProtected = pb.IsProtectedFile(glob, file.Name) } } } diff --git a/services/asymkey/sign.go b/services/asymkey/sign.go index edfd0f6cadf2..30370d7e2d78 100644 --- a/services/asymkey/sign.go +++ b/services/asymkey/sign.go @@ -311,7 +311,7 @@ Loop: return false, "", nil, &ErrWontSign{twofa} } case approved: - protectedBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, pr.BaseBranch) + protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, pr.BaseBranch) if err != nil { return false, "", nil, err } diff --git a/services/pull/check.go b/services/pull/check.go index 288f4dc0b73b..1e0330c0c943 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -127,11 +128,12 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce // isSignedIfRequired check if merge will be signed if required func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User) (bool, error) { - if err := pr.LoadProtectedBranchCtx(ctx); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return false, err } - if pr.ProtectedBranch == nil || !pr.ProtectedBranch.RequireSignedCommits { + if pb == nil || !pb.RequireSignedCommits { return true, nil } diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 5d846129f6ce..6cd6c800a658 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -84,10 +84,11 @@ func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requ // IsPullCommitStatusPass returns if all required status checks PASS func IsPullCommitStatusPass(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { - if err := pr.LoadProtectedBranchCtx(ctx); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return false, errors.Wrap(err, "GetLatestCommitStatus") } - if pr.ProtectedBranch == nil || !pr.ProtectedBranch.EnableStatusCheck { + if pb == nil || !pb.EnableStatusCheck { return true, nil } @@ -138,12 +139,13 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR return "", errors.Wrap(err, "GetLatestCommitStatus") } - if err := pr.LoadProtectedBranchCtx(ctx); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return "", errors.Wrap(err, "LoadProtectedBranch") } var requiredContexts []string - if pr.ProtectedBranch != nil { - requiredContexts = pr.ProtectedBranch.StatusCheckContexts + if pb != nil { + requiredContexts = pb.StatusCheckContexts } return MergeRequiredContextsCommitStatus(commitStatuses, requiredContexts), nil diff --git a/services/pull/merge.go b/services/pull/merge.go index 4cd4e3bd7e05..1a9b05da2a60 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -744,12 +744,12 @@ func IsUserAllowedToMerge(ctx context.Context, pr *issues_model.PullRequest, p a return false, nil } - err := pr.LoadProtectedBranchCtx(ctx) + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) if err != nil { return false, err } - if (p.CanWrite(unit.TypeCode) && pr.ProtectedBranch == nil) || (pr.ProtectedBranch != nil && git_model.IsUserMergeWhitelisted(ctx, pr.ProtectedBranch, user.ID, p)) { + if (p.CanWrite(unit.TypeCode) && pb == nil) || (pb != nil && git_model.IsUserMergeWhitelisted(ctx, pb, user.ID, p)) { return true, nil } @@ -762,10 +762,11 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return fmt.Errorf("LoadBaseRepo: %v", err) } - if err = pr.LoadProtectedBranchCtx(ctx); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return fmt.Errorf("LoadProtectedBranch: %v", err) } - if pr.ProtectedBranch == nil { + if pb == nil { return nil } @@ -779,23 +780,23 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques } } - if !issues_model.HasEnoughApprovals(ctx, pr.ProtectedBranch, pr) { + if !issues_model.HasEnoughApprovals(ctx, pb, pr) { return models.ErrDisallowedToMerge{ Reason: "Does not have enough approvals", } } - if issues_model.MergeBlockedByRejectedReview(ctx, pr.ProtectedBranch, pr) { + if issues_model.MergeBlockedByRejectedReview(ctx, pb, pr) { return models.ErrDisallowedToMerge{ Reason: "There are requested changes", } } - if issues_model.MergeBlockedByOfficialReviewRequests(ctx, pr.ProtectedBranch, pr) { + if issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pr) { return models.ErrDisallowedToMerge{ Reason: "There are official review requests", } } - if issues_model.MergeBlockedByOutdatedBranch(pr.ProtectedBranch, pr) { + if issues_model.MergeBlockedByOutdatedBranch(pb, pr) { return models.ErrDisallowedToMerge{ Reason: "The head branch is behind the base branch", } @@ -805,7 +806,7 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return nil } - if pr.ProtectedBranch.MergeBlockedByProtectedFiles(pr.ChangedProtectedFiles) { + if pb.MergeBlockedByProtectedFiles(pr.ChangedProtectedFiles) { return models.ErrDisallowedToMerge{ Reason: "Changed protected files", } diff --git a/services/pull/patch.go b/services/pull/patch.go index 32895b2e784f..8a80d530c857 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -15,6 +15,7 @@ import ( "strings" "code.gitea.io/gitea/models" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/git" @@ -102,7 +103,7 @@ func TestPatch(pr *issues_model.PullRequest) error { } // 3. Check for protected files changes - if err = checkPullFilesProtection(pr, gitRepo); err != nil { + if err = checkPullFilesProtection(ctx, pr, gitRepo); err != nil { return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err) } @@ -527,23 +528,23 @@ func CheckUnprotectedFiles(repo *git.Repository, oldCommitID, newCommitID string } // checkPullFilesProtection check if pr changed protected files and save results -func checkPullFilesProtection(pr *issues_model.PullRequest, gitRepo *git.Repository) error { +func checkPullFilesProtection(ctx context.Context, pr *issues_model.PullRequest, gitRepo *git.Repository) error { if pr.Status == issues_model.PullRequestStatusEmpty { pr.ChangedProtectedFiles = nil return nil } - if err := pr.LoadProtectedBranch(); err != nil { + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + if err != nil { return err } - if pr.ProtectedBranch == nil { + if pb == nil { pr.ChangedProtectedFiles = nil return nil } - var err error - pr.ChangedProtectedFiles, err = CheckFileProtection(gitRepo, pr.MergeBase, "tracking", pr.ProtectedBranch.GetProtectedFilePatterns(), 10, os.Environ()) + pr.ChangedProtectedFiles, err = CheckFileProtection(gitRepo, pr.MergeBase, "tracking", pb.GetProtectedFilePatterns(), 10, os.Environ()) if err != nil && !models.IsErrFilePathProtected(err) { return err } diff --git a/services/pull/update.go b/services/pull/update.go index e5e26462e5aa..01ed833b0fca 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -9,6 +9,7 @@ import ( "fmt" "code.gitea.io/gitea/models" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -97,13 +98,13 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, BaseBranch: pull.HeadBranch, } - err = pr.LoadProtectedBranch() + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) if err != nil { return false, false, err } // can't do rebase on protected branch because need force push - if pr.ProtectedBranch == nil { + if pb == nil { prUnit, err := pr.BaseRepo.GetUnit(unit.TypePullRequests) if err != nil { log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err) @@ -113,7 +114,7 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, } // Update function need push permission - if pr.ProtectedBranch != nil && !pr.ProtectedBranch.CanUserPush(user.ID) { + if pb != nil && !pb.CanUserPush(user.ID) { return false, false, nil } diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index d55d793f28cd..979f265ca0af 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -67,7 +67,7 @@ func (opts *ApplyDiffPatchOptions) Validate(ctx context.Context, repo *repo_mode return err } } else { - protectedBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, opts.OldBranch) + protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, opts.OldBranch) if err != nil { return err } diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 4615a9153a68..b56704dc0a2a 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -463,7 +463,7 @@ func CreateOrUpdateRepoFile(ctx context.Context, repo *repo_model.Repository, do // VerifyBranchProtection verify the branch protection for modifying the given treePath on the given branch func VerifyBranchProtection(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, branchName, treePath string) error { - protectedBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, branchName) + protectedBranch, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, branchName) if err != nil { return err } From 1dfa78538e61254a94d1e3f6e833f35e59e1697d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 17 Aug 2022 18:20:40 +0800 Subject: [PATCH 02/34] Fix bug --- models/issues/pull.go | 32 +++++---------------- routers/web/repo/issue.go | 1 + templates/repo/issue/view_content/pull.tmpl | 4 +-- 3 files changed, 10 insertions(+), 27 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index fd86ceed68d1..f411623ee295 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -150,14 +150,13 @@ type PullRequest struct { Issue *Issue `xorm:"-"` Index int64 - HeadRepoID int64 `xorm:"INDEX"` - HeadRepo *repo_model.Repository `xorm:"-"` - BaseRepoID int64 `xorm:"INDEX"` - BaseRepo *repo_model.Repository `xorm:"-"` - HeadBranch string - HeadCommitID string `xorm:"-"` - BaseBranch string - // ProtectedBranch *git_model.ProtectedBranch `xorm:"-"` + HeadRepoID int64 `xorm:"INDEX"` + HeadRepo *repo_model.Repository `xorm:"-"` + BaseRepoID int64 `xorm:"INDEX"` + BaseRepo *repo_model.Repository `xorm:"-"` + HeadBranch string + HeadCommitID string `xorm:"-"` + BaseBranch string MergeBase string `xorm:"VARCHAR(40)"` AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` @@ -305,23 +304,6 @@ func (pr *PullRequest) LoadIssueCtx(ctx context.Context) (err error) { return err } -// LoadProtectedBranchRules loads the protected branch of the base branch -/*func (pr *PullRequest) LoadProtectedBranchRules(ctx context.Context) (err error) { - if pr.ProtectedBranch == nil { - if pr.BaseRepo == nil { - if pr.BaseRepoID == 0 { - return nil - } - pr.BaseRepo, err = repo_model.GetRepositoryByIDCtx(ctx, pr.BaseRepoID) - if err != nil { - return - } - } - pr.ProtectedBranch, err = git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepo.ID, pr.BaseBranch) - } - return err -}*/ - // ReviewCount represents a count of Reviews type ReviewCount struct { IssueID int64 diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index baa4ad9f5a70..96a9f33dd3b3 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1641,6 +1641,7 @@ func ViewIssue(ctx *context.Context) { if ctx.Doer != nil { showMergeInstructions = pb.CanUserPush(ctx.Doer.ID) } + ctx.Data["ProtectedBranch"] = pb ctx.Data["IsBlockedByApprovals"] = !issues_model.HasEnoughApprovals(ctx, pb, pull) ctx.Data["IsBlockedByRejection"] = issues_model.MergeBlockedByRejectedReview(ctx, pb, pull) ctx.Data["IsBlockedByOfficialReviewRequests"] = issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pull) diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index fd901f013ebd..30771defdb2a 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -204,7 +204,7 @@ {{if .IsBlockedByApprovals}}
{{svg "octicon-x"}} - {{$.locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} + {{$.locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{else if .IsBlockedByRejection}}
@@ -440,7 +440,7 @@ {{if .IsBlockedByApprovals}}
{{svg "octicon-x"}} - {{$.locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}} + {{$.locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{else if .IsBlockedByRejection}}
From bf27be688a63f934a18bce335f858c937a3d2486 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 17 Aug 2022 21:27:35 +0800 Subject: [PATCH 03/34] Fix fmt --- models/git/protected_branch.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 16108999aea1..0fb4835d0987 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -252,7 +252,6 @@ func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) return rel, nil } - // WhitelistOptions represent all sorts of whitelists used for protected branches type WhitelistOptions struct { UserIDs []int64 From 588c5b74268ac61f1ee81d30d8027b7bff9d379d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Aug 2022 15:01:04 +0800 Subject: [PATCH 04/34] Most UI changes --- models/git/protected_branch.go | 24 +- models/git/protected_branch_list.go | 10 +- options/locale/locale_en-US.ini | 4 + routers/api/v1/repo/branch.go | 4 +- routers/private/hook_pre_receive.go | 2 +- routers/web/repo/branch.go | 8 +- routers/web/repo/setting.go | 1 - routers/web/repo/setting_protected_branch.go | 274 +++++++++--------- routers/web/web.go | 4 +- services/forms/repo_form.go | 3 +- templates/repo/settings/branches.tmpl | 14 +- templates/repo/settings/protected_branch.tmpl | 76 ++--- 12 files changed, 209 insertions(+), 215 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 0fb4835d0987..dc3b9de5b81f 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -59,11 +59,6 @@ func init() { db.RegisterModel(new(ProtectedBranch)) } -// IsProtected returns if the branch is protected -func (protectBranch *ProtectedBranch) IsProtected() bool { - return protectBranch.ID > 0 -} - // Match tests if branchName matches the rule func (protectBranch *ProtectedBranch) Match(branchName string) bool { if strings.EqualFold(protectBranch.BranchName, branchName) { @@ -252,6 +247,19 @@ func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) return rel, nil } +// GetProtectedBranchRuleByID getting protected branch by ID/Name +func GetProtectedBranchRuleByID(ctx context.Context, repoID, ruleID int64) (*ProtectedBranch, error) { + rel := &ProtectedBranch{ID: ruleID, RepoID: repoID} + has, err := db.GetByBean(ctx, rel) + if err != nil { + return nil, err + } + if !has { + return nil, nil + } + return rel, nil +} + // WhitelistOptions represent all sorts of whitelists used for protected branches type WhitelistOptions struct { UserIDs []int64 @@ -325,12 +333,6 @@ func UpdateProtectBranch(ctx context.Context, repo *repo_model.Repository, prote return nil } -// GetProtectedBranches get all protected branches -func GetProtectedBranches(repoID int64) ([]*ProtectedBranch, error) { - protectedBranches := make([]*ProtectedBranch, 0) - return protectedBranches, db.GetEngine(db.DefaultContext).Find(&protectedBranches, &ProtectedBranch{RepoID: repoID}) -} - // IsProtectedBranch checks if branch is protected func IsProtectedBranch(repoID int64, branchName string) (bool, error) { protectedBranch := &ProtectedBranch{ diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index fce049da0c79..9c8cce45d4d6 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -21,18 +21,18 @@ func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedB return nil } -// FindMatchedProtectedBranchRules load all repository's protected rules -func FindMatchedProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedBranchRules, error) { +// FindRepoProtectedBranchRules load all repository's protected rules +func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedBranchRules, error) { var rules []*ProtectedBranch err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Asc("created_unix").Find(&rules) return rules, err } // GetFirstMatchProtectedBranchRule returns the first matched rules -func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { - rules, err := FindMatchedProtectedBranchRules(ctx, repoID) +func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, ruleName string) (*ProtectedBranch, error) { + rules, err := FindRepoProtectedBranchRules(ctx, repoID) if err != nil { return nil, err } - return rules.GetFirstMatched(branchName), nil + return rules.GetFirstMatched(ruleName), nil } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 0e309279d29b..9ec8ab11538e 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1791,6 +1791,7 @@ settings.mirror_sync_in_progress = Mirror synchronization is in progress. Check settings.site = Website settings.update_settings = Update Settings settings.branches.update_default_branch = Update Default Branch +settings.branches.add_new_rule = Add New Rule settings.advanced_settings = Advanced Settings settings.wiki_desc = Enable Repository Wiki settings.use_internal_wiki = Use Built-In Wiki @@ -2031,6 +2032,8 @@ settings.deploy_key_deletion_desc = Removing a deploy key will revoke its access settings.deploy_key_deletion_success = The deploy key has been removed. settings.branches = Branches settings.protected_branch = Branch Protection +settings.protected_branch.save_rule = Save Rule +settings.protected_branch.delete_rule = Delete Rule settings.protected_branch_can_push = Allow push? settings.protected_branch_can_push_yes = You can push settings.protected_branch_can_push_no = You can not push @@ -2065,6 +2068,7 @@ settings.dismiss_stale_approvals = Dismiss stale approvals settings.dismiss_stale_approvals_desc = When new commits that change the content of the pull request are pushed to the branch, old approvals will be dismissed. settings.require_signed_commits = Require Signed Commits settings.require_signed_commits_desc = Reject pushes to this branch if they are unsigned or unverifiable. +settings.protect_branch_name_pattern = Protected Branch Name Pattern settings.protect_protected_file_patterns = Protected file patterns (separated using semicolon '\;'): settings.protect_protected_file_patterns_desc = Protected files that are not allowed to be changed directly even if user has rights to add, edit, or delete files in this branch. Multiple patterns can be separated using semicolon ('\;'). See github.com/gobwas/glob documentation for pattern syntax. Examples: .drone.yml, /docs/**/*.txt. settings.protect_unprotected_file_patterns = Unprotected file patterns (separated using semicolon '\;'): diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 2923502c9531..e876fab7ffc7 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -260,7 +260,7 @@ func ListBranches(ctx *context.APIContext) { return } - rules, err := git_model.FindMatchedProtectedBranchRules(ctx, ctx.Repo.Repository.ID) + rules, err := git_model.FindRepoProtectedBranchRules(ctx, ctx.Repo.Repository.ID) if err != nil { ctx.Error(http.StatusInternalServerError, "FindMatchedProtectedBranchRules", err) return @@ -359,7 +359,7 @@ func ListBranchProtections(ctx *context.APIContext) { // "$ref": "#/responses/BranchProtectionList" repo := ctx.Repo.Repository - bps, err := git_model.GetProtectedBranches(repo.ID) + bps, err := git_model.FindRepoProtectedBranchRules(ctx, repo.ID) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranches", err) return diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 53c0a03fa1b8..19933a3247b2 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -167,7 +167,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN } // Allow pushes to non-protected branches - if protectBranch == nil || !protectBranch.IsProtected() { + if protectBranch == nil { return } diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index d14ba6cbe9a4..b0e67bfd2294 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -186,9 +186,9 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in return nil, nil, 0 } - protectedBranches, err := git_model.GetProtectedBranches(ctx.Repo.Repository.ID) + rules, err := git_model.FindRepoProtectedBranchRules(ctx, ctx.Repo.Repository.ID) if err != nil { - ctx.ServerError("GetProtectedBranches", err) + ctx.ServerError("FindRepoProtectedBranchRules", err) return nil, nil, 0 } @@ -205,7 +205,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in continue } - branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo) + branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, rules, repoIDToRepo, repoIDToGitRepo) if branch == nil { return nil, nil, 0 } @@ -217,7 +217,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in if defaultBranch != nil { // Always add the default branch log.Debug("loadOneBranch: load default: '%s'", defaultBranch.Name) - defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo) + defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, rules, repoIDToRepo, repoIDToGitRepo) branches = append(branches, defaultBranchBranch) } diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index a59824cecdb4..931e81a26a8a 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -55,7 +55,6 @@ const ( tplGithooks base.TplName = "repo/settings/githooks" tplGithookEdit base.TplName = "repo/settings/githook_edit" tplDeployKeys base.TplName = "repo/settings/deploy_keys" - tplProtectedBranch base.TplName = "repo/settings/protected_branch" ) // SettingsCtxData is a middleware that sets all the general context data for the diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index c4cd3486aaa6..e6c8d267a505 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -20,41 +20,27 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/forms" pull_service "code.gitea.io/gitea/services/pull" "code.gitea.io/gitea/services/repository" ) +const ( + tplProtectedBranch base.TplName = "repo/settings/protected_branch" +) + // ProtectedBranch render the page to protect the repository func ProtectedBranch(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.settings") ctx.Data["PageIsSettingsBranches"] = true - protectedBranches, err := git_model.GetProtectedBranches(ctx.Repo.Repository.ID) + rules, err := git_model.FindRepoProtectedBranchRules(ctx, ctx.Repo.Repository.ID) if err != nil { ctx.ServerError("GetProtectedBranches", err) return } - ctx.Data["ProtectedBranches"] = protectedBranches - - branches := ctx.Data["Branches"].([]string) - leftBranches := make([]string, 0, len(branches)-len(protectedBranches)) - for _, b := range branches { - var protected bool - for _, pb := range protectedBranches { - if b == pb.BranchName { - protected = true - break - } - } - if !protected { - leftBranches = append(leftBranches, b) - } - } - - ctx.Data["LeftBranches"] = leftBranches + ctx.Data["ProtectedBranches"] = rules ctx.HTML(http.StatusOK, tplBranches) } @@ -102,41 +88,36 @@ func ProtectedBranchPost(ctx *context.Context) { // SettingsProtectedBranch renders the protected branch setting page func SettingsProtectedBranch(c *context.Context) { - branch := c.Params("*") - if !c.Repo.GitRepo.IsBranchExist(branch) { - c.NotFound("IsBranchExist", nil) - return - } - - c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + branch - c.Data["PageIsSettingsBranches"] = true - - protectBranch, err := git_model.GetProtectedBranchBy(c, c.Repo.Repository.ID, branch) - if err != nil { - if !git.IsErrBranchNotExist(err) { + ruleID := c.ParamsInt64("id") + var rule *git_model.ProtectedBranch + if ruleID > 0 { + var err error + rule, err = git_model.GetProtectedBranchRuleByID(c, c.Repo.Repository.ID, ruleID) + if err != nil { c.ServerError("GetProtectBranchOfRepoByName", err) return } } - if protectBranch == nil { + if rule == nil { // No options found, create defaults. - protectBranch = &git_model.ProtectedBranch{ - BranchName: branch, - } + rule = &git_model.ProtectedBranch{} } + c.Data["PageIsSettingsBranches"] = true + c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + rule.BranchName + users, err := access_model.GetRepoReaders(c.Repo.Repository) if err != nil { c.ServerError("Repo.Repository.GetReaders", err) return } 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), ",") - c.Data["approvals_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.ApprovalsWhitelistUserIDs), ",") + c.Data["whitelist_users"] = strings.Join(base.Int64sToStrings(rule.WhitelistUserIDs), ",") + c.Data["merge_whitelist_users"] = strings.Join(base.Int64sToStrings(rule.MergeWhitelistUserIDs), ",") + c.Data["approvals_whitelist_users"] = strings.Join(base.Int64sToStrings(rule.ApprovalsWhitelistUserIDs), ",") contexts, _ := git_model.FindRepoRecentCommitStatusContexts(c.Repo.Repository.ID, 7*24*time.Hour) // Find last week status check contexts - for _, ctx := range protectBranch.StatusCheckContexts { + for _, ctx := range rule.StatusCheckContexts { var found bool for i := range contexts { if contexts[i] == ctx { @@ -151,7 +132,7 @@ func SettingsProtectedBranch(c *context.Context) { c.Data["branch_status_check_contexts"] = contexts c.Data["is_context_required"] = func(context string) bool { - for _, c := range protectBranch.StatusCheckContexts { + for _, c := range rule.StatusCheckContexts { if c == context { return true } @@ -166,130 +147,139 @@ func SettingsProtectedBranch(c *context.Context) { return } 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["approvals_whitelist_teams"] = strings.Join(base.Int64sToStrings(protectBranch.ApprovalsWhitelistTeamIDs), ",") + c.Data["whitelist_teams"] = strings.Join(base.Int64sToStrings(rule.WhitelistTeamIDs), ",") + c.Data["merge_whitelist_teams"] = strings.Join(base.Int64sToStrings(rule.MergeWhitelistTeamIDs), ",") + c.Data["approvals_whitelist_teams"] = strings.Join(base.Int64sToStrings(rule.ApprovalsWhitelistTeamIDs), ",") } - c.Data["Branch"] = protectBranch + c.Data["Rule"] = rule c.HTML(http.StatusOK, tplProtectedBranch) } // SettingsProtectedBranchPost updates the protected branch settings func SettingsProtectedBranchPost(ctx *context.Context) { f := web.GetForm(ctx).(*forms.ProtectBranchForm) - branch := ctx.Params("*") - if !ctx.Repo.GitRepo.IsBranchExist(branch) { - ctx.NotFound("IsBranchExist", nil) - return - } - - protectBranch, err := git_model.GetProtectedBranchBy(ctx, ctx.Repo.Repository.ID, branch) - if err != nil { - if !git.IsErrBranchNotExist(err) { + var protectBranch *git_model.ProtectedBranch + if f.RuleID > 0 { + var err error + protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) + if err != nil { ctx.ServerError("GetProtectBranchOfRepoByName", err) return } - } - if f.Protected { - if protectBranch == nil { - // No options found, create defaults. - protectBranch = &git_model.ProtectedBranch{ - RepoID: ctx.Repo.Repository.ID, - BranchName: branch, - } - } - if f.RequiredApprovals < 0 { - ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min")) - ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, util.PathEscapeSegments(branch))) + } else { + // No options found, create defaults. + protectBranch = &git_model.ProtectedBranch{ + RepoID: ctx.Repo.Repository.ID, } + } - var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64 - 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 - } + var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64 + protectBranch.BranchName = f.RuleName + if f.RequiredApprovals < 0 { + ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min")) + ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, f.RuleID)) + return + } - protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist - 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, ",")) - } + 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, ",")) } - - protectBranch.EnableStatusCheck = f.EnableStatusCheck - if f.EnableStatusCheck { - protectBranch.StatusCheckContexts = f.StatusCheckContexts - } else { - protectBranch.StatusCheckContexts = nil + if strings.TrimSpace(f.WhitelistTeams) != "" { + whitelistTeams, _ = base.StringsToInt64s(strings.Split(f.WhitelistTeams, ",")) } + default: + protectBranch.CanPush = false + protectBranch.EnableWhitelist = false + protectBranch.WhitelistDeployKeys = false + } - protectBranch.RequiredApprovals = f.RequiredApprovals - 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, ",")) - } + protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist + if f.EnableMergeWhitelist { + if strings.TrimSpace(f.MergeWhitelistUsers) != "" { + mergeWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ",")) } - protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews - protectBranch.BlockOnOfficialReviewRequests = f.BlockOnOfficialReviewRequests - protectBranch.DismissStaleApprovals = f.DismissStaleApprovals - protectBranch.RequireSignedCommits = f.RequireSignedCommits - protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns - protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns - protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch - - err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ - UserIDs: whitelistUsers, - TeamIDs: whitelistTeams, - MergeUserIDs: mergeWhitelistUsers, - MergeTeamIDs: mergeWhitelistTeams, - ApprovalsUserIDs: approvalsWhitelistUsers, - ApprovalsTeamIDs: approvalsWhitelistTeams, - }) - if err != nil { - ctx.ServerError("UpdateProtectBranch", err) - return + if strings.TrimSpace(f.MergeWhitelistTeams) != "" { + mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) } - if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { - ctx.ServerError("CheckPrsForBaseBranch", err) - return - } - ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", branch)) - ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, util.PathEscapeSegments(branch))) + } + + protectBranch.EnableStatusCheck = f.EnableStatusCheck + if f.EnableStatusCheck { + protectBranch.StatusCheckContexts = f.StatusCheckContexts } else { - if protectBranch != nil { - if err := git_model.DeleteProtectedBranch(ctx.Repo.Repository.ID, protectBranch.ID); err != nil { - ctx.ServerError("DeleteProtectedBranch", err) - return - } + protectBranch.StatusCheckContexts = nil + } + + protectBranch.RequiredApprovals = f.RequiredApprovals + 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, ",")) } - ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", branch)) - ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink)) } + protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews + protectBranch.BlockOnOfficialReviewRequests = f.BlockOnOfficialReviewRequests + protectBranch.DismissStaleApprovals = f.DismissStaleApprovals + protectBranch.RequireSignedCommits = f.RequireSignedCommits + protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns + protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns + protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch + + err := git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ + UserIDs: whitelistUsers, + TeamIDs: whitelistTeams, + MergeUserIDs: mergeWhitelistUsers, + MergeTeamIDs: mergeWhitelistTeams, + ApprovalsUserIDs: approvalsWhitelistUsers, + ApprovalsTeamIDs: approvalsWhitelistTeams, + }) + if err != nil { + ctx.ServerError("UpdateProtectBranch", err) + return + } + if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { + ctx.ServerError("CheckPrsForBaseBranch", err) + return + } + ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.BranchName)) + ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, protectBranch.ID)) +} + +func DeleteProtectedBranchRulePost(ctx *context.Context) { + ruleID := ctx.ParamsInt64("id") + if ruleID <= 0 { + return + } + rule, err := git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, ruleID) + if err != nil { + return + } + + if rule == nil { + return + } + + if err := git_model.DeleteProtectedBranch(ctx.Repo.Repository.ID, ruleID); err != nil { + ctx.ServerError("DeleteProtectedBranch", err) + return + } + + ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.BranchName)) + ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink)) } // RenameBranchPost responses for rename a branch diff --git a/routers/web/web.go b/routers/web/web.go index 34d3de6fde0a..c0b628acffcc 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -761,7 +761,9 @@ func RegisterRoutes(m *web.Route) { m.Group("/branches", func() { m.Combo("").Get(repo.ProtectedBranch).Post(repo.ProtectedBranchPost) - m.Combo("/*").Get(repo.SettingsProtectedBranch). + m.Combo("/new").Get(repo.SettingsProtectedBranch). + Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) + m.Combo("/{id}").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) }, repo.MustBeNotEmpty) m.Post("/rename_branch", bindIgnErr(forms.RenameBranchForm{}), context.RepoMustNotBeArchived(), repo.RenameBranchPost) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 7a4a2123ebd2..a7da8730c3ed 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -186,7 +186,8 @@ func (f *RepoSettingForm) Validate(req *http.Request, errs binding.Errors) bindi // ProtectBranchForm form for changing protected branch settings type ProtectBranchForm struct { - Protected bool + RuleID int64 + RuleName string `binding:"Required"` EnablePush string WhitelistUsers string WhitelistTeams string diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index cc8fc604267d..170aa4ceaf43 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -47,16 +47,8 @@
-
-
- {{svg "octicon-triangle-down" 14 "dropdown icon"}} -
{{.locale.Tr "repo.settings.choose_branch"}}
- -
+
+ {{$.locale.Tr "repo.settings.branches.add_new_rule"}}
@@ -67,7 +59,7 @@ {{range .ProtectedBranches}}
{{.BranchName}}
- {{$.locale.Tr "repo.settings.edit_protected_branch"}} + {{$.locale.Tr "repo.settings.edit_protected_branch"}} {{else}} {{.locale.Tr "repo.settings.no_protected_branch"}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index ff93218b2018..1e36a4f68438 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -4,42 +4,43 @@ {{template "repo/settings/navbar" .}}
{{template "base/alert" .}} -

- {{.locale.Tr "repo.settings.branch_protection" (.Branch.BranchName|Escape) | Str2html}} -

-
-
- {{.CsrfTokenHtml}} -
-
- - -

{{.locale.Tr "repo.settings.protect_this_branch_desc"}}

-
+ +

+ {{.locale.Tr "repo.settings.branch_protection" (.Rule.BranchName|Escape) | Str2html}} +

+
+
+ + +
-
+ +
+ + {{.CsrfTokenHtml}} +
- +

{{.locale.Tr "repo.settings.protect_disable_push_desc"}}

- +

{{.locale.Tr "repo.settings.protect_enable_push_desc"}}

- +

{{.locale.Tr "repo.settings.protect_whitelist_committers_desc"}}

-
+
@@ -76,20 +77,22 @@
- +
+
+
- +

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

-
+
@@ -127,13 +130,13 @@
- +

{{.locale.Tr "repo.settings.protect_check_status_contexts_desc"}}

-
+
@@ -159,17 +162,17 @@
- +

{{.locale.Tr "repo.settings.protect_required_approvals_desc"}}

- +

{{.locale.Tr "repo.settings.protect_approvals_whitelist_enabled_desc"}}

-
+
@@ -206,59 +209,60 @@
- +

{{.locale.Tr "repo.settings.block_rejected_reviews_desc"}}

- +

{{.locale.Tr "repo.settings.block_on_official_review_requests_desc"}}

- +

{{.locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}

- +

{{.locale.Tr "repo.settings.require_signed_commits_desc"}}

- +

{{.locale.Tr "repo.settings.block_outdated_branch_desc"}}

- +

{{.locale.Tr "repo.settings.protect_protected_file_patterns_desc" | Safe}}

- +

{{.locale.Tr "repo.settings.protect_unprotected_file_patterns_desc" | Safe}}

-
- + + +
- -
+
+ {{template "base/footer" .}} From b12ac622a7bc035f02ce7319626a3dffb4bb0253 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 6 Sep 2022 20:12:36 +0800 Subject: [PATCH 05/34] Fix test --- routers/web/repo/setting_protected_branch.go | 20 ++++++++++--------- routers/web/web.go | 8 +++++++- services/forms/repo_form.go | 1 - templates/repo/settings/branches.tmpl | 5 ++++- templates/repo/settings/protected_branch.tmpl | 3 +-- tests/integration/editor_test.go | 15 +++++++++++--- 6 files changed, 35 insertions(+), 17 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index e6c8d267a505..9b195185eb4f 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -30,8 +30,8 @@ const ( tplProtectedBranch base.TplName = "repo/settings/protected_branch" ) -// ProtectedBranch render the page to protect the repository -func ProtectedBranch(ctx *context.Context) { +// ProtectedBranchRules render the page to protect the repository +func ProtectedBranchRules(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.settings") ctx.Data["PageIsSettingsBranches"] = true @@ -45,8 +45,8 @@ func ProtectedBranch(ctx *context.Context) { ctx.HTML(http.StatusOK, tplBranches) } -// ProtectedBranchPost response for protect for a branch of a repository -func ProtectedBranchPost(ctx *context.Context) { +// SetDefaultBranchPost set default branch +func SetDefaultBranchPost(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.settings") ctx.Data["PageIsSettingsBranches"] = true @@ -159,19 +159,20 @@ func SettingsProtectedBranch(c *context.Context) { // SettingsProtectedBranchPost updates the protected branch settings func SettingsProtectedBranchPost(ctx *context.Context) { f := web.GetForm(ctx).(*forms.ProtectBranchForm) + ruleID := ctx.ParamsInt64("id") var protectBranch *git_model.ProtectedBranch - if f.RuleID > 0 { + if ruleID > 0 { var err error - protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) + protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, ruleID) if err != nil { ctx.ServerError("GetProtectBranchOfRepoByName", err) return } - } else { // No options found, create defaults. protectBranch = &git_model.ProtectedBranch{ - RepoID: ctx.Repo.Repository.ID, + RepoID: ctx.Repo.Repository.ID, + BranchName: f.RuleName, } } @@ -179,7 +180,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.BranchName = f.RuleName if f.RequiredApprovals < 0 { ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min")) - ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, f.RuleID)) + ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, ruleID)) return } @@ -259,6 +260,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, protectBranch.ID)) } +// DeleteProtectedBranchRulePost delete protected branch rule by id func DeleteProtectedBranchRulePost(ctx *context.Context) { ruleID := ctx.ParamsInt64("id") if ruleID <= 0 { diff --git a/routers/web/web.go b/routers/web/web.go index a71deb42a5ac..30925d760409 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -763,12 +763,18 @@ func RegisterRoutes(m *web.Route) { }) m.Group("/branches", func() { - m.Combo("").Get(repo.ProtectedBranch).Post(repo.ProtectedBranchPost) + m.Post("/", repo.SetDefaultBranchPost) + }, repo.MustBeNotEmpty) + + m.Group("/branches", func() { + m.Get("/", repo.ProtectedBranchRules) m.Combo("/new").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) + m.Get("/{id}/delete", repo.DeleteProtectedBranchRulePost) m.Combo("/{id}").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) }, repo.MustBeNotEmpty) + m.Post("/rename_branch", bindIgnErr(forms.RenameBranchForm{}), context.RepoMustNotBeArchived(), repo.RenameBranchPost) m.Group("/tags", func() { diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index e931b179d76f..dea9991f8dd5 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -186,7 +186,6 @@ func (f *RepoSettingForm) Validate(req *http.Request, errs binding.Errors) bindi // ProtectBranchForm form for changing protected branch settings type ProtectBranchForm struct { - RuleID int64 RuleName string `binding:"Required"` EnablePush string WhitelistUsers string diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index 170aa4ceaf43..c4469109f195 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -59,7 +59,10 @@ {{range .ProtectedBranches}} - + {{else}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 1e36a4f68438..f1f095f5290d 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -257,9 +257,8 @@
- - +
diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index 19e80dc7bfd9..b06026212397 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -77,11 +77,20 @@ func TestCreateFileOnProtectedBranch(t *testing.T) { // 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", + req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/new", map[string]string{ + "_csrf": csrf, + "rule_name": "master", + "enable_push": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + // Change master branch to protected + req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/1/delete", map[string]string{ + "_csrf": csrf, }) + session.MakeRequest(t, req, http.StatusSeeOther) // Check if master branch has been locked successfully flashCookie = session.GetCookie("macaron_flash") From 553869a7e2a6e60ceb30ae1b25d1f730b8051c03 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 12 Sep 2022 17:37:32 +0800 Subject: [PATCH 06/34] support protected branch rules --- models/git/branches.go | 13 +++++- models/git/protected_branch.go | 36 ++++++---------- models/git/protected_branch_list.go | 32 +++++++++++++- modules/structs/repo_branch.go | 2 +- routers/api/v1/repo/branch.go | 45 ++++++++++++++------ routers/api/v1/repo/pull.go | 3 +- routers/web/repo/branch.go | 2 +- routers/web/repo/issue.go | 2 +- routers/web/repo/pull.go | 2 +- routers/web/repo/setting_protected_branch.go | 14 +++++- services/pull/check.go | 4 +- services/repository/branch.go | 8 ++-- tests/integration/editor_test.go | 15 ++----- 13 files changed, 114 insertions(+), 64 deletions(-) diff --git a/models/git/branches.go b/models/git/branches.go index 8556bb4a7e6d..af239dc316ec 100644 --- a/models/git/branches.go +++ b/models/git/branches.go @@ -6,6 +6,7 @@ package git import ( "context" + "errors" "fmt" "time" @@ -16,6 +17,8 @@ import ( "code.gitea.io/gitea/modules/timeutil" ) +var ErrBranchIsProtected = errors.New("branch is protected") + // DeletedBranch struct type DeletedBranch struct { ID int64 `xorm:"pk autoincr"` @@ -149,7 +152,7 @@ func RenameBranch(repo *repo_model.Repository, from, to string, gitAction func(i } // 2. Update protected branch if needed - protectedBranch, err := GetProtectedBranchBy(ctx, repo.ID, from) + protectedBranch, err := GetProtectedBranchRuleByName(ctx, repo.ID, from) if err != nil { return err } @@ -160,6 +163,14 @@ func RenameBranch(repo *repo_model.Repository, from, to string, gitAction func(i if err != nil { return err } + } else { + protected, err := IsBranchProtected(repo.ID, from) + if err != nil { + return err + } + if protected { + return ErrBranchIsProtected + } } // 3. Update all not merged pull request base branch name diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index dc3b9de5b81f..d94f26d556c0 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -26,10 +26,11 @@ import ( // 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"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name + globRule glob.Glob `xorm:"-"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool WhitelistUserIDs []int64 `xorm:"JSON TEXT"` WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` @@ -64,7 +65,10 @@ func (protectBranch *ProtectedBranch) Match(branchName string) bool { if strings.EqualFold(protectBranch.BranchName, branchName) { return true } - return glob.MustCompile(protectBranch.BranchName).Match(branchName) + if protectBranch.globRule == nil { + protectBranch.globRule = glob.MustCompile(protectBranch.BranchName) + } + return protectBranch.globRule.Match(branchName) } // CanUserPush returns if some user could push to this protected branch @@ -234,9 +238,9 @@ func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, pa return r } -// GetProtectedBranchBy getting protected branch by ID/Name -func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { - rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName} +// GetProtectedBranchRuleByName getting protected branch rule by name +func GetProtectedBranchRuleByName(ctx context.Context, repoID int64, ruleName string) (*ProtectedBranch, error) { + rel := &ProtectedBranch{RepoID: repoID, BranchName: ruleName} has, err := db.GetByBean(ctx, rel) if err != nil { return nil, err @@ -247,7 +251,7 @@ func GetProtectedBranchBy(ctx context.Context, repoID int64, branchName string) return rel, nil } -// GetProtectedBranchRuleByID getting protected branch by ID/Name +// GetProtectedBranchRuleByID getting protected branch rule by rule ID func GetProtectedBranchRuleByID(ctx context.Context, repoID, ruleID int64) (*ProtectedBranch, error) { rel := &ProtectedBranch{ID: ruleID, RepoID: repoID} has, err := db.GetByBean(ctx, rel) @@ -333,20 +337,6 @@ func UpdateProtectBranch(ctx context.Context, repo *repo_model.Repository, prote return nil } -// IsProtectedBranch checks if branch is protected -func IsProtectedBranch(repoID int64, branchName string) (bool, error) { - protectedBranch := &ProtectedBranch{ - RepoID: repoID, - BranchName: branchName, - } - - has, err := db.GetEngine(db.DefaultContext).Exist(protectedBranch) - if err != nil { - return true, err - } - return has, nil -} - // updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with // the users from newWhitelist which have explicit read or write access to the repo. func updateApprovalWhitelist(ctx context.Context, repo *repo_model.Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) { diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index 9c8cce45d4d6..934355604b7b 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -8,6 +8,8 @@ import ( "context" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/git" + "github.com/gobwas/glob" ) type ProtectedBranchRules []*ProtectedBranch @@ -28,11 +30,37 @@ func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedB return rules, err } +// FindAllMatchedBranches find all matched branches +func FindAllMatchedBranches(ctx context.Context, gitRepo *git.Repository, ruleName string) ([]string, error) { + // FIXME: how many should we get? + branches, _, err := gitRepo.GetBranchNames(0, 9999999) + if err != nil { + return nil, err + } + rule := glob.MustCompile(ruleName) + results := make([]string, 0, len(branches)) + for _, branch := range branches { + if rule.Match(branch) { + results = append(results, branch) + } + } + return results, nil +} + // GetFirstMatchProtectedBranchRule returns the first matched rules -func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, ruleName string) (*ProtectedBranch, error) { +func GetFirstMatchProtectedBranchRule(ctx context.Context, repoID int64, branchName string) (*ProtectedBranch, error) { rules, err := FindRepoProtectedBranchRules(ctx, repoID) if err != nil { return nil, err } - return rules.GetFirstMatched(ruleName), nil + return rules.GetFirstMatched(branchName), nil +} + +// IsBranchProtected checks if branch is protected +func IsBranchProtected(repoID int64, branchName string) (bool, error) { + rule, err := GetFirstMatchProtectedBranchRule(db.DefaultContext, repoID, branchName) + if err != nil { + return false, err + } + return rule != nil, nil } diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 1f3bc04e8640..02a3915f165a 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -53,7 +53,7 @@ type BranchProtection struct { // CreateBranchProtectionOption options for creating a branch protection type CreateBranchProtectionOption struct { - BranchName string `json:"branch_name"` + RuleName string `json:"branch_name"` // it now in fact stores rule name not only branch name EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index e876fab7ffc7..6a2944b1423b 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -125,7 +125,7 @@ func DeleteBranch(ctx *context.APIContext) { ctx.NotFound(err) case errors.Is(err, repo_service.ErrBranchIsDefault): ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) - case errors.Is(err, repo_service.ErrBranchIsProtected): + case errors.Is(err, git_model.ErrBranchIsProtected): ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) default: ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) @@ -323,7 +323,7 @@ func GetBranchProtection(ctx *context.APIContext) { repo := ctx.Repo.Repository bpName := ctx.Params(":name") - bp, err := git_model.GetProtectedBranchBy(ctx, repo.ID, bpName) + bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err) return @@ -410,12 +410,13 @@ func CreateBranchProtection(ctx *context.APIContext) { repo := ctx.Repo.Repository // Currently protection must match an actual branch - if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.BranchName) { + // FIXME: we should allow glob match + if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.RuleName) { ctx.NotFound() return } - protectBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, form.BranchName) + protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, form.RuleName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectBranchOfRepoByName", err) return @@ -489,7 +490,7 @@ func CreateBranchProtection(ctx *context.APIContext) { protectBranch = &git_model.ProtectedBranch{ RepoID: ctx.Repo.Repository.ID, - BranchName: form.BranchName, + BranchName: form.RuleName, CanPush: form.EnablePush, EnableWhitelist: form.EnablePush && form.EnablePushWhitelist, EnableMergeWhitelist: form.EnableMergeWhitelist, @@ -520,13 +521,22 @@ func CreateBranchProtection(ctx *context.APIContext) { return } - if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { - ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, form.RuleName) + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) return } + for _, branchName := range matchedBranches { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPRsForBaseBranch", err) + return + } + } + // Reload from db to get all whitelists - bp, err := git_model.GetProtectedBranchBy(ctx, ctx.Repo.Repository.ID, form.BranchName) + bp, err := git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, form.RuleName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err) return @@ -578,7 +588,7 @@ func EditBranchProtection(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.EditBranchProtectionOption) repo := ctx.Repo.Repository bpName := ctx.Params(":name") - protectBranch, err := git_model.GetProtectedBranchBy(ctx, repo.ID, bpName) + protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err) return @@ -755,13 +765,22 @@ func EditBranchProtection(ctx *context.APIContext) { return } - if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { - ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) return } + for _, branchName := range matchedBranches { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + return + } + } + // Reload from db to ensure get all whitelists - bp, err := git_model.GetProtectedBranchBy(ctx, repo.ID, bpName) + bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranchBy", err) return @@ -805,7 +824,7 @@ func DeleteBranchProtection(ctx *context.APIContext) { repo := ctx.Repo.Repository bpName := ctx.Params(":name") - bp, err := git_model.GetProtectedBranchBy(ctx, repo.ID, bpName) + bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err) return diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index dda05203df85..d10075f1b8e2 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/models" activities_model "code.gitea.io/gitea/models/activities" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" pull_model "code.gitea.io/gitea/models/pull" @@ -896,7 +897,7 @@ func MergePullRequest(ctx *context.APIContext) { ctx.NotFound(err) case errors.Is(err, repo_service.ErrBranchIsDefault): ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch")) - case errors.Is(err, repo_service.ErrBranchIsProtected): + case errors.Is(err, git_model.ErrBranchIsProtected): ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected")) default: ctx.Error(http.StatusInternalServerError, "DeleteBranch", err) diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index b0e67bfd2294..8717835d8747 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -100,7 +100,7 @@ func DeleteBranchPost(ctx *context.Context) { case errors.Is(err, repo_service.ErrBranchIsDefault): log.Debug("DeleteBranch: Can't delete default branch '%s'", branchName) ctx.Flash.Error(ctx.Tr("repo.branch.default_deletion_failed", branchName)) - case errors.Is(err, repo_service.ErrBranchIsProtected): + case errors.Is(err, git_model.ErrBranchIsProtected): log.Debug("DeleteBranch: Can't delete protected branch '%s'", branchName) ctx.Flash.Error(ctx.Tr("repo.branch.protected_deletion_failed", branchName)) default: diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index b9ebd77690e2..589799cfd4ff 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1600,7 +1600,7 @@ func ViewIssue(ctx *context.Context) { if perm.CanWrite(unit.TypeCode) { // Check if branch is not protected if pull.HeadBranch != pull.HeadRepo.DefaultBranch { - if protected, err := git_model.IsProtectedBranch(pull.HeadRepo.ID, pull.HeadBranch); err != nil { + if protected, err := git_model.IsBranchProtected(pull.HeadRepo.ID, pull.HeadBranch); err != nil { log.Error("IsProtectedBranch: %v", err) } else if !protected { canDelete = true diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 874f10dfd9d2..eb1b81e89d8e 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1390,7 +1390,7 @@ func deleteBranch(ctx *context.Context, pr *issues_model.PullRequest, gitRepo *g ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) case errors.Is(err, repo_service.ErrBranchIsDefault): ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) - case errors.Is(err, repo_service.ErrBranchIsProtected): + case errors.Is(err, git_model.ErrBranchIsProtected): ctx.Flash.Error(ctx.Tr("repo.branch.deletion_failed", fullBranchName)) default: log.Error("DeleteBranch: %v", err) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 9b195185eb4f..99707daa9cd5 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -252,10 +252,20 @@ func SettingsProtectedBranchPost(ctx *context.Context) { ctx.ServerError("UpdateProtectBranch", err) return } - if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { - ctx.ServerError("CheckPrsForBaseBranch", err) + + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + if err != nil { + ctx.ServerError("FindAllMatchedBranches", err) return } + for _, branchName := range matchedBranches { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + ctx.ServerError("CheckPRsForBaseBranch", err) + return + } + } + ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.BranchName)) ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, protectBranch.ID)) } diff --git a/services/pull/check.go b/services/pull/check.go index 1e0330c0c943..9f9f8c125117 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -351,8 +351,8 @@ func testPR(id int64) { checkAndUpdateStatus(pr) } -// CheckPrsForBaseBranch check all pulls with bseBrannch -func CheckPrsForBaseBranch(baseRepo *repo_model.Repository, baseBranchName string) error { +// CheckPRsForBaseBranch check all pulls with baseBrannch +func CheckPRsForBaseBranch(baseRepo *repo_model.Repository, baseBranchName string) error { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(baseRepo.ID, baseBranchName) if err != nil { return err diff --git a/services/repository/branch.go b/services/repository/branch.go index 6c9a5d76caf1..4e7a16208856 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -149,8 +149,7 @@ func RenameBranch(repo *repo_model.Repository, doer *user_model.User, gitRepo *g // enmuerates all branch related errors var ( - ErrBranchIsDefault = errors.New("branch is default") - ErrBranchIsProtected = errors.New("branch is protected") + ErrBranchIsDefault = errors.New("branch is default") ) // DeleteBranch delete branch @@ -159,13 +158,12 @@ func DeleteBranch(doer *user_model.User, repo *repo_model.Repository, gitRepo *g return ErrBranchIsDefault } - isProtected, err := git_model.IsProtectedBranch(repo.ID, branchName) + isProtected, err := git_model.IsBranchProtected(repo.ID, branchName) if err != nil { return err } - if isProtected { - return ErrBranchIsProtected + return git_model.ErrBranchIsProtected } commit, err := gitRepo.GetBranchCommit(branchName) diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index b06026212397..f557fd6f29f8 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -44,9 +44,10 @@ 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/master", map[string]string{ - "_csrf": csrf, - "protected": "on", + req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/new", map[string]string{ + "_csrf": csrf, + "rule_name": "master", + "enable_push": "true", }) session.MakeRequest(t, req, http.StatusSeeOther) // Check if master branch has been locked successfully @@ -78,14 +79,6 @@ func TestCreateFileOnProtectedBranch(t *testing.T) { // 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/new", map[string]string{ - "_csrf": csrf, - "rule_name": "master", - "enable_push": "true", - }) - session.MakeRequest(t, req, http.StatusOK) - // Change master branch to protected req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/1/delete", map[string]string{ "_csrf": csrf, From aba5aada8b41ba205798284e2cdac39981305a09 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 13 Sep 2022 19:38:32 +0800 Subject: [PATCH 07/34] support * not cross slash --- models/git/protected_branch.go | 26 ++++++++++----- models/git/protected_branch_test.go | 49 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 models/git/protected_branch_test.go diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index d94f26d556c0..83f03898eb1d 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -26,11 +26,11 @@ import ( // ProtectedBranch struct type ProtectedBranch struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"UNIQUE(s)"` - BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name - globRule glob.Glob `xorm:"-"` - CanPush bool `xorm:"NOT NULL DEFAULT false"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name + globRule []glob.Glob `xorm:"-"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool WhitelistUserIDs []int64 `xorm:"JSON TEXT"` WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` @@ -66,9 +66,21 @@ func (protectBranch *ProtectedBranch) Match(branchName string) bool { return true } if protectBranch.globRule == nil { - protectBranch.globRule = glob.MustCompile(protectBranch.BranchName) + parts := strings.Split(protectBranch.BranchName, "/") + for _, part := range parts { + protectBranch.globRule = append(protectBranch.globRule, glob.MustCompile(part)) + } + } + fields := strings.Split(branchName, "/") + if len(fields) != len(protectBranch.globRule) { + return false + } + for i := 0; i < len(fields); i++ { + if !protectBranch.globRule[i].Match(fields[i]) { + return false + } } - return protectBranch.globRule.Match(branchName) + return true } // CanUserPush returns if some user could push to this protected branch diff --git a/models/git/protected_branch_test.go b/models/git/protected_branch_test.go new file mode 100644 index 000000000000..36005b7f4e0c --- /dev/null +++ b/models/git/protected_branch_test.go @@ -0,0 +1,49 @@ +// Copyright 2022 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 git + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBranchRuleMatch(t *testing.T) { + kases := []struct { + Rule string + BranchName string + ExpectedMatch bool + }{ + { + Rule: "release/**", + BranchName: "release/v1.17", + ExpectedMatch: true, + }, + { + Rule: "release/v*", + BranchName: "release/v1.16", + ExpectedMatch: true, + }, + { + Rule: "*", + BranchName: "release/v1.16", + ExpectedMatch: false, + }, + } + + for _, kase := range kases { + pb := ProtectedBranch{BranchName: kase.Rule} + var should, infact string + if !kase.ExpectedMatch { + should = " not" + } else { + infact = " not" + } + assert.EqualValues(t, kase.ExpectedMatch, pb.Match(kase.BranchName), + fmt.Sprintf("%s should%s match %s but it is%s", kase.BranchName, should, kase.Rule, infact), + ) + } +} From e894bdf6c442ff9225d8614c4442c3f2fabeabc0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 18 Oct 2022 11:53:57 +0800 Subject: [PATCH 08/34] Fix lint --- templates/swagger/v1_json.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 728e88b734aa..0fcc3b7b9236 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14232,7 +14232,7 @@ }, "branch_name": { "type": "string", - "x-go-name": "BranchName" + "x-go-name": "RuleName" }, "dismiss_stale_approvals": { "type": "boolean", From 957a20b489344807388c6367c1f50bfb4f9be585 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 19 Oct 2022 15:57:22 +0800 Subject: [PATCH 09/34] Fix matching algorithm --- models/git/protected_branch.go | 27 ++++++-------------- models/git/protected_branch_test.go | 22 +++++++++++++++- options/locale/locale_en-US.ini | 6 ++--- routers/web/repo/setting_protected_branch.go | 4 ++- routers/web/web.go | 2 +- templates/repo/settings/branches.tmpl | 17 ++++++++++-- 6 files changed, 51 insertions(+), 27 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 83f03898eb1d..e7747917a3ad 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -26,11 +26,11 @@ import ( // ProtectedBranch struct type ProtectedBranch struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"UNIQUE(s)"` - BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name - globRule []glob.Glob `xorm:"-"` - CanPush bool `xorm:"NOT NULL DEFAULT false"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name + globRule glob.Glob `xorm:"-"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool WhitelistUserIDs []int64 `xorm:"JSON TEXT"` WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` @@ -66,21 +66,10 @@ func (protectBranch *ProtectedBranch) Match(branchName string) bool { return true } if protectBranch.globRule == nil { - parts := strings.Split(protectBranch.BranchName, "/") - for _, part := range parts { - protectBranch.globRule = append(protectBranch.globRule, glob.MustCompile(part)) - } - } - fields := strings.Split(branchName, "/") - if len(fields) != len(protectBranch.globRule) { - return false + protectBranch.globRule = glob.MustCompile(protectBranch.BranchName, '/') } - for i := 0; i < len(fields); i++ { - if !protectBranch.globRule[i].Match(fields[i]) { - return false - } - } - return true + + return protectBranch.globRule.Match(branchName) } // CanUserPush returns if some user could push to this protected branch diff --git a/models/git/protected_branch_test.go b/models/git/protected_branch_test.go index 36005b7f4e0c..e1d1c211c4e9 100644 --- a/models/git/protected_branch_test.go +++ b/models/git/protected_branch_test.go @@ -18,10 +18,25 @@ func TestBranchRuleMatch(t *testing.T) { ExpectedMatch bool }{ { - Rule: "release/**", + Rule: "release/*", BranchName: "release/v1.17", ExpectedMatch: true, }, + { + Rule: "release/**/v1.17", + BranchName: "release/test/v1.17", + ExpectedMatch: true, + }, + { + Rule: "release/**/v1.17", + BranchName: "release/test/1/v1.17", + ExpectedMatch: true, + }, + { + Rule: "release/*/v1.17", + BranchName: "release/test/1/v1.17", + ExpectedMatch: false, + }, { Rule: "release/v*", BranchName: "release/v1.16", @@ -32,6 +47,11 @@ func TestBranchRuleMatch(t *testing.T) { BranchName: "release/v1.16", ExpectedMatch: false, }, + { + Rule: "**", + BranchName: "release/v1.16", + ExpectedMatch: true, + }, } for _, kase := range kases { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 14d76775c446..56373c0a338f 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2090,9 +2090,9 @@ settings.protect_unprotected_file_patterns = Unprotected file patterns (separate settings.protect_unprotected_file_patterns_desc = Unprotected files that are allowed to be changed directly if user has write access, bypassing push restriction. Multiple patterns can be separated using semicolon ('\;'). See github.com/gobwas/glob documentation for pattern syntax. Examples: .drone.yml, /docs/**/*.txt. settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection -settings.update_protect_branch_success = Branch protection for branch '%s' has been updated. -settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled. -settings.protected_branch_deletion = Disable Branch Protection +settings.update_protect_branch_success = Branch protection for rule '%s' has been updated. +settings.remove_protected_branch_success = Branch protection for rule '%s' has been removed. +settings.protected_branch_deletion = Delete Branch Protection settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue? settings.block_rejected_reviews = Block merge on rejected reviews settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, even if there are enough approvals. diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 99707daa9cd5..6d299e9d2423 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -291,7 +291,9 @@ func DeleteProtectedBranchRulePost(ctx *context.Context) { } ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.BranchName)) - ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink)) + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), + }) } // RenameBranchPost responses for rename a branch diff --git a/routers/web/web.go b/routers/web/web.go index cf2cd72e7515..ad554aa69c69 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -809,7 +809,7 @@ func RegisterRoutes(m *web.Route) { m.Get("/", repo.ProtectedBranchRules) m.Combo("/new").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) - m.Get("/{id}/delete", repo.DeleteProtectedBranchRulePost) + m.Post("/{id}/delete", repo.DeleteProtectedBranchRulePost) m.Combo("/{id}").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) }, repo.MustBeNotEmpty) diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index c4469109f195..181bb976dcad 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -47,7 +47,7 @@
-
+
{{$.locale.Tr "repo.settings.branches.add_new_rule"}}
@@ -61,7 +61,8 @@
{{else}} @@ -97,4 +98,16 @@ {{end}} + +
+
+ {{svg "octicon-trash" 16 "mr-2"}} + {{.locale.Tr "repo.settings.protected_branch_deletion"}} +
+
+

{{.locale.Tr "repo.settings.protected_branch_deletion_desc"}}

+
+ {{template "base/delete_modal_actions" .}} +
+ {{template "base/footer" .}} From 30bbfd0d41c55e3d31fb3461d9e43ec38003d509 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 19 Oct 2022 16:09:16 +0800 Subject: [PATCH 10/34] Fix import lint --- models/git/protected_branch_list.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index 934355604b7b..c9f4582f5e35 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/git" + "github.com/gobwas/glob" ) From a82b8d18c3b0c3c937e8723bb5d1a863c232ea0a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 20 Oct 2022 13:24:43 +0800 Subject: [PATCH 11/34] Fix tests --- routers/api/v1/repo/branch.go | 94 ++++++++++++++++++++++------ tests/integration/api_branch_test.go | 4 +- 2 files changed, 78 insertions(+), 20 deletions(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 6a2944b1423b..79ccfbbd1958 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -22,6 +22,8 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" + + "github.com/gobwas/glob" ) // GetBranch get a branch of a repository @@ -409,12 +411,16 @@ func CreateBranchProtection(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.CreateBranchProtectionOption) repo := ctx.Repo.Repository - // Currently protection must match an actual branch - // FIXME: we should allow glob match - if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.RuleName) { - ctx.NotFound() + g, err := glob.Compile(form.RuleName, '/') + if err != nil { + ctx.Error(http.StatusBadRequest, "Create branch protection", "Branch protection rule name is not right") return } + isPlainRule := g.Match(form.RuleName) + isBranchName := isPlainRule + if isBranchName { + isBranchName = git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.RuleName) + } protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, form.RuleName) if err != nil { @@ -521,18 +527,38 @@ func CreateBranchProtection(ctx *context.APIContext) { return } - // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, form.RuleName) - if err != nil { - ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) - return - } - - for _, branchName := range matchedBranches { - if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + if isBranchName { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, form.RuleName); err != nil { ctx.Error(http.StatusInternalServerError, "CheckPRsForBaseBranch", err) return } + } else { + if !isPlainRule { + if ctx.Repo.GitRepo == nil { + ctx.Repo.GitRepo, err = git.OpenRepository(ctx, ctx.Repo.Repository.RepoPath()) + if err != nil { + ctx.Error(http.StatusInternalServerError, "OpenRepository", err) + return + } + defer func() { + ctx.Repo.GitRepo.Close() + ctx.Repo.GitRepo = nil + }() + } + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, form.RuleName) + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) + return + } + + for _, branchName := range matchedBranches { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPRsForBaseBranch", err) + return + } + } + } } // Reload from db to get all whitelists @@ -765,18 +791,50 @@ func EditBranchProtection(ctx *context.APIContext) { return } - // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + g, err := glob.Compile(bpName, '/') if err != nil { - ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) + ctx.Error(http.StatusBadRequest, "Create branch protection", "Branch protection rule name is not right") return } + isPlainRule := g.Match(bpName) + isBranchName := isPlainRule + if isBranchName { + isBranchName = git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), bpName) + } - for _, branchName := range matchedBranches { - if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + if isBranchName { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, bpName); err != nil { ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) return } + } else { + if !isPlainRule { + if ctx.Repo.GitRepo == nil { + ctx.Repo.GitRepo, err = git.OpenRepository(ctx, ctx.Repo.Repository.RepoPath()) + if err != nil { + ctx.Error(http.StatusInternalServerError, "OpenRepository", err) + return + } + defer func() { + ctx.Repo.GitRepo.Close() + ctx.Repo.GitRepo = nil + }() + } + + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) + return + } + + for _, branchName := range matchedBranches { + if err = pull_service.CheckPRsForBaseBranch(ctx.Repo.Repository, branchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + return + } + } + } } // Reload from db to ensure get all whitelists diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index bdfdd3c7520b..4f61598e6dd9 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -176,8 +176,8 @@ func testAPICreateBranch(t testing.TB, session *TestSession, user, repo, oldBran func TestAPIBranchProtection(t *testing.T) { defer tests.PrepareTestEnv(t)() - // Branch protection only on branch that exist - testAPICreateBranchProtection(t, "master/doesnotexist", http.StatusNotFound) + // Branch protection on branch that not exist + testAPICreateBranchProtection(t, "master/doesnotexist", http.StatusCreated) // Get branch protection on branch that exist but not branch protection testAPIGetBranchProtection(t, "master", http.StatusNotFound) From a5cc6d3bfc2c3c04070aba4e6989da76235b11d0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 20 Oct 2022 13:39:58 +0800 Subject: [PATCH 12/34] Rule Sort --- models/git/protected_branch.go | 12 +++++++++--- models/git/protected_branch_list.go | 24 ++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index e7747917a3ad..d1037ab8b32a 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -30,6 +30,7 @@ type ProtectedBranch struct { RepoID int64 `xorm:"UNIQUE(s)"` BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name globRule glob.Glob `xorm:"-"` + isPlainName bool `xorm:"-"` CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool WhitelistUserIDs []int64 `xorm:"JSON TEXT"` @@ -60,14 +61,19 @@ func init() { db.RegisterModel(new(ProtectedBranch)) } +func (protectBranch *ProtectedBranch) loadGlob() { + if protectBranch.globRule == nil { + protectBranch.globRule = glob.MustCompile(protectBranch.BranchName, '/') + protectBranch.isPlainName = protectBranch.globRule.Match(protectBranch.BranchName) + } +} + // Match tests if branchName matches the rule func (protectBranch *ProtectedBranch) Match(branchName string) bool { if strings.EqualFold(protectBranch.BranchName, branchName) { return true } - if protectBranch.globRule == nil { - protectBranch.globRule = glob.MustCompile(protectBranch.BranchName, '/') - } + protectBranch.loadGlob() return protectBranch.globRule.Match(branchName) } diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index c9f4582f5e35..7049b1c9f4a2 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -6,6 +6,7 @@ package git import ( "context" + "sort" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/git" @@ -24,11 +25,30 @@ func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedB return nil } +func (rules ProtectedBranchRules) Sort() { + sort.Slice(rules, func(i, j int) bool { + rules[i].loadGlob() + rules[j].loadGlob() + if rules[i].isPlainName { + if !rules[j].isPlainName { + return true + } + } else if rules[j].isPlainName { + return true + } + return rules[i].CreatedUnix < rules[j].CreatedUnix + }) +} + // FindRepoProtectedBranchRules load all repository's protected rules func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedBranchRules, error) { - var rules []*ProtectedBranch + var rules ProtectedBranchRules err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Asc("created_unix").Find(&rules) - return rules, err + if err != nil { + return nil, err + } + rules.Sort() + return rules, nil } // FindAllMatchedBranches find all matched branches From ad304ec43544bf01711ad527d2f4a6718dc8441c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 22 Oct 2022 17:59:21 +0800 Subject: [PATCH 13/34] Fix tests --- models/git/branches.go | 2 +- models/git/protected_branch.go | 68 ++++++++++++++++++-- models/org_team.go | 29 ++------- models/user.go | 24 +------ modules/convert/convert.go | 4 +- modules/structs/repo_branch.go | 2 +- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/branch.go | 4 +- routers/web/repo/branch.go | 15 ++--- routers/web/repo/setting_protected_branch.go | 32 ++++++--- templates/repo/settings/branches.tmpl | 2 +- tests/integration/api_branch_test.go | 8 +-- tests/integration/editor_test.go | 12 +++- 13 files changed, 117 insertions(+), 86 deletions(-) diff --git a/models/git/branches.go b/models/git/branches.go index af239dc316ec..54814da5071c 100644 --- a/models/git/branches.go +++ b/models/git/branches.go @@ -158,7 +158,7 @@ func RenameBranch(repo *repo_model.Repository, from, to string, gitAction func(i } if protectedBranch != nil { - protectedBranch.BranchName = to + protectedBranch.RuleName = to _, err = sess.ID(protectedBranch.ID).Cols("branch_name").Update(protectedBranch) if err != nil { return err diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index d1037ab8b32a..f9356f414f9a 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -28,7 +28,7 @@ import ( type ProtectedBranch struct { ID int64 `xorm:"pk autoincr"` RepoID int64 `xorm:"UNIQUE(s)"` - BranchName string `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name + RuleName string `xorm:"'branch_name' UNIQUE(s)"` // a branch name or a glob match to branch name globRule glob.Glob `xorm:"-"` isPlainName bool `xorm:"-"` CanPush bool `xorm:"NOT NULL DEFAULT false"` @@ -63,17 +63,17 @@ func init() { func (protectBranch *ProtectedBranch) loadGlob() { if protectBranch.globRule == nil { - protectBranch.globRule = glob.MustCompile(protectBranch.BranchName, '/') - protectBranch.isPlainName = protectBranch.globRule.Match(protectBranch.BranchName) + protectBranch.globRule = glob.MustCompile(protectBranch.RuleName, '/') + protectBranch.isPlainName = protectBranch.globRule.Match(protectBranch.RuleName) } } // Match tests if branchName matches the rule func (protectBranch *ProtectedBranch) Match(branchName string) bool { - if strings.EqualFold(protectBranch.BranchName, branchName) { - return true - } protectBranch.loadGlob() + if protectBranch.isPlainName { + return strings.EqualFold(protectBranch.RuleName, branchName) + } return protectBranch.globRule.Match(branchName) } @@ -247,7 +247,7 @@ func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, pa // GetProtectedBranchRuleByName getting protected branch rule by name func GetProtectedBranchRuleByName(ctx context.Context, repoID int64, ruleName string) (*ProtectedBranch, error) { - rel := &ProtectedBranch{RepoID: repoID, BranchName: ruleName} + rel := &ProtectedBranch{RepoID: repoID, RuleName: ruleName} has, err := db.GetByBean(ctx, rel) if err != nil { return nil, err @@ -432,3 +432,57 @@ func DeleteProtectedBranch(repoID, id int64) (err error) { return nil } + +// RemoveUserIDFromProtectedBranch remove all user ids from protected branch options +func RemoveUserIDFromProtectedBranch(ctx context.Context, p *ProtectedBranch, userID int64) error { + var matched1, matched2, matched3 bool + if len(p.WhitelistUserIDs) != 0 { + p.WhitelistUserIDs, matched1 = util.RemoveIDFromList( + p.WhitelistUserIDs, userID) + } + if len(p.ApprovalsWhitelistUserIDs) != 0 { + p.ApprovalsWhitelistUserIDs, matched2 = util.RemoveIDFromList( + p.ApprovalsWhitelistUserIDs, userID) + } + if len(p.MergeWhitelistUserIDs) != 0 { + p.MergeWhitelistUserIDs, matched3 = util.RemoveIDFromList( + p.MergeWhitelistUserIDs, userID) + } + if matched1 || matched2 || matched3 { + if _, err := db.GetEngine(ctx).ID(p.ID).Cols( + "whitelist_user_i_ds", + "merge_whitelist_user_i_ds", + "approvals_whitelist_user_i_ds", + ).Update(p); err != nil { + return fmt.Errorf("updateProtectedBranches: %v", err) + } + } + return nil +} + +// RemoveTeamIDFromProtectedBranch remove all team ids from protected branch options +func RemoveTeamIDFromProtectedBranch(ctx context.Context, p *ProtectedBranch, teamID int64) error { + var matched1, matched2, matched3 bool + if len(p.WhitelistTeamIDs) != 0 { + p.WhitelistTeamIDs, matched1 = util.RemoveIDFromList( + p.WhitelistTeamIDs, teamID) + } + if len(p.ApprovalsWhitelistTeamIDs) != 0 { + p.ApprovalsWhitelistTeamIDs, matched2 = util.RemoveIDFromList( + p.ApprovalsWhitelistTeamIDs, teamID) + } + if len(p.MergeWhitelistTeamIDs) != 0 { + p.MergeWhitelistTeamIDs, matched3 = util.RemoveIDFromList( + p.MergeWhitelistTeamIDs, teamID) + } + if matched1 || matched2 || matched3 { + if _, err := db.GetEngine(ctx).ID(p.ID).Cols( + "whitelist_team_i_ds", + "merge_whitelist_team_i_ds", + "approvals_whitelist_team_i_ds", + ).Update(p); err != nil { + return fmt.Errorf("updateProtectedBranches: %v", err) + } + } + return nil +} diff --git a/models/org_team.go b/models/org_team.go index 6066e7f5c9b1..e1919b8f649b 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -20,7 +20,6 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" "xorm.io/builder" ) @@ -380,7 +379,6 @@ func DeleteTeam(t *organization.Team) error { return err } defer committer.Close() - sess := db.GetEngine(ctx) if err := t.GetRepositoriesCtx(ctx); err != nil { return err @@ -393,34 +391,15 @@ func DeleteTeam(t *organization.Team) error { // update branch protections { protections := make([]*git_model.ProtectedBranch, 0, 10) - err := sess.In("repo_id", + err := db.GetEngine(ctx).In("repo_id", builder.Select("id").From("repository").Where(builder.Eq{"owner_id": t.OrgID})). Find(&protections) if err != nil { return fmt.Errorf("findProtectedBranches: %v", err) } for _, p := range protections { - var matched1, matched2, matched3 bool - if len(p.WhitelistTeamIDs) != 0 { - p.WhitelistTeamIDs, matched1 = util.RemoveIDFromList( - p.WhitelistTeamIDs, t.ID) - } - if len(p.ApprovalsWhitelistTeamIDs) != 0 { - p.ApprovalsWhitelistTeamIDs, matched2 = util.RemoveIDFromList( - p.ApprovalsWhitelistTeamIDs, t.ID) - } - if len(p.MergeWhitelistTeamIDs) != 0 { - p.MergeWhitelistTeamIDs, matched3 = util.RemoveIDFromList( - p.MergeWhitelistTeamIDs, t.ID) - } - if matched1 || matched2 || matched3 { - if _, err = sess.ID(p.ID).Cols( - "whitelist_team_i_ds", - "merge_whitelist_team_i_ds", - "approvals_whitelist_team_i_ds", - ).Update(p); err != nil { - return fmt.Errorf("updateProtectedBranches: %v", err) - } + if err := git_model.RemoveTeamIDFromProtectedBranch(ctx, p, t.ID); err != nil { + return err } } } @@ -441,7 +420,7 @@ func DeleteTeam(t *organization.Team) error { } // Update organization number of teams. - if _, err := sess.Exec("UPDATE `user` SET num_teams=num_teams-1 WHERE id=?", t.OrgID); err != nil { + if _, err := db.Exec(ctx, "UPDATE `user` SET num_teams=num_teams-1 WHERE id=?", t.OrgID); err != nil { return err } diff --git a/models/user.go b/models/user.go index 68be0d855509..2050fa73a8dc 100644 --- a/models/user.go +++ b/models/user.go @@ -24,7 +24,6 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" ) // DeleteUser deletes models associated to an user. @@ -141,27 +140,8 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) break } for _, p := range protections { - var matched1, matched2, matched3 bool - if len(p.WhitelistUserIDs) != 0 { - p.WhitelistUserIDs, matched1 = util.RemoveIDFromList( - p.WhitelistUserIDs, u.ID) - } - if len(p.ApprovalsWhitelistUserIDs) != 0 { - p.ApprovalsWhitelistUserIDs, matched2 = util.RemoveIDFromList( - p.ApprovalsWhitelistUserIDs, u.ID) - } - if len(p.MergeWhitelistUserIDs) != 0 { - p.MergeWhitelistUserIDs, matched3 = util.RemoveIDFromList( - p.MergeWhitelistUserIDs, u.ID) - } - if matched1 || matched2 || matched3 { - if _, err = e.ID(p.ID).Cols( - "whitelist_user_i_ds", - "merge_whitelist_user_i_ds", - "approvals_whitelist_user_i_ds", - ).Update(p); err != nil { - return fmt.Errorf("updateProtectedBranches: %v", err) - } + if err := git_model.RemoveUserIDFromProtectedBranch(ctx, p, u.ID); err != nil { + return err } } } diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 187c67fa7680..c895ae9e8ea3 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -81,7 +81,7 @@ func ToBranch(repo *repo_model.Repository, b *git.Branch, c *git.Commit, bp *git } if isRepoAdmin { - branch.EffectiveBranchProtectionName = bp.BranchName + branch.EffectiveBranchProtectionName = bp.RuleName } if user != nil { @@ -124,7 +124,7 @@ func ToBranchProtection(bp *git_model.ProtectedBranch) *api.BranchProtection { } return &api.BranchProtection{ - BranchName: bp.BranchName, + RuleName: bp.RuleName, EnablePush: bp.CanPush, EnablePushWhitelist: bp.EnableWhitelist, PushWhitelistUsernames: pushWhitelistUsernames, diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 02a3915f165a..b3dcde7060bf 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -23,7 +23,7 @@ type Branch struct { // BranchProtection represents a branch protection for a repository type BranchProtection struct { - BranchName string `json:"branch_name"` + RuleName string `json:"branch_name"` EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index fb8ec4664124..999ae0360a79 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2098,6 +2098,7 @@ settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection settings.update_protect_branch_success = Branch protection for rule '%s' has been updated. settings.remove_protected_branch_success = Branch protection for rule '%s' has been removed. +settings.remove_protected_branch_failed = Removing branch protection rule '%s' failed. settings.protected_branch_deletion = Delete Branch Protection settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue? settings.block_rejected_reviews = Block merge on rejected reviews diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 79ccfbbd1958..496daf1421bc 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -496,7 +496,7 @@ func CreateBranchProtection(ctx *context.APIContext) { protectBranch = &git_model.ProtectedBranch{ RepoID: ctx.Repo.Repository.ID, - BranchName: form.RuleName, + RuleName: form.RuleName, CanPush: form.EnablePush, EnableWhitelist: form.EnablePush && form.EnablePushWhitelist, EnableMergeWhitelist: form.EnableMergeWhitelist, @@ -822,7 +822,7 @@ func EditBranchProtection(ctx *context.APIContext) { } // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.RuleName) if err != nil { ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err) return diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index 6af1185f4227..77d3dd66c6ea 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -205,7 +205,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in continue } - branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, rules, repoIDToRepo, repoIDToGitRepo) + branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, &rules, repoIDToRepo, repoIDToGitRepo) if branch == nil { return nil, nil, 0 } @@ -217,7 +217,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in if defaultBranch != nil { // Always add the default branch log.Debug("loadOneBranch: load default: '%s'", defaultBranch.Name) - defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, rules, repoIDToRepo, repoIDToGitRepo) + defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, &rules, repoIDToRepo, repoIDToGitRepo) branches = append(branches, defaultBranchBranch) } @@ -233,7 +233,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in return defaultBranchBranch, branches, totalNumOfBranches } -func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, protectedBranches []*git_model.ProtectedBranch, +func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, protectedBranches *git_model.ProtectedBranchRules, repoIDToRepo map[int64]*repo_model.Repository, repoIDToGitRepo map[int64]*git.Repository, ) *Branch { @@ -246,13 +246,8 @@ func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, p } branchName := rawBranch.Name - var isProtected bool - for _, b := range protectedBranches { - if b.BranchName == branchName { - isProtected = true - break - } - } + p := protectedBranches.GetFirstMatched(branchName) + isProtected := p != nil divergence := &git.DivergeObject{ Ahead: -1, diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 6d299e9d2423..89fe3ab5c666 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -105,7 +105,7 @@ func SettingsProtectedBranch(c *context.Context) { } c.Data["PageIsSettingsBranches"] = true - c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + rule.BranchName + c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + rule.RuleName users, err := access_model.GetRepoReaders(c.Repo.Repository) if err != nil { @@ -171,13 +171,13 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } else { // No options found, create defaults. protectBranch = &git_model.ProtectedBranch{ - RepoID: ctx.Repo.Repository.ID, - BranchName: f.RuleName, + RepoID: ctx.Repo.Repository.ID, + RuleName: f.RuleName, } } var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64 - protectBranch.BranchName = f.RuleName + protectBranch.RuleName = f.RuleName if f.RequiredApprovals < 0 { ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min")) ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, ruleID)) @@ -254,7 +254,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } // FIXME: since we only need to recheck files protected rules, we could improve this - matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName) + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.RuleName) if err != nil { ctx.ServerError("FindAllMatchedBranches", err) return @@ -266,7 +266,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } } - ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.BranchName)) + ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.RuleName)) ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, protectBranch.ID)) } @@ -274,23 +274,39 @@ func SettingsProtectedBranchPost(ctx *context.Context) { func DeleteProtectedBranchRulePost(ctx *context.Context) { ruleID := ctx.ParamsInt64("id") if ruleID <= 0 { + ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID))) + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), + }) return } + rule, err := git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, ruleID) if err != nil { + ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID))) + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), + }) return } if rule == nil { + ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID))) + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), + }) return } if err := git_model.DeleteProtectedBranch(ctx.Repo.Repository.ID, ruleID); err != nil { - ctx.ServerError("DeleteProtectedBranch", err) + ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", rule.RuleName)) + ctx.JSON(http.StatusOK, map[string]interface{}{ + "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), + }) return } - ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.BranchName)) + ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.RuleName)) ctx.JSON(http.StatusOK, map[string]interface{}{ "redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink), }) diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index 181bb976dcad..86896dda181d 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -58,7 +58,7 @@ {{range .ProtectedBranches}} - + diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index f1f095f5290d..7a4405efb7bc 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -6,12 +6,12 @@ {{template "base/alert" .}}

- {{.locale.Tr "repo.settings.branch_protection" (.Rule.BranchName|Escape) | Str2html}} + {{.locale.Tr "repo.settings.branch_protection" (.Rule.RuleName|Escape) | Str2html}}

- +
From 6a55815a78dbc5a2ff7f53d9cc681be824169d58 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 5 Dec 2022 21:38:48 +0800 Subject: [PATCH 28/34] Some improvements --- models/git/protected_branch_list.go | 4 ++-- templates/repo/settings/branches.tmpl | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index b8cdba1e2b5e..cc866e3e9d70 100644 --- a/models/git/protected_branch_list.go +++ b/models/git/protected_branch_list.go @@ -24,7 +24,7 @@ func (rules ProtectedBranchRules) GetFirstMatched(branchName string) *ProtectedB return nil } -func (rules ProtectedBranchRules) Sort() { +func (rules ProtectedBranchRules) sort() { sort.Slice(rules, func(i, j int) bool { rules[i].loadGlob() rules[j].loadGlob() @@ -46,7 +46,7 @@ func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedB if err != nil { return nil, err } - rules.Sort() + rules.sort() return rules, nil } diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index 793ed81426af..ad3b3275e888 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -43,15 +43,12 @@

{{.locale.Tr "repo.settings.protected_branch"}} +
+ {{$.locale.Tr "repo.settings.branches.add_new_rule"}} +

-
-
- {{$.locale.Tr "repo.settings.branches.add_new_rule"}} -
-
-
{{.BranchName}}
{{$.locale.Tr "repo.settings.edit_protected_branch"}} + {{$.locale.Tr "repo.settings.edit_protected_branch"}} + {{$.locale.Tr "repo.settings.protected_branch.delete_rule"}} +
{{.locale.Tr "repo.settings.no_protected_branch"}}
{{.BranchName}}
 {{$.locale.Tr "repo.settings.edit_protected_branch"}} - {{$.locale.Tr "repo.settings.protected_branch.delete_rule"}} +
{{.BranchName}}
{{.RuleName}}
 {{$.locale.Tr "repo.settings.edit_protected_branch"}}
{{.RuleName}}
 - {{$.locale.Tr "repo.settings.edit_protected_branch"}} + {{$.locale.Tr "repo.settings.edit_protected_branch"}}
From 40577d7cd0177bca199422bdf19fdcbdc566d36c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 5 Dec 2022 22:40:41 +0800 Subject: [PATCH 29/34] Add branch name back in API but mark it as deprecated --- modules/convert/convert.go | 6 ++++++ modules/structs/repo_branch.go | 8 ++++++-- routers/api/v1/repo/branch.go | 11 ++++++++--- templates/swagger/v1_json.tmpl | 16 +++++++++++++--- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 84c911b3f074..63277a3499c7 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -123,7 +123,13 @@ func ToBranchProtection(bp *git_model.ProtectedBranch) *api.BranchProtection { log.Error("GetTeamNamesByID (ApprovalsWhitelistTeamIDs): %v", err) } + branchName := "" + if !git_model.IsRuleNameSpecial(bp.RuleName) { + branchName = bp.RuleName + } + return &api.BranchProtection{ + BranchName: branchName, RuleName: bp.RuleName, EnablePush: bp.CanPush, EnablePushWhitelist: bp.EnableWhitelist, diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index d78ad4aace95..e9927aec2797 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -22,7 +22,9 @@ type Branch struct { // BranchProtection represents a branch protection for a repository type BranchProtection struct { - RuleName string `json:"branch_name"` + // Deprecated: true + BranchName string `json:"branch_name"` + RuleName string `json:"rule_name"` EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` @@ -52,7 +54,9 @@ type BranchProtection struct { // CreateBranchProtectionOption options for creating a branch protection type CreateBranchProtectionOption struct { - RuleName string `json:"branch_name"` // it now in fact stores rule name not only branch name + // Deprecated: true + BranchName string `json:"branch_name"` + RuleName string `json:"rule_name"` EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index efdaa0f02144..f894a8617381 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -417,13 +417,18 @@ func CreateBranchProtection(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.CreateBranchProtectionOption) repo := ctx.Repo.Repository - isPlainRule := !git_model.IsRuleNameSpecial(form.RuleName) + ruleName := form.RuleName + if ruleName == "" { + ruleName = form.BranchName + } + + isPlainRule := !git_model.IsRuleNameSpecial(ruleName) var isBranchExist bool if isPlainRule { - isBranchExist = git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.RuleName) + isBranchExist = git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), ruleName) } - protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, form.RuleName) + protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, ruleName) if err != nil { ctx.Error(http.StatusInternalServerError, "GetProtectBranchOfRepoByName", err) return diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 9bc65b50e7d8..0bf0df016da1 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -13699,8 +13699,9 @@ "x-go-name": "BlockOnRejectedReviews" }, "branch_name": { + "description": "Deprecated: true", "type": "string", - "x-go-name": "RuleName" + "x-go-name": "BranchName" }, "created_at": { "type": "string", @@ -13776,6 +13777,10 @@ "format": "int64", "x-go-name": "RequiredApprovals" }, + "rule_name": { + "type": "string", + "x-go-name": "RuleName" + }, "status_check_contexts": { "type": "array", "items": { @@ -14231,8 +14236,9 @@ "x-go-name": "BlockOnRejectedReviews" }, "branch_name": { + "description": "Deprecated: true", "type": "string", - "x-go-name": "RuleName" + "x-go-name": "BranchName" }, "dismiss_stale_approvals": { "type": "boolean", @@ -14303,6 +14309,10 @@ "format": "int64", "x-go-name": "RequiredApprovals" }, + "rule_name": { + "type": "string", + "x-go-name": "RuleName" + }, "status_check_contexts": { "type": "array", "items": { @@ -20232,4 +20242,4 @@ "TOTPHeader": [] } ] -} +} \ No newline at end of file From 8fbe8ada0a3247a2de68fdf07f41328b2255631a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 6 Dec 2022 11:59:06 +0800 Subject: [PATCH 30/34] Fix lint --- routers/api/v1/repo/branch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index f894a8617381..5f472da321e6 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -419,7 +419,7 @@ func CreateBranchProtection(ctx *context.APIContext) { ruleName := form.RuleName if ruleName == "" { - ruleName = form.BranchName + ruleName = form.BranchName // nolint } isPlainRule := !git_model.IsRuleNameSpecial(ruleName) From 9f18da1889ebe8cc6910d4c932b5fc97b16af2e8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 11 Dec 2022 11:35:11 +0800 Subject: [PATCH 31/34] fix lint --- routers/api/v1/repo/branch.go | 2 +- templates/swagger/v1_json.tmpl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 5f472da321e6..327fbca19030 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -419,7 +419,7 @@ func CreateBranchProtection(ctx *context.APIContext) { ruleName := form.RuleName if ruleName == "" { - ruleName = form.BranchName // nolint + ruleName = form.BranchName //nolint } isPlainRule := !git_model.IsRuleNameSpecial(ruleName) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 1d3982a2a95e..87fa8d327483 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -20790,4 +20790,4 @@ "TOTPHeader": [] } ] -} \ No newline at end of file +} From 9e3a06d06faaa3bc3b2c563fc120e619181a46de Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 21 Dec 2022 12:32:14 +0800 Subject: [PATCH 32/34] Update models/git/protected_branch.go Co-authored-by: zeripath --- models/git/protected_branch.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 6f65b37c3c12..461721b2fc1b 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -76,7 +76,12 @@ func IsRuleNameSpecial(ruleName string) bool { func (protectBranch *ProtectedBranch) loadGlob() { if protectBranch.globRule == nil { - protectBranch.globRule = glob.MustCompile(protectBranch.RuleName, '/') + var err error + protectBranch.globRule, err = glob.Compile(protectBranch.RuleName, '/') + if err != nil { + log.Warn("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err) + protectBranch.globRule = glob.MustCompile(glob.QuoteMeta(protectBranch.RuleName), '/') + } protectBranch.isPlainName = !IsRuleNameSpecial(protectBranch.RuleName) } } From 59c07140c131463a54912c70c1ec2698b5f2161f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 30 Dec 2022 14:12:57 +0800 Subject: [PATCH 33/34] some improvement --- models/git/protected_branch.go | 38 +++++++++++++++++------------ modules/context/repo.go | 3 ++- routers/private/hook_pre_receive.go | 12 +++++++-- routers/web/repo/issue.go | 3 ++- services/convert/convert.go | 3 ++- services/pull/update.go | 13 ++++++++-- services/repository/files/patch.go | 9 ++++--- services/repository/files/update.go | 3 ++- 8 files changed, 58 insertions(+), 26 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 461721b2fc1b..bf6f59aed886 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -29,12 +29,13 @@ var ErrBranchIsProtected = errors.New("branch is protected") // ProtectedBranch struct type ProtectedBranch struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"UNIQUE(s)"` - RuleName string `xorm:"'branch_name' UNIQUE(s)"` // a branch name or a glob match to branch name - globRule glob.Glob `xorm:"-"` - isPlainName bool `xorm:"-"` - CanPush bool `xorm:"NOT NULL DEFAULT false"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s)"` + Repo *repo_model.Repository `xorm:"-"` + RuleName string `xorm:"'branch_name' UNIQUE(s)"` // a branch name or a glob match to branch name + globRule glob.Glob `xorm:"-"` + isPlainName bool `xorm:"-"` + CanPush bool `xorm:"NOT NULL DEFAULT false"` EnableWhitelist bool WhitelistUserIDs []int64 `xorm:"JSON TEXT"` WhitelistTeamIDs []int64 `xorm:"JSON TEXT"` @@ -96,20 +97,27 @@ func (protectBranch *ProtectedBranch) Match(branchName string) bool { return protectBranch.globRule.Match(branchName) } +func (protectBranch *ProtectedBranch) LoadRepo(ctx context.Context) (err error) { + if protectBranch.Repo != nil { + return nil + } + protectBranch.Repo, err = repo_model.GetRepositoryByID(ctx, protectBranch.RepoID) + return +} + // CanUserPush returns if some user could push to this protected branch -func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, userID int64) bool { +func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, user *user_model.User) bool { if !protectBranch.CanPush { return false } if !protectBranch.EnableWhitelist { - if user, err := user_model.GetUserByID(ctx, userID); err != nil { - log.Error("GetUserByID: %v", err) + if err := protectBranch.LoadRepo(ctx); err != nil { + log.Error("LoadRepo: %v", err) return false - } else if repo, err := repo_model.GetRepositoryByID(ctx, protectBranch.RepoID); err != nil { - log.Error("repo_model.GetRepositoryByID: %v", err) - return false - } else if writeAccess, err := access_model.HasAccessUnit(ctx, user, repo, unit.TypeCode, perm.AccessModeWrite); err != nil { + } + + if writeAccess, err := access_model.HasAccessUnit(ctx, user, protectBranch.Repo, unit.TypeCode, perm.AccessModeWrite); err != nil { log.Error("HasAccessUnit: %v", err) return false } else { @@ -117,7 +125,7 @@ func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, userID in } } - if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) { + if base.Int64sContains(protectBranch.WhitelistUserIDs, user.ID) { return true } @@ -125,7 +133,7 @@ func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, userID in return false } - in, err := organization.IsUserInTeams(ctx, userID, protectBranch.WhitelistTeamIDs) + in, err := organization.IsUserInTeams(ctx, user.ID, protectBranch.WhitelistTeamIDs) if err != nil { log.Error("IsUserInTeams: %v", err) return false diff --git a/modules/context/repo.go b/modules/context/repo.go index 1213349d432a..471d9c9fd345 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -126,7 +126,8 @@ func (r *Repository) CanCommitToBranch(ctx context.Context, doer *user_model.Use userCanPush := true requireSigned := false if protectedBranch != nil { - userCanPush = protectedBranch.CanUserPush(ctx, doer.ID) + protectedBranch.Repo = r.Repository + userCanPush = protectedBranch.CanUserPush(ctx, doer) requireSigned = protectedBranch.RequireSignedCommits } diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 62b45f5f2604..8468227077fd 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -169,6 +169,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN if protectBranch == nil { return } + protectBranch.Repo = repo // This ref is a protected branch. // @@ -238,7 +239,6 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN Err: fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), }) return - } changedProtectedfiles = true @@ -251,7 +251,15 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN if ctx.opts.DeployKeyID != 0 { canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) } else { - canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx, ctx.opts.UserID) + user, err := user_model.GetUserByID(ctx, ctx.opts.UserID) + if err != nil { + log.Error("Unable to GetUserByID for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, private.Response{ + Err: fmt.Sprintf("Unable to GetUserByID for commits from %s to %s: %v", oldCommitID, newCommitID, err), + }) + return + } + canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx, user) } // 6. If we're not allowed to push directly diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 0e07dd487c97..43895b3f83ff 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1687,9 +1687,10 @@ func ViewIssue(ctx *context.Context) { } ctx.Data["ShowMergeInstructions"] = true if pb != nil { + pb.Repo = pull.BaseRepo var showMergeInstructions bool if ctx.Doer != nil { - showMergeInstructions = pb.CanUserPush(ctx, ctx.Doer.ID) + showMergeInstructions = pb.CanUserPush(ctx, ctx.Doer) } ctx.Data["ProtectedBranch"] = pb ctx.Data["IsBlockedByApprovals"] = !issues_model.HasEnoughApprovals(ctx, pb, pull) diff --git a/services/convert/convert.go b/services/convert/convert.go index 63277a3499c7..bba57ebb82f7 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -89,7 +89,8 @@ func ToBranch(repo *repo_model.Repository, b *git.Branch, c *git.Commit, bp *git if err != nil { return nil, err } - branch.UserCanPush = bp.CanUserPush(db.DefaultContext, user.ID) + bp.Repo = repo + branch.UserCanPush = bp.CanUserPush(db.DefaultContext, user) branch.UserCanMerge = git_model.IsUserMergeWhitelisted(db.DefaultContext, bp, user.ID, permission) } diff --git a/services/pull/update.go b/services/pull/update.go index c4a4823dadf7..9e29f63c7c85 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -93,9 +93,15 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, return false, false, err } + if err := pull.LoadBaseRepo(ctx); err != nil { + return false, false, err + } + pr := &issues_model.PullRequest{ HeadRepoID: pull.BaseRepoID, + HeadRepo: pull.BaseRepo, BaseRepoID: pull.HeadRepoID, + BaseRepo: pull.HeadRepo, HeadBranch: pull.BaseBranch, BaseBranch: pull.HeadBranch, } @@ -119,8 +125,11 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, } // Update function need push permission - if pb != nil && !pb.CanUserPush(ctx, user.ID) { - return false, false, nil + if pb != nil { + pb.Repo = pull.BaseRepo + if !pb.CanUserPush(ctx, user) { + return false, false, nil + } } baseRepoPerm, err := access_model.GetUserRepoPermission(ctx, pull.BaseRepo, user) diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index 6771362b5388..73ee0fa815e4 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -70,9 +70,12 @@ func (opts *ApplyDiffPatchOptions) Validate(ctx context.Context, repo *repo_mode if err != nil { return err } - if protectedBranch != nil && !protectedBranch.CanUserPush(ctx, doer.ID) { - return models.ErrUserCannotCommit{ - UserName: doer.LowerName, + if protectedBranch != nil { + protectedBranch.Repo = repo + if !protectedBranch.CanUserPush(ctx, doer) { + return models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } } } if protectedBranch != nil && protectedBranch.RequireSignedCommits { diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 035ece47115f..674f89896d79 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -467,12 +467,13 @@ func VerifyBranchProtection(ctx context.Context, repo *repo_model.Repository, do return err } if protectedBranch != nil { + protectedBranch.Repo = repo isUnprotectedFile := false glob := protectedBranch.GetUnprotectedFilePatterns() if len(glob) != 0 { isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, treePath) } - if !protectedBranch.CanUserPush(ctx, doer.ID) && !isUnprotectedFile { + if !protectedBranch.CanUserPush(ctx, doer) && !isUnprotectedFile { return models.ErrUserCannotCommit{ UserName: doer.LowerName, } From 01c205792f683fe07e751b64eb09c97198733e8c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 30 Dec 2022 17:32:24 +0800 Subject: [PATCH 34/34] Fix lint --- models/git/protected_branch.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index bf6f59aed886..780424f96c9c 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -102,7 +102,7 @@ func (protectBranch *ProtectedBranch) LoadRepo(ctx context.Context) (err error) return nil } protectBranch.Repo, err = repo_model.GetRepositoryByID(ctx, protectBranch.RepoID) - return + return err } // CanUserPush returns if some user could push to this protected branch @@ -117,12 +117,12 @@ func (protectBranch *ProtectedBranch) CanUserPush(ctx context.Context, user *use return false } - if writeAccess, err := access_model.HasAccessUnit(ctx, user, protectBranch.Repo, unit.TypeCode, perm.AccessModeWrite); err != nil { + writeAccess, err := access_model.HasAccessUnit(ctx, user, protectBranch.Repo, unit.TypeCode, perm.AccessModeWrite) + if err != nil { log.Error("HasAccessUnit: %v", err) return false - } else { - return writeAccess } + return writeAccess } if base.Int64sContains(protectBranch.WhitelistUserIDs, user.ID) {