Skip to content

ref(members): Don't let service accounts manage members - #129638

Merged
gricha merged 1 commit into
fix/data-export-requires-userfrom
ref/sa-no-member-management
Nov 4, 2026
Merged

gricha merged 1 commit into
fix/data-export-requires-userfrom
ref/sa-no-member-management

Conversation

@gricha

@gricha gricha commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

Service accounts can't invite, edit, remove or provision members. They can still list members. Drops the four member files from the request.user allowlist (5 → 1).

  • Member detail endpoints (OrganizationMemberEndpoint: member details, member teams, invite request details, SCIM member details) set requires_user. They already returned 404 to API keys; a service account now gets 403 user-required, also when reading a single member (it used to get 200).
  • SCIM (SCIMEndpoint: users, teams, schemas) sets requires_user. A service account with member:write could provision members through SCIM on an SCIM-enabled org; it now gets 403. SCIM's real caller is the SCIM token's proxy user, which is unaffected.
  • Invites and invite requests (POST members/, POST invite-requests/) call require_user() first and record the inviter from it.
    • A service account gets 403. Before, it got a role validation 400 for invites, or reached the write and failed on its ServiceAccountId.
    • A legacy API key never had an org role to invite with (every role was a 400); it now gets 401. Integration tokens still work, since their caller is the integration's proxy user. Production had no invite or invite request without a user in the last 30 days (0 of 5,760 sampled invites).

The new service-account tests fail without this change:

test_service_account_cannot_read_member          assert 200 == 403
test_service_account_cannot_invite               assert 400 == 403   (role validation)
test_service_account_cannot_request_invite       assert 400 == 403   (role validation)
test_service_account_cannot_provision_users      assert 201 == 403   (SCIM created the member)

(test_service_account_cannot_remove_member already got 403 before, from the ServiceAccountId guard.)

With it, the member index, details, team-details, invite-request and SCIM suites pass, and the endpoint parity matrix moves the member and SCIM rows to user-required (318 passed). The two API-key tests from #129538 now expect 401, and still check that nothing is created.

@gricha
gricha requested review from a team as code owners November 3, 2026 01:49
@gricha
gricha added this pull request to stack #129572 November 3, 2026 01:49
@gricha
gricha requested review from cvxluo and skaasten November 3, 2026 01:49
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

decision-model-linter

api

src/sentry/core/endpoints/organization_member_index.py

  • warn readsRequestBodyOutsideSerializer = TRUE (0.91): Request body values should be read from a serializer's validated_data. This file reads a value directly from request.data or request.POST.

decision-model-linter: 18 calls on 6 files, 37,935 input tokens, $0.0016, model typesafe/jev-1.13-20260917

@sentry

sentry Bot commented Nov 3, 2026 •

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/sa-no-member-management branch from b702167 to e1c042a Compare November 3, 2026 02:27
@gricha
gricha requested a review from a team as a code owner November 3, 2026 02:27
@gricha
gricha force-pushed the ref/sa-no-member-management branch 2 times, most recently from 2286fff to f862b22 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
@gricha
gricha force-pushed the ref/sa-no-member-management branch from f862b22 to efbb57f Compare November 3, 2026 19:53
@gricha
gricha removed this pull request from stack #129650 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 20:02
@gricha
gricha requested review from a team as code owners November 3, 2026 20:02
@gricha
gricha force-pushed the ref/sa-no-member-management branch 2 times, most recently from 1b124b9 to decac06 Compare November 3, 2026 20:09
@gricha
gricha force-pushed the ref/sa-no-member-management branch from decac06 to 0a8dd99 Compare November 3, 2026 20:51
@gricha
gricha force-pushed the ref/sa-no-member-management branch 2 times, most recently from 641aab7 to bfcaeae Compare November 3, 2026 23:19
@gricha
gricha force-pushed the ref/sa-no-member-management branch from bfcaeae to 67e9df3 Compare November 4, 2026 00:44
Inviting, editing and removing members needs a user.

- Member detail endpoints (details, member teams, invite request details)
  set requires_user. They already returned 404 to callers with no user;
  a service account now gets 403 user-required instead of reaching the
  member.
- Invites and invite requests call require_user() first. A service
  account gets 403; a legacy API key, which never had an org role to
  invite with and got 400, now gets 401. No production invite without a
  user in the last 30 days.

Listing members is unchanged. Drops the four member files from the
request.user allowlist.
@gricha
gricha force-pushed the ref/sa-no-member-management branch from 67e9df3 to 67e09cc Compare November 4, 2026 01:15
@gricha
gricha merged commit c291e0e into master Nov 4, 2026
78 checks passed
@gricha
gricha deleted the ref/sa-no-member-management branch November 4, 2026 03:39
gricha added a commit that referenced this pull request Nov 4, 2026
… and client config (#129630)

Four one-off `request.user` reads, which drops their files from the
allowlist (10 → 6). What's left is member management (#129638), data
export (#129637) and `group_integration_details.py` (#129639); #129640
then deletes the allowlist.

- **Agent tokens:** `resolve_minting_principal()` rejects any caller
with no user (`has_user`), not just `AnonymousUser`. A service account
used to resolve to a *user* principal carrying its `ServiceAccountId`
and fail later; it now gets the same "Minting requires a user principal"
403 as an anonymous caller. The new assertion fails without this change:
  ```
E AssertionError: assert AgentPrincipal(type=<AgentPrincipalType.USER:
'user'>, id=ServiceAccountId(service_account_id=1)) is
<MintingPrincipalRejection.UNSUPPORTED: 'unsupported'>
  ```
- **Release serialization** takes a `RequestPrincipal`. It only forwards
the caller to `serialize()` and `serialize_generic_user()`, which
already take one, so this is type-only.
- **Notebooks** read the caller through `acting_user_id()`; same value
for users and token callers.
- **Client config** is built only from Django views, so its request is
typed as `HttpRequest`. Type-only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants