[barborico]

Summary

Who's responsible for reviewing a request (all kinds) is now defined once, in api/models/request_reviewers.py: the eligible reviewers at the nearest owner level that has any. Notifications (open and close), all three "Assigned to Me" views, and an updated reviewer panel on the request detail pages use this definition.

Urgency: 2 weeks?
Expected review effort: HIGH (~1h)

Motivation

I noticed that, as an Access admin, I'm assigned a number of "confused deputy" reviews. This led me to discover that the reviewer chain was implemented separately in seven places that disagreed:

• An Access admin's "Assigned to Me" group requests matched "All", including requests for apps that have owners.
• Access and role requests for unowned groups were assigned to nobody.
• App-group creation requests for unowned apps notified nobody.
• Close notifications went to every owner at every level plus every Access admin.
• The request pages rebuilt the chain in the browser, inconsistently.

My intent here is one principle, reflected identically in "Assigned to Me", notification routing, and the UI. The one place that should probably be brought into alignment but was left out of scope for now is endpoint authZ.

The Routing Principle

The owner levels, nearest first:

Request	Owner levels
Access request, individual or role-based, for group G	owners of G → owners of G's app (app groups only) → Access admins
Group request to create G	owners of the requested app (app groups only) → Access admins

Who counts as an eligible reviewer at a level:

• Not the requester.
• Not deleted.
• Unless an Access admin, not blocked by a disallow_self_add_* tag. A role member approving a role-based request would be adding themselves; this mirrors CheckForSelfAdd.execute_for_role.

Screenshots

The pending request details view includes a list of possible reviewers, e.g.:

Description of Changes

• api/models/request_reviewers.py
  • Each owner level is one SQL condition correlated to the request row.
  • The same conditions answer both directions: get_assigned_reviewers(request) ("who reviews this") and is_assigned_reviewer(Model, user_id) ("which requests does this user review").
  • get_eligible_reviewers_by_level lists the eligible reviewers per level, nearest first, each user once at their nearest level; the first level is the assigned one.
  • get_all_possible_request_approvers and api/models/access_request.py are removed.
• Notifications.
  • The create operations notify the assigned reviewers.
  • Close notifications (approve, reject, expiry, direct grants that fulfil a request) go to the reviewers assigned before the closing change. ModifyGroupUsers, ModifyRoleGroups, DeleteGroup, and UnmanageGroup capture them before changing ownership.
  • RejectAccessRequest and RejectRoleRequest gain an optional assigned_reviewers keyword.
  • DeleteApp deletes the app's owners group last, so the other groups' close notifications still see the app owners.
• "Assigned to Me". The access, role, and group request lists filter with is_assigned_reviewer. This replaces three divergent blocks, including a loop over every app.
• Reviewer panel.
  • New routes GET /api/{requests,role-requests,group-requests}/{id}/reviewers return reviewers_by_level: the eligible reviewers by owner level, nearest first. The first level is the assigned one; an empty list means no one can review the request.
  • src/components/RequestReviewers.tsx renders them on all three request pages: one table per level, with an "Assigned" chip on the first. Only people who can act on the request are listed, so the requester and tag-blocked owners don't appear.
  • The access and role pages drop their four member queries each. The role page's admin note about tag-blocked owners is removed; it relied on blocked owners being listed, and the admins it targeted never saw it.
• Performance.
  • The app-owner level is a single, flat EXISTS, so Postgres can use a semi-join.
  • A new migration indexes app_group.app_id.
• Copy and docs.
  • The homepage guide gets a "Who reviews a request?" entry, and the request guides link to it.
  • "Assigned to Me" tooltips now say "assigned to you for review", since those views show every status.
  • NotificationPluginSpec, the README, and .claude/CLAUDE.md ("Approver routing") are updated.

Behavior changes operators will see

• Admins are notified of app-group creation requests for apps with no owners.
• The requester never appears in approvers, and tag-blocked app owners are no longer notified of role requests.
• Close notifications go to the assigned reviewers only.
  • Access admins no longer get a completion notice for every closed request.
  • approvers in *_completed hooks is a list (it was a set).
• Admins' "Assigned to Me" narrows to requests where admins are the nearest eligible level. Access and role request views now include requests for unowned groups.
• New migration: a plain index on app_group.app_id, built non-concurrently on a small table.
• Hook signatures, the approvers parameter name, and the group_owners audit-log field are unchanged. API changes are additive.

Validation of Changes

• tests/test_request_reviewers.py

  • A scenario matrix covers the three request kinds: sole owner is the requester, unowned apps, admin requester, ended or deleted owners, tag-blocked group and app owners under both keys, disabled and app-inherited tags, nobody eligible.
  • An agreement test checks, for every scenario and user, that the user is assigned exactly when "Assigned to Me" lists the request. It also checks that the reviewers listed across all levels are exactly the users the resolve checks permit to approve (reproduced from the same primitives), and that the first level is the assigned one.
  • Close-timing tests cover role-ownership approval, batch adds, delete group, delete app, unmanage group, and a snapshot miss.
  • Route tests cover /reviewers: shape, 404, 401.

• Every commit passes on its own: ruff, ty, pytest (1235 at the tip, rebased on main), vitest (206), tsc --noEmit, and prettier.

• Postgres: reviewer tests pass on a local 15.6 with SAWarning as errors, as does the full suite; the request and reviewer test files also pass on Postgres 16. Migrations pass on SQLite and Postgres: upgrade, alembic check, then downgrade -1 and upgrade again.

• Benchmark: an admin's "Assigned to Me" count queries on local Postgres. Dataset: 20k users, 234k memberships, 100k access and 20k role requests.

  Query	Before	After
  Access requests, pending	11.2 s	57 ms
  Access requests, all statuses	98.9 s	515 ms
  Role requests, pending	78.8 s	24 ms
  Role requests, all statuses	over 300 s	218 ms

  Non-admin lists stay under 16 ms.

• Manual: with seed data, the requester viewed an access, role, and group request page. Each panel listed only eligible reviewers, nearest level first and marked as assigned (see the screenshots).

• I ran Matt Pocock's PR review skill and found no violations.

Guidance for Reviewers

• Review commit by commit. Each commit is one logical step and passes the test suite on its own, and the commit description explains the intent.
• Who may approve or reject is intentionally unchanged; assignment is not authorization. A follow-up can build the resolve checks on the same owner levels. One reason to do that is so a tag-blocked role-request approval becomes an explicit 403 instead of a silent no-op.

🤖 Generated with Claude Code