10000 [Security] Decouple passwords from UserInterface · symfony/symfony@4063d16 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4063d16

Browse files
chalasrchalasr
committed
[Security] Decouple passwords from UserInterface
1 parent 4d91b8f commit 4063d16

27 files changed

+471
-77
lines changed

‎UPGRADE-5.3.md

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,88 @@ PropertyInfo
7171
Security
7272
--------
7373

74+
* Deprecate `UserInterface::getPassword()`
75+
If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
76+
you should implement `PasswordAuthenticatedUserInterface`.
77+
78+
Before:
79+
```php
80+
use Symfony\Component\Security\Core\User\UserInterface;
81+
82+
class User implements UserInterface
83+
{
84+
// ...
85+
86+
public function getPassword()
87+
{
88+
return $this->password;
89+
}
90+
}
91+
```
92+
93+
After:
94+
```php
95+
use Symfony\Component\Security\Core\User\UserInterface;
96+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
97+
98+
class User implements UserInterface, PasswordAuthenticatedUserInterface
99+
{
100+
// ...
101+
102+
public function getPassword(): ?string
103+
{
104+
return $this->password;
105+
}
106+
}
107+
```
108+
109+
* Deprecate `UserInterface::getSalt()`
110+
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),
111+
implement `LegacyPasswordAuthenticatedUserInterface`.
112+
113+
Before:
114+
```php
115+
use Symfony\Component\Security\Core\User\UserInterface;
116+
117+
class User implements UserInterface
118+
{
119+
// ...
120+
121+
public function getPassword()
122+
{
123+
return $this->password;
124+
}
125+
126+
public function getSalt()
127+
{
128+
return $this->salt;
129+
}
130+
}
131+
```
132+
133+
After:
134+
```php
135+
use Symfony\Component\Security\Core\User\UserInterface;
136+
use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
137+
138+
class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
139+
{
140+
// ...
141+
142+
public function getPassword(): ?string
143+
{
144+
return $this->password;
145+
}
146+
147+
public function getSalt(): ?string
148+
{
149+
return $this->salt;
150+
}
151+
}
152+
```
153+
154+
* Deprecate calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
155+
* Deprecate calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
74156
* Deprecate all classes in the `Core\Encoder\` sub-namespace, use the `PasswordHasher` component instead
75157
* Deprecated voters that do not return a valid decision when calling the `vote` method
76158

‎UPGRADE-6.0.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,90 @@ Routing
168168
Security
169169
--------
170170

171+
* Remove `UserInterface::getPassword()`
172+
If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
173+
you should implement `PasswordAuthenticatedUserInterface`.
174+
175+
Before:
176+
```php
177+
use Symfony\Component\Security\Core\User\UserInterface;
178+
179+
class User implements UserInterface
180+
{
181+
// ...
182+
183+
public function getPassword()
184+
{
185+
return $this->password;
186+
}
187+
}
188+
```
189+
190+
After:
191+
```php
192+
use Symfony\Component\Security\Core\User\UserInterface;
193+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
194+
195+
class User implements UserInterface, PasswordAuthenticatedUserInterface
196+
{
197+
// ...
198+
199+
public function getPassword(): ?string
200+
{
201+
return $this->password;
202+
}
203+
}
204+
```
205+
206+
* Remove `UserInterface::getSalt()`
207+
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),
208+
implement `LegacyPasswordAuthenticatedUserInterface`.
209+
210+
Before:
211+
```php
212+
use Symfony\Component\Security\Core\User\UserInterface;
213+
214+
class User implements UserInterface
215+
{
216+
// ...
217+
218+
public function getPassword()
219+
{
220+
return $this->password;
221+
}
222+
223+
public function getSalt()
224+
{
225+
return $this->salt;
226+
}
227+
}
228+
```
229+
230+
After:
231+
```php
232+
use Symfony\Component\Security\Core\User\UserInterface;
233+
use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
234+
235+
class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
236+
{
237+
// ...
238+
239+
public function getPassword(): ?string
240+
{
241+
return $this->password;
242+
}
243+
244+
public function getSalt(): ?string
245+
{
246+
return $this->salt;
247+
}
248+
}
249+
```
250+
251+
* Calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that
252+
does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`.
253+
* Calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface`
254+
with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`
171255
* Drop all classes in the `Core\Encoder\` sub-namespace, use the `PasswordHasher` component instead
172256
* Drop support for `SessionInterface $session` as constructor argument of `SessionTokenStorage`, inject a `\Symfony\Component\HttpFoundation\RequestStack $requestStack` instead
173257
* Drop support for `session` provided by the ServiceLocator injected in `UsageTrackingTokenStorage`, provide a `request_stack` service instead

‎src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Doctrine\Persistence\ObjectRepository;
1818
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
1919
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
20+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
2021
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
2122
use Symfony\Component\Security\Core\User\UserInterface;
2223
use Symfony\Component\Security\Core\User\UserProviderInterface;
@@ -116,8 +117,12 @@ public function supportsClass(string $class)
116117
/**
117118
* {@inheritdoc}
118119
*/
119-
public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
120+
public function upgradePassword($user, string $newEncodedPassword): void
120121
{
122+
if (!$user instanceof PasswordAuthenticatedUserInterface) {
123+
trigger_deprecation('symfony/security-core', '5.3', 'The first argument of "%s:upgradePassword()" method should be an instance of "%s", you should make the "%s" class implement it.', PasswordUpgraderInterface::class, PasswordAuthenticatedUserInterface::class, \get_class($user));
124+
}
125+
121126
$class = $this->getClass();
122127
if (!$user instanceof $class) {
123128
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_debug_type($user)));

‎src/Symfony/Bridge/Doctrine/Tests/Fixtures/User.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,11 @@
1414
use Doctrine\ORM\Mapping\Column;
1515
use Doctrine\ORM\Mapping\Entity;
1616
use Doctrine\ORM\Mapping\Id;
17+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
1718
use Symfony\Component\Security\Core\User\UserInterface;
1819

1920
/** @Entity */
20-
class User implements UserInterface
21+
class User implements UserInterface, PasswordAuthenticatedUserInterface
2122
{
2223
/** @Id @Column(type="integer") */
2324
protected $id1;

‎src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
namespace Symfony\Bridge\Doctrine\Tests\Security\User;
1313

1414
use Doctrine\ORM\EntityManager;
15+
use Doctrine\ORM\EntityManagerInterface;
16+
use Doctrine\ORM\EntityRepository;
17+
use Doctrine\ORM\Mapping;
1518
use Doctrine\ORM\Tools\SchemaTool;
1619
use Doctrine\Persistence\ManagerRegistry;
1720
use Doctrine\Persistence\ObjectManager;
@@ -22,6 +25,7 @@
2225
use Symfony\Bridge\Doctrine\Tests\DoctrineTestHelper;
2326
use Symfony\Bridge\Doctrine\Tests\Fixtures\User;
2427
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
28+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
2529
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
2630
use Symfony\Component\Security\Core\User\UserInterface;
2731

@@ -183,17 +187,15 @@ public function testPasswordUpgrades()
183187
{
184188
$user = new User(1, 1, 'user1');
185189

186-
$repository = $this->createMock(PasswordUpgraderRepository::class);
187-
$repository->expects($this->once())
188-
->method('upgradePassword')
189-
->with($user, 'foobar');
190+
$repository = new PasswordUpgraderRepository();
190191

191192
$provider = new EntityUserProvider(
192193
$this->getManager($this->getObjectManager($repository)),
193194
'Symfony\Bridge\Doctrine\Tests\Fixtures\User'
194195
);
195196

196197
$provider->upgradePassword($user, 'foobar');
198+
$this->assertSame(1, $repository->upgradeCount);
197199
}
198200

199201
private function getManager($em, $name = null)
@@ -232,6 +234,16 @@ abstract class UserLoaderRepository implements ObjectRepository, UserLoaderInter
232234
{
233235
}
234236

235-
abstract class PasswordUpgraderRepository implements ObjectRepository, PasswordUpgraderInterface
237+
class PasswordUpgraderRepository extends EntityRepository implements PasswordUpgraderInterface
236238
{
239+
public $upgradeCount = 0;
240+
241+
public function __construct()
242+
{
243+
}
244+
245+
public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void
246+
{
247+
++$this->upgradeCount;
248+
}
237249
}

‎src/Symfony/Bridge/Doctrine/composer.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
"symfony/property-access": "^4.4|^5.0",
3939
"symfony/property-info": "^5.0",
4040
"symfony/proxy-manager-bridge": "^4.4|^5.0",
41-
"symfony/security-core": "^5.0",
41+
"symfony/security-core": "^5.3",
4242
"symfony/expression-language": "^4.4|^5.0",
4343
"symfony/uid": "^5.1",
4444
"symfony/validator": "^5.2",
@@ -60,7 +60,7 @@
6060
"symfony/messenger": "<4.4",
6161
"symfony/property-info": "<5",
6262
"symfony/security-bundle": "<5",
63-
"symfony/security-core": "<5",
63+
"symfony/security-core": "<5.3",
6464
"symfony/validator": "<5.2"
6565
},
6666
"suggest": {

‎src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\Security\Core\User\ArrayUserProvider;
1515
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
16+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
1617
use Symfony\Component\Security\Core\User\User;
1718
use Symfony\Component\Security\Core\User\UserInterface;
1819

@@ -78,7 +79,7 @@ public function testUserWillBeMarkedAsChangedIfRolesHasChanged(UserInterface $us
7879
}
7980
}
8081

81-
final class UserWithoutEquatable implements UserInterface
82+
final class UserWithoutEquatable implements UserInterface, PasswordAuthenticatedUserInterface
8283
{
8384
private $username;
8485
private $password;
@@ -119,7 +120,7 @@ public function getRoles()
119120
/**
120121
* {@inheritdoc}
121122
*/
122-
public function getPassword()
123+
public function getPassword(): ?string
123124
{
124125
return $this->password;
125126
}
10000

‎src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\Ldap\LdapInterface;
1818
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
1919
use Symfony\Component\Security\Core\Exception\LogicException;
20+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
2021
use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials;
2122
use Symfony\Component\Security\Http\Authenticator\Passport\UserPassportInterface;
2223
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
@@ -68,6 +69,11 @@ public function onCheckPassport(CheckPassportEvent $event)
6869
throw new BadCredentialsException('The presented password cannot be empty.');
6970
}
7071

72+
$user = $passport->getUser();
73+
if (!$user instanceof PasswordAuthenticatedUserInterface) {
74+
trigger_deprecation('symfony/ldap', '5.3', 'Not implementing the "%s" interface in class "%s" while using password-based authenticators is deprecated.', PasswordAuthenticatedUserInterface::class, \get_class($user));
75+
}
76+
7177
/** @var LdapInterface $ldap */
7278
$ldap = $this->ldapLocator->get($ldapBadge->getLdapServiceId());
7379
try {
@@ -77,7 +83,7 @@ public function onCheckPassport(CheckPassportEvent $event)
7783
} else {
7884
throw new LogicException('Using the "query_string" config without using a "search_dn" and a "search_password" is not supported.');
7985
}
80-
$username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_FILTER);
86+
$username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_FILTER);
8187
$query = str_replace('{username}', $username, $ldapBadge->getQueryString());
8288
$result = $ldap->query($ldapBadge->getDnString(), $query)->execute();
8389
if (1 !== $result->count()) {
@@ -86,7 +92,7 @@ public function onCheckPassport(CheckPassportEvent $event)
8692

8793
$dn = $result[0]->getDn();
8894
} else {
89-
$username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_DN);
95+
$username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_DN);
9096
$dn = str_replace('{username}', $username, $ldapBadge->getDnString());
9197
}
9298

‎src/Symfony/Component/Ldap/Security/LdapUser.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,15 @@
1313

1414
use Symfony\Component\Ldap\Entry;
1515
use Symfony\Component\Security\Core\User\EquatableInterface;
16+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
1617
use Symfony\Component\Security\Core\User\UserInterface;
1718

1819
/**
1920
* @author Robin Chalas <robin.chalas@gmail.com>
2021
*
2122
* @final
2223
*/
23-
class LdapUser implements UserInterface, EquatableInterface
24+
class LdapUser implements UserInterface, PasswordAuthenticatedUserInterface, EquatableInterface
2425
{
2526
private $entry;
2627
private $username;

‎src/Symfony/Component/Ldap/Security/LdapUserProvider.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
1919
use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
2020
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
21+
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
2122
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
2223
use Symfony\Component\Security\Core\User\UserInterface;
2324
use Symfony\Component\Security\Core\User\UserProviderInterface;
@@ -123,7 +124,7 @@ public function refreshUser(UserInterface $user)
123124
/**
124125
* {@inheritdoc}
125126
*/
126-
public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
127+
public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newEncodedPassword): void
127128
{
128129
if (!$user instanceof LdapUser) {
129130
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_debug_type($user)));

0 commit comments

Comments
 (0)
0