Fix product null department permissions

Treats null-like optional product query params as omitted and avoids department_access_0 permission checks.
2026-07-06 20:14:45 +02:00
parent c7f5c73a9e ed2736e528
commit 709c6acbba
2 changed files with 150 additions and 42 deletions
@@ -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
@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+usesApiSuite();
+
+it('treats null-like optional product params as omitted for product detail requests', function (): void {
+    api_test_covers('GET /products', 'optional-params');
+
+    $product = api_fixtures()->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');
+});