Skip to content

Commit 17053e9

Browse files
lunnylunny
authored
Fix delete branch perm checking (#32654)
1 parent c9e582c commit 17053e9

File tree

5 files changed

+128
-81
lines changed

5 files changed

+128
-81
lines changed

‎routers/api/v1/repo/branch.go

-5
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,6 @@ func DeleteBranch(ctx *context.APIContext) {
150150
}
151151
}
152152

153-
if ctx.Repo.Repository.IsMirror {
154-
ctx.Error(http.StatusForbidden, "IsMirrored", fmt.Errorf("can not delete branch of an mirror repository"))
155-
return
156-
}
157-
158153
if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
159154
switch {
160155
case git.IsErrBranchNotExist(err):

‎routers/api/v1/repo/pull.go

+44-39
Original file line numberDiff line numberDiff line change
@@ -1057,49 +1057,54 @@ func MergePullRequest(ctx *context.APIContext) {
10571057
}
10581058
log.Trace("Pull request merged: %d", pr.ID)
10591059

1060-
if form.DeleteBranchAfterMerge {
1061-
// Don't cleanup when there are other PR's that use this branch as head branch.
1062-
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1063-
if err != nil {
1064-
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1065-
return
1066-
}
1067-
if exist {
1068-
ctx.Status(http.StatusOK)
1069-
return
1070-
}
1071-
1072-
var headRepo *git.Repository
1073-
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
1074-
headRepo = ctx.Repo.GitRepo
1075-
} else {
1076-
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1060+
// for agit flow, we should not delete the agit reference after merge
1061+
if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub {
1062+
// check permission even it has been checked in repo_service.DeleteBranch so that we don't need to
1063+
// do RetargetChildrenOnMerge
1064+
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil {
1065+
// Don't cleanup when there are other PR's that use this branch as head branch.
1066+
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
10771067
if err != nil {
1078-
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1068+
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
10791069
return
10801070
}
1081-
defer headRepo.Close()
1082-
}
1083-
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
1084-
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
1085-
return
1086-
}
1087-
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
1088-
switch {
1089-
case git.IsErrBranchNotExist(err):
1090-
ctx.NotFound(err)
1091-
case errors.Is(err, repo_service.ErrBranchIsDefault):
1092-
ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
1093-
case errors.Is(err, git_model.ErrBranchIsProtected):
1094-
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
1095-
default:
1096-
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
1071+
if exist {
1072+
ctx.Status(http.StatusOK)
1073+
return
1074+
}
1075+
1076+
var headRepo *git.Repository
1077+
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
1078+
headRepo = ctx.Repo.GitRepo
1079+
} else {
1080+
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1081+
if err != nil {
1082+
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1083+
return
1084+
}
1085+
defer headRepo.Close()
1086+
}
1087+
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
1088+
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
1089+
return
1090+
}
1091+
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
1092+
switch {
1093+
case git.IsErrBranchNotExist(err):
1094+
ctx.NotFound(err)
1095+
case errors.Is(err, repo_service.ErrBranchIsDefault):
1096+
ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
1097+
case errors.Is(err, git_model.ErrBranchIsProtected):
1098+
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
1099+
default:
1100+
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
1101+
}
1102+
return
1103+
}
1104+
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
1105+
// Do not fail here as branch has already been deleted
1106+
log.Error("DeleteBranch: %v", err)
10971107
}
1098-
return
1099-
}
1100-
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
1101-
// Do not fail here as branch has already been deleted
1102-
log.Error("DeleteBranch: %v", err)
11031108
}
11041109
}
11051110

‎routers/web/repo/pull.go

+32-31
Original file line numberDiff line numberDiff line change
@@ -1185,32 +1185,34 @@ func MergePullRequest(ctx *context.Context) {
11851185

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

1188-
if form.DeleteBranchAfterMerge {
1189-
// Don't cleanup when other pr use this branch as head branch
1190-
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1188+
if !form.DeleteBranchAfterMerge {
1189+
ctx.JSONRedirect(issue.Link())
1190+
return
1191+
}
1192+
1193+
// Don't cleanup when other pr use this branch as head branch
1194+
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1195+
if err != nil {
1196+
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1197+
return
1198+
}
1199+
if exist {
1200+
ctx.JSONRedirect(issue.Link())
1201+
return
1202+
}
1203+
1204+
var headRepo *git.Repository
1205+
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
1206+
headRepo = ctx.Repo.GitRepo
1207+
} else {
1208+
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
11911209
if err != nil {
1192-
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1193-
return
1194-
}
1195-
if exist {
1196-
ctx.JSONRedirect(issue.Link())
1210+
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
11971211
return
11981212
}
1199-
1200-
var headRepo *git.Repository
1201-
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
1202-
headRepo = ctx.Repo.GitRepo
1203-
} else {
1204-
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1205-
if err != nil {
1206-
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1207-
return
1208-
}
1209-
defer headRepo.Close()
1210-
}
1211-
deleteBranch(ctx, pr, headRepo)
1213+
defer headRepo.Close()
12121214
}
1213-
1215+
deleteBranch(ctx, pr, headRepo)
12141216
ctx.JSONRedirect(issue.Link())
12151217
}
12161218

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

14041406
pr := issue.PullRequest
14051407

1406-
// Don't cleanup unmerged and unclosed PRs
1407-
if !pr.HasMerged && !issue.IsClosed {
1408+
// Don't cleanup unmerged and unclosed PRs and agit PRs
1409+
if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub {
14081410
ctx.NotFound("CleanUpPullRequest", nil)
14091411
return
14101412
}
@@ -1435,13 +1437,12 @@ func CleanUpPullRequest(ctx *context.Context) {
14351437
return
14361438
}
14371439

1438-
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, ctx.Doer)
1439-
if err != nil {
1440-
ctx.ServerError("GetUserRepoPermission", err)
1441-
return
1442-
}
1443-
if !perm.CanWrite(unit.TypeCode) {
1444-
ctx.NotFound("CleanUpPullRequest", nil)
1440+
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil {
1441+
if errors.Is(err, util.ErrPermissionDenied) {
1442+
ctx.NotFound("CanDeleteBranch", nil)
1443+
} else {
1444+
ctx.ServerError("CanDeleteBranch", err)
1445+
}
14451446
return
14461447
}
14471448

‎services/repository/branch.go

+23-6
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ import (
1414
"code.gitea.io/gitea/models/db"
1515
git_model "code.gitea.io/gitea/models/git"
1616
issues_model "code.gitea.io/gitea/models/issues"
17+
access_model "code.gitea.io/gitea/models/perm/access"
1718
repo_model "code.gitea.io/gitea/models/repo"
19+
"code.gitea.io/gitea/models/unit"
1820
user_model "code.gitea.io/gitea/models/user"
1921
"code.gitea.io/gitea/modules/cache"
2022
"code.gitea.io/gitea/modules/git"
@@ -463,15 +465,17 @@ var (
463465
ErrBranchIsDefault = errors.New("branch is default")
464466
)
465467

466-
// DeleteBranch delete branch
467-
func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
468-
err := repo.MustNotBeArchived()
468+
func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error {
469+
if branchName == repo.DefaultBranch {
470+
return ErrBranchIsDefault
471+
}
472+
473+
perm, err := access_model.GetUserRepoPermission(ctx, repo, doer)
469474
if err != nil {
470475
return err
471476
}
472-
473-
if branchName == repo.DefaultBranch {
474-
return ErrBranchIsDefault
477+
if !perm.CanWrite(unit.TypeCode) {
478+
return util.NewPermissionDeniedErrorf("permission denied to access repo %d unit %s", repo.ID, unit.TypeCode.LogString())
475479
}
476480

477481
isProtected, err := git_model.IsBranchProtected(ctx, repo.ID, branchName)
@@ -481,6 +485,19 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
481485
if isProtected {
482486
return git_model.ErrBranchIsProtected
483487
}
488+
return nil
489+
}
490+
491+
// DeleteBranch delete branch
492+
func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
493+
err := repo.MustNotBeArchived()
494+
if err != nil {
495+
return err
496+
}
497+
498+
if err := CanDeleteBranch(ctx, repo, branchName, doer); err != nil {
499+
return err
500+
}
484501

485502
rawBranch, err := git_model.GetBranch(ctx, repo.ID, branchName)
486503
if err != nil && !git_model.IsErrBranchNotExist(err) {

‎tests/integration/pull_merge_test.go

+29
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
554554

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

557+
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
558+
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
559+
assert.True(t, branchBasePR.IsDeleted)
560+
557561
// Check child PR
558562
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
559563
resp := session.MakeRequest(t, req, http.StatusOK)
@@ -584,6 +588,10 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
584588

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

591+
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
592+
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
593+
assert.True(t, branchBasePR.IsDeleted)
594+
587595
// Check child PR
588596
req := NewRequest(t, "GET", test.RedirectURL(respChildPR))
589597
resp := session.MakeRequest(t, req, http.StatusOK)
@@ -598,6 +606,27 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
598606
})
599607
}
600608

609+
func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
610+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
611+
session := loginUser(t, "user4")
612+
testRepoFork(t, session, "user2", "repo1", "user4", "repo1", "")
613+
testEditFileToNewBranch(t, session, "user4", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n")
614+
615+
respBasePR := testPullCreate(t, session, "user4", "repo1", false, "master", "base-pr", "Base Pull Request")
616+
elemBasePR := strings.Split(test.RedirectURL(respBasePR), "/")
617+
assert.EqualValues(t, "pulls", elemBasePR[3])
618+
619+
// user2 has no permission to delete branch of repo user1/repo1
620+
session2 := loginUser(t, "user2")
621+
testPullMerge(t, session2, elemBasePR[1], elemBasePR[2], elemBasePR[4], repo_model.MergeStyleMerge, true)
622+
623+
repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user4", Name: "repo1"})
624+
branchBasePR := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "base-pr"})
625+
// branch has not been deleted
626+
assert.False(t, branchBasePR.IsDeleted)
627+
})
628+
}
629+
601630
func TestPullMergeIndexerNotifier(t *testing.T) {
602631
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
603632
// create a pull request

0 commit comments

Comments
 (0)