From 4c2ed3c35d52bb897f143e4c0f3f0fd2ae17289a 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. --- 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 70aa294cc18c60982f7c5afe7338f269d8165f61 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. --- 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 16f6ea04f8..53c6e29359 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -368,7 +368,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) @@ -395,14 +395,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")) @@ -417,7 +425,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}) @@ -426,30 +436,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 49aea9879bf9cda05ff6d8e41169b10f21b7867a 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. --- .../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 20591d966e48ae7dfc146398952b89f8b6a3f897 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 --- 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 1d86d9e106..05807cc242 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -45,7 +45,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) @@ -54,22 +67,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 0d8478b82e0a4d24102a582526e4028ba4329d2a 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 --- 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 53c6e29359..af14c0f288 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -86,7 +86,7 @@ func testGit(t *testing.T, u *url.URL) { t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head")) t.Run("InternalReferences", doInternalReferences(&httpContext, dstPath)) - t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath)) + t.Run("BranchProtect", 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) { @@ -130,7 +130,7 @@ func testGit(t *testing.T, u *url.URL) { t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2")) t.Run("InternalReferences", doInternalReferences(&sshContext, dstPath)) - t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath)) + t.Run("BranchProtect", doBranchProtect(&sshContext, dstPath)) t.Run("MergeFork", func(t *testing.T) { defer tests.PrintCurrentTest(t)() t.Run("CreatePRAndMerge", doMergeFork(sshContext, forkedUserCtx, "master", sshContext.Username+":master")) @@ -361,67 +361,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 e0eba21ab7d05a29290b4a7b53908adf79b2f2f9 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. --- 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 af14c0f288..b7469a04cf 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -382,6 +382,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 793421bf5990d1b28d79ea78d310d8a6e15bf562 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. --- .../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 05807cc242..93bd89afdb 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -37,6 +37,7 @@ import ( "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/services/automerge" + "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/pull" commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" files_service "code.gitea.io/gitea/services/repository/files" @@ -696,6 +697,152 @@ func testResetRepo(t *testing.T, repoPath, branch, commitID string) { assert.EqualValues(t, commitID, id) } +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) + }) + } + } + }) +} + func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { // create a pull request From 05f0007437d507e1445fd616594c048e5b9908d8 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. --- 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 f06f6071e9..97568b3f65 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -397,6 +397,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 09f3518069addd51fdf5bb3a7181b70f49f2699b 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 --- 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 97568b3f65..d12a762db6 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -404,7 +404,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), @@ -416,7 +416,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 765f7580cb..2d91ed07f5 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -119,12 +119,16 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc // * 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 + } } }