From 0b4c166e8a90beeb1e71ee2fc16b3a240517c82d Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 21 Aug 2022 18:24:05 +0200 Subject: [PATCH] Fix SQL Query for `SearchTeam` (#20844) - Currently the function takes in the `UserID` option, but isn't being used within the SQL query. This patch fixes that by checking that only teams are being returned that the user belongs to. Fix #20829 Co-authored-by: delvh --- integrations/api_team_test.go | 2 +- integrations/api_user_orgs_test.go | 22 ++++++++++++++++++ integrations/org_test.go | 9 ++++---- models/fixtures/org_user.yml | 6 +++++ models/fixtures/user.yml | 2 +- models/organization/team.go | 37 ++++++++++++++++++++---------- routers/web/org/teams.go | 2 +- 7 files changed, 61 insertions(+), 19 deletions(-) diff --git a/integrations/api_team_test.go b/integrations/api_team_test.go index 82824d610..9ea7a6f78 100644 --- a/integrations/api_team_test.go +++ b/integrations/api_team_test.go @@ -223,7 +223,7 @@ func TestAPITeamSearch(t *testing.T) { defer prepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17}) var results TeamSearchResults diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index 9f3487cb7..1555b5339 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -26,8 +26,19 @@ func TestUserOrgs(t *testing.T) { orgs := getUserOrgs(t, adminUsername, normalUsername) user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user3"}) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}) assert.Equal(t, []*api.Organization{ + { + ID: 17, + UserName: user17.Name, + FullName: user17.FullName, + AvatarURL: user17.AvatarLink(), + Description: "", + Website: "", + Location: "", + Visibility: "public", + }, { ID: 3, UserName: user3.Name, @@ -82,8 +93,19 @@ func TestMyOrgs(t *testing.T) { var orgs []*api.Organization DecodeJSON(t, resp, &orgs) user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user3"}) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}) assert.Equal(t, []*api.Organization{ + { + ID: 17, + UserName: user17.Name, + FullName: user17.FullName, + AvatarURL: user17.AvatarLink(), + Description: "", + Website: "", + Location: "", + Visibility: "public", + }, { ID: 3, UserName: user3.Name, diff --git a/integrations/org_test.go b/integrations/org_test.go index e4677f58d..9fb1175d7 100644 --- a/integrations/org_test.go +++ b/integrations/org_test.go @@ -197,8 +197,8 @@ func TestOrgRestrictedUser(t *testing.T) { func TestTeamSearch(t *testing.T) { defer prepareTestEnv(t)() - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 15}) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17}) var results TeamSearchResults @@ -209,8 +209,9 @@ func TestTeamSearch(t *testing.T) { resp := session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &results) assert.NotEmpty(t, results.Data) - assert.Len(t, results.Data, 1) - assert.Equal(t, "test_team", results.Data[0].Name) + assert.Len(t, results.Data, 2) + assert.Equal(t, "review_team", results.Data[0].Name) + assert.Equal(t, "test_team", results.Data[1].Name) // no access if not organization member user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index a0bc4b9b4..e06d94cfc 100644 --- a/models/fixtures/org_user.yml +++ b/models/fixtures/org_user.yml @@ -63,3 +63,9 @@ uid: 29 org_id: 17 is_public: true + +- + id: 12 + uid: 2 + org_id: 17 + is_public: true diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 67ba869c7..87405bfd2 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -309,7 +309,7 @@ avatar_email: user17@example.com num_repos: 2 is_active: true - num_members: 3 + num_members: 4 num_teams: 3 - diff --git a/models/organization/team.go b/models/organization/team.go index 0b53c84d6..6787b9e0f 100644 --- a/models/organization/team.go +++ b/models/organization/team.go @@ -96,16 +96,7 @@ type SearchTeamOptions struct { IncludeDesc bool } -// SearchTeam search for teams. Caller is responsible to check permissions. -func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { - if opts.Page <= 0 { - opts.Page = 1 - } - if opts.PageSize == 0 { - // Default limit - opts.PageSize = 10 - } - +func (opts *SearchTeamOptions) toCond() builder.Cond { cond := builder.NewCond() if len(opts.Keyword) > 0 { @@ -117,10 +108,28 @@ func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { cond = cond.And(keywordCond) } - cond = cond.And(builder.Eq{"org_id": opts.OrgID}) + if opts.OrgID > 0 { + cond = cond.And(builder.Eq{"`team`.org_id": opts.OrgID}) + } + if opts.UserID > 0 { + cond = cond.And(builder.Eq{"team_user.uid": opts.UserID}) + } + + return cond +} + +// SearchTeam search for teams. Caller is responsible to check permissions. +func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { sess := db.GetEngine(db.DefaultContext) + opts.SetDefaultValues() + cond := opts.toCond() + + if opts.UserID > 0 { + sess = sess.Join("INNER", "team_user", "team_user.team_id = team.id") + } + count, err := sess. Where(cond). Count(new(Team)) @@ -128,7 +137,10 @@ func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { return nil, 0, err } - sess = sess.Where(cond) + if opts.UserID > 0 { + sess = sess.Join("INNER", "team_user", "team_user.team_id = team.id") + } + if opts.PageSize == -1 { opts.PageSize = int(count) } else { @@ -137,6 +149,7 @@ func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { teams := make([]*Team, 0, opts.PageSize) if err = sess. + Where(cond). OrderBy("lower_name"). Find(&teams); err != nil { return nil, 0, err diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 9ee66a1a3..a3c3acb4f 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -339,7 +339,7 @@ func SearchTeam(ctx *context.Context) { } opts := &organization.SearchTeamOptions{ - UserID: ctx.Doer.ID, + // UserID is not set because the router already requires the doer to be an org admin. Thus, we don't need to restrict to teams that the user belongs in Keyword: ctx.FormTrim("q"), OrgID: ctx.Org.Organization.ID, IncludeDesc: ctx.FormString("include_desc") == "" || ctx.FormBool("include_desc"),