From c084571bf00cd44e1466a40ccc7e97c78202a9f4 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Sun, 28 Jun 2026 18:52:23 +0330 Subject: [PATCH] fix(security): enforce doctor scope on POST my/appointment (H1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createAppointment only checked the caller held an allowed role, then booked onto whatever doctor_uuid the request named — a doctor could book onto any other doctor's calendar, a clinic onto doctors outside it, a secretary outside their scope. Add canBookForDoctor(): doctor→own only, clinic→member doctors, secretary→active scope + appointments.create permission, admin→any. Regression: tests/Appointment/BookingScopeTest. Co-Authored-By: Claude Opus 4.8 --- docs/api/appointment.md | 4 +- docs/audit-backlog.md | 2 +- .../Controller/MyAppointmentsController.php | 65 +++++++++++++++++++ tests/Appointment/BookingScopeTest.php | 56 ++++++++++++++++ 4 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 tests/Appointment/BookingScopeTest.php diff --git a/docs/api/appointment.md b/docs/api/appointment.md index 71b2a0af..d7e4bad5 100644 --- a/docs/api/appointment.md +++ b/docs/api/appointment.md @@ -385,6 +385,8 @@ Create a new appointment for a patient. Used by doctor/clinic/secretary to book **Auth:** `IS_AUTHENTICATED_FULLY` — Roles: `ROLE_DOCTOR`, `ROLE_CLINIC`, `ROLE_SECRETARY`, `ROLE_ADMIN` +> **Scope enforced:** the caller must be related to the target `doctor_uuid`, not merely hold an allowed role. A doctor may book only onto their own calendar; a clinic only onto doctors that belong to it; a secretary only within their active clinic/doctor scope **and** with the `appointments.create` permission; admin onto any. Otherwise `403 FORBIDDEN`. + ### Request Body ```json { @@ -415,7 +417,7 @@ Create a new appointment for a patient. Used by doctor/clinic/secretary to book ### Error Responses | Code | HTTP | Description | |------|------|-------------| -| `FORBIDDEN` | 403 | Role not allowed | +| `FORBIDDEN` | 403 | Role not allowed, or caller not scoped to this doctor | | `VALIDATION` | 422 | Missing required fields | | `DOCTOR_NOT_FOUND` | 404 | Doctor UUID not found | | `SLOT_TAKEN` | 409 | Slot already booked | diff --git a/docs/audit-backlog.md b/docs/audit-backlog.md index 8899e49c..22eae9b9 100644 --- a/docs/audit-backlog.md +++ b/docs/audit-backlog.md @@ -37,7 +37,7 @@ _None outstanding._ | # | Task | File:line | Cat | How to test | |---|------|-----------|-----|-------------| -| H1 | IDOR write: `createAppointment` trusts request `doctor_uuid`, no scope check — any staff books onto any doctor's calendar | src/Appointment/Controller/MyAppointmentsController.php:59 | security-idor | POST `/api/v1/my/appointment` w/ unrelated doctor_uuid → expect 403 | +| ✅H1 | IDOR write: `createAppointment` trusts request `doctor_uuid`, no scope check — any staff books onto any doctor's calendar | src/Appointment/Controller/MyAppointmentsController.php:59 | security-idor | **DONE** — `canBookForDoctor()` scope gate + `tests/Appointment/BookingScopeTest` | | H2 | No UNIQUE `(doctor_id, slot_start)` on Appointment → double-booking race (index is non-unique) | src/Appointment/Entity/Appointment.php:13 | db-unique | Concurrent POST same doctor+slot → only one persists. ⚠️ check existing dup data before adding constraint | | H3 | `Payment.referenceId` not unique → same gateway callback credited twice | src/Payment/Entity/Payment.php:61-62 | db-unique | Persist two Payments same reference_id → 2nd rejected. (pairs w/ C1) | | H4 | `FinancialBreakdown.payment` onDelete CASCADE on non-nullable FK → deleting a Payment destroys ledger rows; should be RESTRICT | src/Settlement/Entity/FinancialBreakdown.php:28-30 | db-ondelete | Delete a Payment w/ breakdown → expect FK restrict error, ledger preserved | diff --git a/src/Appointment/Controller/MyAppointmentsController.php b/src/Appointment/Controller/MyAppointmentsController.php index b81f426d..f5a928f9 100644 --- a/src/Appointment/Controller/MyAppointmentsController.php +++ b/src/Appointment/Controller/MyAppointmentsController.php @@ -7,6 +7,7 @@ use App\Appointment\Service\SlotCalculatorService; use App\Auth\Entity\User; use App\Auth\Repository\UserActiveContextRepository; use App\Clinic\Repository\ClinicRepository; +use App\Doctor\Entity\Doctor; use App\Doctor\Repository\DoctorRepository; use App\Secretary\Entity\DoctorSecretary; use App\Secretary\Repository\DoctorSecretaryRepository; @@ -59,6 +60,10 @@ class MyAppointmentsController extends BaseController $doctor = $this->doctorRepo->findByUuid($doctorUuid); if (!$doctor) return $this->error('DOCTOR_NOT_FOUND', 'پزشک یافت نشد', 404); + if (!$this->canBookForDoctor($user, $doctor)) { + return $this->error('FORBIDDEN', 'برای این پزشک مجاز به ثبت نوبت نیستید', 403); + } + $patient = $this->em->getRepository(User::class)->findOneBy(['mobileNumber' => $mobile]); if (!$patient) { $patient = new User($mobile); @@ -269,6 +274,66 @@ class MyAppointmentsController extends BaseController ]); } + /** + * Whether the acting user is allowed to book onto this doctor's calendar. + * The role gate alone is not enough: a doctor/clinic/secretary must be + * scoped to the target doctor, otherwise any staff user could book onto + * any doctor's calendar by passing an arbitrary doctor_uuid. + */ + private function canBookForDoctor(User $user, Doctor $doctor): bool + { + $roles = $user->getRoles(); + + if (in_array('ROLE_ADMIN', $roles, true)) { + return true; + } + + if (in_array('ROLE_CLINIC', $roles, true)) { + $clinic = $this->clinicRepo->findByUser($user); + if ($clinic !== null && $clinic->getDoctors()->contains($doctor)) { + return true; + } + } + + if (in_array('ROLE_DOCTOR', $roles, true)) { + $own = $this->doctorRepo->findByUser($user); + if ($own !== null && $own->getId() === $doctor->getId()) { + return true; + } + } + + if (in_array('ROLE_SECRETARY', $roles, true) && $this->secretaryCanBookForDoctor($user, $doctor)) { + return true; + } + + return false; + } + + private function secretaryCanBookForDoctor(User $user, Doctor $doctor): bool + { + $dbUuid = $this->contextRepo->findByUser($user)?->getDbUuid(); + if ($dbUuid === null) { + return false; + } + + $clinic = $this->clinicRepo->findByUuid($dbUuid); + if ($clinic !== null) { + if (!$clinic->getDoctors()->contains($doctor)) { + return false; + } + $rel = $this->secretaryRepo->findActiveBySecretaryForClinic($user, $clinic); + return $rel !== null && (bool) ($rel->getPermissions()['resources']['appointments']['create'] ?? false); + } + + $scopeDoctor = $this->doctorRepo->findByUuid($dbUuid); + if ($scopeDoctor !== null && $scopeDoctor->getId() === $doctor->getId()) { + $rel = $this->secretaryRepo->findActiveBySecretaryForDoctor($user, $scopeDoctor); + return $rel !== null && (bool) ($rel->getPermissions()['resources']['appointments']['create'] ?? false); + } + + return false; + } + /** * تعیین فیلتر نوبت‌ها برای منشی بر اساس scope فعال: * Returns [type, entity, canView] یا null اگر رابطه‌ای پیدا نشد. diff --git a/tests/Appointment/BookingScopeTest.php b/tests/Appointment/BookingScopeTest.php new file mode 100644 index 00000000..361bcd7a --- /dev/null +++ b/tests/Appointment/BookingScopeTest.php @@ -0,0 +1,56 @@ +createUser(['ROLE_USER', 'ROLE_DOCTOR']); + $doctor = new Doctor($user, 'دکتر تست'); + $this->em->persist($doctor); + $this->em->flush(); + + return $doctor; + } + + private function bookingBody(string $doctorUuid): array + { + $start = time() + 86_400; + + return [ + 'doctor_uuid' => $doctorUuid, + 'slot_start' => $start, + 'slot_end' => $start + 1_800, + 'patient_mobile' => '09120000000', + 'patient_name' => 'بیمار تست', + ]; + } + + public function testDoctorCannotBookOntoAnotherDoctor(): void + { + $doctorA = $this->makeDoctor(); + $doctorB = $this->makeDoctor(); + + $this->authJson('POST', '/api/v1/my/appointment', $doctorA->getUser(), $this->bookingBody($doctorB->getUuid())); + + $this->assertSame(403, $this->responseCode()); + } + + public function testDoctorCanBookOntoOwnCalendar(): void + { + $doctorA = $this->makeDoctor(); + + $this->authJson('POST', '/api/v1/my/appointment', $doctorA->getUser(), $this->bookingBody($doctorA->getUuid())); + + $this->assertSame(201, $this->responseCode()); + } +}