diff --git a/src/firetower/incidents/tests/test_views.py b/src/firetower/incidents/tests/test_views.py index aac21450..1a70c3c2 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,216 @@ 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() + + 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: """Tests for service API endpoints (not UI)""" @@ -1722,7 +1933,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..555e5e8f 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,43 @@ 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: + 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" + ) + 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 +182,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 +358,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 @@ -332,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)}) @@ -374,8 +412,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 +440,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