Skip to content

Added checks for protected branches in pull requests #3544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 13, 2018
15 changes: 15 additions & 0 deletions models/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,21 @@ func (err ErrBranchNameConflict) Error() string {
return fmt.Sprintf("branch conflicts with existing branch [name: %s]", err.BranchName)
}

// ErrNotAllowedToMerge represents an error that a branch is protected and the current user is not allowed to modify it
type ErrNotAllowedToMerge struct {
Reason string
}

// IsErrNotAllowedToMerge checks if an error is an ErrNotAllowedToMerge.
func IsErrNotAllowedToMerge(err error) bool {
_, ok := err.(ErrNotAllowedToMerge)
return ok
}

func (err ErrNotAllowedToMerge) Error() string {
return fmt.Sprintf("not allowed to merge [reason: %s]", err.Reason)
}

// ErrTagAlreadyExists represents an error that tag with such name already exists
type ErrTagAlreadyExists struct {
TagName string
Expand Down
29 changes: 29 additions & 0 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,31 @@ const (
MergeStyleSquash MergeStyle = "squash"
)

// CheckUserAllowedToMerge checks whether the user is allowed to merge
func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) {
if doer == nil {
return ErrNotAllowedToMerge{
"Not signed in",
}
}

if pr.BaseRepo == nil {
if err = pr.GetBaseRepo(); err != nil {
return fmt.Errorf("GetBaseRepo: %v", err)
}
}

if protected, err := pr.BaseRepo.IsProtectedBranch(pr.BaseBranch, doer); err != nil {
return fmt.Errorf("IsProtectedBranch: %v", err)
} else if protected {
return ErrNotAllowedToMerge{
"The branch is protected",
}
}

return nil
}

// Merge merges pull request to base repository.
// FIXME: add repoWorkingPull make sure two merges does not happen at same time.
func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle MergeStyle, message string) (err error) {
Expand All @@ -287,6 +312,10 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle
}
prConfig := prUnit.PullRequestsConfig()

if err := pr.CheckUserAllowedToMerge(doer); err != nil {
return fmt.Errorf("CheckUserAllowedToMerge: %v", err)
}

// Check if merge style is correct and allowed
if !prConfig.IsMergeStyleAllowed(mergeStyle) {
return ErrInvalidMergeStyle{pr.BaseRepo.ID, mergeStyle}
Expand Down
9 changes: 9 additions & 0 deletions routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,15 @@ func ViewIssue(ctx *context.Context) {
}
prConfig := prUnit.PullRequestsConfig()

ctx.Data["AllowMerge"] = ctx.Data["IsRepositoryWriter"]
if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.ServerError("CheckUserAllowedToMerge", err)
return
}
ctx.Data["AllowMerge"] = false
}

// Check correct values and select default
if ms, ok := ctx.Data["MergeStyle"].(models.MergeStyle); !ok ||
!prConfig.IsMergeStyleAllowed(ms) {
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
<span class="octicon octicon-check"></span>
{{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}}
</div>
{{if .IsRepositoryWriter}}
{{if .AllowMerge}}
{{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}}
{{if or $prUnit.PullRequestsConfig.AllowMerge $prUnit.PullRequestsConfig.AllowRebase $prUnit.PullRequestsConfig.AllowSquash}}
<div class="ui divider"></div>
Expand Down