Hi! Thanks for the series of security feature PRs!

However, I have some doubts about this particular feature:

A) If we ignore length constraints, all other constraints seem more suited in a registration process and not an authentication process. I can't recall seeing a website that tells me I have an unsafe password during authentication. For the basic constraints like length, I don't think we need to integrate with the Validator component (we already check on length btw)

B) The badges and passport listeners are created to centralize security sensitive code. I don't consider this validator a security critical feature. Also, given the validator component has its own simple high level API, I think using it directly in your own listener or authenticator instead of this badge is more clear of the intent.

The use case we talked about with @Spomky made sense to me:

By having constraints as a badge, we can then easily implement login strategies where a password reset is enforced when ppl log in with a weak password (when some policy is strengthened).

Hi @wouterj,

Many thanks for your feedback and your advice.
The main idea is that I wanted to prevent login with a compromised password. At first sight, I duplicated the logic from the NotCompromised constraint, but after a short discussion with @nicolas-grekas we went to the idea of a badge that could verify the input against any other constraint.

@wouterj WDYT of our responses to your comment?

Sorry, I have been thinking about this PR the past weeks, but I didn't write down my thoughts here.

I think the "prevent login with a compromised password" idea is very interesting. This is becoming a more common practice, and is also recommended by NIST. However, I think I would like a very scoped badge rather than a generic badge like this one, to "limit" the usages, which makes it easier to make changes on later on and discourage bad practices (e.g. requiring a password to contain at least 1 special character, etc.).

Two other things I have in mind are:

• We were cautious with storing plaintext passwords in badges (maybe overly so). E.g. the PasswordCredentials automatically clears the plaintext password after it is checked. So I'm a bit apprehensive with introducing another badge containing the plaintext password (I realize we did add an PasswordUpgradeBadge before though).
• I'm not that fond of configuring a "password policy" (rules to determine if a user should change their password) in an authenticator (this also applies to PasswordUpgradeBadge). I would rather have the authenticator only add a PasswordCredentials and then have a PasswordPolicyListener that checks whether the password passes the policy or not. We might even add this as a new configuration option security.password_policy or the like to configure the exact rules that must be checked on the password.

I agree such concern would be better handled outside of the authenticator despite the "opportunistic upon login" aspect.

Just a dummy example, we might do something like this:

namespace Symfony\Component\PasswordHasher\Policy;

interface PasswordPolicy
{
    public function accept(string $plaintextPassword): bool;
}

security:
    password_policy:
        - NotCompromisedPolicy
        - 'App\Security\CustomPolicy'

A listener on CheckPassportEvent would then check a PasswordCredentials with these policies and block authentication before the CheckCredentialsListener is executed (necessary to prevent user enumeration).

We could extract the haveibeenpwned integration from the Validation component (into PasswordHasher?) and reuse it in both the constraint and policy. Maybe we implement some more policies, but we should make sure to only implement policies following best practices (e.g. from NIST). This last point is also why I'm not keen to integrate with the Validator component, this makes it way to easy to follow bad practices imho.

This would make it quite easy to enforce these policies, while still allowing users to implement not-best practice policies when necessary (e.g. a client of us requires us to have an "update password every x months" policy).

Many thanks for your feedback.

However, I think I would like a very scoped badge rather than a generic badge like this one, to "limit" the usages, which makes it easier to make changes on later on and discourage bad practices (e.g. requiring a password to contain at least 1 special character, etc.).

To be honest, this is what I usually do and that was used in recent workshops (badge and listener). It works fine and most of the code is mostly copied from the constraint.
If you are ok with copy/paste, I will change it in a dedicated badge

We were cautious with storing plaintext passwords in badges (maybe overly so)

OK noted. When the badge is marked as resolved, I will clear it.

I would rather have the authenticator only add a PasswordCredentials and then have a PasswordPolicyListener

OK. To me, this means this PasswordPolicyListener will be executed before any other listener and especially the PasswordCredentialsListener otherwise the password is cleared.

Just a dummy example, we might do something like this:

I understand the logic. Let me time for implementing it.

I understand the logic. Let me time for implementing it.

Cool, thanks for continueing this. Take it easy, the next future freeze is far away 🙂 and feel free to ping me if you need help getting this ready for 6.4.

Cool, thanks for continueing this. Take it easy, the next future freeze is far away 🙂 and feel free to ping me if you need help getting this ready for 6.4.

🫢I took time to redo this PR (only the component). I concur with you, the design is much better now.

Many thanks @stof. I addressed your comments.

I have an issue with the test that fails. It looks like the value of $config['password_policy'] is always an empty list. It is correctly populated in $configs, but seems to be replaced by the configuration processor call.
@wouterj suggested to change ->fixXmlConfig('password_policy', 'password_policy') in MainConfiguration but the result is always the same.
Do you have any clue on what is causing this issue? I certainly missed something stupid, but I cannot figure out what?

Well, I don't think using the same name for the singular and the plural works fine. I think you should use an actual plural name password_policies, which will solve this issue of using a single and plural with the same value.

That was my assumption, unfortunately the service list is still empty

Closing as the concept should be revised in depth.