From c1b0c9e7c4bbb669ea03694c30c9ff66a109ef17 Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Mon, 16 Oct 2017 10:55:43 +0300 Subject: [PATCH] Fix PR, milestone and label functionality if issue unit is disabled (#2710) * Fix PR, milestone and label functionality if issue unit is disabled or not assigned to user * Fix multi-actions in PR page * Change error message * Fix comment update and delete functionality in PR --- routers/repo/issue.go | 43 +++++++++++++++++++++------------ routers/repo/issue_label.go | 4 --- routers/repo/issue_stopwatch.go | 22 +++++++++-------- routers/repo/issue_timetrack.go | 14 +++++------ routers/repo/issue_watch.go | 8 +++--- routers/routes/routes.go | 33 +++++++++++-------------- templates/repo/issue/list.tmpl | 10 ++++---- 7 files changed, 68 insertions(+), 66 deletions(-) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 091268116b..c24a4e4360 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -720,11 +720,16 @@ func ViewIssue(ctx *context.Context) { func GetActionIssue(ctx *context.Context) *models.Issue { issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { - if models.IsErrIssueNotExist(err) { - ctx.Error(404, "GetIssueByIndex") - } else { - ctx.Handle(500, "GetIssueByIndex", err) - } + ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err) + return nil + } + if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) || + !issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) { + ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil) + return nil + } + if err = issue.LoadAttributes(); err != nil { + ctx.Handle(500, "LoadAttributes", nil) return nil } return issue @@ -749,6 +754,19 @@ func getActionIssues(ctx *context.Context) []*models.Issue { ctx.Handle(500, "GetIssuesByIDs", err) return nil } + // Check access rights for all issues + issueUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) + prUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) + for _, issue := range issues { + if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled { + ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil) + return nil + } + if err = issue.LoadAttributes(); err != nil { + ctx.Handle(500, "LoadAttributes", nil) + return nil + } + } return issues } @@ -884,9 +902,8 @@ func UpdateIssueStatus(ctx *context.Context) { // NewComment create a comment for issue func NewComment(ctx *context.Context, form auth.CreateCommentForm) { - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - if err != nil { - ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err) + issue := GetActionIssue(ctx) + if ctx.Written() { return } @@ -913,7 +930,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { if form.Status == "reopen" && issue.IsPull { pull := issue.PullRequest - pr, err = models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch) + pr, err := models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch) if err != nil { if !models.IsErrPullRequestNotExist(err) { ctx.Handle(500, "GetUnmergedPullRequest", err) @@ -935,7 +952,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { if pr != nil { ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index)) } else { - if err = issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil { + if err := issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil { log.Error(4, "ChangeStatus: %v", err) } else { log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) @@ -962,7 +979,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { return } - comment, err = models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments) + comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments) if err != nil { ctx.Handle(500, "CreateIssueComment", err) return @@ -1032,10 +1049,6 @@ func DeleteComment(ctx *context.Context) { // Milestones render milestones page func Milestones(ctx *context.Context) { - MustEnableIssues(ctx) - if ctx.Written() { - return - } ctx.Data["Title"] = ctx.Tr("repo.milestones") ctx.Data["PageIsIssueList"] = true ctx.Data["PageIsMilestones"] = true diff --git a/routers/repo/issue_label.go b/routers/repo/issue_label.go index 3422677943..9b4da4b500 100644 --- a/routers/repo/issue_label.go +++ b/routers/repo/issue_label.go @@ -18,10 +18,6 @@ const ( // Labels render issue's labels page func Labels(ctx *context.Context) { - MustEnableIssues(ctx) - if ctx.Written() { - return - } ctx.Data["Title"] = ctx.Tr("repo.labels") ctx.Data["PageIsIssueList"] = true ctx.Data["PageIsLabels"] = true diff --git a/routers/repo/issue_stopwatch.go b/routers/repo/issue_stopwatch.go index 7e3121da9f..f4392849aa 100644 --- a/routers/repo/issue_stopwatch.go +++ b/routers/repo/issue_stopwatch.go @@ -13,11 +13,12 @@ import ( // IssueStopwatch creates or stops a stopwatch for the given issue. func IssueStopwatch(c *context.Context) { - issueIndex := c.ParamsInt64("index") - issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex) - - if err != nil { - c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err) + issue := GetActionIssue(c) + if c.Written() { + return + } + if !c.Repo.CanUseTimetracker(issue, c.User) { + c.Handle(http.StatusNotFound, "CanUseTimetracker", nil) return } @@ -32,11 +33,12 @@ func IssueStopwatch(c *context.Context) { // CancelStopwatch cancel the stopwatch func CancelStopwatch(c *context.Context) { - issueIndex := c.ParamsInt64("index") - issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex) - - if err != nil { - c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err) + issue := GetActionIssue(c) + if c.Written() { + return + } + if !c.Repo.CanUseTimetracker(issue, c.User) { + c.Handle(http.StatusNotFound, "CanUseTimetracker", nil) return } diff --git a/routers/repo/issue_timetrack.go b/routers/repo/issue_timetrack.go index 4d77ca3cea..d89c67b497 100644 --- a/routers/repo/issue_timetrack.go +++ b/routers/repo/issue_timetrack.go @@ -15,14 +15,12 @@ import ( // AddTimeManually tracks time manually func AddTimeManually(c *context.Context, form auth.AddTimeManuallyForm) { - issueIndex := c.ParamsInt64("index") - issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex) - if err != nil { - if models.IsErrIssueNotExist(err) { - c.Handle(http.StatusNotFound, "GetIssueByIndex", err) - return - } - c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err) + issue := GetActionIssue(c) + if c.Written() { + return + } + if !c.Repo.CanUseTimetracker(issue, c.User) { + c.Handle(http.StatusNotFound, "CanUseTimetracker", nil) return } url := issue.HTMLURL() diff --git a/routers/repo/issue_watch.go b/routers/repo/issue_watch.go index 382798025e..42ffaec5b8 100644 --- a/routers/repo/issue_watch.go +++ b/routers/repo/issue_watch.go @@ -21,10 +21,8 @@ func IssueWatch(c *context.Context) { return } - issueIndex := c.ParamsInt64("index") - issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex) - if err != nil { - c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err) + issue := GetActionIssue(c) + if c.Written() { return } @@ -33,6 +31,6 @@ func IssueWatch(c *context.Context) { return } - url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issueIndex) + url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index) c.Redirect(url, http.StatusSeeOther) } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 9d9e656433..a6f73aaedd 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -474,12 +474,13 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/:username/:reponame/action/:action", reqSignIn, context.RepoAssignment(), repo.Action) m.Group("/:username/:reponame", func() { - // FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest. - // So they can apply their own enable/disable logic on routers. m.Group("/issues", func() { m.Combo("/new").Get(context.RepoRef(), repo.NewIssue). Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost) - + }, context.CheckUnit(models.UnitTypeIssues)) + // FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest. + // So they can apply their own enable/disable logic on routers. + m.Group("/issues", func() { m.Group("/:index", func() { m.Post("/title", repo.UpdateIssueTitle) m.Post("/content", repo.UpdateIssueContent) @@ -491,30 +492,24 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/toggle", repo.IssueStopwatch) m.Post("/cancel", repo.CancelStopwatch) }) - - }, func(ctx *context.Context) { - if !ctx.Repo.CanUseTimetracker(repo.GetActionIssue(ctx), ctx.User) { - ctx.Handle(404, ctx.Req.RequestURI, nil) - return - } }) }) - m.Post("/labels", repo.UpdateIssueLabel, reqRepoWriter) - m.Post("/milestone", repo.UpdateIssueMilestone, reqRepoWriter) - m.Post("/assignee", repo.UpdateIssueAssignee, reqRepoWriter) - m.Post("/status", repo.UpdateIssueStatus, reqRepoWriter) - }, context.CheckUnit(models.UnitTypeIssues)) + m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel) + m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone) + m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee) + m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus) + }) m.Group("/comments/:id", func() { m.Post("", repo.UpdateCommentContent) m.Post("/delete", repo.DeleteComment) - }, context.CheckUnit(models.UnitTypeIssues)) + }, context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) m.Group("/labels", func() { m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel) m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel) m.Post("/delete", repo.DeleteLabel) m.Post("/initialize", bindIgnErr(auth.InitializeLabelsForm{}), repo.InitializeLabels) - }, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues)) + }, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) m.Group("/milestones", func() { m.Combo("/new").Get(repo.NewMilestone). Post(bindIgnErr(auth.CreateMilestoneForm{}), repo.NewMilestonePost) @@ -522,7 +517,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/:id/edit", bindIgnErr(auth.CreateMilestoneForm{}), repo.EditMilestonePost) m.Get("/:id/:action", repo.ChangeMilestonStatus) m.Post("/delete", repo.DeleteMilestone) - }, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues)) + }, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists). Get(repo.CompareAndPullRequest). @@ -593,8 +588,8 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("", func() { m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues) m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue) - m.Get("/labels/", repo.RetrieveLabels, repo.Labels) - m.Get("/milestones", repo.Milestones) + m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels) + m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones) }, context.RepoRef()) m.Group("/wiki", func() { diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index 5c5a574976..711d70f2a0 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -103,8 +103,8 @@
-
{{.i18n.Tr "repo.issues.action_open"}}
-
{{.i18n.Tr "repo.issues.action_close"}}
+
{{.i18n.Tr "repo.issues.action_open"}}
+
{{.i18n.Tr "repo.issues.action_close"}}
@@ -116,7 +116,7 @@
{{range .Labels}} -
+
{{.Name | Sanitize}}
{{end}} @@ -134,7 +134,7 @@ {{.i18n.Tr "repo.issues.action_milestone_no_select"}}
{{range .Milestones}} -
+
{{.Name | Sanitize}}
{{end}} @@ -152,7 +152,7 @@ {{.i18n.Tr "repo.issues.action_assignee_no_select"}}
{{range .Assignees}} -
+
{{.Name}}
{{end}}