"Delay" constraint allow to change system-generated password

+++ b/plugins/constraint/delay.inc
@@ -41,6 +41,15 @@ function password_policy_delay_constraint($password, $account, $constraint) {
+  $pass_reset = isset($_SESSION['pass_reset_' . $account->uid]) && isset($_GET['pass-reset-token']) && ($_GET['pass-reset-token'] == $_SESSION['pass_reset_' . $account->uid]);

I would recommend get rid of variable and simplify construction to:

if (
  isset($_GET['pass-reset-token'], $_SESSION['pass_reset_' . $account->uid]) &&
  $_GET['pass-reset-token'] === $_SESSION['pass_reset_' . $account->uid]
) {
  // Don't apply constraint on password reset.
  return TRUE;
}

@l0ke, don't you want put every condition inside of if statement to own line? This form allow to nicely comment everyone of them.

Example:

if (
  // Allow to continue if $string variable is equal to "string".
  'string' === $variable ||
  // Handle a case, when value of variable is number and equal to "10000".
  10000 === $variable
) {
  // Code.
}

It's just my proposition - changes are RTBC-ready.

A quick look at the coding style from this module shows that it follows the Drupal coding standards. The conditions should not be wrapped into multiple lines.

FILE: ...ar/www/sites/lush_os/password_policy/includes/PasswordPolicy.inc
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
   1 | ERROR | [x] The PHP open tag must be followed by exactly one
     |       |     blank line
  73 | ERROR | [x] Equals sign not aligned with surrounding
     |       |     assignments; expected 8 spaces but found 1 space
  74 | ERROR | [x] Equals sign not aligned with surrounding
     |       |     assignments; expected 1 space but found 2 spaces
 137 | ERROR | [x] Inline comments must end in full-stops,
     |       |     exclamation marks, colons, question marks, or
     |       |     closing parentheses
 156 | ERROR | [x] Inline comments must end in full-stops,
     |       |     exclamation marks, colons, question marks, or
     |       |     closing parentheses

FILE: ...tes/lush_os/password_policy/includes/PasswordPolicyCondition.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...es/lush_os/password_policy/includes/PasswordPolicyConstraint.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...ww/sites/lush_os/password_policy/includes/PasswordPolicyItem.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: .../shared/var/www/sites/lush_os/password_policy/password_policy.js
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  44 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
 200 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
 211 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found

FILE: .../www/sites/lush_os/password_policy/plugins/condition/authmap.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...var/www/sites/lush_os/password_policy/plugins/condition/role.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: .../sites/lush_os/password_policy/plugins/constraint/alpha_case.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...sites/lush_os/password_policy/plugins/constraint/alpha_count.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...w/sites/lush_os/password_policy/plugins/constraint/blacklist.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: .../sites/lush_os/password_policy/plugins/constraint/char_count.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...sites/lush_os/password_policy/plugins/constraint/consecutive.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...r/www/sites/lush_os/password_policy/plugins/constraint/delay.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...s/lush_os/password_policy/plugins/constraint/drupal_strength.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...w/sites/lush_os/password_policy/plugins/constraint/int_count.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...es/lush_os/password_policy/plugins/constraint/past_passwords.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...ites/lush_os/password_policy/plugins/constraint/symbol_count.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...ww/sites/lush_os/password_policy/plugins/constraint/username.inc
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one
   |       |     blank line

FILE: ...es/lush_os/password_policy/plugins/export_ui/password_policy.inc
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
  1 | ERROR | [x] The PHP open tag must be followed by exactly one
    |       |     blank line
 12 | ERROR | [x] Inline comments must end in full-stops, exclamation
    |       |     marks, colons, question marks, or closing
    |       |     parentheses

FILE: ...ed/var/www/sites/lush_os/password_policy/plugins/item/expire.inc
----------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
   1 | ERROR   | [x] The PHP open tag must be followed by exactly one
     |         |     blank line
 167 | WARNING | [ ] Line exceeds 80 characters; contains 155
     |         |     characters
 167 | ERROR   | [x] Inline comments must end in full-stops,
     |         |     exclamation marks, colons, question marks, or
     |         |     closing parentheses

@badjava, do you see any errors related to changes (phpcs has scanned the code with applied patch)? I am not.

Drupal CS says that conditions "should" not be multiline, not "must". No need to set the status to NW if you aren't love some piece of code only from coding standards perspective (especially in case when module not follows them). The main thing - possibilities provided by patch.

I think the patch has a problem.

My understanding is the purpose of the delay constraint is to prevent the user from changing their password multiple times to bypass the past password constraint. For instance, suppose a policy that disallowed using the past 3 passwords. Suppose a user's password is "foo" and they are forced to change their password.
- User immediately changes password to "foo1".
- User immediately changes password to "foo2".
- User immediately changes password to "foo3".
- User immediately changes password to "foo".

The user has bypassed the past password constraint. The delay constraint, however, prevents doing that. They will have to wait between each password reset.

The patch as written would seem to give the user a way to bypass the delay constraint, and thereby bypass the past password constraint:
- User requests a one-time login link.
- User immediately logs in using one-time login link and changes password to "foo1".
- User requests a one-time login link.
- User immediately logs in using one-time login link and changes password to "foo2".
- User requests a one-time login link.
- User immediately logs in using one-time login link and changes password to "foo3".
- User requests a one-time login link.
- User immediately logs in using one-time login link and changes password to "foo".

It seems the patch would need to make an exception to the delay constraint only for certain cases:
- Drupal has generated a password and the user is setting it for the first time.
- An administrator has forced the user to change their password.
- etc.

Just want to comment that there is probably some better way to solve this than adding exceptions as I described in the last paragraph of #10. Main point of #10 is the patch provides a way to bypass the delay constraint. Thanks for the bug report and patches.

I do like #17!

1. +++ b/password_policy.test
   @@ -22,6 +22,30 @@ class PasswordPolicyBaseTestCase extends DrupalWebTestCase {
   +    if ($account == NULL) {
   

   Use strict comparison. Also, you can switch the operands.

2. +++ b/password_policy.test
   @@ -22,6 +22,30 @@ class PasswordPolicyBaseTestCase extends DrupalWebTestCase {
   +    $edit = array('pass' => user_password());
   

   Pass an array to user_save() and get rid of $edit variable.

3. +++ b/password_policy.test
   @@ -22,6 +22,30 @@ class PasswordPolicyBaseTestCase extends DrupalWebTestCase {
   +
   

   Remove this empty line.

4. +++ b/password_policy.test
   @@ -22,6 +22,30 @@ class PasswordPolicyBaseTestCase extends DrupalWebTestCase {
   +    return $account;
   

   Leave blank line before return statement.

5. +++ b/password_policy.test
   @@ -228,6 +252,11 @@ class PasswordPolicyConstraintsTestCase extends PasswordPolicyBaseTestCase {
   +    $this->assertFalse($this->checkPolicy($policy, 'password', $this->account), t('Delay constraint fails with new password before delay window expires and threshold is reached.'), t('Constraint'));
   

   Not need to use t() for test messages. But I'm seeing that its used everywhere in this module.

6. +++ b/plugins/constraint/delay.inc
   @@ -12,6 +12,7 @@ $plugin = array(
   +    'threshold' => NULL,
   

   Can we set to 1?

7. +++ b/plugins/constraint/delay.inc
   @@ -29,6 +30,13 @@ function password_policy_delay_admin_form($form, &$form_state, $constraint) {
   +    '#title' => t('Maximum number of times password can be changed'),
   

   I'd suggest: "Maximum number of attempts to change the password per time window".

8. +++ b/plugins/constraint/delay.inc
   @@ -29,6 +30,13 @@ function password_policy_delay_admin_form($form, &$form_state, $constraint) {
   +    '#description' => t('Password can be changed this many times in a period above (if not defined 1 time).'),
   

   I'd suggest: "Configure an amount of attempts for changing the password per given time frame."

9. +++ b/plugins/constraint/delay.inc
   @@ -52,6 +60,24 @@ function password_policy_delay_constraint($password, $account, $constraint) {
   +  $period_start = strtotime('-' . $constraint->config['delay']);
   

   We need $period_start variable only once and only if some conditions were passed. I'd suggest to remove the variable and use strtotime('-' . $constraint->config['delay']) directly at place.

Not need to use t() for test messages. But I'm seeing that its used everywhere in this module.

I created an issue for this:
#2863756: Unnecessary t() calls in tests

Looks nice to me. Let's wait the tests to pass and tomorrow I'll be able to "touch" the patch locally.

Reduced #22 to the new threshold functionality. I think it will be easier (for me at least) to focus on just this change first. I removed a few style changes that seem incidental--they can be committed separately.

Removed an inadvertent change from #24.

We can make the other changes too, separately. I just want to focus on the threshold feature first because it is complicated in itself. It is harder to understand patches/commits if they have several purposes.

This updated patch changes more text/messages for the threshold change.

Also removed the comment "// Pass query parameters for correct handling of password reset." This refers specifically to the addition of window.location.search, but I do not think that would be clear to someone reading the code.

It is strange to me that element_validate_integer_positive() allows a blank entry.

Last patch didn't have all my changes. Please disregard.

+++ b/password_policy.js
@@ -95,8 +95,12 @@
+        var threshold = $('input[name="threshold"]', context).val();
+        if (!threshold) {
+          threshold = 1;
+        }

Why not var threshold = $('input[name="threshold"]', context).val() || 1?

Why not var threshold = $('input[name="threshold"]', context).val() || 1?

Because I barely know JS. :) That looks better.

Any reason we can't make the threshold field required? Then we could do away with the parenthetical and the extra logic for handling empty input.

I notice none of the other constraint fields are required, but if the default is set to 1, I don't see why it would be a problem.

New patch that makes threshold required.

Also changed a description from one sentence to two. (The parenthetical was really a separate sentence.)

Thanks. I will try to review #33 in the coming days.

#33 is imperfect because there is a case where $is_password_generated could be TRUE when the password was not system-generated:
1. User sets password.
2. Administrator enables email verification.
3. User logs in via password reset link and can change password in violation of delay constraint. (The password set in #1 is treated as system-generated even though it is not.)

However, I cannot think of a better solution. One idea I had was to not save the password to the password history if email verification is enabled. But this would allow a user to change their password back to the system-generated password in violation of the past passwords constraint.

A downside of this change is it allows a user to change the password back to the system-generated password faster than an administrator might expect from their delay constraint and past passwords constraint settings. For instance, if the delay constraint is set to 1 day and the past passwords constraint is set to 2, an administrator might expect a user cannot re-use a password in less than 2 days. But the user can actually re-use the system-generated password after 1 day.

Updated patch to apply to latest code. Fixed a grammatical error in comment and rewrapped it.

Committed. Let's see how it goes. I'd like to do a new alpha release soon.

Even though #35 is imperfect, it does solve a major problem with using the delay constraint.

Reverted and recommitted to credit BR0kEN also.

I like the solution in #40. I'll more closely review the patch in the coming days.

1. +++ b/plugins/constraint/delay.inc
   @@ -71,7 +71,7 @@ function password_policy_delay_constraint($password, $account, $constraint) {
   +  $is_password_generated = count($account->password_history) == 1 && $account->password_history[0]->is_generated;
   

   isset() seems more logical here except we do need to know that exact number is 1, don't we?

2. +++ b/plugins/item/expire.inc
   @@ -323,7 +324,7 @@ class PasswordPolicyExpire extends PasswordPolicyItem {
   +        $account = user_load($candidate->uid);
   

   Why reset is no longer needed? Maybe it's a good idea to describe this in comments to code?

Anyway, I'm proposing to add a comment like "we need to load the user skipping the cache to reflect saved password".

+++ b/plugins/item/expire.inc
@@ -324,7 +324,7 @@ class PasswordPolicyExpire extends PasswordPolicyItem {
       $candidates = _password_policy_expire_query_users($notice_int, $policy_name);
       foreach ($candidates as $candidate) {
-        $account = user_load($candidate->uid);
+        $account = user_load($candidate->uid, TRUE);

Also, I think we can avoid loading of users without the cache on every candidate's iteration. Here is the PoC, but we need the other issue created for this.

Re the suggestion in #47 to instead use user_load_multiple(): For sites with very many users (e.g. 1000s), loading all the expired accounts into memory can cause the PHP memory limit to be exceeded. It is a longstanding bug in 7.x-1.x, 7.x-2.x, and 8.x-3.x that the expiration code does not scale well: #2252541: Expiration cron does not scale for large numbers of users. So I'm not sure we can use user_load_multiple() for all expired users.

Recently I committed a change that passes $reset = TRUE to user_load() in the loop. It is intended to prevent the user cache from growing so large it exceeds the PHP memory limit.

    // If there is no password history, start one.
    if (!isset($account->password_history[0])) {
      $account->password_history[0] = (object) array(
        'uid' => $account->uid,
        'pass' => $account->pass,
        'created' => REQUEST_TIME,
        'is_generated' => FALSE,
      );
      password_policy_update_password_history($account->password_history[0]);
    }

I'm not sure I understand this code. What are the circumstances where there would be no password history?

My guess is there would be no password history for a user whose password was set before the Password Policy module is enabled. In that case, we do not know whether the password was system-generated or not, so setting it to FALSE seems inaccurate. It's also inaccurate to record the password as created at the time of the request. (I realize that is not your code.)

I am wondering if it would be better to remove this entire code block, perhaps as a separate issue. If a user has no password history, they would have no password history until they set a new password, and the module would have to handle it accordingly.

Please let me know what you think.

Minor grammatical improvement to comment.

It's possessive so it needs to be either "New users' passwords" or "New user's password".

I plan to commit #50 soon.

    // If there is no password history, start one.
    if (!isset($account->password_history[0])) {
      $account->password_history[0] = (object) array(
        'uid' => $account->uid,
        'pass' => $account->pass,
        'created' => REQUEST_TIME,
        'is_generated' => FALSE,
      );
      password_policy_update_password_history($account->password_history[0]);
    }

This code is necessary because otherwise passwords set before Password Policy is installed will never expire. (Expiration is determined by subtracting the current time from the password history's most recent password change time.)

This code is flawed regardless of the patch in #50. One problem: If Password Policy is installed and the Delay constraint is enabled, users will be unable to change their password until the delay has passed since Password Policy was enabled--even if the last time they changed their password was years ago.

The issue l0ke opened can be used for fixing this code. In 7.x-1.x, rather than initializing a password history entry for all users, passwords are expired when the expiration period has elapsed since the policy was enabled. 7.x-2.x should probably work in a similar way.

Setting is_generated to FALSE seems good enough to me, because setting it to TRUE would allow the user to bypass the Delay constraint. This whole code block needs to be removed, anyway.

Thanks for the contributions.

Ideally this would probably have a test, but there is probably lots of functionality needing tests in this code.