From 4d455d0e7d00ccc7d306c9bc43b4c774b3f08875 Mon Sep 17 00:00:00 2001 From: Marien Fressinaud Date: Mon, 26 Aug 2024 11:01:29 +0200 Subject: [PATCH 1/3] Set default ldapIdentifier to empty string --- docs/developers/import-data.md | 2 +- ...958SetDefaultUserLdapIdentifierToEmpty.php | 50 +++++++++++++++++++ src/Entity/User.php | 5 +- src/Repository/UserRepository.php | 2 +- src/Service/DataImporter/DataImporter.php | 2 +- src/Service/UserCreator.php | 2 +- tests/Controller/PasswordsControllerTest.php | 16 +++--- 7 files changed, 65 insertions(+), 14 deletions(-) create mode 100644 migrations/Version20240826082958SetDefaultUserLdapIdentifierToEmpty.php diff --git a/docs/developers/import-data.md b/docs/developers/import-data.md index b1057409..9cb87943 100644 --- a/docs/developers/import-data.md +++ b/docs/developers/import-data.md @@ -61,7 +61,7 @@ The data is stored in a ZIP archive. It contains several files: - email: string (unique, not empty, valid email) - locale: string, optional (must be `en_GB`, or `fr_FR`) - name: string, optional (not empty, max 100 chars) - - ldapIdentifier: string or null, optional + - ldapIdentifier: string, optional - organizationId: string or null, optional (reference to an organization) - authorizations: array of, optional: - roleId: string (reference to a role) diff --git a/migrations/Version20240826082958SetDefaultUserLdapIdentifierToEmpty.php b/migrations/Version20240826082958SetDefaultUserLdapIdentifierToEmpty.php new file mode 100644 index 00000000..36770ba7 --- /dev/null +++ b/migrations/Version20240826082958SetDefaultUserLdapIdentifierToEmpty.php @@ -0,0 +1,50 @@ +connection->getDatabasePlatform(); + if ($dbPlatform instanceof PostgreSQLPlatform) { + $this->addSql("UPDATE users SET ldap_identifier = '' WHERE ldap_identifier IS NULL"); + $this->addSql('ALTER TABLE users ALTER ldap_identifier SET DEFAULT \'\''); + $this->addSql('ALTER TABLE users ALTER ldap_identifier SET NOT NULL'); + } elseif ($dbPlatform instanceof MariaDBPlatform) { + $this->addSql('UPDATE users SET ldap_identifier = "" WHERE ldap_identifier IS NULL'); + $this->addSql('ALTER TABLE users CHANGE ldap_identifier ldap_identifier VARCHAR(255) DEFAULT \'\' NOT NULL'); + } + } + + public function down(Schema $schema): void + { + $dbPlatform = $this->connection->getDatabasePlatform(); + if ($dbPlatform instanceof PostgreSQLPlatform) { + $this->addSql('ALTER TABLE "users" ALTER ldap_identifier DROP NOT NULL'); + $this->addSql('ALTER TABLE "users" ALTER ldap_identifier DROP DEFAULT'); + $this->addSql("UPDATE users SET ldap_identifier = NULL WHERE ldap_identifier = ''"); + } elseif ($dbPlatform instanceof MariaDBPlatform) { + $this->addSql('ALTER TABLE `users` CHANGE ldap_identifier ldap_identifier VARCHAR(255) DEFAULT NULL'); + $this->addSql('UPDATE users SET ldap_identifier = NULL WHERE ldap_identifier = ""'); + } + } +} diff --git a/src/Entity/User.php b/src/Entity/User.php index 4989dfc4..a6cca61f 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -112,7 +112,7 @@ class User implements #[ORM\JoinColumn(onDelete: 'SET NULL')] private ?Organization $organization = null; - #[ORM\Column(length: 255, nullable: true)] + #[ORM\Column(length: 255, options: ['default' => ''])] private ?string $ldapIdentifier = null; /** @var Collection */ @@ -127,6 +127,7 @@ public function __construct() { $this->password = ''; $this->locale = 'en_GB'; + $this->ldapIdentifier = ''; $this->authorizations = new ArrayCollection(); $this->teams = new ArrayCollection(); } @@ -292,7 +293,7 @@ public function setLdapIdentifier(?string $ldapIdentifier): static */ public function getAuthType(): string { - if ($this->getLdapIdentifier() === null) { + if ($this->getLdapIdentifier() === '') { return 'local'; } else { return 'ldap'; diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index f5208942..6cadd5dd 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -43,7 +43,7 @@ public function loadUserByIdentifier(string $identifier): ?User $query = $entityManager->createQuery(<<setParameter('identifier', $identifier); diff --git a/src/Service/DataImporter/DataImporter.php b/src/Service/DataImporter/DataImporter.php index ff3ac942..0bf3472a 100644 --- a/src/Service/DataImporter/DataImporter.php +++ b/src/Service/DataImporter/DataImporter.php @@ -500,7 +500,7 @@ private function processUsers(array $json): \Generator if (isset($jsonUser['locale'])) { $locale = strval($jsonUser['locale']); } - $ldapIdentifier = null; + $ldapIdentifier = ''; if (isset($jsonUser['ldapIdentifier'])) { $ldapIdentifier = strval($jsonUser['ldapIdentifier']); } diff --git a/src/Service/UserCreator.php b/src/Service/UserCreator.php index 8f0352dc..8a4f29b2 100644 --- a/src/Service/UserCreator.php +++ b/src/Service/UserCreator.php @@ -31,7 +31,7 @@ public function create( string $name = '', string $password = '', string $locale = '', - ?string $ldapIdentifier = null, + string $ldapIdentifier = '', ?Entity\Organization $organization = null, bool $flush = true, ): Entity\User { diff --git a/tests/Controller/PasswordsControllerTest.php b/tests/Controller/PasswordsControllerTest.php index 09df645d..ef4f3f14 100644 --- a/tests/Controller/PasswordsControllerTest.php +++ b/tests/Controller/PasswordsControllerTest.php @@ -53,7 +53,7 @@ public function testPostResetRedirectsAndSendsAnEmail(): void $email = 'alix@example.com'; $user = Factory\UserFactory::createOne([ 'email' => $email, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', 'resetPasswordToken' => null, ]); @@ -80,7 +80,7 @@ public function testPostResetFailsIfEmailIsUnknown(): void $email = 'alix@example.com'; $user = Factory\UserFactory::createOne([ 'email' => $email, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', 'resetPasswordToken' => null, ]); @@ -134,7 +134,7 @@ public function testPostResetFailsIfCsrfTokenIsInvalid(): void $email = 'alix@example.com'; $user = Factory\UserFactory::createOne([ 'email' => $email, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', 'resetPasswordToken' => null, ]); @@ -160,7 +160,7 @@ public function testGetEditRendersCorrectly(): void ]); $user = Factory\UserFactory::createOne([ 'resetPasswordToken' => $token, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', ]); $client->request(Request::METHOD_GET, "/passwords/{$token->getValue()}/edit"); @@ -179,7 +179,7 @@ public function testGetEditFailsIfTokenIsNotAssociatedToAUser(): void ]); $user = Factory\UserFactory::createOne([ 'resetPasswordToken' => null, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', ]); $client->catchExceptions(false); @@ -196,7 +196,7 @@ public function testGetEditFailsIfTokenIsExpired(): void ]); $user = Factory\UserFactory::createOne([ 'resetPasswordToken' => $token, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', ]); $client->catchExceptions(false); @@ -232,7 +232,7 @@ public function testPostEditChangesThePasswordAndRedirects(): void $newPassword = 'secret'; $user = Factory\UserFactory::createOne([ 'resetPasswordToken' => $token, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', 'password' => $initialPassword, ]); @@ -262,7 +262,7 @@ public function testPostEditFailsIfCsrfTokenIsInvalid(): void $newPassword = 'secret'; $user = Factory\UserFactory::createOne([ 'resetPasswordToken' => $token, - 'ldapIdentifier' => null, + 'ldapIdentifier' => '', 'password' => $initialPassword, ]); From 06b1fda49825e797ca1ed7b6d70d21db93290569 Mon Sep 17 00:00:00 2001 From: Marien Fressinaud Date: Fri, 16 Aug 2024 17:47:23 +0200 Subject: [PATCH 2/3] Refactor the users forms --- docs/developers/create-user.md | 4 +- src/Controller/UsersController.php | 220 ++++------------------ src/Entity/User.php | 2 + src/Form/Type/OrganizationType.php | 52 +++++ src/Form/UserForm.php | 95 ++++++++++ src/Service/UserCreator.php | 45 +++-- templates/users/_form.html.twig | 178 +++++++++++++++++ templates/users/edit.html.twig | 201 +------------------- templates/users/new.html.twig | 142 +------------- templates/users/show.html.twig | 2 +- tests/Controller/UsersControllerTest.php | 155 ++++++++------- translations/messages+intl-icu.en_GB.yaml | 11 +- translations/messages+intl-icu.fr_FR.yaml | 11 +- 13 files changed, 502 insertions(+), 616 deletions(-) create mode 100644 src/Form/Type/OrganizationType.php create mode 100644 src/Form/UserForm.php create mode 100644 templates/users/_form.html.twig diff --git a/docs/developers/create-user.md b/docs/developers/create-user.md index 2c345442..4cbf740d 100644 --- a/docs/developers/create-user.md +++ b/docs/developers/create-user.md @@ -27,5 +27,7 @@ public function createUser(UserCreator $userCreator) } ``` +If you already have a User entity, call `$userCreator->createUser($user)` instead. + The service automatically flushes the changes so Doctine writes the user to the database immediately. -If you want to delay the flush, you can pass `flush: false` to the method. +If you want to delay the flush, you can pass `flush: false` to the methods. diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 95f71801..cddf2263 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -6,29 +6,19 @@ namespace App\Controller; -use App\Entity\User; -use App\Repository\AuthorizationRepository; -use App\Repository\OrganizationRepository; -use App\Repository\UserRepository; -use App\Service\Sorter\AuthorizationSorter; -use App\Service\Sorter\OrganizationSorter; -use App\Service\Sorter\UserSorter; -use App\Service\UserCreator; -use App\Service\UserCreatorException; -use App\Utils\ConstraintErrorsFormatter; -use App\Utils\Time; -use Symfony\Component\DependencyInjection\Attribute\Autowire; +use App\Entity; +use App\Form; +use App\Repository; +use App\Service; +use App\Service\Sorter; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface; use Symfony\Component\Routing\Annotation\Route; -use Symfony\Component\Validator\Validator\ValidatorInterface; -use Symfony\Contracts\Translation\TranslatorInterface; class UsersController extends BaseController { #[Route('/users', name: 'users', methods: ['GET', 'HEAD'])] - public function index(UserRepository $userRepository, UserSorter $userSorter): Response + public function index(Repository\UserRepository $userRepository, Sorter\UserSorter $userSorter): Response { $this->denyAccessUnlessGranted('admin:manage:users'); @@ -41,96 +31,46 @@ public function index(UserRepository $userRepository, UserSorter $userSorter): R } #[Route('/users/new', name: 'new user', methods: ['GET', 'HEAD'])] - public function new( - OrganizationRepository $organizationRepository, - OrganizationSorter $organizationSorter, - ): Response { + public function new(): Response + { $this->denyAccessUnlessGranted('admin:manage:users'); - $organizations = $organizationRepository->findAll(); - $organizationSorter->sort($organizations); + $user = new Entity\User(); + $form = $this->createNamedForm('user', Form\UserForm::class, $user); return $this->render('users/new.html.twig', [ - 'organizations' => $organizations, - 'email' => '', - 'name' => '', - 'organizationUid' => '', + 'form' => $form, ]); } #[Route('/users/new', name: 'create user', methods: ['POST'])] - public function create( - Request $request, - OrganizationRepository $organizationRepository, - OrganizationSorter $organizationSorter, - UserCreator $userCreator, - TranslatorInterface $translator, - ): Response { + public function create(Request $request, Service\UserCreator $userCreator): Response + { $this->denyAccessUnlessGranted('admin:manage:users'); - /** @var \App\Entity\User $user */ - $user = $this->getUser(); - - /** @var string $email */ - $email = $request->request->get('email', ''); - - /** @var string $name */ - $name = $request->request->get('name', ''); - - /** @var string $password */ - $password = $request->request->get('password', ''); - - /** @var string $organizationUid */ - $organizationUid = $request->request->get('organization', ''); - - /** @var string $csrfToken */ - $csrfToken = $request->request->get('_csrf_token', ''); - - $organizations = $organizationRepository->findAll(); - $organizationSorter->sort($organizations); + $user = new Entity\User(); + $form = $this->createNamedForm('user', Form\UserForm::class, $user); + $form->handleRequest($request); - if (!$this->isCsrfTokenValid('create user', $csrfToken)) { + if (!$form->isSubmitted() || !$form->isValid()) { return $this->renderBadRequest('users/new.html.twig', [ - 'organizations' => $organizations, - 'email' => $email, - 'name' => $name, - 'organizationUid' => $organizationUid, - 'error' => $translator->trans('csrf.invalid', [], 'errors'), + 'form' => $form, ]); } - $organization = $organizationRepository->findOneBy(['uid' => $organizationUid]); - - try { - $newUser = $userCreator->create( - email: $email, - name: $name, - password: $password, - locale: $user->getLocale(), - organization: $organization, - ); - } catch (UserCreatorException $e) { - return $this->renderBadRequest('users/new.html.twig', [ - 'organizations' => $organizations, - 'email' => $email, - 'name' => $name, - 'organizationUid' => $organizationUid, - 'errors' => ConstraintErrorsFormatter::format($e->getErrors()), - ]); - } + $user = $form->getData(); + $userCreator->createUser($user); return $this->redirectToRoute('user', [ - 'uid' => $newUser->getUid(), + 'uid' => $user->getUid(), ]); } #[Route('/users/{uid:user}', name: 'user', methods: ['GET', 'HEAD'])] public function show( - User $user, - AuthorizationRepository $authorizationRepository, - AuthorizationSorter $authorizationSorter, - #[Autowire(env: 'bool:LDAP_ENABLED')] - bool $ldapEnabled, + Entity\User $user, + Repository\AuthorizationRepository $authorizationRepository, + Sorter\AuthorizationSorter $authorizationSorter, ): Response { $this->denyAccessUnlessGranted('admin:manage:users'); @@ -141,132 +81,42 @@ public function show( return $this->render('users/show.html.twig', [ 'user' => $user, - 'ldapEnabled' => $ldapEnabled, - 'managedByLdap' => $ldapEnabled && $user->getAuthType() === 'ldap', 'authorizations' => $authorizations, ]); } #[Route('/users/{uid:user}/edit', name: 'edit user', methods: ['GET', 'HEAD'])] - public function edit( - User $user, - OrganizationRepository $organizationRepository, - OrganizationSorter $organizationSorter, - #[Autowire(env: 'bool:LDAP_ENABLED')] - bool $ldapEnabled, - ): Response { + public function edit(Entity\User $user): Response + { $this->denyAccessUnlessGranted('admin:manage:users'); - $organizations = $organizationRepository->findAll(); - $organizationSorter->sort($organizations); - - $userOrganization = $user->getOrganization(); + $form = $this->createNamedForm('user', Form\UserForm::class, $user); return $this->render('users/edit.html.twig', [ 'user' => $user, - 'organizations' => $organizations, - 'email' => $user->getEmail(), - 'name' => $user->getName(), - 'organizationUid' => $userOrganization ? $userOrganization->getUid() : '', - 'ldapIdentifier' => $user->getLdapIdentifier(), - 'ldapEnabled' => $ldapEnabled, - 'managedByLdap' => $ldapEnabled && $user->getAuthType() === 'ldap', + 'form' => $form, ]); } #[Route('/users/{uid:user}/edit', name: 'update user', methods: ['POST'])] public function update( - User $user, + Entity\User $user, Request $request, - UserRepository $userRepository, - OrganizationRepository $organizationRepository, - OrganizationSorter $organizationSorter, - ValidatorInterface $validator, - TranslatorInterface $translator, - UserPasswordHasherInterface $passwordHasher, - #[Autowire(env: 'bool:LDAP_ENABLED')] - bool $ldapEnabled, + Repository\UserRepository $userRepository, ): Response { $this->denyAccessUnlessGranted('admin:manage:users'); - /** @var string $email */ - $email = $request->request->get('email', ''); - - /** @var string $name */ - $name = $request->request->get('name', ''); - - /** @var string $password */ - $password = $request->request->get('password', ''); - - /** @var string $ldapIdentifier */ - $ldapIdentifier = $request->request->get('ldapIdentifier', ''); - - if ($ldapIdentifier === '') { - $ldapIdentifier = null; - } - - /** @var string $organizationUid */ - $organizationUid = $request->request->get('organization', ''); - - /** @var string $csrfToken */ - $csrfToken = $request->request->get('_csrf_token', ''); - - $organizations = $organizationRepository->findAll(); - $organizationSorter->sort($organizations); - - $managedByLdap = $ldapEnabled && $user->getAuthType() === 'ldap'; - - if ($managedByLdap) { - // If the user is managed by LDAP, these fields cannot be changed. - $email = $user->getEmail(); - $name = $user->getName(); - $password = ''; - } - - if (!$this->isCsrfTokenValid('update user', $csrfToken)) { - return $this->renderBadRequest('users/edit.html.twig', [ - 'user' => $user, - 'organizations' => $organizations, - 'email' => $email, - 'name' => $name, - 'organizationUid' => $organizationUid, - 'ldapIdentifier' => $ldapIdentifier, - 'ldapEnabled' => $ldapEnabled, - 'managedByLdap' => $managedByLdap, - 'error' => $translator->trans('csrf.invalid', [], 'errors'), - ]); - } - - $organization = $organizationRepository->findOneBy(['uid' => $organizationUid]); - - $user->setEmail($email); - $user->setName($name); - $user->setOrganization($organization); - - if ($ldapEnabled) { - $user->setLdapIdentifier($ldapIdentifier); - } - - if ($password !== '') { - $hashedPassword = $passwordHasher->hashPassword($user, $password); - $user->setPassword($hashedPassword); - } + $form = $this->createNamedForm('user', Form\UserForm::class, $user); + $form->handleRequest($request); - $errors = $validator->validate($user); - if (count($errors) > 0) { + if (!$form->isSubmitted() || !$form->isValid()) { return $this->renderBadRequest('users/edit.html.twig', [ 'user' => $user, - 'organizations' => $organizations, - 'email' => $email, - 'name' => $name, - 'organizationUid' => $organizationUid, - 'ldapIdentifier' => $ldapIdentifier, - 'ldapEnabled' => $ldapEnabled, - 'managedByLdap' => $managedByLdap, - 'errors' => ConstraintErrorsFormatter::format($errors), + 'form' => $form, ]); } + $user = $form->getData(); $userRepository->save($user, true); return $this->redirectToRoute('user', [ diff --git a/src/Entity/User.php b/src/Entity/User.php index a6cca61f..4a8b8264 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -125,6 +125,8 @@ class User implements public function __construct() { + $this->name = ''; + $this->email = ''; $this->password = ''; $this->locale = 'en_GB'; $this->ldapIdentifier = ''; diff --git a/src/Form/Type/OrganizationType.php b/src/Form/Type/OrganizationType.php new file mode 100644 index 00000000..6ad2803a --- /dev/null +++ b/src/Form/Type/OrganizationType.php @@ -0,0 +1,52 @@ +setDefaults([ + 'class' => Entity\Organization::class, + + 'choice_loader' => function (Options $options): ChoiceLoaderInterface { + return ChoiceList::lazy( + $this, + function () { + $organizations = $this->organizationRepository->findAll(); + $this->organizationSorter->sort($organizations); + return $organizations; + }, + ); + }, + + 'choice_label' => 'name', + 'choice_value' => 'id', + ]); + } + + public function getParent(): string + { + return Type\EntityType::class; + } +} diff --git a/src/Form/UserForm.php b/src/Form/UserForm.php new file mode 100644 index 00000000..feda3178 --- /dev/null +++ b/src/Form/UserForm.php @@ -0,0 +1,95 @@ +addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void { + $form = $event->getForm(); + $user = $event->getData(); + $managedByLdap = $this->isManagedByLdap($user); + + $form->add('email', Type\EmailType::class, [ + 'empty_data' => '', + 'trim' => true, + 'disabled' => $managedByLdap, + ]); + + $form->add('name', Type\TextType::class, [ + 'empty_data' => '', + 'trim' => true, + 'disabled' => $managedByLdap, + ]); + + if ($this->ldapEnabled) { + $form->add('ldapIdentifier', Type\TextType::class, [ + 'empty_data' => '', + 'trim' => true, + 'required' => false, + ]); + } + + if (!$managedByLdap) { + if ($user->getUid() === null) { + $help = new TranslatableMessage('users.form.password.empty_prevent_login'); + } else { + $help = new TranslatableMessage('users.form.password.empty_keep_current'); + } + + $form->add('plainPassword', Type\PasswordType::class, [ + 'empty_data' => '', + 'hash_property_path' => 'password', + 'mapped' => false, + 'help' => $help, + ]); + } + }); + + $builder->add('organization', AppType\OrganizationType::class); + } + + public function buildView(FormView $view, FormInterface $form, array $options): void + { + $user = $form->getData(); + $view->vars['managedByLdap'] = $this->isManagedByLdap($user); + } + + public function configureOptions(OptionsResolver $resolver): void + { + $resolver->setDefaults([ + 'data_class' => Entity\User::class, + 'csrf_token_id' => 'user', + 'csrf_message' => 'csrf.invalid', + ]); + } + + private function isManagedByLdap(Entity\User $user): bool + { + return $this->ldapEnabled && $user->getAuthType() === 'ldap'; + } +} diff --git a/src/Service/UserCreator.php b/src/Service/UserCreator.php index 8a4f29b2..08cfa1a0 100644 --- a/src/Service/UserCreator.php +++ b/src/Service/UserCreator.php @@ -26,6 +26,31 @@ public function __construct( ) { } + public function createUser(Entity\User $user, bool $flush = true): void + { + $this->userRepository->save($user, $flush); + + $defaultRole = $this->roleRepository->findDefault(); + if ($defaultRole) { + $organization = $user->getOrganization(); + + if ($organization) { + $authorizationOrganization = $organization; + } else { + $emailDomain = Utils\Email::extractDomain($user->getEmail()); + $authorizationOrganization = $this->organizationRepository->findOneByDomainOrDefault($emailDomain); + } + + if ($authorizationOrganization) { + $this->authorizationRepository->grant( + $user, + $defaultRole, + $authorizationOrganization, + ); + } + } + } + public function create( string $email, string $name = '', @@ -56,25 +81,7 @@ public function create( throw new UserCreatorException($errors); } - $this->userRepository->save($user, $flush); - - $defaultRole = $this->roleRepository->findDefault(); - if ($defaultRole) { - if ($organization) { - $authorizationOrganization = $organization; - } else { - $emailDomain = Utils\Email::extractDomain($user->getEmail()); - $authorizationOrganization = $this->organizationRepository->findOneByDomainOrDefault($emailDomain); - } - - if ($authorizationOrganization) { - $this->authorizationRepository->grant( - $user, - $defaultRole, - $authorizationOrganization, - ); - } - } + $this->createUser($user, $flush); return $user; } diff --git a/templates/users/_form.html.twig b/templates/users/_form.html.twig new file mode 100644 index 00000000..b74e94ee --- /dev/null +++ b/templates/users/_form.html.twig @@ -0,0 +1,178 @@ +{# + # This file is part of Bileto. + # Copyright 2022-2024 Probesys + # SPDX-License-Identifier: AGPL-3.0-or-later + #} + +{{ form_start(form, { attr: { + 'class': 'form--standard', +}}) }} + {% if form.vars['managedByLdap'] %} + {{ include('alerts/_alert.html.twig', { + type: 'info', + title: 'users.form.ldap.information' | trans, + message: 'users.form.ldap.managed' | trans, + }, with_context = false) }} + {% endif %} + + {{ form_errors(form) }} + +
+ + + {{ form_errors(form.email) }} + + +
+ +
+ + + {{ form_errors(form.name) }} + + +
+ + {% if form.ldapIdentifier is defined %} +
+ + + {{ form_errors(form.ldapIdentifier) }} + + +
+ {% endif %} + + {% if form.plainPassword is defined %} +
+ + +

+ {{ form.plainPassword.vars.help | trans }} +

+ + {{ form_errors(form.plainPassword) }} + +
+ + + +
+
+ {% endif %} + +
+ + +

+ {{ 'users.form.organization_caption' | trans }} +

+ + {{ form_errors(form.organization) }} + + +
+ +
+ +
+{{ form_end(form) }} diff --git a/templates/users/edit.html.twig b/templates/users/edit.html.twig index 17154d8e..6be1c8a9 100644 --- a/templates/users/edit.html.twig +++ b/templates/users/edit.html.twig @@ -39,203 +39,10 @@

{{ 'users.edit.title' | trans }}

-
- {% if error %} - {{ include('alerts/_error.html.twig', { message: error }, with_context = false) }} - {% endif %} - - {% if managedByLdap %} - {{ include('alerts/_alert.html.twig', { - type: 'info', - title: 'users.edit.ldap.information' | trans, - message: 'users.edit.ldap.managed' | trans, - }, with_context = false) }} - {% endif %} - -
- - - {% if errors.email is defined %} - - {% endif %} - - -
- -
- - - {% if errors.name is defined %} - - {% endif %} - - -
- - {% if ldapEnabled %} - - - {% if errors.ldapIdentifier is defined %} - - {% endif %} - - - {% endif %} - - {% if not managedByLdap %} -
- - -

- {{ 'users.edit.leave_password_empty' | trans }} -

- -
- - - -
-
- {% endif %} - -
- - -

- {{ 'users.edit.organization_caption' | trans }} -

- - {% if errors.organization is defined %} - - {% endif %} - - - - {% if user.organization and not is_granted_to_user(user, 'orga:create:tickets', user.organization) %} - {{ include('alerts/_alert.html.twig', { - type: 'warning', - title: 'common.caution' | trans, - message: 'users.no_organization_authorization' | trans({ - path: path('new user authorization', { uid: user.uid, orga: user.organization.uid }), - }), - raw: true, - }) }} - {% endif %} -
- -
- -
- - -
+ {{ include('users/_form.html.twig', { + 'form': form, + 'submit_label': 'forms.save_changes' | trans, + }) }}
{% endblock %} diff --git a/templates/users/new.html.twig b/templates/users/new.html.twig index 92428fa5..5e31e9f0 100644 --- a/templates/users/new.html.twig +++ b/templates/users/new.html.twig @@ -35,144 +35,10 @@

{{ 'users.new.title' | trans }}

-
- {% if error %} - {{ include('alerts/_error.html.twig', { message: error }, with_context = false) }} - {% endif %} - -
- - - {% if errors.email is defined %} - - {% endif %} - - -
- -
- - - {% if errors.name is defined %} - - {% endif %} - - -
- -
- - -
- - - -
-
- -
- - -

- {{ 'users.new.organization_caption' | trans }} -

- - {% if errors.organization is defined %} - - {% endif %} - - -
- -
- -
- - -
+ {{ include('users/_form.html.twig', { + 'form': form, + 'submit_label': 'users.new.submit' | trans, + }) }}
{% endblock %} diff --git a/templates/users/show.html.twig b/templates/users/show.html.twig index 5ad8da75..86f97184 100644 --- a/templates/users/show.html.twig +++ b/templates/users/show.html.twig @@ -70,7 +70,7 @@ {{ 'users.show.name' | trans({ name: user.name }) }}

- {% if ldapEnabled %} + {% if user.ldapIdentifier %}

{{ 'users.show.ldap_identifier' | trans({ identifier: user.ldapIdentifier }) }}

diff --git a/tests/Controller/UsersControllerTest.php b/tests/Controller/UsersControllerTest.php index a70a02cb..a074fbe8 100644 --- a/tests/Controller/UsersControllerTest.php +++ b/tests/Controller/UsersControllerTest.php @@ -103,12 +103,14 @@ public function testPostCreateCreatesTheUserAndRedirects(): void $this->assertSame(1, UserFactory::count()); - $client->request(Request::METHOD_GET, '/users/new'); - $crawler = $client->submitForm('form-create-user-submit', [ - 'email' => $email, - 'name' => $name, - 'password' => $password, - 'organization' => $organization->getUid(), + $client->request(Request::METHOD_POST, '/users/new', [ + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $email, + 'name' => $name, + 'plainPassword' => $password, + 'organization' => $organization->getId(), + ], ]); $this->assertSame(2, UserFactory::count()); @@ -134,13 +136,15 @@ public function testPostCreateFailsIfEmailIsAlreadyUsed(): void $name = 'Alix Pataquès'; $client->request(Request::METHOD_POST, '/users/new', [ - '_csrf_token' => $this->generateCsrfToken($client, 'create user'), - 'email' => $email, - 'name' => $name, + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $email, + 'name' => $name, + ], ]); $this->assertSelectorTextContains( - '#email-error', + '#user_email-error', 'Enter a different email address, this one is already in use', ); $this->assertSame(1, UserFactory::count()); @@ -156,12 +160,14 @@ public function testPostCreateFailsIfEmailIsEmpty(): void $name = 'Alix Pataquès'; $client->request(Request::METHOD_POST, '/users/new', [ - '_csrf_token' => $this->generateCsrfToken($client, 'create user'), - 'email' => $email, - 'name' => $name, + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $email, + 'name' => $name, + ], ]); - $this->assertSelectorTextContains('#email-error', 'Enter an email address'); + $this->assertSelectorTextContains('#user_email-error', 'Enter an email address'); $this->assertSame(1, UserFactory::count()); } @@ -175,12 +181,14 @@ public function testPostCreateFailsIfEmailIsInvalid(): void $name = 'Alix Pataquès'; $client->request(Request::METHOD_POST, '/users/new', [ - '_csrf_token' => $this->generateCsrfToken($client, 'create user'), - 'email' => $email, - 'name' => $name, + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $email, + 'name' => $name, + ], ]); - $this->assertSelectorTextContains('#email-error', 'Enter a valid email address'); + $this->assertSelectorTextContains('#user_email-error', 'Enter a valid email address'); $this->assertSame(1, UserFactory::count()); } @@ -194,12 +202,14 @@ public function testPostCreateFailsIfCsrfTokenIsInvalid(): void $name = 'Alix Pataquès'; $client->request(Request::METHOD_POST, '/users/new', [ - '_csrf_token' => 'not a token', - 'email' => $email, - 'name' => $name, + 'user' => [ + '_token' => 'not a token', + 'email' => $email, + 'name' => $name, + ], ]); - $this->assertSelectorTextContains('[data-test="alert-error"]', 'The security token is invalid'); + $this->assertSelectorTextContains('#user-error', 'The security token is invalid'); $this->assertSame(1, UserFactory::count()); } @@ -215,9 +225,11 @@ public function testPostCreateFailsIfAccessIsForbidden(): void $client->catchExceptions(false); $client->request(Request::METHOD_POST, '/users/new', [ - '_csrf_token' => $this->generateCsrfToken($client, 'create user'), - 'email' => $email, - 'name' => $name, + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $email, + 'name' => $name, + ], ]); } @@ -295,11 +307,13 @@ public function testPostUpdateSavesTheUser(): void ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + ] ]); $this->assertResponseRedirects("/users/{$otherUser->getUid()}", 302); @@ -334,11 +348,13 @@ public function testPostUpdateDoesNotChangePasswordIfEmpty(): void ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + ], ]); $this->assertResponseRedirects("/users/{$otherUser->getUid()}", 302); @@ -376,12 +392,15 @@ public function testPostUpdateDoesNotChangeEmailNameOrPasswordIfLdapIsEnabled(): ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), - 'ldapIdentifier' => $newLdapIdentifier, + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + // Don't pass the password as Symfony Form will complain about extra field. + // 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + 'ldapIdentifier' => $newLdapIdentifier, + ], ]); $this->assertResponseRedirects("/users/{$otherUser->getUid()}", 302); @@ -416,11 +435,13 @@ public function testPostUpdateAcceptsEmptyOrganization(): void ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => '', + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => '', + ], ]); $this->assertResponseRedirects("/users/{$otherUser->getUid()}", 302); @@ -455,14 +476,16 @@ public function testPostUpdateFailsIfParamsAreInvalid(): void ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + ], ]); - $this->assertSelectorTextContains('#email-error', 'Enter a valid email address'); + $this->assertSelectorTextContains('#user_email-error', 'Enter a valid email address'); $this->clearEntityManager(); $otherUser->_refresh(); $this->assertSame($oldEmail, $otherUser->getEmail()); @@ -495,14 +518,18 @@ public function testPostUpdateFailsIfCsrfTokenIsInvalid(): void ]); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => 'not a token', - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), + 'user' => [ + '_token' => 'not a token', + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + ], ]); - $this->assertSelectorTextContains('[data-test="alert-error"]', 'The security token is invalid'); + $this->clearEntityManager(); + + $this->assertSelectorTextContains('#user-error', 'The security token is invalid'); $otherUser->_refresh(); $this->assertSame($oldEmail, $otherUser->getEmail()); $this->assertSame($oldName, $otherUser->getName()); @@ -534,11 +561,13 @@ public function testPostUpdateFailsIfAccessIsForbidden(): void $client->catchExceptions(false); $client->request(Request::METHOD_POST, "/users/{$otherUser->getUid()}/edit", [ - '_csrf_token' => $this->generateCsrfToken($client, 'update user'), - 'email' => $newEmail, - 'name' => $newName, - 'password' => $newPassword, - 'organization' => $newOrganization->getUid(), + 'user' => [ + '_token' => $this->generateCsrfToken($client, 'user'), + 'email' => $newEmail, + 'name' => $newName, + 'plainPassword' => $newPassword, + 'organization' => $newOrganization->getId(), + ], ]); } } diff --git a/translations/messages+intl-icu.en_GB.yaml b/translations/messages+intl-icu.en_GB.yaml index 2aab0ba5..ede7f755 100644 --- a/translations/messages+intl-icu.en_GB.yaml +++ b/translations/messages+intl-icu.en_GB.yaml @@ -506,12 +506,13 @@ users.color_scheme: 'Color scheme' users.color_scheme.auto: Auto users.color_scheme.dark: Dark users.color_scheme.light: Light -users.edit.ldap.information: Information -users.edit.ldap.managed: 'You can’t change some information because this account is managed by LDAP.' -users.edit.leave_password_empty: 'Leave blank to keep the current password.' -users.edit.organization_caption: 'The tickets opened by email by this user will be attached to this organization.' users.edit.title: 'Edit a user' users.email: 'Email address' +users.form.ldap.information: Information +users.form.ldap.managed: 'You can’t change some information because this account is managed by LDAP.' +users.form.organization_caption: 'The tickets opened by email by this user will be attached to this organization.' +users.form.password.empty_keep_current: 'Leave blank to keep the current password.' +users.form.password.empty_prevent_login: 'Leave blank to prevent this user from logging in.' users.identifier: 'Email address or identifier' users.index.authorizations: '{count, plural, =0 {No authorizations} one {1 authorization} other {# authorizations}}' users.index.no_users: 'No users' @@ -522,8 +523,6 @@ users.language: Language users.ldap_identifier: 'LDAP identifier' users.n_others: '{number, plural, =0 {no other} one {1 other} other {# other}}' users.name: Name -users.new.leave_password_empty: '(leave empty to forbid login)' -users.new.organization_caption: 'The tickets opened by email by this user will be attached to this organization.' users.new.submit: 'Create the user' users.new.title: 'New user' users.no_organization_authorization: 'This user cannot create tickets in their organisation by default. Remember to give this user the appropriate authorization.' diff --git a/translations/messages+intl-icu.fr_FR.yaml b/translations/messages+intl-icu.fr_FR.yaml index 929290ed..87054c00 100644 --- a/translations/messages+intl-icu.fr_FR.yaml +++ b/translations/messages+intl-icu.fr_FR.yaml @@ -506,12 +506,13 @@ users.color_scheme: 'Schéma de couleurs' users.color_scheme.auto: Auto users.color_scheme.dark: Sombre users.color_scheme.light: Clair -users.edit.ldap.information: Information -users.edit.ldap.managed: 'Certaines informations ne sont pas modifiables car ce compte est géré avec LDAP.' -users.edit.leave_password_empty: 'Laissez vide pour conserver le mot de passe actuel.' -users.edit.organization_caption: 'Les tickets ouverts par email par cet utilisateur seront attachés à cette organisation.' users.edit.title: 'Modifier un utilisateur' users.email: 'Adresse email' +users.form.ldap.information: Information +users.form.ldap.managed: 'Certaines informations ne sont pas modifiables car ce compte est géré avec LDAP.' +users.form.organization_caption: 'Les tickets ouverts par email par cet utilisateur seront attachés à cette organisation.' +users.form.password.empty_keep_current: 'Laissez vide pour conserver le mot de passe actuel.' +users.form.password.empty_prevent_login: 'Laissez vide pour empêcher cet utilisateur de se connecter.' users.identifier: 'Adresse email ou identifiant' users.index.authorizations: '{count, plural, =0 {Aucune autorisation} one {1 autorisation} other {# autorisations}}' users.index.new_user: 'Nouvel utilisateur' @@ -522,8 +523,6 @@ users.language: Langue users.ldap_identifier: 'Identifiant LDAP' users.n_others: '{number, plural, =0 {aucun autre} one {1 autre} other {# autres}}' users.name: Nom -users.new.leave_password_empty: '(laissez vide pour empêcher la connexion)' -users.new.organization_caption: 'Les tickets ouverts par email par cet utilisateur seront attachés à cette organisation.' users.new.submit: 'Créer l’utilisateur' users.new.title: 'Nouvel utilisateur' users.no_organization_authorization: 'Cet utilisateur ne peut pas créer de tickets dans son organisation par défaut. Pensez à lui donner une autorisation correspondante.' From ef06261390f5aa7353248423ad1338d1311b2027 Mon Sep 17 00:00:00 2001 From: Marien Fressinaud Date: Mon, 26 Aug 2024 11:45:00 +0200 Subject: [PATCH 3/3] Allow to choose locale when creating/editing a User --- src/Form/UserForm.php | 10 +++++++++ templates/users/_form.html.twig | 26 ++++++++++++++++++++++++ tests/Controller/UsersControllerTest.php | 20 +++++++++++++++++- 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/Form/UserForm.php b/src/Form/UserForm.php index feda3178..8f174ce5 100644 --- a/src/Form/UserForm.php +++ b/src/Form/UserForm.php @@ -8,6 +8,7 @@ use App\Entity; use App\Form\Type as AppType; +use App\Service; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\Extension\Core\Type; @@ -22,6 +23,7 @@ class UserForm extends AbstractType { public function __construct( + private Service\Locales $locales, #[Autowire(env: 'bool:LDAP_ENABLED')] private bool $ldapEnabled, ) { @@ -34,6 +36,10 @@ public function buildForm(FormBuilderInterface $builder, array $options): void $user = $event->getData(); $managedByLdap = $this->isManagedByLdap($user); + if ($user->getUid() === null) { + $user->setLocale($this->locales->getDefaultLocale()); + } + $form->add('email', Type\EmailType::class, [ 'empty_data' => '', 'trim' => true, @@ -46,6 +52,10 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'disabled' => $managedByLdap, ]); + $form->add('locale', Type\ChoiceType::class, [ + 'choices' => array_flip(Service\Locales::SUPPORTED_LOCALES), + ]); + if ($this->ldapEnabled) { $form->add('ldapIdentifier', Type\TextType::class, [ 'empty_data' => '', diff --git a/templates/users/_form.html.twig b/templates/users/_form.html.twig index b74e94ee..7304ee3c 100644 --- a/templates/users/_form.html.twig +++ b/templates/users/_form.html.twig @@ -64,6 +64,32 @@ /> +
+ + + {{ form_errors(form.locale) }} + + +
+ {% if form.ldapIdentifier is defined %}