Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix access issues on milestone and issue overview pages. #9603

Merged
merged 3 commits into from
Jan 5, 2020
Merged

Fix access issues on milestone and issue overview pages. #9603

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c16ed8d
Fix access issues on milestone and issue overview pages.
davidsvantesson Jan 4, 2020
8f36154
Fix filter algorithm
davidsvantesson Jan 5, 2020
b16decc
Merge branch 'master' into fix-milestone-overview
davidsvantesson Jan 5, 2020
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
18 changes: 18 additions & 0 deletions models/repo_permission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,3 +369,21 @@ func hasAccess(e Engine, userID int64, repo *Repository) (bool, error) {
func HasAccess(userID int64, repo *Repository) (bool, error) {
return hasAccess(x, userID, repo)
}

// FilterOutRepoIdsWithoutUnitAccess filter out repos where user has no access to repositories
func FilterOutRepoIdsWithoutUnitAccess(u *User, repoIDs []int64, units ...UnitType) ([]int64, error) {
for i, rID := range repoIDs {
repo, err := GetRepositoryByID(rID)
if err != nil {
return nil, err
}
perm, err := GetUserRepoPermission(repo, u)
if err != nil {
return nil, err
}
if !perm.CanReadAny(units...) {
repoIDs = append(repoIDs[:i], repoIDs[i+1:]...)
}
}
return repoIDs, nil
}
15 changes: 8 additions & 7 deletions models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,19 +638,20 @@ func (u *User) GetRepositoryIDs(units ...UnitType) ([]int64, error) {
func (u *User) GetOrgRepositoryIDs(units ...UnitType) ([]int64, error) {
var ids []int64

sess := x.Table("repository").
if err := x.Table("repository").
Cols("repository.id").
Join("INNER", "team_user", "repository.owner_id = team_user.org_id").
Join("INNER", "team_repo", "repository.is_private != ? OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)", true)
Join("INNER", "team_repo", "repository.is_private != ? OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)", true).
Where("team_user.uid = ?", u.ID).
GroupBy("repository.id").Find(&ids); err != nil {
return nil, err
}

if len(units) > 0 {
sess = sess.Join("INNER", "team_unit", "team_unit.team_id = team_user.team_id")
sess = sess.In("team_unit.type", units)
return FilterOutRepoIdsWithoutUnitAccess(u, ids, units...)
}

return ids, sess.
Where("team_user.uid = ?", u.ID).
GroupBy("repository.id").Find(&ids)
return ids, nil
}

// GetAccessRepoIDs returns all repositories IDs where user's or user is a team member organizations
Expand Down
80 changes: 35 additions & 45 deletions routers/user/home.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,13 @@ func Milestones(ctx *context.Context) {
ctx.ServerError("env.RepoIDs", err)
return
}
userRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctx.User, userRepoIDs, models.UnitTypeIssues, models.UnitTypePullRequests)
if err != nil {
ctx.ServerError("FilterOutRepoIdsWithoutUnitAccess", err)
return
}
} else {
unitType := models.UnitTypeIssues
userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType)
userRepoIDs, err = ctxUser.GetAccessRepoIDs(models.UnitTypeIssues, models.UnitTypePullRequests)
if err != nil {
ctx.ServerError("ctxUser.GetAccessRepoIDs", err)
return
Expand All @@ -201,27 +205,30 @@ func Milestones(ctx *context.Context) {
}

var repoIDs []int64
if issueReposQueryPattern.MatchString(reposQuery) {
// remove "[" and "]" from string
reposQuery = reposQuery[1 : len(reposQuery)-1]
//for each ID (delimiter ",") add to int to repoIDs
reposSet := false
for _, rID := range strings.Split(reposQuery, ",") {
// Ensure nonempty string entries
if rID != "" && rID != "0" {
reposSet = true
rIDint64, err := strconv.ParseInt(rID, 10, 64)
if err == nil && com.IsSliceContainsInt64(userRepoIDs, rIDint64) {
repoIDs = append(repoIDs, rIDint64)
if len(reposQuery) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces #9591

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9591 fixed it for issue page, this fixes it for milestone page. Also I changed log to Warn, IMO for something that user can put into address field that is enough.

if issueReposQueryPattern.MatchString(reposQuery) {
// remove "[" and "]" from string
reposQuery = reposQuery[1 : len(reposQuery)-1]
//for each ID (delimiter ",") add to int to repoIDs
reposSet := false
for _, rID := range strings.Split(reposQuery, ",") {
// Ensure nonempty string entries
if rID != "" && rID != "0" {
reposSet = true
rIDint64, err := strconv.ParseInt(rID, 10, 64)
// If the repo id specified by query is not parseable or not accessible by user, just ignore it.
if err == nil && com.IsSliceContainsInt64(userRepoIDs, rIDint64) {
repoIDs = append(repoIDs, rIDint64)
}
}
}
if reposSet && len(repoIDs) == 0 {
// force an empty result
repoIDs = []int64{-1}
}
} else {
log.Warn("issueReposQueryPattern not match with query")
}
if reposSet && len(repoIDs) == 0 {
// force an empty result
repoIDs = []int64{-1}
}
} else {
log.Error("issueReposQueryPattern not match with query")
}

if len(repoIDs) == 0 {
Expand Down Expand Up @@ -256,26 +263,6 @@ func Milestones(ctx *context.Context) {
}
}
showReposMap[rID] = repo

// Check if user has access to given repository.
perm, err := models.GetUserRepoPermission(repo, ctxUser)
if err != nil {
ctx.ServerError("GetUserRepoPermission", fmt.Errorf("[%d]%v", rID, err))
return
}

if !perm.CanRead(models.UnitTypeIssues) {
if log.IsTrace() {
log.Trace("Permission Denied: User %-v cannot read %-v of repo %-v\n"+
"User in repo has Permissions: %-+v",
ctxUser,
models.UnitTypeIssues,
repo,
perm)
}
ctx.Status(404)
return
}
}

showRepos := models.RepositoryListOfMap(showReposMap)
Expand Down Expand Up @@ -345,9 +332,11 @@ var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`)
// Issues render the user issues page
func Issues(ctx *context.Context) {
isPullList := ctx.Params(":type") == "pulls"
unitType := models.UnitTypeIssues
if isPullList {
ctx.Data["Title"] = ctx.Tr("pull_requests")
ctx.Data["PageIsPulls"] = true
unitType = models.UnitTypePullRequests
} else {
ctx.Data["Title"] = ctx.Tr("issues")
ctx.Data["PageIsIssues"] = true
Expand Down Expand Up @@ -404,7 +393,7 @@ func Issues(ctx *context.Context) {
}
}
} else {
log.Error("issueReposQueryPattern not match with query")
log.Warn("issueReposQueryPattern not match with query")
}
}

Expand All @@ -424,11 +413,12 @@ func Issues(ctx *context.Context) {
ctx.ServerError("env.RepoIDs", err)
return
}
} else {
unitType := models.UnitTypeIssues
if isPullList {
unitType = models.UnitTypePullRequests
userRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctx.User, userRepoIDs, unitType)
if err != nil {
ctx.ServerError("FilterOutRepoIdsWithoutUnitAccess", err)
return
}
} else {
userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType)
if err != nil {
ctx.ServerError("ctxUser.GetAccessRepoIDs", err)
Expand Down