From 17053e953f697ba21e067f1ad7715b18e07e273b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 3 Dec 2024 19:59:48 -0800 Subject: [PATCH] Fix delete branch perm checking (#32654) --- routers/api/v1/repo/branch.go | 5 -- routers/api/v1/repo/pull.go | 83 +++++++++++++++------------- routers/web/repo/pull.go | 67 +++++++++++----------- services/repository/branch.go | 29 ++++++++-- tests/integration/pull_merge_test.go | 29 ++++++++++ 5 files changed, 130 insertions(+), 83 deletions(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 45c5c1cd14..53f3b4648a 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -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): diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 28d7379f07..1116a4e9b1 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -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) } } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index cd20c0b18e..e3b329d01d 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -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 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) + 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(fmt.Sprintf("OpenRepository[%s]", pr.HeadRepo.FullName()), err) + return + } + defer headRepo.Close() + } + deleteBranch(ctx, pr, headRepo) ctx.JSONRedirect(issue.Link()) } @@ -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 } @@ -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 } diff --git a/services/repository/branch.go b/services/repository/branch.go index 600ba96e92..508817c83e 100644 --- a/services/repository/branch.go +++ b/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 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) @@ -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) { diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index bbd99f7aab..eb3743bc17 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -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) @@ -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) @@ -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