Skip to content

Commit

Permalink
Properly handle permissions in the UI and avoid the permissions map
Browse files Browse the repository at this point in the history 
We can avoid the permissions map because the dependencies are ordered by repoid when they are
extracted from the db.

Signed-off-by: Andrew Thornton <art27@cantab.net>
  • Loading branch information
@zeripath
zeripath committed Mar 26, 2023
1 parent d075ffd commit 44a120e
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 24 deletions.
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,9 @@ issues.due_date_invalid = "The due date is invalid or out of range. Please use t
issues.dependency.title = Dependencies
issues.dependency.issue_no_dependencies = No dependencies set.
issues.dependency.pr_no_dependencies = No dependencies set.
issues.dependency.no_permission_1 = "You do not have permission to read %d dependency"
issues.dependency.no_permission_n = "You do not have permission to read %d dependencies"
issues.dependency.no_permission.can_remove = "You do not have permission to read this dependency but can remove this dependency"
issues.dependency.add = Add dependency…
issues.dependency.cancel = Cancel
issues.dependency.remove = Remove
Expand Down
50 changes: 31 additions & 19 deletions routers/api/v1/repo/issue_dependency.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,6 @@ func GetIssueDependencies(ctx *context.APIContext) {

blockerIssues := make([]*issues_model.Issue, 0, limit)

perms := map[int64]access_model.Permission{}
perms[ctx.Repo.Repository.ID] = ctx.Repo.Permission

// 2. Get the issues this issue depends on, i.e. the `<#b>`: `<issue> <- <#b>`
blockersInfo, err := issue.BlockedByDependencies(ctx, db.ListOptions{
Page: page,
Expand All @@ -102,16 +99,24 @@ func GetIssueDependencies(ctx *context.APIContext) {
ctx.Error(http.StatusInternalServerError, "BlockedByDependencies", err)
return
}
for _, blocker := range blockersInfo {

var lastRepoID int64
var lastPerm access_model.Permission
for _, blocker := range blockersInfo {
// Get the permissions for this repository
perm, has := perms[blocker.Repository.ID]
if !has {
perm, err = access_model.GetUserRepoPermission(ctx, &blocker.Repository, ctx.Doer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return
perm := lastPerm
if lastRepoID != blocker.Repository.ID {
if blocker.Repository.ID == ctx.Repo.Repository.ID {
perm = ctx.Repo.Permission
} else {
var err error
perm, err = access_model.GetUserRepoPermission(ctx, &blocker.Repository, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return
}
}
lastRepoID = blocker.Repository.ID
}

// check permission
Expand Down Expand Up @@ -330,22 +335,29 @@ func GetIssueBlocks(ctx *context.APIContext) {
return
}

permMap := map[int64]access_model.Permission{}
permMap[ctx.Repo.Repository.ID] = ctx.Repo.Permission
var lastRepoID int64
var lastPerm access_model.Permission

var issues []*issues_model.Issue
for i, depMeta := range deps {
if i < skip || i >= max {
continue
}

perm, has := permMap[depMeta.Repository.ID]
if !has {
perm, err = access_model.GetUserRepoPermission(ctx, &depMeta.Repository, ctx.Doer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return
// Get the permissions for this repository
perm := lastPerm
if lastRepoID != depMeta.Repository.ID {
if depMeta.Repository.ID == ctx.Repo.Repository.ID {
perm = ctx.Repo.Permission
} else {
var err error
perm, err = access_model.GetUserRepoPermission(ctx, &depMeta.Repository, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return
}
}
permMap[depMeta.Repository.ID] = perm
lastRepoID = depMeta.Repository.ID
}

if !perm.CanReadIssuesOrPulls(depMeta.Issue.IsPull) {
Expand Down
56 changes: 54 additions & 2 deletions routers/web/repo/issue.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1812,17 +1812,27 @@ func ViewIssue(ctx *context.Context) {
}

// Get Dependencies
ctx.Data["BlockedByDependencies"], err = issue.BlockedByDependencies(ctx, db.ListOptions{})
blockedBy, err := issue.BlockedByDependencies(ctx, db.ListOptions{})
if err != nil {
ctx.ServerError("BlockedByDependencies", err)
return
}
ctx.Data["BlockingDependencies"], err = issue.BlockingDependencies(ctx)
ctx.Data["BlockedByDependencies"], ctx.Data["BlockedByDependenciesNotPermitted"] = checkBlockedByIssues(ctx, blockedBy)
if ctx.Written() {
return
}

blocking, err := issue.BlockingDependencies(ctx)
if err != nil {
ctx.ServerError("BlockingDependencies", err)
return
}

ctx.Data["BlockingDependencies"], ctx.Data["BlockingByDependenciesNotPermitted"] = checkBlockedByIssues(ctx, blocking)
if ctx.Written() {
return
}

ctx.Data["Participants"] = participants
ctx.Data["NumParticipants"] = len(participants)
ctx.Data["Issue"] = issue
Expand Down Expand Up @@ -1851,6 +1861,48 @@ func ViewIssue(ctx *context.Context) {
ctx.HTML(http.StatusOK, tplIssueView)
}

func checkBlockedByIssues(ctx *context.Context, blockers []*issues_model.DependencyInfo) (canRead, notPermitted []*issues_model.DependencyInfo) {
var lastRepoID int64
var lastPerm access_model.Permission
for i, blocker := range blockers {
// Get the permissions for this repository
perm := lastPerm
if lastRepoID != blocker.Repository.ID {
if blocker.Repository.ID == ctx.Repo.Repository.ID {
perm = ctx.Repo.Permission
} else {
var err error
perm, err = access_model.GetUserRepoPermission(ctx, &blocker.Repository, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission", err)
return
}
}
lastRepoID = blocker.Repository.ID
}

// check permission
if !perm.CanReadIssuesOrPulls(blocker.Issue.IsPull) {
blockers[len(notPermitted)], blockers[i] = blocker, blockers[len(notPermitted)]
notPermitted = blockers[:len(notPermitted)+1]
}
}
blockers = blockers[len(notPermitted):]
sortDependencyInfo(blockers)
sortDependencyInfo(notPermitted)

return blockers, notPermitted
}

func sortDependencyInfo(blockers []*issues_model.DependencyInfo) {
sort.Slice(blockers, func(i, j int) bool {
if blockers[i].RepoID == blockers[j].RepoID {
return blockers[i].Issue.CreatedUnix < blockers[j].Issue.CreatedUnix
}
return blockers[i].RepoID < blockers[j].RepoID
})
}

// GetActionIssue will return the issue which is used in the context.
func GetActionIssue(ctx *context.Context) *issues_model.Issue {
issue, err := issues_model.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
Expand Down
39 changes: 36 additions & 3 deletions templates/repo/issue/view_content/sidebar.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@
<div class="ui divider"></div>

<div class="ui depending">
{{if (and (not .BlockedByDependencies) (not .BlockingDependencies))}}
{{if (and (not .BlockedByDependencies) (not .BlockedByDependenciesNotPermitted) (not .BlockingDependencies) (not .BlockingDependenciesNotPermitted))}}
<span class="text"><strong>{{.locale.Tr "repo.issues.dependency.title"}}</strong></span>
<br>
<p>
Expand All @@ -432,7 +432,7 @@
</p>
{{end}}

{{if .BlockingDependencies}}
{{if or .BlockingDependencies .BlockingDependenciesNotPermitted}}
<span class="text" data-tooltip-content="{{if .Issue.IsPull}}{{.locale.Tr "repo.issues.dependency.pr_close_blocks"}}{{else}}{{.locale.Tr "repo.issues.dependency.issue_close_blocks"}}{{end}}">
<strong>{{.locale.Tr "repo.issues.dependency.blocks_short"}}</strong>
</span>
Expand All @@ -456,10 +456,15 @@
</div>
</div>
{{end}}
{{if .BlockingDependenciesNotPermitted}}
<div class="item gt-df gt-ac gt-sb">
<span>{{$.locale.TrN (len .BlockingDependenciesNotPermitted) "repo.issues.dependency.no_permission_1" "repo.issues.dependency.no_permission_n" (len .BlockingDependenciesNotPermitted)}}</span>
</div>
{{end}}
</div>
{{end}}

{{if .BlockedByDependencies}}
{{if or .BlockedByDependencies .BlockedByDependenciesNotPermitted}}
<span class="text" data-tooltip-content="{{if .Issue.IsPull}}{{.locale.Tr "repo.issues.dependency.pr_closing_blockedby"}}{{else}}{{.locale.Tr "repo.issues.dependency.issue_closing_blockedby"}}{{end}}">
<strong>{{.locale.Tr "repo.issues.dependency.blocked_by_short"}}</strong>
</span>
Expand All @@ -483,6 +488,34 @@
</div>
</div>
{{end}}
{{if $.CanCreateIssueDependencies}}
{{range .BlockedByDependenciesNotPermitted}}
<div class="item dependency{{if .Issue.IsClosed}} is-closed{{end}} gt-df gt-ac gt-sb">
<div class="item-left gt-df gt-jc gt-fc gt-f1">
<div>
<span data-tooltip-content="{{$.locale.Tr "repo.issues.dependency.no_permission.can_remove"}}">{{svg "octicon-lock" 16}}</span>
<span class="title" data-tooltip-content="#{{.Issue.Index}} {{.Issue.Title | RenderEmoji $.Context}}">
#{{.Issue.Index}} {{.Issue.Title | RenderEmoji $.Context}}
</span>
</div>
<div class="text small">
{{.Repository.OwnerName}}/{{.Repository.Name}}
</div>
</div>
<div class="item-right gt-df gt-ac">
{{if and $.CanCreateIssueDependencies (not $.Repository.IsArchived)}}
<a class="delete-dependency-button ci muted" data-id="{{.Issue.ID}}" data-type="blocking" data-tooltip-content="{{$.locale.Tr "repo.issues.dependency.remove_info"}}">
{{svg "octicon-trash" 16}}
</a>
{{end}}
</div>
</div>
{{end}}
{{else if .BlockedByDependenciesNotPermitted}}
<div class="item gt-df gt-ac gt-sb">
<span>{{$.locale.TrN (len .BlockedByDependenciesNotPermitted) "repo.issues.dependency.no_permission_1" "repo.issues.dependency.no_permission_n" (len .BlockedByDependenciesNotPermitted)}}</span>
</div>
{{end}}
</div>
{{end}}

Expand Down

0 comments on commit 44a120e

Please sign in to comment.