Password history policy inserts password twice in password_policy_history table

The root cause of this issue is that $user->save() is called in a submit handler that Password Policy adds, _password_policy_user_profile_form_submit. Updating the User entity inside the submit handler causes the save hook to be called twice (and generally seems like bad practice).

To avoid this, it is better to make those updates to the $form_state object in a validation handler (so any other submit hooks will have correct information). This approach is used in core and some contrib modules I checked (e.g. Webform) and the Password Policy module looks those fields to the User form display so I think it makes the most sense here. I am attaching a patch to resolve this issue using that approach.

This patch is a different approach (and does not rely on #2867320: Password Policy History module wrong behaviour) so I am not including an interdiff.

Thanks, taeoey@epam and wells. I have a question about #4: Why is an array of an array needed for setting the value of field_last_password_reset? It works, but I spent some time trying to understand why the array of an array is needed, and couldn't figure it out.

@AohRveTPV - I can't say I know exactly why it has to be that way, but I can say that if it just a single array, saving the form throws an exception and fails (e.g. Error: Cannot use object of type Drupal\Core\Datetime\DrupalDateTime as array in Drupal\Core\Field\WidgetBase->extractFormValues() (line 381 of /var/www/drupalvm/drupal/web/core/lib/Drupal/Core/Field/WidgetBase.php) ).

wells, have you seen some other code that does this? Trying to avoid "programming by coincidence"--if there were some precedence for this code I suppose that'd be good enough.

This has to do with how the value is handled from the widget when #access is FALSE. See TimestampDatetimeWidget::massageFormValues and #2326533: The structure of values received in WidgetInterface::massageFormValues() varies depending on #access TRUE/FALSE.

I have re-rolled the patch. Please review this. Thanks

I reviewed the patches, and see where a call to user->save() had been called in the submit handler, but is now removed in favor of setting up the necessary information during validation, so that it can be handled at a later stage, avoiding a double save.

I fired up my database password_policy_history table, and confirmed that before the patch 2 identical rows were written for each password change. Then I applied the patch, and changed my password again. After the patch, a single row is written after each password change.

One caveat I should mention: the environment I have set up for this kind of testing right now is on 9.1.x, so I tested against that version of core rather than the latest 8. If a further test is needed, here are the testing steps.

1. Load up the site with password_policy_history enabled, bring up the password_policy_history table in the db, and observe what the id of the latest entry is.
2. Change you password, and observe that 2 identical rows (timestamp could vary very slightly) are written for the password change
3. Apply the patch.
4. Change password and check that a single row is added for each change.

This one seems ready to go.

Hi ultrabob, Thanks for making the review on my patch.

I've also just tested this under D8.9.13 and it works for me :-)

A possible issue is that existing entries in the history table remain as they were, so using constraints that might limit according a particular number of recent passwords (e.g. for #2867320: Password Policy History module wrong behaviour), will be inaccurate unless the existing data is also cleaned up. That could be done from a database update hook, but won't necessarily be _perfectly_ accurate. For example, we could iterate and remove every other row that is identical to the row next to it (aside from the id primary key), as the timestamp would normally be identical. But it is technically possible for the timestamp to be different for identical submissions (e.g. in case of some delay in the server between validation+submission), or identical for different passwords (e.g. if the password was changed incredibly quickly)!

This is only going to affect people with existing password history entries that want to limit the checking, so is probably outside of the scope of what needs fixing here. But those people may want to consider how they wish to handle that! (In my case, I'm just going to double the limit that I use with the recycled passwords constraint, so our site is at least as secure as my client's written policy requirements, because being 'more' secure won't be considered a problem.)

IMHO we could create a hook update to remove duplicate rows to delete rows that have same uid, pass_hash and timestamp.
As @james.williams pointed, some of them will not have equal column values but those will be exceptions.

I think it is in the issue scope of this issue.

Except for that, patch #9 looks good.

Rerolling to apply since changes in 8.x-3.x

I created a follow-up issue to implement the hook_update. See: #3266140: Create hook_update to remove duplicated entries in password_policy_history

Patch #15 looks good! Thanks.