Fix bug to check whether user can update pull request branch or rebase branch (#36465)

services/pull/update.go

@@ -96,78 +96,105 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
 	return err
 }
 
-// IsUserAllowedToUpdate check if user is allowed to update PR with given permissions and branch protections
-// update PR means send new commits to PR head branch from base branch
-func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, user *user_model.User) (mergeAllowed, rebaseAllowed bool, err error) {
-	if pull.Flow == issues_model.PullRequestFlowAGit {
-		return false, false, nil
-	}
+// isUserAllowedToPushOrForcePushInRepoBranch checks whether user is allowed to push or force push in the given repo and branch
+// it will check both user permission and branch protection rules
+func isUserAllowedToPushOrForcePushInRepoBranch(ctx context.Context, user *user_model.User, repo *repo_model.Repository, branch string) (pushAllowed, forcePushAllowed bool, err error) {
 	if user == nil {
 		return false, false, nil
 	}
-	headRepoPerm, err := access_model.GetUserRepoPermission(ctx, pull.HeadRepo, user)
+
+	// 1. check user push permission on the given repository
+	repoPerm, err := access_model.GetUserRepoPermission(ctx, repo, user)
 	if err != nil {
 		if repo_model.IsErrUnitTypeNotExist(err) {
 			return false, false, nil
 		}
 		return false, false, err
 	}
+	pushAllowed = repoPerm.CanWrite(unit.TypeCode)
+	forcePushAllowed = pushAllowed
 
-	if err := pull.LoadBaseRepo(ctx); err != nil {
+	// 2. check branch protection whether user can push or force push
+	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, branch)
+	if err != nil {
 		return false, false, err
 	}
+	if pb != nil { // override previous results if there is a branch protection rule
+		pb.Repo = repo
+		pushAllowed = pb.CanUserPush(ctx, user)
+		forcePushAllowed = pb.CanUserForcePush(ctx, user)
+	}
+	return pushAllowed, forcePushAllowed, nil
+}
 
-	// 1. check base repository's AllowRebaseUpdate configuration
-	// it is a config in base repo but controls the head (fork) repo's "Update" behavior
-	{
-		prBaseUnit, err := pull.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
-		if repo_model.IsErrUnitTypeNotExist(err) {
-			return false, false, nil // the PR unit is disabled in base repo
-		} else if err != nil {
-			return false, false, fmt.Errorf("get base repo unit: %v", err)
-		}
-		rebaseAllowed = prBaseUnit.PullRequestsConfig().AllowRebaseUpdate
+// IsUserAllowedToUpdate check if user is allowed to update PR with given permissions and branch protections
+// update PR means send new commits to PR head branch from base branch
+func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, user *user_model.User) (pushAllowed, rebaseAllowed bool, err error) {
+	if user == nil {
+		return false, false, nil
+	}
+	if err := pull.LoadBaseRepo(ctx); err != nil {
+		return false, false, err
+	}
+	if err := pull.LoadHeadRepo(ctx); err != nil {
+		return false, false, err
 	}
 
-	// 2. check head branch protection whether rebase is allowed, if pb not found then rebase depends on the above setting
-	{
-		pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.HeadRepoID, pull.HeadBranch)
-		if err != nil {
-			return false, false, err
-		}
-		// If branch protected, disable rebase unless user is whitelisted to force push (which extends regular push)
-		if pb != nil {
-			pb.Repo = pull.HeadRepo
-			rebaseAllowed = rebaseAllowed && pb.CanUserForcePush(ctx, user)
-		}
+	// 1. check whether pull request enabled.
+	prBaseUnit, err := pull.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
+	if repo_model.IsErrUnitTypeNotExist(err) {
+		return false, false, nil // the PR unit is disabled in base repo means no update allowed
+	} else if err != nil {
+		return false, false, fmt.Errorf("get base repo unit: %v", err)
 	}
 
-	// 3. check whether user has write access to head branch
-	baseRepoPerm, err := access_model.GetUserRepoPermission(ctx, pull.BaseRepo, user)
-	if err != nil {
-		return false, false, err
+	// 2. only support Github style pull request
+	if pull.Flow == issues_model.PullRequestFlowAGit {
+		return false, false, nil
 	}
 
-	mergeAllowed, err = isUserAllowedToMergeInRepoBranch(ctx, pull.HeadRepoID, pull.HeadBranch, headRepoPerm, user)
+	// 3. check user push permission on head repository
+	pushAllowed, rebaseAllowed, err = isUserAllowedToPushOrForcePushInRepoBranch(ctx, user, pull.HeadRepo, pull.HeadBranch)
 	if err != nil {
 		return false, false, err
 	}
 
-	// 4. if the pull creator allows maintainer to edit, it means the write permissions of the head branch has been
-	// granted to the user with write permission of the base repository
-	if pull.AllowMaintainerEdit {
-		mergeAllowedMaintainer, err := isUserAllowedToMergeInRepoBranch(ctx, pull.BaseRepoID, pull.BaseBranch, baseRepoPerm, user)
+	// 4. if the pull creator allows maintainer to edit, we need to check whether
+	// user is a maintainer (has permission to merge into base branch) and inherit pull request poster's permission
+	if pull.AllowMaintainerEdit && (!pushAllowed || !rebaseAllowed) {
+		baseRepoPerm, err := access_model.GetUserRepoPermission(ctx, pull.BaseRepo, user)
 		if err != nil {
 			return false, false, err
 		}
-
-		mergeAllowed = mergeAllowed || mergeAllowedMaintainer
+		userAllowedToMergePR, err := isUserAllowedToMergeInRepoBranch(ctx, pull.BaseRepoID, pull.BaseBranch, baseRepoPerm, user)
+		if err != nil {
+			return false, false, err
+		}
+		if userAllowedToMergePR {
+			// the user is maintainer (can merge PR), and this PR is allowed to be edited by maintainers,
+			// then the user should inherit the PR poster's push/rebase permission for the head branch
+			if err := pull.LoadIssue(ctx); err != nil {
+				return false, false, err
+			}
+			if err := pull.Issue.LoadPoster(ctx); err != nil {
+				return false, false, err
+			}
+			posterPushAllowed, posterRebaseAllowed, err := isUserAllowedToPushOrForcePushInRepoBranch(ctx, pull.Issue.Poster, pull.HeadRepo, pull.HeadBranch)
+			if err != nil {
+				return false, false, err
+			}
+			if !pushAllowed {
+				pushAllowed = posterPushAllowed
+			}
+			if !rebaseAllowed {
+				rebaseAllowed = posterRebaseAllowed
+			}
+		}
 	}
 
-	// if merge is not allowed, rebase is also not allowed
-	rebaseAllowed = rebaseAllowed && mergeAllowed
-
-	return mergeAllowed, rebaseAllowed, nil
+	// 5. check base repository's AllowRebaseUpdate configuration
+	// it is a config in base repo but controls the head (fork) repo's "Update" behavior
+	return pushAllowed, rebaseAllowed && prBaseUnit.PullRequestsConfig().AllowRebaseUpdate, nil
 }
 
 // GetDiverging determines how many commits a PR is ahead or behind the PR base branch

services/pull/update_test.go

@@ -0,0 +1,172 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	git_model "code.gitea.io/gitea/models/git"
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm"
+	access_model "code.gitea.io/gitea/models/perm/access"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIsUserAllowedToUpdate(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	setRepoAllowRebaseUpdate := func(t *testing.T, repoID int64, allow bool) {
+		repoUnit := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoUnit{RepoID: repoID, Type: unit.TypePullRequests})
+		repoUnit.PullRequestsConfig().AllowRebaseUpdate = allow
+		require.NoError(t, repo_model.UpdateRepoUnit(t.Context(), repoUnit))
+	}
+
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	t.Run("RespectsProtectedBranch", func(t *testing.T) {
+		pr2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+		protectedBranch := &git_model.ProtectedBranch{
+			RepoID:       pr2.HeadRepoID,
+			RuleName:     pr2.HeadBranch,
+			CanPush:      false,
+			CanForcePush: false,
+		}
+		_, err := db.GetEngine(t.Context()).Insert(protectedBranch)
+		require.NoError(t, err)
+		defer db.DeleteByBean(t.Context(), protectedBranch)
+
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr2, user2)
+		assert.NoError(t, err)
+		assert.False(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+
+	t.Run("DisallowRebaseWhenConfigDisabled", func(t *testing.T) {
+		pr2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+		setRepoAllowRebaseUpdate(t, pr2.BaseRepoID, false)
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr2, user2)
+		assert.NoError(t, err)
+		assert.True(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+
+	t.Run("ReadOnlyAccessDenied", func(t *testing.T) {
+		pr2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+		user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+
+		collaboration := &repo_model.Collaboration{
+			RepoID: pr2.HeadRepoID,
+			UserID: user4.ID,
+			Mode:   perm.AccessModeRead,
+		}
+		require.NoError(t, db.Insert(t.Context(), collaboration))
+		defer db.DeleteByBean(t.Context(), collaboration)
+
+		require.NoError(t, pr2.LoadHeadRepo(t.Context()))
+		assert.NoError(t, access_model.RecalculateUserAccess(t.Context(), pr2.HeadRepo, user4.ID))
+
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr2, user4)
+		assert.NoError(t, err)
+		assert.False(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+
+	t.Run("ProtectedBranchAllowsPushWithoutRebase", func(t *testing.T) {
+		pr2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+		protectedBranch := &git_model.ProtectedBranch{
+			RepoID:       pr2.HeadRepoID,
+			RuleName:     pr2.HeadBranch,
+			CanPush:      true,
+			CanForcePush: false,
+		}
+		_, err := db.GetEngine(t.Context()).Insert(protectedBranch)
+		require.NoError(t, err)
+		defer db.DeleteByBean(t.Context(), protectedBranch)
+
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr2, user2)
+		assert.NoError(t, err)
+		assert.True(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+
+	pr3Poster := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 12})
+
+	t.Run("MaintainerEditRespectsPosterPermissions", func(t *testing.T) {
+		pr3 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3})
+		pr3.AllowMaintainerEdit = true
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr3, pr3Poster)
+		assert.NoError(t, err)
+		assert.False(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+
+	t.Run("MaintainerEditInheritsPosterPermissions", func(t *testing.T) {
+		pr3 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3})
+		pr3.AllowMaintainerEdit = true
+		protectedBranch := &git_model.ProtectedBranch{
+			RepoID:       pr3.HeadRepoID,
+			RuleName:     pr3.HeadBranch,
+			CanPush:      true,
+			CanForcePush: true,
+		}
+		_, err := db.GetEngine(t.Context()).Insert(protectedBranch)
+		require.NoError(t, err)
+		defer db.DeleteByBean(t.Context(), protectedBranch)
+
+		collaboration := &repo_model.Collaboration{
+			RepoID: pr3.HeadRepoID,
+			UserID: pr3Poster.ID,
+			Mode:   perm.AccessModeWrite,
+		}
+		require.NoError(t, db.Insert(t.Context(), collaboration))
+		defer db.DeleteByBean(t.Context(), collaboration)
+
+		require.NoError(t, pr3.LoadHeadRepo(t.Context()))
+		assert.NoError(t, access_model.RecalculateUserAccess(t.Context(), pr3.HeadRepo, pr3Poster.ID))
+
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr3, pr3Poster)
+		assert.NoError(t, err)
+		assert.True(t, pushAllowed)
+		assert.True(t, rebaseAllowed)
+	})
+
+	t.Run("MaintainerEditInheritsPosterPermissionsRebaseDisabled", func(t *testing.T) {
+		pr3 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3})
+		pr3.AllowMaintainerEdit = true
+		protectedBranch := &git_model.ProtectedBranch{
+			RepoID:       pr3.HeadRepoID,
+			RuleName:     pr3.HeadBranch,
+			CanPush:      true,
+			CanForcePush: true,
+		}
+		_, err := db.GetEngine(t.Context()).Insert(protectedBranch)
+		require.NoError(t, err)
+		defer db.DeleteByBean(t.Context(), protectedBranch)
+
+		collaboration := &repo_model.Collaboration{
+			RepoID: pr3.HeadRepoID,
+			UserID: pr3Poster.ID,
+			Mode:   perm.AccessModeWrite,
+		}
+		require.NoError(t, db.Insert(t.Context(), collaboration))
+		defer db.DeleteByBean(t.Context(), collaboration)
+
+		require.NoError(t, pr3.LoadHeadRepo(t.Context()))
+		assert.NoError(t, access_model.RecalculateUserAccess(t.Context(), pr3.HeadRepo, pr3Poster.ID))
+
+		setRepoAllowRebaseUpdate(t, pr3.BaseRepoID, false)
+
+		pushAllowed, rebaseAllowed, err := IsUserAllowedToUpdate(t.Context(), pr3, pr3Poster)
+		assert.NoError(t, err)
+		assert.True(t, pushAllowed)
+		assert.False(t, rebaseAllowed)
+	})
+}

tests/integration/pull_update_test.go

@@ -11,7 +11,6 @@ import (
 	"time"
 
 	auth_model "code.gitea.io/gitea/models/auth"
-	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -113,49 +112,6 @@ func TestAPIPullUpdateByRebase(t *testing.T) {
 	})
 }
 
-func TestAPIPullUpdateByRebase2(t *testing.T) {
-	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
-		// Create PR to test
-		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-		org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26})
-		pr := createOutdatedPR(t, user, org26)
-		assert.NoError(t, pr.LoadBaseRepo(t.Context()))
-		assert.NoError(t, pr.LoadIssue(t.Context()))
-
-		enableRepoAllowUpdateWithRebase(t, pr.BaseRepo.ID, false)
-
-		session := loginUser(t, "user2")
-		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
-		req := NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=rebase", pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index).
-			AddTokenAuth(token)
-		session.MakeRequest(t, req, http.StatusForbidden)
-
-		enableRepoAllowUpdateWithRebase(t, pr.BaseRepo.ID, true)
-		assert.NoError(t, pr.LoadHeadRepo(t.Context()))
-
-		// add a protected branch rule to the head branch to block rebase
-		pb := git_model.ProtectedBranch{
-			RepoID:       pr.HeadRepo.ID,
-			RuleName:     pr.HeadBranch,
-			CanPush:      false,
-			CanForcePush: false,
-		}
-		err := git_model.UpdateProtectBranch(t.Context(), pr.HeadRepo, &pb, git_model.WhitelistOptions{})
-		assert.NoError(t, err)
-		req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=rebase", pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index).
-			AddTokenAuth(token)
-		session.MakeRequest(t, req, http.StatusForbidden)
-
-		// remove the protected branch rule to allow rebase
-		err = git_model.DeleteProtectedBranch(t.Context(), pr.HeadRepo, pb.ID)
-		assert.NoError(t, err)
-
-		req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update?style=rebase", pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Issue.Index).
-			AddTokenAuth(token)
-		session.MakeRequest(t, req, http.StatusOK)
-	})
-}
-
 func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_model.PullRequest {
 	baseRepo, err := repo_service.CreateRepository(t.Context(), actor, actor, repo_service.CreateRepoOptions{
 		Name:        "repo-pr-update",