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

* Fix access issues on milestone and issue overview pages.

* Fix filter algorithm

3 files changed, 63 insertions, 52 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)
+		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 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)