From 34110af2f5fbfef9923f852196c3e594cd47a612 Mon Sep 17 00:00:00 2001 From: louis Date: Sat, 31 Aug 2024 22:00:26 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=E2=99=BB=EF=B8=8FRemove=20PlainPasswordTra?= =?UTF-8?q?it=20from=20User?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/Controller/RecoveryController.php | 2 -- src/Entity/User.php | 6 ++-- src/EventListener/LoginListener.php | 29 ++++------------ src/Handler/RegistrationHandler.php | 1 - src/Handler/UserAuthenticationHandler.php | 2 -- tests/Entity/UserTest.php | 10 ------ tests/EventListener/LoginListenerTest.php | 39 ++-------------------- tests/Handler/RecoveryTokenHandlerTest.php | 5 --- tests/Helper/AdminPasswordUpdaterTest.php | 1 - 9 files changed, 13 insertions(+), 82 deletions(-) diff --git a/src/Controller/RecoveryController.php b/src/Controller/RecoveryController.php index e2f0e5e1..c4f0a976 100644 --- a/src/Controller/RecoveryController.php +++ b/src/Controller/RecoveryController.php @@ -205,8 +205,6 @@ public function recoveryToken(Request $request): Response $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $user->setPlainPassword($recoveryTokenModel->password); - // Check if user has a MailCrypt key if ($user->hasMailCryptSecretBox()) { // Decrypt the MailCrypt key diff --git a/src/Entity/User.php b/src/Entity/User.php index de7b5449..fefc8b44 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -19,7 +19,6 @@ use App\Traits\PasswordTrait; use App\Traits\PasswordVersionTrait; use App\Traits\PlainMailCryptPrivateKeyTrait; -use App\Traits\PlainPasswordTrait; use App\Traits\PlainRecoveryTokenTrait; use App\Traits\QuotaTrait; use App\Traits\RecoverySecretBoxTrait; @@ -53,7 +52,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor use SaltTrait; use DeleteTrait; use InvitationVoucherTrait; - use PlainPasswordTrait; use DomainAwareTrait; use LastLoginTimeTrait; use PasswordVersionTrait; @@ -142,4 +140,8 @@ public function getPasswordHasherName(): ?string // use default encoder return null; } + + public function eraseCredentials() + { + } } diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php index e81e4d12..c8f1fcba 100644 --- a/src/EventListener/LoginListener.php +++ b/src/EventListener/LoginListener.php @@ -4,28 +4,24 @@ use App\Entity\User; use App\Event\LoginEvent; -use App\Helper\PasswordUpdater; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\Security\Http\Event\InteractiveLoginEvent; use Symfony\Component\Security\Http\SecurityEvents; -class LoginListener implements EventSubscriberInterface +readonly class LoginListener implements EventSubscriberInterface { - /** - * LoginListener constructor. - */ - public function __construct(private readonly EntityManagerInterface $manager, private readonly PasswordUpdater $passwordUpdater) + public function __construct(private EntityManagerInterface $manager) { } public function onSecurityInteractiveLogin(InteractiveLoginEvent $event): void { - $request = $event->getRequest(); - /** @var User $user */ $user = $event->getAuthenticationToken()->getUser(); - $user->setPlainPassword($request->get('_password')); - $this->handleLogin($user); + + if ($user instanceof User) { + $this->handleLogin($user); + } } public function onLogin(LoginEvent $event): void @@ -34,25 +30,12 @@ public function onLogin(LoginEvent $event): void } private function handleLogin(User $user): void - { - // update password hash if necessary - if (($user->getPasswordVersion() < User::CURRENT_PASSWORD_VERSION) && null !== $plainPassword = $user->getPlainPassword()) { - $user->setPasswordVersion(User::CURRENT_PASSWORD_VERSION); - $this->passwordUpdater->updatePassword($user, $plainPassword); - } - $this->updateLastLogin($user); - } - - private function updateLastLogin(User $user): void { $user->updateLastLoginTime(); $this->manager->persist($user); $this->manager->flush(); } - /** - * {@inheritdoc} - */ public static function getSubscribedEvents(): array { return [ diff --git a/src/Handler/RegistrationHandler.php b/src/Handler/RegistrationHandler.php index af4eab7d..9c9de2d1 100644 --- a/src/Handler/RegistrationHandler.php +++ b/src/Handler/RegistrationHandler.php @@ -73,7 +73,6 @@ private function buildUser(Registration $registration): User { $user = new User(); $user->setEmail(strtolower((string)$registration->getEmail())); - $user->setPlainPassword($registration->getPlainPassword()); $user->setRoles([Roles::USER]); if (null !== $domain = $this->domainGuesser->guess($registration->getEmail())) { diff --git a/src/Handler/UserAuthenticationHandler.php b/src/Handler/UserAuthenticationHandler.php index e6c1b6b2..66bbbb4b 100644 --- a/src/Handler/UserAuthenticationHandler.php +++ b/src/Handler/UserAuthenticationHandler.php @@ -25,8 +25,6 @@ public function authenticate(User $user, string $password): ?User if (!$hasher->verify($user->getPassword(), $password)) { return null; } - $user->setPlainPassword($password); - $this->eventDispatcher->dispatch(new LoginEvent($user), LoginEvent::NAME); return $user; diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index b8a509e3..cc392308 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -55,16 +55,6 @@ public function testGetPasswordHasherName(): void self::assertEquals('legacy', $user->getPasswordHasherName()); } - public function testPlainPassword(): void - { - $user = new User(); - self::assertEquals(null, $user->getPlainPassword()); - $user->setPlainPassword('test'); - self::assertEquals('test', $user->getPlainPassword()); - $user->eraseCredentials(); - self::assertEquals(null, $user->getPlainPassword()); - } - public function testHasRecoverySecretBox(): void { $user = new User(); diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php index 4388124d..6d61d93a 100644 --- a/tests/EventListener/LoginListenerTest.php +++ b/tests/EventListener/LoginListenerTest.php @@ -7,7 +7,6 @@ use App\EventListener\LoginListener; use App\Helper\PasswordUpdater; use Doctrine\ORM\EntityManagerInterface; -use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; @@ -17,7 +16,6 @@ class LoginListenerTest extends TestCase { private EntityManagerInterface $manager; - private PasswordUpdater $passwordUpdater; private LoginListener $listener; public function setUp(): void @@ -25,25 +23,13 @@ public function setUp(): void $this->manager = $this->getMockBuilder(EntityManagerInterface::class) ->disableOriginalConstructor() ->getMock(); - $this->passwordUpdater = $this->getMockBuilder(PasswordUpdater::class) - ->disableOriginalConstructor() - ->getMock(); - $this->listener = new LoginListener($this->manager, $this->passwordUpdater); + $this->listener = new LoginListener($this->manager); } - /** - * @dataProvider provider - */ - public function testOnSecurityInteractiveLogin(User $user, bool $update): void + public function testOnSecurityInteractiveLogin(): void { + $user = new User(); $this->manager->expects($this->once())->method('flush'); - - if ($update) { - $this->passwordUpdater->expects($this->once())->method('updatePassword'); - } else { - $this->passwordUpdater->expects($this->never())->method('updatePassword'); - } - $event = $this->getEvent($user); $this->listener->onSecurityInteractiveLogin($event); @@ -74,25 +60,6 @@ private function getEvent(User $user): InteractiveLoginEvent return $event; } - public function provider(): array - { - return [ - [$this->getUser(null), true], - [$this->getUser(0), true], - [$this->getUser(1), true], - [$this->getUser(2), false], - [$this->getUser(3), false], - ]; - } - - public function getUser(?int $passwordVersion): User - { - $user = new User(); - $user->setPasswordVersion($passwordVersion); - - return $user; - } - public function testGetSubscribedEvents(): void { $this->assertEquals([ diff --git a/tests/Handler/RecoveryTokenHandlerTest.php b/tests/Handler/RecoveryTokenHandlerTest.php index 82c02e5f..3589e117 100644 --- a/tests/Handler/RecoveryTokenHandlerTest.php +++ b/tests/Handler/RecoveryTokenHandlerTest.php @@ -26,7 +26,6 @@ public function testCreateExceptionPlainMailCryptPrivateKeyNull(): void $handler = $this->createHandler(); $user = new User(); - $user->setPlainPassword('password'); $handler->create($user); } @@ -35,7 +34,6 @@ public function testCreate(): void $handler = $this->createHandler(); $user = new User(); - $user->setPlainPassword('password'); $user->setPlainMailCryptPrivateKey('dummyKey'); $handler->create($user); @@ -49,7 +47,6 @@ public function testVerify(): void self::assertFalse($handler->verify($user, 'recoveryToken')); - $user->setPlainPassword('password'); $user->setPlainMailCryptPrivateKey('dummyKey'); $handler->create($user); @@ -78,7 +75,6 @@ public function testDecryptExceptionDecryptionFailed(): void $this->expectExceptionMessage('decryption of recoverySecretBox failed'); $handler = $this->createHandler(); $user = new User(); - $user->setPlainPassword('password'); $user->setPlainMailCryptPrivateKey('privateKey'); $handler->create($user); @@ -89,7 +85,6 @@ public function testDecrypt(): void { $handler = $this->createHandler(); $user = new User(); - $user->setPlainPassword('password'); $user->setPlainMailCryptPrivateKey('privateKey'); $handler->create($user); $recoveryToken = $user->getPlainRecoveryToken(); diff --git a/tests/Helper/AdminPasswordUpdaterTest.php b/tests/Helper/AdminPasswordUpdaterTest.php index 3b9cb708..33300302 100644 --- a/tests/Helper/AdminPasswordUpdaterTest.php +++ b/tests/Helper/AdminPasswordUpdaterTest.php @@ -28,7 +28,6 @@ public function setUp(): void public function testUpdateAdminPassword(): void { $admin = new User(); - $admin->setPlainPassword('password'); $admin->setPassword('impossible_login'); $adminPasswordUpdater = new AdminPasswordUpdater( From cb9936a491ad06e6b757dfe68cb04a90bd9c91cc Mon Sep 17 00:00:00 2001 From: louis Date: Sun, 29 Sep 2024 14:10:13 +0200 Subject: [PATCH 2/2] Keep PlainPasswordTrait in User Entity --- src/Entity/User.php | 10 ++++++++-- src/Traits/PlainPasswordTrait.php | 8 -------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/Entity/User.php b/src/Entity/User.php index fefc8b44..b9bb6ca3 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -3,6 +3,7 @@ namespace App\Entity; use App\Repository\UserRepository; +use App\Traits\PlainPasswordTrait; use Stringable; use DateTime; use App\Enum\Roles; @@ -55,8 +56,9 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor use DomainAwareTrait; use LastLoginTimeTrait; use PasswordVersionTrait; - use RecoverySecretBoxTrait; + use PlainPasswordTrait; use PlainRecoveryTokenTrait; + use RecoverySecretBoxTrait; use RecoveryStartTimeTrait; use MailCryptTrait; use MailCryptSecretBoxTrait; @@ -141,7 +143,11 @@ public function getPasswordHasherName(): ?string return null; } - public function eraseCredentials() + public function eraseCredentials(): void { + // If you store any temporary, sensitive data on the user, clear it here + $this->plainPassword = null; + $this->erasePlainMailCryptPrivateKey(); + $this->erasePlainRecoveryToken(); } } diff --git a/src/Traits/PlainPasswordTrait.php b/src/Traits/PlainPasswordTrait.php index c96760da..1ebd7eeb 100644 --- a/src/Traits/PlainPasswordTrait.php +++ b/src/Traits/PlainPasswordTrait.php @@ -23,12 +23,4 @@ public function setPlainPassword(?string $plainPassword): void { $this->plainPassword = $plainPassword; } - - public function eraseCredentials(): void - { - // If you store any temporary, sensitive data on the user, clear it here - $this->plainPassword = null; - $this->erasePlainMailCryptPrivateKey(); - $this->erasePlainRecoveryToken(); - } }