mirror of
https://github.com/go-gitea/gitea.git
synced 2026-03-08 02:52:35 +01:00
Fix bug to check whether user can update pull request branch or rebase branch (#36465)
When checking whether a user can update a pull request branch or perform an update via rebase, a maintainer should inherit the pull request author’s permissions if Allow maintainer edits is enabled. --------- Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com> Signed-off-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
Lunny Xiao
GitHub
parent
723ce3579f
commit
99b0bf7324
@ -100,78 +100,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
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
// 1. check base repository's AllowRebaseUpdate configuration
|
||||
// 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)
|
||||
}
|
||||
|
||||
// 2. only support Github style pull request
|
||||
if pull.Flow == issues_model.PullRequestFlowAGit {
|
||||
return false, false, nil
|
||||
}
|
||||
|
||||
// 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, 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
|
||||
}
|
||||
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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 5. 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
|
||||
}
|
||||
|
||||
// 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)
|
||||
}
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
mergeAllowed, err = isUserAllowedToMergeInRepoBranch(ctx, pull.HeadRepoID, pull.HeadBranch, headRepoPerm, user)
|
||||
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)
|
||||
if err != nil {
|
||||
return false, false, err
|
||||
}
|
||||
|
||||
mergeAllowed = mergeAllowed || mergeAllowedMaintainer
|
||||
}
|
||||
|
||||
// if merge is not allowed, rebase is also not allowed
|
||||
rebaseAllowed = rebaseAllowed && mergeAllowed
|
||||
|
||||
return mergeAllowed, rebaseAllowed, nil
|
||||
return pushAllowed, rebaseAllowed && prBaseUnit.PullRequestsConfig().AllowRebaseUpdate, nil
|
||||
}
|
||||
|
||||
func syncCommitDivergence(ctx context.Context, pr *issues_model.PullRequest) error {
|
||||
|
||||
172
services/pull/update_test.go
Normal file
172
services/pull/update_test.go
Normal file
@ -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)
|
||||
})
|
||||
}
|
||||
@ -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"
|
||||
@ -121,49 +120,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",
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user