fix(security): enforce doctor scope on POST my/appointment (H1)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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 |
|
||||
|
||||
@@ -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 اگر رابطهای پیدا نشد.
|
||||
|
||||
@@ -0,0 +1,56 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Appointment;
|
||||
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* POST /api/v1/my/appointment must enforce that the acting user is scoped to
|
||||
* the target doctor. A doctor having the ROLE_DOCTOR role is not enough — they
|
||||
* must not be able to book onto another doctor's calendar via doctor_uuid.
|
||||
*/
|
||||
class BookingScopeTest extends ApiTestCase
|
||||
{
|
||||
private function makeDoctor(): Doctor
|
||||
{
|
||||
$user = $this->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());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user