History constraint assumes hash method

Good find! I updated your patch with some adjacent coding standards updates.

http://drupalcode.org/project/password_policy.git/commit/9863f13

The committed code here doesn't make any sense.

/**
 * Password validation.
 */
function password_policy_constraint_history_validate($password, $constraint, $account) {
  require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc');
  $old_passwords = _password_policy_constraint_history_old_passwords($constraint, $account->uid);
  foreach ($old_passwords as $pw) {
    if (user_check_password($password, $account)) {
      return FALSE;
    }
  }

  return TRUE;
}

We loop over the old password hashes and then do absolutely nothing with them. Therefore it only checks the most recent password. But LOTS of times.

I think the best we can do here without assuming a hash method is to use sha512 by default and have a drupal_alter so that people can hook in with their own hashing scheme if necessary.

Thanks,
Pete

You're right, but I think it's just a matter of misusing a variable.

/**
* Password validation.
*/
function password_policy_constraint_history_validate($password, $constraint, $account) {
  require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc');
  $old_passwords = _password_policy_constraint_history_old_passwords($constraint, $account->uid);
  foreach ($old_passwords as $pw) {
    // Removing current $password variable and replace with each $old_passwords
    if (user_check_password($pw, $account)) {
      return FALSE;
    }
  }

  return TRUE;
}

No, user_check_password expects a plain text password, which I'm happy to say is *not* what is pulled from the password_policy_history table.

Looking at the original post again, I think tonylegrone hacked core to achieve what he wanted (and break this module). So I'm wondering how much of a problem there was with the original code in the first place?

Perhaps upgrades from earlier drupal versions could cause a problem? Not much else I don't think...

Actually, forget that - I notice that a user can legitimately override password.inc by setting the password_inc variable.

Still leaves me without a decent solution for this though... Using user_hash_password would be better, but you might still run into problems with d6 upgrades etc.

Since we can't re-hash a password, I don't think we need to worry about that case. So you're thinking we can just swap in if (user_hash_password($password) == $hashed_old_password;) { ... } and leave the remainder of the code untouched?

I *did* think that yes. But actually that's no good, because if you hash the same thing twice using user_hash_password(), you get a different answer because of salting.

I genuinely think the best thing we can do here is revert to what was there before.

Having learnt a few things about drupal passwords this afternoon, I *think* the best solution to tonylegrone original problem is for him to override _password_crypt(). He will need to work out whether to ignore the 'sha512' parameter he is given by this module by inspecting the value of $stored_hash.

Anything else would need a core patch I think.

So I am proposing:

function password_policy_constraint_history_validate($password, $constraint, $account) {
  require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc');
  $old_passwords = _password_policy_constraint_history_old_passwords($constraint, $account->uid);
  foreach ($old_passwords as $pw) {
    $hash = _password_crypt('sha512', $password, $pw);
    if($hash == $pw) {
      return false;
    }
  }

  return TRUE;
}

and it is up to the users version of _password_crypt to choose whether or not 'sha512' is correct using a similar strategy to Core's user_check_password().

user_hash_password() shouldn't create multiple versions of the same password, as long as the salt is the same. Otherwise there'd be no way to validate a previously hashed password.

Which is why you can call _password_crypt() with an existing hash (as per the original code this patch removed)

https://api.drupal.org/api/drupal/includes!password.inc/function/_passwo...

But that's not what user_hash_password() does

https://api.drupal.org/api/drupal/includes!password.inc/function/user_ha...

and there's nothing you can do about that as far as I can tell.

Hi,
I think this approach suits for everyone.
We still use check_password_account so the correct algorithm is applied to the new password depending on the old one.

Whatever you decide, it would be great to commit it because now the functionality is broken.
Thank you.

Regards.

I committed #16, but someone really needs to review and RTBC this before making a new release.

This works for me!