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

Author: Morlinest

integrations/api_repo_test.go

@@ -50,6 +50,7 @@ func TestAPISearchRepo(t *testing.T) {
 
 	user := models.AssertExistsAndLoadBean(t, &models.User{ID: 15}).(*models.User)
 	user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 16}).(*models.User)
+	user3 := models.AssertExistsAndLoadBean(t, &models.User{ID: 18}).(*models.User)
 	orgUser := models.AssertExistsAndLoadBean(t, &models.User{ID: 17}).(*models.User)
 
 	// Map of expected results, where key is user for login
@@ -85,17 +86,21 @@ func TestAPISearchRepo(t *testing.T) {
 			user2: {count: 4, repoName: "big_test_"}},
 		},
 		{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{
-			// FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user
-			nil:  {count: 2},
-			user: {count: 8, includesPrivate: true},
-			// FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user
-			user2: {count: 2}},
+			nil:   {count: 4},
+			user:  {count: 8, includesPrivate: true},
+			user2: {count: 4}},
 		},
 		{name: "RepositoriesAccessibleAndRelatedToUser2", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user2.ID), expectedResults: expectedResults{
 			nil:   {count: 1},
 			user:  {count: 1},
 			user2: {count: 2, includesPrivate: true}},
 		},
+		{name: "RepositoriesAccessibleAndRelatedToUser3", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user3.ID), expectedResults: expectedResults{
+			nil:   {count: 1},
+			user:  {count: 1},
+			user2: {count: 1},
+			user3: {count: 4, includesPrivate: true}},
+		},
 		{name: "RepositoriesOwnedByOrganization", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", orgUser.ID), expectedResults: expectedResults{
 			nil:   {count: 1, repoOwnerID: orgUser.ID},
 			user:  {count: 2, repoOwnerID: orgUser.ID, includesPrivate: true},

models/fixtures/access.yml

@@ -38,4 +38,28 @@
   id: 7
   user_id: 15
   repo_id: 24
-  mode: 4 # owner
+  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

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

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]'

models/fixtures/team_user.yml

@@ -32,4 +32,10 @@
   id: 6
   org_id: 17
   team_id: 5
-  uid: 15
+  uid: 15
+
+-
+  id: 7
+  org_id: 17
+  team_id: 5
+  uid: 18

models/fixtures/user.yml

@@ -263,5 +263,20 @@
   avatar_email: [email protected]
   num_repos: 2
   is_active: true
-  num_members: 1
-  num_teams: 1
+  num_members: 2
+  num_teams: 1
+
+-
+  id: 18
+  lower_name: user18
+  name: user18
+  full_name: User 18
+  email: [email protected]
+  passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
+  type: 0 # individual
+  salt: ZogKvWdyEx
+  is_admin: false
+  avatar: avatar18
+  avatar_email: [email protected]
+  num_repos: 0
+  is_active: true

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
 }

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,16 +123,16 @@ 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},
 	}
 
 	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)

routers/api/v1/repo/repo.go

@@ -34,31 +34,30 @@ func Search(ctx *context.APIContext) {
 		OwnerID:  ctx.QueryInt64("uid"),
 		PageSize: convert.ToCorrectPageSize(ctx.QueryInt("limit")),
 	}
-	if ctx.User != nil && ctx.User.ID == opts.OwnerID {
-		opts.Searcher = ctx.User
-	}
 
-	// Check visibility.
-	if ctx.IsSigned && opts.OwnerID > 0 {
-		if ctx.User.ID == opts.OwnerID {
-			opts.Private = true
-			opts.Collaborate = true
+	if opts.OwnerID > 0 {
+		var repoOwner *models.User
+		if ctx.User != nil && ctx.User.ID == opts.OwnerID {
+			repoOwner = ctx.User
 		} else {
-			u, err := models.GetUserByID(opts.OwnerID)
+			var err error
+			repoOwner, err = models.GetUserByID(opts.OwnerID)
 			if err != nil {
 				ctx.JSON(500, api.SearchError{
 					OK:    false,
 					Error: err.Error(),
 				})
 				return
 			}
-			if u.IsOrganization() && u.IsOwnedBy(ctx.User.ID) {
-				opts.Private = true
-			}
+		}
 
-			if !u.IsOrganization() {
-				opts.Collaborate = true
-			}
+		if !repoOwner.IsOrganization() {
+			opts.Collaborate = true
+		}
+
+		// Check visibility.
+		if ctx.IsSigned && (ctx.User.ID == repoOwner.ID || (repoOwner.IsOrganization() && repoOwner.IsOwnedBy(ctx.User.ID))) {
+			opts.Private = true
 		}
 	}
 

routers/home.go

@@ -120,7 +120,6 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) {
 		Private:     opts.Private,
 		Keyword:     keyword,
 		OwnerID:     opts.OwnerID,
-		Searcher:    ctx.User,
 		Collaborate: true,
 		AllPublic:   true,
 	})