Fix delete branch perm checking (#32654) (#32707)

Backport #32654 by @lunny

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

routers/api/v1/repo/branch.go

@@ -155,11 +155,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):

routers/api/v1/repo/pull.go

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

routers/web/repo/pull.go

@@ -1160,32 +1160,34 @@ func MergePullRequest(ctx *context.Context) {
 
 	log.Trace("Pull request merged: %d", pr.ID)
 
-	if form.DeleteBranchAfterMerge {
+	if !form.DeleteBranchAfterMerge {
-		// Don't cleanup when other pr use this branch as head branch
+		ctx.JSONRedirect(issue.Link())
-		exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
+		return
-		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(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
-				return
-			}
-			defer headRepo.Close()
-		}
-		deleteBranch(ctx, pr, headRepo)
 	}
 
+	// 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(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err)
+			return
+		}
+		defer headRepo.Close()
+	}
+	deleteBranch(ctx, pr, headRepo)
 	ctx.JSONRedirect(issue.Link())
 }
 
@@ -1367,8 +1369,8 @@ func CleanUpPullRequest(ctx *context.Context) {
 
 	pr := issue.PullRequest
 
-	// Don't cleanup unmerged and unclosed PRs
+	// Don't cleanup unmerged and unclosed PRs and agit PRs
-	if !pr.HasMerged && !issue.IsClosed {
+	if !pr.HasMerged && !issue.IsClosed && pr.Flow != issues_model.PullRequestFlowGithub {
 		ctx.NotFound("CleanUpPullRequest", nil)
 		return
 	}
@@ -1399,13 +1401,12 @@ func CleanUpPullRequest(ctx *context.Context) {
 		return
 	}
 
-	perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, ctx.Doer)
+	if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err != nil {
-	if err != nil {
+		if errors.Is(err, util.ErrPermissionDenied) {
-		ctx.ServerError("GetUserRepoPermission", err)
+			ctx.NotFound("CanDeleteBranch", nil)
-		return
+		} else {
-	}
+			ctx.ServerError("CanDeleteBranch", err)
-	if !perm.CanWrite(unit.TypeCode) {
+		}
-		ctx.NotFound("CleanUpPullRequest", nil)
 		return
 	}
 

services/repository/branch.go

@@ -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"
@@ -463,15 +465,17 @@ var (
 	ErrBranchIsDefault = errors.New("branch is default")
 )
 
-// DeleteBranch delete branch
+func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchName string, doer *user_model.User) error {
-func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
+	if branchName == repo.DefaultBranch {
-	err := repo.MustNotBeArchived()
+		return ErrBranchIsDefault
+	}
+
+	perm, err := access_model.GetUserRepoPermission(ctx, repo, doer)
 	if err != nil {
 		return err
 	}
-
+	if !perm.CanWrite(unit.TypeCode) {
-	if branchName == repo.DefaultBranch {
+		return util.NewPermissionDeniedErrorf("permission denied to access repo %d unit %s", repo.ID, unit.TypeCode.LogString())
-		return ErrBranchIsDefault
 	}
 
 	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
 	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) {

tests/integration/pull_merge_test.go

@@ -553,6 +553,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)
@@ -583,6 +587,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)
@@ -596,6 +604,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