fix(security): enforce ownership on 4 IDOR read/bind endpoints (M2-M5)
- M2 GET /insurance/{id}: was unguarded; now owner-or-admin (403 otherwise) —
stops reading another doctor's negotiated price by id enumeration.
- M3 GET /clinic-pro/doctor-address/{id}: add the same owner/admin check the
sibling PATCH/DELETE already had.
- M4 POST/PATCH /service-item: staff_uuid must belong to the caller's tenant
(entity_type/entity_id) → 422; stops binding another tenant's staff.
- M5 appointment-settings list endpoints (date-override/holidays/
available-locations): add the per-doctor ownership check the sibling
single-record endpoints already enforce.
Regressions (6 negative cases fail without the fixes):
DoctorInsuranceOwnershipTest, DoctorAddressOwnershipTest,
ServiceItemStaffOwnershipTest, AppointmentSettingsListOwnershipTest.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -248,7 +248,7 @@ Override a specific date — mark it inactive (day off) or give it custom sessio
|
||||
|
||||
Get all date overrides for a doctor.
|
||||
|
||||
**Permission:** `AUTH` (class-level)
|
||||
**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise).
|
||||
|
||||
### Response `200`
|
||||
```json
|
||||
@@ -425,7 +425,7 @@ Mark a date range as holiday — all slots blocked, no overrides apply.
|
||||
|
||||
Get all holidays for a doctor.
|
||||
|
||||
**Permission:** `AUTH` (class-level)
|
||||
**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise).
|
||||
|
||||
### Response `200`
|
||||
```json
|
||||
@@ -573,7 +573,7 @@ The `SlotCalculatorService` calculates available slots in this priority order:
|
||||
|
||||
### `GET /api/v1/appointment-settings/available-locations/{doctorUuid}`
|
||||
|
||||
**Permission:** Public
|
||||
**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (`403 ERR_AUTH_006` otherwise).
|
||||
|
||||
Returns all locations a doctor can assign as `location_id` in their schedule sessions. Includes both the doctor's personal addresses and the addresses of all clinics they belong to.
|
||||
|
||||
|
||||
@@ -147,6 +147,8 @@
|
||||
}
|
||||
```
|
||||
|
||||
> `staff_uuid` باید به پرسنل متعلق به همان tenant (`entity_type`/`entity_id` کاربر) اشاره کند؛ ربطدادن پرسنل tenant دیگر → `422 ERR_VALIDATION_001` (`field: staff_uuid`). همین قید روی `POST /service-item` نیز اعمال میشود.
|
||||
|
||||
---
|
||||
|
||||
## DELETE /api/v1/service-item/{uuid}
|
||||
|
||||
@@ -225,7 +225,7 @@ Add an insurance to a doctor's accepted list.
|
||||
|
||||
Get a doctor-insurance link.
|
||||
|
||||
**Permission:** `PUBLIC`
|
||||
**Permission:** `AUTH` — must be the owning doctor or `ROLE_ADMIN` (otherwise `403 ERR_AUTH_006`). Prevents reading another doctor's negotiated price by id enumeration.
|
||||
|
||||
### Response `200`
|
||||
DoctorInsurance object.
|
||||
|
||||
@@ -55,10 +55,10 @@ _None outstanding._
|
||||
| # | Task | File:line | Cat | How to test |
|
||||
|---|------|-----------|-----|-------------|
|
||||
| ✅M1 | Mass-assign: claim approve/pay lets owning tenant set arbitrary `approved_rials`/`paid_rials` ~~+ force status~~ | src/Billing/Controller/BillingController.php | security-massassign | **DONE** — status-jump part was already guarded by `canTransitionTo` (false alarm). Added bounds: approved ∈ [0, claimed], paid ∈ [0, approved] → 422. `tests/Billing/ClaimAmountBoundsTest`. |
|
||||
| M2 | IDOR read: `GET /insurance/{id}` (showDoctorInsurance) leaks any doctor's insurance incl. negotiated price by id enumeration | src/Insurance/Controller/InsuranceController.php:499 | security-idor | GET insurance/{id} as non-owner → 403 |
|
||||
| M3 | IDOR read: `showAddress` loads any DoctorAddress by id, no owner check | src/Doctor/Controller/DoctorController.php:559 | security-idor | GET doctor-address/{id} as non-owner → 403 |
|
||||
| M4 | IDOR write: ClinicService `createItem`/`updateItem` bind staff via global `staffRepo->findByUuid()`, no entity-scope check (cross-tenant staff binding) | src/ClinicService/Controller/ClinicServiceController.php:157,195 | security-idor | Bind other clinic's staff_uuid → 403/validation |
|
||||
| M5 | IDOR read: AppointmentSettings list endpoints leak any doctor's config — `listOverrides`, `listHolidays`, `availableLocations` (no ownership on {doctorUuid}) | src/Appointment/Controller/AppointmentSettingsController.php:150,256,349 | security-idor | GET each list for unowned doctor → 403 |
|
||||
| ✅M2 | IDOR read: `GET /insurance/{id}` (showDoctorInsurance) leaks any doctor's insurance incl. negotiated price by id enumeration | src/Insurance/Controller/InsuranceController.php:499 | security-idor | **DONE** — auth+ownership (owner/admin). `tests/Insurance/DoctorInsuranceOwnershipTest` |
|
||||
| ✅M3 | IDOR read: `showAddress` loads any DoctorAddress by id, no owner check | src/Doctor/Controller/DoctorController.php:559 | security-idor | **DONE** — ownership mirror of PATCH/DELETE. `tests/Doctor/DoctorAddressOwnershipTest` |
|
||||
| ✅M4 | IDOR write: ClinicService `createItem`/`updateItem` bind staff via global `staffRepo->findByUuid()`, no entity-scope check (cross-tenant staff binding) | src/ClinicService/Controller/ClinicServiceController.php:157,195 | security-idor | **DONE** — staff must match tenant (entity_type/id) → 422. `tests/ClinicService/ServiceItemStaffOwnershipTest` |
|
||||
| ✅M5 | IDOR read: AppointmentSettings list endpoints leak any doctor's config — `listOverrides`, `listHolidays`, `availableLocations` (no ownership on {doctorUuid}) | src/Appointment/Controller/AppointmentSettingsController.php:150,256,349 | security-idor | **DONE** — ownership added to listOverrides/listHolidays/availableLocations. `tests/Appointment/AppointmentSettingsListOwnershipTest` |
|
||||
| M6 | OTP send-code has no per-mobile/per-uuid cap, only per-IP (5/hr) → SMS flood from rotating IPs | src/Auth/Controller/AuthController.php:136-153 · OtpService.php:59-77 · rate_limiter.yaml:4-7 | security-ratelimit | Request many codes one mobile across IPs → per-number cap enforced |
|
||||
| M7 | Refresh token not rotated on use (same raw token 30d), never re-checks user status | src/Auth/Controller/AuthController.php:477-480 · TokenService.php:33-45 | security-auth | Call refresh twice → new token issued each time, suspended user rejected |
|
||||
| M8 | N+1: secretary list lazy-loads secretary/doctor ManyToOne per row | src/Secretary/Controller/SecretaryController.php:192,213 | perf-nplus1 | Profiler secretary list → ~2 queries/row |
|
||||
|
||||
@@ -148,13 +148,17 @@ class AppointmentSettingsController extends BaseController
|
||||
// ── Date Overrides ────────────────────────────────────────────────────────
|
||||
|
||||
#[Route('/api/v1/appointment-settings/date-override/list/{doctorUuid}', methods: ['GET'])]
|
||||
public function listOverrides(string $doctorUuid): JsonResponse
|
||||
public function listOverrides(string $doctorUuid, #[CurrentUser] User $user): JsonResponse
|
||||
{
|
||||
$doctor = $this->doctorRepo->findByUuid($doctorUuid);
|
||||
if ($doctor === null) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404);
|
||||
}
|
||||
|
||||
if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) {
|
||||
return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403);
|
||||
}
|
||||
|
||||
$overrides = array_map(
|
||||
fn(DateOverride $o) => $o->toArray(),
|
||||
$this->overrideRepo->findByDoctor($doctor)
|
||||
@@ -254,13 +258,17 @@ class AppointmentSettingsController extends BaseController
|
||||
// ── Holidays ──────────────────────────────────────────────────────────────
|
||||
|
||||
#[Route('/api/v1/appointment-settings/holidays/list/{doctorUuid}', methods: ['GET'])]
|
||||
public function listHolidays(string $doctorUuid): JsonResponse
|
||||
public function listHolidays(string $doctorUuid, #[CurrentUser] User $user): JsonResponse
|
||||
{
|
||||
$doctor = $this->doctorRepo->findByUuid($doctorUuid);
|
||||
if ($doctor === null) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404);
|
||||
}
|
||||
|
||||
if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) {
|
||||
return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403);
|
||||
}
|
||||
|
||||
$items = array_map(fn(Holiday $h) => $h->toArray(), $this->holidayRepo->findAllByDoctor($doctor));
|
||||
|
||||
return $this->success(['data' => $items]);
|
||||
@@ -347,13 +355,17 @@ class AppointmentSettingsController extends BaseController
|
||||
// ── Available Locations ───────────────────────────────────────────────────
|
||||
|
||||
#[Route('/api/v1/appointment-settings/available-locations/{doctorUuid}', methods: ['GET'])]
|
||||
public function availableLocations(string $doctorUuid): JsonResponse
|
||||
public function availableLocations(string $doctorUuid, #[CurrentUser] User $user): JsonResponse
|
||||
{
|
||||
$doctor = $this->doctorRepo->findByUuid($doctorUuid);
|
||||
if ($doctor === null) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_002, 'دکتر یافت نشد', 404);
|
||||
}
|
||||
|
||||
if ($doctor->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) {
|
||||
return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403);
|
||||
}
|
||||
|
||||
$clinics = $this->clinicRepo->findByDoctor($doctor);
|
||||
$clinicIds = array_map(fn(Clinic $c) => $c->getId(), $clinics);
|
||||
$clinicMap = [];
|
||||
|
||||
@@ -156,9 +156,10 @@ class ClinicServiceController extends BaseController
|
||||
|
||||
if (!empty($data['staff_uuid'])) {
|
||||
$staff = $this->staffRepo->findByUuid($data['staff_uuid']);
|
||||
if ($staff !== null) {
|
||||
$item->setStaff($staff);
|
||||
if ($staff === null || $staff->getEntityType() !== $entityType || $staff->getEntityId() !== $entityId) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_001, 'پرسنل انتخابشده متعلق به شما نیست', 422, 'staff_uuid');
|
||||
}
|
||||
$item->setStaff($staff);
|
||||
}
|
||||
|
||||
if (isset($data['insurance_covered'])) {
|
||||
@@ -193,7 +194,13 @@ class ClinicServiceController extends BaseController
|
||||
if (isset($data['price_rials'])) { $item->setPriceRials((int) $data['price_rials']); $priceChanged = true; }
|
||||
if (isset($data['active'])) { $item->setActive((bool) $data['active']); }
|
||||
if (array_key_exists('staff_uuid', $data)) {
|
||||
$staff = $data['staff_uuid'] ? $this->staffRepo->findByUuid($data['staff_uuid']) : null;
|
||||
$staff = null;
|
||||
if ($data['staff_uuid']) {
|
||||
$staff = $this->staffRepo->findByUuid($data['staff_uuid']);
|
||||
if ($staff === null || $staff->getEntityType() !== $entityType || $staff->getEntityId() !== $entityId) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_001, 'پرسنل انتخابشده متعلق به شما نیست', 422, 'staff_uuid');
|
||||
}
|
||||
}
|
||||
$item->setStaff($staff);
|
||||
}
|
||||
if (isset($data['insurance_covered'])) {
|
||||
|
||||
@@ -560,13 +560,17 @@ class DoctorController extends BaseController
|
||||
)]
|
||||
#[Route('/api/v1/clinic-pro/doctor-address/{id}', methods: ['GET'])]
|
||||
#[IsGranted('IS_AUTHENTICATED_FULLY')]
|
||||
public function showAddress(int $id): JsonResponse
|
||||
public function showAddress(int $id, #[CurrentUser] User $user): JsonResponse
|
||||
{
|
||||
$address = $this->addressRepo->find($id);
|
||||
if ($address === null) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_002, 'آدرس یافت نشد', 404);
|
||||
}
|
||||
|
||||
if ($address->getDoctor()?->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) {
|
||||
return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403);
|
||||
}
|
||||
|
||||
return $this->success(['data' => $address->toArray()]);
|
||||
}
|
||||
|
||||
|
||||
@@ -506,13 +506,18 @@ class InsuranceController extends BaseController
|
||||
}
|
||||
|
||||
#[Route('/api/v1/insurance/{id}', methods: ['GET'])]
|
||||
public function showDoctorInsurance(int $id): JsonResponse
|
||||
#[IsGranted('IS_AUTHENTICATED_FULLY')]
|
||||
public function showDoctorInsurance(int $id, #[CurrentUser] User $user): JsonResponse
|
||||
{
|
||||
$doctorInsurance = $this->doctorInsuranceRepo->find($id);
|
||||
if ($doctorInsurance === null) {
|
||||
return $this->error(ErrorCodes::ERR_VALIDATION_002, 'بیمه پزشک یافت نشد', 404);
|
||||
}
|
||||
|
||||
if ($doctorInsurance->getDoctor()->getUser()->getId() !== $user->getId() && !$user->hasRole('ROLE_ADMIN')) {
|
||||
return $this->error(ErrorCodes::ERR_AUTH_006, 'دسترسی ممنوع', 403);
|
||||
}
|
||||
|
||||
return $this->success(['data' => $doctorInsurance->toArray()]);
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Appointment;
|
||||
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Tests\ApiTestCase;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
|
||||
/**
|
||||
* The appointment-settings list endpoints (date-overrides, holidays,
|
||||
* available-locations) must not leak another doctor's configuration.
|
||||
*/
|
||||
class AppointmentSettingsListOwnershipTest extends ApiTestCase
|
||||
{
|
||||
private function makeDoctor(): Doctor
|
||||
{
|
||||
$doctor = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر');
|
||||
$this->em->persist($doctor);
|
||||
$this->em->flush();
|
||||
|
||||
return $doctor;
|
||||
}
|
||||
|
||||
/** @return iterable<string, array{0: string}> */
|
||||
public static function endpoints(): iterable
|
||||
{
|
||||
yield 'date-override list' => ['/api/v1/appointment-settings/date-override/list/'];
|
||||
yield 'holidays list' => ['/api/v1/appointment-settings/holidays/list/'];
|
||||
yield 'available-locations' => ['/api/v1/appointment-settings/available-locations/'];
|
||||
}
|
||||
|
||||
#[DataProvider('endpoints')]
|
||||
public function testStrangerForbidden(string $prefix): void
|
||||
{
|
||||
$doctor = $this->makeDoctor();
|
||||
$stranger = $this->createUser(['ROLE_DOCTOR']);
|
||||
|
||||
$this->authJson('GET', $prefix . $doctor->getUuid(), $stranger);
|
||||
|
||||
$this->assertSame(403, $this->responseCode());
|
||||
}
|
||||
|
||||
#[DataProvider('endpoints')]
|
||||
public function testOwnerAllowed(string $prefix): void
|
||||
{
|
||||
$doctor = $this->makeDoctor();
|
||||
|
||||
$this->authJson('GET', $prefix . $doctor->getUuid(), $doctor->getUser());
|
||||
|
||||
$this->assertSame(200, $this->responseCode());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,44 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\ClinicService;
|
||||
|
||||
use App\ClinicService\Entity\ServiceItem;
|
||||
use App\ClinicService\Entity\ServiceSection;
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Staff\Entity\ClinicStaff;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* Binding staff to a service item must be scoped: a tenant must not attach
|
||||
* another tenant's staff member via staff_uuid.
|
||||
*/
|
||||
class ServiceItemStaffOwnershipTest extends ApiTestCase
|
||||
{
|
||||
public function testCannotBindForeignStaff(): void
|
||||
{
|
||||
// owner A: their section + item
|
||||
$ownerA = $this->createUser(['ROLE_DOCTOR']);
|
||||
$doctorA = new Doctor($ownerA, 'دکتر A');
|
||||
$this->em->persist($doctorA);
|
||||
$this->em->flush();
|
||||
|
||||
$section = new ServiceSection('doctor', $doctorA->getId(), 'بخش A');
|
||||
$item = new ServiceItem($section, 'خدمت');
|
||||
$this->em->persist($section);
|
||||
$this->em->persist($item);
|
||||
|
||||
// unrelated tenant B's staff
|
||||
$doctorB = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر B');
|
||||
$this->em->persist($doctorB);
|
||||
$this->em->flush();
|
||||
$foreignStaff = new ClinicStaff('doctor', $doctorB->getId(), 'پرسنل B');
|
||||
$this->em->persist($foreignStaff);
|
||||
$this->em->flush();
|
||||
|
||||
$this->authJson('PATCH', '/api/v1/service-item/' . $item->getUuid(), $ownerA, [
|
||||
'staff_uuid' => $foreignStaff->getUuid(),
|
||||
]);
|
||||
|
||||
$this->assertSame(422, $this->responseCode());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,45 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Doctor;
|
||||
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Doctor\Entity\DoctorAddress;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* GET /clinic-pro/doctor-address/{id} must enforce ownership — a doctor must not
|
||||
* read another doctor's address by id enumeration.
|
||||
*/
|
||||
class DoctorAddressOwnershipTest extends ApiTestCase
|
||||
{
|
||||
private function makeAddress(): DoctorAddress
|
||||
{
|
||||
$doctor = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر مالک');
|
||||
$this->em->persist($doctor);
|
||||
$address = DoctorAddress::forDoctor($doctor)->setName('مطب');
|
||||
$this->em->persist($address);
|
||||
$this->em->flush();
|
||||
|
||||
return $address;
|
||||
}
|
||||
|
||||
public function testOtherDoctorCannotRead(): void
|
||||
{
|
||||
$address = $this->makeAddress();
|
||||
$stranger = $this->createUser(['ROLE_DOCTOR']);
|
||||
|
||||
$this->authJson('GET', '/api/v1/clinic-pro/doctor-address/' . $address->getId(), $stranger);
|
||||
|
||||
$this->assertSame(403, $this->responseCode());
|
||||
}
|
||||
|
||||
public function testOwnerCanRead(): void
|
||||
{
|
||||
$address = $this->makeAddress();
|
||||
$owner = $address->getDoctor()->getUser();
|
||||
|
||||
$this->authJson('GET', '/api/v1/clinic-pro/doctor-address/' . $address->getId(), $owner);
|
||||
|
||||
$this->assertSame(200, $this->responseCode());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,49 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Insurance;
|
||||
|
||||
use App\Doctor\Entity\Doctor;
|
||||
use App\Insurance\Entity\DoctorInsurance;
|
||||
use App\Insurance\Entity\Insurance;
|
||||
use App\Insurance\Enum\InsuranceType;
|
||||
use App\Tests\ApiTestCase;
|
||||
|
||||
/**
|
||||
* GET /api/v1/insurance/{id} must enforce ownership — a doctor must not read
|
||||
* another doctor's insurance record (incl. negotiated price) by id enumeration.
|
||||
*/
|
||||
class DoctorInsuranceOwnershipTest extends ApiTestCase
|
||||
{
|
||||
private function makeDoctorInsurance(): DoctorInsurance
|
||||
{
|
||||
$doctor = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر مالک');
|
||||
$this->em->persist($doctor);
|
||||
$insurance = new Insurance('بیمه تست', InsuranceType::Basic);
|
||||
$this->em->persist($insurance);
|
||||
$di = new DoctorInsurance($doctor, $insurance);
|
||||
$this->em->persist($di);
|
||||
$this->em->flush();
|
||||
|
||||
return $di;
|
||||
}
|
||||
|
||||
public function testOtherDoctorCannotRead(): void
|
||||
{
|
||||
$di = $this->makeDoctorInsurance();
|
||||
$stranger = $this->createUser(['ROLE_DOCTOR']);
|
||||
|
||||
$this->authJson('GET', '/api/v1/insurance/' . $di->getId(), $stranger);
|
||||
|
||||
$this->assertSame(403, $this->responseCode());
|
||||
}
|
||||
|
||||
public function testOwnerCanRead(): void
|
||||
{
|
||||
$di = $this->makeDoctorInsurance();
|
||||
$owner = $di->getDoctor()->getUser();
|
||||
|
||||
$this->authJson('GET', '/api/v1/insurance/' . $di->getId(), $owner);
|
||||
|
||||
$this->assertSame(200, $this->responseCode());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user