Deprecate user plainPassword in favor of using UserManipulator

nucleos · Mar 6, 2024 · 4d62d16 · 4d62d16
1 parent fee927a
commit 4d62d16
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 11 deletions.
diff --git 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,20 +43,24 @@ 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;
         $this->eventDispatcher = $eventDispatcher;
         $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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,19 +31,26 @@ 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
     {
         $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
@@ -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      = '[email protected]';
+
+        $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);