From 1b53a9e91401c931065949981d02e06bbcd745ef Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 30 Jan 2023 11:12:45 +0100 Subject: [PATCH] Don't return duplicated users who can create org repo (#22560) - Currently the function `GetUsersWhoCanCreateOrgRepo` uses a query that is able to have duplicated users in the result, this is can happen under the condition that a user is in team that either is the owner team or has permission to create organization repositories. - Add test code to simulate the above condition for user 3, [`TestGetUsersWhoCanCreateOrgRepo`](https://github.com/go-gitea/gitea/blob/a1fcb1cfb84fd6b36c8fe9fd56588119fa4377bc/models/organization/org_test.go#L435) is the test function that tests for this. - The fix is quite trivial use a map keyed by user id in order to drop duplicates. --------- Co-authored-by: Lunny Xiao --- models/activities/notification.go | 2 +- models/fixtures/team.yml | 11 +++++++++++ models/fixtures/team_user.yml | 6 ++++++ models/fixtures/user.yml | 2 +- models/organization/org.go | 7 ++++--- models/organization/org_test.go | 9 +++++---- 6 files changed, 28 insertions(+), 9 deletions(-) diff --git a/models/activities/notification.go b/models/activities/notification.go index 321d543c7..f153eb058 100644 --- a/models/activities/notification.go +++ b/models/activities/notification.go @@ -151,7 +151,7 @@ func CreateRepoTransferNotification(ctx context.Context, doer, newOwner *user_mo } for i := range users { notify = append(notify, &Notification{ - UserID: users[i].ID, + UserID: i, RepoID: repo.ID, Status: NotificationStatusUnread, UpdatedBy: doer.ID, diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index ea47a33f1..dd434d78a 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -140,3 +140,14 @@ num_members: 1 includes_all_repositories: false can_create_org_repo: false + +- + id: 14 + org_id: 3 + lower_name: teamcreaterepo + name: teamCreateRepo + authorize: 2 # write + num_repos: 0 + num_members: 1 + includes_all_repositories: false + can_create_org_repo: true diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index 845741eff..de4f29d97 100644 --- a/models/fixtures/team_user.yml +++ b/models/fixtures/team_user.yml @@ -93,3 +93,9 @@ org_id: 19 team_id: 6 uid: 31 + +- + id: 17 + org_id: 3 + team_id: 14 + uid: 2 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 3afed37df..63a5e0f89 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -104,7 +104,7 @@ num_following: 0 num_stars: 0 num_repos: 3 - num_teams: 4 + num_teams: 5 num_members: 3 visibility: 0 repo_admin_change_team_access: false diff --git a/models/organization/org.go b/models/organization/org.go index 9d9e9cda4..05eaead60 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -397,13 +397,14 @@ func (org *Organization) GetOrgUserMaxAuthorizeLevel(uid int64) (perm.AccessMode } // GetUsersWhoCanCreateOrgRepo returns users which are able to create repo in organization -func GetUsersWhoCanCreateOrgRepo(ctx context.Context, orgID int64) ([]*user_model.User, error) { - users := make([]*user_model.User, 0, 10) +func GetUsersWhoCanCreateOrgRepo(ctx context.Context, orgID int64) (map[int64]*user_model.User, error) { + // Use a map, in order to de-duplicate users. + users := make(map[int64]*user_model.User) return users, db.GetEngine(ctx). Join("INNER", "`team_user`", "`team_user`.uid=`user`.id"). Join("INNER", "`team`", "`team`.id=`team_user`.team_id"). Where(builder.Eq{"team.can_create_org_repo": true}.Or(builder.Eq{"team.authorize": perm.AccessModeOwner})). - And("team_user.org_id = ?", orgID).Asc("`user`.name").Find(&users) + And("team_user.org_id = ?", orgID).Find(&users) } // SearchOrganizationsOptions options to filter organizations diff --git a/models/organization/org_test.go b/models/organization/org_test.go index 2f821e3a4..0a3836592 100644 --- a/models/organization/org_test.go +++ b/models/organization/org_test.go @@ -91,11 +91,12 @@ func TestUser_GetTeams(t *testing.T) { org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) teams, err := org.LoadTeams() assert.NoError(t, err) - if assert.Len(t, teams, 4) { + if assert.Len(t, teams, 5) { assert.Equal(t, int64(1), teams[0].ID) assert.Equal(t, int64(2), teams[1].ID) assert.Equal(t, int64(12), teams[2].ID) - assert.Equal(t, int64(7), teams[3].ID) + assert.Equal(t, int64(14), teams[3].ID) + assert.Equal(t, int64(7), teams[4].ID) } } @@ -292,7 +293,7 @@ func TestUser_GetUserTeamIDs(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expected, teamIDs) } - testSuccess(2, []int64{1, 2}) + testSuccess(2, []int64{1, 2, 14}) testSuccess(4, []int64{2}) testSuccess(unittest.NonexistentID, []int64{}) } @@ -447,7 +448,7 @@ func TestGetUsersWhoCanCreateOrgRepo(t *testing.T) { users, err = organization.GetUsersWhoCanCreateOrgRepo(db.DefaultContext, 7) assert.NoError(t, err) assert.Len(t, users, 1) - assert.EqualValues(t, 5, users[0].ID) + assert.NotNil(t, users[5]) } func TestUser_RemoveOrgRepo(t *testing.T) {