From 09e1d7884a5bcbe3586066799ea2e8188e07b055 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Mon, 1 Jun 2026 22:26:49 +0200 Subject: [PATCH] feat(commercial) : enforce RG-1.04 completeness for commerciale role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - RG-1.04 durcie : pour une Commerciale, la completude de l'onglet Information est exigee sur POST et sur tout PATCH, independamment des champs envoyes (suppression de la condition d'intersection dans validateInformationCompleteness). - Onglet Comptabilite editable par Compta : security du Patch /clients/{id} elargie a `manage` OU `accounting.manage` ; nouveau guardManage (ClientProcessor, mode strict RG-1.28) qui refuse a un porteur non-`manage` de modifier les onglets principal / Information -> 403. Compta reste donc cantonne a la Comptabilite. - Spec § 7 RG-1.04 amendee (+ consequence POST 422) + docblock du validator. - Tests unitaires ClientProcessor : guardManage (Compta accounting-only -> 200, champ metier -> 403) + RG-1.04 durcie hors onglet Information. --- docs/specs/M1-clients/spec-back.md | 3 +- ...ClientInformationCompletenessValidator.php | 18 ++- .../Commercial/Domain/Entity/Client.php | 12 +- .../State/Processor/ClientProcessor.php | 119 +++++++++++++++--- .../Commercial/Unit/ClientProcessorTest.php | 105 +++++++++++++++- 5 files changed, 226 insertions(+), 31 deletions(-) diff --git a/docs/specs/M1-clients/spec-back.md b/docs/specs/M1-clients/spec-back.md index cd7359a..3fa11b9 100644 --- a/docs/specs/M1-clients/spec-back.md +++ b/docs/specs/M1-clients/spec-back.md @@ -885,7 +885,8 @@ Cf. § 2.6. Pattern Shared standard. ### Onglet Information -- **RG-1.04** : Pour un utilisateur portant le rôle métier **Commerciale**, **tous** les champs de l'onglet Information (`description`, `competitors`, `foundedAt`, `employeesCount`, `revenueAmount`, `directorName`, `profitAmount`) deviennent obligatoires lors d'un PATCH sur le groupe `client:write:information`. Pour les autres rôles, ces champs restent optionnels. Implémenté via un validator custom `ClientInformationCompletenessValidator` invoqué par le `ClientProcessor` quand le user porte le rôle Commerciale. +- **RG-1.04** _(durcie — ERP-74)_ : Pour un utilisateur portant le rôle métier **Commerciale**, **tous** les champs de l'onglet Information (`description`, `competitors`, `foundedAt`, `employeesCount`, `revenueAmount`, `directorName`, `profitAmount`) sont obligatoires sur **POST et sur tout PATCH**, **indépendamment des champs réellement envoyés** (la condition d'intersection avec `client:write:information` a été retirée). Pour les autres rôles, ces champs restent optionnels. Implémenté via un validator custom `ClientInformationCompletenessValidator` invoqué systématiquement par le `ClientProcessor` quand le user porte le rôle Commerciale. + - **Conséquence** : le POST n'exposant que le groupe `client:write:main`, l'onglet Information n'y est pas renseignable → une Commerciale obtient **422** sur tout POST (cf. § 8.1). La complétude se fait donc via les PATCH `client:write:information` ultérieurs. Un Admin (non gaté) crée normalement (201). ### Onglet Contact diff --git a/src/Module/Commercial/Application/Validator/ClientInformationCompletenessValidator.php b/src/Module/Commercial/Application/Validator/ClientInformationCompletenessValidator.php index 154184d..fe374fe 100644 --- a/src/Module/Commercial/Application/Validator/ClientInformationCompletenessValidator.php +++ b/src/Module/Commercial/Application/Validator/ClientInformationCompletenessValidator.php @@ -10,17 +10,15 @@ use Symfony\Component\Validator\ConstraintViolation; use Symfony\Component\Validator\ConstraintViolationList; /** - * Validator metier RG-1.04 : pour un utilisateur portant le role metier - * Commerciale, TOUS les champs de l'onglet Information deviennent obligatoires - * lors d'un PATCH touchant le groupe `client:write:information`. + * Validator metier RG-1.04 (durcie ERP-74) : pour un utilisateur portant le + * role metier Commerciale, TOUS les champs de l'onglet Information sont + * obligatoires sur POST comme sur tout PATCH, independamment des champs + * reellement envoyes. * - * Invoque par le ClientProcessor UNIQUEMENT quand les deux conditions sont - * reunies (role Commerciale + payload touchant l'onglet Information). Pour les - * autres roles, ces champs restent optionnels — le validator n'est pas appele. - * - * Tant qu'aucun user ne porte le role `commerciale` (seede par ERP-74, - * cf. App\Shared\Domain\Security\BusinessRoles::COMMERCIALE), cette regle reste - * DORMANTE : aucun appelant ne la declenche. + * Invoque par le ClientProcessor des que l'utilisateur courant porte le role + * Commerciale (plus de condition d'intersection avec l'onglet Information). + * Pour les autres roles, ces champs restent optionnels — le validator n'est + * pas appele. * * Leve une ValidationException (HTTP 422) listant chaque champ manquant, par * coherence avec les violations Symfony rendues par API Platform. diff --git a/src/Module/Commercial/Domain/Entity/Client.php b/src/Module/Commercial/Domain/Entity/Client.php index 6240975..bf1d22e 100644 --- a/src/Module/Commercial/Domain/Entity/Client.php +++ b/src/Module/Commercial/Domain/Entity/Client.php @@ -83,11 +83,19 @@ use Symfony\Component\Validator\Constraints as Assert; processor: ClientProcessor::class, ), new Patch( - security: "is_granted('commercial.clients.manage')", + // Security elargie (ERP-74) : `manage` OU `accounting.manage`. Le + // role Compta n'a pas `manage` mais doit pouvoir editer l'onglet + // Comptabilite d'un client existant (§ 2.7). Le ClientProcessor + // re-gate ensuite onglet par onglet : + // - champs accounting -> accounting.manage (guardAccounting, RG-1.28) ; + // - champs main/information -> manage (guardManage : empeche Compta + // d'editer les autres onglets) ; + // - isArchived -> archive (guardArchive, RG-1.22). + security: "is_granted('commercial.clients.manage') or is_granted('commercial.clients.accounting.manage')", // Le ClientProcessor inspecte les champs reellement envoyes pour // autoriser/refuser onglet par onglet (RG-1.22 / RG-1.28) : les // champs accounting exigent accounting.manage, isArchived exige - // archive. + // archive, le reste (main/information) exige manage. normalizationContext: ['groups' => ['client:read', 'default:read']], denormalizationContext: ['groups' => [ 'client:write:main', diff --git a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php index 5f665f1..ee3a1a9 100644 --- a/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php +++ b/src/Module/Commercial/Infrastructure/ApiPlatform/State/Processor/ClientProcessor.php @@ -31,16 +31,19 @@ use Symfony\Component\Validator\ConstraintViolationList; * § 2.9 / § 4.3 / § 4.4 + RG-1.01 a RG-1.28. * * Sequence (POST / PATCH) : - * 1. Autorisation additionnelle par groupe d'onglet (le `security` de - * l'operation a deja exige commercial.clients.manage) : - * - champ comptable dans le payload -> exige accounting.manage (RG-1.28, 403) ; + * 1. Autorisation additionnelle par groupe d'onglet. La security d'operation + * du PATCH a ete elargie (ERP-74) a `manage` OU `accounting.manage` pour + * laisser entrer le role Compta ; ce processor re-gate alors finement : + * - champ comptable modifie dans le payload -> exige accounting.manage (RG-1.28, 403) ; + * - champ main/information modifie -> exige manage (guardManage, 403) : empeche + * Compta d'editer un autre onglet que la Comptabilite (§ 2.7) ; * - champ isArchived dans le payload -> exige archive (RG-1.22, 403) et * interdit toute autre modification dans la meme requete (RG-1.22, 422). * 2. Normalisation serveur (RG-1.18 a 1.21) via ClientFieldNormalizer. * 3. Regles metier : RG-1.01 (prenom/nom), RG-1.03 (distributor/broker * exclusifs + type de categorie), RG-1.12 (Virement -> banque), - * RG-1.13 (LCR -> >= 1 RIB), RG-1.04 (completude Information pour le role - * Commerciale). + * RG-1.13 (LCR -> >= 1 RIB), RG-1.04 (completude Information exigee sur POST + * et tout PATCH pour le role Commerciale). * 4. Pose / retrait de archivedAt (RG-1.22 true=now, RG-1.23 false=null). * 5. Persistance via le persist_processor Doctrine, avec traduction des * collisions d'unicite en 409 (RG-1.16 doublon de nom ; RG-1.23 conflit de @@ -75,6 +78,7 @@ final class ClientProcessor implements ProcessorInterface /** Champ d'archivage (groupe client:write:archive). */ private const string ARCHIVE_FIELD = 'isArchived'; + private const string PERM_MANAGE = 'commercial.clients.manage'; private const string PERM_ACCOUNTING_MANAGE = 'commercial.clients.accounting.manage'; private const string PERM_ARCHIVE = 'commercial.clients.archive'; @@ -101,10 +105,15 @@ final class ClientProcessor implements ProcessorInterface $this->normalize($data); + // guardManage apres normalize : la comparaison « change vs etat + // persiste » des champs texte (companyName, email...) se fait sur des + // valeurs normalisees des deux cotes (l'etat persiste l'a deja ete). + $this->guardManage($data); + $this->validateMainContact($data); $this->validateDistributorBroker($data); $this->validateAccountingConsistency($data); - $this->validateInformationCompleteness($data, $writableKeys); + $this->validateInformationCompleteness($data); try { return $this->persistProcessor->process($data, $operation, $uriVariables, $context); @@ -199,6 +208,87 @@ final class ClientProcessor implements ProcessorInterface } } + /** + * § 2.7 / RG-1.28 (ERP-74) : la modification effective d'un champ « metier » + * (onglets principal ou Information) exige `commercial.clients.manage`. Sans + * cette permission -> 403 sur l'ensemble du payload (mode strict, miroir de + * guardAccounting). C'est ce qui empeche le role Compta — qui entre dans le + * PATCH via `accounting.manage` (security d'operation elargie) — d'editer + * autre chose que l'onglet Comptabilite. + * + * Ne s'applique qu'aux mises a jour (entite geree) : la creation (POST) est + * deja gardee par la security d'operation `manage`, donc inutile de la + * re-gater ici (et un POST par un porteur de `manage` passerait de toute + * facon). + */ + private function guardManage(Client $data): void + { + if (!$this->em->contains($data)) { + return; + } + + $changed = $this->changedBusinessFields($data); + + if ([] === $changed) { + return; + } + + if (!$this->security->isGranted(self::PERM_MANAGE)) { + throw new AccessDeniedHttpException(sprintf( + 'Le champ "%s" requiert la permission "%s".', + $changed[0], + self::PERM_MANAGE, + )); + } + } + + /** + * Champs « metier » (onglets principal + Information, hors comptabilite et + * archivage) dont la valeur courante differe de l'etat persiste. Memes + * regles de comparaison que changedAccountingFields (scalaires par valeur, + * 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). + * + * @return list + */ + private function changedBusinessFields(Client $data): array + { + $newValues = [ + 'companyName' => $data->getCompanyName(), + 'firstName' => $data->getFirstName(), + 'lastName' => $data->getLastName(), + 'phonePrimary' => $data->getPhonePrimary(), + 'phoneSecondary' => $data->getPhoneSecondary(), + 'email' => $data->getEmail(), + 'distributor' => $data->getDistributor(), + 'broker' => $data->getBroker(), + 'triageService' => $data->isTriageService(), + 'description' => $data->getDescription(), + 'competitors' => $data->getCompetitors(), + 'foundedAt' => $data->getFoundedAt(), + 'employeesCount' => $data->getEmployeesCount(), + 'revenueAmount' => $data->getRevenueAmount(), + 'directorName' => $data->getDirectorName(), + 'profitAmount' => $data->getProfitAmount(), + ]; + + $changed = []; + foreach ($newValues as $field => $newValue) { + if ($this->fieldChanged($data, $field, $newValue)) { + $changed[] = $field; + } + } + + if (in_array('categories', $this->writablePayloadKeys(), true)) { + $changed[] = 'categories'; + } + + return $changed; + } + /** * Champs comptables dont la valeur courante differe de l'etat persiste. Les * relations (tvaMode, paymentDelay, paymentType, bank) sont comparees par @@ -353,17 +443,16 @@ final class ClientProcessor implements ProcessorInterface } /** - * RG-1.04 : si l'utilisateur porte le role metier Commerciale ET que le - * payload touche l'onglet Information, tous les champs Information sont - * obligatoires. Dormant tant qu'aucun user ne porte le role `commerciale`. - * - * @param list $payloadKeys + * RG-1.04 (durcie ERP-74) : si l'utilisateur porte le role metier + * Commerciale, TOUS les champs de l'onglet Information sont obligatoires sur + * POST comme sur TOUT PATCH — independamment des champs reellement envoyes + * (plus de condition d'intersection avec INFORMATION_FIELDS). Garantit qu'un + * client cree/edite par une Commerciale ne reste jamais avec un onglet + * Information incomplet. */ - private function validateInformationCompleteness(Client $data, array $payloadKeys): void + private function validateInformationCompleteness(Client $data): void { - $touchesInformation = [] !== array_intersect($payloadKeys, self::INFORMATION_FIELDS); - - if ($touchesInformation && $this->currentUserIsCommerciale()) { + if ($this->currentUserIsCommerciale()) { $this->informationValidator->validate($data); } } diff --git a/tests/Module/Commercial/Unit/ClientProcessorTest.php b/tests/Module/Commercial/Unit/ClientProcessorTest.php index c9e848b..d6270d6 100644 --- a/tests/Module/Commercial/Unit/ClientProcessorTest.php +++ b/tests/Module/Commercial/Unit/ClientProcessorTest.php @@ -134,7 +134,17 @@ final class ClientProcessorTest extends TestCase 'isArchived' => false, ], managed: true, - originalData: ['isArchived' => false], + // Etat persiste complet (valeurs normalisees) : sans les champs + // metier, guardManage (ERP-74) les croirait modifies (companyName, + // lastName... compares a null) et leverait un 403 parasite. + originalData: [ + 'companyName' => 'TEST CO', + 'lastName' => 'Dupont', + 'phonePrimary' => '0102030405', + 'email' => 't@test.fr', + 'triageService' => false, + 'isArchived' => false, + ], ); self::assertInstanceOf(Client::class, $processor->process($client, $this->operation())); @@ -153,8 +163,69 @@ final class ClientProcessorTest extends TestCase 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], + // geree : isArchived (non-null) y figure toujours, ainsi que les + // champs metier (sinon guardManage les croirait modifies). + originalData: [ + 'siren' => '123456789', + 'companyName' => 'TEST CO', + 'lastName' => 'Dupont', + 'phonePrimary' => '0102030405', + 'email' => 't@test.fr', + 'triageService' => false, + 'isArchived' => false, + ], + ); + + self::assertInstanceOf(Client::class, $processor->process($client, $this->operation())); + } + + public function testBusinessFieldWithoutManagePermissionIsForbidden(): void + { + // ERP-74 (guardManage) : modifier un champ metier (companyName) sur un + // client existant sans `manage` -> 403, meme avec accounting.manage + // (cas Compta qui sort de son onglet). + $client = $this->minimalClient(); + $client->setCompanyName('Renamed Co'); + + $processor = $this->makeProcessor( + granted: ['commercial.clients.accounting.manage'], + payload: ['companyName' => 'Renamed Co'], + managed: true, + originalData: [ + 'companyName' => 'TEST CO', + 'lastName' => 'Dupont', + 'phonePrimary' => '0102030405', + 'email' => 't@test.fr', + 'triageService' => false, + 'isArchived' => false, + ], + ); + + $this->expectException(AccessDeniedHttpException::class); + $processor->process($client, $this->operation()); + } + + public function testAccountingOnlyPatchWithAccountingManageOnlyPasses(): void + { + // ERP-74 : Compta (accounting.manage, PAS manage) qui ne touche QUE + // l'onglet Comptabilite d'un client existant -> 200. guardManage ne + // declenche pas (aucun champ metier modifie), guardAccounting passe. + $client = $this->minimalClient(); + $client->setSiren('999999999'); + + $processor = $this->makeProcessor( + granted: ['commercial.clients.accounting.manage'], + payload: ['siren' => '999999999'], + managed: true, + originalData: [ + 'siren' => '111111111', + 'companyName' => 'TEST CO', + 'lastName' => 'Dupont', + 'phonePrimary' => '0102030405', + 'email' => 't@test.fr', + 'triageService' => false, + 'isArchived' => false, + ], ); self::assertInstanceOf(Client::class, $processor->process($client, $this->operation())); @@ -237,6 +308,34 @@ final class ClientProcessorTest extends TestCase $processor->process($client, $this->operation()); } + public function testCommercialeIncompleteInformationOnNonInformationPatchIsUnprocessable(): void + { + // RG-1.04 durcie (ERP-74) : pour une Commerciale, la completude de + // l'onglet Information est exigee meme quand le payload ne touche PAS + // l'onglet Information (ici seulement companyName). L'ancienne condition + // d'intersection avec INFORMATION_FIELDS a ete retiree. + $client = $this->minimalClient(); + $client->setCompanyName('Renamed Co'); // onglet principal uniquement, Information vide + + $processor = $this->makeProcessor( + granted: ['commercial.clients.manage'], + payload: ['companyName' => 'Renamed Co'], + user: $this->commercialeUser(), + managed: true, + originalData: [ + 'companyName' => 'TEST CO', + 'lastName' => 'Dupont', + 'phonePrimary' => '0102030405', + 'email' => 't@test.fr', + 'triageService' => false, + 'isArchived' => false, + ], + ); + + $this->expectException(ValidationException::class); + $processor->process($client, $this->operation()); + } + public function testNonCommercialeSkipsInformationCompleteness(): void { // Meme payload incomplet, mais user non-Commerciale -> aucun blocage.