Skip to content

ref(typing): Type request.user with service accounts outside an allowlist - #129402

Merged
gricha merged 1 commit into
ref/sa-typed-helpersfrom
ref/mypy-request-user-principal
Nov 3, 2026
Merged

ref(typing): Type request.user with service accounts outside an allowlist#129402
gricha merged 1 commit into
ref/sa-typed-helpersfrom
ref/mypy-request-user-principal

Conversation

@gricha

@gricha gricha commented Oct 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

mypy now types DRF request.user as User | AnonymousUser | ServiceAccountPrincipal, except in files on tools/mypy_helpers/request_user_allowlist.txt. Those keep today's type, so mypy stays green, while every other file must narrow before it reads user-only attributes.

  • Plugin: an attribute hook on rest_framework.request.Request.user returns the union outside the allowlist. report_config_data rechecks a cached module when it joins or leaves the list; the list is re-read when the file changes, so a running dmypy uses the current list for any file it rechecks. dmypy doesn't recheck an unchanged file just because it joined or left the list; touch the file or restart the daemon.
  • Sentry only: files outside this repo keep today's type. getsentry loads this plugin through a tools -> ../sentry/tools symlink, and without this its typing job failed with 10 errors in 5 files. test_drf_request_user_keeps_its_type_outside_sentry runs mypy the way getsentry does.
  • ServiceAccountPrincipal.id is hidden from mypy (if not TYPE_CHECKING) but kept at runtime, so ServiceAccountId still guards unconverted reads.
  • Allowlist: 23 files, exactly the ones with errors once the PRs below land (31 errors): member management (4), issue search (5, after fix(issues): Match no issues for my_teams when the caller has no teams #129424), features.batch_has (3) and 11 one-offs. A test fails if the list grows or names a deleted file, and another fails if the hook stops applying.
  • AGENTS.md: a two-line note that fixing a file means removing it from the list.

No runtime change. It lands after the fixes: the PRs below narrow the rest, and each later fix removes its files from the list.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 30, 2026
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 54aacbe to a6954ca Compare October 31, 2026 01:29
@gricha
gricha changed the base branch from master to ref/principal-helper-examples October 31, 2026 01:29
@gricha gricha mentioned this pull request Oct 31, 2026
Merged
@gricha
gricha added this pull request to stack #129364 October 31, 2026 01:30
@gricha
gricha removed this pull request from stack #129364 November 1, 2026 04:24
@gricha
gricha changed the base branch from ref/principal-helper-examples to ref/sa-narrow-widen November 1, 2026 04:24
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from a6954ca to 8d3da3d Compare November 1, 2026 04:24
@gricha
gricha added this pull request to stack #129440 November 1, 2026 04:24
This was referenced Nov 1, 2026
Merged
Merged
Merged
Merged
Merged
@sentry

sentry Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Sentry Snapshot Testing

Name Added Removed Changed Renamed Unchanged Skipped Status
sentry-frontend
sentry-frontend		 0 0 0 0 780 0 ✅ Unchanged

⚙️ sentry-frontend Snapshot Settings

@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 8d3da3d to 9be4ce4 Compare November 1, 2026 04:57
@gricha gricha mentioned this pull request Nov 2, 2026
Merged
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 9be4ce4 to c5539a0 Compare November 2, 2026 19:55
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from c5539a0 to 4ed979e Compare November 2, 2026 20:14
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 4ed979e to 807eec6 Compare November 2, 2026 20:29
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 807eec6 to 51fe411 Compare November 2, 2026 20:33
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 51fe411 to 63a1150 Compare November 2, 2026 20:35
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 63a1150 to 293e9f4 Compare November 2, 2026 21:03
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 413cb8c to 1a545ff Compare November 2, 2026 23:31
@gricha
gricha requested a review from cvxluo November 2, 2026 23:36
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 1a545ff to 16df09d Compare November 2, 2026 23:42
gricha added a commit that referenced this pull request Nov 3, 2026
@gricha
ref(typing): Let shared helpers take a service account (#129413)
5a9aeef 
The helpers that endpoints hand `request.user` to now take a
`RequestPrincipal`: feature gates (`get_excluded_detector_types`,
`has_investigations_access`, `should_serve_action_log_activity`), Seer
agent access (`SeerAgentClient`, `has_seer_agent_access_with_detail`,
`collect_user_org_context`, `commit_author_for_user`), `is_demo_user`,
`has_staff_option`, `TeamManager.get_for_user` and the group activity
serializers.

- Most only pass the caller on to `features.has()` or `serialize()`, or
already check `has_user()`: type-only.
- `SeerAgentClient` reads the user id once through `acting_user_id()`
instead of six `hasattr(self.user, "id")` checks. `is_demo_user`,
`collect_user_org_context` and `commit_author_for_user` use
`acting_user_id()` / `has_user()`.

Users, agents and token callers get the same values as before. A service
account now gets no user (no `user_id` on its Seer runs, org context
without personal teams) instead of a `ServiceAccountId` that 403s when
used as an int.

Without it, #129402's mypy check would flag 14 more files.
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch 2 times, most recently from d978051 to ca074d2 Compare November 3, 2026 00:34
@gricha
gricha force-pushed the ref/mypy-request-user-principal branch 2 times, most recently from 0bc63b8 to 8580c43 Compare November 3, 2026 04:41
@gricha
gricha removed this pull request from stack #129572 November 3, 2026 06:52
@gricha
gricha added this pull request to stack #129650 November 3, 2026 06:52
cvxluo
cvxluo approved these changes Nov 3, 2026
View reviewed changes
Comment thread src/sentry/service_accounts/principal.py
@property
def id(self) -> ServiceAccountId:
return ServiceAccountId(self.service_account_id)
# Hidden from mypy so request.user.id needs narrowing; kept at runtime as a guard.

@cvxluo cvxluo Nov 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just delete this? seems a little insane to keep it in runtime but not in typing

@gricha gricha Nov 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing at the top of the stack

@gricha
gricha force-pushed the ref/mypy-request-user-principal branch from 8580c43 to c39801d Compare November 3, 2026 19:53
@gricha
gricha requested review from a team as code owners November 3, 2026 19:53
@gricha gricha mentioned this pull request Nov 3, 2026
Merged
@gricha
gricha removed this pull request from stack #129650 November 3, 2026 19:53
@gricha
gricha changed the base branch from ref/sa-narrow-widen to ref/sa-typed-helpers November 3, 2026 19:53
@gricha
gricha added this pull request to stack #129747 November 3, 2026 19:54
@gricha
gricha requested review from a team as code owners November 3, 2026 19:59
@gricha
ref(typing): Type request.user with service accounts outside an allow…
4792f48 
…list

request.user on a DRF Request can be a ServiceAccountPrincipal, but mypy
didn't know, so nothing forced callers to narrow. The mypy plugin now types
it as User | AnonymousUser | ServiceAccountPrincipal, except in the 23 files
on tools/mypy_helpers/request_user_allowlist.txt, which keep today's type.
ServiceAccountPrincipal.id is hidden from mypy but still raises UserRequired
at runtime.

A test fails if the allowlist grows or lists a deleted file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments
@cvxluo cvxluo cvxluo approved these changes
@azulus azulus Awaiting requested review from azulus
@skaasten skaasten Awaiting requested review from skaasten

Assignees

No one assigned

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gricha @cvxluo