Repository navigation
feat(auth): Put service accounts on request.user as a principal - #129146
Conversation
Sentry Snapshot Testing
|
The dashboard creator on the model and the serializer read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
54744db to
c77b783
Compare
Scheduled deletion actor, onboarding, Tempest credentials, Seer agentic triage and integration direct-enable read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
update_groups' acting user, group integration and external-issue activity, and per-user issue defaults read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
Discover and Explore saved queries, saved formulas, and the events base read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
Org details, org index, teams, project index, Team lookups, org deletion actor and auth_organization_config read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
The detector and workflow creator in the validators read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
The snapshot uploader on preprod snapshot endpoints read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
has_viewed and viewed-by on replay details read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
Org details, org index, teams, project index, Team lookups, org deletion actor and auth_organization_config read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
2284703 to
01cdaf9
Compare
update_groups' acting user, group integration and external-issue activity, and per-user issue defaults read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
01cdaf9 to
e13701c
Compare
The detector and workflow creator in the validators read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
Discover and Explore saved queries, saved formulas, and the events base read the caller's human from request.user.user_id instead of request.user.id if request.user.is_authenticated else None, or is_authenticated used to mean "is a person". No behavior change: today only AnonymousUser has no user_id, and every authenticated caller's user_id is its id. Once service accounts can sit on request.user (behind auth.service-account-principal), their user_id is None, so these places record no user instead of reading an id that raises UserRequired. Split out of #128983 so owning teams can review their slice; the core change is #129146. Needs #128982 (user_id on request.user).
| @dataclasses.dataclass(frozen=True) | ||
| class ServiceAccountId: | ||
| """ | ||
| A service account's ``request.user.id``. | ||
|
|
||
| It never equals a user id, and turning it into an int raises ``UserRequired``, so a | ||
| lookup or write keyed on ``request.user.id`` fails instead of reaching the user with | ||
| the same number. | ||
|
|
||
| Temporary runtime guard: about 490 ``request.user.id`` reads still assume a user. It | ||
| goes once mypy types ``request.user`` to include ``ServiceAccountPrincipal`` (plugin, | ||
| shrinking per-module allowlist) and those reads are migrated. | ||
| """ | ||
|
|
||
| service_account_id: int | ||
|
|
||
| def __int__(self) -> NoReturn: | ||
| raise UserRequired() | ||
|
|
||
| __index__ = __int__ | ||
|
|
||
| def __str__(self) -> str: | ||
| return f"service_account:{self.service_account_id}" |
There was a problem hiding this comment.
this is kind of stupid but bear with me, it's an explicit decision i kinda run with here.
this is a temporary workaround which lets me to gracefully fail the endpoints that rely on user with "UserRequired" exception.
this will be fast followed with actual change in implementation on endpoints side + mypy change that would make sure request.user CAN be service account principal that will force callers into explicit decision.
56e5749 to
a8d7e08
Compare
a8d7e08 to
40ec8fd
Compare
40ec8fd to
b338a2d
Compare
418311f to
c82a0cd
Compare
c82a0cd to
4f531ba
Compare
4f531ba to
ac8d3fc
Compare
skaasten
left a comment
There was a problem hiding this comment.
(from an high level understanding, not the ins and outs of django auth processing. )
michelletran-sentry
left a comment
There was a problem hiding this comment.
This seems good to me!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 632f066. Configure here.
On a DRF request, a service account's request.user is a ServiceAccountPrincipal instead of AnonymousUser. set_request_principal sets it only on a DRF Request; Django's request.user stays anonymous until DRF authenticates, and session auth never sees it. Service accounts only authenticate for organizations with organizations:service-accounts, now checked at authentication too, so the flag is the one switch. The principal's user_id is None and its actor is SERVICE_ACCOUNT. Its id is a ServiceAccountId that never equals an int and raises UserRequired (403, user-required) when converted to one, so no request.user.id call site can reach the user with the same number. SentryPermission.determine_access asks has_user() and reads user_id, so a service account keeps the credential-based access it had as AnonymousUser. Sudo checks, in the decorator and the middleware, don't treat service accounts as sudo. They get their own rate-limit bucket, and UserRequired passes through RPC as an API error. The auth matrix enables the flag for its service-account rows. Rows that don't match the oracle yet are strict xfails, so the PRs that fix them have to remove them.

On a DRF request, a service account's
request.useris aServiceAccountPrincipalinstead ofAnonymousUser. Service accounts are gated byorganizations:service-accounts, which is now checked at authentication too (authenticate_service_account), so that flag is the one switch. Before, only service-account creation and the token endpoint checked it, and anything holding the shared secret could sign a service-account viewer context.The principal has no user, so
has_user()is False andacting_user_id()is None, and its actor isSERVICE_ACCOUNT. Itsidis aServiceAccountIdthat never equals an int and raisesUserRequired(403user-required) when converted to one, so norequest.user.idcall site can reach the user with the same number.Temporary:
ServiceAccountIdis a runtime guard, because about 490request.user.idreads still assume a user. It's deleted, along with the RPC pass-through forUserRequired, once mypy typesrequest.userto includeServiceAccountPrincipal(plugin, shrinking per-module allowlist) and those reads are migrated.This PR sits right above the chokepoint on purpose: every PR after it is written and tested against the real principal. Endpoints aren't blocked up front. What breaks for a service account shows up in the auth matrix, and the PRs above fix it or mark the endpoint
requires_user.What else this changes:
set_request_principalsets a service account only on a DRFRequest(its setter also copies it to Django'srequest.user). Until DRF authenticates it, Django'srequest.useris anonymous, and session auth never sees it, sinceViewerContextAuthenticationruns beforeSessionAuthentication. The check reads the credential (is_service_account_auth(auth)), notisinstance(user, ...): the middleware passes Django's lazy user, and loading it reads the session, which addsVary: Cookieto every response.test_leaves_lazy_user_unloadedguards it.SentryPermission.determine_accessaskshas_user()andacting_user_id(), so a service account keeps the credential-based access (from_rpc_auth) it had asAnonymousUser, instead of itsServiceAccountIdreaching the org lookup.has_user()beforehas_usable_password(). Sudo re-authenticates a person, sorequire_sudo()answers a caller with no user with a 403user-requiredinstead of a sudo prompt, from@sudo_requiredand from project delete. Org, team and project delete and project transfer stay user-only.UserRequiredpasses through as an API error.The auth matrix enables the flag for its service-account rows and records what still fails at this point in the stack. Rows that don't match the oracle are strict xfails, so the PR that fixes a row has to remove it. At this point 89 service-account rows are known gaps: 28 audit actors, 21 per-user serializer lookups, 20
request.user.idreads, 2serialize_generic_user, 1 project creation that needsrequire_user, 12 control-silo endpoints and 5 that check the caller's own membership or org role. The PRs above remove them. At the top of the stack, 17 remain: the control-silo and membership ones.This replaces #128983, split per review. It builds on #128779, now landed.
Stack (each PR is based on the one above it):
request.userinterfacerequest.user/request.auththrough one helperrequest.userhas_user()in access, org lookups andusers/merequires_user/require_user()serialize_generic_user