Security/usabillity: require password change after one-time login

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

- 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. ;)

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

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?

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. :(

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

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.

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

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.

Let's get back to this later.

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

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

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.

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?

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

1.

-  $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.

@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)).

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"...

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”?

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

@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?

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... ;)

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.

I'm going to postpone this until D7.

@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.

Patch #34 no longer applies.

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.

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

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.

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.

Related issue: #75924: Include fields for resetting password on the one-time password reset page

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).

it's a form. Obviously you can override.

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

With less debug.

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).

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

subscribing

We need a patch for 6.16

Needs to be tackled for HEAD first.

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.

Can we have a version for 6.16?

subscribe

ditto for 6.16

subscribing

subscribing

Subscribe....

Didn't mean to change priority.

Looks like this issue is a dupe of #75924: Include fields for resetting password on the one-time password reset page

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