fix(issues): require per-item write access for batch issue actions (#40047)

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/web/repo/issue.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"html/template"
 	"net/http"
-	"strconv"
 	"strings"
 
 	"gitea.dev/models/db"
@@ -165,14 +164,15 @@ func retrieveProjectsInternal(ctx *context.Context, repo *repo_model.Repository)
 }
 
 // GetActionIssue will return the issue which is used in the context.
-func GetActionIssue(ctx *context.Context) *issues_model.Issue {
+func GetActionIssue(ctx *context.Context, accessModeOpt ...perm.AccessMode) *issues_model.Issue {
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {
 		ctx.ServerError("GetIssueByIndex", err)
 		return nil
 	}
 	issue.Repo = ctx.Repo.Repository
-	if !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull) {
+	accessMode := util.OptionalArg(accessModeOpt, perm.AccessModeRead)
+	if !ctx.Repo.Permission.CanAccess(accessMode, util.Iif(issue.IsPull, unit.TypePullRequests, unit.TypeIssues)) {
 		ctx.NotFound(nil)
 		return nil
 	}
@@ -183,34 +183,20 @@ func GetActionIssue(ctx *context.Context) *issues_model.Issue {
 	return issue
 }
 
-func getActionIssues(ctx *context.Context) issues_model.IssueList {
-	commaSeparatedIssueIDs := ctx.FormString("issue_ids")
-	if len(commaSeparatedIssueIDs) == 0 {
-		return nil
-	}
-	issueIDs := make([]int64, 0, 10)
-	for stringIssueID := range strings.SplitSeq(commaSeparatedIssueIDs, ",") {
-		issueID, err := strconv.ParseInt(stringIssueID, 10, 64)
-		if err != nil {
-			ctx.ServerError("ParseInt", err)
-			return nil
-		}
-		issueIDs = append(issueIDs, issueID)
-	}
+func getActionIssues(ctx *context.Context, accessMode perm.AccessMode) issues_model.IssueList {
+	issueIDs := ctx.FormStringInt64s("issue_ids")
 	issues, err := issues_model.GetIssuesByIDs(ctx, issueIDs)
 	if err != nil {
 		ctx.ServerError("GetIssuesByIDs", err)
 		return nil
 	}
-	// Check access rights for all issues
-	issueUnitEnabled := ctx.Repo.Permission.CanRead(unit.TypeIssues)
-	prUnitEnabled := ctx.Repo.Permission.CanRead(unit.TypePullRequests)
 	for _, issue := range issues {
 		if issue.RepoID != ctx.Repo.Repository.ID {
 			ctx.NotFound(errors.New("some issue's RepoID is incorrect"))
 			return nil
 		}
-		if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled {
+		issue.Repo = ctx.Repo.Repository
+		if !ctx.Repo.Permission.CanAccess(accessMode, util.Iif(issue.IsPull, unit.TypePullRequests, unit.TypeIssues)) {
 			ctx.NotFound(nil)
 			return nil
 		}
@@ -385,7 +371,7 @@ func UpdateIssueDeadline(ctx *context.Context) {
 
 // UpdateIssueMilestone change issue's milestone
 func UpdateIssueMilestone(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}
@@ -418,7 +404,7 @@ func UpdateIssueMilestone(ctx *context.Context) {
 
 // UpdateIssueAssignee change issue's or pull's assignee
 func UpdateIssueAssignee(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}

routers/web/repo/issue_label.go

@@ -12,6 +12,7 @@ import (
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
 	"gitea.dev/models/organization"
+	"gitea.dev/models/perm"
 	"gitea.dev/modules/label"
 	repo_module "gitea.dev/modules/repository"
 	"gitea.dev/modules/templates"
@@ -175,7 +176,7 @@ func DeleteLabel(ctx *context.Context) {
 
 // UpdateIssueLabel change issue's labels
 func UpdateIssueLabel(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}

routers/web/repo/issue_list.go

@@ -15,6 +15,7 @@ import (
 
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	issue_indexer "gitea.dev/modules/indexer/issues"
@@ -309,7 +310,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) {
 }
 
 func BatchDeleteIssues(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}
@@ -324,7 +325,7 @@ func BatchDeleteIssues(ctx *context.Context) {
 
 // UpdateIssueStatus change issue's status
 func UpdateIssueStatus(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}

routers/web/repo/issue_lock.go

@@ -5,6 +5,7 @@ package repo
 
 import (
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	"gitea.dev/modules/web"
 	"gitea.dev/services/context"
 	"gitea.dev/services/forms"
@@ -14,8 +15,8 @@ import (
 // users with write access to the repo.
 func LockIssue(ctx *context.Context) {
 	form := web.GetForm[*forms.IssueLockForm](ctx)
-	issue := GetActionIssue(ctx)
-	if ctx.Written() {
+	issue := GetActionIssue(ctx, perm.AccessModeWrite)
+	if issue == nil {
 		return
 	}
 
@@ -38,8 +39,8 @@ func LockIssue(ctx *context.Context) {
 
 // UnlockIssue unlocks a previously locked issue.
 func UnlockIssue(ctx *context.Context) {
-	issue := GetActionIssue(ctx)
-	if ctx.Written() {
+	issue := GetActionIssue(ctx, perm.AccessModeWrite)
+	if issue == nil {
 		return
 	}
 

routers/web/repo/issue_test.go

@@ -4,13 +4,23 @@
 package repo
 
 import (
+	"net/http"
 	"testing"
 
+	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/organization"
+	"gitea.dev/models/perm"
+	"gitea.dev/models/unit"
+	"gitea.dev/models/unittest"
 	"gitea.dev/modules/references"
 	"gitea.dev/modules/timeutil"
+	"gitea.dev/modules/web"
+	"gitea.dev/services/contexttest"
+	"gitea.dev/services/forms"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCombineXRefComments(t *testing.T) {
@@ -165,3 +175,43 @@ func TestCombineLabelComments(t *testing.T) {
 		})
 	}
 }
+
+func TestIssueActionsRequireItemUnitWrite(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	// user4's team on org3/repo3 uses per-unit permissions: pull requests write, issues read
+	_, err := db.GetEngine(t.Context()).ID(2).Cols("authorize").Update(&organization.Team{AccessMode: perm.AccessModeNone})
+	require.NoError(t, err)
+	_, err = db.GetEngine(t.Context()).Where("team_id = ? AND `type` = ?", 2, unit.TypeIssues).Cols("access_mode").Update(&organization.TeamUnit{AccessMode: perm.AccessModeRead})
+	require.NoError(t, err)
+
+	closeIssues := func(issueIDs string) int {
+		ctx, resp := contexttest.MockContext(t, "org3/repo3/issues/status")
+		contexttest.LoadUser(t, ctx, 4)
+		contexttest.LoadRepo(t, ctx, 3)
+		ctx.Req.Form.Set("issue_ids", issueIDs)
+		ctx.Req.Form.Set("action", "close")
+		UpdateIssueStatus(ctx)
+		return resp.Code
+	}
+	assert.Equal(t, http.StatusNotFound, closeIssues("6,12"))
+	assert.False(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 6}).IsClosed)
+	assert.False(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12}).IsClosed)
+
+	assert.Equal(t, http.StatusOK, closeIssues("12"))
+	assert.True(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12}).IsClosed)
+
+	lockIssue := func(index string) int {
+		ctx, resp := contexttest.MockContext(t, "org3/repo3/issues/"+index+"/lock")
+		contexttest.LoadUser(t, ctx, 4)
+		contexttest.LoadRepo(t, ctx, 3)
+		ctx.SetPathParam("index", index)
+		web.SetForm(ctx, &forms.IssueLockForm{Reason: "Spam"})
+		LockIssue(ctx)
+		return resp.Code
+	}
+	assert.Equal(t, http.StatusNotFound, lockIssue("1"))
+	assert.False(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 6}).IsLocked)
+
+	assert.Equal(t, http.StatusOK, lockIssue("2"))
+	assert.True(t, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12}).IsLocked)
+}

routers/web/repo/projects.go

@@ -12,6 +12,7 @@ import (
 	audit_model "gitea.dev/models/audit"
 	"gitea.dev/models/db"
 	issues_model "gitea.dev/models/issues"
+	"gitea.dev/models/perm"
 	project_model "gitea.dev/models/project"
 	"gitea.dev/models/renderhelper"
 	repo_model "gitea.dev/models/repo"
@@ -446,7 +447,7 @@ func ProjectWorkflows(ctx *context.Context) {
 
 // UpdateIssueProject change an issue's project
 func UpdateIssueProject(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeWrite)
 	if ctx.Written() {
 		return
 	}
@@ -487,7 +488,7 @@ func UpdateIssueProjectColumn(ctx *context.Context) {
 		ctx.ServerError("GetIssueByID", err)
 		return
 	}
-	if !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull) {
+	if !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) {
 		ctx.NotFound(nil)
 		return
 	}

routers/web/repo/pull_review.go

@@ -10,6 +10,7 @@ import (
 
 	issues_model "gitea.dev/models/issues"
 	"gitea.dev/models/organization"
+	"gitea.dev/models/perm"
 	pull_model "gitea.dev/models/pull"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/json"
@@ -345,7 +346,7 @@ func UpdateViewedFiles(ctx *context.Context) {
 
 // UpdatePullReviewRequest add or remove review request
 func UpdatePullReviewRequest(ctx *context.Context) {
-	issues := getActionIssues(ctx)
+	issues := getActionIssues(ctx, perm.AccessModeRead)
 	if ctx.Written() {
 		return
 	}

routers/web/web.go

@@ -927,8 +927,6 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 	reqRepoCodeWriter := context.RequireUnitWriter(unit.TypeCode)
 	reqRepoReleaseWriter := context.RequireUnitWriter(unit.TypeReleases)
 	reqRepoReleaseReader := context.RequireUnitReader(unit.TypeReleases)
-	reqRepoIssuesOrPullsWriter := context.RequireUnitWriter(unit.TypeIssues, unit.TypePullRequests)
-	reqRepoIssuesOrPullsReader := context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests)
 	reqRepoProjectsReader := context.RequireUnitReader(unit.TypeProjects)
 	reqRepoProjectsWriter := context.RequireUnitWriter(unit.TypeProjects)
 	reqRepoActionsReader := context.RequireUnitReader(unit.TypeActions)
@@ -1348,7 +1346,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 		m.Get("/milestones", repo.Milestones)
 		m.Get("/milestone/{id}", repo.MilestoneIssuesAndPulls)
 		m.Get("/issues/suggestions", repo.IssueSuggestions)
-	}, optSignIn, context.RepoAssignment, reqRepoIssuesOrPullsReader) // issue/pull attachments, labels, milestones
+	}, optSignIn, context.RepoAssignment, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests)) // issue/pull attachments, labels, milestones
 	// end "/{username}/{reponame}": view milestone, label, issue, pull, etc
 
 	m.Group("/{username}/{reponame}/{type:issues}", func() {
@@ -1394,8 +1392,8 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 				})
 				m.Post("/time_estimate", repo.UpdateIssueTimeEstimate)
 				m.Post("/reactions/{action}", web.Bind[*forms.ReactionForm](), repo.ChangeIssueReaction)
-				m.Post("/lock", reqRepoIssuesOrPullsWriter, web.Bind[*forms.IssueLockForm](), repo.LockIssue)
-				m.Post("/unlock", reqRepoIssuesOrPullsWriter, repo.UnlockIssue)
+				m.Post("/lock", web.Bind[*forms.IssueLockForm](), repo.LockIssue)
+				m.Post("/unlock", repo.UnlockIssue)
 				m.Post("/delete", reqRepoAdmin, repo.DeleteIssue)
 				m.Post("/content-history/soft-delete", repo.SoftDeleteContentHistory)
 			})
@@ -1403,12 +1401,12 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			m.Post("/attachments", repo.UploadIssueAttachment)
 			m.Post("/attachments/remove", repo.DeleteAttachment)
 
-			m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel)
-			m.Post("/milestone", reqRepoIssuesOrPullsWriter, repo.UpdateIssueMilestone)
-			m.Post("/projects", reqRepoIssuesOrPullsWriter, reqRepoProjectsReader, repo.UpdateIssueProject)
-			m.Post("/projects/column", reqRepoIssuesOrPullsWriter, reqRepoProjectsWriter, repo.UpdateIssueProjectColumn)
-			m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee)
-			m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus)
+			m.Post("/labels", repo.UpdateIssueLabel)
+			m.Post("/milestone", repo.UpdateIssueMilestone)
+			m.Post("/projects", reqRepoProjectsReader, repo.UpdateIssueProject)
+			m.Post("/projects/column", reqRepoProjectsWriter, repo.UpdateIssueProjectColumn)
+			m.Post("/assignee", repo.UpdateIssueAssignee)
+			m.Post("/status", repo.UpdateIssueStatus)
 			m.Post("/delete", reqRepoAdmin, repo.BatchDeleteIssues)
 			m.Delete("/unpin/{index}", reqRepoAdmin, repo.IssueUnpin)
 			m.Post("/move_pin", reqRepoAdmin, repo.IssuePinMove)
@@ -1421,14 +1419,14 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			m.Post("", repo.UpdateCommentContent)
 			m.Post("/delete", repo.DeleteComment)
 			m.Post("/reactions/{action}", web.Bind[*forms.ReactionForm](), repo.ChangeCommentReaction)
-		}, reqRepoIssuesOrPullsReader) // edit issue/pull comment
+		}) // edit issue/pull comment
 
 		m.Group("/labels", func() {
 			m.Post("/new", web.Bind[*forms.CreateLabelForm](), repo.NewLabel)
 			m.Post("/edit", web.Bind[*forms.CreateLabelForm](), repo.UpdateLabel)
 			m.Post("/delete", repo.DeleteLabel)
 			m.Post("/initialize", web.Bind[*forms.InitializeLabelsForm](), repo.InitializeLabels)
-		}, reqRepoIssuesOrPullsWriter)
+		}, context.RequireUnitWriter(unit.TypeIssues, unit.TypePullRequests))
 
 		m.Group("/milestones", func() {
 			m.Combo("/new").Get(repo.NewMilestone).
@@ -1437,7 +1435,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			m.Post("/{id}/edit", web.Bind[*forms.CreateMilestoneForm](), repo.EditMilestonePost)
 			m.Post("/{id}/{action}", repo.ChangeMilestoneStatus)
 			m.Post("/delete", repo.DeleteMilestone)
-		}, reqRepoIssuesOrPullsWriter)
+		}, context.RequireUnitWriter(unit.TypeIssues, unit.TypePullRequests))
 
 		// FIXME: many "pulls" requests are sent to "issues" endpoints incorrectly, need to move these routes to the proper place
 		m.Group("/issues", func() {