From c710ce34fb94e095c362f38dc74dde5fbb92cc36 Mon Sep 17 00:00:00 2001 From: Michael Hoang <10492681+Enzime@users.noreply.github.com> Date: Fri, 6 Mar 2026 11:03:12 +1100 Subject: [PATCH] Fix non-admins unable to automerge PRs from forks (#36833) Make `handlePullRequestAutoMerge` correctly check the permissions of the merging user against pr.BaseRepo. --------- Co-authored-by: Michael Hoang Co-authored-by: wxiaoguang Co-authored-by: Lunny Xiao --- services/automerge/automerge.go | 4 +- tests/integration/pull_merge_test.go | 111 +++++++++------------------ 2 files changed, 39 insertions(+), 76 deletions(-) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index e145f93f04..10a31f4d7b 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -245,9 +245,9 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer) + perm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer) if err != nil { - log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err) + log.Error("GetUserRepoPermission %-v: %v", pr.BaseRepo, err) return } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 5949966b09..c2859e2e16 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -98,7 +98,7 @@ func TestPullMerge(t *testing.T) { assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -135,7 +135,7 @@ func TestPullRebase(t *testing.T) { assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -172,7 +172,7 @@ func TestPullRebaseMerge(t *testing.T) { assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -209,7 +209,7 @@ func TestPullSquash(t *testing.T) { assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n") @@ -235,7 +235,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) { assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n") @@ -275,7 +275,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) { func TestPullCleanUpAfterMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n") @@ -326,7 +326,7 @@ func TestPullCleanUpAfterMerge(t *testing.T) { func TestCantMergeWorkInProgress(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -345,7 +345,7 @@ func TestCantMergeWorkInProgress(t *testing.T) { func TestCantMergeConflict(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n") @@ -387,7 +387,7 @@ func TestCantMergeConflict(t *testing.T) { func TestCantMergeUnrelated(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n") @@ -479,7 +479,7 @@ func TestCantMergeUnrelated(t *testing.T) { func TestFastForwardOnlyMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "update", "README.md", "Hello, World 2\n") @@ -514,7 +514,7 @@ func TestFastForwardOnlyMerge(t *testing.T) { func TestCantFastForwardOnlyMergeDiverging(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "diverging", "README.md", "Hello, World diverged\n") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World 2\n") @@ -631,7 +631,7 @@ func TestConflictChecking(t *testing.T) { func TestPullRetargetChildOnBranchDelete(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testEditFileToNewBranch(t, session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n") testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n(Edited - TestPullRetargetOnCleanup - child PR)") @@ -668,7 +668,7 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) { func TestPullDontRetargetChildOnWrongRepo(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n") testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n(Edited - TestPullDontRetargetChildOnWrongRepo - child PR)") @@ -733,7 +733,7 @@ func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) { func TestPullMergeIndexerNotifier(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull") @@ -793,27 +793,13 @@ func TestPullMergeIndexerNotifier(t *testing.T) { }) } -func testResetRepo(t *testing.T, repo *repo_model.Repository, branch, commitID string) { - assert.NoError(t, gitrepo.UpdateRef(t.Context(), repo, git.BranchPrefix+branch, commitID)) - - gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) - assert.NoError(t, err) - defer gitRepo.Close() - id, err := gitRepo.GetBranchCommitID(branch) - assert.NoError(t, err) - assert.Equal(t, commitID, id) -} - func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) forkedName := "repo1-1" testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "") - defer func() { - testDeleteRepository(t, session, "user1", forkedName) - }() testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") @@ -867,16 +853,10 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) assert.NoError(t, err) - masterCommitID, err := baseGitRepo.GetBranchCommitID("master") - assert.NoError(t, err) - branches, _, err := baseGitRepo.GetBranchNames(0, 100) assert.NoError(t, err) assert.ElementsMatch(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches) baseGitRepo.Close() - defer func() { - testResetRepo(t, baseRepo, "master", masterCommitID) - }() err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{ State: commitstatus.CommitStatusSuccess, @@ -897,18 +877,17 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request - session := loginUser(t, "user1") - user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - forkedName := "repo1-2" - testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "") - defer func() { - testDeleteRepository(t, session, "user1", forkedName) - }() - testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") - testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") + baseUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + baseSession := loginUser(t, "user2") + forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + forkSession := loginUser(t, "user5") + forkedName := "repo1-fork" + testRepoFork(t, forkSession, "user2", "repo1", forkUser.Name, forkedName, "") + testEditFile(t, forkSession, forkUser.Name, forkedName, "master", "README.md", "Hello, World (Edited)\n") + testPullCreate(t, forkSession, forkUser.Name, forkedName, false, "master", "master", "Indexer notifier test pull") baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) - forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName}) + forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: forkUser.Name, Name: forkedName}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ BaseRepoID: baseRepo.ID, BaseBranch: "master", @@ -924,15 +903,15 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { "status_check_contexts": "gitea/actions", "required_approvals": "1", }) - session.MakeRequest(t, req, http.StatusSeeOther) + baseSession.MakeRequest(t, req, http.StatusSeeOther) // first time insert automerge record, return true - scheduled, err := automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) + scheduled, err := automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.NoError(t, err) assert.True(t, scheduled) // second time insert automerge record, return false because it does exist - scheduled, err = automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false) + scheduled, err = automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false) assert.Error(t, err) assert.False(t, scheduled) @@ -946,14 +925,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) assert.NoError(t, err) - masterCommitID, err := baseGitRepo.GetBranchCommitID("master") - assert.NoError(t, err) baseGitRepo.Close() - defer func() { - testResetRepo(t, baseRepo, "master", masterCommitID) - }() - err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{ + err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, baseUser, sha, &git_model.CommitStatus{ State: commitstatus.CommitStatusSuccess, TargetURL: "https://gitea.com", Context: "gitea/actions", @@ -968,16 +942,13 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { assert.Empty(t, pr.MergedCommitID) // approve the PR from non-author - approveSession := loginUser(t, "user2") - testSubmitReview(t, approveSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK) + testSubmitReview(t, baseSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK) - time.Sleep(2 * time.Second) - - // reload pr again - pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) - assert.True(t, pr.HasMerged) + assert.Eventually(t, func() bool { + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + return pr.HasMerged + }, 2*time.Second, 100*time.Millisecond) assert.NotEmpty(t, pr.MergedCommitID) - unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID}) }) } @@ -1035,7 +1006,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. HeadBranch: "user2/test/head2", }) - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing // Change master branch to protected req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{ "rule_name": "master", @@ -1067,13 +1038,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. assert.NoError(t, err) sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) assert.NoError(t, err) - masterCommitID, err := baseGitRepo.GetBranchCommitID("master") - assert.NoError(t, err) baseGitRepo.Close() - defer func() { - testResetRepo(t, baseRepo, "master", masterCommitID) - }() - err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{ State: commitstatus.CommitStatusSuccess, TargetURL: "https://gitea.com", @@ -1087,7 +1052,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. assert.Empty(t, pr.MergedCommitID) // approve the PR from non-author - approveSession := loginUser(t, "user1") + approveSession := loginUser(t, "user1") // FIXME: don't use admin user for testing testSubmitReview(t, approveSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK) // reload pr again @@ -1102,11 +1067,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { // create a pull request - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing forkedName := "repo1-1" testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "") - defer testDeleteRepository(t, session, "user1", forkedName) - testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n") testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull") @@ -1144,7 +1107,7 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) { func TestPullSquashMergeEmpty(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { - session := loginUser(t, "user1") + session := loginUser(t, "user1") // FIXME: don't use admin user for testing testEditFileToNewBranch(t, session, "user2", "repo1", "master", "pr-squash-empty", "README.md", "Hello, World (Edited)\n") resp := testPullCreate(t, session, "user2", "repo1", false, "master", "pr-squash-empty", "This is a pull title")