diff --git a/api/routers/analytics.py b/api/routers/analytics.py index 3b8b33d7..42955417 100644 --- a/api/routers/analytics.py +++ b/api/routers/analytics.py @@ -7,6 +7,7 @@ from sqlalchemy.orm import Session from api.database import get_db +from api.dependencies import get_current_admin_user, verify_org_member from api.models import Assignment, Event, Person, Solution router = APIRouter(prefix="/analytics", tags=["analytics"]) @@ -15,10 +16,16 @@ @router.get("/{org_id}/volunteer-stats") def get_volunteer_stats( org_id: str, - db: Session = Depends(get_db), days: int = Query(30, description="Number of days to analyze"), + current_admin: Person = Depends(get_current_admin_user), + db: Session = Depends(get_db), ): - """Get volunteer participation statistics.""" + """Get volunteer participation statistics. + + Admin-only within `org_id`. The caller must be an authenticated admin + whose own org matches the requested one. + """ + verify_org_member(current_admin, org_id) since_date = datetime.now() - timedelta(days=days) @@ -70,9 +77,14 @@ def get_volunteer_stats( @router.get("/{org_id}/schedule-health") def get_schedule_health( org_id: str, + current_admin: Person = Depends(get_current_admin_user), db: Session = Depends(get_db), ): - """Get schedule health metrics.""" + """Get schedule health metrics. + + Admin-only within `org_id`. + """ + verify_org_member(current_admin, org_id) # Upcoming events upcoming_events = ( @@ -118,10 +130,16 @@ def get_schedule_health( @router.get("/{org_id}/burnout-risk") def get_burnout_risk( org_id: str, - db: Session = Depends(get_db), threshold: int = Query(4, description="Assignments per month threshold"), + current_admin: Person = Depends(get_current_admin_user), + db: Session = Depends(get_db), ): - """Identify volunteers at risk of burnout (serving too frequently).""" + """Identify volunteers at risk of burnout (serving too frequently). + + Admin-only within `org_id`. This endpoint returns other volunteers' + names and emails, so peer volunteers can never read it. + """ + verify_org_member(current_admin, org_id) one_month_ago = datetime.now() - timedelta(days=30) diff --git a/tests/api/test_analytics.py b/tests/api/test_analytics.py index 774005bc..498b0f26 100644 --- a/tests/api/test_analytics.py +++ b/tests/api/test_analytics.py @@ -1,8 +1,14 @@ -"""Tests for /api/v1/analytics — covers the previously zero-test analytics router.""" +"""Tests for /api/v1/analytics — covers the previously zero-test analytics router. + +Sprint 4 PR 4.5d gated all three endpoints behind +`Depends(get_current_admin_user)` + `verify_org_member`. Baseline +smoke tests now authenticate as an admin so they exercise the same +happy paths they always did. +""" import pytest -from tests.api.conftest import seed_org, seed_user +from tests.api.conftest import auth_headers, seed_org, seed_user @pytest.fixture @@ -13,29 +19,46 @@ def org_id(client): return org +@pytest.fixture +def admin_hdrs(client, org_id): + return auth_headers(client, email="admin@a.org", password="AdminPass1!") + + @pytest.mark.no_mock_auth class TestVolunteerStats: - def test_returns_baseline_for_empty_org(self, client, db, org_id): - resp = client.get(f"/api/v1/analytics/{org_id}/volunteer-stats") + def test_returns_baseline_for_empty_org(self, client, db, org_id, admin_hdrs): + resp = client.get( + f"/api/v1/analytics/{org_id}/volunteer-stats", + headers=admin_hdrs, + ) assert resp.status_code == 200 body = resp.json() # Whatever the schema, endpoint must succeed and return JSON assert isinstance(body, dict) - def test_404_or_empty_for_unknown_org(self, client): - # Unknown orgs should not crash; either 404 or empty stats are acceptable. - resp = client.get("/api/v1/analytics/no-such-org/volunteer-stats") - assert resp.status_code in (200, 404) + def test_403_for_unknown_org(self, client, db, org_id, admin_hdrs): + # Admin-of-org-A hitting unknown org must be rejected before any DB read. + resp = client.get( + "/api/v1/analytics/no-such-org/volunteer-stats", + headers=admin_hdrs, + ) + assert resp.status_code == 403 - def test_accepts_days_query_param(self, client, db, org_id): - resp = client.get(f"/api/v1/analytics/{org_id}/volunteer-stats?days=7") + def test_accepts_days_query_param(self, client, db, org_id, admin_hdrs): + resp = client.get( + f"/api/v1/analytics/{org_id}/volunteer-stats?days=7", + headers=admin_hdrs, + ) assert resp.status_code == 200 @pytest.mark.no_mock_auth class TestScheduleHealth: - def test_returns_for_org(self, client, db, org_id): - resp = client.get(f"/api/v1/analytics/{org_id}/schedule-health") + def test_returns_for_org(self, client, db, org_id, admin_hdrs): + resp = client.get( + f"/api/v1/analytics/{org_id}/schedule-health", + headers=admin_hdrs, + ) assert resp.status_code == 200 body = resp.json() assert isinstance(body, dict) @@ -43,15 +66,21 @@ def test_returns_for_org(self, client, db, org_id): @pytest.mark.no_mock_auth class TestBurnoutRisk: - def test_returns_for_org(self, client, db, org_id): - resp = client.get(f"/api/v1/analytics/{org_id}/burnout-risk") + def test_returns_for_org(self, client, db, org_id, admin_hdrs): + resp = client.get( + f"/api/v1/analytics/{org_id}/burnout-risk", + headers=admin_hdrs, + ) assert resp.status_code == 200 body = resp.json() assert isinstance(body, dict | list) - def test_no_assignments_means_no_at_risk(self, client, db, org_id): + def test_no_assignments_means_no_at_risk(self, client, db, org_id, admin_hdrs): """An org with zero events/assignments cannot have burnout risk by definition.""" - resp = client.get(f"/api/v1/analytics/{org_id}/burnout-risk") + resp = client.get( + f"/api/v1/analytics/{org_id}/burnout-risk", + headers=admin_hdrs, + ) assert resp.status_code == 200 body = resp.json() # Whether `at_risk_volunteers` is absent or `[]`, the value must be falsy. diff --git a/tests/api/test_analytics_auth.py b/tests/api/test_analytics_auth.py new file mode 100644 index 00000000..fe226263 --- /dev/null +++ b/tests/api/test_analytics_auth.py @@ -0,0 +1,173 @@ +"""Analytics auth-gap tests (Sprint 4 PR 4.5d). + +Previously `/analytics/{org_id}/volunteer-stats`, `/schedule-health`, and +`/burnout-risk` accepted `org_id` as a path parameter and returned +volunteer names, emails, and event counts with **no auth check at all**. +Anyone who could reach the API could enumerate any org's roster. + +After this PR each endpoint requires an authenticated admin whose own +`org_id` matches the requested one, matching the pattern already used by +`/calendar/org/export` (PR 4.5c). +""" + +import pytest + +from tests.api.conftest import auth_headers, seed_org, seed_user + + +def _admin_for(client, org_id: str, suffix: str): + seed_user( + client, + org_id, + email=f"admin-{suffix}@a.org", + name="Admin", + password="AdminPass1!", + ) + return auth_headers(client, email=f"admin-{suffix}@a.org", password="AdminPass1!") + + +def _volunteer_for(client, org_id: str, suffix: str): + seed_user( + client, + org_id, + email=f"vol-{suffix}@a.org", + name="Vol", + password="VolPass1!", + roles=["volunteer"], + ) + return auth_headers(client, email=f"vol-{suffix}@a.org", password="VolPass1!") + + +# --------------------------------------------------------------------------- +# volunteer-stats +# --------------------------------------------------------------------------- + + +@pytest.mark.no_mock_auth +class TestVolunteerStatsAuth: + def test_no_auth_rejected(self, client, db): + org_id = "an-vs-noauth" + seed_org(client, org_id) + + resp = client.get(f"/api/v1/analytics/{org_id}/volunteer-stats") + assert resp.status_code in (401, 403) + + def test_volunteer_in_same_org_rejected(self, client, db): + org_id = "an-vs-vol" + seed_org(client, org_id) + _admin_for(client, org_id, "vs-vol-a") + vol_hdrs = _volunteer_for(client, org_id, "vs-vol") + + resp = client.get(f"/api/v1/analytics/{org_id}/volunteer-stats", headers=vol_hdrs) + assert resp.status_code == 403 + + def test_admin_cross_org_rejected(self, client, db): + seed_org(client, "an-vs-cr-a") + seed_org(client, "an-vs-cr-b") + a_hdrs = _admin_for(client, "an-vs-cr-a", "vs-cr-a") + _admin_for(client, "an-vs-cr-b", "vs-cr-b") + + # Admin of org A asks for org B's volunteer stats. + resp = client.get( + "/api/v1/analytics/an-vs-cr-b/volunteer-stats", + headers=a_hdrs, + ) + assert resp.status_code == 403 + + def test_admin_same_org_ok(self, client, db): + org_id = "an-vs-ok" + seed_org(client, org_id) + a_hdrs = _admin_for(client, org_id, "vs-ok") + + resp = client.get(f"/api/v1/analytics/{org_id}/volunteer-stats", headers=a_hdrs) + assert resp.status_code == 200 + body = resp.json() + assert body["org_id"] == org_id + + +# --------------------------------------------------------------------------- +# schedule-health +# --------------------------------------------------------------------------- + + +@pytest.mark.no_mock_auth +class TestScheduleHealthAuth: + def test_no_auth_rejected(self, client, db): + org_id = "an-sh-noauth" + seed_org(client, org_id) + + resp = client.get(f"/api/v1/analytics/{org_id}/schedule-health") + assert resp.status_code in (401, 403) + + def test_volunteer_in_same_org_rejected(self, client, db): + org_id = "an-sh-vol" + seed_org(client, org_id) + _admin_for(client, org_id, "sh-vol-a") + vol_hdrs = _volunteer_for(client, org_id, "sh-vol") + + resp = client.get(f"/api/v1/analytics/{org_id}/schedule-health", headers=vol_hdrs) + assert resp.status_code == 403 + + def test_admin_cross_org_rejected(self, client, db): + seed_org(client, "an-sh-cr-a") + seed_org(client, "an-sh-cr-b") + a_hdrs = _admin_for(client, "an-sh-cr-a", "sh-cr-a") + _admin_for(client, "an-sh-cr-b", "sh-cr-b") + + resp = client.get("/api/v1/analytics/an-sh-cr-b/schedule-health", headers=a_hdrs) + assert resp.status_code == 403 + + def test_admin_same_org_ok(self, client, db): + org_id = "an-sh-ok" + seed_org(client, org_id) + a_hdrs = _admin_for(client, org_id, "sh-ok") + + resp = client.get(f"/api/v1/analytics/{org_id}/schedule-health", headers=a_hdrs) + assert resp.status_code == 200 + body = resp.json() + assert body["org_id"] == org_id + + +# --------------------------------------------------------------------------- +# burnout-risk +# --------------------------------------------------------------------------- + + +@pytest.mark.no_mock_auth +class TestBurnoutRiskAuth: + def test_no_auth_rejected(self, client, db): + org_id = "an-br-noauth" + seed_org(client, org_id) + + resp = client.get(f"/api/v1/analytics/{org_id}/burnout-risk") + assert resp.status_code in (401, 403) + + def test_volunteer_in_same_org_rejected(self, client, db): + """Burnout-risk exposes other volunteers' names + emails — never + readable by a peer volunteer, even one in the same org.""" + org_id = "an-br-vol" + seed_org(client, org_id) + _admin_for(client, org_id, "br-vol-a") + vol_hdrs = _volunteer_for(client, org_id, "br-vol") + + resp = client.get(f"/api/v1/analytics/{org_id}/burnout-risk", headers=vol_hdrs) + assert resp.status_code == 403 + + def test_admin_cross_org_rejected(self, client, db): + seed_org(client, "an-br-cr-a") + seed_org(client, "an-br-cr-b") + a_hdrs = _admin_for(client, "an-br-cr-a", "br-cr-a") + _admin_for(client, "an-br-cr-b", "br-cr-b") + + resp = client.get("/api/v1/analytics/an-br-cr-b/burnout-risk", headers=a_hdrs) + assert resp.status_code == 403 + + def test_admin_same_org_ok(self, client, db): + org_id = "an-br-ok" + seed_org(client, org_id) + a_hdrs = _admin_for(client, org_id, "br-ok") + + resp = client.get(f"/api/v1/analytics/{org_id}/burnout-risk", headers=a_hdrs) + assert resp.status_code == 200 + body = resp.json() + assert body["org_id"] == org_id diff --git a/tests/contract/openapi.snapshot.json b/tests/contract/openapi.snapshot.json index a56df1c7..93517c3b 100644 --- a/tests/contract/openapi.snapshot.json +++ b/tests/contract/openapi.snapshot.json @@ -5513,7 +5513,7 @@ }, "/api/v1/analytics/{org_id}/burnout-risk": { "get": { - "description": "Identify volunteers at risk of burnout (serving too frequently).", + "description": "Identify volunteers at risk of burnout (serving too frequently).\n\nAdmin-only within `org_id`. This endpoint returns other volunteers'\nnames and emails, so peer volunteers can never read it.", "operationId": "getBurnoutRisk", "parameters": [ { @@ -5558,6 +5558,11 @@ "description": "Validation Error" } }, + "security": [ + { + "HTTPBearer": [] + } + ], "summary": "Get Burnout Risk", "tags": [ "analytics" @@ -5566,7 +5571,7 @@ }, "/api/v1/analytics/{org_id}/schedule-health": { "get": { - "description": "Get schedule health metrics.", + "description": "Get schedule health metrics.\n\nAdmin-only within `org_id`.", "operationId": "getScheduleHealth", "parameters": [ { @@ -5599,6 +5604,11 @@ "description": "Validation Error" } }, + "security": [ + { + "HTTPBearer": [] + } + ], "summary": "Get Schedule Health", "tags": [ "analytics" @@ -5607,7 +5617,7 @@ }, "/api/v1/analytics/{org_id}/volunteer-stats": { "get": { - "description": "Get volunteer participation statistics.", + "description": "Get volunteer participation statistics.\n\nAdmin-only within `org_id`. The caller must be an authenticated admin\nwhose own org matches the requested one.", "operationId": "getVolunteerStats", "parameters": [ { @@ -5652,6 +5662,11 @@ "description": "Validation Error" } }, + "security": [ + { + "HTTPBearer": [] + } + ], "summary": "Get Volunteer Stats", "tags": [ "analytics" diff --git a/web/routers/pages.py b/web/routers/pages.py index fa8ee347..e4e1102f 100644 --- a/web/routers/pages.py +++ b/web/routers/pages.py @@ -172,9 +172,7 @@ def _open_shifts(db: Session, person: Person) -> list[dict]: { "event_id": e.id, "event_type": e.type, - "when": e.start_time.strftime("%a %d %b %Y · %H:%M") - if e.start_time - else "", + "when": e.start_time.strftime("%a %d %b %Y · %H:%M") if e.start_time else "", "roles": roles, } ) @@ -463,20 +461,26 @@ def volunteer_profile( ) -def _dashboard_kpis(db: Session, org_id: str) -> dict: +def _dashboard_kpis(db: Session, person: Person) -> dict: """Roll the three analytics endpoints into the dashboard tiles. - All three return graceful zeros for an empty org.""" + All three return graceful zeros for an empty org. + + Direct in-process call: the API endpoints require an admin Person + (Sprint 4 PR 4.5d). Pass the caller's session admin so + `verify_org_member` still runs. + """ from api.routers.analytics import ( get_burnout_risk, get_schedule_health, get_volunteer_stats, ) + org_id = person.org_id # Pass the Query-defaulted args explicitly: a direct (non-FastAPI) # call doesn't resolve Query(...) sentinels to their defaults. - vs = get_volunteer_stats(org_id, db, days=30) - sh = get_schedule_health(org_id, db) - br = get_burnout_risk(org_id, db, threshold=4) + vs = get_volunteer_stats(org_id, days=30, current_admin=person, db=db) + sh = get_schedule_health(org_id, current_admin=person, db=db) + br = get_burnout_risk(org_id, threshold=4, current_admin=person, db=db) latest = sh.get("latest_solution") return { "active_volunteers": vs["active_volunteers"], @@ -504,7 +508,7 @@ def admin_dashboard( { "person": person, "active_tab": "dashboard", - "kpis": _dashboard_kpis(db, person.org_id), + "kpis": _dashboard_kpis(db, person), "ob": _onboarding_state(db, person), }, ) @@ -823,18 +827,25 @@ def admin_teams( ) -def _analytics(db: Session, org_id: str, days: int, threshold: int) -> dict: +def _analytics(db: Session, person: Person, days: int, threshold: int) -> dict: """Deep analytics — the three API endpoints with explicit args - (their Query() defaults don't resolve on a direct call).""" + (their Query() defaults don't resolve on a direct call). + + Direct in-process call: the API endpoints require an admin Person + (Sprint 4 PR 4.5d), and the caller here (`admin_analytics`) already + holds one via `get_session_admin`. Pass it as `current_admin` so the + endpoints' `verify_org_member` still runs. + """ from api.routers.analytics import ( get_burnout_risk, get_schedule_health, get_volunteer_stats, ) - vs = get_volunteer_stats(org_id, db, days=days) - sh = get_schedule_health(org_id, db) - br = get_burnout_risk(org_id, db, threshold=threshold) + org_id = person.org_id + vs = get_volunteer_stats(org_id, days=days, current_admin=person, db=db) + sh = get_schedule_health(org_id, current_admin=person, db=db) + br = get_burnout_risk(org_id, threshold=threshold, current_admin=person, db=db) return {"days": days, "threshold": threshold, "participation": vs, "health": sh, "burnout": br} @@ -856,7 +867,7 @@ def admin_analytics( { "person": person, "active_tab": "dashboard", - "a": _analytics(db, person.org_id, days, threshold), + "a": _analytics(db, person, days, threshold), }, )