diff --git a/CHANGELOG.md b/CHANGELOG.md index a21da414..9c1540f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,8 @@ -# 3.2.0 (UNRELEASED) +# 3.3.0 (UNRELEASED) + +* Remove legacy password hash support (drop field `password_version` from `virtual_users` table) + +# 3.2.0 (2023.03.30) * Add Command to export metrics to Prometheus diff --git a/config/doctrine/User.orm.yml b/config/doctrine/User.orm.yml index 774ef902..48d71931 100644 --- a/config/doctrine/User.orm.yml +++ b/config/doctrine/User.orm.yml @@ -33,8 +33,6 @@ App\Entity\User: lastLoginTime: type: datetime nullable: true - passwordVersion: - type: integer recoverySecretBox: type: text nullable: true diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 561f076b..d1ebaa12 100755 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -5,8 +5,6 @@ security: password_hashers: App\Entity\User: algorithm: sodium - legacy: - id: 'App\Security\Encoder\LegacyPasswordHasher' # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers providers: @@ -124,19 +122,31 @@ security: # Easy way to control access for large sections of your site # Note: Only the *first* access control that matches will be used access_control: - - { path: ^/login, roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: ^/logout, roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/init", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/login", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/logout", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/recovery", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/register", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: "^/[a-z]{2}/$", roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: ^/$, roles: IS_AUTHENTICATED_ANONYMOUSLY } - - { path: ^/2fa, roles: IS_AUTHENTICATED_2FA_IN_PROGRESS } - - { path: "^/[a-z]{2}/2fa", roles: IS_AUTHENTICATED_2FA_IN_PROGRESS } - - { path: ^/admin, roles: ROLE_DOMAIN_ADMIN } - - { path: "^/[a-z]{2}/voucher", roles: ROLE_USER, allow_if: "!is_granted('ROLE_SUSPICIOUS')"} - - { path: "^/[a-z]{2}/alias", roles: ROLE_USER, allow_if: "!is_granted('ROLE_SPAM')"} - - { path: "^/[a-z]{2}/account", roles: ROLE_USER, allow_if: "!is_granted('ROLE_SPAM')"} - - { path: ^/, roles: ROLE_USER } + - { path: ^/login, roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: ^/logout, roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/init", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/login", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/logout", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/recovery", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/register", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: "^/[a-z]{2}/$", roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: ^/$, roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: ^/2fa, roles: IS_AUTHENTICATED_2FA_IN_PROGRESS } + - { path: "^/[a-z]{2}/2fa", roles: IS_AUTHENTICATED_2FA_IN_PROGRESS } + - { path: ^/admin, roles: ROLE_DOMAIN_ADMIN } + - { + path: "^/[a-z]{2}/voucher", + roles: ROLE_USER, + allow_if: "!is_granted('ROLE_SUSPICIOUS')", + } + - { + path: "^/[a-z]{2}/alias", + roles: ROLE_USER, + allow_if: "!is_granted('ROLE_SPAM')", + } + - { + path: "^/[a-z]{2}/account", + roles: ROLE_USER, + allow_if: "!is_granted('ROLE_SPAM')", + } + - { path: ^/, roles: ROLE_USER } diff --git a/src/Entity/User.php b/src/Entity/User.php index bf56406d..5f325648 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -15,7 +15,6 @@ use App\Traits\MailCryptTrait; use App\Traits\OpenPgpKeyTrait; use App\Traits\PasswordTrait; -use App\Traits\PasswordVersionTrait; use App\Traits\PlainMailCryptPrivateKeyTrait; use App\Traits\PlainPasswordTrait; use App\Traits\PlainRecoveryTokenTrait; @@ -46,7 +45,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor use PlainPasswordTrait; use DomainAwareTrait; use LastLoginTimeTrait; - use PasswordVersionTrait; use RecoverySecretBoxTrait; use PlainRecoveryTokenTrait; use RecoveryStartTimeTrait; @@ -58,8 +56,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor use TwofactorTrait; use TwofactorBackupCodeTrait; - public const CURRENT_PASSWORD_VERSION = 2; - private array $roles = []; /** @@ -68,7 +64,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor public function __construct() { $this->deleted = false; - $this->passwordVersion = self::CURRENT_PASSWORD_VERSION; $currentDateTime = new \DateTime(); $this->creationTime = $currentDateTime; $this->updatedTime = $currentDateTime; @@ -113,7 +108,8 @@ public function getUsername(): ?string /** * @return string */ - public function getUserIdentifier(): string { + public function getUserIdentifier(): string + { return $this->email; } @@ -122,10 +118,6 @@ public function getUserIdentifier(): string { */ public function getPasswordHasherName(): ?string { - if ($this->getPasswordVersion() < self::CURRENT_PASSWORD_VERSION) { - return 'legacy'; - } - // use default encoder return null; } diff --git a/src/EventListener/LoginListener.php b/src/EventListener/LoginListener.php index a82b073d..1d799b66 100644 --- a/src/EventListener/LoginListener.php +++ b/src/EventListener/LoginListener.php @@ -4,7 +4,6 @@ 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; @@ -13,24 +12,23 @@ class LoginListener implements EventSubscriberInterface { private EntityManagerInterface $manager; - private PasswordUpdater $passwordUpdater; /** * LoginListener constructor. */ - public function __construct(EntityManagerInterface $manager, PasswordUpdater $passwordUpdater) + public function __construct(EntityManagerInterface $manager) { $this->manager = $manager; - $this->passwordUpdater = $passwordUpdater; } public function onSecurityInteractiveLogin(InteractiveLoginEvent $event): void { $request = $event->getRequest(); - /** @var User $user */ + /** @var User|null $user */ $user = $event->getAuthenticationToken()->getUser(); - $user->setPlainPassword($request->get('_password')); - $this->handleLogin($user); + if ($user !== null && $request->get('_password') !== null) { + $this->handleLogin($user); + } } public function onLogin(LoginEvent $event): void @@ -40,11 +38,6 @@ 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); } diff --git a/src/Security/Encoder/LegacyPasswordHasher.php b/src/Security/Encoder/LegacyPasswordHasher.php deleted file mode 100644 index f31af67f..00000000 --- a/src/Security/Encoder/LegacyPasswordHasher.php +++ /dev/null @@ -1,78 +0,0 @@ -algorithm = $algorithm; - $this->encodeHashAsBase64 = $encodeHashAsBase64; - $this->iterations = $iterations; - } - - /** - * @param string $hashedPassword - * - * @return bool - */ - public function needsRehash(string $hashedPassword): bool { - return true; - } - - /** - * {@inheritdoc} - */ - public function hash(string $plainPassword): string - { - if ($this->isPasswordTooLong($plainPassword)) { - throw new InvalidPasswordException('Invalid password.'); - } - - switch ($this->algorithm) { - case 'sha256': - $hashId = 5; - break; - case 'sha512': - $hashId = 6; - break; - default: - throw new \LogicException(sprintf('The algorithm "%s" is not supported.', $this->algorithm)); - } - - $salt = uniqid(mt_rand(), true); - - $digest = crypt($plainPassword, sprintf('$%d$rounds=%d$%s$', $hashId, $this->iterations, $salt)); - - return $this->encodeHashAsBase64 ? base64_encode($digest) : $digest; - } - - /** - * {@inheritdoc} - */ - public function verify(string $hashedPassword, $plainPassword): bool - { - if ('' === $plainPassword || $this->isPasswordTooLong($plainPassword)) { - return false; - } - - return hash_equals(crypt($plainPassword, $hashedPassword), $hashedPassword); - } -} diff --git a/src/Traits/PasswordVersionTrait.php b/src/Traits/PasswordVersionTrait.php deleted file mode 100644 index c035b0e3..00000000 --- a/src/Traits/PasswordVersionTrait.php +++ /dev/null @@ -1,21 +0,0 @@ -passwordVersion; - } - - public function setPasswordVersion(?int $passwordVersion): void - { - $this->passwordVersion = $passwordVersion; - } -} diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index 6fde91c1..da2d06a7 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -50,8 +50,6 @@ public function testGetPasswordHasherName(): void { $user = new User(); self::assertEquals(null, $user->getPasswordHasherName()); - $user->setPasswordVersion(1); - self::assertEquals('legacy', $user->getPasswordHasherName()); } public function testPlainPassword(): void diff --git a/tests/EventListener/LoginListenerTest.php b/tests/EventListener/LoginListenerTest.php index bf32b226..2befcd8c 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; @@ -16,89 +15,57 @@ class LoginListenerTest extends TestCase { - private EntityManagerInterface $manager; - private PasswordUpdater $passwordUpdater; private LoginListener $listener; public function setUp(): void { - $this->manager = $this->getMockBuilder(EntityManagerInterface::class) + $manager = $this->getMockBuilder(EntityManagerInterface::class) ->disableOriginalConstructor() ->getMock(); - $this->passwordUpdater = $this->getMockBuilder(PasswordUpdater::class) + $passwordUpdater = $this->getMockBuilder(PasswordUpdater::class) ->disableOriginalConstructor() ->getMock(); - $this->listener = new LoginListener($this->manager, $this->passwordUpdater); + $this->listener = new LoginListener($manager, $passwordUpdater); } - /** - * @dataProvider provider - */ - public function testOnSecurityInteractiveLogin(User $user, bool $update): void + public function testOnSecurityInteractiveLogin(): void { - $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); - } + $user = new User(); + $user->setLastLoginTime(new \DateTime('1970-01-01 00:00:00')); - /** - * @return \PHPUnit_Framework_MockObject_MockObject|InteractiveLoginEvent - */ - private function getEvent(User $user): InteractiveLoginEvent - { $request = $this->getMockBuilder(Request::class) ->disableOriginalConstructor() ->getMock(); $request->method('get')->willReturn('password'); - - $token = $this->getMockBuilder(TokenInterface::class) + $authenticationToken = $this->getMockBuilder(TokenInterface::class) ->disableOriginalConstructor() ->getMock(); - $token->method('getUser')->willReturn($user); + $authenticationToken->method('getUser')->willReturn($user); + $event = new InteractiveLoginEvent($request, $authenticationToken); - $event = $this->getMockBuilder(InteractiveLoginEvent::class) - ->disableOriginalConstructor() - ->getMock(); + $this->listener->onSecurityInteractiveLogin($event); - $event->method('getRequest')->willReturn($request); - $event->method('getAuthenticationToken')->willReturn($token); + self::assertNotEquals(new \DateTime('1970-01-01 00:00:00'), $user->getLastLoginTime()); - return $event; - } + $user = new User(); + $user->setLastLoginTime(new \DateTime('1970-01-01 00:00:00')); + $authenticationToken->method('getUser')->willReturn(null); - 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], - ]; - } + $event = new InteractiveLoginEvent($request, $authenticationToken); - public function getUser(?int $passwordVersion): User - { - $user = new User(); - $user->setPasswordVersion($passwordVersion); + $this->listener->onSecurityInteractiveLogin($event); - return $user; + self::assertEquals(new \DateTime('1970-01-01 00:00:00'), $user->getLastLoginTime()); } public function testGetSubscribedEvents(): void { - $this->assertEquals([ - SecurityEvents::INTERACTIVE_LOGIN => 'onSecurityInteractiveLogin', - LoginEvent::class => 'onLogin', - ], - $this->listener::getSubscribedEvents()); + $this->assertEquals( + [ + SecurityEvents::INTERACTIVE_LOGIN => 'onSecurityInteractiveLogin', + LoginEvent::class => 'onLogin', + ], + $this->listener::getSubscribedEvents() + ); } } diff --git a/tests/Helper/AdminPasswordUpdaterTest.php b/tests/Helper/AdminPasswordUpdaterTest.php index 68db1965..c734dc0c 100644 --- a/tests/Helper/AdminPasswordUpdaterTest.php +++ b/tests/Helper/AdminPasswordUpdaterTest.php @@ -8,7 +8,6 @@ use App\Helper\PasswordUpdater; use App\Repository\DomainRepository; use App\Repository\UserRepository; -use App\Security\Encoder\LegacyPasswordHasher; use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -33,7 +32,8 @@ public function testUpdateAdminPassword(): void $adminPasswordUpdater = new AdminPasswordUpdater( $this->getManager($admin), $this->getUpdater(), - $this->defaultDomain); + $this->defaultDomain + ); $adminPasswordUpdater->updateAdminPassword('newpassword'); @@ -58,7 +58,8 @@ public function getManager($object): MockObject [ [Domain::class, $domainRepo], [User::class, $userRepo], - ]); + ] + ); return $manager; } diff --git a/tests/Helper/PasswordUpdaterTest.php b/tests/Helper/PasswordUpdaterTest.php index 9481601e..d39b5e44 100644 --- a/tests/Helper/PasswordUpdaterTest.php +++ b/tests/Helper/PasswordUpdaterTest.php @@ -4,7 +4,6 @@ use App\Entity\User; use App\Helper\PasswordUpdater; -use App\Security\Encoder\LegacyPasswordHasher; use PHPUnit\Framework\TestCase; use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface; use Symfony\Component\PasswordHasher\PasswordHasherInterface; diff --git a/tests/Security/Encoder/PasswordHashEncoderTest.php b/tests/Security/Encoder/PasswordHashEncoderTest.php deleted file mode 100644 index feda1efc..00000000 --- a/tests/Security/Encoder/PasswordHashEncoderTest.php +++ /dev/null @@ -1,59 +0,0 @@ -hash(self::PASSWORD); - self::assertTrue($hasher->verify($result, self::PASSWORD)); - self::assertFalse($hasher->verify($result, 'anotherPassword')); - } - - public function testValidationWithWrongAlgorithm(): void - { - $this->expectException(LogicException::class); - $hasher = new LegacyPasswordHasher('sha666'); - $hasher->hash(self::PASSWORD); - } - - public function testValidation(): void - { - $hasher = new LegacyPasswordHasher(); - $result = $hasher->hash(self::PASSWORD); - self::assertTrue($hasher->verify($result, self::PASSWORD)); - self::assertFalse($hasher->verify($result, 'anotherPassword')); - } - - public function testEncodePasswordLength(): void - { - $this->expectException(InvalidPasswordException::class); - $hasher = new LegacyPasswordHasher(); - $hasher->hash(str_repeat('a', 4097)); - } - - public function testCheckPasswordLength(): void - { - $hasher = new LegacyPasswordHasher(); - $result = $hasher->hash(str_repeat('a', 4096)); - self::assertFalse($hasher->verify($result, str_repeat('a', 4097))); - self::assertTrue($hasher->verify($result, str_repeat('a', 4096))); - } - - public function testMd5(): void - { - $hasher = new LegacyPasswordHasher(); - // doveadm pw -s MD5-CRYPT -p password - $result = '$1$Is0rXQe3$CdxfOUEEqjfKZWc03GpEg1'; - self::assertTrue($hasher->verify($result, self::PASSWORD)); - } -}