Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Support for legacy Password Hashers #447

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
2 changes: 0 additions & 2 deletions config/doctrine/User.orm.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ App\Entity\User:
lastLoginTime:
type: datetime
nullable: true
passwordVersion:
type: integer
recoverySecretBox:
type: text
nullable: true
Expand Down
46 changes: 28 additions & 18 deletions config/packages/security.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 }
12 changes: 2 additions & 10 deletions src/Entity/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -46,7 +45,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor
use PlainPasswordTrait;
use DomainAwareTrait;
use LastLoginTimeTrait;
use PasswordVersionTrait;
use RecoverySecretBoxTrait;
use PlainRecoveryTokenTrait;
use RecoveryStartTimeTrait;
Expand All @@ -58,8 +56,6 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, Passwor
use TwofactorTrait;
use TwofactorBackupCodeTrait;

public const CURRENT_PASSWORD_VERSION = 2;

private array $roles = [];

/**
Expand All @@ -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;
Expand Down Expand Up @@ -113,7 +108,8 @@ public function getUsername(): ?string
/**
* @return string
*/
public function getUserIdentifier(): string {
public function getUserIdentifier(): string
{
return $this->email;
}

Expand All @@ -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;
}
Expand Down
17 changes: 5 additions & 12 deletions src/EventListener/LoginListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
}

Expand Down
78 changes: 0 additions & 78 deletions src/Security/Encoder/LegacyPasswordHasher.php

This file was deleted.

21 changes: 0 additions & 21 deletions src/Traits/PasswordVersionTrait.php

This file was deleted.

2 changes: 0 additions & 2 deletions tests/Entity/UserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading