doc/audit-log-review-backlog.md

@@ -0,0 +1,294 @@
+# Backlog — Code review PR #9 (audit-log)
+
+Findings du review multi-agent (security + architecture + codex sceptique) sur la PR #9 `feat/audit-log`, qui **n'ont pas ete traites dans la PR** et sont a ouvrir en tickets dedies.
+
+Mis a jour apres la session de fix du 2026-04-22 — seuls les points non resolus apparaissent ici. Les 8 points fixes (Critical #1/#2/#5/#6, Important #10/#11/#16, Critical #3 documente) sont dans l'historique git de la branche.
+
+Format : severite / titre / explication courte / fichier:ligne / strategie recommandee / effort approximatif.
+
+---
+
+## 🔴 Critical parke
+
+### C-4 — Blacklist password exact-match et non recursive
+
+**Fichier** : `src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:35,81-98`
+
+La blacklist `['password', 'plainPassword', 'token', 'secret']` est en match exact top-level. Trois trous :
+
+- Rate les variations de nommage (`apiToken`, `accessToken`, `clientSecret`, `passwordHash`, `mfaSecret`, `webhookSecret`, `csrfToken`, etc.)
+- Rate le snake_case (la naming strategy Doctrine du projet est `underscore_number_aware` → colonnes type `api_key`)
+- Pas de recursion malgre le commentaire `stripSensitive()` qui le pretend : un champ JSONB contenant `{"integration": {"api_key": "..."}}` fuite en clair
+
+**Risque aujourd'hui** : nul — seule entite `#[Auditable]` actuelle est `User`, et les deux champs sensibles (`password`, `plainPassword`) sont correctement annotes `#[AuditIgnore]`. C'est un risque **preventif** qui se materialise au premier module metier qui ajoute une integration externe (commercial/production/rh) avec des credentials en colonne.
+
+**Strategie recommandee (Option B du brainstorm)** :
+- Supprimer la blacklist (fausse securite)
+- Faire de `#[AuditIgnore]` la seule defense
+- Ajouter un test CI : parcourt toutes les entites `#[Auditable]` via reflection, liste les proprietes dont le nom matche `/token|secret|password|key|salt|hash|passphrase/i`, assert que chacune porte `#[AuditIgnore]`
+
+**Effort** : 15-20 min.
+
+---
+
+## 🔴 Critical documente mais pas fixe code
+
+### C-3 — Savepoints + connexion audit dediee = lignes audit orphelines
+
+**Fichiers** : `src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:19-30`, `config/packages/doctrine.yaml:3-23`
+
+Le contrat est documente dans `doc/audit-log.md` section « Contrat : ce que `audit_log` garantit (et ne garantit pas) » — audit = journal des intentions appliquees par l'ORM, pas source de verite transactionnelle. Acceptable pour un CRM interne (rollbacks outermost rares).
+
+Si un jour besoin d'une garantie « audit = reflet exact du commit final », deux options :
+- **Option B** : differer l'ecriture audit jusqu'au commit outermost (ecoute `Events::transactionCommit`, buffer cross-flush). Complexe, distinguer RELEASE SAVEPOINT d'un vrai COMMIT.
+- **Option C** : ecrire l'audit sur la meme connexion que le metier. Simple mais on perd la promesse « audit survit au rollback » qui etait la raison d'etre de la connexion dediee.
+
+**Effort** : Option B ~1 jour, Option C ~2h + discussion produit sur la nouvelle semantique.
+
+---
+
+## 🟠 Important
+
+### I-7 — `DbalPaginator` fait `COUNT(*)` sur chaque list request
+
+**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:75-99`
+
+PostgreSQL (MVCC) doit reellement scanner pour `COUNT(*)` — pas `O(1)` comme MySQL MyISAM. Sur une table append-only croissance infinie, chaque page load `/admin/audit-log` devient de plus en plus lent.
+
+Estimation : ~10k lignes/jour (50 users × 200 actions) → 3.65M lignes/an → page load de 20ms aujourd'hui, ~2-3s dans 2 ans. Pire avec un filtre ILIKE (wildcard leading = full scan).
+
+**Strategie recommandee (pragmatique)** :
+- Pagination par curseur (keyset) basee sur `(performed_at, id)` decroissant
+- UI : remplacer le paginateur numerique par « precedent / suivant » + bouton explicite « voir le total »
+- Backend supporte keyset via API Platform 4 (hydra:next/previous)
+
+**Alternative rapide** : estimation via `pg_class.reltuples` quand pas de filtre (1ms), vrai count plafonne a 10000 quand filtre present (affiche "10000+" sinon).
+
+**Effort** : keyset complet ~1h30-2h, version pragmatique ~30 min.
+
+**Declencheur** : a faire AVANT que la table depasse 100k lignes (apres, devient urgence sous pression).
+
+---
+
+### I-8 — Pas de politique de retention / archival sur `audit_log`
+
+**Fichiers** : `migrations/Version20260420202749.php`, `doc/audit-log.md`
+
+La migration elle-meme decrit la table comme « croissance infinie ». Aucune TTL, archive job, ou partitioning documente. Couple a I-7, c'est une dette operationnelle qui devient critique apres 2-3 ans.
+
+**Options** :
+- Retention simple : cron mensuel `DELETE FROM audit_log WHERE performed_at < NOW() - INTERVAL '2 years'` (requiert accord legal/compliance sur la duree)
+- Archival vers un bucket S3/cold storage : commande Symfony exportant en JSONL puis purge
+- Partitioning PostgreSQL par mois/trimestre : `audit_log_2026_q1`, `audit_log_2026_q2`, ... drop partition apres N mois
+
+**Effort** : depend du choix. Retention simple ~2h. Archival ~1 jour. Partitioning ~1-2 jours + migration progressive.
+
+**Decision produit requise** avant implementation.
+
+---
+
+### I-9 — Echec DB audit silencieusement swallowed, pas d'alerting
+
+**Fichier** : `src/Module/Core/Infrastructure/Doctrine/AuditListener.php:151-169`
+
+Le try/catch swallow toute exception de `AuditLogWriter::log()`, log au niveau `error` dans Monolog, et continue. Si la connexion `audit` tombe (pool sature, disque plein, etc.), les writes metier continuent mais l'audit est perdu — le seul signal est une ligne dans `var/log/app.log`.
+
+Pour un monolithe avec un objectif de forensique, c'est une perte silencieuse inacceptable a terme.
+
+**Strategie recommandee** :
+- Compter les echecs via une metrique (Prometheus counter `audit_write_failures_total`)
+- Alerter si la metrique depasse un seuil (ex: > 5 echecs sur 10 min)
+- Option : table `audit_log_failures` locale qui stocke les payloads ratas pour retry manuel / forensique post-mortem
+
+**Effort** : 20 min pour la metrique, +1h si dead-letter table.
+
+---
+
+### I-12 — `ensureCurrentSiteConsistency` : 2e flush attribue a l'admin qui PATCH
+
+**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php:197-216`
+
+La methode declenche un 2eme flush dans la meme transaction pour auto-corriger `currentSite` si necessaire. Ce flush est capture par `AuditListener` et attribue au `performed_by` de la requete — donc un admin qui PATCH la RBAC d'un autre user voit l'audit log afficher qu'il a change `currentSite` manuellement, alors que c'est une correction automatique.
+
+**Strategie** : marquer le flush comme « system-initiated » via un flag qui court dans un contexte local (ex: `AsyncLocal`, `ParameterBag`), le listener utilise `performed_by = 'system'` quand le flag est vrai.
+
+**Effort** : 10-15 min.
+
+**Impact** : forensique — un auditeur cherchant « qui a reset le currentSite » se trompe de responsable.
+
+---
+
+### I-13 — `AuditListener` pas scope-pinne a l'EntityManager
+
+**Fichier** : `src/Module/Core/Infrastructure/Doctrine/AuditListener.php:65-66`
+
+Le listener utilise `#[AsDoctrineListener(event: ...)]` sans argument `connection`. Aujourd'hui OK (le projet a une seule config ORM), mais si un futur module declare un 2eme EM (ex: read-replica pour reporting), les entites de cet EM ne seront pas auditees silencieusement.
+
+**Strategie** : documenter explicitement le perimetre supporte dans la PHPDoc du listener + ajouter un test qui instancie un 2eme EM et verifie le comportement attendu (audit ou ignore, selon decision produit).
+
+**Effort** : 5 min doc + 30 min test si besoin.
+
+---
+
+### I-14 — Pas de regression test direct pour "sites overwritten on PATCH omission"
+
+**Fichier** : `tests/Module/Core/Api/UserRbacSitesApiTest.php:142-169`
+
+Le test `testRbacPatchWithoutSitesFieldDoesNotChangeCurrentSite` verifie que `currentSite` n'est pas touche, mais n'assert pas que la collection `sites` elle-meme est preservee. Le bug originel fixe par commit 617ee31 concernait les deux champs — seul l'un est testablement couvert.
+
+**Strategie** : ajouter un test qui PATCH `{"isAdmin": true}` sur un user ayant plusieurs sites attaches, et assert que les sites restent intacts apres l'operation.
+
+**Effort** : 5 min.
+
+---
+
+### I-15 — `AuditLogProvider` trop gras : extraire `DbalAuditLogRepository`
+
+**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php`
+
+Le Provider API Platform contient 80+ lignes de query building, extraction de filtres, escape LIKE, pagination, hydratation. Responsabilite mixte « orchestration API » et « requetes DBAL ». Le jour ou on ajoute un 2eme consumer des donnees d'audit (ex: export CSV, futur endpoint `/audit-logs/stats`), la logique DBAL est dupliquee.
+
+**Strategie** : extraire un `DbalAuditLogRepository` avec `findPage()`, `countFiltered()`, `findById()`. Provider devient un thin adapter.
+
+**Effort** : 20-30 min refacto.
+
+---
+
+### I-18 — `useAuditLog.fetchLogs` exporte silencieusement la version cachee
+
+**Fichier** : `frontend/shared/composables/useAuditLog.ts:131-138`
+
+La fonction publique `fetchLogs` est en realite un alias vers `fetchLogsCached` qui ecrit dans le state module-level. Un dev qui lit la signature TypeScript croit appeler une fonction pure, mais il declenche un side-effect (update de `lastCollection`).
+
+**Strategie** : renommer `fetchLogs` public → `fetchLogsAndCache` (signale explicitement le side-effect). Ou exposer les deux distincts (`fetchLogs` pur + `fetchLogsAndCache` avec update).
+
+**Effort** : 5 min (ripple sur `audit-log.vue` a suivre).
+
+---
+
+### I-19 — `RequestIdProvider` pas reset sur `kernel.finish_request`
+
+**Fichier** : `src/Module/Core/Infrastructure/Audit/RequestIdProvider.php:22-42`
+
+Le `requestId` est set en `kernel.request` mais jamais cleared. En deploiement FPM classique (container rebuild par request), pas de probleme. Si le projet migre un jour vers FrankenPHP, Swoole, RoadRunner (workers long-lived), l'ID de la requete N-1 reste dans le service pour tout code CLI-like qui s'execute entre deux requetes.
+
+**Strategie** : ajouter un event listener sur `kernel.finish_request` qui reset `$this->requestId = null` si c'est la main request.
+
+**Effort** : 5 min + test.
+
+**Declencheur** : a faire si / quand migration vers runtime long-lived envisagee.
+
+---
+
+## 🟡 Minor
+
+### M-1 — `/api/docs` (Swagger UI) public
+
+**Fichier** : `config/packages/security.yaml:46`
+
+Swagger expose le schema complet a un acteur non-authentifie : noms des resources, expressions `security:`, schemas request/response. Pas une faille mais une surface d'info disclosure.
+
+**Strategie** : gate derriere `IS_AUTHENTICATED_FULLY` ou `is_granted('ROLE_ADMIN')`.
+
+**Effort** : 2 min.
+
+---
+
+### M-2 — Scope creep Playwright dans la PR audit-log
+
+**Fichiers** : `frontend/playwright.config.ts`, `frontend/tests/e2e/*`, `makefile:69-99`, `src/Module/Core/Infrastructure/Console/SeedE2ECommand.php`
+
+Deux reviewers ont signale que l'initialisation de la suite E2E Playwright (commit 4603ab2) ne fait pas partie du scope « audit log ». Ideal aurait ete une PR separee.
+
+**Strategie retrospective** : a noter pour discipline future. Aucune action requise sur cette PR.
+
+---
+
+### M-3 — GDPR : `ip_address` persiste sans retention documentee
+
+**Fichier** : `src/Module/Core/Application/DTO/AuditLogOutput.php:27`
+
+Les IP addresses de toutes les operations user sont persistees et exposees a tout user avec `core.audit_log.view`. En EU c'est de la donnee personnelle sous GDPR. Avec une table append-only sans retention (cf. I-8), on cumule les IP indefiniment.
+
+**Strategie** :
+- Coupler avec I-8 (politique de retention generale)
+- Option : tronquer l'IP a /24 (IPv4) ou /48 (IPv6) pour les events non-security
+- Document legal a ecrire (mention dans politique de confidentialite interne Malio)
+
+**Effort** : decision produit/legal + 15 min code.
+
+---
+
+### M-4 — Coverage : test item endpoint `/audit-logs/{id}` sans permission manquant
+
+**Fichier** : `tests/Module/Core/Api/AuditLogApiTest.php`
+
+Test `testAuthenticatedUserWithoutPermissionGets403` couvre la collection. Le Get item (`AuditLogResource.php:51`) declare bien `security: "is_granted('core.audit_log.view')"` mais aucun test symetrique ne le verrouille.
+
+**Strategie** : ajouter `testItemEndpointRequires403WithoutPermission` copie-colle du test collection.
+
+**Effort** : 5 min.
+
+---
+
+### M-5 — `stripSensitive()` commentaire mensonger
+
+**Fichier** : `src/Module/Core/Infrastructure/Audit/AuditLogWriter.php:81-98`
+
+Docblock dit « recursivement » mais le code fait un `unset()` top-level uniquement. Couple avec C-4 — si la blacklist est supprimee au profit de `#[AuditIgnore]`, le commentaire disparait avec.
+
+**Strategie** : traiter via C-4, sinon corriger le commentaire en l'attendant.
+
+**Effort** : 1 min si standalone.
+
+---
+
+### M-6 — LIKE escape comment imprecis
+
+**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/State/Provider/AuditLogProvider.php:175-177`
+
+Le commentaire dit que `\` est le caractere d'echappement LIKE « par defaut en PostgreSQL », ce qui implique une dependance a `standard_conforming_strings`. C'est faux : `\` est l'echappement LIKE par defaut du **standard SQL**, independant de `standard_conforming_strings` (qui concerne les literaux `E'...'`).
+
+**Strategie** : corriger le commentaire pour lever l'ambiguite.
+
+**Effort** : 1 min.
+
+---
+
+### M-7 — `?page=0` → `OFFSET -N` → 500
+
+**Fichier** : `src/Module/Core/Infrastructure/ApiPlatform/Pagination/DbalPaginator.php`
+
+API Platform clampe `itemsPerPage` au max de la resource, mais `page` raw. `?page=0` → `(0-1) × 50 = -50` → `OFFSET -50` → PostgreSQL `22023: OFFSET must not be negative` → 500.
+
+**Strategie** : `max(1, (int) $this->pagination->getPage($context))` dans `provideCollection()`.
+
+**Effort** : 2 min + test.
+
+---
+
+### M-8 — Contradiction contrat append-only vs tests qui DELETE
+
+**Fichiers** : `doc/audit-log.md`, `tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php`, `tests/Module/Core/Api/AuditLogApiTest.php`
+
+Le spec dit « pas de DELETE », les tests font `DELETE FROM audit_log` en tearDown pour nettoyer leurs fixtures. Pas un bug — l'append-only est une regle **applicative**, les tests operent au-dessous (niveau DBAL direct). Juste a clarifier.
+
+**Strategie** : ajouter une note dans `doc/audit-log.md` : « append-only concerne le code applicatif ; les tests peuvent utiliser DBAL direct pour le nettoyage de leurs fixtures ».
+
+**Effort** : 2 min.
+
+---
+
+## Ordre de priorite suggere pour futur ticket
+
+**Bloc 1 — quick wins (~30 min cumule)** :
+I-14, I-18, I-19, M-1, M-4, M-6, M-7, M-8
+
+**Bloc 2 — fix mecaniques (~1-2h cumule)** :
+C-4 (preventif), I-12, I-15, M-5
+
+**Bloc 3 — sujets produit / scaling (1-2 jours)** :
+I-7 (avant 100k lignes), I-8 + M-3 (retention + GDPR groupe), I-9 (alerting)
+
+**Hors ce backlog** :
+C-3 reste a la discretion produit — le contrat actuel est documente et acceptable.