From 553869a7e2a6e60ceb30ae1b25d1f730b8051c03 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 12 Sep 2022 17:37:32 +0800 Subject: [PATCH] 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 8556bb4a7e..af239dc316 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 dc3b9de5b8..d94f26d556 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 9c8cce45d4..934355604b 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 1f3bc04e86..02a3915f16 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 e876fab7ff..6a2944b142 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 dda05203df..d10075f1b8 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 b0e67bfd22..8717835d87 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 b9ebd77690..589799cfd4 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 874f10dfd9..eb1b81e89d 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 9b195185eb..99707daa9c 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 1e0330c0c9..9f9f8c1251 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 6c9a5d76ca..4e7a162088 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 b060262123..f557fd6f29 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,