Two password history entries created for new users, one invalid

Partial fix: Corrects the doubly hashed history entry, but does not eliminate the duplicate history entries upon user registration.

Using hook_user_presave() as in #1967940: Use hook_user_presave in password_policy to be compatible with more modules is the likely solution for eliminating the duplicate history entries. However, would like to give some time for that approach to be reviewed/tested. This patch is more conservative: It uses the same hooks but fixes the hashing.

Bug doesn't exist in 6.x-1.x.

Backport of #1967940-11: Use hook_user_presave in password_policy to be compatible with more modules should fully fix this.

Fix incorrect function name.

It should be possible with a further patch to clean up the invalid history entries. I believe we can check whether the password that matches the second entry is the hash value from the first entry.

It is not possible to distinguish a hash from an actual user password, but we can check that the first two entries occur within a small amount of time versus each other.

This appears still to be happening on 1.12 (and the patch seems to be in).

I cannot reproduce it on 7.x-1.12. Is it possible the duplicate history entries were created when your site was using a previous version of Password Policy?

Can you give steps to reproduce the problem?

Here are the steps I used to try to reproduce it:

1. Install Drupal.
2. Install/enable Password Policy.
3. Administration->Configuration->People->Account settings:
   • Who can register accounts?: Visitors
   • Require e-mail verification when a visitor creates an account.: unchecked
4. Log out.
5. Create new account.
6. Observe only one entry created in password_policy_history table.

It is not possible to check whether the user's first two hashes are duplicates:

One of the hashes is due to password_policy_password_submit() calling user_hash_password() (via _password_policy_store_password()). This is the valid hash. Let's call it A.

The other password hash is due to both user_save() and password_policy_insert() calling user_hash_password(). So this hash is the password hashed doubly. Let's call these hashes B_1 and B_2. Only B_2 is stored.

Since the password hashes are each uniquely salted, the hash B_1 is not the same as A. So we cannot merely hash A with the salt of B_2 to check whether the hashes match.

We could add code that says: "If the first two hashes for a user were created within a second of each other, they are duplicates, delete the invalid one". But I am not certain this is accurate/safe. On slow systems, the time difference between the duplicate hashes might be several seconds, which makes it hard to distinguish from legitimate password changes. I also worry there are some configurations where legitimate password changes could happen within a second.

Fortunately, the invalid hash is the first one of the two hashes stored. This means it has no implication for the history constraint. If a user is not allowed to use their previous password (i.e., history constraint = 1), but they have two previous passwords because of this bug, they still will be disallowed from using the previous password.

So there seem to be no ill effects from leaving these duplicate hashes, other than database clutter.