diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 5732194fd34..2d3f5fe2ac4 100644 --- a/routers/web/repo/issue.go +++ b/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 } diff --git a/routers/web/repo/issue_label.go b/routers/web/repo/issue_label.go index ee4195ebf25..bfde7fc9827 100644 --- a/routers/web/repo/issue_label.go +++ b/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 } diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index 3bda048a1d6..279820403f3 100644 --- a/routers/web/repo/issue_list.go +++ b/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 } diff --git a/routers/web/repo/issue_lock.go b/routers/web/repo/issue_lock.go index 4d9e8fe9048..6e09b576e0e 100644 --- a/routers/web/repo/issue_lock.go +++ b/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 } diff --git a/routers/web/repo/issue_test.go b/routers/web/repo/issue_test.go index b59ff137a32..10a0c311119 100644 --- a/routers/web/repo/issue_test.go +++ b/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) +} diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 9c646082a59..3d1644ff08a 100644 --- a/routers/web/repo/projects.go +++ b/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 } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 90382eb0095..8daa7f91b64 100644 --- a/routers/web/repo/pull_review.go +++ b/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 } diff --git a/routers/web/web.go b/routers/web/web.go index 77ebb76e19b..bdd8560b796 100644 --- a/routers/web/web.go +++ b/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() {