Skip to content

fix bug about wrong dependencies permissions check and other wrong permissions check #9884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion public/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
30 changes: 23 additions & 7 deletions routers/api/v1/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -101,6 +115,7 @@ func ListIssues(ctx *context.APIContext) {
IsClosed: isClosed,
IssueIDs: issueIDs,
LabelIDs: labelIDs,
IsPull: isPull,
})
}

Expand Down Expand Up @@ -292,14 +307,15 @@ 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 {
ctx.Error(500, "LoadAttributes", err)
return
}

if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypeIssues) {
if !issue.IsPoster(ctx.User.ID) && !canWrite {
ctx.Status(403)
return
}
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion routers/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
25 changes: 16 additions & 9 deletions routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
36 changes: 18 additions & 18 deletions routers/repo/issue_dependency.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,21 @@ 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 {
ctx.ServerError("GetIssueByIndex", err)
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)

Expand Down Expand Up @@ -68,23 +68,20 @@ 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 {
ctx.ServerError("GetIssueByIndex", err)
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")
Expand Down Expand Up @@ -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)
}
8 changes: 1 addition & 7 deletions routers/routes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/issue/view_content.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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) }}
<div class="comment form">
<a class="avatar" href="{{.SignedUser.HomeLink}}">
<img src="{{.SignedUser.RelAvatarLink}}">
Expand Down
1 change: 1 addition & 0 deletions templates/repo/issue/view_content/sidebar.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@
<input type="hidden" id="repolink" value="{{$.RepoRelPath}}">
<!-- I know, there is probably a better way to do this -->
<input type="hidden" id="issueIndex" value="{{.Issue.Index}}"/>
<input type="hidden" id="type" value="{{.IssueType}}">

<div class="ui basic modal remove-dependency">
<div class="ui icon header">
Expand Down
6 changes: 6 additions & 0 deletions templates/swagger/v1_json.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down