From e6ec41149103bb1a6059edfdabc2616f4b2b134b Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 21 Aug 2022 20:31:51 +0200 Subject: [PATCH] Fix SQL Query for `SearchTeam` (#20844) (#20872) Backport #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 --- integrations/api_team_test.go | 2 +- integrations/api_user_orgs_test.go | 22 ++++++++++++++++++ integrations/org_test.go | 10 ++++----- models/fixtures/org_user.yml | 6 +++++ models/fixtures/user.yml | 2 +- models/organization/team.go | 36 +++++++++++++++++++++--------- 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 d571342c3d61..64407f79e91f 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}).(*user_model.User) - org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}).(*user_model.User) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17}).(*user_model.User) var results TeamSearchResults diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index 219bd273c99c..9350f6fbfc2a 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"}).(*user_model.User) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}).(*user_model.User) 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"}).(*user_model.User) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}).(*user_model.User) 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 d755385726a5..03d055f5a0d9 100644 --- a/integrations/org_test.go +++ b/integrations/org_test.go @@ -179,8 +179,8 @@ func TestOrgRestrictedUser(t *testing.T) { func TestTeamSearch(t *testing.T) { defer prepareTestEnv(t)() - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) - org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}).(*user_model.User) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 15}).(*user_model.User) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17}).(*user_model.User) var results TeamSearchResults @@ -190,9 +190,9 @@ func TestTeamSearch(t *testing.T) { req.Header.Add("X-Csrf-Token", csrf) 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}).(*user_model.User) diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index a0bc4b9b43a8..e06d94cfcdc9 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 67ba869c76b0..87405bfd261a 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 b32ffa6ca768..bd9919387216 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,6 +137,10 @@ func SearchTeam(opts *SearchTeamOptions) ([]*Team, int64, error) { return nil, 0, err } + if opts.UserID > 0 { + sess = sess.Join("INNER", "team_user", "team_user.team_id = team.id") + } + sess = sess.Where(cond) if opts.PageSize == -1 { opts.PageSize = int(count) @@ -137,6 +150,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 9ee66a1a3e86..a3c3acb4f493 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"),