From 03d59bcd1dc775b6b8e52136dff1ba508838db2d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 5 Jan 2020 02:23:29 +0100 Subject: [PATCH] Fix access issues on milestone and issue overview pages. (#9603) * Fix access issues on milestone and issue overview pages. * Fix filter algorithm --- models/repo_permission.go | 20 ++++++++++ models/user.go | 19 +++++----- routers/user/home.go | 80 +++++++++++++++++---------------------- 3 files changed, 65 insertions(+), 54 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index 782b195629..79d7dd012b 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -369,3 +369,23 @@ 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) { + i := 0 + for _, 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[i] = rID + i++ + } + } + return repoIDs[:i], nil +} diff --git a/models/user.go b/models/user.go index a8f2c6fd22..f2c0a1861e 100644 --- a/models/user.go +++ b/models/user.go @@ -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) - - 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) + 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 } - return ids, sess. - Where("team_user.uid = ?", u.ID). - GroupBy("repository.id").Find(&ids) + if len(units) > 0 { + return FilterOutRepoIdsWithoutUnitAccess(u, ids, units...) + } + + return ids, nil } // GetAccessRepoIDs returns all repositories IDs where user's or user is a team member organizations diff --git a/routers/user/home.go b/routers/user/home.go index f5e74b2406..512c60716d 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -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 @@ -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 { + 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 { @@ -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) @@ -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 @@ -404,7 +393,7 @@ func Issues(ctx *context.Context) { } } } else { - log.Error("issueReposQueryPattern not match with query") + log.Warn("issueReposQueryPattern not match with query") } } @@ -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)