Skip to content

Commit

Permalink
Deprecate user plainPassword in favor of using UserManipulator
Browse files Browse the repository at this point in the history
  • Loading branch information
core23 committed Mar 6, 2024
1 parent ac78dcc commit b8050c1
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 11 deletions.
22 changes: 18 additions & 4 deletions src/Action/ResetAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Check warning on line 63 in src/Action/ResetAction.php

View check run for this annotation

Codecov / codecov/patch

src/Action/ResetAction.php#L63

Added line #L63 was not covered by tests
}

public function __invoke(Request $request, string $token): Response
Expand Down Expand Up @@ -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);

Check warning on line 96 in src/Action/ResetAction.php

View check run for this annotation

Codecov / codecov/patch

src/Action/ResetAction.php#L96

Added line #L96 was not covered by tests

if (null === $response = $event->getResponse()) {
$response = new RedirectResponse($this->router->generate($this->loggedinRoute));
Expand All @@ -108,4 +112,14 @@ public function __invoke(Request $request, string $token): Response
'form' => $form->createView(),
]));
}

private function changePassword(UserInterface $user, Resetting $formModel): void

Check warning on line 116 in src/Action/ResetAction.php

View check run for this annotation

Codecov / codecov/patch

src/Action/ResetAction.php#L116

Added line #L116 was not covered by tests
{
$user->setPlainPassword($formModel->getPlainPassword());
$this->userManager->updateUser($user);

Check warning on line 119 in src/Action/ResetAction.php

View check run for this annotation

Codecov / codecov/patch

src/Action/ResetAction.php#L118-L119

Added lines #L118 - L119 were not covered by tests

if (null !== $this->userManipulator) {
$this->userManipulator->changePassword($user->getUsername(), $formModel->getPlainPassword());

Check warning on line 122 in src/Action/ResetAction.php

View check run for this annotation

Codecov / codecov/patch

src/Action/ResetAction.php#L121-L122

Added lines #L121 - L122 were not covered by tests
}
}
}
3 changes: 3 additions & 0 deletions src/Model/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ abstract class User implements UserInterface, GroupAwareUser, LocaleAwareUser

protected ?string $password = null;

/**
* @deprecated
*/
protected ?string $plainPassword = null;

protected ?DateTime $lastLogin = null;
Expand Down
4 changes: 4 additions & 0 deletions src/Model/UserInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
1 change: 1 addition & 0 deletions src/Resources/config/resetting.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions src/Resources/config/util.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
33 changes: 26 additions & 7 deletions src/Util/SimpleUserManipulator.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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)
);
}
}
}
76 changes: 76 additions & 0 deletions tests/Util/SimpleUserManipulatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit b8050c1

Please sign in to comment.