Skip to content

Commit b0e2aa3

Browse files
Enzimewxiaoguanglunny
committed
Fix non-admins unable to automerge PRs from forks (go-gitea#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 0f55eff commit b0e2aa3

2 files changed

Lines changed: 38 additions & 79 deletions

File tree

‎services/automerge/automerge.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -241,9 +241,9 @@ func handlePullRequestAutoMerge(pullID int64, sha string) {
241241
return
242242
}
243243

244-
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer)
244+
perm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer)
245245
if err != nil {
246-
log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err)
246+
log.Error("GetUserRepoPermission %-v: %v", pr.BaseRepo, err)
247247
return
248248
}
249249

‎tests/integration/pull_merge_test.go‎

Lines changed: 36 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"net/url"
1212
"os"
1313
"path"
14-
"path/filepath"
1514
"strconv"
1615
"strings"
1716
"testing"
@@ -95,7 +94,7 @@ func TestPullMerge(t *testing.T) {
9594
assert.NoError(t, err)
9695
hookTasksLenBefore := len(hookTasks)
9796

98-
session := loginUser(t, "user1")
97+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
9998
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
10099
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
101100

@@ -129,7 +128,7 @@ func TestPullRebase(t *testing.T) {
129128
assert.NoError(t, err)
130129
hookTasksLenBefore := len(hookTasks)
131130

132-
session := loginUser(t, "user1")
131+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
133132
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
134133
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
135134

@@ -163,7 +162,7 @@ func TestPullRebaseMerge(t *testing.T) {
163162
assert.NoError(t, err)
164163
hookTasksLenBefore := len(hookTasks)
165164

166-
session := loginUser(t, "user1")
165+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
167166
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
168167
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
169168

@@ -197,7 +196,7 @@ func TestPullSquash(t *testing.T) {
197196
assert.NoError(t, err)
198197
hookTasksLenBefore := len(hookTasks)
199198

200-
session := loginUser(t, "user1")
199+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
201200
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
202201
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
203202
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")
@@ -216,7 +215,7 @@ func TestPullSquash(t *testing.T) {
216215

217216
func TestPullCleanUpAfterMerge(t *testing.T) {
218217
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
219-
session := loginUser(t, "user1")
218+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
220219
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
221220
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n")
222221

@@ -263,7 +262,7 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
263262

264263
func TestCantMergeWorkInProgress(t *testing.T) {
265264
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
266-
session := loginUser(t, "user1")
265+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
267266
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
268267
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
269268

@@ -282,7 +281,7 @@ func TestCantMergeWorkInProgress(t *testing.T) {
282281

283282
func TestCantMergeConflict(t *testing.T) {
284283
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
285-
session := loginUser(t, "user1")
284+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
286285
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
287286
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
288287
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
@@ -328,7 +327,7 @@ func TestCantMergeConflict(t *testing.T) {
328327

329328
func TestCantMergeUnrelated(t *testing.T) {
330329
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
331-
session := loginUser(t, "user1")
330+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
332331
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
333332
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
334333

@@ -423,7 +422,7 @@ func TestCantMergeUnrelated(t *testing.T) {
423422

424423
func TestFastForwardOnlyMerge(t *testing.T) {
425424
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
426-
session := loginUser(t, "user1")
425+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
427426
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
428427
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "update", "README.md", "Hello, World 2\n")
429428

@@ -464,7 +463,7 @@ func TestFastForwardOnlyMerge(t *testing.T) {
464463

465464
func TestCantFastForwardOnlyMergeDiverging(t *testing.T) {
466465
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
467-
session := loginUser(t, "user1")
466+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
468467
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
469468
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "diverging", "README.md", "Hello, World diverged\n")
470469
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World 2\n")
@@ -587,7 +586,7 @@ func TestConflictChecking(t *testing.T) {
587586

588587
func TestPullRetargetChildOnBranchDelete(t *testing.T) {
589588
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
590-
session := loginUser(t, "user1")
589+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
591590
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n")
592591
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
593592
testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullRetargetOnCleanup - base PR)\n(Edited - TestPullRetargetOnCleanup - child PR)")
@@ -621,7 +620,7 @@ func TestPullRetargetChildOnBranchDelete(t *testing.T) {
621620

622621
func TestPullDontRetargetChildOnWrongRepo(t *testing.T) {
623622
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
624-
session := loginUser(t, "user1")
623+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
625624
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
626625
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n")
627626
testEditFileToNewBranch(t, session, "user1", "repo1", "base-pr", "child-pr", "README.md", "Hello, World\n(Edited - TestPullDontRetargetChildOnWrongRepo - base PR)\n(Edited - TestPullDontRetargetChildOnWrongRepo - child PR)")
@@ -680,7 +679,7 @@ func TestPullRequestMergedWithNoPermissionDeleteBranch(t *testing.T) {
680679
func TestPullMergeIndexerNotifier(t *testing.T) {
681680
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
682681
// create a pull request
683-
session := loginUser(t, "user1")
682+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
684683
testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
685684
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
686685
createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull")
@@ -737,31 +736,13 @@ func TestPullMergeIndexerNotifier(t *testing.T) {
737736
})
738737
}
739738

740-
func testResetRepo(t *testing.T, repoPath, branch, commitID string) {
741-
f, err := os.OpenFile(filepath.Join(repoPath, "refs", "heads", branch), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
742-
assert.NoError(t, err)
743-
_, err = f.WriteString(commitID + "\n")
744-
assert.NoError(t, err)
745-
f.Close()
746-
747-
repo, err := git.OpenRepository(t.Context(), repoPath)
748-
assert.NoError(t, err)
749-
defer repo.Close()
750-
id, err := repo.GetBranchCommitID(branch)
751-
assert.NoError(t, err)
752-
assert.Equal(t, commitID, id)
753-
}
754-
755739
func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
756740
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
757741
// create a pull request
758-
session := loginUser(t, "user1")
742+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
759743
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
760744
forkedName := "repo1-1"
761745
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
762-
defer func() {
763-
testDeleteRepository(t, session, "user1", forkedName)
764-
}()
765746
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
766747
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
767748

@@ -818,16 +799,10 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
818799
assert.NoError(t, err)
819800
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
820801
assert.NoError(t, err)
821-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
822-
assert.NoError(t, err)
823-
824802
branches, _, err := baseGitRepo.GetBranchNames(0, 100)
825803
assert.NoError(t, err)
826804
assert.ElementsMatch(t, []string{"sub-home-md-img-check", "home-md-img-check", "pr-to-update", "branch2", "DefaultBranch", "develop", "feature/1", "master"}, branches)
827805
baseGitRepo.Close()
828-
defer func() {
829-
testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID)
830-
}()
831806

832807
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
833808
State: commitstatus.CommitStatusSuccess,
@@ -848,18 +823,17 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) {
848823
func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
849824
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
850825
// create a pull request
851-
session := loginUser(t, "user1")
852-
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
853-
forkedName := "repo1-2"
854-
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
855-
defer func() {
856-
testDeleteRepository(t, session, "user1", forkedName)
857-
}()
858-
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
859-
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
826+
baseUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
827+
baseSession := loginUser(t, "user2")
828+
forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
829+
forkSession := loginUser(t, "user5")
830+
forkedName := "repo1-fork"
831+
testRepoFork(t, forkSession, "user2", "repo1", forkUser.Name, forkedName, "")
832+
testEditFile(t, forkSession, forkUser.Name, forkedName, "master", "README.md", "Hello, World (Edited)\n")
833+
testPullCreate(t, forkSession, forkUser.Name, forkedName, false, "master", "master", "Indexer notifier test pull")
860834

861835
baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
862-
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName})
836+
forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: forkUser.Name, Name: forkedName})
863837
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
864838
BaseRepoID: baseRepo.ID,
865839
BaseBranch: "master",
@@ -868,7 +842,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
868842
})
869843

870844
// add protected branch for commit status
871-
csrf := GetUserCSRFToken(t, session)
845+
csrf := GetUserCSRFToken(t, baseSession)
872846
// Change master branch to protected
873847
req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{
874848
"_csrf": csrf,
@@ -878,15 +852,15 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
878852
"status_check_contexts": "gitea/actions",
879853
"required_approvals": "1",
880854
})
881-
session.MakeRequest(t, req, http.StatusSeeOther)
855+
baseSession.MakeRequest(t, req, http.StatusSeeOther)
882856

883857
// first time insert automerge record, return true
884-
scheduled, err := automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false)
858+
scheduled, err := automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false)
885859
assert.NoError(t, err)
886860
assert.True(t, scheduled)
887861

888862
// second time insert automerge record, return false because it does exist
889-
scheduled, err = automerge.ScheduleAutoMerge(t.Context(), user1, pr, repo_model.MergeStyleMerge, "auto merge test", false)
863+
scheduled, err = automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false)
890864
assert.Error(t, err)
891865
assert.False(t, scheduled)
892866

@@ -900,14 +874,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
900874
assert.NoError(t, err)
901875
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
902876
assert.NoError(t, err)
903-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
904-
assert.NoError(t, err)
905877
baseGitRepo.Close()
906-
defer func() {
907-
testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID)
908-
}()
909878

910-
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
879+
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, baseUser, sha, &git_model.CommitStatus{
911880
State: commitstatus.CommitStatusSuccess,
912881
TargetURL: "https://gitea.com",
913882
Context: "gitea/actions",
@@ -928,13 +897,11 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) {
928897
htmlDoc := NewHTMLParser(t, resp.Body)
929898
testSubmitReview(t, approveSession, htmlDoc.GetCSRF(), "user2", "repo1", strconv.Itoa(int(pr.Index)), sha, "approve", http.StatusOK)
930899

931-
time.Sleep(2 * time.Second)
932-
933-
// reload pr again
934-
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
935-
assert.True(t, pr.HasMerged)
900+
assert.Eventually(t, func() bool {
901+
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
902+
return pr.HasMerged
903+
}, 2*time.Second, 100*time.Millisecond)
936904
assert.NotEmpty(t, pr.MergedCommitID)
937-
938905
unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{PullID: pr.ID})
939906
})
940907
}
@@ -994,7 +961,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
994961
HeadBranch: "user2/test/head2",
995962
})
996963

997-
session := loginUser(t, "user1")
964+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
998965
// add protected branch for commit status
999966
csrf := GetUserCSRFToken(t, session)
1000967
// Change master branch to protected
@@ -1029,13 +996,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
1029996
assert.NoError(t, err)
1030997
sha, err := baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName())
1031998
assert.NoError(t, err)
1032-
masterCommitID, err := baseGitRepo.GetBranchCommitID("master")
1033-
assert.NoError(t, err)
1034999
baseGitRepo.Close()
1035-
defer func() {
1036-
testResetRepo(t, baseRepo.RepoPath(), "master", masterCommitID)
1037-
}()
1038-
10391000
err = commitstatus_service.CreateCommitStatus(t.Context(), baseRepo, user1, sha, &git_model.CommitStatus{
10401001
State: commitstatus.CommitStatusSuccess,
10411002
TargetURL: "https://gitea.com",
@@ -1049,7 +1010,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
10491010
assert.Empty(t, pr.MergedCommitID)
10501011

10511012
// approve the PR from non-author
1052-
approveSession := loginUser(t, "user1")
1013+
approveSession := loginUser(t, "user1") // FIXME: don't use admin user for testing
10531014
req = NewRequest(t, "GET", fmt.Sprintf("/user2/repo1/pulls/%d", pr.Index))
10541015
resp := approveSession.MakeRequest(t, req, http.StatusOK)
10551016
htmlDoc := NewHTMLParser(t, resp.Body)
@@ -1067,11 +1028,9 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing.
10671028
func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
10681029
onGiteaRun(t, func(t *testing.T, u *url.URL) {
10691030
// create a pull request
1070-
session := loginUser(t, "user1")
1031+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
10711032
forkedName := "repo1-1"
10721033
testRepoFork(t, session, "user2", "repo1", "user1", forkedName, "")
1073-
defer testDeleteRepository(t, session, "user1", forkedName)
1074-
10751034
testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
10761035
testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
10771036

@@ -1113,7 +1072,7 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
11131072

11141073
func TestPullSquashMergeEmpty(t *testing.T) {
11151074
onGiteaRun(t, func(t *testing.T, u *url.URL) {
1116-
session := loginUser(t, "user1")
1075+
session := loginUser(t, "user1") // FIXME: don't use admin user for testing
11171076
testEditFileToNewBranch(t, session, "user2", "repo1", "master", "pr-squash-empty", "README.md", "Hello, World (Edited)\n")
11181077
resp := testPullCreate(t, session, "user2", "repo1", false, "master", "pr-squash-empty", "This is a pull title")
11191078

0 commit comments

Comments
 (0)