Skip to content

Fix org team assignee/reviewer lookups for team member permissions#37365

Merged
silverwind merged 14 commits intogo-gitea:mainfrom
pisarz77:fix/team-unit-zero-access-mode
Apr 23, 2026
Merged

Fix org team assignee/reviewer lookups for team member permissions#37365
silverwind merged 14 commits intogo-gitea:mainfrom
pisarz77:fix/team-unit-zero-access-mode

Conversation

@pisarz77
Copy link
Copy Markdown
Contributor

@pisarz77 pisarz77 commented Apr 22, 2026

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

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 22, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 22, 2026

The proper fix should use GetUserIDsWithUnitAccess or GetTeamUserIDsWithAccessToAnyRepoUnit

@wxiaoguang wxiaoguang marked this pull request as draft April 22, 2026 12:07
@pisarz77 pisarz77 force-pushed the fix/team-unit-zero-access-mode branch from 6d8aae4 to 87f18e7 Compare April 22, 2026 19:46
@pisarz77
Copy link
Copy Markdown
Contributor Author

Updated per review: dropped the migration entirely and refactored GetRepoAssignees to use organization.GetTeamUserIDsWithAccessToAnyRepoUnit, which also matches teams via team.authorize so Owner-team members come through even when team_unit.access_mode is stale.

Two calls preserve the previous condition (write to any unit + read on PullRequests). Existing TestRepoAssignees passes locally. Force-pushed to the branch.

@pisarz77 pisarz77 marked this pull request as ready for review April 22, 2026 19:49
@pisarz77
Copy link
Copy Markdown
Contributor Author

Used GetTeamUserIDsWithAccessToAnyRepoUnit directly. GetUserIDsWithUnitAccess isn't reachable from models/repo without creating an import cycle (models/perm/access already imports models/repo), so the function is effectively re-implemented in place: read the access table once, then delegate the team-member lookup to the helper for both the "write on any repo unit" and "read on PullRequests" cases.

Happy to move GetRepoAssignees into models/perm/access (or another package that can import access) and collapse to two GetUserIDsWithUnitAccess calls instead if you'd prefer that shape — that touches ~12 callers across routers/ and models/. Let me know which direction you want.

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 22, 2026

Are you sure GetRepoAssignees is not right? It already calls GetTeamUserIDsWithAccessToAnyRepoUnit, the SQL already uses team.authorize

The SQL you showed Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). doesn't belong to GetRepoAssignees either.

image

… helpers

`GetRepoAssignees`, `GetOrgAssignees` and `services/pull.GetReviewers`
all collected org team members by joining `team_user` directly against
`team_unit` and filtering on `team_unit.access_mode`. On instances where
some `team_unit` rows ended up with `access_mode = 0` — observed on
databases that were upgraded across several releases — members of the
affected team (typically the `Owners` team) were silently dropped from:

* @mention autocomplete in issues and pull requests
* assignee / reviewer selection (per-repo and org-level project boards)
* email notifications triggered by mentions

This matches the symptoms reported in go-gitea#34871.

Replace each ad-hoc join with the existing helper
`organization.GetTeamUserIDsWithAccessToAnyRepoUnit`, whose query
already handles both sources of truth via
`team.authorize >= ? OR team_unit.access_mode >= ?`. For the org-wide
path (`GetOrgAssignees`) add a matching helper
`GetTeamUserIDsWithAccessToAnyRepoUnitInOrg` that applies the same OR
logic without a per-repo scope.

Also fix a pre-existing guard in `GetOrgAssignees` that skipped the
final user lookup whenever direct-access users were empty but
team-based users were not.

No schema changes; behaviour for healthy instances is identical (the
helper's OR is a superset of the previous condition, but on healthy
data the two branches match). `TestRepoAssignees`,
`TestRepoGetReviewers` and `TestRepoGetReviewerTeams` continue to pass
locally.

Signed-off-by: Jakub Pisarczyk <pisarz77@gmail.com>
@pisarz77 pisarz77 force-pushed the fix/team-unit-zero-access-mode branch from 87f18e7 to e009eb2 Compare April 23, 2026 07:59
@pisarz77 pisarz77 changed the title Fix team members missing from assignees when team_unit.access_mode is 0 Route org team assignee/reviewer lookups through team.authorize-aware helpers Apr 23, 2026
@pisarz77
Copy link
Copy Markdown
Contributor Author

Good catch — the same raw team_user × team_unit JOIN existed in GetOrgAssignees (org-level assignee picker, e.g. org Kanban project boards) and services/pull.GetReviewers (PR reviewer picker). Extended the PR to cover all three call sites:

  • GetRepoAssignees (per-repo) → GetTeamUserIDsWithAccessToAnyRepoUnit
  • GetReviewers (per-repo, PR) → same helper
  • GetOrgAssignees (org-wide) → new sibling helper GetTeamUserIDsWithAccessToAnyRepoUnitInOrg (same OR logic, no per-repo filter)

Also picked up a pre-existing guard in GetOrgAssignees that skipped the final user lookup when the direct-access list was empty but the team-based list wasn't.

TestRepoAssignees, TestRepoGetReviewers, TestRepoGetReviewerTeams all pass locally. No schema changes.

@wxiaoguang
Copy link
Copy Markdown
Contributor

The changed GetOrgAssignees and newly added GetTeamUserIDsWithAccessToAnyRepoUnitInOrg actually is not right.

// 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) {

What if user-a has no access to repo-1? Actually the design here is totally a mess and no clear fix:

  • org-level project can contain any issue from any repo
  • some users have access to repo-1 , but not repo-2, while some users can be on the contrary.

@wxiaoguang
Copy link
Copy Markdown
Contributor

To make things simple, it can simpliy list all org members for the "project assignee list"

assigneeUsers, err := org_model.GetOrgAssignees(ctx, project.OwnerID) -> change it to list all members

@wxiaoguang
Copy link
Copy Markdown
Contributor

OK, even simpler, since you already have all issues for all project columns, so you should already be able to know all assignees here.

@wxiaoguang wxiaoguang mentioned this pull request Apr 23, 2026
25 tasks
@wxiaoguang wxiaoguang added the backport/done All backports for this PR have been created label Apr 23, 2026
@wxiaoguang wxiaoguang changed the title Route org team assignee/reviewer lookups through team.authorize-aware helpers Fix org team assignee/reviewer lookups for team member permissions Apr 23, 2026
@wxiaoguang wxiaoguang added backport/v1.26 This PR should be backported to Gitea 1.26 type/bug and removed backport/done All backports for this PR have been created labels Apr 23, 2026
@wxiaoguang wxiaoguang added this to the 1.27.0 milestone Apr 23, 2026
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 23, 2026
@wxiaoguang wxiaoguang requested a review from Copilot April 23, 2026 08:55
- Add TestRepoAssigneesIncludesOwnerTeamWithZeroUnitAccessMode covering
  the case where team.authorize=Owner and team_unit.access_mode=0
- Note at both call sites that the helper matches via team.authorize
- Fix ServerError label to match function name

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind
Copy link
Copy Markdown
Member

Some cleanups and a test in b7ccc56.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 23, 2026
@silverwind silverwind enabled auto-merge (squash) April 23, 2026 14:46
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 23, 2026
@wxiaoguang wxiaoguang marked this pull request as draft April 23, 2026 15:17
auto-merge was automatically disabled April 23, 2026 15:17

Pull request was converted to draft

Comment thread models/repo/user_repo.go Outdated
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Comment thread services/pull/reviewer.go Outdated
@wxiaoguang wxiaoguang marked this pull request as ready for review April 23, 2026 15:31
@silverwind silverwind merged commit 85192c2 into go-gitea:main Apr 23, 2026
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 23, 2026
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 23, 2026
silverwind added a commit that referenced this pull request Apr 23, 2026
…37365) (#37391)

Backport #37365 by @pisarz77

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 <pisarz77@gmail.com>
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: pisarz77 <pisarz77@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com>
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 24, 2026
* origin/main:
  Allow fast-forward-only merge when signed commits are required (go-gitea#37335)
  Introduce `ActionRunAttempt` to represent each execution of a run (go-gitea#37119)
  Move review request functions to a standalone file (go-gitea#37358)
  Fix repo init README EOL (go-gitea#37388)
  Fix org team assignee/reviewer lookups for team member permissions (go-gitea#37365)
  Remove external service dependencies in migration tests (go-gitea#36866)
  Extend issue context popup beyond markdown content (go-gitea#36908)

# Conflicts:
#	routers/api/v1/repo/action.go
#	web_src/js/components/RepoActionView.vue
silverwind added a commit to mohammad-rj/gitea that referenced this pull request Apr 24, 2026
* origin/main: (127 commits)
  Refactor pull request view (1) (go-gitea#37380)
  Improve AGENTS.md (go-gitea#37382)
  Remove dead CSS (go-gitea#37376)
  Add pr-review e2e test and speed up e2e tests (go-gitea#37345)
  Drop Fomantic tab, checkbox and form patches (go-gitea#37377)
  fix: dump with default zip type produces uncompressed zip (go-gitea#37401)
  Allow fast-forward-only merge when signed commits are required (go-gitea#37335)
  Introduce `ActionRunAttempt` to represent each execution of a run (go-gitea#37119)
  Move review request functions to a standalone file (go-gitea#37358)
  Fix repo init README EOL (go-gitea#37388)
  Fix org team assignee/reviewer lookups for team member permissions (go-gitea#37365)
  Remove external service dependencies in migration tests (go-gitea#36866)
  Extend issue context popup beyond markdown content (go-gitea#36908)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  ...

# Conflicts:
#	modules/eventsource/event.go
#	tests/e2e/events.test.ts
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 25, 2026
* origin/main: (51 commits)
  Fix color regressions, add `priority` color (go-gitea#37417)
  [skip ci] Updated translations via Crowdin
  Stabilize e2e logout propagation test (go-gitea#37403)
  refactor: serve site manifest via `/assets/site-manifest.json` endpoint (go-gitea#37405)
  feat(security): set X-Content-Type-Options: nosniff by default (go-gitea#37354)
  Refactor pull request view (1) (go-gitea#37380)
  Improve AGENTS.md (go-gitea#37382)
  Remove dead CSS (go-gitea#37376)
  Add pr-review e2e test and speed up e2e tests (go-gitea#37345)
  Drop Fomantic tab, checkbox and form patches (go-gitea#37377)
  fix: dump with default zip type produces uncompressed zip (go-gitea#37401)
  Allow fast-forward-only merge when signed commits are required (go-gitea#37335)
  Introduce `ActionRunAttempt` to represent each execution of a run (go-gitea#37119)
  Move review request functions to a standalone file (go-gitea#37358)
  Fix repo init README EOL (go-gitea#37388)
  Fix org team assignee/reviewer lookups for team member permissions (go-gitea#37365)
  Remove external service dependencies in migration tests (go-gitea#36866)
  Extend issue context popup beyond markdown content (go-gitea#36908)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot add an organization owner as a Pull Request reviewer

6 participants