From#56830 (review)

This should target 7.2 as this is a new feature.
@chalasr can you please have another look?
I don't like adding new options so if there's a way without, suggestions welcome :)

@nicolas-grekas unless we decide to revert a security fix we cannot avoid the option I think

I don't like adding new options so if there's a way without, suggestions welcome :)

Maybe an unpopular opinion: I'm not sure if I would implement this feature in Symfony at all.

This is part of code that was released with theuser enumeration CVE. With the new option set to true, you'll reintroduce the possibility of user enumeration based on error messages as you now get "Invalid credentials" (user does not exists) or "This account is locked" (account status error).

Thehide_user_not_found option name might be a bit of a vague name, but it determines whether you want some default user enumeration prevention or not. If a business decides it wants partial user enumeration (by hiding only user not found exceptions), you can already achieve this by settinghide_user_not_found: false and implementing your own authentication failure handler that hides onlyUserNotFoundException.

I'm not convinced we need to provide an extra option to achieve this (especially because we consider the new feature less secure).

This is part of code that was released with theuser enumeration CVE. With the new option set to true, you'll reintroduce the possibility of user enumeration based on error messages as you now get "Invalid credentials" (user does not exists) or "This account is locked" (account status error).

I agree, in theory you could use this to enumerate all users that have anaccount status error, butuser does not exist errors are still silenced with solution. But you may also enumerate users by the time it takes to generate the response at all, if you want to use all possible ways ;)

The current solution looks broken to me. You either expose ALL user information or no user feedback at all. There is no way to inform the user, that it is currently not possible to login with the user. The current solution resulted in several service desk tickets that I need to check every time.

What about creating a new enum config key with three different states like:expose_security_errors: none|account|all?

Renaming/changing the existing option looks sensible to me as account status errors means that a user is actually found.

Changed the config key tohide_account_status_exceptions and applied the proposed changes.

Anything left to do@chalasr ?

Sorry for the back and forths but I think there is confusion about my last comment:

Renaming/changing the existing option looks sensible to me as account status errors means that a user is actually found.

I agree with Wouter this is not worth a dedicated option, but I do think it's worth renaming the existinghide_user_not_found_exceptions option for clarity to make it take an enum as you proposed in#58300 (comment). It should still handle true/false and behave the same as today for BC until 8.0.

So you're fine with deprecating the existinghide_user_not_found_exceptions option in favor of something likeexpose_security_errors with the statesnone,account,all?

And if you are using the newoption, it should override thehide_user_not_found_exceptions setting?

Yes. The new option should win over the to-be-deprecated one when it's explicitly set (to not change the behavior of apps explicitly setting the old option to false). The new option should default toall to keep the same default behavior as today.

@wouterj are you ok with the proposed plan?

Ok, let's go with an enum option with these conditions (maybe worth calling the middle oneaccount_status to be super clear on what errors are exposed).

I guess the easiest BC way would be removing the default value from the current option adding adding the new option with an'all' default value. Then, in the extension use the old option if it's defined (which means the user has configured it explicitly) + a deprecation notice. Otherwise - if the old option was not configured - use the new option.
When the old option is used, the extension can then transform true to'all' and false to'none'.

Sounds good 👍

Thanks for the review@stof :)

Applied your suggestions and the build is passing

@core23 Can you please update the UPGRADE-7.3 file about the deprecated option? And add a line about that deprecation in the already updated CHANGELOG file also?

Done :)

Any chance to get this merge soon-ish?@chalasr

Done. The failing tests look unrelated to the changes

Thank you@core23.