Skip to content

Commit 6d6f1d5

Browse files
lunnyzeripathlafriks
authored and
sapk
committed
Fix wrong permissions check when issues/prs shared operations (#9885)
* 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 81cfe24 commit 6d6f1d5

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
@@ -134,7 +134,7 @@ func (r *Repository) CanUseTimetracker(issue *models.Issue, user *models.User) b
134134
// 2. Is the user a contributor, admin, poster or assignee and do the repository policies require this?
135135
isAssigned, _ := models.IsUserAssignedToIssue(issue, user)
136136
return r.Repository.IsTimetrackerEnabled() && (!r.Repository.AllowOnlyContributorsToTrackTime() ||
137-
r.Permission.CanWrite(models.UnitTypeIssues) || issue.IsPoster(user.ID) || isAssigned)
137+
r.Permission.CanWriteIssuesOrPulls(issue.IsPull) || issue.IsPoster(user.ID) || isAssigned)
138138
}
139139

140140
// 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
@@ -104,8 +104,8 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*r
104104
refMarked[key] = true
105105

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

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

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

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

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

@@ -475,14 +490,15 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
475490
return
476491
}
477492
issue.Repo = ctx.Repo.Repository
493+
canWrite := ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)
478494

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

485-
if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypeIssues) {
501+
if !issue.IsPoster(ctx.User.ID) && !canWrite {
486502
ctx.Status(http.StatusForbidden)
487503
return
488504
}
@@ -495,7 +511,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
495511
}
496512

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

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

522-
if ctx.Repo.CanWrite(models.UnitTypeIssues) && (form.Assignees != nil || form.Assignee != nil) {
538+
if canWrite && (form.Assignees != nil || form.Assignee != nil) {
523539
oneAssignee := ""
524540
if form.Assignee != nil {
525541
oneAssignee = *form.Assignee
@@ -532,7 +548,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
532548
}
533549
}
534550

535-
if ctx.Repo.CanWrite(models.UnitTypeIssues) && form.Milestone != nil &&
551+
if canWrite && form.Milestone != nil &&
536552
issue.MilestoneID != *form.Milestone {
537553
oldMilestoneID := issue.MilestoneID
538554
issue.MilestoneID = *form.Milestone
@@ -618,7 +634,7 @@ func UpdateIssueDeadline(ctx *context.APIContext, form api.EditDeadlineOption) {
618634
return
619635
}
620636

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ func CreateIssueComment(ctx *context.APIContext, form api.CreateIssueCommentOpti
204204
return
205205
}
206206

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

‎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
@@ -344,7 +343,7 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repos
344343

345344
// RetrieveRepoMetas find all the meta information of a repository
346345
func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull bool) []*models.Label {
347-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
346+
if !ctx.Repo.CanWriteIssuesOrPulls(isPull) {
348347
return nil
349348
}
350349

@@ -1022,7 +1021,6 @@ func ViewIssue(ctx *context.Context) {
10221021
ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID)
10231022
ctx.Data["IsIssueWriter"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)
10241023
ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.User.IsAdmin)
1025-
ctx.Data["IsRepoIssuesWriter"] = ctx.IsSigned && (ctx.Repo.CanWrite(models.UnitTypeIssues) || ctx.User.IsAdmin)
10261024
ctx.Data["LockReasons"] = setting.Repository.Issue.LockReasons
10271025
ctx.HTML(200, tplIssueView)
10281026
}
@@ -1283,9 +1281,10 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
12831281
}
12841282

12851283
ctx.Error(403)
1284+
return
12861285
}
12871286

1288-
if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin {
1287+
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.User.IsAdmin {
12891288
ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked"))
12901289
ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther)
12911290
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
@@ -508,19 +508,13 @@ func RegisterRoutes(m *macaron.Macaron) {
508508
reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases)
509509
reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases)
510510
reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki)
511+
reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues)
511512
reqRepoIssueReader := context.RequireRepoReader(models.UnitTypeIssues)
512513
reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests)
513514
reqRepoPullsReader := context.RequireRepoReader(models.UnitTypePullRequests)
514515
reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests)
515516
reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests)
516517

517-
reqRepoIssueWriter := func(ctx *context.Context) {
518-
if !ctx.Repo.CanWrite(models.UnitTypeIssues) {
519-
ctx.Error(403)
520-
return
521-
}
522-
}
523-
524518
// ***** START: Organization *****
525519
m.Group("/org", func() {
526520
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}}">

‎templates/swagger/v1_json.tmpl

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3133,6 +3133,12 @@
31333133
"description": "search string",
31343134
"name": "q",
31353135
"in": "query"
3136+
},
3137+
{
3138+
"type": "string",
3139+
"description": "filter by type (issues / pulls) if set",
3140+
"name": "type",
3141+
"in": "query"
31363142
}
31373143
],
31383144
"responses": {

0 commit comments

Comments
 (0)