fix(commercial) : robust gating + strict category denormalizer + provider via EM (review ERP-55)

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

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);
+        $isArchiveRequest = $this->guardArchive($data, $writableKeys);
-        $this->guardAccounting($payloadKeys);
+        $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
+     * RG-1.22 / RG-1.23 : si le payload bascule reellement isArchived, exige la
-     * archive (403), interdit toute autre modification (422) et pose/retire
+     * permission archive (403), interdit toute autre modification (422) et
-     * archivedAt. Retourne true si la requete est une requete d'archivage.
+     * pose/retire archivedAt. Retourne true si la requete est une requete
+     * d'archivage.
      *
-     * @param list<string> $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<string> $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.
+        // RG-1.22 : une requete d'archivage ne modifie aucun autre champ ecrivable.
-        if ([] !== array_diff($payloadKeys, [self::ARCHIVE_FIELD])) {
+        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,
+     * RG-1.28 : la modification effective d'un champ comptable exige
-     * sinon 403 sur l'ensemble du payload (mode strict, pas de filtrage
+     * accounting.manage, sinon 403 sur l'ensemble du payload (mode strict, pas
-     * silencieux). Le message precise le premier champ fautif.
+     * 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
-     * @param list<string> $payloadKeys
+     * 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<string>
+     */
+    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<string, mixed>
+     */
+    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<string>
+     */
+    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 ;
+     * brut), filtrage compris. Pour un PATCH merge-patch+json, ce sont les seuls
-     * c'est ce qui permet le gating par onglet (RG-1.22 / RG-1.28) et le
+     * champs modifies.
-     * declenchement conditionnel de RG-1.04.
      *
      * @return list<string>
      */

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 <select> cote front).
         if (!$this->pagination->isEnabled($operation, $context)) {
-            /** @var list<Client> $result */
+            // @var list<Client> $result
             return $qb->getQuery()->getResult();
         }
 
@@ -144,8 +146,13 @@ final class ClientProvider implements ProviderInterface
             return;
         }
 
-        $sub = $this->repository->createQueryBuilder('c2')
+        // Sous-requete construite via l'EntityManager (et non
+        // $repository->createQueryBuilder()) : createQueryBuilder() n'est pas
+        // declaree sur ClientRepositoryInterface, l'appeler exposerait un detail
+        // d'implementation Doctrine hors du contrat (fuite d'abstraction).
+        $sub = $this->em->createQueryBuilder()
             ->select('c2.id')
+            ->from(Client::class, 'c2')
             ->join('c2.categories', 'cat2')
             ->join('cat2.categoryType', 'ct2')
             ->where('ct2.code = :categoryType')

tests/Module/Commercial/Unit/CategoryReferenceDenormalizerTest.php

@@ -0,0 +1,62 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Module\Commercial\Unit;
+
+use ApiPlatform\Metadata\IriConverterInterface;
+use App\Module\Commercial\Infrastructure\ApiPlatform\Serializer\CategoryReferenceDenormalizer;
+use App\Shared\Domain\Contract\CategoryInterface;
+use PHPUnit\Framework\TestCase;
+use stdClass;
+use Symfony\Component\Serializer\Exception\UnexpectedValueException;
+
+/**
+ * Tests unitaires du CategoryReferenceDenormalizer : resolution d'un IRI vers
+ * une Category concrete, et rejet explicite d'un IRI pointant sur une autre
+ * ressource (au lieu d'un null silencieux qui perdrait la reference).
+ *
+ * @internal
+ */
+final class CategoryReferenceDenormalizerTest extends TestCase
+{
+    public function testResolvesCategoryIri(): void
+    {
+        $category     = $this->createStub(CategoryInterface::class);
+        $iriConverter = $this->createMock(IriConverterInterface::class);
+        $iriConverter->method('getResourceFromIri')->willReturn($category);
+
+        $denormalizer = new CategoryReferenceDenormalizer($iriConverter);
+
+        self::assertSame(
+            $category,
+            $denormalizer->denormalize('/api/categories/1', CategoryInterface::class),
+        );
+    }
+
+    public function testRejectsIriOfWrongType(): void
+    {
+        // Bug review ERP-55 : un IRI syntaxiquement valide mais pointant sur une
+        // autre ressource (ex: /api/clients/5) doit lever une exception au lieu
+        // d'etre silencieusement ignore.
+        $iriConverter = $this->createMock(IriConverterInterface::class);
+        $iriConverter->method('getResourceFromIri')->willReturn(new stdClass());
+
+        $denormalizer = new CategoryReferenceDenormalizer($iriConverter);
+
+        $this->expectException(UnexpectedValueException::class);
+        $denormalizer->denormalize('/api/clients/5', CategoryInterface::class);
+    }
+
+    public function testReturnsNullForEmptyData(): void
+    {
+        // Valeur vide deleguee par l'ArrayDenormalizer : aucun appel a
+        // l'IriConverter, retour null.
+        $iriConverter = $this->createMock(IriConverterInterface::class);
+        $iriConverter->expects(self::never())->method('getResourceFromIri');
+
+        $denormalizer = new CategoryReferenceDenormalizer($iriConverter);
+
+        self::assertNull($denormalizer->denormalize('', CategoryInterface::class));
+    }
+}

tests/Module/Commercial/Unit/ClientProcessorTest.php

@@ -16,6 +16,8 @@ use App\Module\Commercial\Domain\Entity\PaymentType;
 use App\Module\Commercial\Infrastructure\ApiPlatform\State\Processor\ClientProcessor;
 use App\Shared\Domain\Contract\BusinessRoleAwareInterface;
 use App\Shared\Domain\Security\BusinessRoles;
+use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\ORM\UnitOfWork;
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\HttpFoundation\Request;
@@ -36,45 +38,126 @@ final class ClientProcessorTest extends TestCase
 {
     public function testAccountingFieldWithoutPermissionIsForbidden(): void
     {
-        // RG-1.28 : un champ comptable sans accounting.manage -> 403.
+        // RG-1.28 : la modification effective d'un champ comptable sans
+        // accounting.manage -> 403. En creation (POST), positionner siren est un
+        // changement vs l'etat persiste vide.
+        $client = $this->minimalClient();
+        $client->setSiren('123456789');
+
         $processor = $this->makeProcessor(granted: [], payload: ['siren' => '123456789']);
 
         $this->expectException(AccessDeniedHttpException::class);
-        $processor->process($this->minimalClient(), $this->operation());
+        $processor->process($client, $this->operation());
     }
 
     public function testStrictMixWithAccountingFieldIsForbidden(): void
     {
         // RG-1.28 : payload mixant main + accounting sans la permission -> 403
         // sur l'ensemble (pas de filtrage silencieux).
+        $client = $this->minimalClient();
+        $client->setCompanyName('X');
+        $client->setSiren('123456789');
+
         $processor = $this->makeProcessor(
             granted: [],
             payload: ['companyName' => 'X', 'siren' => '123456789'],
         );
 
         $this->expectException(AccessDeniedHttpException::class);
-        $processor->process($this->minimalClient(), $this->operation());
+        $processor->process($client, $this->operation());
     }
 
     public function testArchiveWithoutPermissionIsForbidden(): void
     {
-        // RG-1.22 : isArchived sans la permission archive -> 403.
+        // RG-1.22 : basculer isArchived sans la permission archive -> 403.
-        $processor = $this->makeProcessor(granted: [], payload: ['isArchived' => true]);
+        $client = $this->minimalClient();
+        $client->setIsArchived(true);
+
+        $processor = $this->makeProcessor(
+            granted: [],
+            payload: ['isArchived' => true],
+            managed: true,
+            originalData: ['isArchived' => false],
+        );
 
         $this->expectException(AccessDeniedHttpException::class);
-        $processor->process($this->minimalClient(), $this->operation());
+        $processor->process($client, $this->operation());
     }
 
     public function testArchiveWithOtherFieldIsUnprocessable(): void
     {
         // RG-1.22 : une requete d'archivage ne modifie aucun autre champ.
+        $client = $this->minimalClient();
+        $client->setIsArchived(true);
+        $client->setCompanyName('X');
+
         $processor = $this->makeProcessor(
             granted: ['commercial.clients.archive'],
             payload: ['isArchived' => true, 'companyName' => 'X'],
+            managed: true,
+            originalData: ['isArchived' => false],
         );
 
         $this->expectException(UnprocessableEntityHttpException::class);
-        $processor->process($this->minimalClient(), $this->operation());
+        $processor->process($client, $this->operation());
+    }
+
+    public function testPostWithIsArchivedFalseIsNotGated(): void
+    {
+        // Bug review ERP-55 : un POST renvoyant isArchived:false (valeur par
+        // defaut) ne doit declencher ni 403 (archive) ni 422, meme sans
+        // permission. L'entite n'est pas encore geree par l'ORM.
+        $client = $this->minimalClient(); // isArchived = false par defaut
+
+        $processor = $this->makeProcessor(
+            granted: [],
+            payload: ['companyName' => 'Test Co', 'isArchived' => false],
+            managed: false,
+        );
+
+        self::assertInstanceOf(Client::class, $processor->process($client, $this->operation()));
+    }
+
+    public function testFullRepresentationPatchWithUnchangedArchiveIsNotGated(): void
+    {
+        // Bug review ERP-55 : un PATCH « representation complete » renvoyant
+        // isArchived inchange + des cles JSON-LD (@id, @context) ne doit pas etre
+        // gate (ni 403 archive ni 422), meme sans permission.
+        $client = $this->minimalClient(); // isArchived = false (inchange)
+
+        $processor = $this->makeProcessor(
+            granted: [],
+            payload: [
+                '@id'         => '/api/clients/1',
+                '@context'    => '/api/contexts/Client',
+                'companyName' => 'Test Co',
+                'isArchived'  => false,
+            ],
+            managed: true,
+            originalData: ['isArchived' => false],
+        );
+
+        self::assertInstanceOf(Client::class, $processor->process($client, $this->operation()));
+    }
+
+    public function testUnchangedAccountingFieldOnPatchIsNotGated(): void
+    {
+        // Bug review ERP-55 : renvoyer un champ comptable a sa valeur persistee
+        // (PATCH representation complete) ne change rien -> pas d'exigence
+        // accounting.manage.
+        $client = $this->minimalClient();
+        $client->setSiren('123456789'); // identique a l'etat persiste
+
+        $processor = $this->makeProcessor(
+            granted: [],
+            payload: ['companyName' => 'Test Co', 'siren' => '123456789'],
+            managed: true,
+            // getOriginalEntityData renvoie tous les champs mappes d'une entite
+            // geree : isArchived (non-null) y figure toujours.
+            originalData: ['siren' => '123456789', 'isArchived' => false],
+        );
+
+        self::assertInstanceOf(Client::class, $processor->process($client, $this->operation()));
     }
 
     public function testVirementWithoutBankIsUnprocessable(): void
@@ -170,11 +253,18 @@ final class ClientProcessorTest extends TestCase
     }
 
     /**
-     * @param list<string>         $granted Permissions accordees a l'utilisateur courant
+     * @param list<string>         $granted      Permissions accordees a l'utilisateur courant
-     * @param array<string, mixed> $payload Corps JSON simule de la requete
+     * @param array<string, mixed> $payload      Corps JSON simule de la requete
+     * @param bool                 $managed      true = entite geree par l'ORM (PATCH), false = creation (POST)
+     * @param array<string, mixed> $originalData Etat persiste simule (getOriginalEntityData) pour la detection de changement
      */
-    private function makeProcessor(array $granted, array $payload, ?UserInterface $user = null): ClientProcessor
+    private function makeProcessor(
-    {
+        array $granted,
+        array $payload,
+        ?UserInterface $user = null,
+        bool $managed = false,
+        array $originalData = [],
+    ): ClientProcessor {
         $persist = new class implements ProcessorInterface {
             public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed
             {
@@ -191,12 +281,23 @@ final class ClientProcessorTest extends TestCase
         $requestStack = new RequestStack();
         $requestStack->push(new Request([], [], [], [], [], [], json_encode($payload, JSON_THROW_ON_ERROR)));
 
+        // EntityManager stub : contains() distingue creation (POST) et mise a
+        // jour (PATCH) ; getOriginalEntityData() fournit l'etat persiste compare
+        // par le gating (RG-1.22 / RG-1.28).
+        $uow = $this->createMock(UnitOfWork::class);
+        $uow->method('getOriginalEntityData')->willReturn($originalData);
+
+        $em = $this->createMock(EntityManagerInterface::class);
+        $em->method('contains')->willReturn($managed);
+        $em->method('getUnitOfWork')->willReturn($uow);
+
         return new ClientProcessor(
             $persist,
             new ClientFieldNormalizer(),
             new ClientInformationCompletenessValidator(),
             $security,
             $requestStack,
+            $em,
         );
     }