Skip to content

[make:user] Use password_hashers instead of encoders #889

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

Merged
merged 1 commit into from
Jun 25, 2021
Merged
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: 6 additions & 0 deletions src/Maker/MakeUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher;
use Symfony\Component\Security\Core\Encoder\Argon2iPasswordEncoder;
use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder;
use Symfony\Component\Security\Core\Exception\UserNotFoundException;
Expand Down Expand Up @@ -116,6 +117,11 @@ class_exists(DoctrineBundle::class)
$userWillHavePassword = $io->confirm('Does this app need to hash/check user passwords?');
$input->setOption('with-password', $userWillHavePassword);

$symfonyGte53 = class_exists(NativePasswordHasher::class);
if ($symfonyGte53) {
return;
}

if ($userWillHavePassword && !class_exists(NativePasswordEncoder::class) && Argon2iPasswordEncoder::isSupported()) {
$io->writeln('The newer <comment>Argon2i</comment> password hasher requires PHP 7.2, libsodium or paragonie/sodium_compat. Your system DOES support this algorithm.');
$io->writeln('You should use <comment>Argon2i</comment> unless your production system will not support it.');
Expand Down
8 changes: 4 additions & 4 deletions src/Resources/skeleton/security/UserProvider.tpl.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ public function supportsClass($class)
<?php if ($password_upgrader): ?>

/**
* Upgrades the encoded password of a user, typically for using a better hash algorithm.
* Upgrades the hashed password of a user, typically for using a better hash algorithm.
*/
public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
public function upgradePassword(UserInterface $user, string $newHashedPassword): void
{
// TODO: when encoded passwords are in use, this method should:
// TODO: when hashed passwords are in use, this method should:
// 1. persist the new password in the user storage
// 2. update the $user object with $user->setPassword($newEncodedPassword);
// 2. update the $user object with $user->setPassword($newHashedPassword);
}
<?php endif ?>
}
42 changes: 34 additions & 8 deletions src/Security/SecurityConfigUpdater.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Bundle\MakerBundle\Util\YamlSourceManipulator;
use Symfony\Component\HttpKernel\Log\Logger;
use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher;
use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder;

/**
Expand Down Expand Up @@ -50,7 +51,8 @@ public function updateForUserClass(string $yamlSource, UserClassConfiguration $u
$this->updateProviders($userConfig, $userClass);

if ($userConfig->hasPassword()) {
$this->updateEncoders($userConfig, $userClass);
$symfonyGte53 = class_exists(NativePasswordHasher::class);
$this->updatePasswordHashers($userConfig, $userClass, $symfonyGte53 ? 'password_hashers' : 'encoders');
}

$contents = $this->manipulator->getContents();
Expand All @@ -72,6 +74,10 @@ public function updateForAuthenticator(string $yamlSource, string $firewallName,
$newData = $this->manipulator->getData();

if (!isset($newData['security']['firewalls'])) {
if ($newData['security']) {
$newData['security']['_firewalls'] = $this->manipulator->createEmptyLine();
}
Copy link
Member

@weaverryan weaverryan Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? Are we suddenly missing an empty line somewhere?

Nevermind - I see that we're now handling some more complex cases than before with placement, etc


$newData['security']['firewalls'] = [];
}

Expand Down Expand Up @@ -153,6 +159,10 @@ private function updateProviders(UserClassConfiguration $userConfig, string $use
$this->removeMemoryProviderIfIsSingleConfigured();

$newData = $this->manipulator->getData();
if ($newData['security'] && !\array_key_exists('providers', $newData['security'])) {
$newData['security']['_providers'] = $this->manipulator->createEmptyLine();
}

$newData['security']['providers']['__'] = $this->manipulator->createCommentLine(
' used to reload user from session & other features (e.g. switch_user)'
);
Expand All @@ -175,18 +185,34 @@ private function updateProviders(UserClassConfiguration $userConfig, string $use
$this->manipulator->setData($newData);
}

private function updateEncoders(UserClassConfiguration $userConfig, string $userClass)
private function updatePasswordHashers(UserClassConfiguration $userConfig, string $userClass, string $keyName = 'password_hashers')
{
$newData = $this->manipulator->getData();
if (!isset($newData['security']['encoders'])) {
// encoders is usually the first key, by convention
$newData['security'] = ['encoders' => []] + $newData['security'];
if ('password_hashers' === $keyName && isset($newData['security']['encoders'])) {
// fallback to "encoders" if the user already defined encoder config
$this->updatePasswordHashers($userClass, $userClass, 'encoders');

return;
}

if (!isset($newData['security'][$keyName])) {
// by convention, password_hashers are put before the user provider option
$providersIndex = array_search('providers', array_keys($newData['security']));
if (false === $providersIndex) {
$newData['security'] = [$keyName => []] + $newData['security'];
} else {
$newData['security'] = array_merge(
\array_slice($newData['security'], 0, $providersIndex),
[$keyName => []],
\array_slice($newData['security'], $providersIndex)
);
}
}

$newData['security']['encoders'][$userClass] = [
'algorithm' => $userConfig->shouldUseArgon2() ? 'argon2i' : (class_exists(NativePasswordEncoder::class) ? 'auto' : 'bcrypt'),
$newData['security'][$keyName][$userClass] = [
'algorithm' => $userConfig->shouldUseArgon2() ? 'argon2i' : ((class_exists(NativePasswordHasher::class) || class_exists(NativePasswordEncoder::class)) ? 'auto' : 'bcrypt'),
];
$newData['security']['encoders']['_'] = $this->manipulator->createEmptyLine();
$newData['security'][$keyName]['_'] = $this->manipulator->createEmptyLine();

$this->manipulator->setData($newData);
}
Expand Down
18 changes: 16 additions & 2 deletions tests/Security/SecurityConfigUpdaterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Symfony\Bundle\MakerBundle\Security\SecurityConfigUpdater;
use Symfony\Bundle\MakerBundle\Security\UserClassConfiguration;
use Symfony\Component\HttpKernel\Log\Logger;
use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher;
use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder;

class SecurityConfigUpdaterTest extends TestCase
Expand Down Expand Up @@ -48,9 +49,10 @@ public function testUpdateForUserClass(UserClassConfiguration $userConfig, strin
$updater = new SecurityConfigUpdater($this->ysmLogger);
$source = file_get_contents(__DIR__.'/yaml_fixtures/source/'.$startingSourceFilename);
$actualSource = $updater->updateForUserClass($source, $userConfig, $userClass);
$expectedSource = file_get_contents(__DIR__.'/yaml_fixtures/expected_user_class/'.$expectedSourceFilename);
$symfonyVersion = class_exists(NativePasswordHasher::class) ? '5.3' : 'legacy';
$expectedSource = file_get_contents(__DIR__.'/yaml_fixtures/expected_user_class/'.$symfonyVersion.'/'.$expectedSourceFilename);

$bcryptOrAuto = class_exists(NativePasswordEncoder::class) ? 'auto' : 'bcrypt';
$bcryptOrAuto = ('5.3' === $symfonyVersion || class_exists(NativePasswordEncoder::class)) ? 'auto' : 'bcrypt';
$expectedSource = str_replace('{BCRYPT_OR_AUTO}', $bcryptOrAuto, $expectedSource);

$this->assertSame($expectedSource, $actualSource);
Expand Down Expand Up @@ -95,6 +97,18 @@ public function getUserClassTests()
'simple_security_with_single_memory_provider_configured.yaml',
'simple_security_with_single_memory_provider_configured.yaml',
];

yield 'security_52_empty_security' => [
new UserClassConfiguration(true, 'email', true),
'security_52_entity_email_with_password.yaml',
'security_52_empty_security.yaml',
];

yield 'security_52_with_firewalls_and_logout' => [
new UserClassConfiguration(true, 'email', true),
'security_52_complex_entity_email_with_password.yaml',
'security_52_with_firewalls_and_logout.yaml',
];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ security:
main:
anonymous: lazy
guard:
authenticators: [App\Security\AppCustomAuthenticator]
authenticators: [App\Security\AppCustomAuthenticator]
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
security:
enable_authenticator_manager: true

firewalls:
main:
lazy: true
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
security:
enable_authenticator_manager: true

firewalls:
main:
lazy: true
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
security:
password_hashers:
App\Security\User:
algorithm: {BCRYPT_OR_AUTO}

providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
id: App\Security\UserProvider
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
security:
password_hashers:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email

firewalls:
dev: ~
main: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
security:
password_hashers:
App\Security\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
in_memory: { memory: ~ }
other_provider: { foo: true }
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
id: App\Security\UserProvider

firewalls:
dev: ~
main: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
security:
password_hashers:
App\Security\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
id: App\Security\UserProvider

firewalls:
dev: ~
main: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
security:
enable_authenticator_manager: true
password_hashers:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email

firewalls:
dev:
pattern: ^/(_(profiler|wdt)|css|images|js)/
security: false
main:
lazy: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
security:
enable_authenticator_manager: true

password_hashers:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
security:
password_hashers:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email

firewalls:
dev: ~
main:
anonymous: lazy
provider: app_user_provider
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ security:
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
id: App\Security\UserProvider
id: App\Security\UserProvider
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
security:
# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: username

firewalls:
dev: ~
main: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
security:
# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
id: App\Security\UserProvider

firewalls:
dev: ~
main: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
security:
enable_authenticator_manager: true
encoders:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

# https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email

firewalls:
dev:
pattern: ^/(_(profiler|wdt)|css|images|js)/
security: false
main:
lazy: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
security:
enable_authenticator_manager: true

encoders:
App\Entity\User:
algorithm: {BCRYPT_OR_AUTO}

providers:
# used to reload user from session & other features (e.g. switch_user)
app_user_provider:
entity:
class: App\Entity\User
property: email
2 changes: 1 addition & 1 deletion tests/Security/yaml_fixtures/source/empty_security.yaml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
security: {}
security: {}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
security:
enable_authenticator_manager: true

firewalls:
main:
lazy: true
Expand Down