From c4e0f717e78e216c5ac6c48b065022b524f24047 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 20 Jan 2020 23:45:42 +0800 Subject: [PATCH] =?UTF-8?q?fix=20bug=20about=20wrong=20dependencies=20perm?= =?UTF-8?q?issions=20check=20and=20other=20wr=E2=80=A6=20(#9884)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix bug about wrong dependencies permissions check and other wrong permissions check * improve code --- modules/context/repo.go | 6 ++-- public/js/index.js | 3 +- routers/api/v1/repo/issue.go | 30 ++++++++++++---- routers/api/v1/repo/issue_comment.go | 2 +- routers/repo/compare.go | 2 +- routers/repo/issue.go | 25 ++++++++----- routers/repo/issue_dependency.go | 36 +++++++++---------- routers/routes/routes.go | 8 +---- templates/repo/issue/view_content.tmpl | 2 +- .../repo/issue/view_content/sidebar.tmpl | 1 + templates/swagger/v1_json.tmpl | 6 ++++ 11 files changed, 73 insertions(+), 48 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 1b57b42006..59b82b6b07 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -91,12 +91,12 @@ func (r *Repository) CanUseTimetracker(issue *models.Issue, user *models.User) b // 2. Is the user a contributor, admin, poster or assignee and do the repository policies require this? isAssigned, _ := models.IsUserAssignedToIssue(issue, user) return r.Repository.IsTimetrackerEnabled() && (!r.Repository.AllowOnlyContributorsToTrackTime() || - r.Permission.CanWrite(models.UnitTypeIssues) || issue.IsPoster(user.ID) || isAssigned) + r.Permission.CanWriteIssuesOrPulls(issue.IsPull) || issue.IsPoster(user.ID) || isAssigned) } // CanCreateIssueDependencies returns whether or not a user can create dependencies. -func (r *Repository) CanCreateIssueDependencies(user *models.User) bool { - return r.Permission.CanWrite(models.UnitTypeIssues) && r.Repository.IsDependenciesEnabled() +func (r *Repository) CanCreateIssueDependencies(user *models.User, isPull bool) bool { + return r.Repository.IsDependenciesEnabled() && r.Permission.CanWriteIssuesOrPulls(isPull) } // GetCommitsCount returns cached commit count for current view diff --git a/public/js/index.js b/public/js/index.js index 966fc05eff..c04af8cbe4 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -3134,10 +3134,11 @@ function deleteDependencyModal(id, type) { function initIssueList() { const repolink = $('#repolink').val(); + const tp = $('#type').val(); $('#new-dependency-drop-list') .dropdown({ apiSettings: { - url: suburl + '/api/v1/repos/' + repolink + '/issues?q={query}', + url: suburl + '/api/v1/repos/' + repolink + '/issues?q={query}&type='+tp, onResponse: function(response) { const filteredResponse = {'success': true, 'results': []}; const currIssueId = $('#new-dependency-drop-list').data('issue-id'); diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 756b2c35f2..256c347588 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -57,6 +57,10 @@ func ListIssues(ctx *context.APIContext) { // in: query // description: search string // type: string + // - name: type + // in: query + // description: filter by type (issues / pulls) if set + // type: string // responses: // "200": // "$ref": "#/responses/IssueList" @@ -91,6 +95,16 @@ func ListIssues(ctx *context.APIContext) { } } + var isPull util.OptionalBool + switch ctx.Query("type") { + case "pulls": + isPull = util.OptionalBoolTrue + case "issues": + isPull = util.OptionalBoolFalse + default: + isPull = util.OptionalBoolNone + } + // Only fetch the issues if we either don't have a keyword or the search returned issues // This would otherwise return all issues if no issues were found by the search. if len(keyword) == 0 || len(issueIDs) > 0 || len(labelIDs) > 0 { @@ -101,6 +115,7 @@ func ListIssues(ctx *context.APIContext) { IsClosed: isClosed, IssueIDs: issueIDs, LabelIDs: labelIDs, + IsPull: isPull, }) } @@ -292,6 +307,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { return } issue.Repo = ctx.Repo.Repository + canWrite := ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) err = issue.LoadAttributes() if err != nil { @@ -299,7 +315,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { return } - if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !issue.IsPoster(ctx.User.ID) && !canWrite { ctx.Status(403) return } @@ -312,7 +328,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { } // Update the deadline - if form.Deadline != nil && ctx.Repo.CanWrite(models.UnitTypeIssues) { + if form.Deadline != nil && canWrite { deadlineUnix := timeutil.TimeStamp(form.Deadline.Unix()) if err := models.UpdateIssueDeadline(issue, deadlineUnix, ctx.User); err != nil { ctx.Error(500, "UpdateIssueDeadline", err) @@ -329,7 +345,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { // Pass one or more user logins to replace the set of assignees on this Issue. // Send an empty array ([]) to clear all assignees from the Issue. - if ctx.Repo.CanWrite(models.UnitTypeIssues) && (form.Assignees != nil || form.Assignee != nil) { + if canWrite && (form.Assignees != nil || form.Assignee != nil) { oneAssignee := "" if form.Assignee != nil { oneAssignee = *form.Assignee @@ -342,7 +358,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { } } - if ctx.Repo.CanWrite(models.UnitTypeIssues) && form.Milestone != nil && + if canWrite && form.Milestone != nil && issue.MilestoneID != *form.Milestone { oldMilestoneID := issue.MilestoneID issue.MilestoneID = *form.Milestone @@ -430,7 +446,7 @@ func UpdateIssueDeadline(ctx *context.APIContext, form api.EditDeadlineOption) { return } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Status(403) return } @@ -497,7 +513,7 @@ func StartIssueStopwatch(ctx *context.APIContext) { return } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Status(403) return } @@ -566,7 +582,7 @@ func StopIssueStopwatch(ctx *context.APIContext) { return } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Status(403) return } diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 60796031a5..fca07ff213 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -185,7 +185,7 @@ func CreateIssueComment(ctx *context.APIContext, form api.CreateIssueCommentOpti return } - if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin { ctx.Error(403, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked"))) return } diff --git a/routers/repo/compare.go b/routers/repo/compare.go index 832f976760..a5faf5b429 100644 --- a/routers/repo/compare.go +++ b/routers/repo/compare.go @@ -407,7 +407,7 @@ func CompareDiff(ctx *context.Context) { if !nothingToCompare { // Setup information for new form. - RetrieveRepoMetas(ctx, ctx.Repo.Repository) + RetrieveRepoMetas(ctx, ctx.Repo.Repository, true) if ctx.Written() { return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 8cbad47f2d..8e44b363d9 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -67,7 +67,7 @@ func MustAllowUserComment(ctx *context.Context) { return } - if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin { ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked")) ctx.Redirect(issue.HTMLURL()) return @@ -346,8 +346,8 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repos } // RetrieveRepoMetas find all the meta information of a repository -func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository) []*models.Label { - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { +func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull bool) []*models.Label { + if !ctx.Repo.CanWriteIssuesOrPulls(isPull) { return nil } @@ -371,7 +371,7 @@ func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository) []*models. ctx.Data["Branches"] = brs // Contains true if the user can create issue dependencies - ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx.User) + ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx.User, isPull) return labels } @@ -441,7 +441,7 @@ func NewIssue(ctx *context.Context) { setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates) renderAttachmentSettings(ctx) - RetrieveRepoMetas(ctx, ctx.Repo.Repository) + RetrieveRepoMetas(ctx, ctx.Repo.Repository, false) if ctx.Written() { return } @@ -456,7 +456,7 @@ func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm, isPull b err error ) - labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository) + labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository, isPull) if ctx.Written() { return nil, nil, 0 } @@ -776,8 +776,16 @@ func ViewIssue(ctx *context.Context) { } } + if issue.IsPull && !ctx.Repo.CanRead(models.UnitTypeIssues) { + ctx.Data["IssueType"] = "pulls" + } else if !issue.IsPull && !ctx.Repo.CanRead(models.UnitTypePullRequests) { + ctx.Data["IssueType"] = "issues" + } else { + ctx.Data["IssueType"] = "all" + } + // Check if the user can use the dependencies - ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx.User) + ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx.User, issue.IsPull) // Render comments and and fetch participants. participants[0] = issue.Poster @@ -963,7 +971,6 @@ func ViewIssue(ctx *context.Context) { ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID) ctx.Data["IsIssueWriter"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.User.IsAdmin) - ctx.Data["IsRepoIssuesWriter"] = ctx.IsSigned && (ctx.Repo.CanWrite(models.UnitTypeIssues) || ctx.User.IsAdmin) ctx.Data["LockReasons"] = setting.Repository.Issue.LockReasons ctx.HTML(200, tplIssueView) } @@ -1208,7 +1215,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { ctx.Error(403) } - if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin { ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked")) ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) return diff --git a/routers/repo/issue_dependency.go b/routers/repo/issue_dependency.go index 730271126d..5a661f54cd 100644 --- a/routers/repo/issue_dependency.go +++ b/routers/repo/issue_dependency.go @@ -14,14 +14,6 @@ import ( // AddDependency adds new dependencies func AddDependency(ctx *context.Context) { - // Check if the Repo is allowed to have dependencies - if !ctx.Repo.CanCreateIssueDependencies(ctx.User) { - ctx.Error(http.StatusForbidden, "CanCreateIssueDependencies") - return - } - - depID := ctx.QueryInt64("newDependency") - issueIndex := ctx.ParamsInt64("index") issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, issueIndex) if err != nil { @@ -29,6 +21,14 @@ func AddDependency(ctx *context.Context) { return } + // Check if the Repo is allowed to have dependencies + if !ctx.Repo.CanCreateIssueDependencies(ctx.User, issue.IsPull) { + ctx.Error(http.StatusForbidden, "CanCreateIssueDependencies") + return + } + + depID := ctx.QueryInt64("newDependency") + // Redirect defer ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issueIndex), http.StatusSeeOther) @@ -68,14 +68,6 @@ func AddDependency(ctx *context.Context) { // RemoveDependency removes the dependency func RemoveDependency(ctx *context.Context) { - // Check if the Repo is allowed to have dependencies - if !ctx.Repo.CanCreateIssueDependencies(ctx.User) { - ctx.Error(http.StatusForbidden, "CanCreateIssueDependencies") - return - } - - depID := ctx.QueryInt64("removeDependencyID") - issueIndex := ctx.ParamsInt64("index") issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, issueIndex) if err != nil { @@ -83,8 +75,13 @@ func RemoveDependency(ctx *context.Context) { return } - // Redirect - ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issueIndex), http.StatusSeeOther) + // Check if the Repo is allowed to have dependencies + if !ctx.Repo.CanCreateIssueDependencies(ctx.User, issue.IsPull) { + ctx.Error(http.StatusForbidden, "CanCreateIssueDependencies") + return + } + + depID := ctx.QueryInt64("removeDependencyID") // Dependency Type depTypeStr := ctx.Req.PostForm.Get("dependencyType") @@ -116,4 +113,7 @@ func RemoveDependency(ctx *context.Context) { ctx.ServerError("RemoveIssueDependency", err) return } + + // Redirect + ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issueIndex), http.StatusSeeOther) } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index d38de7014c..c02b73212f 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -532,18 +532,12 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases) reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) reqRepoIssueReader := context.RequireRepoReader(models.UnitTypeIssues) + reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues) reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests) reqRepoPullsReader := context.RequireRepoReader(models.UnitTypePullRequests) reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests) reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests) - reqRepoIssueWriter := func(ctx *context.Context) { - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { - ctx.Error(403) - return - } - } - // ***** START: Organization ***** m.Group("/org", func() { m.Group("", func() { diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index a68f4e4d71..555f34679e 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -78,7 +78,7 @@ {{ template "repo/issue/view_content/pull". }} {{end}} {{if .IsSigned}} - {{ if and (or .IsRepoAdmin .IsRepoIssuesWriter (or (not .Issue.IsLocked))) (not .Repository.IsArchived) }} + {{ if and (or .IsRepoAdmin .IsIssuesWriter (or (not .Issue.IsLocked))) (not .Repository.IsArchived) }}
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index c42d8aff7f..01809b9fd3 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -426,6 +426,7 @@ +
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 561a67e5d2..a80d81cbc3 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2777,6 +2777,12 @@ "description": "search string", "name": "q", "in": "query" + }, + { + "type": "string", + "description": "filter by type (issues / pulls) if set", + "name": "type", + "in": "query" } ], "responses": {