From c2915630a787888c5d3940fb8bc6548530a8c06e Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 21 Jan 2023 11:55:25 +0100 Subject: [PATCH 1/4] Don't return duplicated users who can create org repo - 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, which is adding `DISTINCT` keyword to `` `user`.id ``. --- models/fixtures/team.yml | 11 +++++++++++ models/fixtures/team_user.yml | 6 ++++++ models/fixtures/user.yml | 2 +- models/organization/org.go | 5 ++++- models/organization/org_test.go | 7 ++++--- 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index ea47a33f1c4db..dd434d78a980c 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 845741effddf8..de4f29d977cbb 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 3afed37df9655..63a5e0f890552 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 9d9e9cda46a48..d7c5f6e08b6e9 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -403,7 +403,10 @@ func GetUsersWhoCanCreateOrgRepo(ctx context.Context, orgID int64) ([]*user_mode 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). + Asc("`user`.name"). + Distinct("`user`.id"). + Find(&users) } // SearchOrganizationsOptions options to filter organizations diff --git a/models/organization/org_test.go b/models/organization/org_test.go index 2f821e3a4c1fa..4203d15bae74d 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{}) } From 77d56f98ea2f3e70456a362f8feb8f63addf9ea7 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 22 Jan 2023 13:59:01 +0100 Subject: [PATCH 2/4] Don't use `Distinct` to avoid duplicated users --- models/organization/org.go | 9 +++------ models/organization/org_test.go | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/models/organization/org.go b/models/organization/org.go index d7c5f6e08b6e9..4bfe2819e2b5a 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -397,16 +397,13 @@ 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) { + 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"). - Distinct("`user`.id"). - Find(&users) + And("team_user.org_id = ?", orgID).Asc("`user`.name").Find(&users) } // SearchOrganizationsOptions options to filter organizations diff --git a/models/organization/org_test.go b/models/organization/org_test.go index 4203d15bae74d..0a38365924ed6 100644 --- a/models/organization/org_test.go +++ b/models/organization/org_test.go @@ -448,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) { From 3c7dcbea2fde9cff07c4d737824215b95b6bf9d2 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 22 Jan 2023 14:28:23 +0100 Subject: [PATCH 3/4] Fix sending notification of repository transfer --- models/activities/notification.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/activities/notification.go b/models/activities/notification.go index 321d543c736ae..f153eb0589aa5 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, From d49610344dd160ff886601d7553861a335d71743 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 29 Jan 2023 18:31:06 +0100 Subject: [PATCH 4/4] Add comment --- models/organization/org.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/organization/org.go b/models/organization/org.go index 4bfe2819e2b5a..05eaead60bf09 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -398,12 +398,13 @@ 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) (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