From 759360c07e49532ec4054774a5bf8529133fbe00 Mon Sep 17 00:00:00 2001 From: Dae Hyeon Kim Date: Mon, 27 Jul 2026 15:26:14 +0900 Subject: [PATCH] =?UTF-8?q?fix(api):=20BUG-092=20=ED=9B=84=EC=86=8D=20?= =?UTF-8?q?=E2=80=94=20clinician=20=EC=9D=91=EB=8B=B5=20EmailStr=20?= =?UTF-8?q?=EC=A0=9C=EA=B1=B0=20+=20=EC=8B=9C=EB=93=9C=20org=20=EB=B0=B0?= =?UTF-8?q?=EC=84=A0=20+=20backfill=20=EC=8A=A4=ED=81=AC=EB=A6=BD=ED=8A=B8?= =?UTF-8?q?=20(ADR-049)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schemas/clinician.py: PatientListItem/PatientDetail email을 EmailStr->str. DB에 이미 저장된 email을 echo하는 응답측 재검증이 RFC 밖 주소(@demo, *.local, example.com 등 special-use 도메인)에서 목록/상세 전체 500을 유발 — DGX 라이브 발병 확인(2026-07-27). - scripts/seed_demo.py: 데모 환자를 clinician org로 배선. org-scope 필터 (ISS-022/PR #22) 하에서 NULL target_hospital_id 시드는 구조적 비가시. - scripts/backfill_bug092_target_hospital.py: 기존 NULL 행 backfill 도구 (idempotent, dry-run 지원). - services/clinician.py: 모듈 docstring을 org-scope 현행으로 정정(로직 무변경). - tests/repro/test_bug_092_org_scope_seed_mismatch.py: repro 3건 (로컬 PG 대상 3/3 passed). --- .../backfill_bug092_target_hospital.py | 80 ++++++++++ apps/api/scripts/seed_demo.py | 10 +- apps/api/src/schemas/clinician.py | 22 ++- apps/api/src/services/clinician.py | 9 +- .../test_bug_092_org_scope_seed_mismatch.py | 151 ++++++++++++++++++ 5 files changed, 265 insertions(+), 7 deletions(-) create mode 100644 apps/api/scripts/backfill_bug092_target_hospital.py create mode 100644 apps/api/tests/repro/test_bug_092_org_scope_seed_mismatch.py diff --git a/apps/api/scripts/backfill_bug092_target_hospital.py b/apps/api/scripts/backfill_bug092_target_hospital.py new file mode 100644 index 0000000..428ca44 --- /dev/null +++ b/apps/api/scripts/backfill_bug092_target_hospital.py @@ -0,0 +1,80 @@ +"""One-off backfill for BUG-092 (error.md). + +`scripts/seed_demo.py::_make_persona` never set `PatientProfile. +target_hospital_id` before this fix, so any DB already seeded from a +pre-fix run has every patient stuck at `target_hospital_id=NULL`, which +the org-scope filter (`src/services/clinician.py`, ISS-022/PR #22) treats +as invisible to every clinician. `_main` in `seed_demo.py` is idempotent by +early-exiting once `CLINICIAN_EMAIL` exists, so simply re-running the seed +does NOT repair already-seeded rows — this script does that repair +in-place instead. + +Scope: this is a local single-tenant demo/dev DB with exactly one +organization and one clinician account (per ADR-049 — org-scope policy +itself is unchanged). Every `patient_profiles` row with a NULL +`target_hospital_id` is assigned to that one clinician's `organization_id` +(there is no other org for them to plausibly belong to in this DB). This +script is not meant to run against a multi-tenant/production DB with more +than one organization — it refuses to guess in that case. + +Run from apps/api: + uv run python -m scripts.backfill_bug092_target_hospital +""" + +from __future__ import annotations + +import asyncio + +from sqlalchemy import select + +from src.db import SessionLocal +from src.models.patient_profile import PatientProfile +from src.models.user import Organization, User + + +async def _main() -> None: + async with SessionLocal() as db: + org_count_row = await db.execute(select(Organization.id)) + org_ids = [row[0] for row in org_count_row.all()] + if len(org_ids) != 1: + print( + f"refusing to backfill: expected exactly 1 organization in this " + f"DB, found {len(org_ids)} — this script only handles the " + f"single-tenant local demo DB shape" + ) + return + + clinician_row = await db.execute( + select(User).where(User.email == "clinician@neurosync.demo") + ) + clinician = clinician_row.scalar_one_or_none() + if clinician is None: + print("no demo clinician found — nothing to backfill") + return + if clinician.organization_id is None: + print("demo clinician has no organization_id — nothing to backfill") + return + org_id = clinician.organization_id + + rows = await db.execute( + select(PatientProfile, User) + .join(User, User.id == PatientProfile.user_id) + .where( + User.role == "patient", + PatientProfile.target_hospital_id.is_(None), + ) + ) + pairs = rows.all() + if not pairs: + print("no NULL target_hospital_id rows among patients — nothing to do") + return + + for profile, _user in pairs: + profile.target_hospital_id = org_id + + await db.commit() + print(f"backfilled {len(pairs)} patient_profiles.target_hospital_id -> {org_id}") + + +if __name__ == "__main__": + asyncio.run(_main()) diff --git a/apps/api/scripts/seed_demo.py b/apps/api/scripts/seed_demo.py index 2853c8a..d38ff33 100644 --- a/apps/api/scripts/seed_demo.py +++ b/apps/api/scripts/seed_demo.py @@ -277,7 +277,7 @@ async def _make_clinician(db, settings: Settings, org_id: uuid.UUID) -> None: ) -async def _make_persona(db, settings: Settings, p: dict) -> None: +async def _make_persona(db, settings: Settings, p: dict, org_id: uuid.UUID) -> None: user = User( email=p["email"], password_hash=hash_password(DEMO_PASSWORD, settings), @@ -304,6 +304,12 @@ async def _make_persona(db, settings: Settings, p: dict) -> None: aad=_profile_aad(user.id, "emergency_contact"), settings=settings, ), + # BUG-092 fix: org-scope filter (services/clinician.py, ISS-022/PR + # #22) requires target_hospital_id == actor.organization_id. This + # seed's demo patients belong to the demo clinician's org, so + # they must be assigned to it or the dashboard is structurally + # empty (NULL never matches a real UUID). + target_hospital_id=org_id, ) ) consent = ConsentSnapshot( @@ -398,7 +404,7 @@ async def _main() -> None: await _make_clinician(db, settings, org.id) for persona in PERSONAS: - await _make_persona(db, settings, persona) + await _make_persona(db, settings, persona, org.id) await db.commit() print( diff --git a/apps/api/src/schemas/clinician.py b/apps/api/src/schemas/clinician.py index 628a08e..ce17e00 100644 --- a/apps/api/src/schemas/clinician.py +++ b/apps/api/src/schemas/clinician.py @@ -11,7 +11,7 @@ from typing import Any from uuid import UUID -from pydantic import BaseModel, ConfigDict, EmailStr, Field +from pydantic import BaseModel, ConfigDict, Field class RiskBadge(BaseModel): @@ -24,7 +24,15 @@ class RiskBadge(BaseModel): class PatientListItem(BaseModel): user_id: UUID = Field(alias="userId") - email: EmailStr + # BUG-092 follow-on (discovered live, error.md): plain `str`, not + # `EmailStr` — these values echo an already-existing `User.email` row + # from the DB, not user input being validated. `EmailStr` rejected the + # demo dataset's `@demo` (no-TLD) addresses with a 500 the moment the + # org-scope filter (ISS-022) stopped masking it by returning empty + # first. Response-side re-validation of a value that already exists in + # the DB is not a security control here — it only breaks legitimately + # stored rows that don't fit RFC 5322 assumptions. + email: str name: str birth_year: int = Field(alias="birthYear") is_minor: bool = Field(alias="isMinor") @@ -60,7 +68,15 @@ class ConsentSnapshotOut(BaseModel): class PatientDetail(BaseModel): user_id: UUID = Field(alias="userId") - email: EmailStr + # BUG-092 follow-on (discovered live, error.md): plain `str`, not + # `EmailStr` — these values echo an already-existing `User.email` row + # from the DB, not user input being validated. `EmailStr` rejected the + # demo dataset's `@demo` (no-TLD) addresses with a 500 the moment the + # org-scope filter (ISS-022) stopped masking it by returning empty + # first. Response-side re-validation of a value that already exists in + # the DB is not a security control here — it only breaks legitimately + # stored rows that don't fit RFC 5322 assumptions. + email: str name: str birth_year: int = Field(alias="birthYear") is_minor: bool = Field(alias="isMinor") diff --git a/apps/api/src/services/clinician.py b/apps/api/src/services/clinician.py index a438ad8..1feeae3 100644 --- a/apps/api/src/services/clinician.py +++ b/apps/api/src/services/clinician.py @@ -1,7 +1,12 @@ """Clinician read service — decrypts PII and assembles dashboard payloads. -Phase 1a Demo simplification: every authenticated clinician sees every patient. -PRD §0.1 organization-scoped RLS lands in Phase 2. +Org-scoped per PRD §0.1 / ISS-022 (PR #22): a clinician / org_admin may read +only patients whose `PatientProfile.target_hospital_id` matches the actor's +`organization_id`; `super_admin` (service_role) sees all. See +`_can_access_patient` / `_patient_list_filter` for the exact rule. (BUG-092, +error.md: this docstring previously claimed "every authenticated clinician +sees every patient" — stale since PR #22 landed the org-scope filter; fixed +here to match actual behavior.) """ from __future__ import annotations diff --git a/apps/api/tests/repro/test_bug_092_org_scope_seed_mismatch.py b/apps/api/tests/repro/test_bug_092_org_scope_seed_mismatch.py new file mode 100644 index 0000000..b7af3cb --- /dev/null +++ b/apps/api/tests/repro/test_bug_092_org_scope_seed_mismatch.py @@ -0,0 +1,151 @@ +"""Regression test for BUG-092 (open) — live-discovered during +PLAN-2026-W31-WEBDASH's real-stack apps/web <-> apps/api <-> DB +verification. + +Live repro (2026-07-27, qa): logged in as the documented demo clinician +(`clinician@neurosync.demo`, `docs/PROGRESS.md:126-138`) through the real +`apps/web` HTTP path (login -> cookie -> server-component fetch -> +apps/api), and separately hit `GET /api/v1/clinician/patients` directly +against apps/api with the same bearer token. Both returned an EMPTY list +(`{"success":true,"data":[]}`) even though the demo DB had 5 real seeded +patients. `GET /api/v1/clinician/patients/{id}` and +`GET /api/v1/clinician/sessions/{id}` for real, existing IDs both 404'd. + +Root cause: `scripts/seed_demo.py::_make_persona` (PR #13, `d04c808`) never +sets `PatientProfile.target_hospital_id` — every seeded patient has it +`NULL`. `src/services/clinician.py`'s org-scope filter (PR #22, `c718809`, +ISS-022) requires `PatientProfile.target_hospital_id == actor. +organization_id`; the demo clinician DOES have a real `organization_id`. +`NULL == ` is never true in SQL, so every demo patient is +structurally excluded — contradicting the module's own docstring ("every +authenticated clinician sees every patient"). + +This test reproduces the live shape (clinician with a real +`organization_id`, patient profile whose `target_hospital_id` mirrors +whatever `seed_demo.py` currently sets it to) against a throwaway schema +(no live/demo DB touched) and asserts the CORRECT, expected behavior — +patients in the clinician's care ARE visible. It was RED (documented the +bug) against pre-fix `target_hospital_id=None`; ADR-049 chose backfill +(seed sets `target_hospital_id=`, org-scope +policy unchanged) over relaxing the "no-org == visible" policy, so the +fixture below now mirrors that post-fix seed shape and the test is GREEN. +""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +import pytest_asyncio +from sqlalchemy.ext.asyncio import AsyncSession + +from src.models.patient_profile import PatientProfile + +# conftest.py patches `PatientProfile.is_minor`'s column to +# `server_default=text("false")` (a TEST-ONLY convenience for callers that +# omit `is_minor`). SQLAlchemy's `Mapper._insert_cols_as_none` iterates +# every server_default and does a bare `not col.server_default` — a +# `TextClause` has no `__bool__`, so ANY insert of `PatientProfile` (this +# is the first test module in the suite to construct one via the ORM +# directly) raises `TypeError: Boolean value of this clause is not +# defined`. Not part of BUG-092 — a separate, pre-existing conftest gap +# with zero prior coverage — worked around locally here since we always +# pass `is_minor` explicitly and don't need the server_default at all. +from src.models.patient_profile import PatientProfile as _PatientProfile # noqa: E402 +from src.models.session import Session +from src.models.user import Organization, User +from src.services.clinician import get_patient_detail, get_session_detail, list_patients + +_PatientProfile.__table__.c.is_minor.server_default = None + + +@pytest_asyncio.fixture +async def clinician_and_patient(db_session: AsyncSession): + org = Organization(id=uuid.uuid4(), name="demo hospital", type="clinic") + db_session.add(org) + await db_session.flush() + + clinician = User( + email="clinician@neurosync.demo.test", + password_hash="x", + role="clinician", + organization_id=org.id, + ) + db_session.add(clinician) + await db_session.flush() + + patient = User(email="patient@demo.test", password_hash="x", role="patient") + db_session.add(patient) + await db_session.flush() + + profile = PatientProfile( + user_id=patient.id, + name_encrypted=b"not-really-encrypted-test-blob", + birth_year=1996, + is_minor=False, + gender="female", + # BUG-092 fix (ADR-049, option a): scripts/seed_demo.py now assigns + # the demo clinician's organization_id to every seeded patient's + # target_hospital_id, so this fixture mirrors seed_demo.py's + # post-fix, real, current behavior. Org-scope policy itself + # (services/clinician.py) is unchanged — this data now satisfies + # it instead of violating it. + target_hospital_id=org.id, + ) + db_session.add(profile) + await db_session.flush() + + session = Session( + patient_id=patient.id, + status="report_ready", + submitted_at=datetime.now(UTC) - timedelta(hours=1), + ) + db_session.add(session) + await db_session.flush() + + return clinician, patient, session + + +@pytest.mark.asyncio +async def test_bug092_demo_shaped_patient_is_visible_in_list( + db_session: AsyncSession, clinician_and_patient +): + clinician, patient, _session = clinician_and_patient + + items = await list_patients(db_session, actor=clinician, limit=50) + + assert any(i.user_id == patient.id for i in items), ( + "BUG-092: a patient seeded exactly like scripts/seed_demo.py " + "(target_hospital_id=None) must be visible to the demo clinician " + "(organization_id set) — got an empty/missing list because the " + "org-scope filter (PR #22) excludes NULL-hospital patients" + ) + + +@pytest.mark.asyncio +async def test_bug092_demo_shaped_patient_detail_is_reachable( + db_session: AsyncSession, clinician_and_patient +): + clinician, patient, _session = clinician_and_patient + + detail = await get_patient_detail(db_session, actor=clinician, patient_id=patient.id) + + assert detail is not None, ( + "BUG-092: get_patient_detail 404s (returns None) for a real, " + "existing patient seeded exactly like the live demo data" + ) + + +@pytest.mark.asyncio +async def test_bug092_demo_shaped_session_detail_is_reachable( + db_session: AsyncSession, clinician_and_patient +): + clinician, _patient, session = clinician_and_patient + + detail = await get_session_detail(db_session, actor=clinician, session_id=session.id) + + assert detail is not None, ( + "BUG-092: get_session_detail 404s (returns None) for a real, " + "existing session seeded exactly like the live demo data" + )