-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add incident status endpoint with privileged read access #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # Generated by Django 5.2.14 on 2026-05-15 19:22 | ||
|
|
||
| from django.db import migrations | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("incidents", "0016_schedule_demo"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterModelOptions( | ||
| name="incident", | ||
| options={ | ||
| "ordering": ["-created_at"], | ||
| "permissions": [ | ||
| ( | ||
| "view_all_incident_statuses", | ||
| "Can view status of all incidents, including private", | ||
| ) | ||
| ], | ||
| }, | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,19 +29,20 @@ | |
| Tag, | ||
| TagType, | ||
| filter_visible_to_user, | ||
| ) | ||
| from .permissions import IncidentPermission | ||
| from .permissions import IncidentPermission, IncidentStatusPermission | ||
| from .reporting_utils import ( | ||
| build_incidents_by_tag, | ||
| compute_regions, | ||
| get_month_periods, | ||
| get_quarter_periods, | ||
|
Check failure on line 38 in src/firetower/incidents/views.py
|
||
| get_year_periods, | ||
| ) | ||
| from .serializers import ( | ||
| IncidentListUISerializer, | ||
| IncidentOrRedirectReadSerializer, | ||
| IncidentReadSerializer, | ||
| IncidentStatusSerializer, | ||
| IncidentWriteSerializer, | ||
| TagCreateSerializer, | ||
| TagSerializer, | ||
|
|
@@ -265,6 +266,46 @@ | |
| return obj | ||
|
|
||
|
|
||
| class IncidentStatusRetrieveAPIView(generics.RetrieveAPIView): | ||
| """ | ||
| Service API for retrieving an incident's status only. | ||
|
|
||
| GET: Get incident status | ||
|
|
||
| Accepts incident_id in format: INC-2000 | ||
|
|
||
| Access is granted if either: | ||
| - The user has normal read visibility to the incident (IncidentPermission), or | ||
| - The user has the `incidents.view_all_incident_statuses` permission | ||
| (IncidentStatusPermission), which grants status access to any incident | ||
| including private ones. | ||
| """ | ||
|
|
||
| permission_classes = [IncidentPermission | IncidentStatusPermission] | ||
| serializer_class = IncidentStatusSerializer | ||
| lookup_field = "id" | ||
|
|
||
| def get_queryset(self) -> QuerySet[Incident]: | ||
| return Incident.objects.all() | ||
|
|
||
| def get_object(self) -> Incident: | ||
| incident_id = self.kwargs["incident_id"] | ||
| project_key = settings.PROJECT_KEY | ||
|
|
||
| incident_pattern = rf"^{re.escape(project_key)}-(\d+)$" | ||
| match = re.match(incident_pattern, incident_id, re.IGNORECASE) | ||
|
|
||
| if not match: | ||
| raise ValidationError( | ||
| f"Invalid incident ID format. Expected format: {project_key}-<number> (e.g., {project_key}-123)" | ||
| ) | ||
|
|
||
| numeric_id = int(match.group(1)) | ||
|
Check warning on line 303 in src/firetower/incidents/views.py
|
||
|
Comment on lines
+301
to
+303
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 VerificationChecked Identified by Warden code-review · 44H-AK7 |
||
| obj = get_object_or_404(self.get_queryset(), id=numeric_id) | ||
| self.check_object_permissions(self.request, obj) | ||
| return obj | ||
|
|
||
|
|
||
| class SyncIncidentParticipantsView(generics.GenericAPIView): | ||
| """ | ||
| Force sync incident participants from Slack channel. | ||
|
|
||
There was a problem hiding this comment.
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
IncidentStatusPermissionnever overrideshas_object_permission, DRF'sBasePermissiondefault returnsTruefor it — soIncidentPermission | IncidentStatusPermissionalways resolveshas_object_permissiontoTruefor any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to addhas_object_permissiontoIncidentStatusPermission(inpermissions.py) that mirrors itshas_permissioncheck:return request.user.has_perm('incidents.view_all_incident_statuses').Verification
Traced DRF's OR operator:
OR.has_object_permissioncalls both operands'has_object_permissionindependently and returnsop1 or op2.IncidentStatusPermissiononly defineshas_permission;BasePermission.has_object_permissionreturnsTrueunconditionally. InIncidentStatusRetrieveAPIView.get_object,get_queryset()returnsIncident.objects.all()(unfiltered), and thencheck_object_permissionsis 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_deniedasserts 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:34src/firetower/incidents/views.py:32-38Identified by Warden code-review · WS3-MGR