Merge branch 'lunny/fix_maintainer_edit_check' into lunny/fix_maintainer_edit_check2

Author: lunny

models/issues/pull_list.go

@@ -50,37 +50,38 @@ func listPullRequestStatement(ctx context.Context, baseRepoID int64, opts *PullR
 }
 
 // GetUnmergedPullRequestsByHeadInfo returns all pull requests that are open and has not been merged
-func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) ([]*PullRequest, error) {
+func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) (PullRequestList, error) {
 	prs := make([]*PullRequest, 0, 2)
 	sess := db.GetEngine(ctx).
 		Join("INNER", "issue", "issue.id = pull_request.issue_id").
 		Where("head_repo_id = ? AND head_branch = ? AND has_merged = ? AND issue.is_closed = ? AND flow = ?", repoID, branch, false, false, PullRequestFlowGithub)
 	return prs, sess.Find(&prs)
 }
 
-// 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 {
+func CanUserWriteToBranch(ctx context.Context, p access_model.Permission, headRepoID int64, branch string, user *user_model.User) bool {
 	if p.CanWrite(unit.TypeCode) {
 		return true
 	}
 
-	// the code below depends on units to get the repository ID, not ideal but just keep it for now
-	firstUnitRepoID := p.GetFirstUnitRepoID()
-	if firstUnitRepoID == 0 {
+	return canMaintainerWriteToHeadBranch(ctx, headRepoID, branch, user)
+}
+
+// canMaintainerWriteToHeadBranch check whether user is a maintainer and could write to the branch
+func canMaintainerWriteToHeadBranch(ctx context.Context, headRepoID int64, branch string, user *user_model.User) bool {
+	prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, headRepoID, branch)
+	if err != nil {
+		log.Error("GetUnmergedPullRequestsByHeadInfo: %v", err)
 		return false
 	}
 
-	prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, firstUnitRepoID, branch)
-	if err != nil {
+	if err := prs.LoadRepositories(ctx); err != nil {
+		log.Error("LoadBaseRepos: %v", err)
 		return false
 	}
 
+	// user can write to the branch once one pull request allowed the user edit the branch
 	for _, pr := range prs {
 		if pr.AllowMaintainerEdit {
-			err = pr.LoadBaseRepo(ctx)
-			if err != nil {
-				continue
-			}
 			prPerm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, user)
 			if err != nil {
 				continue

models/perm/access/repo_permission.go

@@ -63,15 +63,6 @@ func (p *Permission) HasUnits() bool {
 	return len(p.units) > 0
 }
 
-// GetFirstUnitRepoID returns the repo ID of the first unit, it is a fragile design and should NOT be used anymore
-// deprecated
-func (p *Permission) GetFirstUnitRepoID() int64 {
-	if len(p.units) > 0 {
-		return p.units[0].RepoID
-	}
-	return 0
-}
-
 // UnitAccessMode returns current user access mode to the specify unit of the repository
 // It also considers "everyone access mode"
 func (p *Permission) UnitAccessMode(unitType unit.Type) perm_model.AccessMode {

routers/private/hook_pre_receive.go

@@ -55,7 +55,7 @@ func (ctx *preReceiveContext) CanWriteCode() bool {
 		if !ctx.loadPusherAndPermission() {
 			return false
 		}
-		ctx.canWriteCode = issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
+		ctx.canWriteCode = issues_model.CanUserWriteToBranch(ctx, ctx.userPerm, ctx.Repo.Repository.ID, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
 		ctx.checkedCanWriteCode = true
 	}
 	return ctx.canWriteCode

routers/web/repo/pull.go

@@ -871,7 +871,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 					return
 				}
 
-				if perm.CanWrite(unit.TypeCode) || issues_model.CanMaintainerWriteToBranch(ctx, perm, pull.HeadBranch, ctx.Doer) {
+				if issues_model.CanUserWriteToBranch(ctx, perm, pull.HeadRepoID, pull.HeadBranch, ctx.Doer) {
 					ctx.Data["CanEditFile"] = true
 					ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.edit_this_file")
 					ctx.Data["HeadRepoLink"] = pull.HeadRepo.Link()

services/context/repo.go

@@ -70,7 +70,7 @@ type Repository struct {
 
 // CanWriteToBranch checks if the branch is writable by the user
 func (r *Repository) CanWriteToBranch(ctx context.Context, user *user_model.User, branch string) bool {
-	return issues_model.CanMaintainerWriteToBranch(ctx, r.Permission, branch, user)
+	return issues_model.CanUserWriteToBranch(ctx, r.Permission, r.Repository.ID, branch, user)
 }
 
 // CanEnableEditor returns true if repository is editable and user has proper access level.

services/convert/convert.go

@@ -67,7 +67,7 @@ func ToBranch(ctx context.Context, repo *repo_model.Repository, branchName strin
 			if err != nil {
 				return nil, err
 			}
-			canPush = issues_model.CanMaintainerWriteToBranch(ctx, perms, branchName, user)
+			canPush = issues_model.CanUserWriteToBranch(ctx, perms, repo.ID, branchName, user)
 		}
 
 		return &api.Branch{

services/pull/pull.go

@@ -331,11 +331,10 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
 		}
 
 		if isSync {
-			requests := issues_model.PullRequestList(prs)
-			if err = requests.LoadAttributes(ctx); err != nil {
+			if err = prs.LoadAttributes(ctx); err != nil {
 				log.Error("PullRequestList.LoadAttributes: %v", err)
 			}
-			if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch); invalidationErr != nil {
+			if invalidationErr := checkForInvalidation(ctx, prs, repoID, doer, branch); invalidationErr != nil {
 				log.Error("checkForInvalidation: %v", invalidationErr)
 			}
 			if err == nil {
@@ -627,7 +626,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64,
 	}
 
 	prs = append(prs, prs2...)
-	if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil {
+	if err := prs.LoadAttributes(ctx); err != nil {
 		return err
 	}
 
@@ -657,7 +656,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 			return err
 		}
 
-		if err = issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil {
+		if err = prs.LoadAttributes(ctx); err != nil {
 			return err
 		}
 

services/pull/review.go

@@ -16,6 +16,7 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
@@ -71,7 +72,10 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis
 	if len(prs) == 0 {
 		return nil
 	}
-	issueIDs := prs.GetIssueIDs()
+
+	issueIDs := container.FilterSlice(prs, func(pr *issues_model.PullRequest) (int64, bool) {
+		return pr.IssueID, true
+	})
 
 	codeComments, err := db.Find[issues_model.Comment](ctx, issues_model.FindCommentsOptions{
 		ListOptions: db.ListOptionsAll,

tests/integration/pull_allowedit_test.go

@@ -0,0 +1,52 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/url"
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	pull_service "code.gitea.io/gitea/services/pull"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestPullAllowMaintainerEdit(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		// create a pull request
+		session := loginUser(t, "user1")
+		user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+		forkedName := "repo1"
+		testRepoFork(t, session, "org3", "repo5", "user1", forkedName, "master")
+		defer func() {
+			testDeleteRepository(t, session, "user1", forkedName)
+		}()
+		testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
+		testPullCreate(t, session, "user1", forkedName, false, "master", "master", "Indexer notifier test pull")
+
+		baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "org3", Name: "repo5"})
+		forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: forkedName})
+		pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
+			BaseRepoID: baseRepo.ID,
+			BaseBranch: "master",
+			HeadRepoID: forkedRepo.ID,
+			HeadBranch: "master",
+		})
+		assert.False(t, pr.AllowMaintainerEdit)
+		assert.NoError(t, pr.LoadIssue(db.DefaultContext))
+
+		// allow org3's member to edit the branch's files
+		err := pull_service.SetAllowEdits(db.DefaultContext, user1, pr, true)
+		assert.NoError(t, err)
+
+		// user2 is in org3 team
+		session = loginUser(t, "user2")
+		testEditFile(t, session, "user1", forkedName, "master", "README.md", "Hello, World (Edited)\n")
+	})
+}