Fix repo search result #2600

daviian · 2017-09-25T11:36:06Z

Targets #2562, #2399, #1794 and #2595 too.

fixes wrong search result.
Most of the changes are adding tests from @Morlinest

codecov-io · 2017-09-25T19:44:51Z

Codecov Report

Merging #2600 into master will increase coverage by 0.08%.
The diff coverage is 35.13%.

@@            Coverage Diff            @@
##           master   #2600      +/-   ##
=========================================
+ Coverage   27.32%   27.4%   +0.08%     
=========================================
  Files          86      86              
  Lines       17143   17165      +22     
=========================================
+ Hits         4684    4704      +20     
- Misses      11781   11782       +1     
- Partials      678     679       +1

Impacted Files	Coverage Δ
models/user.go	`18.74% <100%> (+0.32%)`	⬆️
models/repo_list.go	`30.84% <22.22%> (+6.12%)`	⬆️
models/org.go	`69.36% <62.5%> (-0.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 339d7de...866c5cf. Read the comment docs.

Morlinest · 2017-09-25T20:17:21Z

First, you are using my (older) tests, while PR with my tests is still pending (#2575). Second, you had problem with review of my changes to repo search because PR was too big but now you are doing the same. I am dividing that PR to multiple PRs one by one, now blocked only by aprooving and merging... So I don't understand (and don't like) this.

daviian · 2017-09-25T20:22:47Z

@Morlinest As I told you in a PM in discord. Your tests are great. And instead of pushing a whole refactoring and fix I only did a fix. Your refactoring should be added after this and all is fine.
And I even said you can take my changes I did on thursday. (which are exactly this)

Morlinest · 2017-09-25T20:26:46Z

But why you are adding tests when I've already posted PR with tests only?

daviian · 2017-09-25T20:27:21Z

Because these are tests for the old code with Searcher as it is. And they are working.
The only difference is that they have the old search results. Like transitive ownership as described in your PR.
The code only removes non accessible repositories in list view,

Morlinest · 2017-09-25T20:32:51Z

@daviian That tests contain it too. I think you missed it:

if testCase.opts.OwnerID > 0 {
	testCase.opts.Searcher = &User{ID: testCase.opts.OwnerID}
}

It should be same (old vs new version), only rewritten to testCases.

daviian · 2017-09-25T20:38:57Z

@Morlinest It was important for me to leave existing code as it is. Just to make it more easier to see that nothing has changed. A rewriting of tests doesn't necessarily mean that tests haven't changed.

And as you can see it was important for me to give credits to you too.
And moreover this PR doesn't make your PR's useless. They should be added nevertheless as yours make code a lot cleaner.

Morlinest · 2017-09-25T20:47:10Z

I've created that tests, they are not in codebase, they have same results and improved. I haven't spent a little amount of time to do it. Nor first version of tests, nor second one.

daviian · 2017-09-25T21:00:15Z

@Morlinest Changed PR such that it totally uses your commit. And if that doesn't please you I can offer you to take my code from that PR and create your own PR. Or i remove your tests from this PR. What would you like?
I don't want anyone to be upset. Only to get 1.2 released.

ethantkoenig · 2017-09-25T21:54:23Z

integrations/api_repo_test.go

@@ -46,6 +46,220 @@ func TestAPISearchRepoNotLogin(t *testing.T) {
 		assert.Contains(t, repo.Name, keyword)
 		assert.False(t, repo.Private)
 	}
+
+	// Should return all (max 50) public repositories
+	req = NewRequest(t, "GET", "/api/v1/repos/search?limit=50")


This is a lot of duplicated code. Maybe we should add a helper function

func apiRepoSearchResult(t testing.T, params string, expectedLen int) *api.SearchResults ... apiRepoSearchResult(t, "?limit=50", 12)

Even better, add a session parameter, so you can use it for the logged-in tests too.

@ethantkoenig If you want that you can review #2575 as it rewrites tests. I wanted to change as few as possible to make reviewing easier, but if it is desired the mentioned PR does this ;-)

ethantkoenig · 2017-09-25T21:55:33Z

models/org.go

@@ -350,8 +350,11 @@ func GetOrgsByUserID(userID int64, showAll bool) ([]*User, error) {
 	return getOrgsByUserID(sess, userID, showAll)
 }

-func getOwnedOrgsByUserID(sess *xorm.Session, userID int64) ([]*User, error) {
+func getOwnedOrgsByUserID(sess *xorm.Session, userID int64, showAll bool) ([]*User, error) {


nit: could we call it includePrivate or something similar so it is clearer what is included and unincluded when the parameter is false?

I called it showAll to be consistent with other methods used similarly. But I can change that at least for my PR.

ethantkoenig · 2017-09-25T21:59:48Z

models/repo_list_test.go

+	assert.Len(t, repos, 3)
+
+	// Get all public repositories by name
+	repos, count, err = SearchRepositoryByName(&SearchRepoOptions{


This is a lot of duplicated code, maybe we should make this a table-driven test.

Same as above ;-)

Morlinest · 2017-09-26T15:01:37Z

@daviian You know I have PR for solving problems with this function :). Anyway, regardless of tests. I looked at your changes to implementation. It looks like something I was doing in first version of my PR, see dae998e. You are making same authorization decisions inside that shouldn't be there. I think more changes to current implementation with Searcher and OwnerID will not (can't) make it better.

My opinion:
Try to think about results. With your changes, if Searcher is not empty and you search for repositories of another user (OwnerID), you will get your own repositories and repositories of owned (or related) organizations + repositories owned by OwnerID. But if Searcher is empty, you will get repositories of OwnerID and repositories of organization owned by OwnerID. But I would expect to get just subset of repos from first scenario (when there is Searcher and OwnerID). In second scenario you will get some repositories from first case, but also some repositories that aren't returned in first scenario. Also it doesn't solve problem, when Searcher is set but OwnerID is not.

daviian · 2017-09-26T17:11:21Z

@Morlinest
First of all, I don't want to change existing search behaviour, that is way out of scope. PR should only remove repositories a user has no access to. That's why I don't break down the whole search function.
And because it shouldn't be there you should nevertheless refactor that stuff in your PR after that.
Second as I commented in code: If Searcher is not empty it means that OwnerID = SearcherID

And last but not least and as I said multiple times: This PR doesn't replace your refactoring, which includes changing search behaviour. It is only an interim step to fix permission problems.

Morlinest · 2017-09-26T18:19:15Z

models/repo_list.go

 					opts.Searcher.ID, opts.Searcher.ID))
 			}
+		} else if opts.OwnerID > 0 {


Not needed. It's already inside opts.OwnerID > 0 block.

Morlinest · 2017-09-26T18:19:45Z

models/repo_list.go

+				return nil, 0, fmt.Errorf("User: %v", err)
+			}
+
+			err = owner.GetOwnedOrganizations(false)


Authorization decision making.

Morlinest · 2017-09-26T18:56:27Z

models/repo_list.go

+			if err != nil {
+				return nil, 0, fmt.Errorf("Organization: %v", err)
+			}
+


Missing collaborate repos.

Morlinest · 2017-09-26T18:58:38Z

models/repo_list.go

 		}
+		searcherReposCond = searcherReposCond.Or(builder.In("owner_id", ownerIds))


Already in condition, line 157.

daviian · 2017-09-26T20:29:46Z

Closing PR as there exist scenarios in which this only solves a small portion of the search problem.
So I see that we don't come around a not that small refactoring of search.

But I want to point out that search should be fixed asap it is a massive security issue IMO.

daviian mentioned this pull request Sep 25, 2017

After upgrading, repositories without an issuetracker can not be accessed. #2595

Closed

7 tasks

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 25, 2017

daviian changed the title ~~Fix repo unit permission check after ugprade from 1.1.4 to 1.2.0-rc3~~ Fix repo search result Sep 25, 2017

daviian force-pushed the fix-repo-units branch 2 times, most recently from 14ae44a to c35c3a9 Compare September 25, 2017 18:34

daviian changed the base branch from release/v1.2 to master September 25, 2017 18:34

daviian force-pushed the fix-repo-units branch 2 times, most recently from 4a60099 to e4f1f1e Compare September 25, 2017 19:28

Morlinest and others added 2 commits September 25, 2017 22:50

Add repository search tests

fed4ee8

fix search implementation

866c5cf

daviian force-pushed the fix-repo-units branch from e4f1f1e to 866c5cf Compare September 25, 2017 20:59

ethantkoenig suggested changes Sep 25, 2017

View reviewed changes

Morlinest suggested changes Sep 26, 2017

View reviewed changes

daviian closed this Sep 26, 2017

daviian deleted the fix-repo-units branch September 26, 2017 20:44

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix repo search result #2600

Fix repo search result #2600

daviian commented Sep 25, 2017 •

edited

Loading

codecov-io commented Sep 25, 2017 •

edited

Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 •

edited

Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 •

edited

Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 •

edited

Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 •

edited

Loading

ethantkoenig Sep 25, 2017 •

edited

Loading

daviian Sep 25, 2017

ethantkoenig Sep 25, 2017

daviian Sep 25, 2017

ethantkoenig Sep 25, 2017

daviian Sep 25, 2017

Morlinest commented Sep 26, 2017

daviian commented Sep 26, 2017

Morlinest Sep 26, 2017

Morlinest Sep 26, 2017

Morlinest Sep 26, 2017

Morlinest Sep 26, 2017

daviian commented Sep 26, 2017

		}
		searcherReposCond = searcherReposCond.Or(builder.In("owner_id", ownerIds))

Fix repo search result #2600

Fix repo search result #2600

Conversation

daviian commented Sep 25, 2017 • edited Loading

codecov-io commented Sep 25, 2017 • edited Loading

Codecov Report

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 • edited Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 • edited Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 • edited Loading

Morlinest commented Sep 25, 2017

daviian commented Sep 25, 2017 • edited Loading

ethantkoenig Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest commented Sep 26, 2017

daviian commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian commented Sep 26, 2017

daviian commented Sep 25, 2017 •

edited

Loading

codecov-io commented Sep 25, 2017 •

edited

Loading

daviian commented Sep 25, 2017 •

edited

Loading

daviian commented Sep 25, 2017 •

edited

Loading

daviian commented Sep 25, 2017 •

edited

Loading

daviian commented Sep 25, 2017 •

edited

Loading

ethantkoenig Sep 25, 2017 •

edited

Loading