From b4d792d2a21ca0315739df82dda2fbffed99c344 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 13:57:35 +0200 Subject: [PATCH 1/9] test(integration): add t.Helper() to reduce stack polution Without the a testify stack is likely to not show the relevant test. (cherry picked from commit 4c2ed3c35d52bb897f143e4c0f3f0fd2ae17289a) --- tests/integration/git_helper_for_declarative_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/git_helper_for_declarative_test.go b/tests/integration/git_helper_for_declarative_test.go index ff06dab07a..3c05932955 100644 --- a/tests/integration/git_helper_for_declarative_test.go +++ b/tests/integration/git_helper_for_declarative_test.go @@ -89,6 +89,7 @@ func onGiteaRun[T testing.TB](t T, callback func(T, *url.URL)) { func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { return func(t *testing.T) { + t.Helper() assert.NoError(t, git.CloneWithArgs(context.Background(), git.AllowLFSFiltersArgs(), u.String(), dstLocalPath, git.CloneRepoOptions{})) exist, err := util.IsExist(filepath.Join(dstLocalPath, "README.md")) assert.NoError(t, err) @@ -98,6 +99,7 @@ func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { func doPartialGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { return func(t *testing.T) { + t.Helper() assert.NoError(t, git.CloneWithArgs(context.Background(), git.AllowLFSFiltersArgs(), u.String(), dstLocalPath, git.CloneRepoOptions{ Filter: "blob:none", })) @@ -109,6 +111,7 @@ func doPartialGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { func doGitCloneFail(u *url.URL) func(*testing.T) { return func(t *testing.T) { + t.Helper() tmpDir := t.TempDir() assert.Error(t, git.Clone(git.DefaultContext, u.String(), tmpDir, git.CloneRepoOptions{})) exist, err := util.IsExist(filepath.Join(tmpDir, "README.md")) @@ -119,6 +122,7 @@ func doGitCloneFail(u *url.URL) func(*testing.T) { func doGitInitTestRepository(dstPath string, objectFormat git.ObjectFormat) func(*testing.T) { return func(t *testing.T) { + t.Helper() // Init repository in dstPath assert.NoError(t, git.InitRepository(git.DefaultContext, dstPath, false, objectFormat.Name())) // forcibly set default branch to master @@ -141,6 +145,7 @@ func doGitInitTestRepository(dstPath string, objectFormat git.ObjectFormat) func func doGitAddRemote(dstPath, remoteName string, u *url.URL) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommand(git.DefaultContext, "remote", "add").AddDynamicArguments(remoteName, u.String()).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } @@ -156,6 +161,7 @@ func doGitPushTestRepository(dstPath string, args ...string) func(*testing.T) { func doGitPushTestRepositoryFail(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommand(git.DefaultContext, "push").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.Error(t, err) } @@ -163,6 +169,7 @@ func doGitPushTestRepositoryFail(dstPath string, args ...string) func(*testing.T func doGitCreateBranch(dstPath, branch string) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommand(git.DefaultContext, "checkout", "-b").AddDynamicArguments(branch).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } @@ -170,6 +177,7 @@ func doGitCreateBranch(dstPath, branch string) func(*testing.T) { func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("checkout").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } @@ -177,6 +185,7 @@ func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) { func doGitMerge(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommand(git.DefaultContext, "merge").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } @@ -184,6 +193,7 @@ func doGitMerge(dstPath string, args ...string) func(*testing.T) { func doGitPull(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { + t.Helper() _, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("pull").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } From 68d803aae46d12917b4448018e7ce8f6c5344442 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:35:01 +0200 Subject: [PATCH 2/9] test(integration): refactor doProtectBranch explicitly specify the parameters instead of providing them as arguments so the caller has a more fine grain control over them. (cherry picked from commit 70aa294cc18c60982f7c5afe7338f269d8165f61) --- tests/integration/git_test.go | 54 +++++++++++++++++------------------ 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 98a2636c53..349a028338 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -367,7 +367,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes t.Run("PushProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) ctx := NewAPITestContext(t, baseCtx.Username, baseCtx.Reponame, auth_model.AccessTokenScopeWriteRepository) - t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected", "", "")) + t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected")) t.Run("GenerateCommit", func(t *testing.T) { _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") assert.NoError(t, err) @@ -394,14 +394,22 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes t.Run("MergePR", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr.Index)) t.Run("PullProtected", doGitPull(dstPath, "origin", "protected")) - t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", "", "unprotected-file-*")) + t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", parameterProtectBranch{ + "unprotected_file_patterns": "unprotected-file-*", + })) t.Run("GenerateCommit", func(t *testing.T) { _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "unprotected-file-") assert.NoError(t, err) }) t.Run("PushUnprotectedFilesToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) - t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username, "")) + user, err := user_model.GetUserByName(db.DefaultContext, baseCtx.Username) + assert.NoError(t, err) + t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", parameterProtectBranch{ + "enable_push": "whitelist", + "enable_whitelist": "on", + "whitelist_users": strconv.FormatInt(user.ID, 10), + })) t.Run("CheckoutMaster", doGitCheckoutBranch(dstPath, "master")) t.Run("CreateBranchForced", doGitCreateBranch(dstPath, "toforce")) @@ -416,7 +424,9 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes } } -func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFilePatterns string) func(t *testing.T) { +type parameterProtectBranch map[string]string + +func doProtectBranch(ctx APITestContext, branch string, addParameter ...parameterProtectBranch) func(t *testing.T) { // We are going to just use the owner to set the protection. return func(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: ctx.Reponame, OwnerName: ctx.Username}) @@ -425,30 +435,20 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFil csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame))) - if userToWhitelist == "" { - // Change branch to protected - req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{ - "_csrf": csrf, - "rule_id": strconv.FormatInt(rule.ID, 10), - "rule_name": branch, - "unprotected_file_patterns": unprotectedFilePatterns, - }) - ctx.Session.MakeRequest(t, req, http.StatusSeeOther) - } else { - user, err := user_model.GetUserByName(db.DefaultContext, userToWhitelist) - assert.NoError(t, err) - // Change branch to protected - req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{ - "_csrf": csrf, - "rule_name": branch, - "rule_id": strconv.FormatInt(rule.ID, 10), - "enable_push": "whitelist", - "enable_whitelist": "on", - "whitelist_users": strconv.FormatInt(user.ID, 10), - "unprotected_file_patterns": unprotectedFilePatterns, - }) - ctx.Session.MakeRequest(t, req, http.StatusSeeOther) + parameter := parameterProtectBranch{ + "_csrf": csrf, + "rule_id": strconv.FormatInt(rule.ID, 10), + "rule_name": branch, } + if len(addParameter) > 0 { + for k, v := range addParameter[0] { + parameter[k] = v + } + } + + // Change branch to protected + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), parameter) + ctx.Session.MakeRequest(t, req, http.StatusSeeOther) // Check if master branch has been locked successfully flashCookie := ctx.Session.GetCookie(gitea_context.CookieNameFlash) assert.NotNil(t, flashCookie) From 9cd730a063310e90319fb1de5f525c31193e4a2c Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:37:35 +0200 Subject: [PATCH 3/9] test(integration): refactor doAPIMergePullRequest * http.StatusMethodNotAllowed can be expected: only retry if the error message is "Please try again later" * split into doAPIMergePullRequestForm which can be called directly if the caller wants to specify extra parameters. (cherry picked from commit 49aea9879bf9cda05ff6d8e41169b10f21b7867a) --- .../api_helper_for_declarative_test.go | 74 +++++++++++-------- .../git_helper_for_declarative_test.go | 8 -- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/tests/integration/api_helper_for_declarative_test.go b/tests/integration/api_helper_for_declarative_test.go index 3e54e2fe3f..1aceda8241 100644 --- a/tests/integration/api_helper_for_declarative_test.go +++ b/tests/integration/api_helper_for_declarative_test.go @@ -257,41 +257,51 @@ func doAPIGetPullRequest(ctx APITestContext, owner, repo string, index int64) fu func doAPIMergePullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) { return func(t *testing.T) { - urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner, repo, index) - - var req *RequestWrapper - var resp *httptest.ResponseRecorder - - for i := 0; i < 6; i++ { - req = NewRequestWithJSON(t, http.MethodPost, urlStr, &forms.MergePullRequestForm{ - MergeMessageField: "doAPIMergePullRequest Merge", - Do: string(repo_model.MergeStyleMerge), - }).AddTokenAuth(ctx.Token) - - resp = ctx.Session.MakeRequest(t, req, NoExpectedStatus) - - if resp.Code != http.StatusMethodNotAllowed { - break - } - err := api.APIError{} - DecodeJSON(t, resp, &err) - assert.EqualValues(t, "Please try again later", err.Message) - queue.GetManager().FlushAll(context.Background(), 5*time.Second) - <-time.After(1 * time.Second) - } - - expected := ctx.ExpectedCode - if expected == 0 { - expected = http.StatusOK - } - - if !assert.EqualValues(t, expected, resp.Code, - "Request: %s %s", req.Method, req.URL.String()) { - logUnexpectedResponse(t, resp) - } + t.Helper() + doAPIMergePullRequestForm(t, ctx, owner, repo, index, &forms.MergePullRequestForm{ + MergeMessageField: "doAPIMergePullRequest Merge", + Do: string(repo_model.MergeStyleMerge), + }) } } +func doAPIMergePullRequestForm(t *testing.T, ctx APITestContext, owner, repo string, index int64, merge *forms.MergePullRequestForm) *httptest.ResponseRecorder { + t.Helper() + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner, repo, index) + + var req *RequestWrapper + var resp *httptest.ResponseRecorder + + for i := 0; i < 6; i++ { + req = NewRequestWithJSON(t, http.MethodPost, urlStr, merge).AddTokenAuth(ctx.Token) + + resp = ctx.Session.MakeRequest(t, req, NoExpectedStatus) + + if resp.Code != http.StatusMethodNotAllowed { + break + } + err := api.APIError{} + DecodeJSON(t, resp, &err) + if err.Message != "Please try again later" { + break + } + queue.GetManager().FlushAll(context.Background(), 5*time.Second) + <-time.After(1 * time.Second) + } + + expected := ctx.ExpectedCode + if expected == 0 { + expected = http.StatusOK + } + + if !assert.EqualValues(t, expected, resp.Code, + "Request: %s %s", req.Method, req.URL.String()) { + logUnexpectedResponse(t, resp) + } + + return resp +} + func doAPIManuallyMergePullRequest(ctx APITestContext, owner, repo, commitID string, index int64) func(*testing.T) { return func(t *testing.T) { urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner, repo, index) diff --git a/tests/integration/git_helper_for_declarative_test.go b/tests/integration/git_helper_for_declarative_test.go index 3c05932955..e9df1d70a4 100644 --- a/tests/integration/git_helper_for_declarative_test.go +++ b/tests/integration/git_helper_for_declarative_test.go @@ -183,14 +183,6 @@ func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) { } } -func doGitMerge(dstPath string, args ...string) func(*testing.T) { - return func(t *testing.T) { - t.Helper() - _, _, err := git.NewCommand(git.DefaultContext, "merge").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) - assert.NoError(t, err) - } -} - func doGitPull(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { t.Helper() From 9b17f6fd24f209271f602c5f0e2e99f3a3408a5e Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 22:18:17 +0200 Subject: [PATCH 4/9] test(integration): refactor testPullMerge * split into testPullMergeForm which can be called directly if the caller wants to specify extra parameters. * testPullMergeForm can expect something different than StatusOK (cherry picked from commit 20591d966e48ae7dfc146398952b89f8b6a3f897) --- tests/integration/pull_merge_test.go | 33 +++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 6dddb84bcd..0c9e85cd24 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -39,7 +39,20 @@ import ( "github.com/stretchr/testify/assert" ) +type optionsPullMerge map[string]string + func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder { + options := optionsPullMerge{ + "do": string(mergeStyle), + } + if deleteBranch { + options["delete_branch_after_merge"] = "on" + } + + return testPullMergeForm(t, session, http.StatusOK, user, repo, pullnum, options) +} + +func testPullMergeForm(t *testing.T, session *TestSession, expectedCode int, user, repo, pullnum string, addOptions optionsPullMerge) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum)) resp := session.MakeRequest(t, req, http.StatusOK) @@ -48,22 +61,22 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin options := map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "do": string(mergeStyle), } - - if deleteBranch { - options["delete_branch_after_merge"] = "on" + for k, v := range addOptions { + options[k] = v } req = NewRequestWithValues(t, "POST", link, options) - resp = session.MakeRequest(t, req, http.StatusOK) + resp = session.MakeRequest(t, req, expectedCode) - respJSON := struct { - Redirect string - }{} - DecodeJSON(t, resp, &respJSON) + if expectedCode == http.StatusOK { + respJSON := struct { + Redirect string + }{} + DecodeJSON(t, resp, &respJSON) - assert.EqualValues(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect) + assert.EqualValues(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect) + } return resp } From e0cd81392743576432bf3a575f6e38af2441db08 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 14:13:30 +0200 Subject: [PATCH 5/9] test(integration): refactor doBranchProtectPRMerge * group test cases to clarify their purpose * remove pull request branch protection tests, they are redundant with TestPullMergeBranchProtect (cherry picked from commit 0d8478b82e0a4d24102a582526e4028ba4329d2a) Conflicts: tests/integration/git_test.go trivial context conflict --- tests/integration/git_test.go | 91 +++++++++++++++++------------------ 1 file changed, 45 insertions(+), 46 deletions(-) diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 349a028338..e32c8a3298 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -85,7 +85,7 @@ func testGit(t *testing.T, u *url.URL) { t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "master", "test/head")) t.Run("InternalReferences", doInternalReferences(&httpContext, dstPath)) - t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath)) + t.Run("BranchProtectMerge", doBranchProtect(&httpContext, dstPath)) t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath)) t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge")) t.Run("MergeFork", func(t *testing.T) { @@ -127,7 +127,7 @@ func testGit(t *testing.T, u *url.URL) { t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "master", "test/head2")) t.Run("InternalReferences", doInternalReferences(&sshContext, dstPath)) - t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath)) + t.Run("BranchProtectMerge", doBranchProtect(&sshContext, dstPath)) t.Run("MergeFork", func(t *testing.T) { defer tests.PrintCurrentTest(t)() t.Run("CreatePRAndMerge", doMergeFork(sshContext, forkedUserCtx, "master", sshContext.Username+":master")) @@ -360,67 +360,66 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin return filepath.Base(tmpFile.Name()), err } -func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *testing.T) { +func doBranchProtect(baseCtx *APITestContext, dstPath string) func(t *testing.T) { return func(t *testing.T) { defer tests.PrintCurrentTest(t)() t.Run("CreateBranchProtected", doGitCreateBranch(dstPath, "protected")) t.Run("PushProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) ctx := NewAPITestContext(t, baseCtx.Username, baseCtx.Reponame, auth_model.AccessTokenScopeWriteRepository) - t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected")) - t.Run("GenerateCommit", func(t *testing.T) { - _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") - assert.NoError(t, err) - }) - t.Run("FailToPushToProtectedBranch", doGitPushTestRepositoryFail(dstPath, "origin", "protected")) - t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "protected:unprotected")) - var pr api.PullRequest - var err error - t.Run("CreatePullRequest", func(t *testing.T) { - pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, "protected", "unprotected")(t) - assert.NoError(t, err) - }) - t.Run("GenerateCommit", func(t *testing.T) { - _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") - assert.NoError(t, err) - }) - t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "protected:unprotected-2")) - var pr2 api.PullRequest - t.Run("CreatePullRequest", func(t *testing.T) { - pr2, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, "unprotected", "unprotected-2")(t) - assert.NoError(t, err) - }) - t.Run("MergePR2", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr2.Index)) - t.Run("MergePR", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr.Index)) - t.Run("PullProtected", doGitPull(dstPath, "origin", "protected")) - t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", parameterProtectBranch{ - "unprotected_file_patterns": "unprotected-file-*", - })) - t.Run("GenerateCommit", func(t *testing.T) { - _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "unprotected-file-") - assert.NoError(t, err) + t.Run("FailToPushToProtectedBranch", func(t *testing.T) { + t.Run("ProtectProtectedBranch", doProtectBranch(ctx, "protected")) + t.Run("Create modified-protected-branch", doGitCheckoutBranch(dstPath, "-b", "modified-protected-branch", "protected")) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") + assert.NoError(t, err) + }) + + doGitPushTestRepositoryFail(dstPath, "origin", "modified-protected-branch:protected")(t) + }) + + t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "modified-protected-branch:unprotected")) + + t.Run("PushUnprotectedFilesToProtectedBranch", func(t *testing.T) { + t.Run("Create modified-unprotected-file-protected-branch", doGitCheckoutBranch(dstPath, "-b", "modified-unprotected-file-protected-branch", "protected")) + t.Run("UnprotectedFilePaths", doProtectBranch(ctx, "protected", parameterProtectBranch{ + "unprotected_file_patterns": "unprotected-file-*", + })) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "unprotected-file-") + assert.NoError(t, err) + }) + doGitPushTestRepository(dstPath, "origin", "modified-unprotected-file-protected-branch:protected")(t) + doGitCheckoutBranch(dstPath, "protected")(t) + doGitPull(dstPath, "origin", "protected")(t) }) - t.Run("PushUnprotectedFilesToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) user, err := user_model.GetUserByName(db.DefaultContext, baseCtx.Username) assert.NoError(t, err) - t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", parameterProtectBranch{ + t.Run("WhitelistUsers", doProtectBranch(ctx, "protected", parameterProtectBranch{ "enable_push": "whitelist", "enable_whitelist": "on", "whitelist_users": strconv.FormatInt(user.ID, 10), })) - t.Run("CheckoutMaster", doGitCheckoutBranch(dstPath, "master")) - t.Run("CreateBranchForced", doGitCreateBranch(dstPath, "toforce")) - t.Run("GenerateCommit", func(t *testing.T) { - _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") - assert.NoError(t, err) + t.Run("WhitelistedUserFailToForcePushToProtectedBranch", func(t *testing.T) { + t.Run("Create toforce", doGitCheckoutBranch(dstPath, "-b", "toforce", "master")) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") + assert.NoError(t, err) + }) + doGitPushTestRepositoryFail(dstPath, "-f", "origin", "toforce:protected")(t) + }) + + t.Run("WhitelistedUserPushToProtectedBranch", func(t *testing.T) { + t.Run("Create topush", doGitCheckoutBranch(dstPath, "-b", "topush", "protected")) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") + assert.NoError(t, err) + }) + doGitPushTestRepository(dstPath, "origin", "topush:protected")(t) }) - t.Run("FailToForcePushToProtectedBranch", doGitPushTestRepositoryFail(dstPath, "-f", "origin", "toforce:protected")) - t.Run("MergeProtectedToToforce", doGitMerge(dstPath, "protected")) - t.Run("PushToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "toforce:protected")) - t.Run("CheckoutMasterAgain", doGitCheckoutBranch(dstPath, "master")) } } From 6827a4a66949546cc23ecfcf885a402e8bd8fc00 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 22:28:42 +0200 Subject: [PATCH 6/9] test(integration): add protected file to doBranchProtect A protected file pushed to a protected branch branch is not allowed. (cherry picked from commit e0eba21ab7d05a29290b4a7b53908adf79b2f2f9) --- tests/integration/git_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index e32c8a3298..2dba9e277b 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -381,6 +381,20 @@ func doBranchProtect(baseCtx *APITestContext, dstPath string) func(t *testing.T) t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "modified-protected-branch:unprotected")) + t.Run("FailToPushProtectedFilesToProtectedBranch", func(t *testing.T) { + t.Run("Create modified-protected-file-protected-branch", doGitCheckoutBranch(dstPath, "-b", "modified-protected-file-protected-branch", "protected")) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "protected-file-") + assert.NoError(t, err) + }) + + t.Run("ProtectedFilePathsApplyToAdmins", doProtectBranch(ctx, "protected")) + doGitPushTestRepositoryFail(dstPath, "origin", "modified-protected-file-protected-branch:protected")(t) + + doGitCheckoutBranch(dstPath, "protected")(t) + doGitPull(dstPath, "origin", "protected")(t) + }) + t.Run("PushUnprotectedFilesToProtectedBranch", func(t *testing.T) { t.Run("Create modified-unprotected-file-protected-branch", doGitCheckoutBranch(dstPath, "-b", "modified-unprotected-file-protected-branch", "protected")) t.Run("UnprotectedFilePaths", doProtectBranch(ctx, "protected", parameterProtectBranch{ From bad8e72bcd4e044fe3b05b4fc1a9152ad7aac7a2 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:58:46 +0200 Subject: [PATCH 7/9] tests(integration): add TestPullMergeBranchProtect Verify variations of branch protection that are in play when merging a pull request as: * instance admin * repository admin / owner * user with write permissions on the repository In all cases the result is expected to be the same when merging the pull request via: * API * web Although the implementations are different. (cherry picked from commit 793421bf5990d1b28d79ea78d310d8a6e15bf562) Conflicts: tests/integration/pull_merge_test.go trivial context conflict --- .../user5/repo4.git/hooks/post-receive | 7 + .../repo4.git/hooks/post-receive.d/gitea | 2 + .../user5/repo4.git/hooks/pre-receive | 7 + .../user5/repo4.git/hooks/pre-receive.d/gitea | 2 + .../user5/repo4.git/hooks/update | 7 + .../user5/repo4.git/hooks/update.d/gitea | 2 + tests/integration/pull_merge_test.go | 147 ++++++++++++++++++ 7 files changed, 174 insertions(+) create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive.d/gitea create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive.d/gitea create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/update create mode 100755 tests/gitea-repositories-meta/user5/repo4.git/hooks/update.d/gitea diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive b/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive new file mode 100755 index 0000000000..4b3d452abc --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +ORI_DIR=`pwd` +SHELL_FOLDER=$(cd "$(dirname "$0")";pwd) +cd "$ORI_DIR" +for i in `ls "$SHELL_FOLDER/post-receive.d"`; do + sh "$SHELL_FOLDER/post-receive.d/$i" +done \ No newline at end of file diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive.d/gitea b/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive.d/gitea new file mode 100755 index 0000000000..43a948da3a --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/post-receive.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" post-receive diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive b/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive new file mode 100755 index 0000000000..4127013053 --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +ORI_DIR=`pwd` +SHELL_FOLDER=$(cd "$(dirname "$0")";pwd) +cd "$ORI_DIR" +for i in `ls "$SHELL_FOLDER/pre-receive.d"`; do + sh "$SHELL_FOLDER/pre-receive.d/$i" +done \ No newline at end of file diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive.d/gitea b/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive.d/gitea new file mode 100755 index 0000000000..49d0940636 --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/pre-receive.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" pre-receive diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/update b/tests/gitea-repositories-meta/user5/repo4.git/hooks/update new file mode 100755 index 0000000000..c186fe4a18 --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/update @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +ORI_DIR=`pwd` +SHELL_FOLDER=$(cd "$(dirname "$0")";pwd) +cd "$ORI_DIR" +for i in `ls "$SHELL_FOLDER/update.d"`; do + sh "$SHELL_FOLDER/update.d/$i" $1 $2 $3 +done \ No newline at end of file diff --git a/tests/gitea-repositories-meta/user5/repo4.git/hooks/update.d/gitea b/tests/gitea-repositories-meta/user5/repo4.git/hooks/update.d/gitea new file mode 100755 index 0000000000..38101c2426 --- /dev/null +++ b/tests/gitea-repositories-meta/user5/repo4.git/hooks/update.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" update $1 $2 $3 diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 0c9e85cd24..49f2317b0e 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -32,6 +32,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/pull" files_service "code.gitea.io/gitea/services/repository/files" webhook_service "code.gitea.io/gitea/services/webhook" @@ -674,3 +675,149 @@ func TestPullMergeIndexerNotifier(t *testing.T) { } }) } + +func TestPullMergeBranchProtect(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + admin := "user1" + owner := "user5" + notOwner := "user4" + repo := "repo4" + + dstPath := t.TempDir() + + u.Path = fmt.Sprintf("%s/%s.git", owner, repo) + u.User = url.UserPassword(owner, userPassword) + + t.Run("Clone", doGitClone(dstPath, u)) + + for _, testCase := range []struct { + name string + doer string + expectedCode map[string]int + filename string + protectBranch parameterProtectBranch + }{ + { + name: "SuccessAdminNotEnoughMergeRequiredApprovals", + doer: admin, + expectedCode: map[string]int{"api": http.StatusOK, "web": http.StatusOK}, + filename: "branch-data-file-", + protectBranch: parameterProtectBranch{ + "required_approvals": "1", + "apply_to_admins": "true", + }, + }, + { + name: "FailOwnerProtectedFile", + doer: owner, + expectedCode: map[string]int{"api": http.StatusMethodNotAllowed, "web": http.StatusBadRequest}, + filename: "protected-file-", + protectBranch: parameterProtectBranch{ + "protected_file_patterns": "protected-file-*", + "apply_to_admins": "true", + }, + }, + { + name: "OwnerProtectedFile", + doer: owner, + expectedCode: map[string]int{"api": http.StatusOK, "web": http.StatusOK}, + filename: "protected-file-", + protectBranch: parameterProtectBranch{ + "protected_file_patterns": "protected-file-*", + "apply_to_admins": "false", + }, + }, + { + name: "FailNotOwnerProtectedFile", + doer: notOwner, + expectedCode: map[string]int{"api": http.StatusMethodNotAllowed, "web": http.StatusBadRequest}, + filename: "protected-file-", + protectBranch: parameterProtectBranch{ + "protected_file_patterns": "protected-file-*", + }, + }, + { + name: "FailOwnerNotEnoughMergeRequiredApprovals", + doer: owner, + expectedCode: map[string]int{"api": http.StatusMethodNotAllowed, "web": http.StatusBadRequest}, + filename: "branch-data-file-", + protectBranch: parameterProtectBranch{ + "required_approvals": "1", + "apply_to_admins": "true", + }, + }, + { + name: "SuccessOwnerNotEnoughMergeRequiredApprovals", + doer: owner, + expectedCode: map[string]int{"api": http.StatusOK, "web": http.StatusOK}, + filename: "branch-data-file-", + protectBranch: parameterProtectBranch{ + "required_approvals": "1", + "apply_to_admins": "false", + }, + }, + { + name: "FailNotOwnerNotEnoughMergeRequiredApprovals", + doer: notOwner, + expectedCode: map[string]int{"api": http.StatusMethodNotAllowed, "web": http.StatusBadRequest}, + filename: "branch-data-file-", + protectBranch: parameterProtectBranch{ + "required_approvals": "1", + "apply_to_admins": "false", + }, + }, + { + name: "SuccessNotOwner", + doer: notOwner, + expectedCode: map[string]int{"api": http.StatusOK, "web": http.StatusOK}, + filename: "branch-data-file-", + protectBranch: parameterProtectBranch{ + "required_approvals": "0", + }, + }, + } { + mergeWith := func(t *testing.T, ctx APITestContext, apiOrWeb string, expectedCode int, pr int64) { + switch apiOrWeb { + case "api": + ctx.ExpectedCode = expectedCode + doAPIMergePullRequestForm(t, ctx, owner, repo, pr, + &forms.MergePullRequestForm{ + MergeMessageField: "doAPIMergePullRequest Merge", + Do: string(repo_model.MergeStyleMerge), + ForceMerge: true, + }) + ctx.ExpectedCode = 0 + case "web": + testPullMergeForm(t, ctx.Session, expectedCode, owner, repo, fmt.Sprintf("%d", pr), optionsPullMerge{ + "do": string(repo_model.MergeStyleMerge), + "force_merge": "true", + }) + default: + panic(apiOrWeb) + } + } + for _, withAPIOrWeb := range []string{"api", "web"} { + t.Run(testCase.name+" "+withAPIOrWeb, func(t *testing.T) { + branch := testCase.name + "-" + withAPIOrWeb + unprotected := branch + "-unprotected" + doGitCheckoutBranch(dstPath, "master")(t) + doGitCreateBranch(dstPath, branch)(t) + doGitPushTestRepository(dstPath, "origin", branch)(t) + + ctx := NewAPITestContext(t, owner, repo, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + doProtectBranch(ctx, branch, testCase.protectBranch)(t) + + ctx = NewAPITestContext(t, testCase.doer, "not used", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + ctx.Username = owner + ctx.Reponame = repo + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", testCase.filename) + assert.NoError(t, err) + doGitPushTestRepository(dstPath, "origin", branch+":"+unprotected)(t) + pr, err := doAPICreatePullRequest(ctx, owner, repo, branch, unprotected)(t) + assert.NoError(t, err) + mergeWith(t, ctx, withAPIOrWeb, testCase.expectedCode[withAPIOrWeb], pr.Index) + }) + } + } + }) +} From baec3dc6b9c65c9a46c88da2859ee7a49df53fca Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:41:10 +0200 Subject: [PATCH 8/9] fix(hook): instance admins wrongly restricted by permissions checks This exception existed for both instance admins and repo admins before ApplyToAdmins was introduced in 79b70893601c33a33d8d44eb0421797dfd846a47. It should have been kept for instance admins only because they are not subject to permission checks. (cherry picked from commit 05f0007437d507e1445fd616594c048e5b9908d8) --- routers/private/hook_pre_receive.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index cb356a184a..33f09aa096 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -398,6 +398,11 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r return } + // If we're an admin for the instance, we can ignore checks + if ctx.user.IsAdmin { + return + } + // It's not allowed t overwrite protected files. Unless if the user is an // admin and the protected branch rule doesn't apply to admins. if changedProtectedfiles && (!ctx.user.IsAdmin || protectBranch.ApplyToAdmins) { From 2df082393e8ddf918aa0794702dc90d431abfeb5 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:45:20 +0200 Subject: [PATCH 9/9] fix(hook): repo admins are wrongly denied the right to force merge The right to force merge is uses the wrong predicate and applies to instance admins: ctx.user.IsAdmin It must apply to repository admins and use the following predicate: ctx.userPerm.IsAdmin() This regression is from the ApplyToAdmins implementation in 79b70893601c33a33d8d44eb0421797dfd846a47. Fixes: https://codeberg.org/forgejo/forgejo/issues/3780 (cherry picked from commit 09f3518069addd51fdf5bb3a7181b70f49f2699b) --- release-notes/8.0.0/fix/3976.md | 1 + routers/private/hook_pre_receive.go | 4 ++-- services/pull/check.go | 16 ++++++++++------ 3 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 release-notes/8.0.0/fix/3976.md diff --git a/release-notes/8.0.0/fix/3976.md b/release-notes/8.0.0/fix/3976.md new file mode 100644 index 0000000000..3588f94dfc --- /dev/null +++ b/release-notes/8.0.0/fix/3976.md @@ -0,0 +1 @@ +- repository admins are always denied the right to force merge and instance admins are subject to restrictions to merge that must only apply to repository admins diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 33f09aa096..7821f858a6 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -405,7 +405,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r // It's not allowed t overwrite protected files. Unless if the user is an // admin and the protected branch rule doesn't apply to admins. - if changedProtectedfiles && (!ctx.user.IsAdmin || protectBranch.ApplyToAdmins) { + if changedProtectedfiles && (!ctx.userPerm.IsAdmin() || protectBranch.ApplyToAdmins) { log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath) ctx.JSON(http.StatusForbidden, private.Response{ UserMsg: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath), @@ -417,7 +417,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r if pb, err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil { if models.IsErrDisallowedToMerge(err) { // Allow this if the rule doesn't apply to admins and the user is an admin. - if ctx.user.IsAdmin && !pb.ApplyToAdmins { + if ctx.userPerm.IsAdmin() && !pb.ApplyToAdmins { return } log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", ctx.opts.UserID, branchName, repo, pr.Index, err.Error()) diff --git a/services/pull/check.go b/services/pull/check.go index 9aab3c94f3..3b0eea3ea5 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -119,12 +119,16 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce // * if the doer is admin, they could skip the branch protection check, // if that's allowed by the protected branch rule. - if adminSkipProtectionCheck && !pb.ApplyToAdmins { - if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil { - log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin) - return errCheckAdmin - } else if isRepoAdmin { - err = nil // repo admin can skip the check, so clear the error + if adminSkipProtectionCheck { + if doer.IsAdmin { + err = nil // instance admin can skip the check, so clear the error + } else if !pb.ApplyToAdmins { + if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil { + log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin) + return errCheckAdmin + } else if isRepoAdmin { + err = nil // repo admin can skip the check, so clear the error + } } }