From cbf376b72d30a7ca94aed7b601707e8d6c15d548 Mon Sep 17 00:00:00 2001 From: core23 Date: Wed, 6 Mar 2024 20:11:49 +0100 Subject: [PATCH] Deprecate user plainPassword in favor of using UserManipulator --- src/Action/ResetAction.php | 22 +++++-- src/Model/User.php | 3 + src/Model/UserInterface.php | 4 ++ src/Resources/config/resetting.php | 1 + src/Resources/config/util.php | 1 + src/Util/SimpleUserManipulator.php | 33 +++++++--- tests/Util/SimpleUserManipulatorTest.php | 76 ++++++++++++++++++++++++ 7 files changed, 129 insertions(+), 11 deletions(-) diff --git a/src/Action/ResetAction.php b/src/Action/ResetAction.php index 68da8601..89d363fa 100644 --- a/src/Action/ResetAction.php +++ b/src/Action/ResetAction.php @@ -16,8 +16,10 @@ use Nucleos\UserBundle\Event\GetResponseUserEvent; use Nucleos\UserBundle\Form\Model\Resetting; use Nucleos\UserBundle\Form\Type\ResettingFormType; +use Nucleos\UserBundle\Model\UserInterface; use Nucleos\UserBundle\Model\UserManager; use Nucleos\UserBundle\NucleosUserEvents; +use Nucleos\UserBundle\Util\UserManipulator; use Symfony\Component\Form\Extension\Core\Type\SubmitType; use Symfony\Component\Form\FormFactoryInterface; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -41,13 +43,16 @@ final class ResetAction private readonly string $loggedinRoute; + private readonly ?UserManipulator $userManipulator; + public function __construct( Environment $twig, RouterInterface $router, EventDispatcherInterface $eventDispatcher, FormFactoryInterface $formFactory, UserManager $userManager, - string $loggedinRoute + string $loggedinRoute, + ?UserManipulator $userManipulator = null ) { $this->twig = $twig; $this->router = $router; @@ -55,6 +60,7 @@ public function __construct( $this->formFactory = $formFactory; $this->userManager = $userManager; $this->loggedinRoute = $loggedinRoute; + $this->userManipulator = $userManipulator; } public function __invoke(Request $request, string $token): Response @@ -87,9 +93,7 @@ public function __invoke(Request $request, string $token): Response $event = new FormEvent($form, $request); $this->eventDispatcher->dispatch($event, NucleosUserEvents::RESETTING_RESET_SUCCESS); - $user->setPlainPassword($formModel->getPlainPassword()); - - $this->userManager->updateUser($user); + $this->changePassword($user, $formModel); if (null === $response = $event->getResponse()) { $response = new RedirectResponse($this->router->generate($this->loggedinRoute)); @@ -108,4 +112,14 @@ public function __invoke(Request $request, string $token): Response 'form' => $form->createView(), ])); } + + private function changePassword(UserInterface $user, Resetting $formModel): void + { + $user->setPlainPassword($formModel->getPlainPassword()); + $this->userManager->updateUser($user); + + if (null !== $this->userManipulator) { + $this->userManipulator->changePassword($user->getUsername(), $formModel->getPlainPassword()); + } + } } diff --git a/src/Model/User.php b/src/Model/User.php index 437bdbd6..5e7a62c9 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -37,6 +37,9 @@ abstract class User implements UserInterface, GroupAwareUser, LocaleAwareUser protected ?string $password = null; + /** + * @deprecated + */ protected ?string $plainPassword = null; protected ?DateTime $lastLogin = null; diff --git a/src/Model/UserInterface.php b/src/Model/UserInterface.php index f9e905ae..4090eddf 100644 --- a/src/Model/UserInterface.php +++ b/src/Model/UserInterface.php @@ -49,11 +49,15 @@ public function setEmail(string $email): void; /** * Gets the plain password. + * + * @deprecated */ public function getPlainPassword(): ?string; /** * Sets the plain password. + * + * @deprecated */ public function setPlainPassword(?string $password): void; diff --git a/src/Resources/config/resetting.php b/src/Resources/config/resetting.php index a773274a..97980311 100644 --- a/src/Resources/config/resetting.php +++ b/src/Resources/config/resetting.php @@ -64,6 +64,7 @@ new Reference('form.factory'), new Reference('nucleos_user.user_manager'), '%nucleos_user.loggedin.route%', + new Reference('nucleos_user.util.user_manipulator'), ]) ->set(CheckEmailAction::class) diff --git a/src/Resources/config/util.php b/src/Resources/config/util.php index 697bc0e9..6f68534e 100644 --- a/src/Resources/config/util.php +++ b/src/Resources/config/util.php @@ -25,6 +25,7 @@ new Reference('nucleos_user.user_manager'), new Reference('event_dispatcher'), new Reference('request_stack'), + new Reference('security.password_hasher'), ]) ->alias('nucleos_user.util.user_manipulator', 'nucleos_user.util.user_manipulator.simple') diff --git a/src/Util/SimpleUserManipulator.php b/src/Util/SimpleUserManipulator.php index 58c3dd50..b0e6087c 100644 --- a/src/Util/SimpleUserManipulator.php +++ b/src/Util/SimpleUserManipulator.php @@ -20,6 +20,7 @@ use Nucleos\UserBundle\NucleosUserEvents; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; final class SimpleUserManipulator implements UserManipulator @@ -30,11 +31,18 @@ final class SimpleUserManipulator implements UserManipulator private readonly RequestStack $requestStack; - public function __construct(UserManager $userManager, EventDispatcherInterface $dispatcher, RequestStack $requestStack) - { - $this->userManager = $userManager; - $this->dispatcher = $dispatcher; - $this->requestStack = $requestStack; + private readonly ?UserPasswordHasherInterface $userPasswordHasher; + + public function __construct( + UserManager $userManager, + EventDispatcherInterface $dispatcher, + RequestStack $requestStack, + ?UserPasswordHasherInterface $userPasswordHasher = null + ) { + $this->userManager = $userManager; + $this->dispatcher = $dispatcher; + $this->requestStack = $requestStack; + $this->userPasswordHasher = $userPasswordHasher; } public function create(string $username, string $password, string $email, bool $active, bool $superadmin): UserInterface @@ -42,7 +50,7 @@ public function create(string $username, string $password, string $email, bool $ $user = $this->userManager->createUser(); $user->setUsername($username); $user->setEmail($email); - $user->setPlainPassword($password); + $this->setPassword($user, $password); $user->setEnabled($active); $user->setSuperAdmin($superadmin); $this->userManager->updateUser($user); @@ -76,7 +84,7 @@ public function deactivate(string $username): void public function changePassword(string $username, string $password): void { $user = $this->findUserByUsernameOrThrowException($username); - $user->setPlainPassword($password); + $this->setPassword($user, $password); $this->userManager->updateUser($user); $event = new UserEvent($user, $this->getRequest()); @@ -151,4 +159,15 @@ private function getRequest(): ?Request { return $this->requestStack->getCurrentRequest(); } + + private function setPassword(UserInterface $user, string $password): void + { + $user->setPlainPassword($password); + + if (null !== $this->userPasswordHasher) { + $user->setPassword( + $this->userPasswordHasher->hashPassword($user, $password) + ); + } + } } diff --git a/tests/Util/SimpleUserManipulatorTest.php b/tests/Util/SimpleUserManipulatorTest.php index b9a508ac..40392ff6 100644 --- a/tests/Util/SimpleUserManipulatorTest.php +++ b/tests/Util/SimpleUserManipulatorTest.php @@ -21,6 +21,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** @@ -61,6 +62,43 @@ public function testCreate(): void self::assertFalse($user->isSuperAdmin()); } + public function testCreateWithHasher(): void + { + $userManagerMock = $this->getMockBuilder(UserManager::class)->getMock(); + $user = new TestUser(); + + $username = 'test_username'; + $password = 'test_password'; + $email = 'test@email.org'; + + $userManagerMock->expects(self::once()) + ->method('createUser') + ->willReturn($user) + ; + + $userManagerMock->expects(self::once()) + ->method('updateUser') + ->with(self::isInstanceOf(TestUser::class)) + ; + + $eventDispatcherMock = $this->getEventDispatcherMock(NucleosUserEvents::USER_CREATED, true); + + $requestStackMock = $this->getRequestStackMock(true); + + $hasher = $this->createMock(UserPasswordHasherInterface::class); + $hasher->method('hashPassword')->with($user, $password)->willReturn('hashed_password'); + + $manipulator = new SimpleUserManipulator($userManagerMock, $eventDispatcherMock, $requestStackMock, $hasher); + $manipulator->create($username, $password, $email, true, false); + + self::assertSame($username, $user->getUsername()); + self::assertSame($password, $user->getPlainPassword()); + self::assertSame('hashed_password', $user->getPassword()); + self::assertSame($email, $user->getEmail()); + self::assertTrue($user->isEnabled()); + self::assertFalse($user->isSuperAdmin()); + } + public function testActivateWithValidUsername(): void { $userManagerMock = $this->getMockBuilder(UserManager::class)->getMock(); @@ -319,6 +357,44 @@ public function testChangePasswordWithValidUsername(): void self::assertSame($password, $user->getPlainPassword()); } + public function testChangePasswordWithValidUsernameAndHasher(): void + { + $userManagerMock = $this->getMockBuilder(UserManager::class)->getMock(); + + $user = new TestUser(); + $username = 'test_username'; + $password = 'test_password'; + $oldpassword = 'old_password'; + + $user->setUsername($username); + $user->setPlainPassword($oldpassword); + + $userManagerMock->expects(self::once()) + ->method('findUserByUsername') + ->willReturn($user) + ->with(self::equalTo($username)) + ; + + $userManagerMock->expects(self::once()) + ->method('updateUser') + ->with(self::isInstanceOf(TestUser::class)) + ; + + $eventDispatcherMock = $this->getEventDispatcherMock(NucleosUserEvents::USER_PASSWORD_CHANGED, true); + + $requestStackMock = $this->getRequestStackMock(true); + + $hasher = $this->createMock(UserPasswordHasherInterface::class); + $hasher->method('hashPassword')->with($user, $password)->willReturn('hashed_password'); + + $manipulator = new SimpleUserManipulator($userManagerMock, $eventDispatcherMock, $requestStackMock, $hasher); + $manipulator->changePassword($username, $password); + + self::assertSame($username, $user->getUsername()); + self::assertSame($password, $user->getPlainPassword()); + self::assertSame('hashed_password', $user->getPassword()); + } + public function testChangePasswordWithInvalidUsername(): void { $this->expectException(InvalidArgumentException::class);