From 82a512460e67ef23c263e99ab5f20479517a4c6f Mon Sep 17 00:00:00 2001 From: Richard Gibert Date: Thu, 11 Jun 2026 14:56:03 -0400 Subject: [PATCH] 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