Skip to content

fix(teams): Keep a team's issue subscriptions when removing a pending invite - #129600

Merged
gricha merged 1 commit into
ref/mypy-none-lookupfrom
fix/pending-invite-team-unsubscribe
Nov 4, 2026
Merged

fix(teams): Keep a team's issue subscriptions when removing a pending invite#129600
gricha merged 1 commit into
ref/mypy-none-lookupfrom
fix/pending-invite-team-unsubscribe

Conversation

@gricha

@gricha gricha commented Nov 2, 2026

Copy link
Copy Markdown
Member

Removing a pending invite from a team deleted the team's own issue subscriptions.

When a member leaves a team, _unsubscribe_issues drops their subscriptions to the team's issues with GroupSubscription.objects.filter(group_id__in=..., user_id=member.user_id).delete(). A pending invite has no user, so user_id=None became user_id IS NULL, which matches the team's own subscription rows (team subscriptions have user_id NULL). The team then stopped getting workflow notifications for issues assigned to it. Invites can be on teams, so this happens whenever someone removes an invite from a team.

A pending invite has no subscriptions, so the helper now returns early when the member has no user. Members with a user are unaffected.

Found by the None-lookup audit behind #129532.

Evidence: test_removing_pending_invite_keeps_team_subscriptions fails without the fix:

E   AssertionError: assert False
E    +  where False = exists()
E    +    where exists = <BaseQuerySet []>.exists

and the file passes with it:

66 passed in 27.11s

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 2, 2026
@gricha
gricha added this pull request to stack #129572 November 2, 2026 23:01
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from a73ce9b to ad35d54 Compare November 2, 2026 23:06
@gricha gricha mentioned this pull request Nov 2, 2026
Merged
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from ad35d54 to a042da3 Compare November 2, 2026 23:08
@sentry

sentry Bot commented Nov 2, 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 marked this pull request as ready for review November 2, 2026 23:11
@gricha
gricha requested review from azulus and skaasten November 2, 2026 23:11
@gricha
gricha requested a review from a team as a code owner November 2, 2026 23:11
@gricha
gricha requested a review from cvxluo November 2, 2026 23:15
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch 3 times, most recently from 08ca54c to c121454 Compare November 2, 2026 23:42
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch 3 times, most recently from cf40834 to 61c71df Compare November 3, 2026 04:03
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from 61c71df to 7db689a 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 fix/pending-invite-team-unsubscribe branch from 7db689a to fb5fae1 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 a review from a team as a code owner November 3, 2026 20:00
@gricha
gricha requested review from a team as code owners November 3, 2026 20:00
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from fb5fae1 to d20d2c4 Compare November 3, 2026 20:00
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from d20d2c4 to 2e79050 Compare November 3, 2026 20:09
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from 2e79050 to 2f42f5a Compare November 3, 2026 20:51
cvxluo
cvxluo approved these changes Nov 3, 2026
View reviewed changes
Comment thread src/sentry/core/endpoints/organization_member_team_details.py
Unsubscribe user from issues the team is subscribed to
"""
# A pending invite has no subscriptions, and user_id=None would match the team's own.
if member.user_id is None:

@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 require_user in the delete func?

@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.

yeah this is member, not request user, so it can't just check the require_user

@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch 2 times, most recently from 71449a3 to bdb51ed Compare November 3, 2026 23:19
@gricha
fix(teams): Keep a team's issue subscriptions when removing a pending…
6204cf6 
… invite

Removing a member from a team unsubscribes them from the team's issues with
GroupSubscription.filter(user_id=member.user_id). A pending invite has no
user, so user_id=None matched the team's own subscriptions (user_id is NULL on
those rows) and deleted them.
@gricha
gricha force-pushed the fix/pending-invite-team-unsubscribe branch from bdb51ed to 6204cf6 Compare November 4, 2026 00:45
@gricha
gricha merged commit 519be18 into master Nov 4, 2026
79 of 121 checks passed
@gricha
gricha deleted the fix/pending-invite-team-unsubscribe branch November 4, 2026 01:15
gricha added a commit that referenced this pull request Nov 4, 2026
@gricha
ref(lint): Flag ORM lookups that may match NULL rows (#129532)
f7787e9 
Adds `lookupMayMatchNullRows` to the decision-model linter's security
suite.

An exact ORM lookup whose value can be None, like
`filter(user_id=request.user.id)` for an API key, compiles to `user_id
IS NULL` and matches rows with no user: pending invites, deleted users'
exports, token-created rows. That caused #129441, #129442 and #129443,
and the two fixes below this PR (#129538, #129540). Flake8 rule S032
only catches a literal `acting_user_id(...)` keyword, so it missed all
of them.

The rule runs on changed files only, at `warn`. Its report says how to
fix it: decide whether None means no rows (check for None first, or
`owned_by_acting_user()` for the caller's own rows), no filter, or NULL
rows (pass None or `__isnull=True`).

An earlier version of this PR was a mypy plugin. On top of the
request.user stack it found 147 such lookups in 74 files; that list is
the cleanup worklist, handled in follow-up PRs instead of an allowlist
here.

Checked locally with this config: the full security suite in diff mode,
as CI runs it, on 12 diffs. Seven add one of this week's bugs and five
add lookups that are fine. The threshold is 0.7.

| Diff adds | P(TRUE) | Flagged |
|---|---|---|
| Data export lookup by `request.user.id` (#129442) | 0.81 | yes |
| `is_member` projects by `request.user.id` (#129443) | 0.83 | yes |
| Member roles by `request.user.id` (#129538) | 0.80 | yes |
| Replay access by `request.user.id` (#129540) | 0.79 | yes |
| Team unsubscribe by `member.user_id` (#129600) | 0.72 | yes |
| Dashboard edit check (#129441) | 0.51 | no |
| Sentry app token rescope (#129602) | 0.10 | no |
| An `__isnull=False` guard | 0.27 | no |
| An `application_id is None` guard | 0.14 | no |
| `dist_id=dist.id if dist else None` | 0.13 | no |
| Global user options (`project_id=None`) | 0.46 | no |
| Derived `pipeline_hash` (commented as meant) | 0.35 | no |

The two misses: #129441's None comes through an untyped parameter and a
`==`, and #129602 depends on `SentryApp.application` being nullable,
which the diff doesn't show. A diff that only deletes a guard isn't
checked either, since the linter asks about added lines. An earlier,
more abstract wording flagged none of the seven.



**Reviewer cases (re-measured Nov 2, changeset mode, full `security`
suite):**

| Case | Expected | P(TRUE) |
|---|---|---|
| `UserEmail.objects.filter(user_id=authenticator.user_id)`, NOT NULL
column | no warning | 0.19 |
| `filter(project_id=project_id)` where None means the global user
option | no warning | 0.49 |
|
`organization_service.check_membership_by_id(user_id=request.user.id)`,
lookup behind a service call | warning | 0.22 (missed) |

The two false-positive risks stay under the 0.7 threshold. A rewording
that limits the rule to nullable columns, exempts "None means global"
and mentions service calls dropped two real bugs below it (#129540 to
0.63, #129600 to 0.55) and still missed the service call. A lighter one
kept 5 of 8 but left #129540 and #129600 at 0.72 and 0.70. So the
wording stays; lookups behind service calls are a known gap.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azulus azulus azulus approved these changes
@cvxluo cvxluo cvxluo approved these changes
@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.

3 participants

@gricha @azulus @cvxluo