From ed2736e52893d3512b62861e76d0afd0aba319f8 Mon Sep 17 00:00:00 2001 From: Jeppe Bundgaard Date: Mon, 6 Jul 2026 19:56:07 +0200 Subject: [PATCH] Fix product null department permissions --- services/nginx/app/routes/productsRoute.php | 117 +++++++++++------- .../nginx/app/tests/Api/ProductsApiTest.php | 75 +++++++++++ 2 files changed, 150 insertions(+), 42 deletions(-) create mode 100644 services/nginx/app/tests/Api/ProductsApiTest.php diff --git a/services/nginx/app/routes/productsRoute.php b/services/nginx/app/routes/productsRoute.php index f19314be..555aac9b 100644 --- a/services/nginx/app/routes/productsRoute.php +++ b/services/nginx/app/routes/productsRoute.php @@ -22,21 +22,23 @@ class productsRoute */ private function getCustomerIfProvided(): ?users_o { - global $response; - if (self::isParametersSet(['customer_id'])) { - $customerId = (int)self::getParameter('customer_id'); - try { - $customerObject = (new users_o())->getUserByCustomerNumber((int)$customerId); - if ($customerObject->exists()) { - return $customerObject; - } - } catch (\Exception $e) { - // Log the incident - (new logs_o())->add('products', 'global', 3, 0, 'GET_CUSTOMER_FAILED', 'Failed to get customer with id ' . $customerId . '. Error: ' . $e->getMessage()); - // Return null - return null; - } + $customerId = $this->getOptionalPositiveIntParameter('customer_id'); + if ($customerId === null) { + return null; } + + try { + $customerObject = (new users_o())->getUserByCustomerNumber($customerId); + if ($customerObject->exists()) { + return $customerObject; + } + } catch (\Exception $e) { + // Log the incident + (new logs_o())->add('products', 'global', 3, 0, 'GET_CUSTOMER_FAILED', 'Failed to get customer with id ' . $customerId . '. Error: ' . $e->getMessage()); + // Return null + return null; + } + return null; } @@ -45,12 +47,49 @@ class productsRoute * @return int|null */ private function getDepartmentIdIfProvided(): ?int + { + return $this->getOptionalPositiveIntParameter('department_id'); + } + + private function getOptionalPositiveIntParameter(string $parameter): ?int { global $response; - if (self::isParametersSet(['department_id'])) { - return (int)self::getParameter('department_id'); + if (!self::isParametersSet([$parameter])) { + return null; } - return null; + + $value = self::getParameter($parameter); + if ($this->isNullLikeOptionalParameter($value)) { + return null; + } + + $parsed = null; + if (is_int($value)) { + $parsed = $value; + } elseif (is_string($value) && preg_match('/^\d+$/', trim($value)) === 1) { + $parsed = (int)trim($value); + } else { + $response->error('Invalid ' . $parameter, 400); + } + + if ($parsed === null || $parsed <= 0) { + $response->error('Invalid ' . $parameter, 400); + } + + return $parsed; + } + + private function isNullLikeOptionalParameter(mixed $value): bool + { + if ($value === null) { + return true; + } + + if (!is_string($value)) { + return false; + } + + return in_array(strtolower(trim($value)), ['', 'null', 'undefined'], true); } private function assertCanUseDepartmentPricing(mixed $user, ?int $departmentId): void @@ -72,11 +111,7 @@ class productsRoute */ private function getCategoryIfProvided(): ?int { - global $response; - if (self::isParametersSet(['category'])) { - return (int)self::getParameter('category'); - } - return null; + return $this->getOptionalPositiveIntParameter('category'); } /** @@ -85,11 +120,7 @@ class productsRoute */ private function getProductIdIfProvided(): ?int { - global $response; - if (self::isParametersSet(['id'])) { - return (int)self::getParameter('id'); - } - return null; + return $this->getOptionalPositiveIntParameter('id'); } /** @@ -210,13 +241,14 @@ class productsRoute // Check if the request was successful if ($user || $isProductDetailsRestricted) { // Define the variables - $customer = self::getCustomerIfProvided(); // This is only used if the customer_id parameter is provided - $departmentId = self::getDepartmentIdIfProvided(); // This is only used if the department_id parameter is provided - $this->assertCanUseDepartmentPricing($user, $departmentId); - $category = self::getCategoryIfProvided(); // This is only used if the category parameter is provided (ID of the category) - $productId = self::getProductIdIfProvided(); // This is only used if the id parameter is provided (ID of the product) + $customer = $this->getCustomerIfProvided(); // This is only used if the customer_id parameter is provided + $departmentId = $this->getDepartmentIdIfProvided(); // This is only used if the department_id parameter is provided + $category = $this->getCategoryIfProvided(); // This is only used if the category parameter is provided (ID of the category) + $productId = $this->getProductIdIfProvided(); // This is only used if the id parameter is provided (ID of the product) + $useFinalPrice = self::isParametersSet(['final_price']) && self::getParameter('final_price') === 'true'; // Check if the "final_price" parameter is set, and true. - if (self::isParametersSet(['final_price']) && self::getParameter('final_price') === 'true') { + if ($useFinalPrice) { + $this->assertCanUseDepartmentPricing($user, $departmentId); // Determine the products to return if ($category) { // Get products in the category @@ -274,17 +306,17 @@ class productsRoute ); } // Check if the category is set in the request - $data = $_GET ?? []; // Check if the category is set - if (isset($data['category'])) { + if ($category !== null) { // Log the incident - (new logs_o())->add('products', 'global', 1, $responsibleUserId, 'LIST_PRODUCTS', 'Successfully listed products in category ' . $data['category']); + (new logs_o())->add('products', 'global', 1, $responsibleUserId, 'LIST_PRODUCTS', 'Successfully listed products in category ' . $category); // Return the list of products - $products = (new products_o())->listObjectsByCategory($data['category']); + $products = (new products_o())->listObjectsByCategory($category); // Check if the department_id is set - if (isset($data['department_id'])) { + if ($departmentId !== null) { + $this->assertCanUseDepartmentPricing($user, $departmentId); // Apply the departments unique pricing - $products = (new products_o())->applyDepartmentPricing((array)$products, (int)$data['department_id']); + $products = (new products_o())->applyDepartmentPricing((array)$products, $departmentId); } $response->success( array_map(function ($product) use ($isProductDetailsRestricted) { @@ -295,9 +327,10 @@ class productsRoute // Log the incident (new logs_o())->add('products', 'global', 1, $responsibleUserId, 'LIST_PRODUCTS', 'Successfully listed products'); // Check if the department_id is set - if (isset($data['department_id'])) { + if ($departmentId !== null) { + $this->assertCanUseDepartmentPricing($user, $departmentId); // Get all product ids contained in a category attached to the department - $departmentSpecificProducts = (new departments_o())->select((int)$data['department_id'])->getAllProductInDepartmentCategories(); + $departmentSpecificProducts = (new departments_o())->select($departmentId)->getAllProductInDepartmentCategories(); // Get the product ids as an array $departmentSpecificProductIds = array_map(function ($product) { return $product->id; @@ -312,7 +345,7 @@ class productsRoute (new products_o())->forceRestrictFilters([ 'id' => $departmentSpecificProductIds, ]) - ), (int)$data['department_id']) + ), $departmentId) ); } // Return the list of products diff --git a/services/nginx/app/tests/Api/ProductsApiTest.php b/services/nginx/app/tests/Api/ProductsApiTest.php new file mode 100644 index 00000000..3d2066d0 --- /dev/null +++ b/services/nginx/app/tests/Api/ProductsApiTest.php @@ -0,0 +1,75 @@ +createProduct([ + 'name' => 'Null Query Product', + 'price' => 400, + ]); + $session = api_fixtures()->createUserSession([], ['group_id' => 1]); + + $response = api_client()->get( + '/products?id=' . (int)$product['id'] + . '&department_id=null&customer_id=null&category_id=null&final_price=false', + $session['headers'] + ); + + $response + ->assertStatus(200) + ->assertEnvelope() + ->assertSuccess(); + + expect($response->data()) + ->toBeArray() + ->toHaveKey('id', (int)$product['id']); + expect($response->body)->not->toContain('department_access_0'); +}); + +it('still requires department access when final product pricing uses a real department', function (): void { + api_test_covers('GET /products', 'permissions'); + + $department = api_fixtures()->createDepartment(['name' => 'Product Pricing Department']); + $product = api_fixtures()->createProduct([ + 'name' => 'Department Priced Product', + 'price' => 500, + ]); + $session = api_fixtures()->createUserSession(['list_products']); + + api_client()->get( + '/products?final_price=true&id=' . (int)$product['id'] + . '&department_id=' . (int)$department['id'], + $session['headers'] + ) + ->assertStatus(403) + ->assertEnvelope() + ->assertSuccess(false) + ->assertMissingPermissions(['department_access_' . (int)$department['id']]); +}); + +it('rejects invalid department ids without requesting department access zero', function (): void { + api_test_covers('GET /products', 'validation'); + + $product = api_fixtures()->createProduct([ + 'name' => 'Invalid Department Product', + 'price' => 600, + ]); + $session = api_fixtures()->createUserSession(['list_products']); + + $response = api_client()->get( + '/products?final_price=true&id=' . (int)$product['id'] . '&department_id=0', + $session['headers'] + ); + + $response + ->assertStatus(400) + ->assertEnvelope() + ->assertSuccess(false) + ->assertMessage('Invalid department_id'); + + expect($response->body)->not->toContain('department_access_0'); +});