From 82a512460e67ef23c263e99ab5f20479517a4c6f Mon Sep 17 00:00:00 2001 From: Richard Gibert Date: Thu, 11 Jun 2026 14:56:03 -0400 Subject: [PATCH 1/2] fix(incidents): Sync Slack participants before visibility check for private incidents The UI views checked visibility before syncing participants from Slack, causing 404s for channel members who hadn't been synced to the DB yet. Meanwhile, Slack commands had no visibility check at all, so channel members could interact with private incidents via /inc but not view them in the Firetower UI. Add a _get_visible_incident helper that tries the normal visibility filter first, then falls back to a force-sync from Slack for private incidents before re-checking visibility. This ensures channel members are recognised on their first UI visit. Refs LINEAR-RELENG-832 Co-Authored-By: Claude Agent transcript: https://claudescope.sentry.dev/share/Z34J4fwlHa2-tleFcG8vgI7ko2aMzMxOpyE6yfvdj5Y --- src/firetower/incidents/tests/test_views.py | 185 +++++++++++++++++++- src/firetower/incidents/views.py | 59 +++++-- 2 files changed, 225 insertions(+), 19 deletions(-) diff --git a/src/firetower/incidents/tests/test_views.py b/src/firetower/incidents/tests/test_views.py index aac21450..62840231 100644 --- a/src/firetower/incidents/tests/test_views.py +++ b/src/firetower/incidents/tests/test_views.py @@ -344,10 +344,11 @@ def test_sync_participants_endpoint_respects_privacy(self): captain=other_user, ) - self.client.force_authenticate(user=self.user) - response = self.client.post( - f"/api/incidents/{incident.incident_number}/sync-participants/" - ) + with patch("firetower.incidents.views.sync_incident_participants_from_slack"): + self.client.force_authenticate(user=self.user) + response = self.client.post( + f"/api/incidents/{incident.incident_number}/sync-participants/" + ) assert response.status_code == 404 @@ -359,6 +360,174 @@ def test_sync_participants_endpoint_invalid_format(self): assert response.status_code == 400 +@pytest.mark.django_db +class TestPrivateIncidentVisibilitySync: + """Tests that Slack channel members can access private incidents in the UI + even before their participant record has been synced to the DB.""" + + def setup_method(self): + self.client = APIClient() + self.user = User.objects.create_user( + username="channel_member@example.com", + email="channel_member@example.com", + password="testpass123", + ) + self.captain = User.objects.create_user( + username="captain@example.com", + email="captain@example.com", + password="testpass123", + ) + + def _make_private_incident(self) -> Incident: + return Incident.objects.create( + title="Private Incident", + status=IncidentStatus.ACTIVE, + severity=IncidentSeverity.P1, + is_private=True, + captain=self.captain, + ) + + def test_detail_view_syncs_before_visibility_check(self): + """User in Slack channel but not yet synced gets access after force-sync.""" + incident = self._make_private_incident() + + def sync_adds_user(inc, force=False): + inc.participants.add(self.user) + return ParticipantsSyncStats(added=1) + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + side_effect=sync_adds_user, + ): + self.client.force_authenticate(user=self.user) + response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/") + + assert response.status_code == 200 + assert response.data["incident"]["id"] == incident.incident_number + + def test_detail_view_404_when_sync_does_not_add_user(self): + """User NOT in Slack channel still gets 404 after sync attempt.""" + incident = self._make_private_incident() + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + return_value=ParticipantsSyncStats(), + ): + self.client.force_authenticate(user=self.user) + response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/") + + assert response.status_code == 404 + + def test_detail_view_404_when_sync_raises(self): + """Sync failure on the fallback path results in 404, not 500.""" + incident = self._make_private_incident() + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + side_effect=Exception("Slack API error"), + ): + self.client.force_authenticate(user=self.user) + response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/") + + assert response.status_code == 404 + + def test_detail_view_fast_path_for_existing_participant(self): + """User already in participants skips the fallback sync entirely.""" + incident = self._make_private_incident() + incident.participants.add(self.user) + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack" + ) as mock_sync: + self.client.force_authenticate(user=self.user) + response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/") + + assert response.status_code == 200 + # Called once from the view's post-fetch sync, NOT from the helper. + mock_sync.assert_called_once_with(incident) + + def test_public_incident_does_not_trigger_fallback_sync(self): + """Public incidents never hit the fallback sync path.""" + incident = Incident.objects.create( + title="Public Incident", + status=IncidentStatus.ACTIVE, + severity=IncidentSeverity.P1, + is_private=False, + ) + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack" + ) as mock_sync: + self.client.force_authenticate(user=self.user) + response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/") + + assert response.status_code == 200 + # Only the view's normal post-fetch sync, not a force sync. + mock_sync.assert_called_once_with(incident) + + def test_action_items_view_syncs_before_visibility_check(self): + """Action items endpoint also syncs participants for private incidents.""" + incident = self._make_private_incident() + ActionItem.objects.create( + incident=incident, + linear_issue_id="id-1", + linear_identifier="ENG-1", + title="Fix the bug", + status=ActionItemStatus.TODO, + url="https://linear.app/team/issue/ENG-1", + ) + + def sync_adds_user(inc, force=False): + inc.participants.add(self.user) + return ParticipantsSyncStats(added=1) + + with ( + patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + side_effect=sync_adds_user, + ), + patch( + "firetower.incidents.views.sync_action_items_from_linear", + ), + ): + self.client.force_authenticate(user=self.user) + response = self.client.get( + f"/api/ui/incidents/{incident.incident_number}/action-items/" + ) + + assert response.status_code == 200 + assert len(response.data) == 1 + assert response.data[0]["title"] == "Fix the bug" + + def test_action_items_view_404_when_not_in_channel(self): + """Action items endpoint returns 404 when user is not a channel member.""" + incident = self._make_private_incident() + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + return_value=ParticipantsSyncStats(), + ): + self.client.force_authenticate(user=self.user) + response = self.client.get( + f"/api/ui/incidents/{incident.incident_number}/action-items/" + ) + + assert response.status_code == 404 + + def test_nonexistent_incident_returns_404(self): + """Fallback path does not reveal whether an incident exists.""" + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack" + ) as mock_sync: + self.client.force_authenticate(user=self.user) + response = self.client.get( + f"/api/ui/incidents/{settings.PROJECT_KEY}-99999/" + ) + + assert response.status_code == 404 + mock_sync.assert_not_called() + + @pytest.mark.django_db class TestIncidentAPIViews: """Tests for service API endpoints (not UI)""" @@ -1722,7 +1891,11 @@ def test_private_incident_not_visible_to_non_member(self): url="https://linear.app/team/issue/ENG-99", ) - self.client.force_authenticate(user=other_user) - response = self.client.get(self._url(private_incident.incident_number)) + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + return_value=ParticipantsSyncStats(), + ): + self.client.force_authenticate(user=other_user) + response = self.client.get(self._url(private_incident.incident_number)) assert response.status_code == 404 diff --git a/src/firetower/incidents/views.py b/src/firetower/incidents/views.py index 00af7196..c97175e2 100644 --- a/src/firetower/incidents/views.py +++ b/src/firetower/incidents/views.py @@ -4,7 +4,9 @@ from dataclasses import asdict from django.conf import settings +from django.contrib.auth.models import User from django.db.models import Case, Count, F, QuerySet, When +from django.http import Http404 from django.shortcuts import get_object_or_404 from django.utils import timezone from rest_framework import generics, serializers @@ -77,6 +79,42 @@ def parse_incident_id(incident_id: str) -> int: return int(match.group(1)) +def _get_visible_incident( + queryset: QuerySet[Incident], numeric_id: int, user: User +) -> Incident: + """Get an incident, syncing Slack participants first if needed for private incidents. + + Slack commands let channel members interact with private incidents, but + the UI requires DB participant records for visibility. When a channel + member hasn't been synced yet, the normal visibility filter returns 404. + This helper force-syncs participants on the fallback path so that channel + members are recognised on their first UI visit. + """ + visible_qs = filter_visible_to_user(queryset, user) + incident = visible_qs.filter(id=numeric_id).first() + if incident is not None: + return incident + + # Not visible yet -- if the incident exists and is private, the user + # might be a Slack channel member who hasn't been synced to the DB. + incident = queryset.filter(id=numeric_id).first() + if incident is None or not incident.is_private: + raise Http404 + + try: + sync_incident_participants_from_slack(incident, force=True) + except Exception: + logger.exception( + f"Failed to sync participants for incident {incident.id} during visibility check" + ) + raise Http404 + + if not incident.is_visible_to_user(user): + raise Http404 + + return incident + + class IncidentListUIView(generics.ListAPIView): """ List all incidents from database. @@ -143,12 +181,9 @@ def get_object(self) -> IncidentOrRedirect: """ incident_id = self.kwargs["incident_id"] numeric_id = parse_incident_id(incident_id) - - # Get incident by numeric ID - queryset = self.get_queryset() - queryset = filter_visible_to_user(queryset, self.request.user) - - incident = get_object_or_404(queryset, id=numeric_id) + incident = _get_visible_incident( + self.get_queryset(), numeric_id, self.request.user + ) try: sync_incident_participants_from_slack(incident) @@ -322,9 +357,7 @@ def get_object(self) -> Incident: """ incident_id = self.kwargs["incident_id"] numeric_id = parse_incident_id(incident_id) - queryset = filter_visible_to_user(self.get_queryset(), self.request.user) - - obj = get_object_or_404(queryset, id=numeric_id) + obj = _get_visible_incident(self.get_queryset(), numeric_id, self.request.user) self.check_object_permissions(self.request, obj) return obj @@ -374,8 +407,9 @@ def get_queryset(self) -> QuerySet[ActionItem]: def _get_incident(self) -> Incident: if not hasattr(self, "_incident"): numeric_id = parse_incident_id(self.kwargs["incident_id"]) - queryset = filter_visible_to_user(Incident.objects.all(), self.request.user) - self._incident = get_object_or_404(queryset, id=numeric_id) + self._incident = _get_visible_incident( + Incident.objects.all(), numeric_id, self.request.user + ) self.check_object_permissions(self.request, self._incident) return self._incident @@ -401,8 +435,7 @@ def get_queryset(self) -> QuerySet[Incident]: def get_object(self) -> Incident: numeric_id = parse_incident_id(self.kwargs["incident_id"]) - queryset = filter_visible_to_user(self.get_queryset(), self.request.user) - obj = get_object_or_404(queryset, id=numeric_id) + obj = _get_visible_incident(self.get_queryset(), numeric_id, self.request.user) self.check_object_permissions(self.request, obj) return obj From 65158a3e50b1d1b4a4cabb2ac09875b9877be579 Mon Sep 17 00:00:00 2001 From: Richard Gibert Date: Fri, 3 Jul 2026 11:51:56 -0400 Subject: [PATCH 2/2] fix: Eliminate double force-sync in SyncIncidentParticipantsView When a user goes through the visibility fallback path in _get_visible_incident, a force sync already fires to check channel membership. SyncIncidentParticipantsView.post() then called sync_incident_participants_from_slack(force=True) again, wasting a Slack API round-trip. Stash the sync stats on the incident object during the visibility check and reuse them in post() instead of syncing twice. Co-Authored-By: Claude Opus 4.6 Agent transcript: https://claudescope.sentry.dev/share/nq_KjfQLgQvXAVm98OBQSCW9RSeli5h0kIU-ZL0ldxk --- src/firetower/incidents/tests/test_views.py | 42 +++++++++++++++++++++ src/firetower/incidents/views.py | 7 +++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/firetower/incidents/tests/test_views.py b/src/firetower/incidents/tests/test_views.py index 62840231..1a70c3c2 100644 --- a/src/firetower/incidents/tests/test_views.py +++ b/src/firetower/incidents/tests/test_views.py @@ -527,6 +527,48 @@ def test_nonexistent_incident_returns_404(self): assert response.status_code == 404 mock_sync.assert_not_called() + def test_sync_participants_endpoint_no_double_sync(self): + """Sync-participants endpoint does not re-sync when the visibility + check already performed a force sync.""" + incident = self._make_private_incident() + + def sync_adds_user(inc, force=False): + inc.participants.add(self.user) + return ParticipantsSyncStats(added=1) + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + side_effect=sync_adds_user, + ) as mock_sync: + self.client.force_authenticate(user=self.user) + response = self.client.post( + f"/api/incidents/{incident.incident_number}/sync-participants/" + ) + + assert response.status_code == 200 + assert response.data["success"] is True + assert response.data["stats"]["added"] == 1 + mock_sync.assert_called_once_with(incident, force=True) + + def test_sync_participants_endpoint_syncs_when_already_visible(self): + """Sync-participants endpoint performs sync when user is already + visible (no fallback path triggered).""" + incident = self._make_private_incident() + incident.participants.add(self.user) + + with patch( + "firetower.incidents.views.sync_incident_participants_from_slack", + return_value=ParticipantsSyncStats(added=0, already_existed=5), + ) as mock_sync: + self.client.force_authenticate(user=self.user) + response = self.client.post( + f"/api/incidents/{incident.incident_number}/sync-participants/" + ) + + assert response.status_code == 200 + assert response.data["stats"]["already_existed"] == 5 + mock_sync.assert_called_once_with(incident, force=True) + @pytest.mark.django_db class TestIncidentAPIViews: diff --git a/src/firetower/incidents/views.py b/src/firetower/incidents/views.py index c97175e2..555e5e8f 100644 --- a/src/firetower/incidents/views.py +++ b/src/firetower/incidents/views.py @@ -102,7 +102,8 @@ def _get_visible_incident( raise Http404 try: - sync_incident_participants_from_slack(incident, force=True) + stats = sync_incident_participants_from_slack(incident, force=True) + incident._visibility_sync_stats = stats except Exception: logger.exception( f"Failed to sync participants for incident {incident.id} during visibility check" @@ -365,6 +366,10 @@ def get_object(self) -> Incident: def post(self, request: Request, incident_id: str) -> Response: incident = self.get_object() + visibility_stats = getattr(incident, "_visibility_sync_stats", None) + if visibility_stats is not None: + return Response({"success": True, "stats": asdict(visibility_stats)}) + try: stats = sync_incident_participants_from_slack(incident, force=True) return Response({"success": True, "stats": asdict(stats)})