Add row-level authorization to password change endpoints (#4401)

* fix(security): add row-level authorization to password change endpoints

- Prevents non-admin users from viewing other users' password forms
- Prevents non-admin users from changing other users' passwords
- Uses can_modify_employee() check consistent with Employees controller fix
- Addresses BOLA vulnerability in Home controller (GHSA-q58g-gg7v-f9rf)

* test(security): add BOLA authorization tests for Home controller

- Test non-admin cannot view/change admin password
- Test user can view/change own password
- Test admin can view/change any password
- Test default employee_id uses current user
- Add JUnit test result upload to CI workflow

* refactor: apply PSR-12 naming and add DEFAULT_EMPLOYEE_ID constant

- Add DEFAULT_EMPLOYEE_ID constant to Constants.php
- Rename variables to follow PSR-12 camelCase convention
- Use ternary for default employee ID assignment

* refactor: use NEW_ENTRY constant instead of adding DEFAULT_EMPLOYEE_ID

Reuse existing NEW_ENTRY constant for default employee ID parameter.
Avoids adding redundant constants to Constants.php with same value (-1).

---------

Co-authored-by: jekkos <jeroen@steganos.dev>

.github/workflows/phpunit.yml

@@ -111,7 +111,15 @@ jobs:
         env:
           CI_ENVIRONMENT: testing
           MYSQL_HOST_NAME: 127.0.0.1
-        run: composer test
+        run: composer test -- --log-junit test-results/junit.xml
+
+      - name: Upload test results
+        uses: actions/upload-artifact@v4
+        if: always()
+        with:
+          name: test-results-php-${{ matrix.php-version }}
+          path: test-results/
+          retention-days: 30
 
       - name: Stop MariaDB
         if: always()

app/Controllers/Home.php

@@ -39,9 +39,19 @@ class Home extends Secure_Controller
      * @return string
      * @noinspection PhpUnused
      */
-    public function getChangePassword(int $employee_id = -1): string    // TODO: Replace -1 with a constant
+    public function getChangePassword(int $employeeId = NEW_ENTRY): string
     {
-        $person_info = $this->employee->get_info($employee_id);
+        $loggedInEmployee = $this->employee->get_logged_in_employee_info();
+        $currentPersonId = $loggedInEmployee->person_id;
+
+        $employeeId = $employeeId === NEW_ENTRY ? $currentPersonId : $employeeId;
+
+        if (!$this->employee->can_modify_employee($employeeId, $currentPersonId)) {
+            header('Location: ' . base_url('no_access/home/home'));
+            exit();
+        }
+
+        $person_info = $this->employee->get_info($employeeId);
         foreach (get_object_vars($person_info) as $property => $value) {
             $person_info->$property = $value;
         }
@@ -55,9 +65,20 @@ class Home extends Secure_Controller
      *
      * @return ResponseInterface
      */
-    public function postSave(int $employee_id = -1): ResponseInterface    // TODO: Replace -1 with a constant
+    public function postSave(int $employeeId = NEW_ENTRY): ResponseInterface
     {
-        if (!empty($this->request->getPost('current_password')) && $employee_id != -1) {
+        $currentUser = $this->employee->get_logged_in_employee_info();
+
+        $employeeId = $employeeId === NEW_ENTRY ? $currentUser->person_id : $employeeId;
+
+        if (!$this->employee->can_modify_employee($employeeId, $currentUser->person_id)) {
+            return $this->response->setStatusCode(403)->setJSON([
+                'success' => false,
+                'message' => lang('Employees.unauthorized_modify')
+            ]);
+        }
+
+        if (!empty($this->request->getPost('current_password')) && $employeeId != NEW_ENTRY) {
             if ($this->employee->check_password($this->request->getPost('username', FILTER_SANITIZE_FULL_SPECIAL_CHARS), $this->request->getPost('current_password'))) {
                 // Validate password length BEFORE hashing
                 $new_password = $this->request->getPost('password');
@@ -66,7 +87,7 @@ class Home extends Secure_Controller
                     return $this->response->setJSON([
                         'success' => false,
                         'message' => lang('Employees.password_minlength'),
-                        'id'      => -1
+                        'id'      => NEW_ENTRY
                     ]);
                 }
                 
@@ -76,32 +97,32 @@ class Home extends Secure_Controller
                     'hash_version' => 2
                 ];
 
-                if ($this->employee->change_password($employee_data, $employee_id)) {
+                if ($this->employee->change_password($employee_data, $employeeId)) {
                     return $this->response->setJSON([
                         'success' => true,
                         'message' => lang('Employees.successful_change_password'),
-                        'id'      => $employee_id
+                        'id'      => $employeeId
                     ]);
-                } else { // Failure    // TODO: Replace -1 with constant
+                } else {
                     return $this->response->setJSON([
                         'success' => false,
                         'message' => lang('Employees.unsuccessful_change_password'),
-                        'id'      => -1
+                        'id'      => NEW_ENTRY
                     ]);
                 }
-            } else {    // TODO: Replace -1 with constant
+            } else {
                 return $this->response->setJSON([
                     'success' => false,
                     'message' => lang('Employees.current_password_invalid'),
-                    'id'      => -1
+                    'id'      => NEW_ENTRY
                 ]);
             }
-        } else {    // TODO: Replace -1 with constant
+        } else {
             return $this->response->setJSON([
                 'success' => false,
                 'message' => lang('Employees.current_password_invalid'),
-                'id'      => -1
+                'id'      => NEW_ENTRY
             ]);
         }
     }
-}
+}

tests/Controllers/HomeTest.php

@@ -228,4 +228,199 @@ class HomeTest extends CIUnitTestCase
         $session->destroy();
         $session->set('person_id', 1); // Admin user
     }
+    
+    /**
+     * Create a non-admin employee for testing
+     * 
+     * @return int The person_id of the created employee
+     */
+    protected function createNonAdminEmployee(): int
+    {
+        $personData = [
+            'first_name'   => '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('');
+    }
+    
+    /**
+     * Login as a specific user
+     * 
+     * @param int $personId
+     * @return void
+     */
+    protected function loginAs(int $personId): void
+    {
+        $session = Services::session();
+        $session->destroy();
+        $session->set('person_id', $personId);
+        $session->set('menu_group', 'home');
+    }
+    
+    // ========== BOLA Authorization Tests ==========
+    
+    /**
+     * Test non-admin cannot view admin password change form
+     * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf
+     * 
+     * @return void
+     */
+    public function testNonAdminCannotViewAdminPasswordForm(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->loginAs($nonAdminId);
+        
+        $response = $this->get('/home/changePassword/1');
+        
+        $response->assertRedirect();
+        $this->assertStringContainsString('no_access', $response->getRedirectUrl());
+    }
+    
+    /**
+     * Test non-admin cannot change admin password
+     * BOLA vulnerability fix: GHSA-q58g-gg7v-f9rf
+     * 
+     * @return void
+     */
+    public function testNonAdminCannotChangeAdminPassword(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->loginAs($nonAdminId);
+        
+        $response = $this->post('/home/save/1', [
+            'username' => 'admin',
+            'current_password' => 'pointofsale',
+            'password' => 'hacked123'
+        ]);
+        
+        $response->assertStatus(403);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertFalse($result['success']);
+        
+        // Verify admin password was NOT changed
+        $employee = model(Employee::class);
+        $admin = $employee->get_info(1);
+        $this->assertTrue(password_verify('pointofsale', $admin->password), 
+            'Admin password should not have been changed by non-admin');
+    }
+    
+    /**
+     * Test user can view their own password change form
+     * 
+     * @return void
+     */
+    public function testUserCanViewOwnPasswordForm(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->loginAs($nonAdminId);
+        
+        $response = $this->get('/home/changePassword/' . $nonAdminId);
+        
+        $response->assertStatus(200);
+        $response->assertSee('nonadmin'); // Username should be visible
+    }
+    
+    /**
+     * Test user can change their own password
+     * 
+     * @return void
+     */
+    public function testUserCanChangeOwnPassword(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->loginAs($nonAdminId);
+        
+        $response = $this->post('/home/save/' . $nonAdminId, [
+            'username' => 'nonadmin',
+            'current_password' => 'password123',
+            'password' => 'newpassword123'
+        ]);
+        
+        $response->assertStatus(200);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertTrue($result['success']);
+        
+        // Verify password was changed
+        $employee = model(Employee::class);
+        $user = $employee->get_info($nonAdminId);
+        $this->assertTrue(password_verify('newpassword123', $user->password));
+    }
+    
+    /**
+     * Test admin can view any user's password form
+     * 
+     * @return void
+     */
+    public function testAdminCanViewAnyPasswordForm(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->resetSession(); // Login as admin
+        
+        $response = $this->get('/home/changePassword/' . $nonAdminId);
+        
+        $response->assertStatus(200);
+        $response->assertSee('nonadmin');
+    }
+    
+    /**
+     * Test admin can change any user's password
+     * 
+     * @return void
+     */
+    public function testAdminCanChangeAnyPassword(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->resetSession(); // Login as admin
+        
+        $response = $this->post('/home/save/' . $nonAdminId, [
+            'username' => 'nonadmin',
+            'current_password' => 'password123',
+            'password' => 'adminset123'
+        ]);
+        
+        $response->assertStatus(200);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertTrue($result['success']);
+        
+        // Verify password was changed
+        $employee = model(Employee::class);
+        $user = $employee->get_info($nonAdminId);
+        $this->assertTrue(password_verify('adminset123', $user->password));
+    }
+    
+    /**
+     * Test default employee_id parameter uses current user
+     * 
+     * @return void
+     */
+    public function testDefaultEmployeeIdUsesCurrentUser(): void
+    {
+        $nonAdminId = $this->createNonAdminEmployee();
+        $this->loginAs($nonAdminId);
+        
+        // Calling without employee_id should use current user
+        $response = $this->get('/home/changePassword');
+        
+        $response->assertStatus(200);
+        $response->assertSee('nonadmin');
+    }
 }