Merge pull request 'fix(hook): repo admins are wrongly denied the right to force merge' (#3976) from earl-warren/forgejo:wip-admin-protection into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3976
Reviewed-by: Victoria <efertone@noreply.codeberg.org>

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

routers/private/hook_pre_receive.go

@@ -397,9 +397,14 @@ 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) {
+		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),
@@ -411,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())

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 adminSkipProtectionCheck {
-				if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
+				if doer.IsAdmin {
-					log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
+					err = nil // instance admin can skip the check, so clear the error
-					return errCheckAdmin
+				} else if !pb.ApplyToAdmins {
-				} else if isRepoAdmin {
+					if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
-					err = nil // repo admin can skip the check, so clear the error
+						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
+					}
 				}
 			}
 

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

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

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

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

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

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

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)
+		t.Helper()
-
+		doAPIMergePullRequestForm(t, ctx, owner, repo, index, &forms.MergePullRequestForm{
-		var req *RequestWrapper
+			MergeMessageField: "doAPIMergePullRequest Merge",
-		var resp *httptest.ResponseRecorder
+			Do:                string(repo_model.MergeStyleMerge),
-
+		})
-		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)
-		}
 	}
 }
 
+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)

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,20 +177,15 @@ 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)
 	}
 }
 
-func doGitMerge(dstPath string, args ...string) func(*testing.T) {
-	return func(t *testing.T) {
-		_, _, 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()
 		_, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("pull").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath})
 		assert.NoError(t, err)
 	}

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,63 +361,86 @@ 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", "", "unprotected-file-*"))
+		t.Run("FailToPushToProtectedBranch", func(t *testing.T) {
-		t.Run("GenerateCommit", func(t *testing.T) {
+			t.Run("ProtectProtectedBranch", doProtectBranch(ctx, "protected"))
-			_, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "unprotected-file-")
+			t.Run("Create modified-protected-branch", doGitCheckoutBranch(dstPath, "-b", "modified-protected-branch", "protected"))
-			assert.NoError(t, err)
+			t.Run("GenerateCommit", func(t *testing.T) {
-		})
+				_, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-")
-		t.Run("PushUnprotectedFilesToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected"))
+				assert.NoError(t, err)
+			})
 
-		t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username, ""))
+			doGitPushTestRepositoryFail(dstPath, "origin", "modified-protected-branch:protected")(t)
-
+		})
-		t.Run("CheckoutMaster", doGitCheckoutBranch(dstPath, "master"))
+
-		t.Run("CreateBranchForced", doGitCreateBranch(dstPath, "toforce"))
+		t.Run("PushToUnprotectedBranch", doGitPushTestRepository(dstPath, "origin", "modified-protected-branch:unprotected"))
-		t.Run("GenerateCommit", func(t *testing.T) {
+
-			_, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-")
+		t.Run("FailToPushProtectedFilesToProtectedBranch", func(t *testing.T) {
-			assert.NoError(t, err)
+			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{
+				"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)
+		})
+
+		user, err := user_model.GetUserByName(db.DefaultContext, baseCtx.Username)
+		assert.NoError(t, err)
+		t.Run("WhitelistUsers", doProtectBranch(ctx, "protected", parameterProtectBranch{
+			"enable_push":      "whitelist",
+			"enable_whitelist": "on",
+			"whitelist_users":  strconv.FormatInt(user.ID, 10),
+		}))
+
+		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"))
 	}
 }
 
-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 +449,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 == "" {
+		parameter := parameterProtectBranch{
-			// Change branch to protected
+			"_csrf":     csrf,
-			req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{
+			"rule_id":   strconv.FormatInt(rule.ID, 10),
-				"_csrf":                     csrf,
+			"rule_name": branch,
-				"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)
 		}
+		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)

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"
@@ -45,7 +46,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 +68,22 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
 
 	options := map[string]string{
 		"_csrf": htmlDoc.GetCSRF(),
-		"do":    string(mergeStyle),
 	}
-
+	for k, v := range addOptions {
-	if deleteBranch {
+		options[k] = v
-		options["delete_branch_after_merge"] = "on"
 	}
 
 	req = NewRequestWithValues(t, "POST", link, options)
-	resp = session.MakeRequest(t, req, http.StatusOK)
+	resp = session.MakeRequest(t, req, expectedCode)
 
-	respJSON := struct {
+	if expectedCode == http.StatusOK {
-		Redirect string
+		respJSON := struct {
-	}{}
+			Redirect string
-	DecodeJSON(t, resp, &respJSON)
+		}{}
+		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
 }
@@ -683,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