Skip to content

Commit 2b061c6

Browse files
lunnyGiteaBot
lunny
authored and
GiteaBot
committed
Fix delete branch perm checking (go-gitea#32654)
1 parent a332805 commit 2b061c6

File tree

5 files changed

+128
-81
lines changed

5 files changed

+128
-81
lines changed

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,6 @@ func DeleteBranch(ctx *context.APIContext) {
155155
}
156156
}
157157

158-
if ctx.Repo.Repository.IsMirror {
159-
ctx.Error(http.StatusForbidden, "IsMirrored", fmt.Errorf("can not delete branch of an mirror repository"))
160-
return
161-
}
162-
163158
if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
164159
switch {
165160
case git.IsErrBranchNotExist(err):

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

Lines changed: 44 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,49 +1000,54 @@ func MergePullRequest(ctx *context.APIContext) {
10001000
}
10011001
log.Trace("Pull request merged: %d", pr.ID)
10021002

1003-
if form.DeleteBranchAfterMerge {
1004-
// Don't cleanup when there are other PR's that use this branch as head branch.
1005-
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1006-
if err != nil {
1007-
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1008-
return
1009-
}
1010-
if exist {
1011-
ctx.Status(http.StatusOK)
1012-
return
1013-
}
1014-
1015-
var headRepo *git.Repository
1016-
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
1017-
headRepo = ctx.Repo.GitRepo
1018-
} else {
1019-
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1003+
// for agit flow, we should not delete the agit reference after merge
1004+
if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub {
1005+
// check permission even it has been checked in repo_service.DeleteBranch so that we don't need to
1006+
// do RetargetChildrenOnMerge
1007+
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil {
1008+
// Don't cleanup when there are other PR's that use this branch as head branch.
1009+
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
10201010
if err != nil {
1021-
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1011+
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
10221012
return
10231013
}
1024-
defer headRepo.Close()
1025-
}
1026-
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
1027-
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
1028-
return
1029-
}
1030-
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
1031-
switch {
1032-
case git.IsErrBranchNotExist(err):
1033-
ctx.NotFound(err)
1034-
case errors.Is(err, repo_service.ErrBranchIsDefault):
1035-
ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
1036-
case errors.Is(err, git_model.ErrBranchIsProtected):
1037-
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
1038-
default:
1039-
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
1014+
if exist {
1015+
ctx.Status(http.StatusOK)
1016+
return
1017+
}
1018+
1019+
var headRepo *git.Repository
1020+
if ctx.Repo != nil && ctx.Repo.Repository != nil && ctx.Repo.Repository.ID == pr.HeadRepoID && ctx.Repo.GitRepo != nil {
1021+
headRepo = ctx.Repo.GitRepo
1022+
} else {
1023+
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1024+
if err != nil {
1025+
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1026+
return
1027+
}
1028+
defer headRepo.Close()
1029+
}
1030+
if err := pull_service.RetargetChildrenOnMerge(ctx, ctx.Doer, pr); err != nil {
1031+
ctx.Error(http.StatusInternalServerError, "RetargetChildrenOnMerge", err)
1032+
return
1033+
}
1034+
if err := repo_service.DeleteBranch(ctx, ctx.Doer, pr.HeadRepo, headRepo, pr.HeadBranch); err != nil {
1035+
switch {
1036+
case git.IsErrBranchNotExist(err):
1037+
ctx.NotFound(err)
1038+
case errors.Is(err, repo_service.ErrBranchIsDefault):
1039+
ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
1040+
case errors.Is(err, git_model.ErrBranchIsProtected):
1041+
ctx.Error(http.StatusForbidden, "IsProtectedBranch", fmt.Errorf("branch protected"))
1042+
default:
1043+
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
1044+
}
1045+
return
1046+
}
1047+
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
1048+
// Do not fail here as branch has already been deleted
1049+
log.Error("DeleteBranch: %v", err)
10401050
}
1041-
return
1042-
}
1043-
if err := issues_model.AddDeletePRBranchComment(ctx, ctx.Doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
1044-
// Do not fail here as branch has already been deleted
1045-
log.Error("DeleteBranch: %v", err)
10461051
}
10471052
}
10481053

‎routers/web/repo/pull.go

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1160,32 +1160,34 @@ func MergePullRequest(ctx *context.Context) {
11601160

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

1163-
if form.DeleteBranchAfterMerge {
1164-
// Don't cleanup when other pr use this branch as head branch
1165-
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1163+
if !form.DeleteBranchAfterMerge {
1164+
ctx.JSONRedirect(issue.Link())
1165+
return
1166+
}
1167+
1168+
// Don't cleanup when other pr use this branch as head branch
1169+
exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
1170+
if err != nil {
1171+
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1172+
return
1173+
}
1174+
if exist {
1175+
ctx.JSONRedirect(issue.Link())
1176+
return
1177+
}
1178+
1179+
var headRepo *git.Repository
1180+
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
1181+
headRepo = ctx.Repo.GitRepo
1182+
} else {
1183+
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
11661184
if err != nil {
1167-
ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)
1168-
return
1169-
}
1170-
if exist {
1171-
ctx.JSONRedirect(issue.Link())
1185+
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
11721186
return
11731187
}
1174-
1175-
var headRepo *git.Repository
1176-
if ctx.Repo != nil && ctx.Repo.Repository != nil && pr.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil {
1177-
headRepo = ctx.Repo.GitRepo
1178-
} else {
1179-
headRepo, err = gitrepo.OpenRepository(ctx, pr.HeadRepo)
1180-
if err != nil {
1181-
ctx.ServerError(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
1182-
return
1183-
}
1184-
defer headRepo.Close()
1185-
}
1186-
deleteBranch(ctx, pr, headRepo)
1188+
defer headRepo.Close()
11871189
}
1188-
1190+
deleteBranch(ctx, pr, headRepo)
11891191
ctx.JSONRedirect(issue.Link())
11901192
}
11911193

@@ -1367,8 +1369,8 @@ func CleanUpPullRequest(ctx *context.Context) {
13671369

13681370
pr := issue.PullRequest
13691371

1370-
// Don't cleanup unmerged and unclosed PRs
1371-
if !pr.HasMerged && !issue.IsClosed {
1372+
// Don't cleanup unmerged and unclosed PRs and agit PRs
1373+
if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub {
13721374
ctx.NotFound("CleanUpPullRequest", nil)
13731375
return
13741376
}
@@ -1399,13 +1401,12 @@ func CleanUpPullRequest(ctx *context.Context) {
13991401
return
14001402
}
14011403

1402-
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, ctx.Doer)
1403-
if err != nil {
1404-
ctx.ServerError("GetUserRepoPermission", err)
1405-
return
1406-
}
1407-
if !perm.CanWrite(unit.TypeCode) {
1408-
ctx.NotFound("CleanUpPullRequest", nil)
1404+
if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil {
1405+
if errors.Is(err, util.ErrPermissionDenied) {
1406+
ctx.NotFound("CanDeleteBranch", nil)
1407+
} else {
1408+
ctx.ServerError("CanDeleteBranch", err)
1409+
}
14091410
return
14101411
}
14111412

‎services/repository/branch.go

Lines changed: 23 additions & 6 deletions
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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,10 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
553553

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

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

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

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

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

0 commit comments

Comments
 (0)