[Security] Warn for implementing eraseCredentials

[rvanlaak]

... as implementing eraseCredentials on a Doctrine entity will be flushed. Setting password to null will actually be saved at every login attempt.

This might also could be a warning with the UserInterface docblock directly (?)

[javiereguiluz]

In addition to being "dangerous", this method is very confusing, specially for newcomers. Should we deprecate it from the User interface? Or maybe we should wait for #21068, which could remove the UserInterface.

[rvanlaak]

I'm not really sure about what it does too, maybe improve the docblock on the interface instead of deprecating it? Basically it is there to clear sensitive data from the session right? That's great, except if the subject is a doctrine entity that gets flushed ;-)

[xabbuh]

Maybe change this to warn that you shouldn't clear data here that are mapped to the underlying database?

[rvanlaak]

Yes that's the obvious thing, I don't want to clear my database stuff. The thing is that I wasn't aware that eraseCredentials was executed every request, so somehow exactly that should be mentioned. This could also be deprecated on the interface, and transformed into an event?

[javiereguiluz]

👍

[HeahDude]

Hello @rvanlaak, thank you for opening this PR :).

I'm sorry I don't really get the issue you're trying to solve. The method ereaseCredentials is only about clearing any plain values (99,99% of cases the plain password) which are not meant to be in any database, ever.

Could you explain a bit more why we should add this caution instead of trying to better describe the interface?

[rvanlaak]

@HeahDude the reason for the notice is that the security component is really flexible. It allows you to implement the UserInterface on a Doctrine entity directly. Implementing eraseCredentials (e.g. clearing the password) can have side effects, as you don't expect the password to be removed.

A better implementation would be to decouple this and have a layer in between the security component and entity, as @iltar describes here: https://stovepipe.systems/post/decoupling-your-security-user

[xabbuh]

@rvanlaak If you cleared the password through the eraseCredentials() call, this means that you would have stored the plaintext password in the database which is definitely a bad idea. Clearing credentials does not mean that you should throw away the hashed password. Is that something that could be made more clear?

[HeahDude]

Thanks @rvanlaak for your response, but I think we don't understand each other :). When I say

The method ereaseCredentials is only about clearing any plain values

You say

Implementing eraseCredentials (e.g. clearing the password) can have side effects, as you don't expect the password to be removed

But this sounds wrong to me, this is a bad usage of the interface, so I think we should better explain it rather than adding a tip as you suggest (ref https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/User/UserInterface.php#L80-L87):

Removes sensitive data from the user.
This is important if, at any given point, sensitive information like the plain-text password is stored on this object.

[rvanlaak]

... well, as I now agree with all these arguments, we actually back then implemented it according to the cookbook: https://symfony.com/doc/2.2/cookbook/doctrine/registration_form.html

So, yes as back then I just started with Symfony, I think there are some more documentation improvements here.

[HeahDude]

Would like to work further on this @rvanlaak or should we close here?

[rvanlaak]

@HeahDude yes, I'll improve the message.

The method ereaseCredentials is only about clearing any plain values (99,99% of cases the plain password) which are not meant to be in any database, ever.

That actually is exactly what I think should clarified just a bit better. Yes, there never should be a plain password in the user entity, so that's why the method can be confusing when you directly implement the interface on the doctrine entity. Back then we expected the user entity to do some magic in the security component core classes.

Should we improve the interface docblock too?

/**
 * Removes sensitive data from the user.
 *
 *Implement this method if, at any given point, sensitive information like
 *the plain-text password is stored on this object. Make sure to not clear
 *the hashed password on the object, as that then will be cleared as well.
 */
public function eraseCredentials();

[xabbuh]

What about something like:

The earaseCredentials() method is only meant to clean up possibly stored plain text passwords (or similar credentials). Be careful what to erase if your user class is also mapped to a database as the modified object will likely be persisted during the request.

[rvanlaak]

@xabbuh sounds good! Should it be added both in the interface itself and the docs, or you think only adding it to the docs is enough?

[xabbuh]

IMO it's enough to have this in the docs.

[rvanlaak]

Okay great! @xabbuh I've updated the PR

[xabbuh]

👍

[xabbuh]

@HeahDude good for you too now?

[HeahDude]

Great! Thanks for this PR @rvanlaak!

[xabbuh]

Thank you @rvanlaak.