The username constraint compare the submited password with the login in database and not with the login in form

yeah, the api didn't have a way to support this. I've modified the constraint API to use

function ...($constraint, $account) {

Instead of:

function ...($pass, $constraint, $uid) {

uid and pass are still available because we add them to the account object before we pass it into validation, but now we also have the username as well, so we can check and see if the new password matches the new (or current) username. We also avoid a couple user_loads() as a result. I've tested this locally and it seems to work well.

Ah, a patch file would help :)

I agree this seems like a fundamental problem with this constraint. Unfortunately I don't think an API change for the 6.x version is appropriate at this point.

This seems like a security issue. If you have the username constraint enabled, you expect that passwords will never be the username. Since the constraint does not actually work in cases, you are misled into thinking your accounts are secure against unauthorized access by attackers guessing the username as the password.

I think this constraint should be removed from Password Policy 6.x-1.x since the API does not correctly support it and it gives a false impression of security.

I agree that this is a functionality issue, that leads to lessened security. I think this should likely be fixed in place, however, as this module is still widely used in D6 and should not be dropped after release.

This patch fixes the bug where the username constraint is not applied when the username is changed on the user register form (both cases mentioned in the issue summary).

A difficulty was that the username value from the form was not available to the constraint validation function. I solved this by making an exception just for the username constraint: password_policy.module passes the username constraint validate function an extra username parameter. I could not think of a more elegant solution.

The JavaScript password checking is not perfect: If you set a password, then change the username, the password checking will not update until you refocus the password input. I could not figure out a simple way to trigger the password checking from the username input.

Uploaded the wrong patch file in previous comment.

I hadn't previously noticed the patch in #2 which attempts to fix this bug in D6. It takes a different approach than my patch.

There are at least two ways to fix the bug:
1. Stuff the username into the $account object and change the constraint API to pass it instead of the uid.
2. Make an exception to the constraint API for the username constraint.

I opted for the second approach. I think it is the better short-term solution: The scope of the changes is small and there is not much risk of breaking other functionality. The API should probably eventually be generalized though, like in #2, to pass whatever extra validation parameters are needed. For instance, if you have a user profile provided by the profile2 module that contains the user's real name, you might want to have a constraint that disallows passwords containing the real name, but you'd need some way to pass it to the constraint validate function. Also, most of the constraints don't need uid in the validate function, yet it is passed anyway.

This is really a gaping whole in the API itself. Ideally we should change everything to pass the account object, but that will obviously require significant work. This should speed up the performance as well, because we are not running needless user_load()'s on every constraint. Any guess how much work this would be?

I think we need an object more generic than account that can contain any field values from the user registration form. If you use a module that supplements the registration form with fields, such as Profile 2, you need to pass those field values to the constraint validate(), otherwise it is impossible to validate on those values. Right now, the account object that is created in the password_policy validation code is not a true account object returned from user_load()--instead it is a partial account object built from form values since the form values are not yet stored in the database--so it might as well be some more generic validation-parameters object.

Maybe my point is a semantic one, in that I agree an object is needed that can hold account and user registration data, just I think it should not be the "account" object that is strictly a subset of the data normally returned from a user_load().

In case it's not clear what I'm trying to say, suppose I write a module that uses hook_form_alter() to add a birthday field to the user registration form. I would like to write a custom, site-specific constraint to make sure the user does not use their birthday as the password. The constraint will only be possible if password_policy passes to validate() all values from the user registration form, since the birthday is not yet stored in the database.

I completely agree with what you're saying and I think that's perfectly valid feedback for the 7.x-2.x branch. For now, to keep the API as similar as possible, we should just convert the $uid parameter to a user object and leave it at that.

I'd love to see a feature request added to 7.x-2.x to accomodate additional user modules like Profile2.

That seems reasonable. Re your question in #12, I'm not familiar enough with the password_policy code to make a good guess, but I'm pretty sure it'd take me at least a few hours, as I'm no Drupal/PHP maven. I may not have time to attempt a patch for a week or so, so maybe someone else would want to give it a go? (frankcarey?)

The patch I provided might be more appropriate for D6 than D7, since we would probably not want to change the constraint API there, and the patch does not change the API except for constraint_username. (People might have custom constraints written on the constraint API that would be broken.) I think this bug is one that ought to be fixed in D6, particularly since there may be security implications. Do you agree, erikwebb? Should I backport the patch?

Currently there's no real way to "implement your own constraints" without patching the module itself. Because I haven't seen any issues that mention people doing this, I'm not too concerned with changing the API to a small degree like this. I'd be more concerned if we were reordering parameters. Overall I think the disruption caused by introducing one constraint that behaves differently than the others is a larger problem.

Custom code to work around this change would be simple anyway (and mostly support either version of the parameter list) -

function password_policy_constraint_username_validate($password, $constraint, $account) {
  if (is_int($account)) {
    $account = user_load($account);
  }
  ...
}

Currently there's no real way to "implement your own constraints" without patching the module itself. Because I haven't seen any issues that mention people doing this, I'm not too concerned with changing the API to a small degree like this.

You're right. We do this to add constraints to help us comply with our enterprise password policy, but we are probably the exception.

Overall I think the disruption caused by introducing one constraint that behaves differently than the others is a larger problem.

Agreed. I thought though it was worth introducing a temporary nonideality to the code to fix the bug. I will plan to make an API change when I get the opportunity since it seems you want a long-term solution straight away.

Here is the D6 version of the fix in #10 we are using for our sites, in case anyone needs this bug fixed before a release-worthy patch is developed.

I'm afraid this may introduce some PHP issues. Code like this -
if (isset($account->force_password_change) && $account->force_password_change) {

If this is fired on a user registration page, then $account probably won't exist yet and will cause an notice/warning/something. I'm not in a position to test this weekend, but would you mind testing these on a new user registration?

$account should always exist due to this preexisting code in password_policy.module which precedes the validate function calls:

  $account = isset($form['#user']) ? $form['#user'] : (object)array('uid' => 0);

For good measure I will test user registration with all constraints enabled. I believe I have already tested this patch for user registration with the delay constraint enabled.

No notices/warnings/errors at user registration with all constraints enabled.

Incidentally, there seems to be a preexisting bug with the delay constraint: If a new user first sets their password within X hours of registering, where X is the delay constraint value, validation fails. I verified this on unmodified password_policy code. I have not checked yet whether it is reported elsewhere.

The delay constraint bug has been reported in #1694448: Delay constraint interferes with user registration.

The patch in #19 has no known problems and needs review.

Because it's such a large patch, I'd like to get a third opinion if possible. I'll apply it myself and test today.

The patch could be put into 7.x-1.x-dev and thereby get tested by anyone using 7.x-1.x-dev.

The usage statistics don't even indicate any usage for 7.x-1.x-dev. I think I'm okay with putting this in since all of the tests still pass. The array of constraints is the one place we have a comfortable amount of tests written.

http://drupalcode.org/project/password_policy.git/commit/dfb061d

Thanks dw317 for your patch. Applied and tested: it fixes the issue.
Awaiting now a new release version...

This has been released with 7.x-1.2. Please test!

Seems to work as expected. Thanks!