From 19eb43270a9a8e9a726c0c8d0ce2b1fce254a1db Mon Sep 17 00:00:00 2001 From: jekkos Date: Thu, 5 Mar 2026 19:46:39 +0100 Subject: [PATCH] Fix broken object-level authorization in Employees controller (CVE-worthy) (#4391) - Non-admin employees can no longer view/modify admin accounts - Non-admin employees can no longer delete admin accounts - Non-admin employees can only grant permissions they themselves have - Added is_admin() and can_modify_employee() methods to Employee model - Prevents privilege escalation via permission grants Add tests for BOLA fix and permission delegation - EmployeeTest: Unit tests for is_admin() and can_modify_employee() methods - EmployeesControllerTest: Test cases for authorization checks (integration tests require DB) - ReportsControllerTest: Test validating the constructor redirect fix pattern Fix return type error in Employees controller Use $this->response->setJSON() instead of echo json_encode() + return to properly satisfy the ResponseInterface return type. --- app/Controllers/Employees.php | 34 +++ app/Language/en/Employees.php | 2 + app/Models/Employee.php | 39 ++++ tests/Controllers/EmployeesControllerTest.php | 219 ++++++++++++++++++ tests/Controllers/ReportsControllerTest.php | 57 +++++ tests/Models/EmployeeTest.php | 169 ++++++++++++++ 6 files changed, 520 insertions(+) create mode 100644 tests/Controllers/EmployeesControllerTest.php create mode 100644 tests/Controllers/ReportsControllerTest.php create mode 100644 tests/Models/EmployeeTest.php diff --git a/app/Controllers/Employees.php b/app/Controllers/Employees.php index 0cd7d571d..18ee6283c 100644 --- a/app/Controllers/Employees.php +++ b/app/Controllers/Employees.php @@ -76,6 +76,13 @@ class Employees extends Persons public function getView(int $employee_id = NEW_ENTRY): string { $person_info = $this->employee->get_info($employee_id); + $current_user = $this->employee->get_logged_in_employee_info(); + + if ($employee_id != NEW_ENTRY && !$this->employee->can_modify_employee($person_info->person_id, $current_user->person_id)) { + header('Location: ' . base_url('no_access/employees/employees')); + exit(); + } + foreach (get_object_vars($person_info) as $property => $value) { $person_info->$property = $value; } @@ -109,6 +116,19 @@ class Employees extends Persons */ public function postSave(int $employee_id = NEW_ENTRY): ResponseInterface { + $current_user = $this->employee->get_logged_in_employee_info(); + + if ($employee_id != NEW_ENTRY) { + $target_employee = $this->employee->get_info($employee_id); + if (!$this->employee->can_modify_employee($target_employee->person_id, $current_user->person_id)) { + return $this->response->setJSON([ + 'success' => false, + 'message' => lang('Employees.error_updating_admin'), + 'id' => NEW_ENTRY + ]); + } + } + $first_name = $this->request->getPost('first_name', FILTER_SANITIZE_FULL_SPECIAL_CHARS); // TODO: duplicated code $last_name = $this->request->getPost('last_name', FILTER_SANITIZE_FULL_SPECIAL_CHARS); $email = strtolower($this->request->getPost('email', FILTER_SANITIZE_EMAIL)); @@ -133,11 +153,16 @@ class Employees extends Persons ]; $grants_array = []; + $is_admin = $this->employee->is_admin($current_user->person_id); + foreach ($this->module->get_all_permissions()->getResult() as $permission) { $grants = []; $grant = $this->request->getPost('grant_' . $permission->permission_id) != null ? $this->request->getPost('grant_' . $permission->permission_id, FILTER_SANITIZE_FULL_SPECIAL_CHARS) : ''; if ($grant == $permission->permission_id) { + if (!$is_admin && !$this->employee->has_grant($permission->permission_id, $current_user->person_id)) { + continue; + } $grants['permission_id'] = $permission->permission_id; $grants['menu_group'] = $this->request->getPost('menu_group_' . $permission->permission_id) != null ? $this->request->getPost('menu_group_' . $permission->permission_id, FILTER_SANITIZE_FULL_SPECIAL_CHARS) : '--'; $grants_array[] = $grants; @@ -199,6 +224,15 @@ class Employees extends Persons public function postDelete(): ResponseInterface { $employees_to_delete = $this->request->getPost('ids', FILTER_SANITIZE_FULL_SPECIAL_CHARS); + $current_user = $this->employee->get_logged_in_employee_info(); + + if (!$this->employee->is_admin($current_user->person_id)) { + foreach ($employees_to_delete as $emp_id) { + if ($this->employee->is_admin((int)$emp_id)) { + return $this->response->setJSON(['success' => false, 'message' => lang('Employees.error_deleting_admin')]); + } + } + } if ($this->employee->delete_list($employees_to_delete)) { // TODO: this is passing a string, but delete_list expects an array return $this->response->setJSON([ diff --git a/app/Language/en/Employees.php b/app/Language/en/Employees.php index a4f865376..b4d071172 100644 --- a/app/Language/en/Employees.php +++ b/app/Language/en/Employees.php @@ -14,7 +14,9 @@ return [ "current_password_invalid" => "Current Password is invalid.", "employee" => "Employee", "error_adding_updating" => "Employee add or update failed.", + "error_deleting_admin" => "You cannot delete an admin user.", "error_deleting_demo_admin" => "You can not delete the demo admin user.", + "error_updating_admin" => "You cannot modify an admin user.", "error_updating_demo_admin" => "You can not change the demo admin user.", "language" => "Language", "login_info" => "Login", diff --git a/app/Models/Employee.php b/app/Models/Employee.php index 5149be1da..56b77164e 100644 --- a/app/Models/Employee.php +++ b/app/Models/Employee.php @@ -534,4 +534,43 @@ class Employee extends Person return $success; } + + /** + * Checks if the employee has admin privileges (all module permissions). + * The first employee (person_id = 1) is considered admin by default. + */ + public function is_admin(int $person_id): bool + { + if ($person_id === 1) { + return true; + } + + $modules = ['customers', 'employees', 'giftcards', 'items', 'item_kits', 'messages', 'receivings', 'reports', 'sales', 'config', 'suppliers']; + + foreach ($modules as $module) { + if (!$this->has_grant($module, $person_id)) { + return false; + } + } + + return true; + } + + /** + * Checks if current user can modify target employee. + * Only admins can modify other admin accounts. + * Users cannot modify their own grants unless they are admin. + */ + public function can_modify_employee(int $target_person_id, int $current_person_id): bool + { + if ($target_person_id === $current_person_id) { + return !$this->is_admin($target_person_id) || $this->is_admin($current_person_id); + } + + if ($this->is_admin($target_person_id) && !$this->is_admin($current_person_id)) { + return false; + } + + return true; + } } diff --git a/tests/Controllers/EmployeesControllerTest.php b/tests/Controllers/EmployeesControllerTest.php new file mode 100644 index 000000000..a0b63cd30 --- /dev/null +++ b/tests/Controllers/EmployeesControllerTest.php @@ -0,0 +1,219 @@ + 'NonAdmin', + 'last_name' => 'User', + 'email' => 'nonadmin@test.com', + 'phone_number' => '555-1234' + ]; + + $employeeData = [ + 'username' => 'nonadmin', + 'password' => password_hash('password123', PASSWORD_DEFAULT), + 'hash_version' => 2, + 'language_code' => 'en', + 'language' => 'english' + ]; + + $grantsData = [ + ['permission_id' => 'customers', 'menu_group' => 'home'], + ['permission_id' => 'sales', 'menu_group' => 'home'] + ]; + + $employeeModel = model(Employee::class); + $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY); + + return $employeeModel->get_found_rows(''); + } + + protected function loginAsAdmin(): void + { + $session = Services::session(); + $session->destroy(); + $session->set('person_id', 1); + $session->set('menu_group', 'office'); + } + + protected function loginAsNonAdmin(int $personId): void + { + $session = Services::session(); + $session->destroy(); + $session->set('person_id', $personId); + $session->set('menu_group', 'home'); + } + + public function testNonAdminCannotViewAdminAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsNonAdmin($nonAdminId); + + $response = $this->get('/employees/view/1'); + + $response->assertRedirect(); + $this->assertStringContainsString('no_access', $response->getRedirectUrl()); + } + + public function testNonAdminCannotModifyAdminAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsNonAdmin($nonAdminId); + + $response = $this->post('/employees/save/1', [ + 'first_name' => 'Hacked', + 'last_name' => 'Admin', + 'email' => 'hacked@evil.com', + 'username' => 'admin' + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + $this->assertStringContainsString('admin', strtolower($result['message'])); + } + + public function testNonAdminCannotDeleteAdminAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsNonAdmin($nonAdminId); + + $response = $this->post('/employees/delete', [ + 'ids' => [1] + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + $this->assertStringContainsString('admin', strtolower($result['message'])); + } + + public function testNonAdminCannotGrantPermissionsTheyDontHave(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsNonAdmin($nonAdminId); + + $targetEmployeeId = $nonAdminId + rand(1000, 9999); + $this->createTestEmployee($targetEmployeeId); + + $response = $this->post('/employees/save/' . $targetEmployeeId, [ + 'first_name' => 'Test', + 'last_name' => 'Employee', + 'email' => 'test@test.com', + 'username' => 'testuser', + 'grant_employees' => 'employees', + 'grant_config' => 'config' + ]); + + $employeeModel = model(Employee::class); + $hasEmployeesGrant = $employeeModel->has_grant('employees', $targetEmployeeId); + $hasConfigGrant = $employeeModel->has_grant('config', $targetEmployeeId); + + $this->assertFalse($hasEmployeesGrant); + $this->assertFalse($hasConfigGrant); + } + + public function testAdminCanModifyAnyAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsAdmin(); + + $response = $this->post('/employees/save/' . $nonAdminId, [ + 'first_name' => 'Modified', + 'last_name' => 'User', + 'email' => 'modified@test.com', + 'username' => 'nonadmin' + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + public function testAdminCanDeleteAnyAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsAdmin(); + + $response = $this->post('/employees/delete', [ + 'ids' => [$nonAdminId] + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + public function testUserCanModifyOwnAccount(): void + { + $nonAdminId = $this->createNonAdminEmployee(); + $this->loginAsNonAdmin($nonAdminId); + + $response = $this->post('/employees/save/' . $nonAdminId, [ + 'first_name' => 'Modified', + 'last_name' => 'OwnAccount', + 'email' => 'own@test.com', + 'username' => 'nonadmin' + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + public function testPermissionDelegationRule(): void + { + $permissionsRequested = ['customers', 'employees', 'sales', 'config']; + $userPermissions = ['customers', 'sales']; + $isAdmin = false; + + $granted = []; + foreach ($permissionsRequested as $perm) { + if ($isAdmin || in_array($perm, $userPermissions)) { + $granted[] = $perm; + } + } + + $this->assertEquals(['customers', 'sales'], $granted); + } + + public function testAdminCanGrantAnyPermission(): void + { + $permissionsRequested = ['customers', 'employees', 'sales', 'config']; + $userPermissions = ['customers', 'sales']; + $isAdmin = true; + + $granted = []; + foreach ($permissionsRequested as $perm) { + if ($isAdmin || in_array($perm, $userPermissions)) { + $granted[] = $perm; + } + } + + $this->assertEquals($permissionsRequested, $granted); + } +} \ No newline at end of file diff --git a/tests/Controllers/ReportsControllerTest.php b/tests/Controllers/ReportsControllerTest.php new file mode 100644 index 000000000..584b81b5e --- /dev/null +++ b/tests/Controllers/ReportsControllerTest.php @@ -0,0 +1,57 @@ +assertStringContainsString("header('Location: ' . base_url(", $constructorCode); + $this->assertStringContainsString('exit();', $constructorCode); + + // Verify the buggy pattern is NOT present in the permission check area + // (Note: redirect() may appear elsewhere in the codebase for valid uses) + $lines = explode("\n", $constructorCode); + $inConstructor = false; + foreach ($lines as $line) { + if (strpos($line, 'public function __construct') !== false) { + $inConstructor = true; + } + if ($inConstructor && strpos($line, '}') !== false && trim($line) === '}') { + break; + } + if ($inConstructor && strpos($line, "redirect('no_access") !== false) { + $this->fail('Old redirect() pattern found in constructor - should use header() + exit()'); + } + } + + $this->assertTrue(true, 'Permission check pattern validated'); + } + + public function testSubmodulePermissionCheckOccursBeforeControllerInitialization(): void + { + // Verify that permission checks happen in the constructor + // before any controller methods can execute + + $constructorCode = file_get_contents(APPPATH . 'Controllers/Reports.php'); + + // Verify the permission check is in the constructor + $this->assertStringContainsString('has_grant', $constructorCode); + $this->assertStringContainsString('reports_', $constructorCode); + $this->assertStringContainsString('submodule_id', $constructorCode); + } +} \ No newline at end of file diff --git a/tests/Models/EmployeeTest.php b/tests/Models/EmployeeTest.php new file mode 100644 index 000000000..e3e81f6be --- /dev/null +++ b/tests/Models/EmployeeTest.php @@ -0,0 +1,169 @@ +is_admin(1); + + $this->assertTrue($result); + } + + public function testIsAdminReturnsTrueForEmployeeWithAllPermissions(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['has_grant']) + ->getMock(); + + $employeeModel->method('has_grant') + ->willReturn(true); + + $result = $employeeModel->is_admin(2); + + $this->assertTrue($result); + } + + public function testIsAdminReturnsFalseWhenMissingPermissions(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['has_grant']) + ->getMock(); + + $employeeModel->method('has_grant') + ->willReturnCallback(function($permissionId, $personId) { + return $permissionId !== 'config'; + }); + + $result = $employeeModel->is_admin(3); + + $this->assertFalse($result); + } + + public function testCanModifyEmployeeReturnsTrueForOwnAccount(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturn(false); + + $result = $employeeModel->can_modify_employee(1, 1); + + $this->assertTrue($result); + } + + public function testCanModifyEmployeeReturnsTrueForOwnAdminAccount(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturn(true); + + $result = $employeeModel->can_modify_employee(1, 1); + + $this->assertTrue($result); + } + + public function testCanModifyEmployeeReturnsFalseWhenNonAdminModifiesAdmin(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturnCallback(function($personId) { + return $personId === 1; + }); + + $result = $employeeModel->can_modify_employee(1, 2); + + $this->assertFalse($result); + } + + public function testCanModifyEmployeeReturnsTrueWhenAdminModifiesNonAdmin(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturnCallback(function($personId) { + return $personId === 1; + }); + + $result = $employeeModel->can_modify_employee(2, 1); + + $this->assertTrue($result); + } + + public function testCanModifyEmployeeReturnsTrueWhenNonAdminModifiesNonAdmin(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturn(false); + + $result = $employeeModel->can_modify_employee(2, 3); + + $this->assertTrue($result); + } + + public function testCanModifyEmployeeReturnsFalseForNonAdminEditingAdmin(): void + { + $employeeModel = $this->getMockBuilder(Employee::class) + ->onlyMethods(['is_admin']) + ->getMock(); + + $employeeModel->method('is_admin') + ->willReturnCallback(function($personId) { + return $personId === 1; + }); + + $result = $employeeModel->can_modify_employee(1, 2); + + $this->assertFalse($result); + } + + public function testHasGrantReturnsTrueForActualGrant(): void + { + $employeeModel = model(Employee::class); + + $result = $employeeModel->has_grant('employees', 1); + + $this->assertTrue($result); + } + + public function testHasGrantReturnsFalseForMissingGrant(): void + { + $employeeModel = model(Employee::class); + + $result = $employeeModel->has_grant('nonexistent_permission', 1); + + $this->assertFalse($result); + } +} \ No newline at end of file