From f96c1a2c79a86a157f5eb697a5f1ac89ee33267f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 20 Jan 2020 23:59:33 +0800 Subject: [PATCH] Fix wrong permissions check when issues/prs shared operations (#9885) (#9889) * Fix wrong permissions check when issues/prs shared operations * move redirect to the last of the function * fix swagger Co-authored-by: zeripath Co-authored-by: Lauris BH --- modules/context/repo.go | 2 +- modules/repofiles/action.go | 4 ++-- routers/api/v1/repo/issue.go | 26 +++++++++++++++++++++----- routers/api/v1/repo/issue_comment.go | 2 +- routers/api/v1/repo/issue_reaction.go | 4 ++-- routers/api/v1/repo/issue_stopwatch.go | 2 +- routers/repo/issue.go | 9 ++++----- routers/repo/issue_dependency.go | 6 +++--- routers/routes/routes.go | 8 +------- templates/repo/issue/view_content.tmpl | 2 +- templates/swagger/v1_json.tmpl | 6 ++++++ 11 files changed, 43 insertions(+), 28 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index af2b3902a..d0a982646 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -91,7 +91,7 @@ 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. diff --git a/modules/repofiles/action.go b/modules/repofiles/action.go index 5fe933359..29438a880 100644 --- a/modules/repofiles/action.go +++ b/modules/repofiles/action.go @@ -103,8 +103,8 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*m refMarked[key] = true // FIXME: this kind of condition is all over the code, it should be consolidated in a single place - canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(models.UnitTypeIssues) || refIssue.PosterID == doer.ID - cancomment := canclose || perm.CanRead(models.UnitTypeIssues) + canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWriteIssuesOrPulls(refIssue.IsPull) || refIssue.PosterID == doer.ID + cancomment := canclose || perm.CanReadIssuesOrPulls(refIssue.IsPull) // Don't proceed if the user can't comment if !cancomment { diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 7440f65be..e00c24ea3 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -207,6 +207,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" @@ -242,6 +246,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 { @@ -252,6 +266,7 @@ func ListIssues(ctx *context.APIContext) { IsClosed: isClosed, IssueIDs: issueIDs, LabelIDs: labelIDs, + IsPull: isPull, }) } @@ -476,6 +491,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 { @@ -483,7 +499,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(http.StatusForbidden) return } @@ -496,7 +512,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { } // Update or remove the deadline, only if set and allowed - if (form.Deadline != nil || form.RemoveDeadline != nil) && ctx.Repo.CanWrite(models.UnitTypeIssues) { + if (form.Deadline != nil || form.RemoveDeadline != nil) && canWrite { var deadlineUnix timeutil.TimeStamp if (form.RemoveDeadline == nil || !*form.RemoveDeadline) && !form.Deadline.IsZero() { @@ -520,7 +536,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 @@ -533,7 +549,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 @@ -619,7 +635,7 @@ func UpdateIssueDeadline(ctx *context.APIContext, form api.EditDeadlineOption) { return } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Error(http.StatusForbidden, "", "Not repo writer") return } diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index c13fc93cd..0e4ed6827 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -190,7 +190,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(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked"))) return } diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index b943ea698..f546c2746 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/routers/api/v1/repo/issue_reaction.go @@ -179,7 +179,7 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err) } - if comment.Issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) { ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction")) return } @@ -380,7 +380,7 @@ func changeIssueReaction(ctx *context.APIContext, form api.EditReactionOption, i return } - if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction")) return } diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go index 3ffdf2440..3b7c20d4d 100644 --- a/routers/api/v1/repo/issue_stopwatch.go +++ b/routers/api/v1/repo/issue_stopwatch.go @@ -170,7 +170,7 @@ func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*models.I return nil, err } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Status(http.StatusForbidden) return nil, err } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 52f21b16a..f69e0bb60 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -63,13 +63,12 @@ var ( // If locked and user has permissions to write to the repository, // then the comment is allowed, else it is blocked func MustAllowUserComment(ctx *context.Context) { - issue := GetActionIssue(ctx) if ctx.Written() { 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 @@ -349,7 +348,7 @@ 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, isPull bool) []*models.Label { - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + if !ctx.Repo.CanWriteIssuesOrPulls(isPull) { return nil } @@ -1006,7 +1005,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) } @@ -1267,9 +1265,10 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { } ctx.Error(403) + 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(), http.StatusSeeOther) return diff --git a/routers/repo/issue_dependency.go b/routers/repo/issue_dependency.go index 055b5ed2a..8a83c7bae 100644 --- a/routers/repo/issue_dependency.go +++ b/routers/repo/issue_dependency.go @@ -93,9 +93,6 @@ func RemoveDependency(ctx *context.Context) { return } - // Redirect - ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) - // Dependency Type depTypeStr := ctx.Req.PostForm.Get("dependencyType") @@ -126,4 +123,7 @@ func RemoveDependency(ctx *context.Context) { ctx.ServerError("RemoveIssueDependency", err) return } + + // Redirect + ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 58a2da82f..08cc52e0e 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -503,19 +503,13 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases) reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases) reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) + reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues) reqRepoIssueReader := context.RequireRepoReader(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 bee8a3dfe..d336b7804 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -66,7 +66,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 .IsIssueWriter (or (not .Issue.IsLocked))) (not .Repository.IsArchived) }}
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index fad47731f..731d2d36a 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2909,6 +2909,12 @@ "description": "search string", "name": "q", "in": "query" + }, + { + "type": "string", + "description": "filter by type (issues / pulls) if set", + "name": "type", + "in": "query" } ], "responses": {