From e7a5f268d21303e4dad701e0337d9ed1262c07aa Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 20 Aug 2022 06:24:54 +0200 Subject: [PATCH 1/2] Fix SQL Query for `SearchTeam` (#20844) - Backport of #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. - Resolves #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 d571342c3d618..64407f79e91fa 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 219bd273c99c9..e073a9b5704ac 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"}) 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"}) 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 d755385726a55..03d055f5a0d94 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 a0bc4b9b43a83..e06d94cfcdc97 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 67ba869c76b08..87405bfd261a4 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 b32ffa6ca7681..bd99193872162 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 9ee66a1a3e86c..a3c3acb4f493e 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"), From 77848f5f4adfdcac5a1d8ffda4751654c097f8bd Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 21 Aug 2022 07:27:59 +0200 Subject: [PATCH 2/2] Fix backport --- integrations/api_user_orgs_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index e073a9b5704ac..9350f6fbfc2ae 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -26,7 +26,7 @@ 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"}) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}).(*user_model.User) assert.Equal(t, []*api.Organization{ { @@ -93,7 +93,7 @@ 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"}) + user17 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user17"}).(*user_model.User) assert.Equal(t, []*api.Organization{ {