Skip to content

Commit f96c1a2

Browse files
lunnyzeripathlafriks
committed
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 <[email protected]> Co-authored-by: Lauris BH <[email protected]>
1 parent ce756ee commit f96c1a2

File tree

11 files changed

+43
-28
lines changed

11 files changed

+43
-28
lines changed

‎modules/context/repo.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ func (r *Repository) CanUseTimetracker(issue *models.Issue, user *models.User) b
9191
// 2. Is the user a contributor, admin, poster or assignee and do the repository policies require this?
9292
isAssigned, _ := models.IsUserAssignedToIssue(issue, user)
9393
return r.Repository.IsTimetrackerEnabled() && (!r.Repository.AllowOnlyContributorsToTrackTime() ||
94-
r.Permission.CanWrite(models.UnitTypeIssues) || issue.IsPoster(user.ID) || isAssigned)
94+
r.Permission.CanWriteIssuesOrPulls(issue.IsPull) || issue.IsPoster(user.ID) || isAssigned)
9595
}
9696

9797
// CanCreateIssueDependencies returns whether or not a user can create dependencies.

‎modules/repofiles/action.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*m
103103
refMarked[key] = true
104104

105105
// FIXME: this kind of condition is all over the code, it should be consolidated in a single place
106-
canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWrite(models.UnitTypeIssues) || refIssue.PosterID == doer.ID
107-
cancomment := canclose || perm.CanRead(models.UnitTypeIssues)
106+
canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWriteIssuesOrPulls(refIssue.IsPull) || refIssue.PosterID == doer.ID
107+
cancomment := canclose || perm.CanReadIssuesOrPulls(refIssue.IsPull)
108108

109109
// Don't proceed if the user can't comment
110110
if !cancomment {

‎routers/api/v1/repo/issue.go‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,10 @@ func ListIssues(ctx *context.APIContext) {
207207
// in: query
208208
// description: search string
209209
// type: string
210+
// - name: type
211+
// in: query
212+
// description: filter by type (issues / pulls) if set
213+
// type: string
210214
// responses:
211215
// "200":
212216
// "$ref": "#/responses/IssueList"
@@ -242,6 +246,16 @@ func ListIssues(ctx *context.APIContext) {
242246
}
243247
}
244248

249+
var isPull util.OptionalBool
250+
switch ctx.Query("type") {
251+
case "pulls":
252+
isPull = util.OptionalBoolTrue
253+
case "issues":
254+
isPull = util.OptionalBoolFalse
255+
default:
256+
isPull = util.OptionalBoolNone
257+
}
258+
245259
// Only fetch the issues if we either don't have a keyword or the search returned issues
246260
// This would otherwise return all issues if no issues were found by the search.
247261
if len(keyword) == 0 || len(issueIDs) > 0 || len(labelIDs) > 0 {
@@ -252,6 +266,7 @@ func ListIssues(ctx *context.APIContext) {
252266
IsClosed: isClosed,
253267
IssueIDs: issueIDs,
254268
LabelIDs: labelIDs,
269+
IsPull: isPull,
255270
})
256271
}
257272

@@ -476,14 +491,15 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
476491
return
477492
}
478493
issue.Repo = ctx.Repo.Repository
494+
canWrite := ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)
479495

480496
err = issue.LoadAttributes()
481497
if err != nil {
482498
ctx.Error(http.StatusInternalServerError, "LoadAttributes", err)
483499
return
484500
}
485501

486-
if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypeIssues) {
502+
if !issue.IsPoster(ctx.User.ID) && !canWrite {
487503
ctx.Status(http.StatusForbidden)
488504
return
489505
}
@@ -496,7 +512,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
496512
}
497513

498514
// Update or remove the deadline, only if set and allowed
499-
if (form.Deadline != nil || form.RemoveDeadline != nil) && ctx.Repo.CanWrite(models.UnitTypeIssues) {
515+
if (form.Deadline != nil || form.RemoveDeadline != nil) && canWrite {
500516
var deadlineUnix timeutil.TimeStamp
501517

502518
if (form.RemoveDeadline == nil || !*form.RemoveDeadline) && !form.Deadline.IsZero() {
@@ -520,7 +536,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
520536
// Pass one or more user logins to replace the set of assignees on this Issue.
521537
// Send an empty array ([]) to clear all assignees from the Issue.
522538

523-
if ctx.Repo.CanWrite(models.UnitTypeIssues) && (form.Assignees != nil || form.Assignee != nil) {
539+
if canWrite && (form.Assignees != nil || form.Assignee != nil) {
524540
oneAssignee := ""
525541
if form.Assignee != nil {
526542
oneAssignee = *form.Assignee
@@ -533,7 +549,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
533549
}
534550
}
535551

536-
if ctx.Repo.CanWrite(models.UnitTypeIssues) && form.Milestone != nil &&
552+
if canWrite && form.Milestone != nil &&
537553
issue.MilestoneID != *form.Milestone {
538554
oldMilestoneID := issue.MilestoneID
539555
issue.MilestoneID = *form.Milestone
@@ -619,7 +635,7 @@ func UpdateIssueDeadline(ctx *context.APIContext, form api.EditDeadlineOption) {
619635
return
620636
}
621637

622-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
638+
if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
623639
ctx.Error(http.StatusForbidden, "", "Not repo writer")
624640
return
625641
}

‎routers/api/v1/repo/issue_comment.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func CreateIssueComment(ctx *context.APIContext, form api.CreateIssueCommentOpti
190190
return
191191
}
192192

193-
if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin {
193+
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin {
194194
ctx.Error(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked")))
195195
return
196196
}

‎routers/api/v1/repo/issue_reaction.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp
179179
ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err)
180180
}
181181

182-
if comment.Issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) {
182+
if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) {
183183
ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction"))
184184
return
185185
}
@@ -380,7 +380,7 @@ func changeIssueReaction(ctx *context.APIContext, form api.EditReactionOption, i
380380
return
381381
}
382382

383-
if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) {
383+
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
384384
ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction"))
385385
return
386386
}

‎routers/api/v1/repo/issue_stopwatch.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*models.I
170170
return nil, err
171171
}
172172

173-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
173+
if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
174174
ctx.Status(http.StatusForbidden)
175175
return nil, err
176176
}

‎routers/repo/issue.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,12 @@ var (
6363
// If locked and user has permissions to write to the repository,
6464
// then the comment is allowed, else it is blocked
6565
func MustAllowUserComment(ctx *context.Context) {
66-
6766
issue := GetActionIssue(ctx)
6867
if ctx.Written() {
6968
return
7069
}
7170

72-
if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin {
71+
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin {
7372
ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked"))
7473
ctx.Redirect(issue.HTMLURL())
7574
return
@@ -349,7 +348,7 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repos
349348

350349
// RetrieveRepoMetas find all the meta information of a repository
351350
func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull bool) []*models.Label {
352-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
351+
if !ctx.Repo.CanWriteIssuesOrPulls(isPull) {
353352
return nil
354353
}
355354

@@ -1006,7 +1005,6 @@ func ViewIssue(ctx *context.Context) {
10061005
ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID)
10071006
ctx.Data["IsIssueWriter"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)
10081007
ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.User.IsAdmin)
1009-
ctx.Data["IsRepoIssuesWriter"] = ctx.IsSigned && (ctx.Repo.CanWrite(models.UnitTypeIssues) || ctx.User.IsAdmin)
10101008
ctx.Data["LockReasons"] = setting.Repository.Issue.LockReasons
10111009
ctx.HTML(200, tplIssueView)
10121010
}
@@ -1267,9 +1265,10 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
12671265
}
12681266

12691267
ctx.Error(403)
1268+
return
12701269
}
12711270

1272-
if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin {
1271+
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin {
12731272
ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked"))
12741273
ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther)
12751274
return

‎routers/repo/issue_dependency.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,6 @@ func RemoveDependency(ctx *context.Context) {
9393
return
9494
}
9595

96-
// Redirect
97-
ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther)
98-
9996
// Dependency Type
10097
depTypeStr := ctx.Req.PostForm.Get("dependencyType")
10198

@@ -126,4 +123,7 @@ func RemoveDependency(ctx *context.Context) {
126123
ctx.ServerError("RemoveIssueDependency", err)
127124
return
128125
}
126+
127+
// Redirect
128+
ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther)
129129
}

‎routers/routes/routes.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -503,19 +503,13 @@ func RegisterRoutes(m *macaron.Macaron) {
503503
reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases)
504504
reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases)
505505
reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki)
506+
reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues)
506507
reqRepoIssueReader := context.RequireRepoReader(models.UnitTypeIssues)
507508
reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests)
508509
reqRepoPullsReader := context.RequireRepoReader(models.UnitTypePullRequests)
509510
reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests)
510511
reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests)
511512

512-
reqRepoIssueWriter := func(ctx *context.Context) {
513-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
514-
ctx.Error(403)
515-
return
516-
}
517-
}
518-
519513
// ***** START: Organization *****
520514
m.Group("/org", func() {
521515
m.Group("", func() {

‎templates/repo/issue/view_content.tmpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@
6666
{{ template "repo/issue/view_content/pull". }}
6767
{{end}}
6868
{{if .IsSigned}}
69-
{{ if and (or .IsRepoAdmin .IsRepoIssuesWriter (or (not .Issue.IsLocked))) (not .Repository.IsArchived) }}
69+
{{ if and (or .IsRepoAdmin .IsIssueWriter (or (not .Issue.IsLocked))) (not .Repository.IsArchived) }}
7070
<div class="comment form">
7171
<a class="avatar" href="{{.SignedUser.HomeLink}}">
7272
<img src="{{.SignedUser.RelAvatarLink}}">

0 commit comments

Comments
 (0)