From 0281984196aacbf5bfb8879cc0cf87f1bd9b23f4 Mon Sep 17 00:00:00 2001 From: Imugiii Date: Thu, 25 Jun 2026 08:42:36 +0000 Subject: [PATCH] feat(security): garde visibilite source/role sur DELIVER_ORDER (PRE-3) + audit et re-verification du PIN self-service --- src/app/Controllers/OrderAdminController.php | 57 ++++++++++++- src/app/Controllers/ProfileController.php | 68 ++++++++++++++++ src/app/Views/admin/profile/pin.php | 6 ++ tests/Support/FakeDatabase.php | 31 +++++++ tests/Unit/Admin/OrderAdminControllerTest.php | 75 +++++++++++++++++ tests/Unit/Admin/ProfileControllerTest.php | 80 ++++++++++++++++++- 6 files changed, 313 insertions(+), 4 deletions(-) diff --git a/src/app/Controllers/OrderAdminController.php b/src/app/Controllers/OrderAdminController.php index dd348d7..d4b25f6 100644 --- a/src/app/Controllers/OrderAdminController.php +++ b/src/app/Controllers/OrderAdminController.php @@ -53,6 +53,15 @@ class OrderAdminController extends AdminController * Remise au client : paid -> delivered (mlt 6.1). POST + CSRF, garde order.deliver. * Pas de PIN (geste routinier). Issue affichee en flash, retour a la liste. * + * PRE-3 / ERR-2 (6.1) : au-dela de la permission, la SOURCE de la commande doit + * etre VISIBLE par le role de l'acteur (role_visible_source). Sans ce controle, + * tout role detenant order.deliver pourrait remettre une commande de n'importe + * quel canal (ex. un equipier drive remettant une commande comptoir). La file + * KDS (KitchenController) est deja filtree par visibleSources a l'affichage ; + * cette garde rejoue la regle cote ecriture (defense en profondeur, meme posture + * que cancel() qui ne se fie pas a l'affichage de la liste). Hors des sources + * visibles -> 403 FORBIDDEN (memes statut/vue que la garde de permission). + * * @param array $params */ public function deliver(array $params = []): Response @@ -67,8 +76,22 @@ class OrderAdminController extends AdminController return $this->invalidCsrf(); } + $number = (string) ($params['number'] ?? ''); + + // PRE-3 (6.1) : refuse la remise d'une commande dont la source n'est pas dans + // les sources visibles du role agissant. visibleSources reutilise + // role_visible_source (memes donnees que KitchenController) : liste vide en base + // = vue globale (admin/manager voient les trois sources). Le numero inconnu + // (source null) retombe sur le chemin "non visible" -> 403 ; la branche + // ORDER_NOT_FOUND ci-dessous reste atteignable pour une course (commande + // supprimee entre la lecture et la transition). + $source = $this->orderSource($number); + if ($source === null || !in_array($source, $this->orderQuery()->visibleSources($guard->roleId ?? 0), true)) { + return $this->forbidden($guard); + } + try { - $this->orders()->deliver((string) ($params['number'] ?? '')); + $this->orders()->deliver($number); $this->setFlash('Commande remise (livree).'); } catch (OrderValidationException $exception) { $this->setFlash( @@ -178,6 +201,28 @@ class OrderAdminController extends AdminController return new OrderQueryRepository($this->db()); } + /** + * Source (canal) d'une commande par son numero, pour la garde de visibilite + * PRE-3 (6.1). Lecture ciblee d'une seule colonne : OrderRepository::findByNumber + * renvoie une forme typee {id, order_number, total_ttc_cents, status} qui n'expose + * pas la source ; plutot que d'elargir ce contrat partage, on lit ici le seul champ + * requis (meme couture db() que logFailedPin). Renvoie null si le numero est + * inconnu (traite comme non visible par l'appelant). + */ + protected function orderSource(string $number): ?string + { + if ($number === '') { + return null; + } + $row = $this->db()->fetch( + 'SELECT source FROM customer_order WHERE order_number = :n', + ['n' => $number], + ); + $source = is_string($row['source'] ?? null) ? (string) $row['source'] : ''; + + return $source === '' ? null : $source; + } + protected function orders(): OrderRepository { $db = $this->db(); @@ -247,6 +292,16 @@ class OrderAdminController extends AdminController return $this->adminView('admin/not_found', ['title' => 'Introuvable', 'activeNav' => 'orders'], $guard, 404); } + /** + * Refus de visibilite (ERR-2, 6.1) : la source de la commande n'est pas visible + * par le role agissant. Memes statut (403) et vue (admin/forbidden) que la garde + * de permission d'AdminController::guard, pour une convention de refus homogene. + */ + private function forbidden(GuardResult $guard): Response + { + return $this->adminView('admin/forbidden', ['title' => 'Acces refuse', 'activeNav' => 'orders'], $guard, 403); + } + private function redirect(string $location): Response { return Response::make('', 302, ['Location' => $location]); diff --git a/src/app/Controllers/ProfileController.php b/src/app/Controllers/ProfileController.php index 2b88ea8..210b309 100644 --- a/src/app/Controllers/ProfileController.php +++ b/src/app/Controllers/ProfileController.php @@ -17,6 +17,14 @@ use App\Core\Response; * actions sensibles, RG-T13). Accessible a tout utilisateur authentifie ; aucune * permission specifique (on n'agit que sur son propre compte = session userId). * + * Le PIN est un credential sensible : le (re)definir exige le mot de passe COURANT + * (re-verification d'identite sur poste a session partagee, meme posture que la + * verification PIN d'ADR-0004) ET ecrit une ligne `audit_log` (ADR-0004, RG-T14). + * Le SET du PIN n'est PAS throttle : la surface de brute-force est la VERIFICATION + * du PIN (couverte par pin_throttle / RG-T22, ADR-0005), pas sa definition par un + * utilisateur deja authentifie. L'audit ne porte que l'evenement (set vs change), + * jamais le PIN ni un hash. + * * Non `final` : les tests sous-classent pour injecter des doubles. */ class ProfileController extends AdminController @@ -66,6 +74,7 @@ class ProfileController extends AdminController $pin = $form['pin'] ?? ''; $confirm = $form['pin_confirm'] ?? ''; + $currentPassword = $form['current_password'] ?? ''; $error = null; if (!$this->pinVerifier()->meetsLengthPolicy($pin)) { @@ -78,12 +87,30 @@ class ProfileController extends AdminController return $this->renderPinForm($guard, $userId, $error, 422); } + // Re-verification d'identite : (re)definir un credential sensible exige le mot + // de passe courant. Message generique (ne distingue pas mot de passe vide / + // faux) ; verify paie le cout argon2id, sans leurre dedie ici car l'utilisateur + // est deja authentifie (l'enumeration de comptes ne s'applique pas a sa propre + // session). Echec -> 422 (requete bien formee, semantiquement refusee). + if (!$this->passwordHasher()->verify($currentPassword, $this->currentPasswordHash($userId))) { + return $this->renderPinForm($guard, $userId, 'Mot de passe actuel incorrect.', 422); + } + + // `pinIsSet` AVANT l'ecriture : distingue une premiere definition d'un changement + // pour le libelle d'audit (aucune valeur sensible n'est tracee). + $wasSet = $this->userRepository()->pinIsSet($userId); + // Gate sur 1 ligne affectee : une cible inexistante (0 ligne) ne doit pas // produire un faux "PIN enregistre" (defense en profondeur). if ($this->userRepository()->setPinHash($userId, $this->passwordHasher()->hash($pin)) !== 1) { return $this->renderPinForm($guard, $userId, 'Echec de l enregistrement du PIN.', 500); } + // Trace d'audit (ADR-0004, RG-T14) : l'acteur est l'utilisateur de session + // (action self-service, pas de PIN equipier tiers). Le summary ne porte que + // l'evenement set/change, jamais le PIN ni un hash. + $this->writePinAudit($userId, $guard->roleId ?? 0, $wasSet); + $this->setFlash('PIN enregistre.'); return Response::make('', 302, ['Location' => '/admin/profile/pin']); @@ -113,4 +140,45 @@ class ProfileController extends AdminController { return new PasswordHasher($this->config); } + + /** + * Hash du mot de passe courant de l'utilisateur de session, pour la + * re-verification d'identite. Lecture ciblee d'une colonne (UserRepository + * n'expose pas le hash : son allowlist d'ecriture ne le lie jamais) ; un compte + * absent/inactif renvoie une chaine vide -> verify echoue (refus generique). + * is_active = 1 : un compte desactive ne peut pas (re)definir son PIN. + */ + protected function currentPasswordHash(int $userId): string + { + $row = $this->db()->fetch( + 'SELECT password_hash FROM user WHERE id = :id AND is_active = 1', + ['id' => $userId], + ); + + return is_string($row['password_hash'] ?? null) ? (string) $row['password_hash'] : ''; + } + + /** + * Ecrit la trace d'audit du set/change de PIN (ADR-0004, RG-T14). action_code + * `pin.set` pour les deux cas (definition ET changement) ; le summary distingue + * via $wasSet. entity = l'utilisateur agissant (self-service). Aucune valeur + * sensible (PIN, hash) n'est journalisee. Hors transaction : l'ecriture du PIN est + * un seul UPDATE deja committe ; l'audit suit immediatement (pas d'effet composite + * a rendre atomique, a la difference de l'annulation OrderRepository::cancel). + */ + protected function writePinAudit(int $userId, int $roleId, bool $wasSet): void + { + $this->db()->execute( + 'INSERT INTO audit_log (actor_user_id, actor_role_id, action_code, entity_type, entity_id, summary) ' + . 'VALUES (:uid, :rid, :code, :etype, :eid, :summary)', + [ + 'uid' => $userId, + 'rid' => $roleId, + 'code' => 'pin.set', + 'etype' => 'user', + 'eid' => $userId, + 'summary' => $wasSet ? 'PIN modifie (self-service)' : 'PIN defini (self-service)', + ], + ); + } } diff --git a/src/app/Views/admin/profile/pin.php b/src/app/Views/admin/profile/pin.php index c854c18..50aff96 100644 --- a/src/app/Views/admin/profile/pin.php +++ b/src/app/Views/admin/profile/pin.php @@ -32,6 +32,12 @@ $errorMessage = isset($error) && is_string($error) ? $error : null;
+
+ + + Confirme votre identite avant de definir un PIN d action sensible. +
+
diff --git a/tests/Support/FakeDatabase.php b/tests/Support/FakeDatabase.php index 0c0aa2f..dc18bd2 100644 --- a/tests/Support/FakeDatabase.php +++ b/tests/Support/FakeDatabase.php @@ -120,6 +120,14 @@ final class FakeDatabase implements DatabaseInterface /** Resultat de UserRepository::pinIsSet() (true = un PIN est defini). */ public bool $userPinSet = false; + /** + * Ligne {password_hash} renvoyee pour la re-verification d'identite au set de PIN + * (ProfileController::currentPasswordHash) ; null = compte absent/inactif. + * + * @var array|null + */ + public ?array $currentPasswordRow = null; + /** * Lignes renvoyees par ProductRepository::all(). * @@ -149,6 +157,14 @@ final class FakeDatabase implements DatabaseInterface */ public ?array $orderByNumberRow = null; + /** + * Ligne {source} renvoyee pour OrderAdminController::orderSource (garde de + * visibilite PRE-3, 6.1) ; null = numero inconnu (traite comme non visible). + * + * @var array|null + */ + public ?array $orderSourceRow = null; + /** * Lignes renvoyees par MenuRepository::all(). * @@ -418,6 +434,13 @@ final class FakeDatabase implements DatabaseInterface return $this->userPinSet ? ['id' => 1] : null; } + // Re-verification d'identite au set de PIN (ProfileController) : lecture du + // password_hash du compte actif de session. is_active = 1 dans le predicat : + // retirer ce filtre en production ferait virer au rouge le test du compte inactif. + if (str_contains($sql, 'SELECT password_hash FROM user WHERE id') && str_contains($sql, 'is_active = 1')) { + return $this->currentPasswordRow; + } + // Exige is_active = 1 (garde RG-T13) : retirer le predicat en production // ferait virer au rouge les tests de resolveActingUser. if (str_contains($sql, 'pin_hash FROM user WHERE email') && str_contains($sql, 'is_active = 1')) { @@ -436,6 +459,14 @@ final class FakeDatabase implements DatabaseInterface return $this->menuRow; } + // Garde de visibilite PRE-3 (6.1) : lecture ciblee de la seule colonne source + // par OrderAdminController::orderSource. Doit passer AVANT la route generique + // 'FROM customer_order WHERE order_number' (orderByNumberRow) qu'elle matche + // aussi. null = numero inconnu (l'appelant le traite comme non visible). + if (str_contains($sql, 'SELECT source FROM customer_order WHERE order_number')) { + return $this->orderSourceRow; + } + if (str_contains($sql, 'FROM customer_order WHERE order_number')) { return $this->orderByNumberRow; } diff --git a/tests/Unit/Admin/OrderAdminControllerTest.php b/tests/Unit/Admin/OrderAdminControllerTest.php index 0a928b8..79a5794 100644 --- a/tests/Unit/Admin/OrderAdminControllerTest.php +++ b/tests/Unit/Admin/OrderAdminControllerTest.php @@ -183,6 +183,81 @@ final class OrderAdminControllerTest extends TestCase self::assertSame(403, $response->status()); } + // --- DELIVER_ORDER (6.1, garde de visibilite PRE-3 / ERR-2) --- + + private function deliverDb(): FakeDatabase + { + // order.deliver accorde + permission CSRF franchie en aval ; la source de la + // commande est scriptee par chaque test via orderSourceRow. + $db = $this->permittedDb(); + $db->permissionCodes = ['order.read', 'order.deliver']; + + return $db; + } + + public function testDeliverRejectsSourceNotVisibleByRole(): void + { + // ERR-2 (6.1) : le role agissant ne voit que 'drive' (role_visible_source), + // mais la commande est de source 'counter' -> 403 FORBIDDEN, aucune transition. + $db = $this->deliverDb(); + $db->roleSources = [['source' => 'drive']]; + $db->orderSourceRow = ['source' => 'counter']; + + $request = $this->post(['_csrf' => $this->csrf], '/admin/orders/C42/deliver'); + $response = $this->controllerWith($request, $db)->deliver(['number' => 'C42']); + + self::assertSame(403, $response->status()); + self::assertFalse($db->wrote('UPDATE customer_order SET status')); + } + + public function testDeliverUnknownOrderIsRejectedAsNotVisible(): void + { + // Numero inconnu -> source null -> chemin "non visible" (403), pas de transition. + $db = $this->deliverDb(); + $db->roleSources = []; // vue globale + $db->orderSourceRow = null; // numero inconnu + + $request = $this->post(['_csrf' => $this->csrf], '/admin/orders/K99/deliver'); + $response = $this->controllerWith($request, $db)->deliver(['number' => 'K99']); + + self::assertSame(403, $response->status()); + self::assertFalse($db->wrote('UPDATE customer_order SET status')); + } + + public function testDeliverSucceedsWhenSourceVisibleToRole(): void + { + // Source visible (le role voit kiosk+counter ; commande 'counter') -> la + // transition paid -> delivered est tentee et la liste est re-affichee (302). + $db = $this->deliverDb(); + $db->roleSources = [['source' => 'kiosk'], ['source' => 'counter']]; + $db->orderSourceRow = ['source' => 'counter']; + // OrderRepository::deliver lit la commande (paid) via findByNumber/SELECT statut. + $db->orderByNumberRow = ['id' => 100, 'order_number' => 'C42', 'total_ttc_cents' => 1990, 'status' => 'paid']; + + $request = $this->post(['_csrf' => $this->csrf], '/admin/orders/C42/deliver'); + $response = $this->controllerWith($request, $db)->deliver(['number' => 'C42']); + + self::assertSame(302, $response->status()); + self::assertSame('/admin/orders', $response->header('Location')); + self::assertTrue($db->wrote('UPDATE customer_order SET status')); + } + + public function testDeliverGlobalViewRoleSeesAllSources(): void + { + // role_visible_source vide -> vue globale (admin/manager) : une commande de + // n'importe quel canal est remisable. visibleSources renvoie les trois sources. + $db = $this->deliverDb(); + $db->roleSources = []; // aucune ligne -> vue globale + $db->orderSourceRow = ['source' => 'drive']; + $db->orderByNumberRow = ['id' => 101, 'order_number' => 'D7', 'total_ttc_cents' => 1500, 'status' => 'paid']; + + $request = $this->post(['_csrf' => $this->csrf], '/admin/orders/D7/deliver'); + $response = $this->controllerWith($request, $db)->deliver(['number' => 'D7']); + + self::assertSame(302, $response->status()); + self::assertTrue($db->wrote('UPDATE customer_order SET status')); + } + // --- CANCEL_ORDER (7.1, order.cancel + PIN equipier RG-T13) --- public function testCancelRequiresOrderCancelPermission(): void diff --git a/tests/Unit/Admin/ProfileControllerTest.php b/tests/Unit/Admin/ProfileControllerTest.php index ca4a77f..17fea27 100644 --- a/tests/Unit/Admin/ProfileControllerTest.php +++ b/tests/Unit/Admin/ProfileControllerTest.php @@ -7,6 +7,7 @@ namespace App\Tests\Unit\Admin; use PHPUnit\Framework\TestCase; use App\Auth\Authorizer; use App\Auth\Csrf; +use App\Auth\PasswordHasher; use App\Auth\SessionGuard; use App\Auth\SessionManager; use App\Auth\UserDirectory; @@ -14,6 +15,7 @@ use App\Auth\UserRepository; use App\Controllers\ProfileController; use App\Core\Config; use App\Core\Database; +use App\Core\DatabaseInterface; use App\Core\Request; use App\Tests\Support\FakeDatabase; @@ -34,6 +36,13 @@ final class TestProfileController extends ProfileController return $this->testSession; } + // Couture DB unique : la re-verification du mot de passe courant et l'ecriture + // d'audit du set de PIN passent par db() ; on la route vers le double. + protected function db(): DatabaseInterface + { + return $this->fakeDb; + } + protected function sessionGuard(): SessionGuard { return new SessionGuard($this->testSession, $this->fakeDb, $this->config); @@ -96,6 +105,9 @@ final class ProfileControllerTest extends TestCase putenv($key . '=' . $value); } + /** Mot de passe courant de reference pour la re-verification au set de PIN. */ + private const CURRENT_PASSWORD = 'S3cret-Wakdo!'; + private function permittedDb(): FakeDatabase { $db = new FakeDatabase(); @@ -103,6 +115,9 @@ final class ProfileControllerTest extends TestCase $db->userDisplayRow = ['first_name' => 'Corentin', 'last_name' => 'J', 'role_label' => 'Administrateur']; $db->canResult = true; $db->permissionCodes = ['category.manage']; + // Re-verification d'identite : hash argon2id du mot de passe courant (couts + // de test poses en setUp). currentPasswordRow null -> verify echoue. + $db->currentPasswordRow = ['password_hash' => (new PasswordHasher(new Config()))->hash(self::CURRENT_PASSWORD)]; return $db; } @@ -122,6 +137,17 @@ final class ProfileControllerTest extends TestCase ); } + /** + * Requete nominale de set de PIN : CSRF valide, PIN + confirmation, et le mot de + * passe courant attendu par la re-verification d'identite (permittedDb). + */ + private function validPost(): Request + { + return $this->post([ + '_csrf' => $this->csrf, 'pin' => '4729', 'pin_confirm' => '4729', 'current_password' => self::CURRENT_PASSWORD, + ]); + } + private function controller(Request $request, FakeDatabase $db): TestProfileController { return new TestProfileController($request, new Config(), new Database(new Config()), $this->session, $db); @@ -155,7 +181,8 @@ final class ProfileControllerTest extends TestCase public function testUpdatePinValidStoresHashAndRedirects(): void { $db = $this->permittedDb(); - $response = $this->controller($this->post(['_csrf' => $this->csrf, 'pin' => '4729', 'pin_confirm' => '4729']), $db)->updatePin(); + $db->userPinSet = false; // premiere definition -> summary "PIN defini" + $response = $this->controller($this->validPost(), $db)->updatePin(); self::assertSame(302, $response->status()); self::assertSame('/admin/profile/pin', $response->header('Location')); @@ -175,16 +202,63 @@ final class ProfileControllerTest extends TestCase self::assertNotSame('4729', $write['params']['hash'] ?? null); } + public function testUpdatePinWritesAuditTrace(): void + { + // ADR-0004 / RG-T14 : le set de PIN ecrit une ligne audit_log (action pin.set), + // imputee a l'utilisateur de session, sans jamais journaliser le PIN ni un hash. + $db = $this->permittedDb(); + $db->userPinSet = true; // un PIN existe deja -> changement + $response = $this->controller($this->validPost(), $db)->updatePin(); + + self::assertSame(302, $response->status()); + self::assertSame(['pin.set'], $db->auditActions()); + + $audit = null; + foreach ($db->writes as $w) { + if (str_contains($w['sql'], 'INSERT INTO audit_log')) { + $audit = $w; + break; + } + } + self::assertNotNull($audit); + self::assertSame(1, $audit['params']['uid'] ?? null); // acteur = session userId + self::assertSame('user', $audit['params']['etype'] ?? null); + self::assertSame(1, $audit['params']['eid'] ?? null); + // Aucune valeur sensible dans le summary (ni PIN clair, ni hash). + $summary = (string) ($audit['params']['summary'] ?? ''); + self::assertStringNotContainsString('4729', $summary); + self::assertStringContainsString('modifie', $summary); // userPinSet=true -> "PIN modifie" + } + + public function testUpdatePinRejectsWrongCurrentPassword(): void + { + // Re-verification d'identite : mauvais mot de passe courant -> 422, ni + // ecriture du PIN, ni trace d'audit. + $db = $this->permittedDb(); + $request = $this->post([ + '_csrf' => $this->csrf, 'pin' => '4729', 'pin_confirm' => '4729', 'current_password' => 'wrong-password', + ]); + $response = $this->controller($request, $db)->updatePin(); + + self::assertSame(422, $response->status()); + self::assertStringContainsString('Mot de passe actuel incorrect', $response->body()); + self::assertFalse($db->wrote('UPDATE user SET pin_hash')); + self::assertFalse($db->wrote('INSERT INTO audit_log')); + self::assertNull($this->session->get('_flash')); + } + public function testUpdatePinFailsWhenNoRowAffected(): void { - // Cible inexistante (0 ligne affectee) : pas de faux succes, pas de flash. + // Cible inexistante (0 ligne affectee) : pas de faux succes, pas de flash, pas + // d'audit (l'ecriture du PIN n'a rien affecte). $db = $this->permittedDb(); $db->executeRowCount = 0; - $response = $this->controller($this->post(['_csrf' => $this->csrf, 'pin' => '4729', 'pin_confirm' => '4729']), $db)->updatePin(); + $response = $this->controller($this->validPost(), $db)->updatePin(); self::assertSame(500, $response->status()); self::assertNull($this->session->get('_flash')); + self::assertFalse($db->wrote('INSERT INTO audit_log')); } public function testUpdatePinMismatchRerenders422(): void