[Security] Deprecate UserInterface & TokenInterface's eraseCredentials()
#59106
Conversation
src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php
Outdated
Show resolved
Hide resolved
| * Deprecate `UserInterface::eraseCredentials()`, use a dedicated DTO or erase credentials | ||
| on your own e.g. upon `AuthenticationTokenCreatedEvent` instead |
There was a problem hiding this comment.
Would it be possible to use __serialize()/__deserialize() instead to only serialize the intended fields? Or is this something we don't want to encourage?
There was a problem hiding this comment.
Good point, I think the DTO approach is the best but the serialization one is also valid https://stackoverflow.com/questions/42074225/symfony-userinterface-is-serializing-the-entire-massive-user-entity. I'm going to update the deprecation messages and upgrade/changelog instructions to mention it, thanks.
There was a problem hiding this comment.
Thinking more about this, I think we should not suggest it: indefinitely keeping plain credentials on this object is likely to lead to bad situations e.g. having credentials persisted in database or exposed elsewhere e.g. logs
There was a problem hiding this comment.
even if you have then in a non-persisted field of the entity, you will still need to configure serialization (PHP's serialization will not be impacted by which properties are mapped in the ORM metadata)
There was a problem hiding this comment.
and there is also another solution: never writing the plain password at all in your entity by using -h' hash_property_path option of the PasswordType (defined by the appropriate type extension when the password-hasher component is available) to write directly the password hash instead.
a0b8874 to
e622479
Compare
2263cdf to
3a93276
Compare
UserInterface::eraseCredentials()UserInterface & TokenInterface's eraseCredentials()
aece9de to
3f2503c
Compare
|
All comments have been addressed |
|
The verity packages job failure kids |
3f2503c to
c057ba3
Compare
|
@xabbuh Indeed, fixed. Thank you |
| @@ -135,6 +135,9 @@ public function load(array $configs, ContainerBuilder $container): void | ||
|
|
||
| // set some global scalars | ||
| $container->setParameter('security.access.denied_url', $config['access_denied_url']); | ||
| if (true === $config['erase_credentials']) { |
There was a problem hiding this comment.
Why don't we do this check in the MainConfiguration class instead?
|
What's the alternative? Isn't this PR making it easier to inadvertently put passwords in the session storage? |
|
The alternative is to never store plain credentials in the user object especially if it's a doctrine entity but also any object that lives longer than the time required to deal with plain credentials e.g. using a DTO instead as suggested in deprec notices or just an extra form field in case of symfony/form, one that don't map to any property of the User object - that's what |
|
I'm wondering if moving the method Combined with not deprecating the |
|
Based on my experience both as a Symfony core developer and a consultant, we don't need an equivalent given this is not needed for 90+% use case, a few sentences in the documentation are enough instead taking what we already have in term of educative content and docs into account. But maybe I'm wrong, if other core members and good part of Symfony developers think such alternative is needed in core, I will happily reconsider and smoothly turn this config option to false by default + extract this out of UserInterface. |
|
To be clear. I'm fine with completely removing the erase credentials feature. I just figured if others see a roadblock splitting the interface might be a way out to get ride of the empty |
|
👍 I don't think we are there. Also talked about this on stage at SymfonyCon and didn't got any negative reaction, not even questions about this. The practice behind the deprecated feature is subject to trouble, I think the community already moved away from it. |
|
Note that I'm not sure putting hashed passwords in the session storage should really be encouraged, so having a non-empty eraseCrentials (as generated by maker-bundle) might not be a best-practice either. |
|
Let’s go for a dedicated interface then, leaving the config as it is today. |
|
@nicolas-grekas |
|
Maybe the security system should always clone the object it stores before? Because I agree mutating is risky. |
I'm wondering about this also: is the proper solution to implement |
|
Having a method on the class that is unsafe to call without first cloning the object might be a footgun in projects. And this would also require that the object is cloneable, which is not a requirement today (and for instance, I'm totally not sure about whether cloning a Doctrine ORM proxy instance works fine when the proxy has not been initialized yet or whether it produces a broken instance that cannot ever be initialized, while using a Doctrine entity as the user implementation is a common case and so we could end up with a proxy instance) |
|
I don't think serializing an uninitialized entity can happen: it'd mean serializing an entity who's password was never checked. From the pov of the security component, this doesn't make sense. But cloning using |
|
So, it looks like we're good with adding the suggestion of implementing This is somewhat related to #59562 where I explain how the magic method can be used to tweak what's serialized. |
|
Not the alternative I would take but that'll be for a blogpost, let's do it the fast way 👍 Thanks for your help on moving forward here, I'll update this asap. |
…as not stored in the session (nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Don't invalidate the user when the password was not stored in the session | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Related to #59106: this PR does is considering that if `$originalUser` (the object coming from the session) has a null password, then we don't consider it changed from `$refreshedUser`. Aka we don't log out the user in such case. The benefit is allowing to not put the hashed password in the session. I think that's desirable. Commits ------- 3d618db [Security] Don't invalidate the user when the password was not stored in the session
99372fe to
e36259d
Compare
src/Symfony/Component/Ldap/Security/EraseLdapUserCredentialsListener.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Two things:
- I think we should tell about
__serialize, it's a way better alternative that doesn't require mutating the user/token objects. My review notes are in this direction. - I don't like the fact we have yet another option to explicitly configure just to unconfigure it on 8.1, forcing 100% of apps to add the line, and remove it in a few years. I have an idea but I need to code it - doesn't fit as a review comment and might be a dead end :)
src/Symfony/Component/Ldap/Security/EraseLdapUserCredentialsListener.php
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LdapFactoryTrait.php
Show resolved
Hide resolved
|
See #59682 for what I had in mind. |
|
Closing in favor of #59682 |
…`eraseCredentials()` (chalasr, nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Deprecate UserInterface & TokenInterface's `eraseCredentials()` | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | yes | Issues | Fix #57842 | License | MIT As promised, this PR adds a commit on top of #59106 to improve the BC layer. This approach didn't fit in a review comment :) /cc `@chalasr` This PR leverages the new `#[\Deprecated]` attribute to decide if some `eraseCredentials()` method is to be called or not. My target DX here is to save us all (the community) from having to add `erase_credentials: false` configuration in all our apps. So, instead of having to opt-out from the deprecation using this config option, the opt-out is done by adding the attribute on the method: Before: ```php public function eraseCredentials(): void { } ``` After: ```php #[\Deprecated] public function eraseCredentials(): void { } // If your eraseCredentials() method was used to empty a "password" property: public function __serialize(): array { $data = (array) $this; unset($data["\0".self::class."\0password"]); return $data; } ``` This should provide a smoother upgrade path (and maker-bundle could be updated right-away). Commits ------- e5c94e6 [Security] Improve BC-layer to deprecate eraseCredentials methods e556606 [Security] Deprecate `UserInterface` & `TokenInterface`'s `eraseCredentials()`
Better avoid storing plain credentials on the security user object itself. Core no longer needs to handle this behavior to me.