Skip to content

feat: Add incident status endpoint with privileged read access#201

Draft
taylor-osler-sentry wants to merge 1 commit into
mainfrom
feat/sudo-private
Draft

feat: Add incident status endpoint with privileged read access#201
taylor-osler-sentry wants to merge 1 commit into
mainfrom
feat/sudo-private

Conversation

@taylor-osler-sentry
Copy link
Copy Markdown
Contributor

Summary

  • Adds GET /api/incidents/{id}/status/ returning only id and status — no title or other fields — using the new IncidentStatusSerializer.
  • Access is granted to anyone with normal read visibility for the incident; users holding the new incidents.view_all_incident_statuses Django permission can read the status of any incident (including private ones) as a fallback when visibility alone would deny them. Composed in the view via IncidentPermission | IncidentStatusPermission.
  • Adds matching FiretowerClient.get_incident_status(incident_id) SDK method.
  • Adds TestIncidentStatusRetrieveAPIView covering: response shape (only id + status), normal-visibility access (public + captain of private), denial without visibility/perm, fallback access via the new perm, unauthenticated, superuser, invalid ID format, and 404.

Test plan

  • pytest src/firetower/incidents/tests/test_views.py::TestIncidentStatusRetrieveAPIView (needs a reachable Postgres — I couldn't run this locally from my environment)
  • Manual: hit /api/incidents/{id}/status/ as a user without visibility but with the new perm, confirm 200 + status only
  • Manual: same endpoint as a user without visibility and without the perm, confirm 403
  • Confirm migration 0017_alter_incident_options applies cleanly

🤖 Generated with Claude Code

Adds GET /api/incidents/{id}/status/ which returns only the incident's id
and status. Access is granted by normal read visibility, or as a fallback
via the new `incidents.view_all_incident_statuses` Django permission so
privileged users can check status of any incident (including private)
without seeing title or other details. Also exposes a matching
`get_incident_status` SDK method.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
import pytest
from django.conf import settings
from django.contrib.auth.models import User
from django.contrib.auth.models import Permission, User
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IncidentStatusPermission missing has_object_permission causes OR composition to bypass visibility checks

Because IncidentStatusPermission never overrides has_object_permission, DRF's BasePermission default returns True for it — so IncidentPermission | IncidentStatusPermission always resolves has_object_permission to True for any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to add has_object_permission to IncidentStatusPermission (in permissions.py) that mirrors its has_permission check: return request.user.has_perm('incidents.view_all_incident_statuses').

Verification

Traced DRF's OR operator: OR.has_object_permission calls both operands' has_object_permission independently and returns op1 or op2. IncidentStatusPermission only defines has_permission; BasePermission.has_object_permission returns True unconditionally. In IncidentStatusRetrieveAPIView.get_object, get_queryset() returns Incident.objects.all() (unfiltered), and then check_object_permissions is called. For a regular user targeting a private incident they don't own: IncidentPermission.has_object_permission → False (visibility denied); IncidentStatusPermission.has_object_permission → True (default); combined → True → 200. test_user_without_visibility_or_perm_is_denied asserts 403 but would actually receive 200, confirming the test would fail. Files checked: src/firetower/incidents/permissions.py (both classes), src/firetower/incidents/views.py (IncidentStatusRetrieveAPIView).

Also found at 2 additional locations
  • src/firetower/incidents/urls.py:34
  • src/firetower/incidents/views.py:32-38

Identified by Warden code-review · WS3-MGR

Comment on lines +301 to +303
)

numeric_id = int(match.group(1))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private incident existence disclosed via 403 vs 404 on status endpoint

Unlike every other get_object() in this file, this implementation fetches the incident first (returns 404 only if the row is missing) and then checks permissions (returning 403 if denied). A user without visibility and without the special permission receives 403, confirming the incident exists — whereas all other endpoints apply filter_visible_to_user before the lookup so non-visible incidents return 404 and reveal nothing. Consider applying filter_visible_to_user to the queryset for users who do not hold incidents.view_all_incident_statuses before calling get_object_or_404.

Verification

Checked IncidentPermission.has_object_permission() (permissions.py:54-58): for SAFE_METHODS it returns obj.is_visible_to_user(user). For a non-captain, non-admin user on a private incident this returns False. IncidentStatusPermission.has_permission() (permissions.py:36-38) also returns False for users without the Django perm. The OR composition therefore denies at the object-permission stage — after the incident has already been fetched — and DRF raises PermissionDenied (HTTP 403). Every other retrieval view (lines 78, 140, 181, 250, 347) applies filter_visible_to_user before get_object_or_404, causing invisible incidents to return 404. The test test_user_without_visibility_or_perm_is_denied (test_views.py ~1567) explicitly asserts 403, confirming the disclosed behaviour is present.

Identified by Warden code-review · 44H-AK7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant