Logan CusanoandClaude Opus 5
8b6c170265
POST /incidents/{id}/summarize was gated by require_service_or_firebase_token,
which accepts any authenticated Firebase user including role "viewer". That
route spends OpenAI credits via the background summarizer. The call-side
equivalent was already moved to require_admin_token; this brings the incident
side in line with it.
The frontend's two "summarize now" buttons on the incident detail page are
already gated behind isAdmin, so this backend change matches existing UI
behavior exactly and does not break any viewer/operator surface — it only
closes direct-API access for non-admins.
Swept every other route in incidents.py: list/get are reads with no spend and
correctly stay open to any signed-in user; create/update/delete/link/unlink
were already require_admin_token. No other sibling route needed changing.
Adds test_incident_summarize_auth.py pinning the dependency wiring directly
(the convention used in test_admin_feature_flags.py), so a future revert back
to the weak dependency fails a test immediately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
53 lines
2.3 KiB
Python
53 lines
2.3 KiB
Python
"""
|
|
server-26#81 — any signed-in viewer could trigger OpenAI summary spend.
|
|
|
|
``POST /incidents/{incident_id}/summarize`` was gated by
|
|
``require_service_or_firebase_token``, which accepts ANY authenticated
|
|
Firebase user (including role "viewer"), not just admins. Hitting the route
|
|
spends OpenAI credits via the background summarizer task. The call-side
|
|
equivalent (``PATCH /calls/{id}/transcript``) was already moved to
|
|
``require_admin_token``; the incident side was not moved with it.
|
|
|
|
Following the wiring-test convention in test_admin_feature_flags.py
|
|
(``test_features_routes_use_the_agent_dependency_and_others_do_not``): assert
|
|
against the route's actual dependant.dependencies rather than round-tripping
|
|
through TestClient, so this pins the credential wiring itself and would fail
|
|
immediately if someone reverts the dependency back to the weak one.
|
|
"""
|
|
from app.internal import auth
|
|
from app.routers import incidents
|
|
|
|
|
|
def _deps(path: str, method: str) -> set:
|
|
for r in incidents.router.routes:
|
|
if r.path == path and method in r.methods:
|
|
return {d.call for d in r.dependant.dependencies}
|
|
raise AssertionError(f"no route {method} {path}")
|
|
|
|
|
|
def test_summarize_incident_requires_admin_not_any_firebase_user():
|
|
deps = _deps("/incidents/{incident_id}/summarize", "POST")
|
|
assert auth.require_admin_token in deps
|
|
assert auth.require_service_or_firebase_token not in deps
|
|
|
|
|
|
def test_read_only_incident_routes_still_accept_any_signed_in_user():
|
|
"""Guards against an overcorrection: reads are not spend, they stay open
|
|
to any authenticated viewer."""
|
|
assert auth.require_service_or_firebase_token in _deps("/incidents", "GET")
|
|
assert auth.require_service_or_firebase_token in _deps("/incidents/{incident_id}", "GET")
|
|
|
|
|
|
def test_other_mutating_incident_routes_are_still_admin_only():
|
|
"""Unchanged by this fix, but pinned so a future edit can't quietly
|
|
loosen them while touching this file."""
|
|
for path, method in [
|
|
("/incidents/summarize", "POST"),
|
|
("/incidents", "POST"),
|
|
("/incidents/{incident_id}", "PUT"),
|
|
("/incidents/{incident_id}", "DELETE"),
|
|
("/incidents/{incident_id}/calls/{call_id}", "POST"),
|
|
("/incidents/{incident_id}/calls/{call_id}", "DELETE"),
|
|
]:
|
|
assert auth.require_admin_token in _deps(path, method), f"{method} {path}"
|