Fix repository search function (#2689)

* Fix and remove FIXME

* Respect membership visibility

* Fix/rewrite searchRepositoryByName function

* Add unit tests

* Add integration tests

* Remove Searcher completely

* Remove trailing space

7 files changed, 116 insertions, 67 deletions

diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml
index 34d5c5c594..9c149b78d0 100644
--- a/models/fixtures/access.yml
+++ b/models/fixtures/access.yml
@@ -38,4 +38,28 @@
   id: 7
   user_id: 15
   repo_id: 24
-  mode: 4 # owner
\ No newline at end of file
+  mode: 4 # owner
+
+-
+  id: 8
+  user_id: 18
+  repo_id: 23
+  mode: 4 # owner
+
+-
+  id: 9
+  user_id: 18
+  repo_id: 24
+  mode: 4 # owner
+
+-
+  id: 10
+  user_id: 18
+  repo_id: 22
+  mode: 2 # write
+
+-
+  id: 11
+  user_id: 18
+  repo_id: 21
+  mode: 2 # write
\ No newline at end of file
diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml
index c7fe2cf369..50d8ef5e68 100644
--- a/models/fixtures/org_user.yml
+++ b/models/fixtures/org_user.yml
@@ -37,3 +37,11 @@
   is_public: true
   is_owner: true
   num_teams: 1
+
+-
+  id: 6
+  uid: 18
+  org_id: 17
+  is_public: false
+  is_owner: true
+  num_teams: 1
\ No newline at end of file
diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml
index 298de648dd..2b2186deae 100644
--- a/models/fixtures/team.yml
+++ b/models/fixtures/team.yml
@@ -44,5 +44,5 @@
   name: Owners
   authorize: 4 # owner
   num_repos: 2
-  num_members: 1
-  unit_types: '[1,2,3,4,5,6,7]'
+  num_members: 2
+  unit_types: '[1,2,3,4,5,6,7]'
\ No newline at end of file
diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml
index 8d9d920378..56025bb0b7 100644
--- a/models/fixtures/team_user.yml
+++ b/models/fixtures/team_user.yml
@@ -32,4 +32,10 @@
   id: 6
   org_id: 17
   team_id: 5
-  uid: 15
\ No newline at end of file
+  uid: 15
+
+-
+  id: 7
+  org_id: 17
+  team_id: 5
+  uid: 18
\ No newline at end of file
diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml
index e1fbc9e578..73ec1c8593 100644
--- a/models/fixtures/user.yml
+++ b/models/fixtures/user.yml
@@ -263,5 +263,20 @@
   avatar_email: user17@example.com
   num_repos: 2
   is_active: true
-  num_members: 1
-  num_teams: 1
\ No newline at end of file
+  num_members: 2
+  num_teams: 1
+
+-
+  id: 18
+  lower_name: user18
+  name: user18
+  full_name: User 18
+  email: user18@example.com
+  passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
+  type: 0 # individual
+  salt: ZogKvWdyEx
+  is_admin: false
+  avatar: avatar18
+  avatar_email: user18@example.com
+  num_repos: 0
+  is_active: true
\ No newline at end of file
diff --git a/models/repo_list.go b/models/repo_list.go
index 4aeda90677..2c4c66ac3e 100644
--- a/models/repo_list.go
+++ b/models/repo_list.go
@@ -98,7 +98,6 @@ type SearchRepoOptions struct {
 	//
 	// in: query
 	OwnerID     int64         `json:"uid"`
-	Searcher    *User         `json:"-"` //ID of the person who's seeking
 	OrderBy     SearchOrderBy `json:"-"`
 	Private     bool          `json:"-"` // Include private repositories in results
 	Collaborate bool          `json:"-"` // Include collaborative repositories
@@ -136,57 +135,48 @@ const (
 
 // SearchRepositoryByName takes keyword and part of repository name to search,
 // it returns results in given range and number of total results.
-func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, count int64, err error) {
-	var cond = builder.NewCond()
+func SearchRepositoryByName(opts *SearchRepoOptions) (RepositoryList, int64, error) {
 	if opts.Page <= 0 {
 		opts.Page = 1
 	}
 
-	var starJoin bool
-	if opts.Starred && opts.OwnerID > 0 {
-		cond = builder.Eq{
-			"star.uid": opts.OwnerID,
-		}
-		starJoin = true
-	}
-
-	// Append conditions
-	if !opts.Starred && opts.OwnerID > 0 {
-		var searcherReposCond builder.Cond = builder.Eq{"owner_id": opts.OwnerID}
-		if opts.Searcher != nil {
-			var ownerIds []int64
-
-			ownerIds = append(ownerIds, opts.Searcher.ID)
-			err = opts.Searcher.GetOrganizations(true)
+	var cond = builder.NewCond()
 
-			if err != nil {
-				return nil, 0, fmt.Errorf("Organization: %v", err)
-			}
+	if !opts.Private {
+		cond = cond.And(builder.Eq{"is_private": false})
+	}
 
-			for _, org := range opts.Searcher.Orgs {
-				ownerIds = append(ownerIds, org.ID)
+	starred := false
+	if opts.OwnerID > 0 {
+		if opts.Starred {
+			starred = true
+			cond = builder.Eq{
+				"star.uid": opts.OwnerID,
 			}
+		} else {
+			var accessCond builder.Cond = builder.Eq{"owner_id": opts.OwnerID}
 
-			searcherReposCond = searcherReposCond.Or(builder.In("owner_id", ownerIds))
 			if opts.Collaborate {
-				searcherReposCond = searcherReposCond.Or(builder.Expr("id IN (SELECT repo_id FROM `access` WHERE access.user_id = ? AND owner_id != ?)",
-					opts.Searcher.ID, opts.Searcher.ID))
+				collaborateCond := builder.And(
+					builder.Expr("id IN (SELECT repo_id FROM `access` WHERE access.user_id = ?)", opts.OwnerID),
+					builder.Neq{"owner_id": opts.OwnerID})
+				if !opts.Private {
+					collaborateCond = collaborateCond.And(builder.Expr("owner_id NOT IN (SELECT org_id FROM org_user WHERE org_user.uid = ? AND org_user.is_public = ?)", opts.OwnerID, false))
+				}
+
+				accessCond = accessCond.Or(collaborateCond)
 			}
-		}
-		cond = cond.And(searcherReposCond)
-	}
 
-	if !opts.Private {
-		cond = cond.And(builder.Eq{"is_private": false})
+			cond = cond.And(accessCond)
+		}
 	}
 
 	if opts.OwnerID > 0 && opts.AllPublic {
 		cond = cond.Or(builder.Eq{"is_private": false})
 	}
 
-	opts.Keyword = strings.ToLower(opts.Keyword)
 	if opts.Keyword != "" {
-		cond = cond.And(builder.Like{"lower_name", opts.Keyword})
+		cond = cond.And(builder.Like{"lower_name", strings.ToLower(opts.Keyword)})
 	}
 
 	if len(opts.OrderBy) == 0 {
@@ -196,26 +186,23 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 	sess := x.NewSession()
 	defer sess.Close()
 
-	if starJoin {
-		count, err = sess.
-			Join("INNER", "star", "star.repo_id = repository.id").
-			Where(cond).
-			Count(new(Repository))
-		if err != nil {
-			return nil, 0, fmt.Errorf("Count: %v", err)
-		}
+	if starred {
+		sess.Join("INNER", "star", "star.repo_id = repository.id")
+	}
 
+	count, err := sess.
+		Where(cond).
+		Count(new(Repository))
+	if err != nil {
+		return nil, 0, fmt.Errorf("Count: %v", err)
+	}
+
+	// Set again after reset by Count()
+	if starred {
 		sess.Join("INNER", "star", "star.repo_id = repository.id")
-	} else {
-		count, err = sess.
-			Where(cond).
-			Count(new(Repository))
-		if err != nil {
-			return nil, 0, fmt.Errorf("Count: %v", err)
-		}
 	}
 
-	repos = make([]*Repository, 0, opts.PageSize)
+	repos := make(RepositoryList, 0, opts.PageSize)
 	if err = sess.
 		Where(cond).
 		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
@@ -230,5 +217,5 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 		}
 	}
 
-	return
+	return repos, count, nil
 }
diff --git a/models/repo_list_test.go b/models/repo_list_test.go
index 160ba57d32..4d125633a5 100644
--- a/models/repo_list_test.go
+++ b/models/repo_list_test.go
@@ -42,7 +42,6 @@ func TestSearchRepositoryByName(t *testing.T) {
 		Page:     1,
 		PageSize: 10,
 		Private:  true,
-		Searcher: &User{ID: 14},
 	})
 
 	assert.NoError(t, err)
@@ -56,7 +55,6 @@ func TestSearchRepositoryByName(t *testing.T) {
 		Page:     1,
 		PageSize: 10,
 		Private:  true,
-		Searcher: &User{ID: 14},
 	})
 
 	assert.NoError(t, err)
@@ -82,16 +80,28 @@ func TestSearchRepositoryByName(t *testing.T) {
 			count: 8},
 		{name: "PublicRepositoriesOfUser",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15},
-			count: 3}, // FIXME: Should return 2 (only directly owned repositories), now includes 1 public repository from owned organization
+			count: 2},
+		{name: "PublicRepositoriesOfUser2",
+			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18},
+			count: 0},
 		{name: "PublicAndPrivateRepositoriesOfUser",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true},
-			count: 6}, // FIXME: Should return 4 (only directly owned repositories), now includes 2 repositories from owned organization
+			count: 4},
+		{name: "PublicAndPrivateRepositoriesOfUser2",
+			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true},
+			count: 0},
 		{name: "PublicRepositoriesOfUserIncludingCollaborative",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Collaborate: true},
 			count: 4},
+		{name: "PublicRepositoriesOfUser2IncludingCollaborative",
+			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Collaborate: true},
+			count: 1},
 		{name: "PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true},
 			count: 8},
+		{name: "PublicAndPrivateRepositoriesOfUser2IncludingCollaborative",
+			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 18, Private: true, Collaborate: true},
+			count: 4},
 		{name: "PublicRepositoriesOfOrganization",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17},
 			count: 1},
@@ -113,6 +123,9 @@ func TestSearchRepositoryByName(t *testing.T) {
 		{name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
 			opts:  &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 15, Private: true, Collaborate: true, AllPublic: true},
 			count: 10},
+		{name: "AllPublic/PublicAndPrivateRepositoriesOfUser2IncludingCollaborativeByName",
+			opts:  &SearchRepoOptions{Keyword: "test", Page: 1, PageSize: 10, OwnerID: 18, Private: true, Collaborate: true, AllPublic: true},
+			count: 8},
 		{name: "AllPublic/PublicRepositoriesOfOrganization",
 			opts:  &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 17, AllPublic: true},
 			count: 12},
@@ -120,9 +133,6 @@ func TestSearchRepositoryByName(t *testing.T) {
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			if testCase.opts.OwnerID > 0 {
-				testCase.opts.Searcher = &User{ID: testCase.opts.OwnerID}
-			}
 			repos, count, err := SearchRepositoryByName(testCase.opts)
 
 			assert.NoError(t, err)
@@ -143,10 +153,9 @@ func TestSearchRepositoryByName(t *testing.T) {
 					assert.Contains(t, repo.Name, testCase.opts.Keyword)
 				}
 
-				// FIXME: Can't check, need to fix current behaviour (see previous FIXME comments in test cases)
-				/*if testCase.opts.OwnerID > 0 && !testCase.opts.Collaborate && !AllPublic {
+				if testCase.opts.OwnerID > 0 && !testCase.opts.Collaborate && !testCase.opts.AllPublic {
 					assert.Equal(t, testCase.opts.OwnerID, repo.Owner.ID)
-				}*/
+				}
 
 				if !testCase.opts.Private {
 					assert.False(t, repo.IsPrivate)