Skip to content

Commit 5b77767

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 b0f7e2c commit 5b77767

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
@@ -342,6 +342,8 @@ def _resolve_seer_organization(
342342
Returns ``(organization_id, installation)`` or ``None`` when the
343343
event should be halted (the halt reason is already recorded).
344344
345+
We also check that the requesting user is a member of the organization that Seer is accessing.
346+
345347
Note: There is a limitation here of only grabbing the first organization with access to Seer.
346348
If a Slack installation corresponds to multiple organizations with Seer access, this will not work,
347349
and must be revisited.
@@ -355,6 +357,8 @@ def _resolve_seer_organization(
355357
lifecycle.record_halt(SeerSlackHaltReason.NO_VALID_INTEGRATION)
356358
return None
357359

360+
identity_user = slack_request.get_identity_user()
361+
358362
lifecycle.add_extra("organization_ids", [oi.organization_id for oi in ois])
359363
for oi in ois:
360364
organization_id = oi.organization_id
@@ -363,17 +367,22 @@ def _resolve_seer_organization(
363367
except Organization.DoesNotExist:
364368
continue
365369

366-
installation = slack_request.integration.get_installation(
367-
organization_id=organization_id
368-
)
369-
assert isinstance(installation, SlackIntegration)
370-
371370
if organization.status != OrganizationStatus.ACTIVE:
372371
continue
373372

374373
if not SlackExplorerEntrypoint.has_access(organization):
375374
continue
376375

376+
# When the user's identity is linked, verify they belong to this
377+
# org. If not linked the downstream task will prompt to link.
378+
if identity_user and not organization.has_access(identity_user):
379+
continue
380+
381+
installation = slack_request.integration.get_installation(
382+
organization_id=organization_id
383+
)
384+
assert isinstance(installation, SlackIntegration)
385+
377386
return organization_id, installation
378387
lifecycle.record_halt(SeerSlackHaltReason.NO_VALID_ORGANIZATION)
379388
return None
@@ -385,7 +394,7 @@ def _handle_seer_mention(
385394
) -> Response | None:
386395
"""Shared handler for app mentions and DMs that trigger the Seer workflow.
387396
388-
Returns ``None`` when the feature flag is off (DM messages only),
397+
Returns ``None`` when org resolution fails (DM messages only),
389398
allowing the caller to fall back to alternative handling.
390399
"""
391400
with MessagingInteractionEvent(
@@ -402,8 +411,8 @@ def _handle_seer_mention(
402411

403412
result = self._resolve_seer_organization(slack_request, lifecycle)
404413
if result is None:
405-
# For DMs, return None on feature-flag halt so caller can
406-
# fall back to the help message.
414+
# For DMs, return None on org resolution failure so caller
415+
# can fall back to the help message.
407416
if interaction_type == MessagingInteractionType.DM_MESSAGE:
408417
return None
409418
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)