Password Policy History module wrong behaviour

In support of gulab.bisht:

Websites will not uncommonly have a restriction like "Your password must be different than your last 10 passwords."

The "History" constraint in 7.x-1.x and the "Past Passwords" constraint in 7.x-2.x make this restriction possible, by setting a value of 10 for the constraint.

The idea is the constraint coupled with expiration forces the user to regularly change their password. That, for one, provides some defense against compromise due to re-use of a password compromised from another system.

It looks like in 8.x-3.x the behavior is different. I think the 7.x form of the constraint is useful to have, and some organizational policies require it.

Great work on the module! I noticed that the history constraint worked slightly differently to how it did in D7. I created a new constraint that allows the password recycle functionality that I think is defined here (while keeping the history constraint as well).

The Password Recycle constraint works by allowing you to configure how many passwords must be used before you can recycle a password. For example:

If I configure the constraint to 5.

• Password_1 (ok)
• Password_1 (error)
• Password_2 (ok)
• Password_3 (ok)
• Password_1 (error)
• Password_4 (ok)
• Password_5 (ok)
• Password_1 (ok)

Forgive me if this is not the right issue for this constraint.

steveworley, I haven't reviewed your patch but in your example, should the last line actually be:
Password_1 (ok)

Five passwords have been used (Password_1, Password_2, Password_3, Password_4, Password_5), so you can re-use Password_1, right?

You're absolutely right! Sorry a bit of copy paste anxiety there.

Tested this patch using the dev version of the module and seems to be working fine.

Changing to Needs Review to get maintainer feedback

I think we should replace the existing D8 Password History constraint, which does not seem useful (or at least is unconventional), with one that has the same functionality as in the D7 branches. As I wrote in #3, the 7.x-1.x Password History constraint provides a password constraint that other, non-Drupal password policies commonly use.

It seems like the Password Recycle constraint in #4 is the same as the Password History contraint in the 7.x-1.x branch. If so, would it be fine to rename it "Password History" for consistency and replace the current Password History constraint? Or is there a reason to prefer the "Password Recycle" name?

I just enabled and tested this module on our soon to be launched Drupal 8 migration of a Drupal 7 site that used this module, and noticed this discrepancy. I'm writing to give a ++ to replacing the current functionality with the one described here. It would be ok to have both, but the current version is pretty unusual, so it almost seems like a good candidate for an add on module.

I agree the functionality to restrict use of last N passwords should be available. Currently it is not.
i tried to use the Password Policy History, but that does not provide the needed functionality.
This issue was created almost 2 years ago what the current thinking, have the maintainers commented on this?

rocket777, ultrabob, thanks for your thoughts on this. I'm also in favor of changing the history constraint to work as in the Drupal 7 versions of Password Policy, and not including the current history submodule's functionality by default in this module, since it seems like an uncommon password constraint.

The Password Recycle module for which steveworley gave a patch in #4 looks like it provides the desired functionality, if you want to try it. It could be renamed Password History and tested/reviewed. Contributions welcome, I'm uncertain when I will have time to work on this issue.

After applying this patch to 8.x-3.0-beta1, when editing any user profile page the follow error is thrown and the page is inaccessible.

php-error ded-15106 [20-Apr-2020 14:34:27 America/New_York] Error: Cannot use object of type Drupal\user\Entity\User as array in REDACTED/docroot/modules/contrib/password_policy/password_policy_recycle/src/Plugin/PasswordConstraint/PasswordRecycle.php on line 167 #0 /mnt/www/html/hrnih/docroot/modules/contrib/password_policy/src/PasswordPolicyValidator.php(123): Drupal\password_policy_recycle\Plugin\PasswordConstraint\PasswordRecycle->validate('', Object(Drupal\user\Entity\User))

I found some time to take a look tonight, and validated the report in #14. I'm attaching a patch to resolve this issue. I also tested the module briefly, and unless I'm mistaken, it may currently be suffering an off by one error. I need to check more carefully to be sure, but I set the number of passwords required between reuses to 2.

started with Original Password (OP) before installing constraints
Tried to change password to OP again, got an error from password recycle
Changed Password to P2 was successful
Tried to change password to OP. Expected an error, but was successful.

Finally, I took a look at what it will take to replace the password_policy_history constraint with the password_policy_recycle constraint. The recycle constraint currently depends on the history constraint, and uses the database schema that was established by the history constraint. This is find, and means that we basically need to merge the two under the history name. I didn't have time to work on that part tonight, and I'm not sure when I will have, but I think with this patch, the functionality should at least be testable again.

Briefly looking at this while waiting for a process to finish, it appears that my "off by one error" is because each password hash is being written to the database twice.

password_policy_history_user_update()

is being fired twice, and I don't know why.

The way

_password_policy_history_insert_password_hash()

operates lines up with 7.x-1.x of the module, but seems quite simple and is causing problems for me because hook_user_update is running twice and creating duplicate entries. (Aside: One other point of investigation: the 7.x-1.x version o fthe constraint used hook_user_presave instead of hook_user_update, is this significant?) It just naively writes a new hash value and timestamp to the database whenever it is called. That means that if something causes the hook_user_update hook to be called more than once in rapid succession, there is no debouncing happening. For the purposes of the password history (and recycle module) we could either adjust this behavior or the validation side behaviour.

Input side:
When a new insert_password_hash is called it checks to see if the id/has pair already exists in the database, and if it does, replace the timestamp with the current one. That would mean only one of each id/password hash combo would exist in the table, and it would always be the most recent one. This is the perfect behavior for the history constraint, it doesn't add extra rows to the database that will never be needed. If there is some case where knowing that we've reused the same password in the past is important, this behavior will clear that information out.

Validation side:
We could query for the last $password_reuse_gap_count (don't remember the real setting name) unique id/password hash combos and check to see if the new password matches that list. This would have the pro that we maintain a complete history of all passwords stored, but the cons that if the hook runs twice in quick succession we'll have a duplicate entry in the database, it just won't affect the history validation, and the table will contain significantly more rows.

I lean toward fixing this on the input side, but welcome feedback.

I've made a change to debounce the password history hash write to the table (some of my assumptions in #17 were wrong, but this part is working) I was trying to get the unit tests attached to this constraint to work, but they seem to have been copied from somewhere else, and would not run at all. I've got them running now, but all the tests fail, and I have no experience with writing these tests. I can upload a patch of what I've got so far, hopefully sometime tomorrow, but would anyone more experienced with testing be able to spare a bit of time to help me get them figured out?

I reproduced #14 after installing 8.x-3 beta 2.

The latest dev build from July 20th,2020 8.x-3.x-dev is producing the error:

The website encountered an unexpected error. Please try again later.
Error: Cannot use object of type Drupal\user\Entity\User as array in Drupal\password_policy_recycle\Plugin\PasswordConstraint\PasswordRecycle->validate() (line 167 of modules/contrib/password_policy/password_policy_recycle/src/Plugin/PasswordConstraint/PasswordRecycle.php).
Drupal\password_policy_recycle\Plugin\PasswordConstraint\PasswordRecycle->validate('', Object) (Line: 123)
Drupal\password_policy\PasswordPolicyValidator->buildPasswordPolicyConstraintsTableRows('', Object, Array) (Line: 57)
password_policy_form_user_form_alter(Array, Object, 'user_form') (Line: 539)
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'user_form') (Line: 838)
Drupal\Core\Form\FormBuilder->prepareForm('user_form', Array, Object) (Line: 279)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 91)
Drupal\Core\Controller\FormController->getContentResult(Object, Object) (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Tagging this as novice for DrupalCon Global contribs day

Using the new patch from #15 I made a couple of changes.

The signature for PasswordConstraintInterface::validate() changed at some point so updated PasswordRecycle::validate() to match and I don't think

if (!is_a($user_context, 'Drupal\user\Entity\User')) {
  return $validation;
}

is needed since the User object should always have an id and the addition of the UserInterface type hint.
validate($password, UserInterface $user)

Any thoughts or concerns?

Minor change in #21 patch.
Added support for D9.

I seemed to fail the password recycle check no matter how many new passwords I used before reusing. If I set the limit to 5, and started with password:

password05

I changed to:
password06
password07
password08
password09
password10
password11

then tried changing back to password05. I still got the error about violating password recycle.

Also it may be a peculiarity of my install, but I get two password history entries in the database for each password change made from the logged in user, and once when changing the password from the admin account.

Working on this issue. Assigning to myself.

Hi ultrabob, As per my analyse, password policy history saves two for authenticated changes the password. Becase user is called in custom submit handler in password_policy.module. We already have issue. Please look at Here. I have tested the functionality with the patch I posted.

Thanks

Added the default value to configuration form because I have compared other policy configuration form, it has default value set. Please look at the inter diff. We can test this patch once the https://www.drupal.org/project/password_policy/issues/3095980 is fixed. Please review the patch. Thanks

Testing steps with password_policy_recycle installed and enabled, set to 2. No other password policy criteria set..
1. Use reset password link to set password to 10
2. observe one new entry in the password_policy_history table for this change (due to patch on issue #3095980
3. try to change password to 10 again. Observe fail warning after ajax runs and actual fail on submit
4. change password to 11
5. try to change password to 10. observe fail warning and actual fail.
6. change password to 12
7. change password to 10.

Up to this point everything works as expected. I'm so happy to see this moving forward, thank you! I have found one problem (which may be a different issue, not specific to recycle), and one more step I think we need to accomplish before this can be committed.

The issue:
If you submit the password change form while the password policy ajax is running, the password reset fails everytime with error:

The additional Step:

I agree with @AohRveTPV that it would be better to replace the history module with this recycle one. Since currently the recycle module depends on the history mosule, it seems like it could potentially be complicated to roll out a version of the module with both history and recycle, and then remove history. Even if both modules stay in place, it will confuse the interface if history has to be installed for recycle to be used. I think we should move the recycle code into the history module instead of making a new module. Optionally we could add a new module with the behavior currently in history, but the use case where current history is useful must be extremely rare.

Hi ultrabob, Thanks for making the review. I am agree with your suggestion on replacing the history module with recycle module.

Also I am unable to reproduce the ajax issue while changing the password. Please provide video if possible. So I can reproduce the issue and fix.

Thanks

Changing the status to Review for reviewing of module maintainer.

Patch #28 works for me.
Cheers

#28 worked well

Confirming RTBC status

Also agree with @ultrabob that we should replace the PasswordHistory module.

I run in to the Error: Call to a member function select() on null in Drupal\password_policy_history\Plugin\PasswordConstraint\PasswordHistory->getHashes() (line 95 of /app/modules/contrib/password_policy/
when I tried to open the user page after configuring a password policy history.

Now it is working, thank you!

just please in password_policy_history/src/Plugin/PasswordConstraint/PasswordHistory.php add the parameter $connection on the doblock.

Added the parameter $connection to the docblock as pointed in #44

Kindly review it :)

I'll try to review it!

I didn't understand if the issue's version is 8.x-3.0-alpha3 or the version that has been worked on the comments of this issue.

Hello, i'll try review it.

I reviewed, and all changes that was asked, was implemented and work correctly.
So i'll change this issue to RTBC.

Thanks to everyone who has contributed to this!

The only thing I would add to this patch is to reword the configuration page and error message. "Number of allowed repeated passwords" reflects the pre-patch behavior of counting repeats and it should be something that indicates none of the last n passwords can be used.

Letting the user know how many are forbidden would be nice, too. Something like "The last @count passwords cannot be reused."

We're using Password Policy along with PRLP. I'm not sure if it is a side-effect of installing both of them or if there is still an issue since #17 above, but we get duplicate password history entries. We've added an internal patch that checks if there is already a record with the current UID and request time, and skips inserting a new record if that's the case.

Updated the labels and and fixed a bug when the history configuration is set to '0' so that all passwords are checked.

Regarding #52, check if the duplicated entries bug was fixed in #3095980: Password history policy inserts password twice in password_policy_history table @John Franklin

Patch no longer applies

Wrong status, sorry

Strangest thing.
If I use:
https://git.drupalcode.org/project/password_policy/-/merge_requests/28.p...
I get all sorts of errors

error: patch failed: password_policy_history/tests/src/FunctionalJavascript/PasswordHistoryTest.php:1
error: password_policy_history/tests/src/FunctionalJavascript/PasswordHistoryTest.php: patch does not apply
error: password_policy_recycle/src/Tests/PasswordRecycleTests.php: No such file or directory

however, using:
https://git.drupalcode.org/project/password_policy/-/merge_requests/28.diff
The patch applies without issue.

Tested with the following steps and results:
Create a password policy with 1 constraint - password_policy_history_constraint
- Number of passwords that will be checked in the user password update history: 5
Create a test user account set an initial password
Edit the account and set a new password to Password_001 and click save
Edit the account and set a new password to Password_001 which fails then change it to Password_002 and click save
Edit the account and set a new password to Password_001 which fails then change it to Password_003 and click save
Edit the account and set a new password to Password_001 which fails then change it to Password_004 and click save
Edit the account and set a new password to Password_001 which fails then change it to Password_005 and click save
Edit the account and set a new password to Password_001 which fails then change it to Password_006 and click save
Edit the account and set a new password to Password_001 which is allowed and click save

Hi!

I applied MR !28 against 9.4.x-dev and works as it should, confirming RTBC.

Steps:
- set the number of allowed repeated passwords to 5;
- created a new user with password "pass1";
- changed new user's password to "pass2"
- tried to change new user's password to "pass1" once more: blocked with "The last 5 passwords cannot be reused" error;
- changed new user's password to "pass3";
- tried to change new user's password to "pass1" and "pass2": blocked;
- changed new user's password to "pass4"
- tried to change new user's password to "pass1", "pass2" and "pass3": blocked;
- changed new user's password to "pass5";
- tried to change new user's password to "pass1", "pass2", "pass3" and "pass4": blocked;
- changed new user's password to "pass6";
- now I was able to change new user's password to "pass1" again.

Updated #28 to support Drupal 10.
Only .info had to be updated.

#64 is not applying.

Updated patch for #64

If there are still issues, please open a new issue rather than update one that is already marked closed/fixed. You can link to this one. Thanks.