From 52c45443c58d036e1c2db47c4d9dfb66f8021336 Mon Sep 17 00:00:00 2001 From: hamed <15238-genius.ha@users.noreply.drupalcode.org> Date: Fri, 7 Aug 2026 18:46:16 +0330 Subject: [PATCH] fix(security): gate every ServiceCatalogController route on the services permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The controller carried only IS_AUTHENTICATED_FULLY on the class and none of its 15 routes checked a permission. A secretary whose owner had turned `services` fully off could still create, rename and delete service categories, build item groups, replace group members, and rewrite service relations and per-branch overrides. Scope is intra-tenant privilege escalation, not IDOR: owned() and requireItem() already resolve every uuid against the caller's active environment, so no data crossed tenants. Gating is per-action (view/create/update/delete) and reuses denyServices() from ClinicServiceController in the same domain, so a secretary with `update` cannot create or delete. The call is the first statement in every action, before requireCategory/requireItem — placed after, an unknown uuid would answer 404 and leak whether the record exists. An earlier note claimed these endpoints were consumed by the booking flow and so could not be closed. That was wrong. service-selection/validate, the group routes and the relation routes have no consumer in any of the three API clients, and the sibling controller already puts every service read behind services.view — the booking modal reads service-items through it — so any flow needing services already needed the permission. The docs claimed appointment_settings.* for the includes routes, which was never enforced either; corrected to services.*. The test loops the whole route list rather than sampling, and a guard asserts the count of #[Route( equals the count of denyServices( so a future ungated route fails here. Co-Authored-By: Claude Opus 5 --- docs/api/clinic-services.md | 47 ++++- docs/api/secretary.md | 2 +- .../Controller/ServiceCatalogController.php | 47 +++++ .../ServiceCatalogPermissionTest.php | 166 ++++++++++++++++++ 4 files changed, 258 insertions(+), 4 deletions(-) create mode 100644 tests/ClinicService/ServiceCatalogPermissionTest.php diff --git a/docs/api/clinic-services.md b/docs/api/clinic-services.md index 331badae..9faf1abc 100644 --- a/docs/api/clinic-services.md +++ b/docs/api/clinic-services.md @@ -461,6 +461,47 @@ override فقط وقتی اعمال می‌شود که `branch_uuid` به `valid **قیمت اینجا نیست.** `price_rials` از این اندپوینت حذف شده؛ ارسالش نادیده گرفته می‌شود و در پاسخ هم نمی‌آید. +## مجوزهای کاتالوگ — `ServiceCatalogController` + +هر ۱۵ routeِ این کنترلر پشت منبعِ `services` است، per-action و با همان +`denyServices()` کنترلرِ خواهر (`ClinicServiceController`). منشی و پزشکِ عضوِ کلینیک +هرکدام با مجوزِ خودشان سنجیده می‌شوند؛ مالکِ کلینیک، پزشکِ مطبِ شخصی و ادمین عبور +می‌کنند. + +> تا پیش از این کنترلر **هیچ گِیتی نداشت** و فقط `IS_AUTHENTICATED_FULLY` روی کلاس بود؛ +> منشی با `services` کاملاً خاموش هم می‌توانست کاتالوگِ محیط خودش را بنویسد. مالکیتِ +> محیط همیشه enforce بوده (`owned()` / `requireItem()`)، پس دادهٔ محیط دیگری در دسترس +> نبوده — مسئله بالا رفتن سطح دسترسی داخل همان محیط بود. + +| متد | مسیر | مجوز | +|---|---|---| +| GET | `/api/v1/service-categories/tree` | `services.view` | +| GET | `/api/v1/service-category/{uuid}/includes` | `services.view` | +| GET | `/api/v1/service-item/{uuid}/groups` | `services.view` | +| POST | `/api/v1/service-selection/validate` | `services.view` | +| POST | `/api/v1/service-category` | `services.create` | +| POST | `/api/v1/service-category/{uuid}/includes` | `services.create` | +| POST | `/api/v1/service-item/{uuid}/groups` | `services.create` | +| PATCH | `/api/v1/service-category/{uuid}` | `services.update` | +| PATCH | `/api/v1/item-group/{uuid}` | `services.update` | +| PUT | `/api/v1/item-group/{uuid}/items` | `services.update` | +| PUT | `/api/v1/service-item/{uuid}/relations` | `services.update` | +| PUT | `/api/v1/service-item/{uuid}/branch-overrides` | `services.update` | +| DELETE | `/api/v1/service-category/{uuid}` | `services.delete` | +| DELETE | `/api/v1/service-category/{uuid}/includes/{childUuid}` | `services.delete` | +| DELETE | `/api/v1/item-group/{uuid}` | `services.delete` | + +`service-selection/validate` عمداً `view` است نه `create`: چیزی نمی‌سازد و فقط یک انتخاب +را اعتبارسنجی می‌کند؛ POST بودنش به‌خاطر حجمِ بدنه است. + +گِیت **اولین دستور هر action** است، پیش از `requireCategory`/`requireItem`. اگر بعد از آن +می‌آمد، uuidِ ناشناخته ۴۰۴ می‌داد و وجود/نبودِ رکورد لو می‌رفت. نبودِ مجوز → +`403 ERR_FORBIDDEN_001`. + +تست: `tests/ClinicService/ServiceCatalogPermissionTest.php` — روی **کل** فهرست routeها +حلقه می‌زند و یک تستِ نگهبان دارد که تعداد `#[Route(` و `denyServices(` را برابر می‌خواهد، +تا routeِ تازهٔ بدون گِیت قرمز شود. + ## دستهٔ درختی `GET /api/v1/service-categories/tree` · `POST/PATCH/DELETE /api/v1/service-category[/{uuid}]` @@ -481,9 +522,9 @@ override فقط وقتی اعمال می‌شود که `branch_uuid` به `valid | متد | مسیر | مجوز | |---|---|---| -| GET | `/api/v1/service-category/{uuid}/includes` | `appointment_settings.view` | -| POST | `/api/v1/service-category/{uuid}/includes` | `appointment_settings.update` | -| DELETE | `/api/v1/service-category/{uuid}/includes/{childUuid}` | `appointment_settings.update` | +| GET | `/api/v1/service-category/{uuid}/includes` | `services.view` | +| POST | `/api/v1/service-category/{uuid}/includes` | `services.create` | +| DELETE | `/api/v1/service-category/{uuid}/includes/{childUuid}` | `services.delete` | **POST body:** `{ "child_category_uuid": "" }` — الزامی. diff --git a/docs/api/secretary.md b/docs/api/secretary.md index 7f13371c..ebdda652 100644 --- a/docs/api/secretary.md +++ b/docs/api/secretary.md @@ -150,7 +150,7 @@ Create a secretary for a doctor. | `insurances` | `InsuranceController` (insurance-pricing, tenant-insurances, service-coverage, doctor-insurance) | view/create/update/delete | | `inventory` | `InventoryController` (items + packages) | view/create/update/delete | | `tags` | `TenantTagController` (لیست با `tags.view` یا `patients.view`؛ نوشتن‌ها با `tags.*`) | view/create/update/delete | -| `services` | `ClinicServiceController` (sections + items). ⚠ `ServiceCatalogController` (دسته‌بندی سرویس‌ها، گروه‌ها، روابط) گِیت **ندارد** — اندپوینت‌هایش در جریانِ ثبت نوبت هم مصرف می‌شوند و بستنِ یکجا نوبت‌دهی منشی را می‌شکند. owner از محیطِ فعال با `SecretaryAccessChecker::resolveOwnerEntity` حل می‌شود چون `EntityContextResolver` منشی را نمی‌شناسد. گیتِ `services.*` پیش از گیتِ اشتراک اجرا می‌شود | view/create/update/delete | +| `services` | `ClinicServiceController` (sections + items) و `ServiceCatalogController` (دستهٔ درختی، گروه انتخاب، روابط، override شعبه‌ای — هر ۱۵ route، per-action؛ [clinic-services.md](clinic-services.md)). owner از محیطِ فعال با `SecretaryAccessChecker::resolveOwnerEntity` حل می‌شود چون `EntityContextResolver` منشی را نمی‌شناسد. گیتِ `services.*` پیش از گیتِ اشتراک اجرا می‌شود | view/create/update/delete | | `staff` | `StaffController` (resolveEntity منشی‌آگاه) | view/create/update/delete | | `discounts` | `DiscountController` (CRUD؛ `suggestions` جزو flowِ جلسه است و با discounts گِیت نمی‌شود) | view/create/update/delete | | `sms` | `SmsWalletController` (balance/charge/logs/settings). endpointهای admin (قالب/ارسال) همچنان `ROLE_ADMIN` | view/create/update | diff --git a/src/ClinicService/Controller/ServiceCatalogController.php b/src/ClinicService/Controller/ServiceCatalogController.php index 7714b2bf..37c5172a 100644 --- a/src/ClinicService/Controller/ServiceCatalogController.php +++ b/src/ClinicService/Controller/ServiceCatalogController.php @@ -3,6 +3,8 @@ namespace App\ClinicService\Controller; use App\Auth\Entity\User; +use App\Clinic\Security\ClinicDoctorAccessChecker; +use App\Secretary\Security\SecretaryAccessChecker; use App\Doctor\Service\AddressResolver; use App\ClinicService\Entity\CatalogCategory; use App\ClinicService\Entity\CatalogCategoryInclude; @@ -55,13 +57,30 @@ class ServiceCatalogController extends BaseController private readonly CatalogCategoryIncludeRepository $includes, private readonly CategoryClosureResolver $closure, private readonly EntityManagerInterface $em, + private readonly SecretaryAccessChecker $secretaryAccess, + private readonly ClinicDoctorAccessChecker $clinicDoctorAccess, ) {} + /** + * گِیتِ ترکیبی: منشی + پزشکِ عضوِ کلینیک (هرکدام فقط نقشِ خودش را محدود می‌کند). + * قرینهٔ ClinicServiceController::denyServices — همان دامنه، همان معنا. + * + * پیش از هر کار دیگری صدا زده می‌شود؛ اگر بعد از requireCategory بیاید، + * ۴۰۴/۴۲۲ جای ۴۰۳ برمی‌گردد و وجود/نبودِ رکورد لو می‌رود. + */ + private function denyServices(User $user, string $action): void + { + $this->secretaryAccess->denyUnlessGranted($user, 'services', $action); + $this->clinicDoctorAccess->denyUnlessGranted($user, 'services', $action); + } + // ── دستهٔ درختی ───────────────────────────────────────────────────────── #[Route('/api/v1/service-categories/tree', name: 'service_category_tree', methods: ['GET'])] public function tree(#[CurrentUser] User $user): JsonResponse { + $this->denyServices($user, 'view'); + [$entityType, $entityId] = $this->branches->pair($user); // کل درخت با یک کوئری خوانده و در PHP بسته می‌شود. @@ -89,6 +108,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category', name: 'service_category_create', methods: ['POST'])] public function createCategory(#[CurrentUser] User $user, Request $request): JsonResponse { + $this->denyServices($user, 'create'); + $data = json_decode($request->getContent(), true); $name = is_array($data) && is_string($data['name'] ?? null) ? trim($data['name']) : ''; @@ -126,6 +147,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category/{uuid}', name: 'service_category_update', methods: ['PATCH'])] public function updateCategory(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'update'); + $data = json_decode($request->getContent(), true); $category = $this->requireCategory($user, $uuid); @@ -153,6 +176,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category/{uuid}', name: 'service_category_delete', methods: ['DELETE'])] public function deleteCategory(#[CurrentUser] User $user, string $uuid): JsonResponse { + $this->denyServices($user, 'delete'); + $category = $this->requireCategory($user, $uuid); if ($this->categories->countChildren($category) > 0) { @@ -176,6 +201,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category/{uuid}/includes', name: 'service_category_includes', methods: ['GET'])] public function listIncludes(#[CurrentUser] User $user, string $uuid): JsonResponse { + $this->denyServices($user, 'view'); + $category = $this->requireCategory($user, $uuid); return $this->success(array_map( @@ -187,6 +214,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category/{uuid}/includes', name: 'service_category_include_add', methods: ['POST'])] public function addInclude(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'create'); + $data = json_decode($request->getContent(), true); $childUuid = is_array($data) && is_string($data['child_category_uuid'] ?? null) ? trim($data['child_category_uuid']) : ''; @@ -213,6 +242,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-category/{uuid}/includes/{childUuid}', name: 'service_category_include_remove', methods: ['DELETE'])] public function removeInclude(#[CurrentUser] User $user, string $uuid, string $childUuid): JsonResponse { + $this->denyServices($user, 'delete'); + $edge = $this->includes->findEdge( $this->requireCategory($user, $uuid), $this->requireCategory($user, $childUuid), @@ -231,6 +262,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-item/{uuid}/groups', name: 'service_item_groups', methods: ['GET'])] public function listGroups(#[CurrentUser] User $user, string $uuid): JsonResponse { + $this->denyServices($user, 'view'); + $service = $this->requireItem($user, $uuid); return $this->success(array_map( @@ -242,6 +275,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-item/{uuid}/groups', name: 'service_item_group_create', methods: ['POST'])] public function createGroup(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'create'); + $data = json_decode($request->getContent(), true); $service = $this->requireItem($user, $uuid); $name = is_array($data) && is_string($data['name'] ?? null) ? trim($data['name']) : ''; @@ -266,6 +301,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/item-group/{uuid}', name: 'item_group_update', methods: ['PATCH'])] public function updateGroup(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'update'); + $data = json_decode($request->getContent(), true); $group = $this->requireGroup($user, $uuid); @@ -291,6 +328,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/item-group/{uuid}', name: 'item_group_delete', methods: ['DELETE'])] public function deleteGroup(#[CurrentUser] User $user, string $uuid): JsonResponse { + $this->denyServices($user, 'delete'); + $this->em->remove($this->requireGroup($user, $uuid)); $this->em->flush(); @@ -301,6 +340,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/item-group/{uuid}/items', name: 'item_group_items_replace', methods: ['PUT'])] public function replaceGroupItems(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'update'); + $data = json_decode($request->getContent(), true); if (!is_array($data) || !is_array($data['items'] ?? null)) { @@ -343,6 +384,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-item/{uuid}/relations', name: 'service_item_relations_replace', methods: ['PUT'])] public function replaceRelations(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'update'); + $data = json_decode($request->getContent(), true); if (!is_array($data) || !is_array($data['relations'] ?? null)) { @@ -431,6 +474,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-item/{uuid}/branch-overrides', name: 'service_item_overrides_replace', methods: ['PUT'])] public function replaceBranchOverrides(#[CurrentUser] User $user, string $uuid, Request $request): JsonResponse { + $this->denyServices($user, 'update'); + $data = json_decode($request->getContent(), true); if (!is_array($data) || !is_array($data['overrides'] ?? null)) { @@ -487,6 +532,8 @@ class ServiceCatalogController extends BaseController #[Route('/api/v1/service-selection/validate', name: 'service_selection_validate', methods: ['POST'])] public function validateSelection(#[CurrentUser] User $user, Request $request): JsonResponse { + $this->denyServices($user, 'view'); + $data = json_decode($request->getContent(), true); if (!is_array($data) || !is_array($data['item_uuids'] ?? null)) { diff --git a/tests/ClinicService/ServiceCatalogPermissionTest.php b/tests/ClinicService/ServiceCatalogPermissionTest.php new file mode 100644 index 00000000..511a876e --- /dev/null +++ b/tests/ClinicService/ServiceCatalogPermissionTest.php @@ -0,0 +1,166 @@ + [method, path, action, body] */ + private static function routes(): array + { + $cat = '11111111-1111-1111-1111-111111111111'; + $item = '22222222-2222-2222-2222-222222222222'; + $grp = '33333333-3333-3333-3333-333333333333'; + + return [ + ['GET', '/api/v1/service-categories/tree', 'view', []], + ['GET', "/api/v1/service-category/{$cat}/includes", 'view', []], + ['GET', "/api/v1/service-item/{$item}/groups", 'view', []], + ['POST', '/api/v1/service-selection/validate', 'view', ['item_uuids' => []]], + ['POST', '/api/v1/service-category', 'create', ['name' => 'x']], + ['POST', "/api/v1/service-category/{$cat}/includes", 'create', ['child_category_uuid' => $cat]], + ['POST', "/api/v1/service-item/{$item}/groups", 'create', ['name' => 'x']], + ['PATCH', "/api/v1/service-category/{$cat}", 'update', ['name' => 'x']], + ['PATCH', "/api/v1/item-group/{$grp}", 'update', ['name' => 'x']], + ['PUT', "/api/v1/item-group/{$grp}/items", 'update', ['item_uuids' => []]], + ['PUT', "/api/v1/service-item/{$item}/relations", 'update', ['relations' => []]], + ['PUT', "/api/v1/service-item/{$item}/branch-overrides", 'update', ['overrides' => []]], + ['DELETE', "/api/v1/service-category/{$cat}/includes/{$cat}", 'delete', []], + ['DELETE', "/api/v1/item-group/{$grp}", 'delete', []], + ['DELETE', "/api/v1/service-category/{$cat}", 'delete', []], + ]; + } + + /** @return array{0: \App\Auth\Entity\User, 1: DoctorSecretary, 2: \App\Auth\Entity\User} */ + private function makeClinicSecretary(): array + { + $owner = $this->createUser(['ROLE_CLINIC']); + $clinic = new Clinic($owner); + $this->em->persist($clinic); + + $doctor = new Doctor($this->createUser(['ROLE_DOCTOR']), 'دکتر تست'); + $this->em->persist($doctor); + $clinic->getDoctors()->add($doctor); + + $secretary = $this->createUser(['ROLE_SECRETARY']); + $rel = new DoctorSecretary($doctor, $secretary, $clinic); + $this->em->persist($rel); + $this->em->persist(new UserActiveContext($secretary, $clinic->getUuid(), 'clinic')); + $this->em->persist(new UserActiveContext($owner, $clinic->getUuid(), 'clinic')); + + return [$secretary, $rel, $owner]; + } + + private function denyAllServices(DoctorSecretary $rel): void + { + $rel->mergePermissions(['resources' => ['services' => [ + 'view' => false, 'create' => false, 'update' => false, 'delete' => false, + ]]]); + } + + public function testEveryRouteIsDeniedWhenServicesIsOff(): void + { + [$secretary, $rel] = $this->makeClinicSecretary(); + $this->denyAllServices($rel); + $this->em->flush(); + + foreach (self::routes() as [$method, $path, $action, $body]) { + $this->authJson($method, $path, $secretary, $body); + self::assertSame( + 403, + $this->responseCode(), + "{$method} {$path} باید ۴۰۳ بدهد وقتی services.{$action} خاموش است", + ); + } + } + + /** + * گِیت باید **قبل** از requireCategory/requireItem اجرا شود. اگر بعدش بیاید، + * uuidِ ساختگی ۴۰۴ می‌دهد و وجود/نبودِ رکورد لو می‌رود. + */ + public function testDeniedWithForbiddenNotNotFoundForUnknownUuid(): void + { + [$secretary, $rel] = $this->makeClinicSecretary(); + $this->denyAllServices($rel); + $this->em->flush(); + + $this->authJson('DELETE', '/api/v1/service-category/00000000-0000-0000-0000-000000000000', $secretary); + + self::assertSame(403, $this->responseCode(), 'گِیت باید پیش از حل uuid اجرا شود'); + } + + public function testViewOpensReadsButNotWrites(): void + { + [$secretary, $rel] = $this->makeClinicSecretary(); + $this->denyAllServices($rel); + $rel->mergePermissions(['resources' => ['services' => ['view' => true]]]); + $this->em->flush(); + + $this->authJson('GET', '/api/v1/service-categories/tree', $secretary); + self::assertSame(200, $this->responseCode(), 'خواندنِ درخت با services.view باید باز باشد'); + + foreach (self::routes() as [$method, $path, $action, $body]) { + if ($action === 'view') { + continue; + } + $this->authJson($method, $path, $secretary, $body); + self::assertSame( + 403, + $this->responseCode(), + "{$method} {$path} با فقط services.view نباید اجازه داشته باشد", + ); + } + } + + /** منشیِ دارای update نباید بتواند بسازد یا حذف کند. */ + public function testUpdateDoesNotImplyCreateOrDelete(): void + { + [$secretary, $rel] = $this->makeClinicSecretary(); + $this->denyAllServices($rel); + $rel->mergePermissions(['resources' => ['services' => ['view' => true, 'update' => true]]]); + $this->em->flush(); + + $this->authJson('POST', '/api/v1/service-category', $secretary, ['name' => 'x']); + self::assertSame(403, $this->responseCode(), 'update نباید create بدهد'); + + $this->authJson('DELETE', '/api/v1/service-category/11111111-1111-1111-1111-111111111111', $secretary); + self::assertSame(403, $this->responseCode(), 'update نباید delete بدهد'); + } + + /** مالکِ کلینیک از گِیت عبور می‌کند — هرگز نباید بتواند خودش را قفل کند. */ + public function testClinicOwnerBypassesTheGate(): void + { + [, , $owner] = $this->makeClinicSecretary(); + $this->em->flush(); + + $this->authJson('GET', '/api/v1/service-categories/tree', $owner); + + self::assertSame(200, $this->responseCode()); + } + + /** تضمینِ پوششِ کامل: اگر routeی به کنترلر اضافه شود و به این فهرست نه، اینجا می‌شکند. */ + public function testRouteListCoversEveryControllerRoute(): void + { + $source = file_get_contents( + \dirname(__DIR__, 2) . '/src/ClinicService/Controller/ServiceCatalogController.php', + ); + $routeCount = preg_match_all('/#\[Route\(/', $source); + $gateCount = preg_match_all('/denyServices\(\$user,/', $source); + + self::assertSame($routeCount, $gateCount, 'هر route باید دقیقاً یک denyServices داشته باشد'); + self::assertCount($routeCount, self::routes(), 'فهرست تستِ بالا با تعداد routeهای کنترلر نمی‌خواند'); + } +}