Skip to content

Commit

Permalink
Fix delete branch perm checking (go-gitea#32654)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lunny
lunny authored Dec 4, 2024
1 parent c9e582c commit 17053e9
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 81 deletions.
5 changes: 0 additions & 5 deletions routers/api/v1/repo/branch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,6 @@ func DeleteBranch(ctx *context.APIContext) {
}
}

if ctx.Repo.Repository.IsMirror {
ctx.Error(http.StatusForbidden, "IsMirrored", fmt.Errorf("can not delete branch of an mirror repository"))
return
}

if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
switch {
case git.IsErrBranchNotExist(err):
Expand Down
83 changes: 44 additions & 39 deletions routers/api/v1/repo/pull.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1057,49 +1057,54 @@ func MergePullRequest(ctx *context.APIContext) {
}
log.Trace("Pull request merged: %d", pr.ID)

if form.DeleteBranchAfterMerge {
// Don't cleanup when there are other PR's that use this branch as head branch.
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
if err != nil {
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
return
}
if exist {
ctx.Status(http.StatusOK)
return
}

var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo
} else {
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
// for agit flow, we should not delete the agit reference after merge
if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub {
// check permission even it has been checked in repo_service.DeleteBranch so that we don't need to
// do RetargetChildrenOnMerge
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil {
// Don't cleanup when there are other PR's that use this branch as head branch.
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
return
}
defer headRepo.Close()
}
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
return
}
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
switch {
case git.IsErrBranchNotExist(err):
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, git_model.ErrBranchIsProtected):
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
default:
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
if exist {
ctx.Status(http.StatusOK)
return
}

var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo
} else {
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
return
}
defer headRepo.Close()
}
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
return
}
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
switch {
case git.IsErrBranchNotExist(err):
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, git_model.ErrBranchIsProtected):
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
default:
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
}
return
}
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
// Do not fail here as branch has already been deleted
log.Error("DeleteBranch: %v", err)
}
return
}
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
// Do not fail here as branch has already been deleted
log.Error("DeleteBranch: %v", err)
}
}

Expand Down
63 changes: 32 additions & 31 deletions routers/web/repo/pull.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1185,32 +1185,34 @@ func MergePullRequest(ctx *context.Context) {

log.Trace("Pull request merged: %d", pr.ID)

if form.DeleteBranchAfterMerge {
// Don't cleanup when other pr use this branch as head branch
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
if !form.DeleteBranchAfterMerge {
ctx.JSONRedirect(issue.Link())
return
}

// Don't cleanup when other pr use this branch as head branch
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
if err != nil {
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
return
}
if exist {
ctx.JSONRedirect(issue.Link())
return
}

var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo
} else {
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
if err != nil {
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
return
}
if exist {
ctx.JSONRedirect(issue.Link())
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
return
}

var headRepo *git.Repository
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
headRepo = ctx.Repo.GitRepo
} else {
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
if err != nil {
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
return
}
defer headRepo.Close()
}
deleteBranch(ctx, pr, headRepo)
defer headRepo.Close()
}

deleteBranch(ctx, pr, headRepo)
ctx.JSONRedirect(issue.Link())
}

Expand Down Expand Up @@ -1403,8 +1405,8 @@ func CleanUpPullRequest(ctx *context.Context) {

pr := issue.PullRequest

// Don't cleanup unmerged and unclosed PRs
if !pr.HasMerged && !issue.IsClosed {
// Don't cleanup unmerged and unclosed PRs and agit PRs
if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub {
ctx.NotFound("CleanUpPullRequest", nil)
return
}
Expand Down Expand Up @@ -1435,13 +1437,12 @@ func CleanUpPullRequest(ctx *context.Context) {
return
}

perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return
}
if !perm.CanWrite(unit.TypeCode) {
ctx.NotFound("CleanUpPullRequest", nil)
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil {
if errors.Is(err, util.ErrPermissionDenied) {
ctx.NotFound("CanDeleteBranch", nil)
} else {
ctx.ServerError("CanDeleteBranch", err)
}
return
}

Expand Down
29 changes: 23 additions & 6 deletions services/repository/branch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import (
"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"
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/cache"
"code.gitea.io/gitea/modules/git"
Expand Down Expand Up @@ -463,15 +465,17 @@ var (
ErrBranchIsDefault = errors.New("branch is default")
)

// DeleteBranch delete branch
func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
err := repo.MustNotBeArchived()
func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error {
if branchName == repo.DefaultBranch {
return ErrBranchIsDefault
}

perm, err := access_model.GetUserRepoPermission(ctx, repo, doer)
if err != nil {
return err
}

if branchName == repo.DefaultBranch {
return ErrBranchIsDefault
if !perm.CanWrite(unit.TypeCode) {
return util.NewPermissionDeniedErrorf("permission denied to access repo %d unit %s", repo.ID, unit.TypeCode.LogString())
}

isProtected, err := git_model.IsBranchProtected(ctx, repo.ID, branchName)
Expand All @@ -481,6 +485,19 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
if isProtected {
return git_model.ErrBranchIsProtected
}
return nil
}

// DeleteBranch delete branch
func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
err := repo.MustNotBeArchived()
if err != nil {
return err
}

if err := CanDeleteBranch(ctx, repo, branchName, doer); err != nil {
return err
}

rawBranch, err := git_model.GetBranch(ctx, repo.ID, branchName)
if err != nil && !git_model.IsErrBranchNotExist(err) {
Expand Down
29 changes: 29 additions & 0 deletions tests/integration/pull_merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {

testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)

repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
assert.True(t, branchBasePR.IsDeleted)

// Check child PR
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
resp := session.MakeRequest(t, req, http.StatusOK)
Expand Down Expand Up @@ -584,6 +588,10 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {

testPullMerge(t, session, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)

repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
assert.True(t, branchBasePR.IsDeleted)

// Check child PR
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
resp := session.MakeRequest(t, req, http.StatusOK)
Expand All @@ -598,6 +606,27 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
})
}

func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
session := loginUser(t, "user4")
testRepoFork(t, session, "user2", "repo1", "user4", "repo1", "")
testEditFileToNewBranch(t, session, "user4", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n")

respBasePR := testPullCreate(t, session, "user4", "repo1", false, "master", "base-pr", "Base Pull Request")
elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/")
assert.EqualValues(t, "pulls", elemBasePR[3])

// user2 has no permission to delete branch of repo user1/repo1
session2 := loginUser(t, "user2")
testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)

repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: "repo1"})
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
// branch has not been deleted
assert.False(t, branchBasePR.IsDeleted)
})
}

func TestPullMergeIndexerNotifier(t *testing.T) {
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
// create a pull request
Expand Down

0 comments on commit 17053e9

Please sign in to comment.