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

* Fix access issues on milestone and issue overview pages.

* Fix filter algorithm

diff --git a/models/repo_permission.go b/models/repo_permission.go
index 782b195629c928225e33a6b36a4632880f7fa148..79d7dd012b66ac2108e5cef8e6529988f795c0ae 100644 (file)
--- 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 a8f2c6fd223362d6fc19330e98f8fb88bcb0cbcd..f2c0a1861e5a04760a8348dcb9cd333ce9c546c9 100644 (file)
--- 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)
+               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

diff --git a/routers/user/home.go b/routers/user/home.go
index f5e74b240664ebd7951ba2668b0495dfc6b3e3d0..512c60716d1672b30cdb0bbe93b2e3cf5bfa2937 100644 (file)
--- 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)