From 85192c2e9f39c7fa974c3de37e333771815ccd9a Mon Sep 17 00:00:00 2001 From: pisarz77 Date: Thu, 23 Apr 2026 18:14:29 +0200 Subject: [PATCH] Fix org team assignee/reviewer lookups for team member permissions (#37365) Fix team members missing from assignee list when `team_unit.access_mode` is 0 but the doer is owner. Fix #34871 1. Use `GetTeamUserIDsWithAccessToAnyRepoUnit` for repo assignee list 2. Load assignee list for project issues directly 3. Use `GetTeamUserIDsWithAccessToAnyRepoUnit` for repo reviewer list Signed-off-by: Jakub Pisarczyk Signed-off-by: wxiaoguang Co-authored-by: wxiaoguang Co-authored-by: silverwind Co-authored-by: Claude (Opus 4.7) Co-authored-by: Giteabot --- models/organization/org_user.go | 46 --------------------------------- models/repo/user_repo.go | 16 ++++-------- models/repo/user_repo_test.go | 38 ++++++++++++++++++++++++--- routers/web/org/projects.go | 5 ++-- services/projects/issue.go | 28 ++++++++++++++++++++ services/projects/issue_test.go | 15 +++++++++++ services/pull/reviewer.go | 11 ++------ 7 files changed, 86 insertions(+), 73 deletions(-) diff --git a/models/organization/org_user.go b/models/organization/org_user.go index 69cd9609446..627c1c2edf0 100644 --- a/models/organization/org_user.go +++ b/models/organization/org_user.go @@ -8,10 +8,7 @@ import ( "fmt" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/perm" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/log" "xorm.io/builder" @@ -129,49 +126,6 @@ func IsUserOrgOwner(ctx context.Context, users user_model.UserList, orgID int64) return results } -// GetOrgAssignees returns all users that have write access and can be assigned to issues -// of the any repository in the organization. -func GetOrgAssignees(ctx context.Context, orgID int64) (_ []*user_model.User, err error) { - e := db.GetEngine(ctx) - userIDs := make([]int64, 0, 10) - if err = e.Table("access"). - Join("INNER", "repository", "`repository`.id = `access`.repo_id"). - Where("`repository`.owner_id = ? AND `access`.mode >= ?", orgID, perm.AccessModeWrite). - Select("user_id"). - Find(&userIDs); err != nil { - return nil, err - } - - additionalUserIDs := make([]int64, 0, 10) - if err = e.Table("team_user"). - Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). - Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). - Join("INNER", "repository", "`repository`.id = `team_repo`.repo_id"). - Where("`repository`.owner_id = ? AND (`team_unit`.access_mode >= ? OR (`team_unit`.access_mode = ? AND `team_unit`.`type` = ?))", - orgID, perm.AccessModeWrite, perm.AccessModeRead, unit.TypePullRequests). - Distinct("`team_user`.uid"). - Select("`team_user`.uid"). - Find(&additionalUserIDs); err != nil { - return nil, err - } - - uniqueUserIDs := make(container.Set[int64]) - uniqueUserIDs.AddMultiple(userIDs...) - uniqueUserIDs.AddMultiple(additionalUserIDs...) - - users := make([]*user_model.User, 0, len(uniqueUserIDs)) - if len(userIDs) > 0 { - if err = e.In("id", uniqueUserIDs.Values()). - Where(builder.Eq{"`user`.is_active": true}). - OrderBy(user_model.GetOrderByName()). - Find(&users); err != nil { - return nil, err - } - } - - return users, nil -} - func loadOrganizationOwners(ctx context.Context, users user_model.UserList, orgID int64) (map[int64]*TeamUser, error) { if len(users) == 0 { return nil, nil //nolint:nilnil // return nil when there are no users diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index e15a64b01e8..28ae83a095e 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -8,6 +8,7 @@ import ( "strings" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -94,8 +95,7 @@ func GetWatchedRepos(ctx context.Context, opts *WatchedReposOptions) ([]*Reposit return db.FindAndCount[Repository](ctx, opts) } -// GetRepoAssignees returns all users that have write access and can be assigned to issues -// of the repository, +// GetRepoAssignees returns all users that have write access and can be assigned to issues or pull-requests of the repository, func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.User, err error) { if err = repo.LoadOwner(ctx); err != nil { return nil, err @@ -114,15 +114,9 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us uniqueUserIDs.AddMultiple(userIDs...) if repo.Owner.IsOrganization() { - additionalUserIDs := make([]int64, 0, 10) - if err = e.Table("team_user"). - Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). - Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). - Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? OR (`team_unit`.access_mode = ? AND `team_unit`.`type` = ?))", - repo.ID, perm.AccessModeWrite, perm.AccessModeRead, unit.TypePullRequests). - Distinct("`team_user`.uid"). - Select("`team_user`.uid"). - Find(&additionalUserIDs); err != nil { + // issues and pull requests both need "assignee list" + additionalUserIDs, err := organization.GetTeamUserIDsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypeIssues, unit.TypePullRequests) + if err != nil { return nil, err } uniqueUserIDs.AddMultiple(additionalUserIDs...) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index cd8a0f1a1f7..cd45db61d08 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -6,7 +6,12 @@ package repo_test import ( "testing" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + perm_model "code.gitea.io/gitea/models/perm" + access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -14,9 +19,14 @@ import ( "github.com/stretchr/testify/require" ) -func TestRepoAssignees(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func TestUserRepo(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + t.Run("GetIssuePostersWithSearch", testUserRepoGetIssuePostersWithSearch) + t.Run("Assignees", testUserRepoAssignees) + t.Run("AssigneesNoTeamUnit", testRepoAssigneesNoTeamUnit) +} +func testUserRepoAssignees(t *testing.T) { repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) users, err := repo_model.GetRepoAssignees(t.Context(), repo2) assert.NoError(t, err) @@ -39,9 +49,29 @@ func TestRepoAssignees(t *testing.T) { } } -func TestGetIssuePostersWithSearch(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testRepoAssigneesNoTeamUnit(t *testing.T) { + ctx := t.Context() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 32}) + require.NoError(t, repo.LoadOwner(ctx)) + require.True(t, repo.Owner.IsOrganization()) + + require.NoError(t, db.TruncateBeans(ctx, &organization.Team{}, &organization.TeamUser{}, &organization.TeamRepo{}, &organization.TeamUnit{}, &access_model.Access{})) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + team := &organization.Team{OrgID: repo.OwnerID, LowerName: "admin-team", AccessMode: perm_model.AccessModeAdmin} + require.NoError(t, db.Insert(ctx, team)) + require.NoError(t, db.Insert(ctx, &organization.TeamUser{OrgID: repo.OwnerID, TeamID: team.ID, UID: user.ID})) + require.NoError(t, db.Insert(ctx, &organization.TeamRepo{OrgID: repo.OwnerID, TeamID: team.ID, RepoID: repo.ID})) + require.NoError(t, db.Insert(ctx, &organization.TeamUnit{OrgID: repo.OwnerID, TeamID: team.ID, Type: unit.TypePullRequests, AccessMode: perm_model.AccessModeNone})) + + users, err := repo_model.GetRepoAssignees(ctx, repo) + require.NoError(t, err) + require.Len(t, users, 1) + assert.ElementsMatch(t, []int64{4}, []int64{users[0].ID}) +} + +func testUserRepoGetIssuePostersWithSearch(t *testing.T) { repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) users, err := repo_model.GetIssuePostersWithSearch(t.Context(), repo2, false, "USER") diff --git a/routers/web/org/projects.go b/routers/web/org/projects.go index 59aee819742..ae32be05757 100644 --- a/routers/web/org/projects.go +++ b/routers/web/org/projects.go @@ -11,7 +11,6 @@ import ( "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" - org_model "code.gitea.io/gitea/models/organization" project_model "code.gitea.io/gitea/models/project" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -451,9 +450,9 @@ func ViewProject(ctx *context.Context) { ctx.Data["MilestoneID"] = milestoneID // Get assignees. - assigneeUsers, err := org_model.GetOrgAssignees(ctx, project.OwnerID) + assigneeUsers, err := project_service.LoadIssuesAssigneesForProject(ctx, issuesMap) if err != nil { - ctx.ServerError("GetRepoAssignees", err) + ctx.ServerError("LoadIssuesAssigneesForProject", err) return } ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers) diff --git a/services/projects/issue.go b/services/projects/issue.go index 377c4b9d583..ece9910cd23 100644 --- a/services/projects/issue.go +++ b/services/projects/issue.go @@ -6,11 +6,14 @@ package project import ( "context" "errors" + "slices" + "strings" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" project_model "code.gitea.io/gitea/models/project" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/optional" ) @@ -86,6 +89,31 @@ func MoveIssuesOnProjectColumn(ctx context.Context, doer *user_model.User, colum }) } +func LoadIssuesAssigneesForProject(ctx context.Context, issuesMap map[int64]issues_model.IssueList) ([]*user_model.User, error) { + var issueList issues_model.IssueList + for _, colIssues := range issuesMap { + issueList = append(issueList, colIssues...) + } + err := issueList.LoadAssignees(ctx) + if err != nil { + return nil, err + } + users := make([]*user_model.User, 0, len(issueList)) + usersAdded := container.Set[int64]{} + for _, issue := range issueList { + for _, assignee := range issue.Assignees { + if !usersAdded.Contains(assignee.ID) { + usersAdded.Add(assignee.ID) + users = append(users, assignee) + } + } + } + slices.SortFunc(users, func(a, b *user_model.User) int { + return strings.Compare(a.Name, b.Name) + }) + return users, nil +} + // LoadIssuesFromProject load issues assigned to each project column inside the given project func LoadIssuesFromProject(ctx context.Context, project *project_model.Project, opts *issues_model.IssuesOptions) (results map[int64]issues_model.IssueList, _ error) { issueList, err := issues_model.Issues(ctx, opts.Copy(func(o *issues_model.IssuesOptions) { diff --git a/services/projects/issue_test.go b/services/projects/issue_test.go index 7255cdfe527..17d0fef2e6e 100644 --- a/services/projects/issue_test.go +++ b/services/projects/issue_test.go @@ -15,6 +15,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_Projects(t *testing.T) { @@ -207,4 +208,18 @@ func Test_Projects(t *testing.T) { assert.Len(t, columnIssues[3], 1) }) }) + + t.Run("LoadIssuesAssigneesForProject", func(t *testing.T) { + issuesMap := map[int64]issues_model.IssueList{} + issue1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}) + issue6 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 6}) + issuesMap[1] = issues_model.IssueList{issue1} + issuesMap[2] = issues_model.IssueList{issue6} + assignees, err := LoadIssuesAssigneesForProject(t.Context(), issuesMap) + require.NoError(t, err) + require.Len(t, assignees, 3) + require.Equal(t, "user1", assignees[0].Name) + require.Equal(t, "user10", assignees[1].Name) + require.Equal(t, "user2", assignees[2].Name) + }) } diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index 52f2f3401c2..139aeeb9503 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -40,15 +40,8 @@ func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post uniqueUserIDs.AddMultiple(collaboratorIDs...) if repo.Owner.IsOrganization() { - additionalUserIDs := make([]int64, 0, 10) - if err := e.Table("team_user"). - Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). - Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). - Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)", - repo.ID, perm.AccessModeRead, unit.TypePullRequests). - Distinct("`team_user`.uid"). - Select("`team_user`.uid"). - Find(&additionalUserIDs); err != nil { + additionalUserIDs, err := organization.GetTeamUserIDsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) + if err != nil { return nil, err } uniqueUserIDs.AddMultiple(additionalUserIDs...)