Skip to content

Commit c710ce3

Browse files
Enzimewxiaoguanglunny
authored
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 <enzime@users.noreply.github.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
1 parent 9c2c9c5 commit c710ce3

2 files changed

Lines changed: 39 additions & 76 deletions

File tree

‎services/automerge/automerge.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,9 @@ func handlePullRequestAutoMerge(pullID int64, sha string) {
245245
return
246246
}
247247

248-
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer)
248+
perm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer)
249249
if err != nil {
250-
log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err)
250+
log.Error("GetUserRepoPermission %-v: %v", pr.BaseRepo, err)
251251
return
252252
}
253253

‎tests/integration/pull_merge_test.go‎

Lines changed: 37 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func TestPullMerge(t *testing.T) {
9898
assert.NoError(t, err)
9999
hookTasksLenBefore := len(hookTasks)
100100

101-
session := loginUser(t, "user1")
101+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
102102
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
103103
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
104104

@@ -135,7 +135,7 @@ func TestPullRebase(t *testing.T) {
135135
assert.NoError(t, err)
136136
hookTasksLenBefore := len(hookTasks)
137137

138-
session := loginUser(t, "user1")
138+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
139139
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
140140
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
141141

@@ -172,7 +172,7 @@ func TestPullRebaseMerge(t *testing.T) {
172172
assert.NoError(t, err)
173173
hookTasksLenBefore := len(hookTasks)
174174

175-
session := loginUser(t, "user1")
175+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
176176
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
177177
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
178178

@@ -209,7 +209,7 @@ func TestPullSquash(t *testing.T) {
209209
assert.NoError(t, err)
210210
hookTasksLenBefore := len(hookTasks)
211211

212-
session := loginUser(t, "user1")
212+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
213213
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
214214
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
215215
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")
@@ -235,7 +235,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) {
235235
assert.NoError(t, err)
236236
hookTasksLenBefore := len(hookTasks)
237237

238-
session := loginUser(t, "user1")
238+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
239239
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
240240
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
241241
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")
@@ -275,7 +275,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) {
275275

276276
func TestPullCleanUpAfterMerge(t *testing.T) {
277277
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
278-
session := loginUser(t, "user1")
278+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
279279
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
280280
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n")
281281

@@ -326,7 +326,7 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
326326

327327
func TestCantMergeWorkInProgress(t *testing.T) {
328328
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
329-
session := loginUser(t, "user1")
329+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
330330
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
331331
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
332332

@@ -345,7 +345,7 @@ func TestCantMergeWorkInProgress(t *testing.T) {
345345

346346
func TestCantMergeConflict(t *testing.T) {
347347
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
348-
session := loginUser(t, "user1")
348+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
349349
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
350350
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
351351
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
@@ -387,7 +387,7 @@ func TestCantMergeConflict(t *testing.T) {
387387

388388
func TestCantMergeUnrelated(t *testing.T) {
389389
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
390-
session := loginUser(t, "user1")
390+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
391391
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
392392
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
393393

@@ -479,7 +479,7 @@ func TestCantMergeUnrelated(t *testing.T) {
479479

480480
func TestFastForwardOnlyMerge(t *testing.T) {
481481
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
482-
session := loginUser(t, "user1")
482+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
483483
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
484484
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "update", "README.md", "Hello, World 2\n")
485485

@@ -514,7 +514,7 @@ func TestFastForwardOnlyMerge(t *testing.T) {
514514

515515
func TestCantFastForwardOnlyMergeDiverging(t *testing.T) {
516516
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
517-
session := loginUser(t, "user1")
517+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
518518
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
519519
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "diverging", "README.md", "Hello, World diverged\n")
520520
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World 2\n")
@@ -631,7 +631,7 @@ func TestConflictChecking(t *testing.T) {
631631

632632
func TestPullRetargetChildOnBranchDelete(t *testing.T) {
633633
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
634-
session := loginUser(t, "user1")
634+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
635635
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n")
636636
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
637637
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) {
668668

669669
func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
670670
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
671-
session := loginUser(t, "user1")
671+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
672672
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
673673
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n")
674674
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) {
733733
func TestPullMergeIndexerNotifier(t *testing.T) {
734734
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
735735
// create a pull request
736-
session := loginUser(t, "user1")
736+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
737737
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
738738
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
739739
createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull")
@@ -793,27 +793,13 @@ func TestPullMergeIndexerNotifier(t *testing.T) {
793793
})
794794
}
795795

796-
func testResetRepo(t *testing.T, repo *repo_model.Repository, branch, commitID string) {
797-
assert.NoError(t, gitrepo.UpdateRef(t.Context(), repo, git.BranchPrefix+branch, commitID))
798-
799-
gitRepo, err := gitrepo.OpenRepository(t.Context(), repo)
800-
assert.NoError(t, err)
801-
defer gitRepo.Close()
802-
id, err := gitRepo.GetBranchCommitID(branch)
803-
assert.NoError(t, err)
804-
assert.Equal(t, commitID, id)
805-
}
806-
807796
func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
808797
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
809798
// create a pull request
810-
session := loginUser(t, "user1")
799+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
811800
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
812801
forkedName := "repo1-1"
813802
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
814-
defer func() {
815-
testDeleteRepository(t, session, "user1", forkedName)
816-
}()
817803
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
818804
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
819805

@@ -867,16 +853,10 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
867853
assert.NoError(t, err)
868854
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
869855
assert.NoError(t, err)
870-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
871-
assert.NoError(t, err)
872-
873856
branches, _, err := baseGitRepo.GetBranchNames(0, 100)
874857
assert.NoError(t, err)
875858
assert.ElementsMatch(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches)
876859
baseGitRepo.Close()
877-
defer func() {
878-
testResetRepo(t, baseRepo, "master", masterCommitID)
879-
}()
880860

881861
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
882862
State: commitstatus.CommitStatusSuccess,
@@ -897,18 +877,17 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
897877
func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
898878
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
899879
// create a pull request
900-
session := loginUser(t, "user1")
901-
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
902-
forkedName := "repo1-2"
903-
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
904-
defer func() {
905-
testDeleteRepository(t, session, "user1", forkedName)
906-
}()
907-
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
908-
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
880+
baseUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
881+
baseSession := loginUser(t, "user2")
882+
forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
883+
forkSession := loginUser(t, "user5")
884+
forkedName := "repo1-fork"
885+
testRepoFork(t, forkSession, "user2", "repo1", forkUser.Name, forkedName, "")
886+
testEditFile(t, forkSession, forkUser.Name, forkedName, "master", "README.md", "Hello, World (Edited)\n")
887+
testPullCreate(t, forkSession, forkUser.Name, forkedName, false, "master", "master", "Indexer notifier test pull")
909888

910889
baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
911-
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName})
890+
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: forkUser.Name, Name: forkedName})
912891
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
913892
BaseRepoID: baseRepo.ID,
914893
BaseBranch: "master",
@@ -924,15 +903,15 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
924903
"status_check_contexts": "gitea/actions",
925904
"required_approvals": "1",
926905
})
927-
session.MakeRequest(t, req, http.StatusSeeOther)
906+
baseSession.MakeRequest(t, req, http.StatusSeeOther)
928907

929908
// first time insert automerge record, return true
930-
scheduled, err := automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false)
909+
scheduled, err := automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false)
931910
assert.NoError(t, err)
932911
assert.True(t, scheduled)
933912

934913
// second time insert automerge record, return false because it does exist
935-
scheduled, err = automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false)
914+
scheduled, err = automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false)
936915
assert.Error(t, err)
937916
assert.False(t, scheduled)
938917

@@ -946,14 +925,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
946925
assert.NoError(t, err)
947926
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
948927
assert.NoError(t, err)
949-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
950-
assert.NoError(t, err)
951928
baseGitRepo.Close()
952-
defer func() {
953-
testResetRepo(t, baseRepo, "master", masterCommitID)
954-
}()
955929

956-
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
930+
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, baseUser, sha, &git_model.CommitStatus{
957931
State: commitstatus.CommitStatusSuccess,
958932
TargetURL: "https://gitea.com",
959933
Context: "gitea/actions",
@@ -968,16 +942,13 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
968942
assert.Empty(t, pr.MergedCommitID)
969943

970944
// approve the PR from non-author
971-
approveSession := loginUser(t, "user2")
972-
testSubmitReview(t, approveSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK)
973-
974-
time.Sleep(2 * time.Second)
945+
testSubmitReview(t, baseSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK)
975946

976-
// reload pr again
977-
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
978-
assert.True(t, pr.HasMerged)
947+
assert.Eventually(t, func() bool {
948+
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
949+
return pr.HasMerged
950+
}, 2*time.Second, 100*time.Millisecond)
979951
assert.NotEmpty(t, pr.MergedCommitID)
980-
981952
unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID})
982953
})
983954
}
@@ -1035,7 +1006,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
10351006
HeadBranch: "user2/test/head2",
10361007
})
10371008

1038-
session := loginUser(t, "user1")
1009+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
10391010
// Change master branch to protected
10401011
req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{
10411012
"rule_name": "master",
@@ -1067,13 +1038,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
10671038
assert.NoError(t, err)
10681039
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
10691040
assert.NoError(t, err)
1070-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
1071-
assert.NoError(t, err)
10721041
baseGitRepo.Close()
1073-
defer func() {
1074-
testResetRepo(t, baseRepo, "master", masterCommitID)
1075-
}()
1076-
10771042
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
10781043
State: commitstatus.CommitStatusSuccess,
10791044
TargetURL: "https://gitea.com",
@@ -1087,7 +1052,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
10871052
assert.Empty(t, pr.MergedCommitID)
10881053

10891054
// approve the PR from non-author
1090-
approveSession := loginUser(t, "user1")
1055+
approveSession := loginUser(t, "user1") // FIXME: don't use admin user for testing
10911056
testSubmitReview(t, approveSession, "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK)
10921057

10931058
// reload pr again
@@ -1102,11 +1067,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
11021067
func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
11031068
onGiteaRun(t, func(t *testing.T, u *url.URL) {
11041069
// create a pull request
1105-
session := loginUser(t, "user1")
1070+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
11061071
forkedName := "repo1-1"
11071072
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
1108-
defer testDeleteRepository(t, session, "user1", forkedName)
1109-
11101073
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
11111074
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
11121075

@@ -1144,7 +1107,7 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
11441107

11451108
func TestPullSquashMergeEmpty(t *testing.T) {
11461109
onGiteaRun(t, func(t *testing.T, u *url.URL) {
1147-
session := loginUser(t, "user1")
1110+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
11481111
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "pr-squash-empty", "README.md", "Hello, World (Edited)\n")
11491112
resp := testPullCreate(t, session, "user2", "repo1", false, "master", "pr-squash-empty", "This is a pull title")
11501113

0 commit comments

Comments
 (0)