From 8cfd1a4b1d233e14f521502eb91866aed671b3f2 Mon Sep 17 00:00:00 2001 From: objecttothis <17935339+objecttothis@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:08:33 +0400 Subject: [PATCH] fix(sales): enforce reports_sales grant on search endpoint (#4673) - Sales::getSearch now enforces reports_sales grant before running search, returns 403 with lang message when missing - Rename snake_case helpers/methods to camelCase across Sales controller, Sale model, and tabular_helper (get_sale_data_row -> getSaleDataRow, get_payments_summary -> getPaymentsSummary, sales_headers -> salesHeaders, etc.) - Config/OSPOS: reset DB data cache before checking app_config table existence to avoid stale schema cache in tests - TestDatabaseBootstrapSeeder: expose static reset() so tests can rebuild schema once per class instead of only via seeder run() - SalesControllerTest: bootstrap DB once per class, seed once, refresh app settings each setUp, add tests for search endpoint authorization (cashier denied, supervisor allowed), move createTestItem into shared ItemFixtureTrait Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com> --- app/Config/OSPOS.php | 1 + app/Controllers/Sales.php | 32 +++--- .../Seeds/TestDatabaseBootstrapSeeder.php | 7 +- app/Helpers/tabular_helper.php | 10 +- app/Models/Sale.php | 2 +- tests/Controllers/SalesControllerTest.php | 103 +++++++++++++----- 6 files changed, 105 insertions(+), 50 deletions(-) diff --git a/app/Config/OSPOS.php b/app/Config/OSPOS.php index 1b89532b2..a3b4ecb1b 100644 --- a/app/Config/OSPOS.php +++ b/app/Config/OSPOS.php @@ -39,6 +39,7 @@ class OSPOS extends BaseConfig try { $db = Database::connect(); + $db->resetDataCache(); if (!$db->tableExists('app_config')) { $this->settings = $this->getDefaultSettings(); diff --git a/app/Controllers/Sales.php b/app/Controllers/Sales.php index aa9730cad..28627379c 100644 --- a/app/Controllers/Sales.php +++ b/app/Controllers/Sales.php @@ -137,20 +137,26 @@ class Sales extends Secure_Controller } $saleInfo = $this->sale->get_info($rowId)->getRow(); - $dataRow = get_sale_data_row($saleInfo); + $dataRow = getSaleDataRow($saleInfo); return $this->response->setJSON($dataRow); } /** - * @return void + * @return ResponseInterface */ public function getSearch(): ResponseInterface { + $personId = $this->session->get('person_id'); + + if (!$this->employee->has_grant('reports_sales', $personId)) { + return $this->response->setStatusCode(403)->setJSON(['success' => false, 'message' => lang('Sales.not_authorized')]); + } + $search = $this->request->getGet('search', FILTER_SANITIZE_FULL_SPECIAL_CHARS); $limit = $this->request->getGet('limit', FILTER_SANITIZE_NUMBER_INT); $offset = $this->request->getGet('offset', FILTER_SANITIZE_NUMBER_INT); - $sort = $this->sanitizeSortColumn(sales_headers(), $this->request->getGet('sort', FILTER_SANITIZE_FULL_SPECIAL_CHARS), 'sale_id'); + $sort = $this->sanitizeSortColumn(salesHeaders(), $this->request->getGet('sort', FILTER_SANITIZE_FULL_SPECIAL_CHARS), 'sale_id'); $order = $this->request->getGet('order', FILTER_SANITIZE_FULL_SPECIAL_CHARS); $filters = [ @@ -171,24 +177,24 @@ class Sales extends Secure_Controller ]; // Check if any filter is set in the multiselect dropdown - $request_filters = array_fill_keys($this->request->getGet('filters', FILTER_SANITIZE_FULL_SPECIAL_CHARS) ?? [], true); - $filters = array_merge($filters, $request_filters); + $requestFilters = array_fill_keys($this->request->getGet('filters', FILTER_SANITIZE_FULL_SPECIAL_CHARS) ?? [], true); + $filters = array_merge($filters, $requestFilters); $sales = $this->sale->search($search, $filters, $limit, $offset, $sort, $order); - $total_rows = $this->sale->get_found_rows($search, $filters); - $payments = $this->sale->get_payments_summary($search, $filters); - $payment_summary = get_sales_manage_payments_summary($payments); + $totalRows = $this->sale->get_found_rows($search, $filters); + $payments = $this->sale->getPaymentsSummary($search, $filters); + $paymentSummary = getSalesManagePaymentsSummary($payments); - $data_rows = []; + $dataRows = []; foreach ($sales->getResult() as $sale) { - $data_rows[] = get_sale_data_row($sale); + $dataRows[] = getSaleDataRow($sale); } - if ($total_rows > 0) { - $data_rows[] = get_sale_data_last_row($sales); + if ($totalRows > 0) { + $dataRows[] = getSaleDataLastRow($sales); } - return $this->response->setJSON(['total' => $total_rows, 'rows' => $data_rows, 'payment_summary' => $payment_summary]); + return $this->response->setJSON(['total' => $totalRows, 'rows' => $dataRows, 'payment_summary' => $paymentSummary]); } /** diff --git a/app/Database/Seeds/TestDatabaseBootstrapSeeder.php b/app/Database/Seeds/TestDatabaseBootstrapSeeder.php index 335ba1021..a1e156541 100644 --- a/app/Database/Seeds/TestDatabaseBootstrapSeeder.php +++ b/app/Database/Seeds/TestDatabaseBootstrapSeeder.php @@ -7,7 +7,7 @@ use Config\Database; class TestDatabaseBootstrapSeeder extends Seeder { - public function run(): void + public static function reset(): void { if (ENVIRONMENT !== 'testing') { throw new \RuntimeException('TestDatabaseBootstrapSeeder can only run in the testing environment.'); @@ -34,4 +34,9 @@ class TestDatabaseBootstrapSeeder extends Seeder $serverConn->query("DROP DATABASE IF EXISTS `{$dbName}`"); $serverConn->query("CREATE DATABASE IF NOT EXISTS `{$dbName}`"); } + + public function run(): void + { + self::reset(); + } } diff --git a/app/Helpers/tabular_helper.php b/app/Helpers/tabular_helper.php index 3ed5144ca..129e6c921 100644 --- a/app/Helpers/tabular_helper.php +++ b/app/Helpers/tabular_helper.php @@ -61,7 +61,7 @@ function transform_headers(array $headers, bool $readonly = false, bool $editabl } -function sales_headers(): array +function salesHeaders(): array { return [ ['sale_id' => lang('Common.id')], @@ -79,7 +79,7 @@ function sales_headers(): array */ function get_sales_manage_table_headers(): string { - $headers = sales_headers(); + $headers = salesHeaders(); $config = config(OSPOS::class)->settings; if ($config['invoice_enable']) { @@ -95,7 +95,7 @@ function get_sales_manage_table_headers(): string /** * Get the html data row for the sales */ -function get_sale_data_row(object $sale): array +function getSaleDataRow(object $sale): array { $uri = current_url(true); $controller = $uri->getSegment(1); @@ -145,7 +145,7 @@ function get_sale_data_row(object $sale): array /** * Get the html data last row for the sales */ -function get_sale_data_last_row(ResultInterface $sales): array +function getSaleDataLastRow(ResultInterface $sales): array { $sum_amount_due = 0; $sum_amount_tendered = 0; @@ -169,7 +169,7 @@ function get_sale_data_last_row(ResultInterface $sales): array /** * Get the sales payments summary */ -function get_sales_manage_payments_summary(array $payments): string +function getSalesManagePaymentsSummary(array $payments): string { $table = '
'; $total = 0; diff --git a/app/Models/Sale.php b/app/Models/Sale.php index 1dbf2ba03..16229cb7f 100644 --- a/app/Models/Sale.php +++ b/app/Models/Sale.php @@ -210,7 +210,7 @@ class Sale extends Model /** * Get the payment summary for the takings (sales/manage) view */ - public function get_payments_summary(?string $search, array $filters): array + public function getPaymentsSummary(?string $search, array $filters): array { $config = config(OSPOS::class)->settings; diff --git a/tests/Controllers/SalesControllerTest.php b/tests/Controllers/SalesControllerTest.php index fe57887df..57fd939f0 100644 --- a/tests/Controllers/SalesControllerTest.php +++ b/tests/Controllers/SalesControllerTest.php @@ -6,8 +6,10 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; use CodeIgniter\Test\FeatureTestTrait; use CodeIgniter\Config\Services; +use App\Database\Seeds\TestDatabaseBootstrapSeeder; use App\Models\Employee; -use App\Models\Item; +use Config\OSPOS; +use Tests\Support\ItemFixtureTrait; /** * Regression tests for GHSA-3xf6-8fmq-44wg. @@ -21,12 +23,34 @@ class SalesControllerTest extends CIUnitTestCase { use DatabaseTestTrait; use FeatureTestTrait; + use ItemFixtureTrait; protected $migrate = true; protected $migrateOnce = true; + protected $seedOnce = true; protected $refresh = false; protected $namespace = null; + private static bool $doneBootstrap = false; + + protected function setUp(): void + { + if (self::$doneBootstrap === false) { + TestDatabaseBootstrapSeeder::reset(); + + self::$doneBootstrap = true; + } + + parent::setUp(); + + config(OSPOS::class)->update_settings(); + } + + protected function tearDown(): void + { + parent::tearDown(); + } + protected function createCashierEmployee(): int { $unique = uniqid(); @@ -35,7 +59,14 @@ class SalesControllerTest extends CIUnitTestCase 'first_name' => 'Cashier', 'last_name' => 'NoReports', 'email' => "cashier.$unique@test.com", - 'phone_number' => '555-0001' + 'phone_number' => '555-0001', + 'address_1' => '', + 'address_2' => '', + 'city' => '', + 'state' => '', + 'zip' => '', + 'country' => '', + 'comments' => '', ]; $employeeData = [ @@ -69,7 +100,14 @@ class SalesControllerTest extends CIUnitTestCase 'first_name' => 'Supervisor', 'last_name' => 'WithReports', 'email' => "supervisor.$unique@test.com", - 'phone_number' => '555-0002' + 'phone_number' => '555-0002', + 'address_1' => '', + 'address_2' => '', + 'city' => '', + 'state' => '', + 'zip' => '', + 'country' => '', + 'comments' => '', ]; $employeeData = [ @@ -220,30 +258,6 @@ class SalesControllerTest extends CIUnitTestCase return $saleId; } - protected function createTestItem(): int - { - $itemData = [ - 'item_id' => null, - 'name' => 'Test Item', - 'description' => 'Test Item', - 'category' => 'Test Category', - 'cost_price' => 1.00, - 'unit_price' => 5.00, - 'reorder_level' => 0, - 'item_number' => 'TEST-' . uniqid(), - 'allow_alt_description' => 0, - 'is_serialized' => 0, - 'stock_type' => HAS_NO_STOCK, - 'deleted' => 0, - ]; - - $itemModel = model(Item::class); - $itemModel->save_value($itemData); - $this->assertTrue($itemModel->save_value($itemData)); - - return (int) $itemData['item_id']; - } - /** * Seeds a single cart line directly in the session, mirroring the shape * Sale_lib::add_item() produces, so tests can target postEditItem's @@ -382,6 +396,35 @@ class SalesControllerTest extends CIUnitTestCase $this->assertSame(lang('Sales.not_authorized'), $result['message']); } + public function testCashierWithoutReportsSalesCannotGetSearch(): void + { + $cashierId = $this->createCashierEmployee(); + $this->createSale($cashierId); + $this->loginAs($cashierId); + + $response = $this->get('/sales/search'); + + $response->assertStatus(403); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + } + + public function testEmployeeWithReportsSalesCanGetSearch(): void + { + $supervisorId = $this->createReportsSalesEmployee(); + $this->createSale($supervisorId); + $this->loginAs($supervisorId); + + $response = $this->get('/sales/search'); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertArrayNotHasKey('success', $result); + $this->assertArrayHasKey('total', $result); + $this->assertArrayHasKey('rows', $result); + $this->assertArrayHasKey('payment_summary', $result); + } + public function testEmployeeWithReportsSalesCanGetRow(): void { $supervisorId = $this->createReportsSalesEmployee(); @@ -410,7 +453,7 @@ class SalesControllerTest extends CIUnitTestCase { $cashierId = $this->createCashierWithoutChangePriceGrant(); $this->loginAs($cashierId); - $itemId = $this->createTestItem(); + $itemId = $this->createTestItem(HAS_NO_STOCK); $this->seedCartLine(1, '5.00', $itemId); $response = $this->post('/sales/editItem/1', [ @@ -434,7 +477,7 @@ class SalesControllerTest extends CIUnitTestCase { $cashierId = $this->createCashierWithoutChangePriceGrant(); $this->loginAs($cashierId); - $itemId = $this->createTestItem(); + $itemId = $this->createTestItem(HAS_NO_STOCK); $this->seedCartLine(1, '5.00', $itemId); $response = $this->post('/sales/editItem/1', [ @@ -458,7 +501,7 @@ class SalesControllerTest extends CIUnitTestCase { $cashierId = $this->createCashierWithChangePriceGrant(); $this->loginAs($cashierId); - $itemId = $this->createTestItem(); + $itemId = $this->createTestItem(HAS_NO_STOCK); $this->seedCartLine(1, '5.00', $itemId); $response = $this->post('/sales/editItem/1', [