Fix allow maintainer edit permission check (#37479)

models/issues/pull_list.go

@@ -71,38 +71,69 @@ func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch
 }
 
 // CanMaintainerWriteToBranch check whether user is a maintainer and could write to the branch
-func CanMaintainerWriteToBranch(ctx context.Context, p access_model.Permission, branch string, user *user_model.User) bool {
-	if p.CanWrite(unit.TypeCode) {
-		return true
+func CanMaintainerWriteToBranch(ctx context.Context, headPerm access_model.Permission, headBranch string, doer *user_model.User) bool {
+	can, err := canMaintainerWriteToBranch(ctx, headPerm, headBranch, doer)
+	if err != nil {
+		log.Error("CanMaintainerWriteToBranch: %v", err)
+		return false
+	}
+	return can
+}
+
+func canMaintainerWriteToBranch(ctx context.Context, headPerm access_model.Permission, headBranch string, doer *user_model.User) (bool, error) {
+	if headPerm.CanWrite(unit.TypeCode) {
+		return true, nil
 	}
 
 	// the code below depends on units to get the repository ID, not ideal but just keep it for now
-	firstUnitRepoID := p.GetFirstUnitRepoID()
+	firstUnitRepoID := headPerm.GetFirstUnitRepoID()
 	if firstUnitRepoID == 0 {
-		return false
+		return false, nil
 	}
 
-	prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, firstUnitRepoID, branch)
+	prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, firstUnitRepoID, headBranch)
 	if err != nil {
-		return false
+		return false, err
+	}
+	if _, err := prs.LoadIssues(ctx); err != nil {
+		return false, err
 	}
-
 	for _, pr := range prs {
-		if pr.AllowMaintainerEdit {
-			err = pr.LoadBaseRepo(ctx)
-			if err != nil {
-				continue
-			}
-			prPerm, err := access_model.GetIndividualUserRepoPermission(ctx, pr.BaseRepo, user)
-			if err != nil {
-				continue
-			}
-			if prPerm.CanWrite(unit.TypeCode) {
-				return true
-			}
+		if !pr.AllowMaintainerEdit {
+			continue
+		}
+
+		// check the PR's poster's permissions
+		// If a "reader" poster created the PR in base repo from head repo, even if it is allowed to be edited by maintainers,
+		// the maintainers should not be allowed to write, because they don't really have "write" permission in the head repo
+		if err := pr.Issue.LoadPoster(ctx); err != nil {
+			return false, err
+		}
+		if err := pr.LoadHeadRepo(ctx); err != nil {
+			return false, err
+		}
+		posterHeadPerm, err := access_model.GetIndividualUserRepoPermission(ctx, pr.HeadRepo, pr.Issue.Poster)
+		if err != nil {
+			return false, err
+		}
+		if !posterHeadPerm.CanWrite(unit.TypeCode) {
+			continue
+		}
+
+		// check the doer's permission
+		// Only allow the doer to edit the PR if they have write access to the base repository
+		if err := pr.LoadBaseRepo(ctx); err != nil {
+			return false, err
+		}
+		doerBasePerm, err := access_model.GetIndividualUserRepoPermission(ctx, pr.BaseRepo, doer)
+		if err != nil {
+			return false, err
+		}
+		if doerBasePerm.CanWrite(unit.TypeCode) {
+			return true, nil
 		}
 	}
-	return false
+	return false, nil
 }
 
 // HasUnmergedPullRequestsByHeadInfo checks if there are open and not merged pull request

models/issues/pull_list_test.go

@@ -6,15 +6,28 @@ package issues_test
 import (
 	"testing"
 
+	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm"
+	"code.gitea.io/gitea/models/perm/access"
+	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"xorm.io/builder"
 )
 
-func TestPullRequestList_LoadAttributes(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
+func TestPullRequestList(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+	t.Run("LoadAttributes", testPullRequestListLoadAttributes)
+	t.Run("LoadReviewCommentsCounts", testPullRequestListLoadReviewCommentsCounts)
+	t.Run("LoadReviews", testPullRequestListLoadReviews)
+	t.Run("CanMaintainerWriteToBranch", testCanMaintainerWriteToBranch)
+}
 
+func testPullRequestListLoadAttributes(t *testing.T) {
 	prs := issues_model.PullRequestList{
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}),
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}),
@@ -28,9 +41,7 @@ func TestPullRequestList_LoadAttributes(t *testing.T) {
 	assert.NoError(t, issues_model.PullRequestList([]*issues_model.PullRequest{}).LoadAttributes(t.Context()))
 }
 
-func TestPullRequestList_LoadReviewCommentsCounts(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
+func testPullRequestListLoadReviewCommentsCounts(t *testing.T) {
 	prs := issues_model.PullRequestList{
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}),
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}),
@@ -43,9 +54,7 @@ func TestPullRequestList_LoadReviewCommentsCounts(t *testing.T) {
 	}
 }
 
-func TestPullRequestList_LoadReviews(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
+func testPullRequestListLoadReviews(t *testing.T) {
 	prs := issues_model.PullRequestList{
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}),
 		unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}),
@@ -61,3 +70,73 @@ func TestPullRequestList_LoadReviews(t *testing.T) {
 	assert.EqualValues(t, 10, reviewList[4].ID)
 	assert.EqualValues(t, 22, reviewList[5].ID)
 }
+
+func testCanMaintainerWriteToBranch(t *testing.T) {
+	ctx := t.Context()
+	baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
+	headRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
+
+	_ = baseRepo.LoadOwner(ctx)
+	_ = headRepo.LoadOwner(ctx)
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	// a PR from header's owner
+	headOwnerPR := &issues_model.PullRequest{
+		Issue: &issues_model.Issue{
+			RepoID:   baseRepo.ID,
+			PosterID: headRepo.OwnerID,
+		},
+		HeadRepoID: headRepo.ID,
+		BaseRepoID: baseRepo.ID,
+		HeadBranch: "pr-from-head-owner",
+		BaseBranch: "master",
+	}
+	require.NoError(t, issues_model.NewPullRequest(ctx, baseRepo, headOwnerPR.Issue, nil, nil, headOwnerPR))
+
+	// a PR from a user, they might have or not have "write" permission in the target repo
+	anyUserPR := &issues_model.PullRequest{
+		Issue: &issues_model.Issue{
+			RepoID:   baseRepo.ID,
+			PosterID: user.ID,
+		},
+		HeadRepoID: headRepo.ID,
+		BaseRepoID: baseRepo.ID,
+		HeadBranch: "pr-from-head-user",
+		BaseBranch: "master",
+	}
+	require.NoError(t, issues_model.NewPullRequest(ctx, baseRepo, anyUserPR.Issue, nil, nil, anyUserPR))
+
+	doerCanWrite := func(doer *user_model.User, pr *issues_model.PullRequest) bool {
+		headPerm, _ := access.GetIndividualUserRepoPermission(ctx, headRepo, doer)
+		return issues_model.CanMaintainerWriteToBranch(ctx, headPerm, pr.HeadBranch, doer)
+	}
+
+	t.Run("NoAllowMaintainerEdit", func(t *testing.T) {
+		assert.True(t, doerCanWrite(headRepo.Owner, headOwnerPR))
+		assert.False(t, doerCanWrite(baseRepo.Owner, headOwnerPR))
+		assert.False(t, doerCanWrite(baseRepo.Owner, anyUserPR))
+		assert.False(t, doerCanWrite(user, anyUserPR))
+	})
+
+	t.Run("WithAllowMaintainerEdit-HeadPosterReader", func(t *testing.T) {
+		_, err := db.GetEngine(ctx).Where(builder.In("id", []int64{headOwnerPR.ID, anyUserPR.ID})).
+			Cols("allow_maintainer_edit").
+			Update(&issues_model.PullRequest{AllowMaintainerEdit: true})
+		require.NoError(t, err)
+		assert.True(t, doerCanWrite(baseRepo.Owner, headOwnerPR))
+		assert.False(t, doerCanWrite(baseRepo.Owner, anyUserPR)) // poster doesn't have write permission, so maintainer can't write either
+	})
+
+	t.Run("WithAllowMaintainerEdit-HeadPosterWriter", func(t *testing.T) {
+		_, err := db.GetEngine(ctx).Where(builder.In("id", []int64{headOwnerPR.ID, anyUserPR.ID})).
+			Cols("allow_maintainer_edit").
+			Update(&issues_model.PullRequest{AllowMaintainerEdit: true})
+		require.NoError(t, err)
+		err = db.Insert(ctx, &repo_model.Collaboration{RepoID: headRepo.ID, UserID: user.ID, Mode: perm.AccessModeWrite})
+		require.NoError(t, err)
+		err = db.Insert(ctx, &access.Access{RepoID: headRepo.ID, UserID: user.ID, Mode: perm.AccessModeWrite})
+		require.NoError(t, err)
+		assert.True(t, doerCanWrite(baseRepo.Owner, headOwnerPR))
+		assert.True(t, doerCanWrite(baseRepo.Owner, anyUserPR)) // now the poster has the write permission
+	})
+}

routers/web/repo/pull.go

@@ -958,13 +958,13 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) {
 
 		if pull.HeadRepo != nil {
 			if !pull.HasMerged && ctx.Doer != nil {
-				perm, err := access_model.GetDoerRepoPermission(ctx, pull.HeadRepo, ctx.Doer)
+				headPerm, err := access_model.GetDoerRepoPermission(ctx, pull.HeadRepo, ctx.Doer)
 				if err != nil {
 					ctx.ServerError("GetDoerRepoPermission", err)
 					return
 				}
 
-				if perm.CanWrite(unit.TypeCode) || issues_model.CanMaintainerWriteToBranch(ctx, perm, pull.HeadBranch, ctx.Doer) {
+				if issues_model.CanMaintainerWriteToBranch(ctx, headPerm, pull.HeadBranch, ctx.Doer) {
 					ctx.Data["CanEditFile"] = true
 					ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.edit_this_file")
 					ctx.Data["HeadRepoLink"] = pull.HeadRepo.Link()