feat(security): garde DELIVER_ORDER (PRE-3) + audit/re-verif PIN (#110)
All checks were successful
CI / secret-scan (push) Successful in 24s
CI / php-lint (push) Successful in 38s
CI / static-tests (push) Successful in 1m13s
CI / js-tests (push) Successful in 44s

This commit is contained in:
Corentin JOGUET 2026-06-25 10:49:06 +02:00
parent 6cc762a964
commit a6dcb31c16
6 changed files with 313 additions and 4 deletions

View file

@ -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<string, string> $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]);

View file

@ -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)',
],
);
}
}

View file

@ -32,6 +32,12 @@ $errorMessage = isset($error) && is_string($error) ? $error : null;
<form method="post" action="/admin/profile/pin" class="form-card">
<input type="hidden" name="_csrf" value="<?= $csrf ?>">
<div class="form-group">
<label class="form-label" for="current_password">Mot de passe actuel</label>
<input class="form-input" type="password" id="current_password" name="current_password" autocomplete="current-password" required>
<small>Confirme votre identite avant de definir un PIN d action sensible.</small>
</div>
<div class="form-group">
<label class="form-label" for="pin">Nouveau PIN</label>
<input class="form-input" type="password" id="pin" name="pin" inputmode="numeric" autocomplete="off" required>

View file

@ -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<string, mixed>|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<string, mixed>|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;
}

View file

@ -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

View file

@ -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