From 0f8fc48df078f2613fcf545fcc6f95b07abbe08d Mon Sep 17 00:00:00 2001 From: Matthieu Date: Mon, 1 Jun 2026 12:15:33 +0200 Subject: [PATCH] fix(commercial) : robust gating + strict category denormalizer + provider via EM (review ERP-55) --- .../CategoryReferenceDenormalizer.php | 14 +- .../State/Processor/ClientProcessor.php | 143 +++++++++++++++--- .../State/Provider/ClientProvider.php | 11 +- .../CategoryReferenceDenormalizerTest.php | 62 ++++++++ .../Commercial/Unit/ClientProcessorTest.php | 123 +++++++++++++-- 5 files changed, 315 insertions(+), 38 deletions(-) create mode 100644 tests/Module/Commercial/Unit/CategoryReferenceDenormalizerTest.php diff --git a/src/Module/Commercial/Infrastructure/ApiPlatform/Serializer/CategoryReferenceDenormalizer.php b/src/Module/Commercial/Infrastructure/ApiPlatform/Serializer/CategoryReferenceDenormalizer.php index a1f1e72..e588c85 100644 --- a/src/Module/Commercial/Infrastructure/ApiPlatform/Serializer/CategoryReferenceDenormalizer.php +++ b/src/Module/Commercial/Infrastructure/ApiPlatform/Serializer/CategoryReferenceDenormalizer.php @@ -6,6 +6,7 @@ namespace App\Module\Commercial\Infrastructure\ApiPlatform\Serializer; use ApiPlatform\Metadata\IriConverterInterface; use App\Shared\Domain\Contract\CategoryInterface; +use Symfony\Component\Serializer\Exception\UnexpectedValueException; use Symfony\Component\Serializer\Normalizer\DenormalizerInterface; /** @@ -40,7 +41,18 @@ final class CategoryReferenceDenormalizer implements DenormalizerInterface // est le comportement attendu pour une reference cassee. $resource = $this->iriConverter->getResourceFromIri($data); - return $resource instanceof CategoryInterface ? $resource : null; + // IRI syntaxiquement valide mais pointant sur une autre ressource (ex: + // '/api/clients/5' la ou une categorie est attendue) : on refuse + // explicitement plutot que de retourner null silencieusement, ce qui + // perdrait la reference sans erreur. UnexpectedValueException -> 400. + if (!$resource instanceof CategoryInterface) { + throw new UnexpectedValueException(sprintf( + 'L\'IRI "%s" ne référence pas une catégorie.', + $data, + )); + } + + return $resource; } public function supportsDenormalization(mixed $data, string $type, ?string $format = null, array $context = []): bool diff --git a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php index e083744..5f665f1 100644 --- a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php +++ b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php @@ -15,6 +15,7 @@ use App\Shared\Domain\Contract\CategoryInterface; use App\Shared\Domain\Security\BusinessRoles; use DateTimeImmutable; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; +use Doctrine\ORM\EntityManagerInterface; use JsonException; use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\DependencyInjection\Attribute\Autowire; @@ -84,6 +85,7 @@ final class ClientProcessor implements ProcessorInterface private readonly ClientInformationCompletenessValidator $informationValidator, private readonly Security $security, private readonly RequestStack $requestStack, + private readonly EntityManagerInterface $em, ) {} public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed @@ -92,17 +94,17 @@ final class ClientProcessor implements ProcessorInterface return $this->persistProcessor->process($data, $operation, $uriVariables, $context); } - $payloadKeys = $this->payloadKeys(); + $writableKeys = $this->writablePayloadKeys(); - $isArchiveRequest = $this->guardArchive($data, $payloadKeys); - $this->guardAccounting($payloadKeys); + $isArchiveRequest = $this->guardArchive($data, $writableKeys); + $this->guardAccounting($data); $this->normalize($data); $this->validateMainContact($data); $this->validateDistributorBroker($data); $this->validateAccountingConsistency($data); - $this->validateInformationCompleteness($data, $payloadKeys); + $this->validateInformationCompleteness($data, $writableKeys); try { return $this->persistProcessor->process($data, $operation, $uriVariables, $context); @@ -126,15 +128,28 @@ final class ClientProcessor implements ProcessorInterface } /** - * RG-1.22 / RG-1.23 : si le payload porte isArchived, exige la permission - * archive (403), interdit toute autre modification (422) et pose/retire - * archivedAt. Retourne true si la requete est une requete d'archivage. + * RG-1.22 / RG-1.23 : si le payload bascule reellement isArchived, exige la + * permission archive (403), interdit toute autre modification (422) et + * pose/retire archivedAt. Retourne true si la requete est une requete + * d'archivage. * - * @param list $payloadKeys + * Le gating est restreint a la mise a jour d'un client existant ET au seul + * cas ou isArchived change vraiment : un POST (entite non encore geree par + * l'ORM) ou un PATCH « representation complete » renvoyant isArchived + * inchange ne doit declencher ni 403 ni 422 parasite. + * + * @param list $writableKeys cles ecrivables du payload (hors @* et champs inconnus) */ - private function guardArchive(Client $data, array $payloadKeys): bool + private function guardArchive(Client $data, array $writableKeys): bool { - if (!in_array(self::ARCHIVE_FIELD, $payloadKeys, true)) { + // POST / entite non geree : l'archivage est une action de mise a jour. + if (!$this->em->contains($data)) { + return false; + } + + // isArchived inchange par rapport a l'etat persiste : pas une requete + // d'archivage (cas du PATCH representation complete). + if (!$this->fieldChanged($data, 'isArchived', $data->isArchived())) { return false; } @@ -146,8 +161,8 @@ final class ClientProcessor implements ProcessorInterface )); } - // RG-1.22 : une requete d'archivage ne modifie aucun autre champ. - if ([] !== array_diff($payloadKeys, [self::ARCHIVE_FIELD])) { + // RG-1.22 : une requete d'archivage ne modifie aucun autre champ ecrivable. + if ([] !== array_diff($writableKeys, [self::ARCHIVE_FIELD])) { throw new UnprocessableEntityHttpException( 'Une requête d\'archivage ne peut modifier aucun autre champ que "isArchived".', ); @@ -160,29 +175,88 @@ final class ClientProcessor implements ProcessorInterface } /** - * RG-1.28 : un champ comptable dans le payload exige accounting.manage, - * sinon 403 sur l'ensemble du payload (mode strict, pas de filtrage - * silencieux). Le message precise le premier champ fautif. - * - * @param list $payloadKeys + * RG-1.28 : la modification effective d'un champ comptable exige + * accounting.manage, sinon 403 sur l'ensemble du payload (mode strict, pas + * de filtrage silencieux). On ne gate que si un champ change reellement par + * rapport a l'etat persiste : un POST/PATCH renvoyant des champs comptables + * inchanges (ou null en creation) ne declenche pas de 403 parasite. Le + * message precise le premier champ fautif. */ - private function guardAccounting(array $payloadKeys): void + private function guardAccounting(Client $data): void { - $touched = array_values(array_intersect($payloadKeys, self::ACCOUNTING_FIELDS)); + $changed = $this->changedAccountingFields($data); - if ([] === $touched) { + if ([] === $changed) { return; } if (!$this->security->isGranted(self::PERM_ACCOUNTING_MANAGE)) { throw new AccessDeniedHttpException(sprintf( 'Le champ "%s" requiert la permission "%s".', - $touched[0], + $changed[0], self::PERM_ACCOUNTING_MANAGE, )); } } + /** + * Champs comptables dont la valeur courante differe de l'etat persiste. Les + * relations (tvaMode, paymentDelay, paymentType, bank) sont comparees par + * identite d'objet : l'identity map Doctrine renvoie la meme instance tant + * que la reference est inchangee. + * + * @return list + */ + private function changedAccountingFields(Client $data): array + { + $changed = []; + + foreach (self::ACCOUNTING_FIELDS as $field) { + $newValue = match ($field) { + 'siren' => $data->getSiren(), + 'accountNumber' => $data->getAccountNumber(), + 'tvaMode' => $data->getTvaMode(), + 'nTva' => $data->getNTva(), + 'paymentDelay' => $data->getPaymentDelay(), + 'paymentType' => $data->getPaymentType(), + 'bank' => $data->getBank(), + }; + + if ($this->fieldChanged($data, $field, $newValue)) { + $changed[] = $field; + } + } + + return $changed; + } + + /** + * Vrai si la valeur courante d'un champ differe de l'etat persiste. Pour une + * entite non geree (creation/POST), l'etat persiste est vide : toute valeur + * non-null est alors un changement. + */ + private function fieldChanged(Client $data, string $field, mixed $newValue): bool + { + $original = $this->originalData($data); + + return $newValue !== ($original[$field] ?? null); + } + + /** + * Snapshot des valeurs persistees de l'entite (telles que chargees, avant + * application du payload). Vide pour une entite non geree (POST). + * + * @return array + */ + private function originalData(Client $data): array + { + if (!$this->em->contains($data)) { + return []; + } + + return $this->em->getUnitOfWork()->getOriginalEntityData($data); + } + /** * Normalisation serveur (RG-1.18 a 1.21). Les setters non-nullables * (companyName, email, phonePrimary) ne sont touches que si une valeur est @@ -317,11 +391,32 @@ final class ClientProcessor implements ProcessorInterface && $user->hasBusinessRole(BusinessRoles::COMMERCIALE); } + /** + * Cles ecrivables effectivement presentes dans le payload : on retire les + * cles JSON-LD (@id, @context, @var...) et tout champ non rattache a un + * groupe d'ecriture connu. C'est la base du 422 d'archivage (RG-1.22) et du + * declenchement conditionnel de RG-1.04 — sans elles, un PATCH + * « representation complete » porteur de @id ferait croire a une + * modification multi-onglets. + * + * @return list + */ + private function writablePayloadKeys(): array + { + $writable = array_merge( + self::MAIN_FIELDS, + self::INFORMATION_FIELDS, + self::ACCOUNTING_FIELDS, + [self::ARCHIVE_FIELD], + ); + + return array_values(array_intersect($this->payloadKeys(), $writable)); + } + /** * Cles de premier niveau effectivement envoyees par le client (payload JSON - * brut). Pour un PATCH merge-patch+json, ce sont les seuls champs modifies ; - * c'est ce qui permet le gating par onglet (RG-1.22 / RG-1.28) et le - * declenchement conditionnel de RG-1.04. + * brut), filtrage compris. Pour un PATCH merge-patch+json, ce sont les seuls + * champs modifies. * * @return list */ diff --git a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Provider/ClientProvider.php b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Provider/ClientProvider.php index bbd6e52..f401375 100644 --- a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Provider/ClientProvider.php +++ b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Provider/ClientProvider.php @@ -11,6 +11,7 @@ use ApiPlatform\State\Pagination\Pagination; use ApiPlatform\State\ProviderInterface; use App\Module\Commercial\Domain\Entity\Client; use App\Module\Commercial\Domain\Repository\ClientRepositoryInterface; +use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\QueryBuilder; use Doctrine\ORM\Tools\Pagination\Paginator as DoctrinePaginator; use Symfony\Component\DependencyInjection\Attribute\Autowire; @@ -45,6 +46,7 @@ final class ClientProvider implements ProviderInterface #[Autowire(service: 'App\Module\Commercial\Infrastructure\Doctrine\DoctrineClientRepository')] private readonly ClientRepositoryInterface $repository, private readonly Pagination $pagination, + private readonly EntityManagerInterface $em, ) {} public function provide(Operation $operation, array $uriVariables = [], array $context = []): Client|iterable|Paginator|null @@ -73,7 +75,7 @@ final class ClientProvider implements ProviderInterface // Echappatoire ?pagination=false : collection complete sans Paginator // (cf. convention ERP-72 — utile pour un