Skip to content

Commit 6f4a9d9

Browse files
leeandherclaude
andcommitted
fix(slack): Validate user org membership in Seer org resolution
Address PR review feedback: - Change SLACK_PROVIDERS from set to list to match the RPC method's expected `list[str]` parameter type, preventing serialization errors - Check org status before calling get_installation to avoid unnecessary queries for inactive orgs - Verify the requesting Slack user belongs to the resolved org when their identity is linked, preventing cross-org data access when multiple orgs share a Slack workspace - Fix inaccurate comments about DM fallback behavior Refs ISWF-2388 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 6a28028 commit 6f4a9d9

File tree

2 files changed

+68
-9
lines changed

2 files changed

+68
-9
lines changed

src/sentry/integrations/slack/webhooks/event.py

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
"Hold on, I've seen this one before...",
5656
"It worked on my machine...",
5757
]
58-
SLACK_PROVIDERS = {IntegrationProviderSlug.SLACK, IntegrationProviderSlug.SLACK_STAGING}
58+
SLACK_PROVIDERS = [IntegrationProviderSlug.SLACK, IntegrationProviderSlug.SLACK_STAGING]
5959

6060

6161
@all_silo_endpoint # Only challenge verification is handled at control
@@ -351,6 +351,8 @@ def _resolve_seer_organization(
351351
Returns ``(organization_id, installation)`` or ``None`` when the
352352
event should be halted (the halt reason is already recorded).
353353
354+
We also check that the requesting user is a member of the organization that Seer is accessing.
355+
354356
Note: There is a limitation here of only grabbing the first organization with access to Seer.
355357
If a Slack installation corresponds to multiple organizations with Seer access, this will not work,
356358
and must be revisited.
@@ -364,6 +366,8 @@ def _resolve_seer_organization(
364366
lifecycle.record_halt(SeerSlackHaltReason.NO_VALID_INTEGRATION)
365367
return None
366368

369+
identity_user = slack_request.get_identity_user()
370+
367371
lifecycle.add_extra("organization_ids", [oi.organization_id for oi in ois])
368372
for oi in ois:
369373
organization_id = oi.organization_id
@@ -372,17 +376,22 @@ def _resolve_seer_organization(
372376
except Organization.DoesNotExist:
373377
continue
374378

375-
installation = slack_request.integration.get_installation(
376-
organization_id=organization_id
377-
)
378-
assert isinstance(installation, SlackIntegration)
379-
380379
if organization.status != OrganizationStatus.ACTIVE:
381380
continue
382381

383382
if not SlackExplorerEntrypoint.has_access(organization):
384383
continue
385384

385+
# When the user's identity is linked, verify they belong to this
386+
# org. If not linked the downstream task will prompt to link.
387+
if identity_user and not organization.has_access(identity_user):
388+
continue
389+
390+
installation = slack_request.integration.get_installation(
391+
organization_id=organization_id
392+
)
393+
assert isinstance(installation, SlackIntegration)
394+
386395
return organization_id, installation
387396
lifecycle.record_halt(SeerSlackHaltReason.NO_VALID_ORGANIZATION)
388397
return None
@@ -394,7 +403,7 @@ def _handle_seer_mention(
394403
) -> Response | None:
395404
"""Shared handler for app mentions and DMs that trigger the Seer workflow.
396405
397-
Returns ``None`` when the feature flag is off (DM messages only),
406+
Returns ``None`` when org resolution fails (DM messages only),
398407
allowing the caller to fall back to alternative handling.
399408
"""
400409
with MessagingInteractionEvent(
@@ -411,8 +420,8 @@ def _handle_seer_mention(
411420

412421
result = self._resolve_seer_organization(slack_request, lifecycle)
413422
if result is None:
414-
# For DMs, return None on feature-flag halt so caller can
415-
# fall back to the help message.
423+
# For DMs, return None on org resolution failure so caller
424+
# can fall back to the help message.
416425
if interaction_type == MessagingInteractionType.DM_MESSAGE:
417426
return None
418427
return self.respond()

tests/sentry/integrations/slack/webhooks/events/test_app_mention.py

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22

33
from sentry.integrations.messaging.metrics import SeerSlackHaltReason
44
from sentry.models.organization import Organization
5+
from sentry.silo.base import SiloMode
56
from sentry.testutils.asserts import assert_halt_metric
7+
from sentry.testutils.silo import assume_test_silo_mode
8+
from sentry.users.models.identity import Identity, IdentityStatus
69

710
from . import SEER_EXPLORER_FEATURES, BaseEventTest
811

@@ -116,3 +119,50 @@ def test_app_mention_org_not_found(self, mock_apply_async, mock_record):
116119
assert resp.status_code == 200
117120
mock_apply_async.assert_not_called()
118121
assert_halt_metric(mock_record, SeerSlackHaltReason.NO_VALID_ORGANIZATION)
122+
123+
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
124+
@patch("sentry.seer.entrypoints.slack.tasks.process_mention_for_slack.apply_async")
125+
def test_app_mention_linked_user_not_org_member(self, mock_apply_async, mock_record):
126+
"""When the Slack user has a linked identity but is not a member of the
127+
org with Seer access, the task should not be dispatched."""
128+
other_user = self.create_user()
129+
with assume_test_silo_mode(SiloMode.CONTROL):
130+
idp = self.create_identity_provider(type="slack", external_id="TXXXXXXX1")
131+
Identity.objects.create(
132+
external_id="U1234567890",
133+
idp=idp,
134+
user=other_user,
135+
status=IdentityStatus.VALID,
136+
scopes=[],
137+
)
138+
139+
with self.feature(SEER_EXPLORER_FEATURES):
140+
resp = self.post_webhook(event_data=APP_MENTION_EVENT)
141+
142+
assert resp.status_code == 200
143+
mock_apply_async.assert_not_called()
144+
assert_halt_metric(mock_record, SeerSlackHaltReason.NO_VALID_ORGANIZATION)
145+
146+
@patch("sentry.seer.entrypoints.slack.tasks.process_mention_for_slack.apply_async")
147+
def test_app_mention_linked_user_is_org_member(self, mock_apply_async):
148+
"""When the Slack user has a linked identity and IS a member of the
149+
org with Seer access, the task should be dispatched normally."""
150+
with assume_test_silo_mode(SiloMode.CONTROL):
151+
idp = self.create_identity_provider(type="slack", external_id="TXXXXXXX1")
152+
Identity.objects.create(
153+
external_id="U1234567890",
154+
idp=idp,
155+
user=self.user,
156+
status=IdentityStatus.VALID,
157+
scopes=[],
158+
)
159+
160+
with self.feature(SEER_EXPLORER_FEATURES):
161+
resp = self.post_webhook(
162+
event_data=THREADED_APP_MENTION_EVENT, data=AUTHORIZATIONS_DATA
163+
)
164+
165+
assert resp.status_code == 200
166+
mock_apply_async.assert_called_once()
167+
kwargs = mock_apply_async.call_args[1]["kwargs"]
168+
assert kwargs["organization_id"] == self.organization.id

0 commit comments

Comments
 (0)