diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index bf8f9b7d914d0..6a6abca9709df 100644 --- a/models/issues/pull_list.go +++ b/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 diff --git a/models/issues/pull_list_test.go b/models/issues/pull_list_test.go index 437830701c671..302b2ca0ba7da 100644 --- a/models/issues/pull_list_test.go +++ b/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 + }) +} diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index e312fc9d2a816..3933da8764e2c 100644 --- a/routers/web/repo/pull.go +++ b/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()