diff --git a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php index ee3a1a9..de2017b 100644 --- a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php +++ b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php @@ -16,6 +16,7 @@ use App\Shared\Domain\Security\BusinessRoles; use DateTimeImmutable; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\PersistentCollection; use JsonException; use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\DependencyInjection\Attribute\Autowire; @@ -249,8 +250,10 @@ final class ClientProcessor implements ProcessorInterface * relations ManyToOne distributor/broker par identite via l'identity map). * * Cas particulier `categories` (M2M) : non trace par getOriginalEntityData, - * on se rabat sur sa presence explicite dans le payload (modifier les - * categories = action metier exigeant manage). + * compare par valeur via le snapshot de la PersistentCollection (cf. + * categoriesChanged) — la simple presence dans le payload ne suffit pas, sous + * peine de 403 parasite sur un PATCH representation complete reincluant des + * categories inchangees. * * @return list */ @@ -282,13 +285,69 @@ final class ClientProcessor implements ProcessorInterface } } - if (in_array('categories', $this->writablePayloadKeys(), true)) { + if ($this->categoriesChanged($data)) { $changed[] = 'categories'; } return $changed; } + /** + * Vrai si l'ensemble des categories (M2M) differe reellement de l'etat + * persiste. La collection n'etant pas tracee par getOriginalEntityData, on + * compare par identifiants (independamment de l'ordre) le snapshot de la + * PersistentCollection (etat charge depuis la base) a l'etat courant (apres + * application du payload). Symetrique de changedAccountingFields : seul un + * changement effectif compte, pas la simple presence dans le payload. + * + * - POST / entite non geree : fournir des categories est un acte metier + * (comportement historique conserve) — branche defensive, guardManage ne + * s'execute de toute facon que sur entite geree. + * - categories absent du payload (PATCH partiel) : aucun changement. + */ + private function categoriesChanged(Client $data): bool + { + if (!$this->em->contains($data)) { + return true; + } + + if (!in_array('categories', $this->payloadKeys(), true)) { + return false; + } + + $collection = $data->getCategories(); + + // Hors PersistentCollection (cas limite hors flux PATCH reel) : faute + // d'etat persiste comparable, on se rabat sur la presence payload. + if (!$collection instanceof PersistentCollection) { + return true; + } + + return $this->categoryIdSet($collection->toArray()) + !== $this->categoryIdSet($collection->getSnapshot()); + } + + /** + * Ensemble trie des identifiants d'une liste de categories — pour une + * comparaison par valeur independante de l'ordre. + * + * @param array $categories + * + * @return list + */ + private function categoryIdSet(array $categories): array + { + $ids = array_map( + static fn (object $category): mixed => method_exists($category, 'getId') + ? $category->getId() + : spl_object_id($category), + array_values($categories), + ); + sort($ids); + + return $ids; + } + /** * Champs comptables dont la valeur courante differe de l'etat persiste. Les * relations (tvaMode, paymentDelay, paymentType, bank) sont comparees par diff --git a/tests/Module/Commercial/Api/ClientRBACMatrixTest.php b/tests/Module/Commercial/Api/ClientRBACMatrixTest.php index 06107f5..772c336 100644 --- a/tests/Module/Commercial/Api/ClientRBACMatrixTest.php +++ b/tests/Module/Commercial/Api/ClientRBACMatrixTest.php @@ -218,6 +218,60 @@ final class ClientRBACMatrixTest extends AbstractCommercialApiTestCase self::assertResponseStatusCodeSame(201); } + public function testComptaFullRepresentationPatchWithUnchangedCategoriesIsNotForbidden(): void + { + // FIX review MR #40 : un Compta (accounting.manage, PAS manage) faisant un + // PATCH representation complete de l'onglet Comptabilite et reincluant ses + // categories INCHANGEES ne doit PAS prendre de 403. guardManage compare + // desormais les categories par valeur (et non par simple presence) : seul + // l'onglet Comptabilite change ici -> 200. + $seed = $this->seedClient('Compta Cat Unchanged'); + $category = $seed->getCategories()->first(); + self::assertNotFalse($category); + $catId = $category->getId(); + $client = $this->authAs('compta'); + + $client->request('PATCH', '/api/clients/'.$seed->getId(), [ + 'headers' => ['Content-Type' => self::MERGE], + 'json' => [ + 'siren' => '123456789', + 'categories' => ['/api/categories/'.$catId], + ], + ]); + self::assertResponseStatusCodeSame(200); + } + + public function testComptaChangingCategoriesIsForbidden(): void + { + // Non-regression : si le Compta change REELLEMENT l'ensemble des + // categories (sans manage) -> 403 via guardManage. La comparaison par + // valeur detecte bien le changement. + $seed = $this->seedClient('Compta Cat Change'); + $newCat = $this->createCategory('SECTEUR'); + $client = $this->authAs('compta'); + + $client->request('PATCH', '/api/clients/'.$seed->getId(), [ + 'headers' => ['Content-Type' => self::MERGE], + 'json' => ['categories' => ['/api/categories/'.$newCat->getId()]], + ]); + self::assertResponseStatusCodeSame(403); + } + + public function testBureauChangingCategoriesIsAllowed(): void + { + // Non-regression : un role porteur de `manage` (Bureau) peut changer les + // categories -> 200. + $seed = $this->seedClient('Bureau Cat Change'); + $newCat = $this->createCategory('SECTEUR'); + $client = $this->authAs('bureau'); + + $client->request('PATCH', '/api/clients/'.$seed->getId(), [ + 'headers' => ['Content-Type' => self::MERGE], + 'json' => ['categories' => ['/api/categories/'.$newCat->getId()]], + ]); + self::assertResponseStatusCodeSame(200); + } + private function authAs(string $role): Client { return $this->authenticatedClient($role, self::PWD);