Scenario:
After a user's password has expired, after 90 days for example, their account is timestamped blocked in the expiration table. If blocking accounts is selected, the next cron will block that user's access.

Then a site administrator tries to unblock a blocked user by going to the default user node edit form selecting "Active" and clicking save.

This process will unblock the user (user status = 1) until the next cron run.

Issue:
The next Password Policy cron will see the blocked timestamp and block the user (user status = 0).

The hook_user_update in Password Policy does not recognize the user status active/blocked being changed. The $edit['status'] and $account->status will have the same value because the user node will already have been saved at this time. The _password_policy_unblock() will not be called. And if it was called, the user node would be saved again because there is a user_save() in this function. This would create another user save cycle.

My suggestion:
I created a patch that contains another check in the hook_user_update() that compares $account->status and $account->original->status, and timestamps unblocked if applicable.

This allows a site administrator to unblock a user for the 24h allowance to get a password reset.

My situation has multiple thousands of users and multiple crons per hour. The unblock feature of expired accounts is a table without search capabilities and can be cumbersome to use with thousands of users.

Version: 7.x-1.11

CommentFileSizeAuthor
#18 password_policy-7.x-1.x-attempted_reblock_fix-2562449-18.patch6.33 KBaohrvetpv
#16 password_policy-7.x-1.x-attempted_reblock_fix-2562449-16.patch6.6 KBaohrvetpv
#15 password_policy-7.x-1.x-attempted_reblock_fix-2562449-15.patch4.79 KBaohrvetpv
#11 password_policy-7.x-1.x-attempted_reblock_fix-2562449-11.patch4.76 KBaohrvetpv
#9 password_policy-7.x-1.x-reproduce_bug-2562449-9.patch2.98 KBaohrvetpv
#8 password-policy-user-update-2562449-5.patch1.91 KBmikebarkas
#5 password-policy-user-update-2562449-4.patch2.08 KBmikebarkas
#3 password-policy-user-update-2562449-3.patch2.1 KBmikebarkas

Comments

mikebarkas created an issue. See original summary.

mikebarkas’s picture

mikebarkas commented
Title: Saving the user node edit form does not unblock user » Saving the user node edit form may not unblock user
mikebarkas’s picture

mikebarkas commented
StatusFileSize
new password-policy-user-update-2562449-3.patch2.1 KB

Attaching patch.

Status: Needs review » Needs work

The last submitted patch, 3: password-policy-user-update-2562449-3.patch, failed testing.

mikebarkas’s picture

mikebarkas commented
StatusFileSize
new password-policy-user-update-2562449-4.patch2.08 KB

Changed directory location for patch.

mikebarkas’s picture

mikebarkas commented
Status: Needs work » Needs review

Setting this to needs review.

Status: Needs review » Needs work

The last submitted patch, 5: password-policy-user-update-2562449-4.patch, failed testing.

mikebarkas’s picture

mikebarkas commented
Status: Needs work » Needs review
StatusFileSize
new password-policy-user-update-2562449-5.patch1.91 KB

Fix directory location of patch file.

Setting to needs review.

aohrvetpv’s picture

aohrvetpv commented
Version: 7.x-1.11 » 7.x-1.x-dev
Category: Feature request » Bug report
StatusFileSize
new password_policy-7.x-1.x-reproduce_bug-2562449-9.patch2.98 KB

Thank you for the report and patch. Reviewing this I think there may be some code in the module that could be eliminated/changed to simplify the fix. The expiration code is a problem spot in this module and I have been working locally to refactor it.

First, here is a test method to reproduce the problem as you described it. Testbot failure expected. I think your patch will cause the tests to pass but I haven't yet tried.

Setting to bug report as this seems to me a bug. Accounts aren't supposed to be reblocked until the 24-hour window has elapsed.

Status: Needs review » Needs work

The last submitted patch, 9: password_policy-7.x-1.x-reproduce_bug-2562449-9.patch, failed testing.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-1.x-attempted_reblock_fix-2562449-11.patch4.76 KB

Merged patch in #8 with test method that reproduces problem in #9.

Status: Needs review » Needs work

The last submitted patch, 11: password_policy-7.x-1.x-attempted_reblock_fix-2562449-11.patch, failed testing.

aohrvetpv’s picture

aohrvetpv commented

Patch in #8 does not seem to fix the problem.

aohrvetpv’s picture

aohrvetpv commented

My mistake, I did not merge the patch in #8 properly.

The patch in #8 was against 7.x-1.11. In 7.x-1.x-dev, _password_policy_get_request_time() and _password_policy_get_current_time() are now used in place of REQUEST_TIME and time(). So I just needed to change time() to _password_policy_get_request_time().

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-1.x-attempted_reblock_fix-2562449-15.patch4.79 KB

Again merged patch in #8 with test method that reproduces problem in #9.

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-1.x-attempted_reblock_fix-2562449-16.patch6.6 KB

This patch eliminates the code duplication in #8 between _password_policy_unblock() and _password_policy_unblock_from_user_edit_form(). The former is removed and the latter is renamed to _password_policy_handle_unblock(). (It does not actually unblock the user; it just responds to the unblocking.)

Unblocking (i.e., setting status to 1) is only directly done by this module when the Password Policy Expired Accounts UI is used.

This code has also been removed:

   if (isset($edit['status']) && $edit['status'] != $account->status && $edit['status'] == 1) {
     _password_policy_unblock($account);
   }

I do not see the circumstances in which that condition would evaluate true. I am suspecting it is a useless relic from the D6 version of this module. Please correct me if it does have a purpose.

With this patch, a DB query is made redundantly when a user is unblocked via the Expired Accounts UI. I think since users are unblocked individually via that UI, it is not a performance issue.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Needs work

A to-do: Remove code duplication between testExpiration() and testUnblockingExpiredAccountViaUserEditForm() methods.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-1.x-attempted_reblock_fix-2562449-18.patch6.33 KB

Simplified test method. (This completes the to-do in #17, I think).

aohrvetpv’s picture

aohrvetpv commented

Note that this probably does not fix all problems with unblocking users blocked due to expiration:
#2468449: Cannot unblock reblocked expired account

AohRveTPV queued 18: password_policy-7.x-1.x-attempted_reblock_fix-2562449-18.patch for re-testing.

mikebarkas’s picture

mikebarkas commented
Status: Needs review » Reviewed & tested by the community

@AohRveTPV thanks for you help with this issue.

  • AohRveTPV committed 7f18d45 on 7.x-1.x 
    Issue #2562449 by AohRveTPV, mikebarkas: Saving the user node edit form...
aohrvetpv’s picture

aohrvetpv commented
Status: Reviewed & tested by the community » Fixed
aohrvetpv’s picture

aohrvetpv commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)
aohrvetpv’s picture

aohrvetpv commented
Status: Patch (to be ported) » Closed (won't fix)

6.x-1.x is no longer supported.