Skip to content

[Security] Decouple passwords from UserInterface #40267

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
Mar 6, 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
82 changes: 82 additions & 0 deletions UPGRADE-5.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,88 @@ Routing
Security
--------

* Deprecate `UserInterface::getPassword()`
If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
you should implement `PasswordAuthenticatedUserInterface`.

Before:
```php
use Symfony\Component\Security\Core\User\UserInterface;

class User implements UserInterface
{
// ...

public function getPassword()
{
return $this->password;
}
}
```

After:
```php
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;

class User implements UserInterface, PasswordAuthenticatedUserInterface
{
// ...

public function getPassword(): ?string
{
return $this->password;
}
}
```

* Deprecate `UserInterface::getSalt()`
If your `getSalt()` method does not return `null` (i.e. you are using password-based authentication with an old password hash algorithm that requires user-provided salts),
implement `LegacyPasswordAuthenticatedUserInterface`.

Before:
```php
use Symfony\Component\Security\Core\User\UserInterface;

class User implements UserInterface
{
// ...

public function getPassword()
{
return $this->password;
}

public function getSalt()
{
return $this->salt;
}
}
```

After:
```php
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;

class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
{
// ...

public function getPassword(): ?string
{
return $this->password;
}

public function getSalt(): ?string
{
return $this->salt;
}
}
```

* Deprecate calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
* Deprecate calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
* Deprecate all classes in the `Core\Encoder\` sub-namespace, use the `PasswordHasher` component instead
* Deprecated voters that do not return a valid decision when calling the `vote` method

Expand Down
84 changes: 84 additions & 0 deletions UPGRADE-6.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,90 @@ Routing
Security
--------

* Remove `UserInterface::getPassword()`
If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
you should implement `PasswordAuthenticatedUserInterface`.

Before:
```php
use Symfony\Component\Security\Core\User\UserInterface;

class User implements UserInterface
{
// ...

public function getPassword()
{
return $this->password;
}
}
```

After:
```php
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;

class User implements UserInterface, PasswordAuthenticatedUserInterface
{
// ...

public function getPassword(): ?string
{
return $this->password;
}
}
```

* Remove `UserInterface::getSalt()`
If your `getSalt()` method does not return `null` (i.e. you are using password-based authentication with an old password hash algorithm that requires user-provided salts),
implement `LegacyPasswordAuthenticatedUserInterface`.

Before:
```php
use Symfony\Component\Security\Core\User\UserInterface;

class User implements UserInterface
{
// ...

public function getPassword()
{
return $this->password;
}

public function getSalt()
{
return $this->salt;
}
}
```

After:
```php
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;

class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
{
// ...

public function getPassword(): ?string
{
return $this->password;
}

public function getSalt(): ?string
{
return $this->salt;
}
}
```

* Calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that
does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`.
* Calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface`
with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`
* Drop all classes in the `Core\Encoder\` sub-namespace, use the `PasswordHasher` component instead
* Drop support for `SessionInterface $session` as constructor argument of `SessionTokenStorage`, inject a `\Symfony\Component\HttpFoundation\RequestStack $requestStack` instead
* Drop support for `session` provided by the ServiceLocator injected in `UsageTrackingTokenStorage`, provide a `request_stack` service instead
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Doctrine\Persistence\ObjectRepository;
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\UserProviderInterface;
Expand Down Expand Up @@ -115,9 +116,15 @@ public function supportsClass(string $class)

/**
* {@inheritdoc}
*
* @final
*/
public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
{
if (!$user instanceof PasswordAuthenticatedUserInterface) {
trigger_deprecation('symfony/doctrine-bridge', '5.3', 'The "%s::upgradePassword()" method expects an instance of "%s" as first argument, the "%s" class should implement it.', PasswordUpgraderInterface::class, PasswordAuthenticatedUserInterface::class, get_debug_type($user));
}

$class = $this->getClass();
if (!$user instanceof $class) {
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_debug_type($user)));
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Bridge/Doctrine/Tests/Fixtures/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\Id;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\UserInterface;

/** @Entity */
class User implements UserInterface
class User implements UserInterface, PasswordAuthenticatedUserInterface
{
/** @Id @Column(type="integer") */
protected $id1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Symfony\Bridge\Doctrine\Tests\DoctrineTestHelper;
use Symfony\Bridge\Doctrine\Tests\Fixtures\User;
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
use Symfony\Component\Security\Core\User\UserInterface;

Expand Down Expand Up @@ -234,4 +235,5 @@ abstract class UserLoaderRepository implements ObjectRepository, UserLoaderInter

abstract class PasswordUpgraderRepository implements ObjectRepository, PasswordUpgraderInterface
{
abstract public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void;
}
4 changes: 2 additions & 2 deletions src/Symfony/Bridge/Doctrine/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"symfony/property-access": "^4.4|^5.0",
"symfony/property-info": "^5.0",
"symfony/proxy-manager-bridge": "^4.4|^5.0",
"symfony/security-core": "^5.0",
"symfony/security-core": "^5.3",
"symfony/expression-language": "^4.4|^5.0",
"symfony/uid": "^5.1",
"symfony/validator": "^5.2",
Expand All @@ -60,7 +60,7 @@
"symfony/messenger": "<4.4",
"symfony/property-info": "<5",
"symfony/security-bundle": "<5",
"symfony/security-core": "<5",
"symfony/security-core": "<5.3",
"symfony/validator": "<5.2"
},
"suggest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder;
use Symfony\Component\Security\Core\Encoder\SodiumPasswordEncoder;
use Symfony\Component\Security\Core\User\ChainUserProvider;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\UserProviderInterface;
use Symfony\Component\Security\Http\Event\CheckPassportEvent;

Expand Down Expand Up @@ -664,6 +665,9 @@ private function createEncoders(array $encoders, ContainerBuilder $container)
{
$encoderMap = [];
foreach ($encoders as $class => $encoder) {
if (class_exists($class) && !is_a($class, PasswordAuthenticatedUserInterface::class, true)) {
trigger_deprecation('symfony/security-bundle', '5.3', 'Configuring an encoder for a user class that does not implement "%s" is deprecated, class "%s" should implement it.', PasswordAuthenticatedUserInterface::class, $class);
}
$encoderMap[$class] = $this->createEncoder($encoder);
}

Expand Down Expand Up @@ -775,6 +779,10 @@ private function createHashers(array $hashers, ContainerBuilder $container)
{
$hasherMap = [];
foreach ($hashers as $class => $hasher) {
// @deprecated since Symfony 5.3, remove the check in 6.0
if (class_exists($class) && !is_a($class, PasswordAuthenticatedUserInterface::class, true)) {
trigger_deprecation('symfony/security-bundle', '5.3', 'Configuring a password hasher for a user class that does not implement "%s" is deprecated, class "%s" should implement it.', PasswordAuthenticatedUserInterface::class, $class);
}
$hasherMap[$class] = $this->createHasher($hasher);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\Security\Core\User\ArrayUserProvider;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\User;
use Symfony\Component\Security\Core\User\UserInterface;

Expand Down Expand Up @@ -78,7 +79,7 @@ public function testUserWillBeMarkedAsChangedIfRolesHasChanged(UserInterface $us
}
}

final class UserWithoutEquatable implements UserInterface
final class UserWithoutEquatable implements UserInterface, PasswordAuthenticatedUserInterface
{
private $username;
private $password;
Expand Down Expand Up @@ -119,7 +120,7 @@ public function getRoles()
/**
* {@inheritdoc}
*/
public function getPassword()
public function getPassword(): ?string
{
return $this->password;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\Ldap\LdapInterface;
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
use Symfony\Component\Security\Core\Exception\LogicException;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials;
use Symfony\Component\Security\Http\Authenticator\Passport\UserPassportInterface;
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
Expand Down Expand Up @@ -68,6 +69,11 @@ public function onCheckPassport(CheckPassportEvent $event)
throw new BadCredentialsException('The presented password cannot be empty.');
}

$user = $passport->getUser();
if (!$user instanceof PasswordAuthenticatedUserInterface) {
trigger_deprecation('symfony/ldap', '5.3', 'Not implementing the "%s" interface in class "%s" while using password-based authenticators is deprecated.', PasswordAuthenticatedUserInterface::class, get_debug_type($user));
}

/** @var LdapInterface $ldap */
$ldap = $this->ldapLocator->get($ldapBadge->getLdapServiceId());
try {
Expand All @@ -77,7 +83,7 @@ public function onCheckPassport(CheckPassportEvent $event)
} else {
throw new LogicException('Using the "query_string" config without using a "search_dn" and a "search_password" is not supported.');
}
$username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_FILTER);
$username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_FILTER);
$query = str_replace('{username}', $username, $ldapBadge->getQueryString());
$result = $ldap->query($ldapBadge->getDnString(), $query)->execute();
if (1 !== $result->count()) {
Expand All @@ -86,7 +92,7 @@ public function onCheckPassport(CheckPassportEvent $event)

$dn = $result[0]->getDn();
} else {
$username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_DN);
$username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_DN);
$dn = str_replace('{username}', $username, $ldapBadge->getDnString());
}

Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Ldap/Security/LdapUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@

use Symfony\Component\Ldap\Entry;
use Symfony\Component\Security\Core\User\EquatableInterface;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\UserInterface;

/**
* @author Robin Chalas <robin.chalas@gmail.com>
*
* @final
*/
class LdapUser implements UserInterface, EquatableInterface
class LdapUser implements UserInterface, PasswordAuthenticatedUserInterface, EquatableInterface
{
private $entry;
private $username;
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Ldap/Security/LdapUserProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ public function refreshUser(UserInterface $user)

/**
* {@inheritdoc}
*
* @final
*/
public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
{
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Ldap/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@
"ext-ldap": "*"
},
"require-dev": {
"symfony/security-core": "^5.0"
"symfony/security-core": "^5.3"
},
"conflict": {
"symfony/options-resolver": "<4.4",
"symfony/security-core": "<5"
"symfony/security-core": "<5.3"
},
"autoload": {
"psr-4": { "Symfony\\Component\\Ldap\\": "" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\PasswordHasher\Hasher;

use Symfony\Component\PasswordHasher\PasswordHasherInterface;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\UserInterface;

/**
Expand All @@ -25,7 +26,7 @@ interface PasswordHasherFactoryInterface
/**
* Returns the password hasher to use for the given user.
*
* @param UserInterface|string $user A UserInterface instance or a class name
* @param PasswordAuthenticatedUserInterface|UserInterface|string $user A PasswordAuthenticatedUserInterface/UserInterface instance or a class name
*
* @throws \RuntimeException When no password hasher could be found for the user
*/
Expand Down
Loading