Skip to content

fix: Admin can see all private repositories on Explore page. #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 26, 2017
Merged

fix: Admin can see all private repositories on Explore page. #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49b77ec
fix: Admin can see all private repositories on Explore page.
appleboy Feb 23, 2017
608a183
refactor: fix session
appleboy Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/issue_indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func createIssueIndexer() error {
// populateIssueIndexer populate the issue indexer with issue data
func populateIssueIndexer() error {
for page := 1; ; page++ {
repos, err := Repositories(&SearchRepoOptions{
repos, _, err := Repositories(&SearchRepoOptions{
Page: page,
PageSize: 10,
})
Expand Down
48 changes: 32 additions & 16 deletions models/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ func CountUserRepositories(userID int64, private bool) int64 {
}

// Repositories returns all repositories
func Repositories(opts *SearchRepoOptions) (_ RepositoryList, err error) {
func Repositories(opts *SearchRepoOptions) (_ RepositoryList, count int64, err error) {
Copy link
Member

@ethantkoenig ethantkoenig Feb 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDITED: Why does this function not examine the contents of opts.Searcher, and exclude repos accordingly (like GetRecentUpdateRepositories(..)~)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need to examine the contents of opts.Searcher?

https://github.com/go-gitea/gitea/blob/master/models/issue_indexer.go#L119
https://github.com/go-gitea/gitea/blob/master/routers/admin/repos.go#L28

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant like GetRecentUpdateRepsitories(..); sorry if that caused any confusion.

Even if every codepath that calls Repositories(..) has opts.Searcher set to nil, someone might in the future add a changes that calls Repositories(..), have opts.Searcher not be nil, and expect Repositories to filter accordingly.

Copy link
Member

@ethantkoenig ethantkoenig Feb 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Repositories(..) only looks at opts.PageSize, opts.Page, and opts.OrderBy, would it be better to have it take those 3 values as separate arguments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO. I think using SearchRepoOptions would be better than take 3 arguments if we want to extend the function.

if len(opts.OrderBy) == 0 {
opts.OrderBy = "id ASC"
}
Expand All @@ -1242,14 +1242,16 @@ func Repositories(opts *SearchRepoOptions) (_ RepositoryList, err error) {
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
OrderBy(opts.OrderBy).
Find(&repos); err != nil {
return nil, fmt.Errorf("Repo: %v", err)
return nil, 0, fmt.Errorf("Repo: %v", err)
}

if err = repos.loadAttributes(x); err != nil {
return nil, fmt.Errorf("LoadAttributes: %v", err)
return nil, 0, fmt.Errorf("LoadAttributes: %v", err)
}

return repos, nil
count = countRepositories(-1, opts.Private)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we check opts.Private to see if we should include private repos, but when populating the repos slice, we always include private repos regardless of opts.Private

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repositories always returns all repositories. maybe we need to change countRepositories(-1, true)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Repositories(..) always returns all repositories, I think it is misleading for it to take a SearchRepoOptions as a parameter, since it ignores most of the fields of opts.

Copy link
Member Author

@appleboy appleboy Feb 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can create another PR to refactor the codes according to your suggestion.

Copy link
Member

@ethantkoenig ethantkoenig Feb 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I suppose I could do it too. Either way, it's outside the scope of this PR.


return repos, count, nil
}

// RepoPath returns repository path by given user and repository name.
Expand Down Expand Up @@ -1757,40 +1759,54 @@ func GetUserMirrorRepositories(userID int64) ([]*Repository, error) {
}

// GetRecentUpdatedRepositories returns the list of repositories that are recently updated.
func GetRecentUpdatedRepositories(opts *SearchRepoOptions) (repos RepositoryList, err error) {
func GetRecentUpdatedRepositories(opts *SearchRepoOptions) (repos RepositoryList, _ int64, _ error) {
var cond = builder.NewCond()

if len(opts.OrderBy) == 0 {
opts.OrderBy = "updated_unix DESC"
}

sess := x.Where("is_private=?", false).
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
Limit(opts.PageSize)
if !opts.Private {
cond = builder.Eq{
"is_private": false,
}
}

if opts.Searcher != nil {
sess.Or("owner_id = ?", opts.Searcher.ID)
if opts.Searcher != nil && !opts.Searcher.IsAdmin {
var ownerIds []int64

ownerIds = append(ownerIds, opts.Searcher.ID)
err := opts.Searcher.GetOrganizations(true)

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

for _, org := range opts.Searcher.Orgs {
sess.Or("owner_id = ?", org.ID)
ownerIds = append(ownerIds, org.ID)
}

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

if err = sess.
count, err := x.Where(cond).Count(new(Repository))
if err != nil {
return nil, 0, fmt.Errorf("Count: %v", err)
}

if err = x.Where(cond).
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
Limit(opts.PageSize).
OrderBy(opts.OrderBy).
Find(&repos); err != nil {
return nil, fmt.Errorf("Repo: %v", err)
return nil, 0, fmt.Errorf("Repo: %v", err)
}

if err = repos.loadAttributes(x); err != nil {
return nil, fmt.Errorf("LoadAttributes: %v", err)
return nil, 0, fmt.Errorf("LoadAttributes: %v", err)
}

return repos, nil
return repos, count, nil
}

func getRepositoryCount(e Engine, u *User) (int64, error) {
Expand Down
1 change: 0 additions & 1 deletion routers/admin/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ func Repos(ctx *context.Context) {
ctx.Data["PageIsAdminRepositories"] = true

routers.RenderRepoSearch(ctx, &routers.RepoSearchOptions{
Counter: models.CountRepositories,
Ranger: models.Repositories,
Private: true,
PageSize: setting.UI.Admin.RepoPagingNum,
Expand Down
13 changes: 6 additions & 7 deletions routers/home.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ func Home(ctx *context.Context) {

// RepoSearchOptions when calling search repositories
type RepoSearchOptions struct {
Counter func(bool) int64
Ranger func(*models.SearchRepoOptions) (models.RepositoryList, error)
Ranger func(*models.SearchRepoOptions) (models.RepositoryList, int64, error)
Searcher *models.User
Private bool
PageSize int
Expand Down Expand Up @@ -101,17 +100,17 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) {

keyword := strings.Trim(ctx.Query("q"), " ")
if len(keyword) == 0 {
repos, err = opts.Ranger(&models.SearchRepoOptions{
repos, count, err = opts.Ranger(&models.SearchRepoOptions{
Page: page,
PageSize: opts.PageSize,
Searcher: ctx.User,
OrderBy: orderBy,
Private: opts.Private,
})
if err != nil {
ctx.Handle(500, "opts.Ranger", err)
return
}
count = opts.Counter(opts.Private)
} else {
if isKeywordValid(keyword) {
repos, count, err = models.SearchRepositoryByName(&models.SearchRepoOptions{
Expand Down Expand Up @@ -143,10 +142,10 @@ func ExploreRepos(ctx *context.Context) {
ctx.Data["PageIsExploreRepositories"] = true

RenderRepoSearch(ctx, &RepoSearchOptions{
Counter: models.CountRepositories,
Ranger: models.GetRecentUpdatedRepositories,
PageSize: setting.UI.ExplorePagingNum,
Searcher: ctx.User,
Private: ctx.User != nil && ctx.User.IsAdmin,
TplName: tplExploreRepos,
})
}
Expand Down Expand Up @@ -175,7 +174,6 @@ func RenderUserSearch(ctx *context.Context, opts *UserSearchOptions) {
)

ctx.Data["SortType"] = ctx.Query("sort")
//OrderBy: "id ASC",
switch ctx.Query("sort") {
case "oldest":
orderBy = "id ASC"
Expand All @@ -193,7 +191,8 @@ func RenderUserSearch(ctx *context.Context, opts *UserSearchOptions) {

keyword := strings.Trim(ctx.Query("q"), " ")
if len(keyword) == 0 {
users, err = opts.Ranger(&models.SearchUserOptions{OrderBy: orderBy,
users, err = opts.Ranger(&models.SearchUserOptions{
OrderBy: orderBy,
Page: page,
PageSize: opts.PageSize,
})
Expand Down