From 02b6df5e3b711d6cbc3351e7fae01329bbd17074 Mon Sep 17 00:00:00 2001 From: Jeppe Bundgaard Date: Mon, 29 Jun 2026 14:53:58 +0200 Subject: [PATCH] Refactor authentication handling and permission checks in self-serve routes --- ...artmentSelfserveVehicleConditionsRoute.php | 88 +++++++++++++------ .../nginx/app/routes/moduleSelfServeRoute.php | 40 +++++---- .../SelfserveCustomerLaneAccessTest.php | 7 +- .../Selfserve/SelfserveRouteWiringTest.php | 5 ++ 4 files changed, 95 insertions(+), 45 deletions(-) diff --git a/services/nginx/app/routes/departmentSelfserveVehicleConditionsRoute.php b/services/nginx/app/routes/departmentSelfserveVehicleConditionsRoute.php index 883d1a32..8797bcc9 100644 --- a/services/nginx/app/routes/departmentSelfserveVehicleConditionsRoute.php +++ b/services/nginx/app/routes/departmentSelfserveVehicleConditionsRoute.php @@ -9,6 +9,7 @@ use classes\authentication; use classes\response; use classes\selfserve; use modules\selfserve\classes\selfserve_wash_flow; +use modules\subusers\helpers\subusers_permission_node_key; use objects\customer_vehicles_o; use objects\department_lanes_o; use objects\department_selfserve_tasks_o; @@ -118,17 +119,19 @@ class departmentSelfserveVehicleConditionsRoute */ $this->get('/department/selfserve/vehicle/allowed', function () { global $response; - $user = (new authentication())->get_user(); - if (!$user) { - $response->error('Invalid session', 400); - } + [$user, $actor_id] = $this->getAuthenticatedSelfServePrincipal(); + $own_permission = self::definePermission('list_own_department_selfserve_vehicle_conditions', subusers_permission_node_key::SELFSERVE_LIST); - $has_global = $user->hasPermission('list_department_selfserve_vehicle_conditions'); - $has_own = $user->hasPermission('list_own_department_selfserve_vehicle_conditions'); + $has_global = $user !== null && $this->hasPermission('list_department_selfserve_vehicle_conditions'); + $has_own = $this->hasPermission($own_permission); if (!$has_global && !$has_own) { $response->forbidden(['list_department_selfserve_vehicle_conditions', 'list_own_department_selfserve_vehicle_conditions']); } + if ($user === null && !$has_own) { + $response->error('Invalid session', 400); + } + self::requireParameters(['lane_id', 'reg']); $lane_id = (int)self::getParameter('lane_id'); $reg = selfserve::standardize_registration((string)self::getParameter('reg')); @@ -147,7 +150,7 @@ class departmentSelfserveVehicleConditionsRoute ]); } - (new logs_o())->add('department_selfserve_vehicle_conditions', (int)$lane->department->value(), 1, $user->id, 'CHECK_VEHICLE_ALLOWED', 'User checked self-serve eligibility for lane ' . $lane_id . ' and vehicle ' . $reg); + (new logs_o())->add('department_selfserve_vehicle_conditions', (int)$lane->department->value(), 1, $actor_id, 'CHECK_VEHICLE_ALLOWED', 'User checked self-serve eligibility for lane ' . $lane_id . ' and vehicle ' . $reg); $response->success($flow->previewVehicleEligibility($lane_id, $reg, $customer_number, $vehicle_type_id)); }, [ 'list_department_selfserve_vehicle_conditions' => 'Check whether self-serve is allowed for a specific vehicle', @@ -159,17 +162,19 @@ class departmentSelfserveVehicleConditionsRoute */ $this->get('/department/selfserve/washes/summary', function () { global $response; - $user = (new authentication())->get_user(); - if (!$user) { - $response->error('Invalid session', 400); - } + [$user] = $this->getAuthenticatedSelfServePrincipal(); + $own_permission = self::definePermission('list_own_department_selfserve_vehicle_conditions', subusers_permission_node_key::SELFSERVE_LIST); - $has_global = $user->hasPermission('list_department_selfserve_vehicle_conditions'); - $has_own = $user->hasPermission('list_own_department_selfserve_vehicle_conditions'); + $has_global = $user !== null && $this->hasPermission('list_department_selfserve_vehicle_conditions'); + $has_own = $this->hasPermission($own_permission); if (!$has_global && !$has_own) { $response->forbidden(['list_department_selfserve_vehicle_conditions', 'list_own_department_selfserve_vehicle_conditions']); } + if ($user === null && !$has_own) { + $response->error('Invalid session', 400); + } + $flow = $this->getWashFlow(); $vehicle_type_id = $this->resolveVehicleTypeIdFromQuery(); try { @@ -236,18 +241,20 @@ class departmentSelfserveVehicleConditionsRoute */ $this->post('/department/selfserve/vehicle/conditions', function () { global $response; - $user = (new authentication())->get_user(); - if (!$user) { - $response->error('Invalid session', 400); - } + [$user, $actor_id] = $this->getAuthenticatedSelfServePrincipal(); + $own_permission = self::definePermission('add_own_department_selfserve_vehicle_conditions', subusers_permission_node_key::SELFSERVE_ADD); - $has_global = $user->hasPermission('add_department_selfserve_vehicle_conditions'); - $has_own = $user->hasPermission('add_own_department_selfserve_vehicle_conditions'); + $has_global = $user !== null && $this->hasPermission('add_department_selfserve_vehicle_conditions'); + $has_own = $this->hasPermission($own_permission); if (!$has_global && !$has_own) { $response->forbidden(['add_department_selfserve_vehicle_conditions', 'add_own_department_selfserve_vehicle_conditions']); } + if ($user === null && !$has_own) { + $response->error('Invalid session', 400); + } + $department = (int)$response->getRequestParameter('department'); $lane = (int)$response->getRequestParameter('lane'); $reg = selfserve::standardize_registration((string)$response->getRequestParameter('reg')); @@ -275,7 +282,7 @@ class departmentSelfserveVehicleConditionsRoute $condition_o = new department_selfserve_vehicle_conditions_o(); $condition_o->add($department, $lane, $reg, $question, $value, $customer_id); $summary = $this->getWashFlow()->synchronizeSession($lane, $reg, $customer_id, $activate_machine, $vehicle_type_id, $sync_relay_state); - (new logs_o())->add('department_selfserve_vehicle_conditions', 'global', 1, $user->id, 'ADD_VEHICLE_CONDITION', 'User added department self-serve vehicle condition ' . $condition_o->id); + (new logs_o())->add('department_selfserve_vehicle_conditions', 'global', 1, $actor_id, 'ADD_VEHICLE_CONDITION', 'User added department self-serve vehicle condition ' . $condition_o->id); $response->success([ 'condition' => $condition_o->asArray(), 'selfserve' => $summary, @@ -455,6 +462,24 @@ class departmentSelfserveVehicleConditionsRoute return new selfserve_wash_flow(); } + private function getAuthenticatedSelfServePrincipal(): array + { + global $response; + + $auth = new authentication(); + $user = $auth->get_user(); + if ($user !== false) { + return [$user, (int)$user->id]; + } + + $subuser = $auth->get_subuser(); + if ($subuser !== false) { + return [null, (int)$subuser->id]; + } + + $response->error('Invalid session', 400); + } + private function resolveVehicleTypeIdFromQuery(): ?int { $rawVehicleType = null; @@ -549,7 +574,7 @@ class departmentSelfserveVehicleConditionsRoute } private function assertLaneAccess( - object $user, + ?object $user, int $laneId, bool $hasGlobalPermission = true, bool $hasOwnPermission = false, @@ -563,7 +588,7 @@ class departmentSelfserveVehicleConditionsRoute $response->error('Department lane not found', 404); } - if ($hasGlobalPermission && $this->userHasLaneDepartmentAccess($user, $lane)) { + if ($hasGlobalPermission && $user !== null && $this->userHasLaneDepartmentAccess($user, $lane)) { return $lane; } @@ -604,16 +629,17 @@ class departmentSelfserveVehicleConditionsRoute } } - private function summaryBelongsToCustomer(object $user, array $summary): bool + private function summaryBelongsToCustomer(?object $user, array $summary): bool { + $customer_number = $this->requireAuthenticatedCustomerNumber($user, 'list_department_selfserve_vehicle_conditions'); $session_customer_number = $summary['session']['customer_number'] ?? null; - if ($session_customer_number !== null && (int)$session_customer_number === (int)$user->customer_number->value()) { + if ($session_customer_number !== null && (int)$session_customer_number === $customer_number) { return true; } $reg = (string)($summary['session']['reg'] ?? ''); $vehicle_o = (new customer_vehicles_o())->selectByPlate($reg); - return $vehicle_o->exists() && (int)$vehicle_o->customer_id->value() === (int)$user->customer_number->value(); + return $vehicle_o->exists() && (int)$vehicle_o->customer_id->value() === $customer_number; } private function summaryDepartmentId(array $summary): int @@ -636,11 +662,15 @@ class departmentSelfserveVehicleConditionsRoute return in_array($lane_department, $authorized_department_ids, true); } - private function requireAuthenticatedCustomerNumber(object $user, string $elevatedPermission): int + private function requireAuthenticatedCustomerNumber(?object $user, string $elevatedPermission): int { global $response; - $customer_number = (int)$user->customer_number->value(); + $customer_number = $this->resolveEffectiveCustomerNumber(); + if ($customer_number === null && $user !== null && isset($user->customer_number)) { + $customer_number = (int)$user->customer_number->value(); + } + $customer_number = (int)$customer_number; if ($customer_number <= 0) { $response->forbidden([$elevatedPermission]); } @@ -649,7 +679,7 @@ class departmentSelfserveVehicleConditionsRoute } private function assertSummaryAccess( - object $user, + ?object $user, array $summary, bool $hasGlobalPermission, bool $hasOwnPermission, @@ -658,7 +688,7 @@ class departmentSelfserveVehicleConditionsRoute { global $response; - if ($hasGlobalPermission && $this->userHasSummaryDepartmentAccess($user, $summary)) { + if ($hasGlobalPermission && $user !== null && $this->userHasSummaryDepartmentAccess($user, $summary)) { return; } diff --git a/services/nginx/app/routes/moduleSelfServeRoute.php b/services/nginx/app/routes/moduleSelfServeRoute.php index 4560e989..e6ebb8e4 100644 --- a/services/nginx/app/routes/moduleSelfServeRoute.php +++ b/services/nginx/app/routes/moduleSelfServeRoute.php @@ -17,6 +17,7 @@ use modules\selfserve\helpers\selfserve_lane_relay; use modules\selfserve\helpers\selfserve_lane_state; use modules\selfserve\helpers\selfserve_lane_status; use modules\selfserve\helpers\selfserve_wash_session_status; +use modules\subusers\helpers\subusers_permission_node_key; use objects\department_lanes_o; use objects\departments_o; use objects\logs_o; @@ -1330,8 +1331,7 @@ class moduleSelfServeRoute { global $response; - $user = (new authentication())->get_user(); - if (!$user) { + if (!$this->hasAuthenticatedUserOrSubuser()) { $response->error('Authentication failed. Invalid or missing token.', 401); } @@ -1344,7 +1344,7 @@ class moduleSelfServeRoute return null; } - if (self::hasPermission('list_own_department_selfserve_vehicle_conditions')) { + if (self::hasPermission($this->customerSelfServePermission())) { $customer_number = $this->resolveEffectiveCustomerNumber(); if ($customer_number !== null && $customer_number > 0) { return (int)$customer_number; @@ -1353,7 +1353,7 @@ class moduleSelfServeRoute $this->emitForbidden([ 'modules_selfserve_lane_wash_in_progress_view', - 'list_own_department_selfserve_vehicle_conditions', + $this->customerSelfServePermission(), ]); return null; } @@ -1412,13 +1412,12 @@ class moduleSelfServeRoute { global $response; - $user = (new authentication())->get_user(); - if (!$user) { + if (!$this->hasAuthenticatedUserOrSubuser()) { $response->error('Authentication failed. Invalid or missing token.', 401); } - if (!self::hasPermission(self::CUSTOMER_SELFSERVE_PERMISSION)) { - $this->emitForbidden([self::CUSTOMER_SELFSERVE_PERMISSION]); + if (!self::hasPermission($this->customerSelfServePermission())) { + $this->emitForbidden([$this->customerSelfServePermission()]); } $customer_number = $this->resolveEffectiveCustomerNumber(); @@ -1429,6 +1428,17 @@ class moduleSelfServeRoute return (int)$customer_number; } + private function customerSelfServePermission(): \classes\permission_node + { + return self::definePermission(self::CUSTOMER_SELFSERVE_PERMISSION, subusers_permission_node_key::SELFSERVE_LIST); + } + + private function hasAuthenticatedUserOrSubuser(): bool + { + $auth = new authentication(); + return $auth->get_user() !== false || $auth->get_subuser() !== false; + } + private function findLatestActiveSelfServeSessionForCustomer(int $customer_number): selfserve_wash_sessions_o { if ($customer_number <= 0) { @@ -1685,7 +1695,7 @@ class moduleSelfServeRoute $missing_permissions = $department_id > 0 ? ['department_access_' . $department_id] : []; if ($allow_customer_self_serve) { - $missing_permissions[] = self::CUSTOMER_SELFSERVE_PERMISSION; + $missing_permissions[] = $this->customerSelfServePermission(); } $this->emitForbidden($missing_permissions); } @@ -1729,7 +1739,7 @@ class moduleSelfServeRoute return; } - $this->emitForbidden([...$elevated_permissions, self::CUSTOMER_SELFSERVE_PERMISSION]); + $this->emitForbidden([...$elevated_permissions, $this->customerSelfServePermission()]); } private function requireSelfServeLaneCommandPermission( @@ -1760,7 +1770,7 @@ class moduleSelfServeRoute $this->emitForbidden( $allow_customer_self_serve - ? [...$elevated_permissions, self::CUSTOMER_SELFSERVE_PERMISSION] + ? [...$elevated_permissions, $this->customerSelfServePermission()] : $elevated_permissions ); } @@ -1791,7 +1801,7 @@ class moduleSelfServeRoute return; } - $this->emitForbidden([...$elevated_permissions, self::CUSTOMER_SELFSERVE_PERMISSION]); + $this->emitForbidden([...$elevated_permissions, $this->customerSelfServePermission()]); } protected function isSelfServeModuleEnabled(): bool @@ -1806,13 +1816,13 @@ class moduleSelfServeRoute protected function canCustomerUseSelfServeLane(selfserve_lane $lane, int $customer_number): bool { return $customer_number > 0 - && $this->hasPermission(self::CUSTOMER_SELFSERVE_PERMISSION) + && $this->hasPermission($this->customerSelfServePermission()) && $this->isLaneSelfServeOperationallyEnabled($lane); } protected function canCustomerUseActiveSelfServeLane(selfserve_lane $lane, int $customer_number): bool { - if ($customer_number <= 0 || !$this->hasPermission(self::CUSTOMER_SELFSERVE_PERMISSION)) { + if ($customer_number <= 0 || !$this->hasPermission($this->customerSelfServePermission())) { return false; } @@ -1859,7 +1869,7 @@ class moduleSelfServeRoute protected function canCustomerUseActiveSelfServeLaneSession(selfserve_lane $lane, int $customer_number): bool { - if ($customer_number <= 0 || !$this->hasPermission(self::CUSTOMER_SELFSERVE_PERMISSION)) { + if ($customer_number <= 0 || !$this->hasPermission($this->customerSelfServePermission())) { return false; } diff --git a/services/nginx/app/tests/Unit/Selfserve/SelfserveCustomerLaneAccessTest.php b/services/nginx/app/tests/Unit/Selfserve/SelfserveCustomerLaneAccessTest.php index 29f44aec..084de273 100644 --- a/services/nginx/app/tests/Unit/Selfserve/SelfserveCustomerLaneAccessTest.php +++ b/services/nginx/app/tests/Unit/Selfserve/SelfserveCustomerLaneAccessTest.php @@ -57,7 +57,12 @@ class SelfserveCustomerLaneAccessRouteHarness extends moduleSelfServeRoute protected function emitForbidden(array $permissions): void { - $this->forbiddenPermissions = array_values($permissions); + $this->forbiddenPermissions = array_values(array_map( + static fn(string|\classes\permission_node $permission): string => $permission instanceof \classes\permission_node + ? $permission->permission + : $permission, + $permissions + )); throw new RuntimeException('forbidden'); } diff --git a/services/nginx/app/tests/Unit/Selfserve/SelfserveRouteWiringTest.php b/services/nginx/app/tests/Unit/Selfserve/SelfserveRouteWiringTest.php index 623110be..534a9011 100644 --- a/services/nginx/app/tests/Unit/Selfserve/SelfserveRouteWiringTest.php +++ b/services/nginx/app/tests/Unit/Selfserve/SelfserveRouteWiringTest.php @@ -12,6 +12,10 @@ it('wires self-serve machine types, eligibility, summaries, and machine-start we expect($vehicleConditionsRoute)->toContain('/department/selfserve/vehicle/allowed'); expect($vehicleConditionsRoute)->toContain('/department/selfserve/washes/summary'); expect($vehicleConditionsRoute)->toContain('synchronizeSession'); + expect($vehicleConditionsRoute)->toContain("definePermission('list_own_department_selfserve_vehicle_conditions', subusers_permission_node_key::SELFSERVE_LIST)"); + expect($vehicleConditionsRoute)->toContain("definePermission('add_own_department_selfserve_vehicle_conditions', subusers_permission_node_key::SELFSERVE_ADD)"); + expect($vehicleConditionsRoute)->toContain('getAuthenticatedSelfServePrincipal'); + expect($vehicleConditionsRoute)->toContain('resolveEffectiveCustomerNumber()'); expect($webhookRoute)->not->toBeFalse(); expect($webhookRoute)->toContain('/relay/button/press/post'); @@ -189,6 +193,7 @@ it('wires machine relay status get and set endpoints', function (): void { expect($moduleSelfServeRoute)->not->toBeFalse(); expect($relayController)->not->toBeFalse(); + expect($moduleSelfServeRoute)->toContain("definePermission(self::CUSTOMER_SELFSERVE_PERMISSION, subusers_permission_node_key::SELFSERVE_LIST)"); expect($moduleSelfServeRoute)->toContain('/modules/self-serve/lane/relay/machine/status'); expect($moduleSelfServeRoute)->toContain('/modules/self-serve/lane/relay/machine/set'); expect($moduleSelfServeRoute)->toContain('/modules/self-serve/lane/relay/machine_program_picker/status');