From 9d98b8ffc69400d52e0871b25b1df901c0a60219 Mon Sep 17 00:00:00 2001 From: Maxiloud <61539540+maxime-urbanski@users.noreply.github.com> Date: Fri, 31 Oct 2025 16:35:20 +0100 Subject: [PATCH] fix: prevent empty phone numbers from being accepted when updating user information (#774) --- .../Security/AccountCreateController.php | 73 ++++++++++++------- .../User/Account/EditProfileAction.php | 34 +++++++-- .../User/Account/EditProfileActionTest.php | 4 +- .../security/account_create/validators.fr.xlf | 10 +++ 4 files changed, 85 insertions(+), 36 deletions(-) diff --git a/src/Controller/Security/AccountCreateController.php b/src/Controller/Security/AccountCreateController.php index 371708a..91a85b8 100644 --- a/src/Controller/Security/AccountCreateController.php +++ b/src/Controller/Security/AccountCreateController.php @@ -21,12 +21,15 @@ use App\MessageBus\CommandBus; use App\MessageBus\QueryBus; use App\MessageHandler\Command\Security\AccountCreateStep1CommandHandler; use App\Repository\ConfigurationRepository; +use libphonenumber\PhoneNumber; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\SecurityBundle\Security; +use Symfony\Component\Form\FormError; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Messenger\Exception\HandlerFailedException; use Symfony\Component\Routing\Annotation\Route; +use Symfony\Contracts\Translation\TranslatorInterface; /** * @see AccountCreateActionStep1Test @@ -44,6 +47,7 @@ final class AccountCreateController extends AbstractController private readonly CommandBus $commandBus, private readonly Security $security, private readonly ConfigurationRepository $configurationRepository, + private readonly TranslatorInterface $translator, ) { $this->i18nPrefix = $this->getI18nPrefix(); } @@ -103,40 +107,55 @@ final class AccountCreateController extends AbstractController $configuration = $this->configurationRepository->getInstanceConfigurationOrCreate(); // nominal case: user found and token not expired $form = $this->createForm(AccountCreateStep2FormType::class, $user->setStep2Defaults())->handleRequest($request); - if ($form->isSubmitted() && $form->isValid()) { - /** @var User $user */ - $user = $form->getData(); - $user->setType(UserType::USER); - $this->commandBus->dispatch(new AccountCreateStep2Command($user)); - $this->security->login($user); // auto-log the user + if ($form->isSubmitted()) { + $phone = $form->get('phone')->getData() ?? ''; - // If user has pending invitations then redirect them to the first group - // found without doing the confirmation stuff, it must be done on the - // page group. - $group = $user->getMyGroupsAsInvited()->first(); - if ($group !== false) { - // If platform needs payment, redirect to payment - if ($configuration->getPaidMembership()) { - $successMessage = $this->i18nPrefix.'.step2.with_invitation.global_paid_membership.flash.success'; + if (!$phone instanceof PhoneNumber || $phone->getNationalNumber() === '') { + $form->get('phone')->addError( + new FormError($this->translator->trans('account_create.phone.empty.error', [], 'validators')) + ); + } + + if ($phone instanceof PhoneNumber && \strlen($phone->getNationalNumber() ?? '') < 8) { + $form->get('phone')->addError( + new FormError($this->translator->trans('account_create.phone.short.error', [], 'validators')) + ); + } + if ($form->isValid()) { + /** @var User $user */ + $user = $form->getData(); + $user->setType(UserType::USER); + $this->commandBus->dispatch(new AccountCreateStep2Command($user)); + $this->security->login($user); // auto-log the user + + // If user has pending invitations then redirect them to the first group + // found without doing the confirmation stuff, it must be done on the + // page group. + $group = $user->getMyGroupsAsInvited()->first(); + if ($group !== false) { + // If platform needs payment, redirect to payment + if ($configuration->getPaidMembership()) { + $successMessage = $this->i18nPrefix.'.step2.with_invitation.global_paid_membership.flash.success'; + $this->addFlashSuccess($successMessage); + + return $this->redirectToRoute('redirect_to_payment'); + } + $successMessage = $this->i18nPrefix.'.step2.with_invitation.flash.success'; $this->addFlashSuccess($successMessage); - return $this->redirectToRoute('redirect_to_payment'); + return $this->redirectToRoute('app_group_show_logged', $group->getRoutingParameters()); } - $successMessage = $this->i18nPrefix.'.step2.with_invitation.flash.success'; + + if ($configuration->getPaidMembership()) { + $successMessage = $this->i18nPrefix.'.step2.global_paid_membership.flash.success'; + } else { + $successMessage = $this->i18nPrefix.'.step2.flash.success'; + } + // otherwise go to the address form $this->addFlashSuccess($successMessage); - return $this->redirectToRoute('app_group_show_logged', $group->getRoutingParameters()); + return $this->redirectToRoute(MyAccountAction::ROUTE); } - - if ($configuration->getPaidMembership()) { - $successMessage = $this->i18nPrefix.'.step2.global_paid_membership.flash.success'; - } else { - $successMessage = $this->i18nPrefix.'.step2.flash.success'; - } - // otherwise go to the address form - $this->addFlashSuccess($successMessage); - - return $this->redirectToRoute(MyAccountAction::ROUTE); } return $this->render('pages/register/step2.html.twig', compact('form', 'user')); diff --git a/src/Controller/User/Account/EditProfileAction.php b/src/Controller/User/Account/EditProfileAction.php index 9380580..8a0613a 100644 --- a/src/Controller/User/Account/EditProfileAction.php +++ b/src/Controller/User/Account/EditProfileAction.php @@ -13,7 +13,9 @@ use App\Form\Type\User\EditProfileFormType; use App\Repository\UserRepository; use App\Tests\Functional\Controller\User\Account\EditProfileActionTest; use Doctrine\ORM\EntityManagerInterface; +use libphonenumber\PhoneNumber; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; +use Symfony\Component\Form\FormError; use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -21,6 +23,7 @@ use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter; use Symfony\Component\Security\Http\Attribute\CurrentUser; use Symfony\Component\Security\Http\Attribute\IsGranted; +use Symfony\Contracts\Translation\TranslatorInterface; /** * @see EditProfileActionTest @@ -34,6 +37,7 @@ final class EditProfileAction extends AbstractController private readonly UserRepository $userRepository, private readonly UserManager $userManager, private readonly EntityManagerInterface $entityManager, + private readonly TranslatorInterface $translator, ) { } @@ -45,14 +49,30 @@ final class EditProfileAction extends AbstractController public function __invoke(Request $request, #[CurrentUser] User $user): Response { $form = $this->createForm(EditProfileFormType::class, $user)->handleRequest($request); - if ($form->isSubmitted() && $form->isValid()) { - /** @var UploadedFile|null $avatar */ - $avatar = $form->get('avatar')->getData(); - $this->userManager->upload($avatar, $user); - $this->userRepository->save($user, true); - $this->addFlashSuccess($this->getI18nPrefix().'.flash.success'); + if ($form->isSubmitted()) { + $phone = $form->get('phone')->getData() ?? ''; - return $this->redirectToRoute(MyAccountAction::ROUTE); + if (!$phone instanceof PhoneNumber || $phone->getNationalNumber() === '') { + $form->get('phone')->addError( + new FormError($this->translator->trans('account_create.phone.empty.error', [], 'validators')) + ); + } + + if ($phone instanceof PhoneNumber && \strlen($phone->getNationalNumber() ?? '') < 8) { + $form->get('phone')->addError( + new FormError($this->translator->trans('account_create.phone.short.error', [], 'validators')) + ); + } + + if ($form->isValid()) { + /** @var UploadedFile|null $avatar */ + $avatar = $form->get('avatar')->getData(); + $this->userManager->upload($avatar, $user); + $this->userRepository->save($user, true); + $this->addFlashSuccess($this->getI18nPrefix().'.flash.success'); + + return $this->redirectToRoute(MyAccountAction::ROUTE); + } } // In case of error, we must reload the original firstname (to display it in navbar) diff --git a/tests/Functional/Controller/User/Account/EditProfileActionTest.php b/tests/Functional/Controller/User/Account/EditProfileActionTest.php index fcae901..518a54a 100644 --- a/tests/Functional/Controller/User/Account/EditProfileActionTest.php +++ b/tests/Functional/Controller/User/Account/EditProfileActionTest.php @@ -44,7 +44,7 @@ final class EditProfileActionTest extends WebTestCase $form->getName().'[category]' => TestReference::CATEGORY_OBJECT_1, $form->getName().'[description]' => 'description test', $form->getName().'[phone][country]' => 'FR', - $form->getName().'[phone][number]' => '', + $form->getName().'[phone][number]' => '634563424', $form->getName().'[smsNotifications]' => false, ]); @@ -53,7 +53,7 @@ final class EditProfileActionTest extends WebTestCase /** @var User $editedUser */ $editedUser = $repo->find(TestReference::USER_16); - self::assertNull($editedUser->getPhoneNumber()); + self::assertNotNull($editedUser->getPhoneNumber()); self::assertResponseRedirects(); $client->followRedirect(); diff --git a/translations/security/account_create/validators.fr.xlf b/translations/security/account_create/validators.fr.xlf index 5229d73..1c12cf8 100644 --- a/translations/security/account_create/validators.fr.xlf +++ b/translations/security/account_create/validators.fr.xlf @@ -26,6 +26,16 @@ Le nom du lieu est obligatoire + + account_create.phone.empty.error + Le numéro de téléphone est obligatoire + + + + account_create.phone.short.error + Veuillez indiquer un numéro de téléphone valide + +