[Security] Make it possible to inherit MessageDigestPasswordHasher #41696
Conversation
|
What do you mean by "it breaks them" ? This private method is in a new class, not in an existing class. |
|
Well if your application uses MessageDigestPasswordEncoder then an overwritten mergePasswordAndSalt method will never be used, as the code was copied to MessageDigestPasswordHasher and MessageDigestPasswordEncoder now uses MessageDigestPasswordHasher via LegacyEncoderTrait So then the next step is, like the deprecation message suggest, to use MessageDigestPasswordHasher directly. But as mergePasswordAndSalt is private, mergePasswordAndSalt cannot also not be overwritten. Both cases break current apps that need to overwrite mergePasswordAndSalt. |
|
If MessageDigestPasswordEncoder broke its extension point, then this is indeed a bug that should be fixed. However, I'm not sure we should make |
|
I am not sure what the advantage of using private methods or All it does is to force me to duplicate code. Making the method |
|
Making the method protected has a maintenance cost, as it forces us to preserve BC on it, forbidding us to refactor things in some ways (for instance, here, we will have to revert the usage of the LegacyEncoderTrait in that class). |
|
I see. Well I tried 😉 Should I close this PR? |
|
As explained by @stof, this PR is not a bugfix. And I agree with his suggestion to implement the new interface directly. |
|
@derrabus we should still fix the regression in MessageDigestPasswordEncoder which was refactored to use the LegacyEncoderTrait without accounting for the fact that it was extendable with a protected method though. |
…dEncoder (derrabus) This PR was merged into the 5.3 branch. Discussion ---------- [Security] Restore extension point in MessageDigestPasswordEncoder | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #41696 (comment) | License | MIT | Doc PR | N/A Until Symfony 5.2, it was possible to extend `MessageDigestPasswordEncoder` and override the way password and salt are merged. This broke with #39802. I've restored the old logic and added a test case to cover that scenario. Commits ------- 4568876 [Security] Restore extension point in MessageDigestPasswordEncoder
Symfony 5.3 breaks existing installations that inherit MessageDigestPasswordEncoder (MessageDigestPasswordHasher) because the mergePasswordAndSalt method is private and thus cannot be overwritten.