By default, if you force the user to change their password, it's possible for them to log in with their old password, be re-directed to the account page and change their password there. The problem with this approach, especially in light of Heartbleed, is that if someone was able to get the user's password, they could then use that to reset the password to something they want and gain access to the account.

The only way to prevent this is to force the user to be logged out and then only allow them to reset their password using the built-in password reset form at user/password.

Patch incoming...

CommentFileSizeAuthor
#27 password_policy-7.x-1.x-revert_force_change_by_mail-2291417-27.patch10.55 KBAohRveTPV
#25 password_policy-7.x-1.x-revert_force_change_by_mail-2291417-25.patch11.07 KBAohRveTPV
#20 password_policy-7.x-1.x-force_change_by_mail-2291417-20.patch10.72 KBAohRveTPV
#18 interdiff.txt2.44 KBAohRveTPV
#17 password_policy-7.x-1.x-force_change_by_mail-2291417-17.patch10.69 KBAohRveTPV
#13 password_policy-7.x-1.x-force_change_by_mail-2291417-13.patch9.08 KBAohRveTPV
#11 password_policy-7.x-1.x-force_change_by_mail-2291417-11.patch8.5 KBAohRveTPV
#10 password_policy-7.x-1.x-force_change_by_mail-2291417-10.patch8.45 KBAohRveTPV
#7 password_policy-7.x-1.x-force_change_by_mail-2291417-7.patch5.2 KBAohRveTPV
#6 password_policy-7.x-1.x-force_change_by_mail-2291417-6.patch5.22 KBAohRveTPV
#3 allow_force_user_logout-2291417-3.patch4.36 KBanemirovsky
#1 allow_force_user_logout-2291417-1.patch2.44 KBanemirovsky
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anemirovsky’s picture

anemirovsky CreditAttribution: anemirovsky commented
Status: Active » Needs review
FileSize
allow_force_user_logout-2291417-1.patch2.44 KB

The attached patch adds an option to force the user to be logged out if they have been forced to reset their password.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

I agree and have had the same concern with the force password change feature. Ordinarily new features should go in 7.x-2.x first, but I think this warrants an exception since it fixes a security deficiency.

The setting implemented in the patch could be a bit confusing to administrators: An administrator might check the "Force password change at next login" checkbox on a user's page, but if the new setting is set, the password change would actually be forced before, not at, the next login. (That the user is technically logged in and immediately logged out is an implementation detail.)

It would add clarity, I think, to change the text of that field depending on how your new setting is set. In the case that your setting is set, it could be:

Label: "Force password change before next login"
Description: "The user will be logged out if currently logged in."

What do you think?

Also, I think a variable_del() is needed in hook_uninstall() implementation.

anemirovsky’s picture

anemirovsky CreditAttribution: anemirovsky commented
FileSize
allow_force_user_logout-2291417-3.patch4.36 KB

The attached patch includes the changes you requested. Let me know if there's anything else that needs tweaking.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs review » Needs work

Thanks, I will plan to try it out soon.

I am wondering if this behavior should become the default for forced password changes.

We should probably add some SimpleTest testing of this new functionality.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

A problem is the combination of this new setting and "Force password change on first-time login". If both are enabled, the user is required to use the password-reset form to change their password before their first allowed login, which is not a useful behavior. It would be effectively redundant with the functionality provided by the Drupal "Require e-mail verification" account setting.

I am updating the patch to disallow the combination of settings.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-6.patch5.22 KB

Sorry for not getting back sooner on this. I want to get this feature in.

Wrote a new patch:

1. Change setting to "Force password change by e-mail" and do not log user out.

To me it seems the purpose of this feature is to force the user to reset their password via e-mail. Verifying ownership of the registered e-mail address is another way of authenticating the user when the password might be compromised.

Logging out the user is a way to accomplish this, but does not seem necessary. We can just force the user to be redirected to the "Request new password" page.

2. Patch in #3 looks to allow an access bypass. If a user is forced to change their password, and they log in directly from the password change page (e.g., user/2/edit), they would not be logged out.

3. Make this the default setting for new installations. Allowing a user whose password is expired, for instance, to change their password using only the expired password to authenticate, is insecure. Existing installations will have this setting off so the behavior does not change unexpectedly after update.

Please review. I would be especially interested to know what is thought about the change to not log the user out. Is there any problem doing it this way?

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-7.patch5.2 KB

Minor simplification versus #6.

The last submitted patch, 6: password_policy-7.x-1.x-force_change_by_mail-2291417-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: password_policy-7.x-1.x-force_change_by_mail-2291417-7.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-10.patch8.45 KB

Changes versus #7:
- Fix syntax error.
- Add test.
- Change setting to default to off. (I am still thinking it should be the default, but it can be made the default later. Probably should allow for some post-release testing of the feature first.)

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-11.patch8.5 KB

Versus #10, tried to clarify the setting description.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs review » Needs work

Setting needs to go under "Settings" tab, not "Force password change" tab. A user with the "Force password change" permission should be able to force password changes, but not change how forced password changes work.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-13.patch9.08 KB

Add "Force password change settings" fieldset on configuration page and put "Force password change by e-mail" setting there, per #12.

anemirovsky’s picture

anemirovsky CreditAttribution: anemirovsky commented
Status: Needs review » Needs work

The code looks good. The only problem I see with not forcing the user to be logged out is that they then have to manually log themselves out in order to be able to reset their password via the link provided in the password reset email. So, it's nice that they are redirected to the reset password by email page, but after clicking "E-mail new password" they then have to follow the direction on that page that says "You must log out to use the password reset link in the e-mail."

My guess (based on answering a fair amount of support requests from clients surrounding resetting passwords) is that a lot of people are not going to read that and are not going to log out. Then, when they click the link in the password reset email, it's going to take them right back to the reset password by email page with a message like "You are logged in as . Change your password [which is a link to the user edit page]." If they click "Change your password", it just takes them right back to the reset password by email page. So, basically, they're in an endless loop until they manually log out. That's why I think it makes more sense to just log them out.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Agreed. I see two possible solutions:

Solution 1:
- Add a submit handler for the button to log them out once they press the button.
- Change the text to remove the sentence about logging out.

Solution 2:
- Log out the user immediately if they are forced to change their password, and redirect them to the user/password page with a message explaining what to do.

Solution 1 seems like it might be simpler for the user because they only have to enter their username once. Do you agree? Solution 2 may be preferable if it is simpler to implement.

Your patch in #3 implemented solution 2, but I did not see any message explaining to the user what to do when I tested. The user would just be logged out and redirected without any explanation. (I might have misapplied the patch though.)

anemirovsky’s picture

anemirovsky CreditAttribution: anemirovsky commented

It's been a while, but I believe it should have added a message like, 'Your password has expired. You have been logged out and must change your password to proceed on the site.'

I like option 1 better, too, so if that works, I think going that route is preferred.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-17.patch10.69 KB

Changed to automatically log user out upon submission.

The default message seems to be removed by logging out so I duplicated it in password_policy_user_pass_submit().

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
interdiff.txt2.44 KB

Diff between #13 and #17.

anemirovsky’s picture

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

I gave the latest patch a test and it looks good to me. Nice work!

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Reviewed & tested by the community » Needs review
FileSize
password_policy-7.x-1.x-force_change_by_mail-2291417-20.patch10.72 KB

Minor formatting change per coding standards.

  • AohRveTPV committed 6aada0f on 7.x-1.x 
    Issue #2291417 by AohRveTPV, anemirovsky: Allow for forcing the user to...
AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks for reviewing/testing. Went ahead and committed so this can hopefully get some testing in use. I think the messages might need some improvement, but that can be done with further commits.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

This feature in 7.x-1.x seems to have a problem: It will start prompting the user to change their password via e-mail during the warning period before expiration. I think it should only be asking the user to change their password via e-mail upon expiration. Will open a new issue for this.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Opened issue per #23: #2543966: Password change should not be forced by e-mail during expiration warning period

This is only a problem if using the expiration constraint with the "Force password change by e-mail" setting enabled.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
FileSize
password_policy-7.x-1.x-revert_force_change_by_mail-2291417-25.patch11.07 KB

Patch to revert this feature.

I would like to make a Password Policy release for some bug fixes, but this feature still has at least one serious bug per #23/#24. Planning to re-add feature after release.

Status: Needs review » Needs work

The last submitted patch, 25: password_policy-7.x-1.x-revert_force_change_by_mail-2291417-25.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-1.x-revert_force_change_by_mail-2291417-27.patch10.55 KB

Undid reversion of a few lines that shouldn't have been reverted in last patch.

  • AohRveTPV committed 995ff93 on 7.x-1.x 
    Issue #2291417 by AohRveTPV: Remove "Force password change by e-mail"...
AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs review » Active
AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Future implementations of this feature should not force the user to change their password by e-mail during the expiration warning period. Password changes should only be forced by e-mail when (a) the password has expired, or (b) a password change has been forced for the user by an administrator.

See #2543966: Password change should not be forced by e-mail during expiration warning period for more details. (Note I am closing it because it is a bug report for a bug that no longer exists, since we reverted #21.)