Part 1:

When a user requests a one-time login/password reset, we email them a new random password in plaintext. When they use it, we say "please change your password" but do not require them to do so. I suggest that we require them to change it. Here is how I've implemented it so far:

- Whenever a one-time login is used, set a session variable ($_SESSION['user_pw_change']) to TRUE.

- On hook_init, if the session variable is true, display a message and redirect to user/uid/edit.

- If the session variable is set, the password fields on the user_edit form are required.

- When the password is changed, unset the session variable.

The end result is that you can't go anywhere on the site except user/uid/edit until you change your password. Since I am using a session variable, this effect is NOT persistent if you wipe your session cookie; we could of course make it persistent via a column in the users table.

Part 2:

Once we are doing this, I suggest that we stop issuing randomly generated passwords via email at all. We can just send initial-login links that are valid until the user first logs in, then require them to set their initial password immediately.

Comments

bjaspan’s picture

Status:Active» Needs work
StatusFileSize
new2.08 KB

Initial patch attached.

bjaspan’s picture

Status:Needs work» Needs review
StatusFileSize
new7.94 KB

I was somewhat mistaken in my initial post for this issue. I wrote: "When a user requests a one-time login/password reset, we email them a new random password in plaintext." That isn't true. There are two scenarios:

1. When a new account is created, we generate a new random password and send it via email along with a one-time login link.

2. When a password reset is requested, we do NOT change the password or mail them a new one. We mail them a one-time login link which they can use. But if they suddenly remember their password, they can still use it.

What this means is that my original patch is better than I thought. No extra code is required to make required password changes persistent.

1. When a new account is created, we simply remove the initial random password from the default email template (sites can always put it back, of course). Without the initial random password, users have to use the one-time link and then change their password.

2. If they request a password reset, we don't have to make the required password change persistent because we haven't actually changed their old password or generated a new one! They've forgotten it, so they use the one-time login link, then change their password. If they remember it, fine. We haven't disclosed nor changed their old password so there is no risk.

In both cases, if they do not change their password before closing their browser, they can just request another password reset and get another one-time login link.

One more thing. The password reset menu item used to say "E-mail new password" but that hasn't been true for a while. I updated the text to say "Reset password" which isn't strictly true either but is closer to the mark.

New patch attached, ready for review.

dww’s picture

Status:Needs review» Needs work

visually, the patch mostly looks good. i haven't tried testing it, yet. i noticed a few minor coding style issues:

  • drupal_goto('user/'.$user->uid.'/edit'); -- needs spaces around $user->uid.
  • +      '#required' => (isset($_SESSION['user_pw_change']) &&
    +        $_SESSION['user_pw_change']),

    -- core always seems to do such things on the same line...

a somewhat related topic is another pet peave of mine: http://drupal.org/node/110474 -- the drupal_set_message() when you create a new account is lying in some cases, especially if you use admin approval and the user status change notification module...

anyway, i'm certainly in favor of removing the plain-text passwords, and user_status.module already handles some of the same issues you're addressing in this patch. big +1 in spirit... as soon as the minor code style issues are fixed and i have a chance to test/review thoroughly, i'd RTBC in an instant.

thanks,
-derek

bjaspan’s picture

Status:Needs work» Needs review
StatusFileSize
new7.92 KB

Code style issues fixed.

dww’s picture

StatusFileSize
new8.23 KB

- patch didn't apply any more due to http://drupal.org/node/128082 -- now fixed
- re-roll from the root of drupal core, not inside modules/user
still haven't thoroughly tested, but at least it's a clean patch again. ;)

bjaspan’s picture

StatusFileSize
new7.96 KB

Thanks for fixing it up. I notice there is a UTF-8 character change at the end of your version of the patch. That usually happens to me when emacs gets confused about the file encoding. I've re-re-attached the patch, re-rolled against today's HEAD (it's hard to keep up!), and without the UTF-8 mixup.

bjaspan’s picture

StatusFileSize
new8.42 KB

New patch based on feedback from chx. There is now a user permission 'no required password change'. If a user has that permission, they are never required to change their password by this patch.

bjaspan’s picture

StatusFileSize
new8.42 KB

Re-rolled for HEAD.

chx’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new8.42 KB

I like the solution and only corrected one space in it.

dww’s picture

don't want to derail progress here, but if we're removing !password from all those notification templates (which i certainly support), should we even provide !password as a valid token at all? it's already an evil special-case we have to deal with in the code, so i'd be happier to see it go completely. or, do we want to still provide that as an available token, in case sites want to do the wrong thing and continue to use that in their welcome emails? i'm not touching the status, but if we're in favor of just removing !password entirely, it'd make the user.module code better, and a re-roll of this patch would be in order. thoughts?

dww’s picture

Status:Reviewed & tested by the community» Needs work

chx: seems like your patch wasn't rolled from user.module revision 1.785, since it doesn't apply (lots of offsets/fuzz, 1 hunk (non-trivial) failed). in fact, looks like your core workspace must have revision 1.782 or so. :(

dww’s picture

Assigned:Unassigned» dww

i'm re-rolling now... 1 sec.

dww’s picture

Status:Needs work» Needs review
StatusFileSize
new7.82 KB

just the old patch, but relative to revision 1.785. note that since http://drupal.org/node/144859 landed, the initial email for users pending admin approval doesn't include any login info at all, so that text is unchanged in this version of the patch.

i'll roll another patch right now that removes !password entirely, too, in case folks like that approach.

dww’s picture

Assigned:dww» Unassigned
StatusFileSize
new11.37 KB

and here's one completely removing !password. this is untested, so please review/test before RTBC'ing it.

bjaspan’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new10.07 KB

I updated my patch to 1.785 but then must have attached the old version.

I think removing support for !password is a substantially more controversial change than changing the default messages not to include it. Some sites might depend on it for some reason. As it is, the patch adds functionality but allows admins to configure the old behavior if they want. So, if you want to remove support for !password, I suggest creating a new issue for that so this patch can be committed.

I updated the patch (the one leaving in !password support) to improve the text strings. For example, I changed "Your password and further instructions have been mailed..." to "Login instructions have been mailed...". I made no other code changes. chx and dww have already reviewed the logic of this patch, chx marked it RTBC, and all I did was update some strings, so I'm marking it RTBC again.

dww’s picture

re-reviewed and briefly re-tested. #15 is RTBC indeed. i agree removing !password is potentially controversial, so i'll leave that alone, just wanted to post it in case folks wanted to try to include that here.

bjaspan’s picture

StatusFileSize
new10.07 KB

Re-rolled for HEAD.

chx’s picture

Version:6.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review

Let's get back to this later.

chx’s picture

Version:7.x-dev» 6.x-dev

Maybe not, there is no API change, it can be thought as usability stuff.

chx’s picture

Status:Needs review» Needs work

Finally, it's CNW 'cos it does not apply any more. 4 out of 13 hunks fail in user module.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new10.16 KB

Currently on all the sites I implement, I change the one-time password email to remove the temp password. I’m greatly in favor of making the password change required.

I’ve re-rolled the patch. I made no modifications and only verified that it doesn’t produce any PHP parse errors. I would bet that Barry and Derick’s previous reviews are still valid, but I’ll let them mark it RTBC since I’m not doing a proper review.

JohnAlbin’s picture

Looking at the patch a little more closely now, I have a question. If we don’t provide the new password in the email, what’s the point of a "no required password change" privilege? How would the user know the new password? Wouldn’t they would still need to set their own password?

bjaspan’s picture

This patch is a combination security improvement and UI improvement.

The security improvement is simply just changing the default outgoing email messages not to include the randomly-generated plaintext passwords. We do not need to send them and doing so just smells like poor security practice.

The UI improvement consists of forcing users to change their password after a one-time login link. Yes, this is a UI improvement, not a security improvement. As John points out, forcing users to change a password they do not know and has never been disclosed is not a security benefit. It is a UI improvement because it solves this problem: User forgets password, asks for a one-time login link, and uses it. The site says "please change your password" but they forget to and they proceed to use the site. The next time they come back, they still cannot log in and get annoyed at having to do the email thing again. By forcing them not to forget to change their password, we avoid the annoyance on the follow-up visit. Also, when a password-based site sends a one-time login link, users expect to have to change their passwords, so by requiring it we are following user expectation which improves experience.

In short: In this case, restricting the user's behavior constitutes an improved experience. "Form is liberating." :-)

The "no required password change" permission is to allow sites that *always* use one-time logins to continue their current form of operation. chx says he has some sites in this category.

dww’s picture

Title:Require password change after one-time login» Security/usabillity: require password change after one-time login
Status:Needs review» Reviewed & tested by the community

I just tested #21 thoroughly on a site that properly sends out emails (it still applies with fuzz). I also re-reviewed the patch and found nothing wrong with the code.

This is urgently needed, since sending clear-text passwords is a shame, and we can do better. I'm with bjaspan: this is a security enhancement and a usability improvement, both of which should still be allowed in at this point. Also, this touches a lot of strings, so it should definitely go in before the string freeze.

Thanks!
-Derek

Dries’s picture

Status:Reviewed & tested by the community» Needs work

1.

<?php
$items[] = l(t('Request new password'), 'user/password', array('title' => t('Request new password via e-mail.')));
$items[] = l(t('Reset password'), 'user/password', array('title' => t('Request a one-time login link via e-mail.')));
?>

I think that both 'Request new password' and 'Request new password via e-mail.' are more friendly terminology then 'Reset password' and 'Request a one-time login link via e-mail.'

2. In core, 'user_pw_change' -- we don't abbreviate the word 'password' like 'pw'. Write 'user_password_reset' or something else without abbreviations.

3. I don't know about the 'password has been sent' vs 'login instructions have been sent' change. I don't need instructions to login (I can figure that out on my own, thank you very much) -- however, it _is_ important to know that a _password_ has been sent.

We want to undo a number of changes in this patch, IMO.

dww’s picture

Status:Needs work» Needs review
StatusFileSize
new10.21 KB

@Dries, I think you miss some of the point of the rest of this patch. The word "password" is removed from all these user-facing texts since there is no password included in these emails. That's the security fix in this patch. While "Your password has been sent to you" might seem more "friendly", it's wrong. Therefore, the text should stay as it is in the patch, or another alternative should be proposed that doesn't involve the term "password".

So, here's a re-roll that:
- Resolves the conflict in user.module from http://drupal.org/node/165358
- Renames the temporary SESSION variable from "user_pw_change" to "user_password_needs_change"

Otherwise, I'm leaving all the strings in here from bjaspan's previous efforts, since they're accurate. If someone has a suggestion that's both more user-friendly and still true, I'd love to hear it, but these all look good to my eyes. (Side note, we had the same debate over at http://drupal.org/node/110474 -- another bug fixed by this patch (which is even more important for core, now that user_status.module is just part of user.module)).

dww’s picture

Just to clarify -- the specific example of the password reset email -- "Request a new password" tells you that you're going to get a new password sent to you. Instead, you get a link that lets you log in, and then (depending on the permissions), you are redirected to your account edit tab to change your password. So, you're not requesting a new password at all. You're requesting a way to log back in so you can change it yourself...

Similarly, the "welcome email" doesn't include a password. So, "Your password and further instructions have been sent to your e-mail address" is simply a lie. Login instructions (and a 1-time link) are sent, not a password.

While I respect the desire to get the user's attention about these messages, I think it's worse to print incorrect instructions that catch their attention than more "boring" instructions that are true. And, for the security conscious like me, it's a feature that the drupal site doesn't tell me "Your password was just sent to you in clear-text through email"...

JohnAlbin’s picture

StatusFileSize
new9.77 KB

Regarding the text Dries mentioned:

1. The links and tabs leading to the form should reflect what the user wants to do. In that sense, “Request new password” is much better than “Reset password.” Once we are on the “request new password” form, we should explain what will happen when the form is submitted. In the attached patch, I’ve added some brief instructions and changed the button text on the form. But left the tabs as before.

3. I agree that “login instructions” is not very engaging. And “Password and further instructions” is inaccurate. How about “Password creation instructions”?

JohnAlbin’s picture

StatusFileSize
new9.77 KB

New patch also changes the new permission from “no required password change” to “skip required password change”.

dww’s picture

StatusFileSize
new9.78 KB

@JohnAlbin: thanks, almost of all of that looks like a step in the right direction, and it think your strings satisfy both Dries and my concerns. ;)

My only remaining gripe is that the drupal_set_message() text (which is hard-coded, not something the site can alter via the admin UI or even the theme) is still a lie for admin-approval sites:

Thank you for applying for an account. Your account is currently pending approval by the site administrator.
In the meantime, password creation instructions have been sent to your e-mail address.

This is exactly why I opened http://drupal.org/node/110474 in the first place...

To summarize, for sites that require admin approval, the default text of the e-mail messages (now in core, thanks to http://drupal.org/node/144859) are as follows:

1) Initial account request -- e-mail just says:

Thank you for registering at !site. Your application for an account is currently pending approval. Once it has been approved, you will receive another e-mail containing information about how to log in, set your password, and other details.

2) Once the admin approves the account:

Your account at !site has been activated.

You may now log in by clicking on this link or copying and pasting it in your browser:

!login_url

This is a one-time login, so it can be used only once.

After logging in, you will be redirected to !edit_uri so you can change your pass word.

Once you have set your own password, you will be able to log in to !login_uri in the future using the following username:

username: !username

So, in this case, "In the meantime, password creation instructions have been sent to your e-mail address." is a lie. All that was sent is an email telling them their account is pending approval, and only once their account is active are then sent password creation instructions... :( Nearly every drupal site I setup requires admin approval, and this is obviously an important use-case supported by Drupal, so it'd be nice if the UI didn't ignore this case.

One possible solution I proposed at #110474 is to make the text used for this message another setting at admin/user/settings, but we'd really need 2 separate settings, one for admin approval, and one for open registration, and that form is already rather complicated. :( So, attached patch just changes the hard-coded text to be more accurate for the admin approval case:

Thank you for applying for an account. Your account is currently pending approval by the site administrator.
If your account is approved, password creation instructions will be sent to your e-mail address.

How's that?

dww’s picture

StatusFileSize
new10.17 KB

New patch that applies cleanly after user.module was split into .inc files, and fixes a tiny typo in a previous patch (a 'by' was dropped from one of the template emails, apparently by accident). Any chance this is going into D6? This is the last time I'll re-roll this patch until a core committer agrees this is going in... ;)

bjaspan’s picture

Status:Needs review» Reviewed & tested by the community

I tested this version of this patch:

- Upon "request new password" (RNP), uid 1 is not required to change its password ("required") even without the "skip password change" permission.

- A new normal user is "required" when the account is created.

- A normal user is "required" upon RNP.

- A normal user is not "required" if it has the "skip password change" permission.

Ergo, the patch is done. Whether it goes into D6 at this stage of the release cycle is a separate question.

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs review

This generally looks good but:

- why do we need the skip required password change permission?
- typo here: "Instructions on how to change your password will sent to your e-mail address."

The patch also seems to address Dries' concern, so I think this will be ready to go to D6.

bjaspan’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new10.28 KB

I fixed the typo.

The 'skip required password change' permission got added after discussion with chx a few months ago. He observed that some sites may depend on the no-mandatory-change policy such as, e.g., a site for which users *never* know their own password and only ever use one-time login links. By providing this permission we make the previous behavior available as an admin option.

This should go in the D6 admin upgrade/release notes. Once committed, put the issue back to CNW and I'll do that.

Dries’s picture

Version:6.x-dev» 7.x-dev

I'm going to postpone this until D7.

dww’s picture

@Dries: given the time Barry and I (and others) put into this issue, is it too much to ask for you to spend 2 minutes providing justification for your decision? It would be very helpful to know what's going on in your head so that we can avoid wasting effort in the future. Thanks.

sun’s picture

Status:Reviewed & tested by the community» Postponed
lilou’s picture

Status:Postponed» Needs work

Patch #34 no longer applies.

chx’s picture

I think there is an easier / more complete solution to this. If we use a session variable then a simple logout solves the problem. Rather, let's move the password edit widget to the one time login page thus forcing the password change -- you can only log in by changing the password.

bjaspan’s picture

Wow, a blast from the past! I submitted this issue just after Drupalcon Sunnyvale. :-)

I like chx's new suggestion. It does seem simpler and cleaner.

Also, I now realize the correct solution to the "Request new password" text debate earlier in this issue. The text on the tab can be changed to "Reset password." When you click that tab and enter an email address, we sent you an email message *without* a password, so your password has not been reset; we previously felt that that meant "Reset password" was misleading text. However, when you click the link, you have the *opportunity* to reset your password. Thus, the text is correct: "Click this link if you want to reset your password; we'll email you a link you can click on which lets you reset it."

Now all we need is a patch.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new4.18 KB
Failed: Failed to apply patch.
[ View ]

Although I continue posting to this issue nothing is used from earlier patches so please credit accordingly :)

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Erm. Why forcing a password change? I like being able to log in via email without ever using a password. I'm ok if we suggest a password change on this page, but forcing it would make login via email impossible.

chx’s picture

Status:Needs work» Needs review

My thought was that even if someone steals a one time login link and logs in as you, you would know something has happened because the password has changed.

mr.baileys’s picture

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Ok, after discussing with chx and webchick, I'm +1 on this, assuming it is configurable (or at least that a contrib module can override this behavior).

chx’s picture

it's a form. Obviously you can override.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new6.61 KB
Passed: 12204 passes, 0 fails, 0 exceptions
[ View ]

After a lot of suffering now comes with tests updated too.

chx’s picture

StatusFileSize
new6.61 KB
Passed: 12234 passes, 0 fails, 0 exceptions
[ View ]

With less debug.

c960657’s picture

Status:Needs review» Needs work

The last paragraph of the email says “After logging in, you will be redirected to http://drupal7.dev.chsc.dk/user/1/edit so you can change your password.” This is no longer true.

In the password change form, you may get the impression that you have to enter your current password. To emphasize that the user should choose a new password, I suggest a label right above the first password field saying e.g. “Choose a new password for the account admin”.

I widely used alternative to the link title “Request new password” is “Forgot your password?”.

The UI talks about a “one-time login”. Now that you are forced to choose a new password, I think the term “password reset link” makes more sense. With that in mind, I don't think it is necessary to write both in the email, on the password reset form, and in the confirmation message upon login that the link can only be used once, or that it expires at a specific time, as long as the user gets a proper error message if he tries to use an old link (the current message is fine, I think).

vegemite4me’s picture

If someone needs this functionality in 5.x, I have added rough instructions here http://drupal.org/node/572610

markus_petrux’s picture

subscribing

Alan.Guggenheim’s picture

Version:7.x-dev» 6.16

We need a patch for 6.16

sun’s picture

Needs to be tackled for HEAD first.

Alan.Guggenheim’s picture

Version:6.16» 7.x-dev

Is there a clear document or issue page for the whole registartion/login/password lost process?
For "managed" sites with many new users (of all ages and degree of computer saviness), the current implementation really is generating 80%+ dissatisfaction, rejects and non-returns.

Alan.Guggenheim’s picture

Can we have a version for 6.16?

goatvirus’s picture

subscribe

animelion’s picture

ditto for 6.16

crea’s picture

subscribing

Fortyhands’s picture

subscribing

RobLoach’s picture

Priority:Normal» Major

Subscribe....

RobLoach’s picture

Priority:Major» Normal

Didn't mean to change priority.

jenlampton’s picture

Status:Needs work» Closed (duplicate)
nicodv’s picture

Hello, I know this thread is quite old but I noticed this "error" on one time login process and I was struggling to fix it... so considering it's 2012 and the patch is from 2009, what's the reason this patch hasn't been included in core yet?

And, since I'm not a good coder, how would I add this patch without getting it cleaned out in the next core update?

I know i'm missing something, but still I'm very interested in this fix.

Thanks