Following a password reset link while logged in leaves users unable to change their password

I can't reproduce this with current HEAD using user #1 and a normal user. Can you retry this with a HEAD install?

- Do you get the "You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password." message?

Seems to be ok.
If you still see this and can reproduce with HEAD, retag 7.x-dev and reopen.

I can reproduce this using the release version. There is simply no way to change the password without the old password. If you go the route of resetting the password it sends you a link to login, but the link from the page to which you are taken is the standard account edit page, and requires the old password. At no point are you provided with a temporary password, nor is there a password change option that doesn't require the old password.

Stuck. I'll try the workaround, but this is definitely a bug.

I forgot to change the status to active, since the bug is apparently still evident in the release.

if you are log out and click the reset links, there is no ask for the old password. I tested with both D7 & Dev version.

can you provide more detail steps

I didn't get a request for the old password to click on the reset link, but that just triggers an email. After you click the link in the email the page says you are logged in but you should reset the password. If you are user 1 the edit page that link takes you to requires the old password, which defeats the whole purpose of the password reset.

FWIW the workaround was successful, but it shouldn't be that complicated.

If you are LOGGED and request a new password:
on edit page, Drupal ask for old password (ALL USERS)

IF you are LOG OUT and request a new password:
on edit page, Drupal don't ask for old password (ALL USERS)

so the problem here is for all users.

I get this error for normal users as well, using drupal 7.8, same as #3

I have the same problem, with drupal 7.8.

.b

Make sure you are logged out, then request a password reset
http://drupal.org/node/1314654#comment-5138056

Just had this occur to me - the first time you click on the password reset link and are taken to the user page to reset your password it does not request the old password.

However, if you do not enter a new password (in my case I got distracted with some other user settings and hit save without setting a new password) it will then save the profile and from thereon will request the old password.

The workaround is to simple request to reset the password once more.

The most efficient solution is probably what is suggested here: #75924: Include fields for resetting password on the one-time password reset page so I would recommend this issue be closed.

I encountered this problem too.

While #75924: Include fields for resetting password on the one-time password reset page is being resolved, you can use this stop-gap module to remove the Current Password requirement.
No Current Password module: http://drupal.org/project/nocurrent_pass

If you want the user to set the password when they click the one time login link, see http://drupal.org/project/password_hustle

Is this due to the timeout? I've had users request a new password or get the same one time link when I activate their account (users must have admin approval), then they don't use this link within the 1 day, but the link still logs them them in. When they then try to change their password the user edit requires the old password as normal. If they use the link within one day then the edit page just has new password and repeat new password boxes, and does not include current password.

The error then would seem to be that the one time link still logs them in even after one day has lapsed. It should really not do this and instead say one time link has expired.

UPDATE: I can't get this to repeat unless it happens just the first time an account is created. I've just clicked an old link and it says this has expired as it should do.

From #11 does this mean we can close this issue?

I'm affected by this, too.
The user clicks immediately the link when receiving the email, but the "current password" still appears.

The patch in the link given at #11 failed the test. The issue is still not solved.

Issue definitely still exists in core. By default the password login link takes you account page where you can change the password without requiring to eter the current password. But, if you navigate away from the account page before changing your password, or if you have a rul that sends the user elsewhere upon login then subsequently visiting the account page required current password to change it, which obviously they dont have as they were logged in via a one time link from a password reset email.

My new user clicks on the one-time link in their email and gets a "Reset password" page that invites them to "Click on this button to log in to the site and change your password." When they click on the button they go to the site front page, which I presume is incorrect behaviour. They cannot change their password via the User menu as they have to supply the "current" password, which they don't know.

timjh, check out the module http://drupal.org/project/password_hustle. It gives them the change password form as they log in.

Yes, I had spotted your mention earlier in the thread and looked at password_hustle. I will give it a try. I posted becasue I was surprised that there weren't more bug reports on this serious issue. Only 65 sites are using password_hustle. Does that mean that one-time logins work perfectly OK for most sites?

I think one time logins work fine for most basic installs (which is why password_hustle doesn't have thousands of installs). However, if you have any module that modifies redirection away from the user edit page at login, you will not be able to return to the user page and modify the password without the original password. For example, logintoboggan states, "Optionally redirect the user to a specific page when using the 'immediate login' feature." For our purposes, we had a custom module that was sending people upon login to different pages depending on their progress in a process, meaning they were never able to change their passwords.

Marking this case as "works as designed," especially since password_hustle exists for sites that want to redirect away from user edit upon login.

Thanks for your response, Solomon. You made me think about the problem more carefully. I thought I was not redirecting after login, but had forgotten that I had set up a trigger to do exactly that. I've removed the trigger and now everything works as it should.

I'm adding my site to the list of affect installs with this bug. The bug was not present until recently, but it is now generating dozens of support requests each week for me, which is something I do not have time for. I understand how the workaround works, and it does fix the issue.. however it is not a solution as it still generates support requests as each user affected by this issue on a site must be then told to logout and retry.

Same thing happens when a new users clicks on their user/rest/#$%^#^& link for first access and creating new password. If the user attempts to edit any other field on the page...username, name, etc. they can't and are asked to enter old password, which they've never had.

I am facing the exact same problem as IWasBornToWin!

The one time log in is not a fool-proof system! If the user gets distracted and goes to another page, without changing the password, they are stuck in this endless loop. Having the system send an email with a random generated password will make things to much easier!

Any solutions?

Thanks!
PJ

As a temporary helper - I created a block in the highlighted area to only show on user/rest/* for anonymous roles with this message

<p><span style="color:#ff0000;">Important</span> - <u>Before</u> adding or changing anything in your profile settings you must first create your new password <strong><u>and save it</u></strong>. Then you may add or make other changes.</p>

Thank you!! Excellent idea. However, in my case, the user must enter all the required fields when they save the new password as the user information is on the same page as the edit password. Hope this wont pose a problem! Thank you!

PJ

Not sure I understand, all required fields would have to be entered prior to the user registering and getting a password. Unless you're allowing the user to set password at registration? I don't. I allow them to register & enter required fields....then after saving they get temp password. But all their required fields were already input at registration.

I am guessing you meant "user/reset" page in comment #28.

Yes, I do allow users to create a password while registration itself.

Thank you!

Yes, "reset"

I do not know how to help you if allowing them to create passwords up front.

#13 worked for me !

Thank you

I don't think that this Issue is resolved or "works as designed" though.
I'd guess that many "more complex" sites use redirects on login and then run into this - just like me now.
Why doesn't Drupal store the recovery-state in the session or something? Who implemented this strange behavior that one can only reset its password when the first page is the password-reset page?
This is totally frustrating.. Anyway, #13 works..

Yes same problem here.

#13 worked for me !

Thank you

I had this issue because I was using Rules to redirect users to the home page right upon login, so they missed the special form to save a new PW. I added a condition to only redirect users who were anonymous, and it fixed the problem for my particular circumstances, since you are authenticated once you click the one-time login button, and thus don't get redirected like someone who logs in through the regular means.

from Rules...

User has logged in
NOT User has role(s)
Parameter: User: [account], Roles: authenticated user
 
Page redirect
Parameter: URL: [site:url]

I came on this thread with the same behavioral issue as most of you, but I found no explanation as to what is really going on, so I did some digging in the core user.module and found the explanation in the user_pass_reset() function in user.pages.inc:

[...]
        if ($action == 'login') {
          watchdog('user', 'User %name used one-time login link at time %timestamp.', array('%name' => $account->name, '%timestamp' => $timestamp));
          // Set the new user.
          $user = $account;
          // user_login_finalize() also updates the login timestamp of the
          // user, which invalidates further use of the one-time login link.
          user_login_finalize();
          drupal_set_message(t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
          // Let the user's password be changed without the current password check.
          $token = drupal_hash_base64(drupal_random_bytes(55));
          $_SESSION['pass_reset_' . $user->uid] = $token;
          drupal_goto('user/' . $user->uid . '/edit', array('query' => array('pass-reset-token' => $token)));
        }
[...]

user_login_finalize() calls hook_user_login() before the user_pass_reset() function sets a session variable and does a drupal_goto() to the user/[uid]/edit page. If you, or any other module does a drupal_goto() inside a hook_user_login() then you're in trouble.

If you want to redirect inside a hook_user_login(), you should do it like mentioned here and you will eliminate this problem altogether.

Ofcourse, this won't work for everybody; no idea about actions and triggers...

I've just run into this issue myself. I use LoginToboggan but I don't have either of the redirection URLs set to anything. On clicking the link in the email, the user is told "Click on this button to log in to the site and change your password" but that's not what the button does - its simply logs the user in and they've got to find their own way to change the password - whereupon they're asked for their old password. This issue is not resolved - if it "works as designed" then the design is wrong.

Drupal 7.26
Login Toboggan 7.x-1.3

Note that the No Current Password module gets over this issue
https://drupal.org/project/nocurrent_pass

Updating the title & summary based on comments -- this affects all users.

This does seem like a not very sensible workflow for users, but on one of my sites with ~1400 active users, I get bugged by this about every 2 weeks or so.

Also confirming -- looking at the code, since I've not found an email logging module for D8 -- that this happens on D8 too.

I see two ways to fix this:

A. Change the message "You are logged in as USERNAME. [LINK]Change your password[/LINK]" to say that the user should log out and start again if they actually want to change their password. Eg. "You have followed a password reset link but you are already logged in. To reset your password, please [LINK]logout[/LINK] and use the password reset form again."
B. Change the link in the message to include the token in the URL string and add the token to the session, so that the user form doesn't ask for the existing password.

Proof of concept patch on D7 for method B.

Patch for D8, method B:

- the message "You are logged in as USERNAME. [LINK]Change your password[/LINK]" is not changed
- however, the link now includes the token, and the token is set on the session when the link is output
- this means that if the user follows the link to the user form, it doesn't show the existing password form element
- the session token is cleared when the user submits the form with a new password.

Tagging: this affects d.org, see issue (with duplicates) at #2185247: Fix the password reset process.

I do not think that undoing the security improvement will be ok in this issue.

As I understand history, in Drupal 6, editing user could change password without entering current password.
In drupal 7, it was a security improvement to require current password for some changes on user edit, including picking new password.

----

Thought about this a bit.
I suggest another approach:
a) When someone clicks the "request new password", it logs them out.
OR
b) When someone clicks the link in their email, it logs them out, and then shows the normal form for resetting password.

I suggest we try to implement b). Not sure if there was previous work on that implementation option.

> b) When someone clicks the link in their email, it logs them out, and then shows the normal form for resetting password.

I'd be fine with that.

But the end result is the same: the user gets to input a new password without having to enter their old one. Is this a security improvement just because we don't have the switch on the $SESSION value?

People will forget their passwords.

It would be great if in D9 we had Two-factor Authentication in Core, but it's not going to happen in D8. It could be added in drupal.org though https://drupal.org/project/tfa

I like @YesCT's option b) and really don't see another viable option just now. People use multiple devices and browsers. It gets confusing for the user and ultimately, this is a usability problem.

@joachim I think your solution would work well from /user/%/edit but not from /user/password

Obviously, the second is only a problem if you're logged in on one browser, but not on another (like whatever you set your default browser to be).

We don't really know how folks are getting to the Request new password function really. Are there other options other than these two?

I don't see this as something we can avoid. It's great if you have the password stored, but ultimately most folks don't have good practices.

> I do not think that undoing the security improvement will be ok in this issue.

> @joachim I think your solution would work well from /user/%/edit but not from /user/password

I'm suddenly thinking that perhaps people haven't understood my patch.

As things stand, the user edit form requests the current password to change the password. EXCEPT if there is a special value set in the session.

This is what the password reset link does when you follow it: it sets the special session token for 'don't ask current password'.

What my patch does is add this token when you follow the password reset link AND you're logged in.

I had mis-understood that.. I'll have to test it but this is good to know that you've got both angles covered.

It's all within if ($user->id() == $uid) {} so I assumed your new code would only work if you were already logged in.

I didn't see where it was in context though so thought it was doing something else.

Still seeing this problem.

Reproduced it multiple times (without ever logging in).

Deleting all site cookies fixes it.

Seems like a pretty legit bug.

The patch no longer applies.

I'll start working on a reroll of this patch.

I've rerolled the patch. The code moved from core/modules/user/user.pages.inc to core/modules/user/src/Controller/UserController.php

I'll give this a review.

After applying the patch, I'm getting the following error message.

Fatal error: Class 'Drupal\user\Controller\Crypt' not found in /Users/opdavies/Code/drupal/core/modules/user/src/Controller/UserController.php on line 89

This needs the Crypt class loaded. Here's an interdiff that does that.

This though creates another error, which I'm trying to fix.

Fatal error: Call to undefined function Drupal\user\Controller\url() in /Users/opdavies/Code/drupal/core/modules/user/src/Controller/UserController.php on line 94

Attached patch(es) solves exactly this case for D7:

following a password reset link while logged in leaves users unable to change their password

How: Well, the same way at it works for not logged in users, but instead adding the token to the redirect it's added to the link the user has to click to go to the edit page.
Another more "radical" approach could be to simply redirect logged in users as well - see the more radical patch :)

sorry, versions tripped me up.

retesting with those on 7.x.

I will review the 8.x patch from #54 (which passed 8.0.x tests)

1.
We dont have to @see issues that are the issue fixing something (only when the issue being referenced is a todo. Git blame can tell us to find this issue that changed these lines.

2.
2a. fixed a nit wrap at 80 characters for comments.
"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions"
https://www.drupal.org/node/1354#drupal

2b. Also, while doing that, clarified the wording.

3. While looking at that, with the patch applied, I noticed a nearby comment that is not correct with this patch. (fixed that; took out the incorrect comment)

---------
Manual tested.

4. On the request password form, it says "You must log out to use the password reset link in the email."
But now, that is not true. So changing that also.

5. Actually trying the link sent via email, I get ( ! ) Fatal error: Class 'Drupal\user\Controller\Crypt' not found in /Users/ctheys/foo/drupal2/core/modules/user/src/Controller/UserController.php on line 86

Similar to #58

So, this is still needs work for that (at least)

6. Since that

--

updated the issue summary.

--

7.
I wonder if we can add an automated test. ... something like post the url that that generates, and assert sees the form, assert that the currect password field is not on that form.

Actually trying the link sent via email, I get ( ! ) Fatal error: Class 'Drupal\user\Controller\Crypt' not found in /Users/ctheys/foo/drupal2/core/modules/user/src/Controller/UserController.php on line 86

#58 has an interdiff.txt attached to it, which includes the

use Drupal\Component\Utility\Crypt;

line so that it's included by the autoloader. That fixed the missing Crypt issue for me. Should I have uploaded an updated patch with this included as well as the interdiff?

Here's a re-rolled interdiff from #58, based on #67, along with the updated patch.

Manually testing with using the one-time link by a logged in user breaks with:
Call to undefined function Drupal\user\Controller\url()

Attached patch replaces url() by _url(). This is not a final solution because _url() is deprecated, but it will be fixed in #2343669: Remove _l() and _url().

Having read through https://www.drupal.org/node/2346779, here is another patch that removes _url() and replaces it with \Drupal::url().

We 'd better use $this->url()

That's cleaner. :)

I didn't try it that way.

Still needs tests?

Test added

Interdiffs are very helpful for reviewers.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

Also, we should have a tests only patch to make sure it is failing (the way we expect it to).
https://www.drupal.org/contributor-tasks/write-tests has some instructions on making test only patch that might help.

I had the same issue as described by @Grum about a week ago when i signed up for a new account. Fortunately, I was able to change my password after several retries.

My curiosity with this issue has turned into a major headache. Trying to duplicate this issue I went to my account settings, while logged in, and clicked on request new password. I ignored the instructions on the next page "you must log off before using the one-time link." and just used the one-time link. To be honest I can't remember exactly what happen next. I've been pulling my hair out for the past 3 hours. I think when I clicked on the one-time link (still logged in) i got "access denied", and I think i was automatically logged out. I then did the standard /user/password (now logged off) and was able to reset it normally. This is where the headache started and where I'm at now. Now...as long as I stay logged in there's no problems. BUT, if I log off and then try to log back in I get "password not recognized". ((It also says "5 failed login attempts for this account. It is temporarily blocked." This is because I did try my password 5 times and failed. )) So, I'm stuck with having to reset my password EVERY TIME I'm logged off. By the way, this is now the same issue I've had with Drupal.org since I've joined. i.e., if I ever log off I cannot log back in until I reset my password. I think for now until this is resolved the link "request new password" in the account settings should be removed and simply add a statement "If you forgot your current password, please log off to request a new password."

However, this can be an issue as well. What if a user can no longer access the email he's/she's registered with? Then resetting their password would do no good because they wont get the one-time link. This is originally what got me looking into this. What if a user needs to change their email because they don't have access to it anymore (for whatever reason), AND they forgot their site password? This really isn't too unrealistic with most people using auto saved passwords. Not hard to forget. Ultimately this is less of an issue for me since I/we can't resolve every single type of issue with forgotten passwords.

I'm more concerned about why I have to reset my password all the time now...on my site (and drupal.org since we're discussing it). I have cleared all cache, deleted all cookies, and still no change. Also, it says that my account is temporarily blocked, but in admin/people it still shows me "active." I'm guessing I need to get into my DB and delete an entry associated with this, but I do not know what that would be. Meanwhile I have used CSS to display:none "Request new password" for logged in users. Hopefully this will at least prevent my users from running into this issue.

Any suggestions? I'm using 7.33

UPDATE
Fixed the issue with having to always reset password.
Went into phpmyadmin > tabel:flood > deleted all occurrences with my UID

UPDATE
Removed my custom CSS "Request new password" > display:none
Changed the t() in user.module

public_html/modules/user/user.module
Line: 1081

CHANGED
$current_pass_description = t('Enter your current password to change the %mail or %pass. !request_new.', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));

TO
$current_pass_description = t('<strong>Enter your current password to change the %mail or %pass. If you forgot your password you must log off and click on "Reset password."</strong>', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));

Not proud of this hack but I needed it. Now the only way a logged in user can request a new password (while logged in) is if they type in the URL /user/password. 99.9% of my users wouldn't know to do that.

I would still greatly appreciate better alternatives. I do not want to get rid of the "Current password" field.

I'm testing a different approach now that would permit me to lose the "Current password" requirement (e.g., project/nocurrent_pass) whereby I created a Rule to notify the user at their original email address if someone was able to hack their account and change the email address. So far works nicely. No matter what the email is changed to this rule always sends to the original registered address. Still testing, but I'm liking this.

{ "rules_email_change" : {
    "LABEL" : "Email change",
    "PLUGIN" : "reaction rule",
    "ACTIVE" : false,
    "OWNER" : "rules",
    "REQUIRES" : [ "rules" ],
    "ON" : { "user_update" : [] },
    "IF" : [
      { "NOT data_is" : { "data" : [ "account-unchanged:mail" ], "value" : [ "account:mail" ] } }
    ],
    "DO" : [
      { "mail" : {
          "to" : "[account-unchanged:mail]",
          "subject" : "Your Email Address Has Changed On Site.com",
          "message" : "* JUST WANT TO MAKE SURE YOU ARE AWARE *\r\n\r\nYour account email address was just changed to: [account:mail]\r\n\r\n---------------------------------\r\n\r\nThis email is a security feature that alerts you when your email address has been changed.\r\n\r\nIf you did not make this change, please \u0022Reply\u0022 to this email to notify us. Or contact us at: support@site.com\r\n\r\nWe will permanently delete this account:  [account:mail] (Usually within 10 minutes of receiving your reply.) \r\n\r\nNote: You do not need to write anything in your \u0022Reply.\u0022 Receiving this email is all we need to act.\r\nNote: All your ads will be deleted as well. Please safeguard your password and do not share it.\r\n\r\n\r\n-Support Team",
          "language" : [ "" ]
        }
      }
    ]
  }
}

Can anyone confirm what else needs to be done with this issue, or is it just a case of waiting for it to be reviewed further?

@System Lord you could try the Drupal 7 patch in #59.

I marked #1872404: One-time password link has broken message if the user is logged in as a duplicate.

@joachim the patch in #83 is supposed to fail. #81 includes the fix

error: patch failed: core/modules/user/src/Controller/UserController.php:14
error: core/modules/user/src/Controller/UserController.php: patch does not apply
error: patch failed: core/modules/user/src/Form/UserPasswordForm.php:92
error: core/modules/user/src/Form/UserPasswordForm.php: patch does not apply
error: patch failed: core/modules/user/src/Tests/UserPasswordResetTest.php:72
error: core/modules/user/src/Tests/UserPasswordResetTest.php: patch does not apply

Just rerolling the patch in #81 for now.

The patch in #98 no longer applies to 8.0.x.

error: patch failed: core/modules/user/src/Controller/UserController.php:88
error: core/modules/user/src/Controller/UserController.php: patch does not apply

I'll re-roll the patch.

Patch re-rolled. Testing it locally before uploading it.

Rerolled #98 and fixed failing tests.

#111 works for me.

Thanks everyone -- I see this issue is tagged as affecting d.o, so I am bumping it to major. However, I'm unclear on how the patch addresses the issue described in the title.

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -88,12 +89,22 @@ public static function create(ContainerInterface $container) {
   +        // Add a session token to the link to let the user change their password
   +        // without having to enter their current password, since they may not
   +        // know it.
   ...
   +        drupal_set_message($this->t('You are logged in as %user. <a href="@user_edit">Change your password.</a>', array(
   +          '%user' => $account->getUsername(),
   +          '@user_edit' => $this->url('entity.user.edit_form', array(
   +            'user' => $account->id(),
   +            'pass-reset-token' => $token,
   +          )),
   +        )));
   

   This seems like a security regression to me. Users should have to know their password to change it under normal circumstances, or should follow the normal email password reset request process. Otherwise, this allows someone to change someone's password just by getting access to an active session.

   We could change this to a link to send the password reset token? Or am I misunderstanding this change?

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -88,12 +89,22 @@ public static function create(ContainerInterface $container) {
   +        $_SESSION['pass_reset_' . $account->id()] = $token;
   

   Storing this directly in the $_SESSION here seems... not right.

3. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -92,7 +92,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        '#markup' => $this->t('Password reset instructions will be mailed to %email. You must log out to use the password reset link in the email.', array('%email' => $user->getEmail())),
   +        '#markup' => $this->t('Password reset instructions will be mailed to %email. You must log out to use the password reset link in the email.', ['%email' => $user->getEmail()]),
   
   +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -51,21 +51,16 @@ protected function setUp() {
   -    $account = $this->drupalCreateUser();
   +    $this->account = $this->drupalCreateUser();
   ...
   -    $this->drupalLogin($account);
   ...
   +    $this->drupalLogin($this->account);
   
   @@ -85,7 +80,7 @@ function testUserPasswordReset() {
   -     // Verify that the user was sent an email.
   +    // Verify that the user was sent an email.
   
   @@ -159,12 +154,40 @@ function testUserPasswordReset() {
   -    $_emails = $this->drupalGetMails();
   -    $email = end($_emails);
   +    $emails = $this->drupalGetMails();
   +    $email = end($emails);
   ...
   -    return $urls[0];
   +    return reset($urls);
   

   These changes are out of scope, and should be removed from the patch.

4. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -51,21 +51,16 @@ protected function setUp() {
   -    $this->account = user_load($account->id());
   ...
   -    $account->login = REQUEST_TIME - mt_rand(10, 100000);
   -    db_update('users_field_data')
   -      ->fields(array('login' => $account->getLastLoginTime()))
   -      ->condition('uid', $account->id())
   -      ->execute();
   +    $this->account->setLastLoginTime(REQUEST_TIME - mt_rand(10, 100000));
   +    $this->account->save();
   

   Are these changes related to the patch? If not they should be removed and handled in a separate issue.

5. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -159,12 +154,40 @@ function testUserPasswordReset() {
   +  }
   +
   +
   +  /**
   

   Minor: there's a double blank line here.

Before/after screenshots would also be a help since there is a UI change.

This seems like a security regression to me. Users should have to know their password to change it under normal circumstances, or should follow the normal email password reset request process. Otherwise, this allows someone to change someone's password just by getting access to an active session

After reading all the previous comments I think you are right and I think the easiest way to fix this is like @YesCT previously suggested:

When someone clicks the link in their email, it logs them out, and then shows the normal form for resetting password.

The patch in #114 still applies to 8.0.x.

I'll move on and perform a functional review.

Confirmed that after applying the patch and requesting a new password, once I click the login link, I'm logged out of my previous session and can then continue with the standard one-time login process to set a new password.

Setting as RTBC.

Looks like a test bot failure

Thanks, this seems like a much better solution. Thanks also for documenting the manual testing.

Note that, in general, it's good to document the specific testbot failure before retesting a patch, since sometimes this can indicate the patch is introducing fragility or a random failure. The added test coverage doesn't include anything suspicious, and I ran the test locally a number of times and didn't get any fails.

I reviewed the patch with git diff -w and confirmed that the only functional changes are adding the user logout, and then executing the password reset. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

Yay!

Can this be backported?

Not sure if there is a better way to handle the user logout since it's not possible to use user_logout() due to hard-coded drupal_goto() call.

The patch in #124 applies cleanly, and the active session is closed and the user is logged out after clicking the one-time login link.

terrible, a simple but top-priority bug was NOT solved in core since 2010

why was current password required when resetting password ?

It does NOT make sense at all and often make drupal laughable due to this bug.

I cannot begin to explain the importance of this patch. While for the computer savy people, the current password reset workflow is ok. For the people that run websites with very computer unsavy people, this patch is a MASSIVE improvement. I run a website of over 100,000 golfers that have an average age of 72. Eight out of ten customer service issues stem from password reset process. This is going to help so much.

I reviewed the patch code and I tested it on our development infrastructure. I am now applying this patch to our production website because it really fixes a problem with Drupal 7.

+1 to RTBC

Bump

For what it is worth. I am running a website with 220,000 user accounts. This patch is much needed to improve the workflow of users resetting their passwords.

#124 failed for me on Drupal 7.34

@TravisJohnston Upgrade to 7.39?

Can you explain more, @TravisJohnston? Failed in what way? If you mean the patch didn't apply, then that could because you applied it to 7.34 rather than 7.x.

I really wish we could get this in core. It's a MASSIVE improvement to the user workflow. Our customer service requests numbers took a nose dive after I implemented this. Best improvement I have done to our site in over a year.

Yeah it could be that it was specific to 7.34, on the site I am working with I can not at this time upgrade 7.

patching file modules/user/user.pages.inc
Hunk #2 FAILED at 114.
1 out of 2 hunks FAILED -- saving rejects to file modules/user/user.pages.inc.rej
patching file modules/user/user.test

What is in the .rej file?

I will also point out that our issue is similar but not exact. In our problem, a user clicks the "Request new password" link in their account, then clicks the Send Request to Email Address button.

They are still logged in, they go to their email and click the reset link in their email. They are then sent to an Access Denied page.

If I viewed my email in a separate tab during this process, I can log myself out in the original tab for the website, refresh the Access Denied page, and it works. Some users report that they can get passed the Access Denied page but it asks for a Current Password which they don't have since they used the link.

Here it is

***************
*** 110,167 ****
          // Invalid one-time link specifies an unknown user.
          drupal_set_message(t('The one-time login link you clicked is invalid.'));
        }
      }
-     drupal_goto();
    }
-   else {
-     // Time out, in seconds, until login URL expires. Defaults to 24 hours =
-     // 86400 seconds.
-     $timeout = variable_get('user_password_reset_timeout', 86400);
-     $current = REQUEST_TIME;
-     // Some redundant checks for extra security ?
-     $users = user_load_multiple(array($uid), array('status' => '1'));
-     if ($timestamp <= $current && $account = reset($users)) {
-       // No time out for first time login.
-       if ($account->login && $current - $timestamp > $timeout) {
-         drupal_set_message(t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'));
-         drupal_goto('user/password');
-       }
-       elseif ($account->uid && $timestamp >= $account->login && $timestamp <= $current && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login, $account->uid)) {
-         // First stage is a confirmation form, then login
-         if ($action == 'login') {
-           // Set the new user.
-           $user = $account;
-           // user_login_finalize() also updates the login timestamp of the
-           // user, which invalidates further use of the one-time login link.
-           user_login_finalize();
-           watchdog('user', 'User %name used one-time login link at time %timestamp.', array('%name' => $account->name, '%timestamp' => $timestamp));
-           drupal_set_message(t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
-           // Let the user's password be changed without the current password check.
-           $token = drupal_random_key();
-           $_SESSION['pass_reset_' . $user->uid] = $token;
-           drupal_goto('user/' . $user->uid . '/edit', array('query' => array('pass-reset-token' => $token)));
-         }
-         else {
-           $form['message'] = array('#markup' => t('<p>This is a one-time login for %user_name and will expire on %expiration_date.</p><p>Click on this button to log in to the site and change your password.</p>', array('%user_name' => $account->name, '%expiration_date' => format_date($timestamp + $timeout))));
-           $form['help'] = array('#markup' => '<p>' . t('This login can be used only once.') . '</p>');
-           $form['actions'] = array('#type' => 'actions');
-           $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Log in'));
-           $form['#action'] = url("user/reset/$uid/$timestamp/$hashed_pass/login");
-           return $form;
-         }
        }
        else {
-         drupal_set_message(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'));
-         drupal_goto('user/password');
        }
      }
      else {
-       // Deny access, no more clues.
-       // Everything will be in the watchdog's URL for the administrator to check.
-       drupal_access_denied();
-       drupal_exit();
      }
    }
  }
  
  /**
--- 114,169 ----
          // Invalid one-time link specifies an unknown user.
          drupal_set_message(t('The one-time login link you clicked is invalid.'));
        }
+       drupal_goto();
      }
    }
+   // Time out, in seconds, until login URL expires. Defaults to 24 hours =
+   // 86400 seconds.
+   $timeout = variable_get('user_password_reset_timeout', 86400);
+   $current = REQUEST_TIME;
+   // Some redundant checks for extra security ?
+   $users = user_load_multiple(array($uid), array('status' => '1'));
+   if ($timestamp <= $current && $account = reset($users)) {
+     // No time out for first time login.
+     if ($account->login && $current - $timestamp > $timeout) {
+       drupal_set_message(t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'));
+       drupal_goto('user/password');
+     }
+     elseif ($account->uid && $timestamp >= $account->login && $timestamp <= $current && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login, $account->uid)) {
+       // First stage is a confirmation form, then login
+       if ($action == 'login') {
+         // Set the new user.
+         $user = $account;
+         // user_login_finalize() also updates the login timestamp of the
+         // user, which invalidates further use of the one-time login link.
+         user_login_finalize();
+         watchdog('user', 'User %name used one-time login link at time %timestamp.', array('%name' => $account->name, '%timestamp' => $timestamp));
+         drupal_set_message(t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
+         // Let the user's password be changed without the current password check.
+         $token = drupal_random_key();
+         $_SESSION['pass_reset_' . $user->uid] = $token;
+         drupal_goto('user/' . $user->uid . '/edit', array('query' => array('pass-reset-token' => $token)));
        }
        else {
+         $form['message'] = array('#markup' => t('<p>This is a one-time login for %user_name and will expire on %expiration_date.</p><p>Click on this button to log in to the site and change your password.</p>', array('%user_name' => $account->name, '%expiration_date' => format_date($timestamp + $timeout))));
+         $form['help'] = array('#markup' => '<p>' . t('This login can be used only once.') . '</p>');
+         $form['actions'] = array('#type' => 'actions');
+         $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Log in'));
+         $form['#action'] = url("user/reset/$uid/$timestamp/$hashed_pass/login");
+         return $form;
        }
      }
      else {
+       drupal_set_message(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'));
+       drupal_goto('user/password');
      }
    }
+   else {
+     // Deny access, no more clues.
+     // Everything will be in the watchdog's URL for the administrator to check.
+     drupal_access_denied();
+     drupal_exit();
+   }
  }
  
  /**

TravisJohnston,

That behaviour is exactly what is expected without the patch applied.

TravisJohnston,

As you're not running the version of drupal 7 the patch is based on - you will need to manually apply the patch.

Yeah that rejected everything in the patch so you will have to do as @MJCO explained and manually apply the patch or upgrade the site. If you are using drush, upgrading the site would be quicker and easier than manually applying the patch.

Yeah I had a feeling, thanks for the work so far though for me to work with.

I applied the patch manually and tested it. Doesn't work. Also some more information:

After a user "successfully" manages to change their password, logging back in with that password after doesn't work. So they are stuck in a loop of needing to use a new reset link each time they want to log in.

This is similar, and a little closer to what I am dealing with, but no responses. https://www.drupal.org/node/2448021

Yeah something isn't right... This has been one of the best things I have done to my site in over a year. It's a godsend to punt the session when following a one-time-login link.

I applied the patch manually and tested it. Doesn't work.

@TravisJohnston: does it work when patched against the 7.x-dev branch, or just not on the version that you're manually patching against?

Yeah something isn't right

@Perignon: is the patch in #124 no longer working for you to fix the original issue against the latest 7.x-dev version?

@opdavies I was commenting on @TravisJohnston's situation.

@opdavies

I guess against the version 7.34 that I'm applying it to, since I haven't applied it to a separate instance. Applying the patch not only didn't work but also it stopped the user from being able to log out properly at all afterwards. For example: User A initiates a reset, clicks the link, and resets the password. Attempts to log out, and is redirected to the homepage as logged in.

Marking back as "Needs Review" whilst we confirm there's an issue with 7.x-dev.

The patch applies cleanly for me, using the git apply command.

I'll test that it still works functionality when applied to 7.x-dev.

With the patch in #124 I find that after using the link, the user is logged out, and logged in again (by the watchdog messages), but then the session is lost, so I get 'Access denied' on the resulting user/%/edit page.

Tested manually on a complex, real D7 site, and also on a fresh D7 install.

Is there a need to log out the user, anyway?

Also, could the watchdog message be a bit more descriptive? maybe: Session closed for %name (one-time login link used).

Is there a need to log out the user, anyway?

I was wondering that too. For security reasons we need to make them click a link in their email, but I don't think it's necessary to log them out also.

On the other hand, when you request a new password while already logged in, the message you see on the screen says:

Password reset instructions will be mailed to [address]. You must log out to use the password reset link in the e-mail.

So we've already told them they need to log out, and forcing the logout automatically if they ignore those instructions would be consistent with that. It therefore seems like the simplest fix, if it can be made to actually work...

While you try to make things idiot proof, someone goes and creates a better idiot. In this case, this was one of the single most helpful things I have ever applied to my Drupal site. It cut support requests down by 70-80%! I would have at least one support ticket a week from someone saying "it is asking me for my current password and i don't know it."

I got to look into why this patch (#124) no longer works for other people here. I have it up and running on a live site and it is working like a charm.

Just ran through a test installation of a fresh Drupal 7.39 and applied the patch. Works like a charm.

Here is a screenshot of the user page with a successful password change. No Access Denied pages. I am going to move this back to RTBC since I tested it again on a fresh install and I am using this patch on a live website with great success.

Well, there are a couple people above who reported this broke things - the first was trying to apply the patch to Drupal 7.34 (where the patch doesn't apply) but not the second.

@pjcdawkins, can you explain in more detail what this means:

With the patch in #124 I find that after using the link, the user is logged out, and logged in again (by the watchdog messages), but then the session is lost, so I get 'Access denied' on the resulting user/%/edit page.

What exactly happens when "the session is lost" and what triggers it? What modules is the site using that might change the behavior of the login or password reset process?

Hrm. I should have tested against -dev.

Just tested against 7.x-dev and no issues.

Ah. It's because I'm testing with "drush uli" instead of emailing myself a link. The drush command appends "/login" to the reset URL, to skip the confirm form. It seems that bit isn't covered properly by this patch?

Setting "Needs work" because:

1. It should work if you append /login to the reset link. Drush does that, and there may be many other modules/configurations that do so.
2. We can remove the text that David pointed out ("You must log out to use the password reset link in the e-mail.")

@pjcdawkins I have never used that Drush command, but I went ahead and gave your scenario a try. I used Drupal 7.x-1.x and used the drush user-login command to generate a link and I do not get an access denied on the user/%/edit page after I reset a password. And to be thorough (test in at least two environments) I tested this with my production site using the Drush command generated link. So I have tested in on 7.39 and 7.x-1.x, in two environments, with and without Drush. All tests have passed. My local testing is Mac OSX and Fedora, my production is AWS Linux. All running PHP 5.5 or greater via PHP-FPM and all using MySQL.

OK normally I'd be hunting for the actual bug, but I can't see it yet... so I did some more manual testing.

Here's a site which shows the behaviour I'm talking about:
https://master-ywpmjuxyeercg.eu.platform.sh/

1. Register an account (requires email verification)
2. Go to user/%/edit, and then click "Request new password"
3. You should receive the email. Copy the link (don't click it!), paste it into your browser, append /login, then hit Enter
4. Observe that you get to user/%/edit with "Access denied" (you've been logged out)

and this is the site's source code, just to show you it's fresh D7 with only this patch:
https://github.com/pjcdawkins/demo_issue_889772

Small world, I now realize who you are from your Github account. Remember the guy showing you Freshdesk at Barcelona? :-D

So this issue gets even more complicated!

I followed your steps as you said and with your test site and I still don't get an "Access Denied" message. So I recorded what I did in a short screencast. Let me know if I misinterpreted your steps.

https://youtu.be/VKc0W1oco7w

I was able to reproduce this by doing what @pjcdawkins did.

The difference seems to be if you are logged in or not. If you are not logged into the website, such as what I did by starting a new incognito window in Chrome, then the /login doesn't matter. If you are still logged into the website and use the url with /login then it doesn't work.

I guess I have one fundamental question. Why does Drush put /login at the end of the URL when core does not.

@Perignon Drupal requires the /login to be added by someone clicking a button because the login link can only be used once, and some email clients will pre-fetch a link before a user has clicked on it to see if it's safe or give a little preview etc. This would use up the token, hence /login is required to actually use the token and login.

Drush automatically adds /login because the one-time login links that it generates usually get pasted straight into a browser or open the browser directly and so the pre-fetching issue isn't really there, and there's the benefit of saving a click in the browser to actually log in.

@Steven Jones Ok that makes sense with the email clients. So I have one question. Where does the /login get added when you click the link in an email? Since the link in the email doesn't have /login in it?

@Perignon - if you don't have /login, Drupal shows a confirmation form. You then click a button "Log in", which redirects you to the same URL with /login appended.

(and yes I remember you from Barcelona, hi!)

Just to chime back in, this module solved the problem for me.

Simple Password Reset https://www.drupal.org/project/simple_pass_reset

@pjcdawkins Ah... I have altered that login landing page for my site (see below). That is why I don't get the two stepped process anymore.

@TravisJohnston. I saw that module when I was looking for a solution but went an even simpler rounte. I am using this one, https://www.drupal.org/project/prlp, to give a quick password reset form when following the one-time login link rather than having to click "login" to get to it. This patch here allows me to kill the active login if they are.

Either way, I'll keep patching core if I have to. It is too valuable to me not too!

Just a note: we've been successfully using drupal7-backport-889772-114.patch on 7.39 for quite some time in several production environments and it resolves the issue for us.

@joseph.olstad Thanks for the report. Your experience matches mine.

I kinda don't think this /login thing is an issue because Drupal core gives you the login link without the /login appended. It only appears that Drush does this. user_pass_reset_url() returns a link without /login appended to it. So IMHO, the problem here is Drush. This patch is being used by several of us on many Drupal sites. In my case I am using it on a site with over 100,000 users with a couple hundred logins a day and at least two password resets every day. With this amount of use, I cannot see a reason to block this patch.

Perignon: Drush may not be the only thing doing this. Drupal core exposes /login as a legitimate URL. So it needs to work - do something, anything predictable/logical - if you visit it. #124 is buggy.

Also, #124 needs other work, its messaging is wrong - users still get the "You must log out to use the password reset link in the e-mail." message, even though they don't need to log out.

@pjcdawkins: Oh you mean the default password reset email in regards to "You must log out to use the password reset link..."? That I did not check since I don't use the standard email. That may need work, agreed!

What I am saying is core does not create a reset login link with /login at the end of it. Execute user_pass_reset_url() with drush. Example:

drush php-eval 'print_r(user_pass_reset_url(array('uid'=>1, login=>'1445446278')))'

You will get a URL that does not have /login in it. So this is my foundation of the argument that the problem resides with Drush. Drush should not be giving you a login URL with /login appended to the end of it. Drupal core does not give you a URL with /login when you call the function that creates the one-time login link. So, Drush has to be adding that (an assumption since I haven't looked at the Drush code yet).

For the record, I am using #124 on my site without bugs :-D

So I am going to go have a look at the Drush code in the next day or so to find out where the /login is coming from in the drush project code.

I'm pretty sure Drush adds the /login on purpose (wasn't that discussed above)? I don't think there's anything wrong with it doing that.

Regarding the "You must log out to use the password reset link" messaging, we probably shouldn't break translations for this (unless there's a really good reason to) since it's an end-user-facing string... If we remove an entire translatable string that's certainly OK, but since this is part of a larger paragraph I'm not sure that will work.

Ok this did not take me long. Drush is adding this to the URL. So it what @Steve Jones has said is true. This is wrong. Drush should not be doing this. It should only be added on page load of the URL to prevent the email client issue.

Here is the link to the code https://github.com/drush-ops/drush/blob/master/lib/Drush/User/UserSingle...

I submitted a change to Drush to remove the /login. From what I can tell by looking at the patch and what is happening in user.module, drush adding the /login is circumventing processing in core and it is not allowing key information to be setup in the environment (on page load).

https://github.com/drush-ops/drush/compare/master...taz77:patch-1

The reason for my passion behind this is because this patch represents is a huge improvement to D7's user experience.

It seems that Drush did this only for convenience to keep you from having to click the button to proceed to change your password. But again, Drush is adding this - not core!

Drush issue which added /login - https://www.drupal.org/node/1829596

@jonhattan Thanks for the historical reference. So it was indeed added out of convenience.

I just realized 7.41 broke this patch. Will have to re-roll.

I don't think we should break existing functionality with this. I rerolled the patch and added drupal_goto() back to user_pass_reset() after destroying the session which seems to fix the issue.

Thanks for doing that. I didn't have any time this morning to work on it. I will give it a spin sometime tonight or in the morning.

@tuutti You missed re-rolling the test file changes.

Reroll of the reroll to include the test file changes.

Awesome. #178 works for me, manual testing. Environment to help test the patch:
https://pr-1-ywpmjuxyeercg.eu.platform.sh/

(you can register an account, it will email you)

I'll give it a thumbs up. Works for me both on pjcdawkins test site and my dev site.

But I used the patch I made in #182

Here's a new patch, same as 182 but removing the message "You must log out to use the password reset link in the e-mail."

This environment has the patch installed on plain D7:
https://p-186-ywpmjuxyeercg.eu.platform.sh/
and this has the patch, with simple_pass_reset enabled:
https://p-186-spr-ywpmjuxyeercg.eu.platform.sh/
and this has the patch, with prlp enabled:
https://p-186-prlp-ywpmjuxyeercg.eu.platform.sh/

To me it seems to work with those contrib modules, but I don't have much experience with them.

(you can use 182 in those URLs to test the previous patch, if you want)

@David_Rothstein (#172)

Regarding the "You must log out to use the password reset link" messaging, we probably shouldn't break translations for this (unless there's a really good reason to) since it's an end-user-facing string... If we remove an entire translatable string that's certainly OK, but since this is part of a larger paragraph I'm not sure that will work.

Really? is that enforced by a policy on changing strings? It seems weird to me that we'd need to leave it the same - it's now dishonest to say "You must log out".

Should this still be "Needs review"?

@opdavies: the remaining question is about the string change - can we remove the 'You must log out' sentence? (i.e. difference between patches #182 and #187)

It would make no sense to keep it IMO, but it would break translations.

Hope this is breathing life again!

OK... this patch is the same as #182 but with a comment to explain why the string has been left unchanged.

@David_Rothstein, @Perignon, @tuutti et al. - any feedback? This seems pretty well tested

So sorry! Yes I was able to apply the patch without issues. I swore I updated this issue. Must have been dreaming or something.

The patch is now broken due to 7.43.

This reroll goes pretty deep. It won't apply to 7.42 either.

Rerolled the patch to 7.43. It was a clean rewind too.

This is a reroll of #192

I've cleanly applied the patch in #199.

If I log out and request a new password link, I am logged in but I'm still expected to enter my current password. Is this the expected behaviour?

This is the same for non-admin users.

(Sorry, I've fallen off the loop with this thread so I haven't kept up to date with the updates).

Yes, that is the correct behavior. It is status quo with the current behavior of Drupal. This patch is to kill the sessionof someone that is currently logged in and log them out at the same time when they follow a one-time login link. The current behavior of Drupal is that it asks for the person's current password to change the password when the follow a one-time login link if they are still under and active session. They would not have requested the link if they knew their password! :-D

Apologies. I think I was confusing "Current password" with "Confirm password".

Of course, "Confirm password" is confirming the new password that you are entering.

The "Current password" field is then shown again after submitting the form, as I'd expect.

Not sure why PHP 5.3 is the only one passing...

@Perignon: that is because the 7.x branch tests are not passing on PHP >= 5.4, so there's little point in testing patches on these php versions for now.

Ah ok. Gotcha.

1. +      watchdog('user', 'Session closed for %name.', array('%name' => $user->name));
   +
   +      module_invoke_all('user_logout', $user);
   +      // Destroy the current session, and reset $user to the anonymous user.
   +      session_destroy();
   

   Would be nicer to reuse the code in user_logout() (via a helper function if necessary) rather than repeating it.

2. +      module_invoke_all('user_logout', $user);
   

   Invoking this and then continuing on to do other things aftewards gives me some pause... this hook is usually invoked on user/logout right before a redirect, and I think some hook implementations out there assume that context.

3. +      session_destroy();
   +
   +      if ($action === 'login') {
   +        drupal_goto("user/reset/$uid/$timestamp/$hashed_pass/login");
   +      }
   

   Redirecting to the same page we're already on seems like it could use some explanation, and it may actually be a workaround for another bug. For example, the real problem may be that session_destroy() is called above when drupal_session_regenerate() should have been called instead (since the session is being switched, not ended)?

   That is, assuming we don't want to just redirect always, based on my above comment...

4. +      // The second sentence of this string is factually incorrect after issue
   +      // https://www.drupal.org/node/889772 was fixed: you do not need to log
   +      // out to use the link. The string has been left unchanged intentionally
   +      // so as not to break translations.
          '#markup' =>  t('Password reset instructions will be mailed to %email. You must log out to use the password reset link in the e-mail.', array('%email' => $user->mail)),
   

   Good idea to have some kind of code comment here, although it's maybe a little strong. If we're going to log them out anyway then it's not necessarily wrong to tell them to do it manually (it's generally preferable to have a user log themselves out rather than have the system do it for them behind the scenes). This issue just makes it so if they forget, we go ahead and do it for them (see also #150).

   By the way, regarding the policy question:

   Really? is that enforced by a policy on changing strings? It seems weird to me that we'd need to leave it the same

   Based on https://www.drupal.org/node/1527558 and the fact that this is an end-user-facing (as opposed to admin-facing) string, we shouldn't change it unless there's a really really good reason to.

Here's a different approach that does the immediate logout for everyone and addresses the other issues mentioned above also. I think it's a simpler approach overall.

I left the tests the same but the rest of the patch is changed. Please review and test this - it seems to work OK for me (including with Drush).

I'm going to lower the priority here also. It is definitely a nice improvement to fix this, but I don't think it's a major bug when the current UI tells you explicitly that you must log out before using the link - and the bug only happens for people who don't do that.

Tested it on a fresh install of Drupal locally and it performed exactly as it should. I also tested it with the PRLP module and it works correctly.

I would humbly disagree with you on the priority. If you had to answer all the support requests I had to answer you would think differently! :-D I have a site with over 150,000 accounts of people that are on average computer illiterate with a median age of 72 years old (golfers). This broken password workflow was a HUGE issue for us, averaging, at least, two support requests a week for password resets. Now we get a support request for password resets maybe once a month. As a developer, you got to remember to not beat-up your user/customer or they will go away and find something else taking their money with them. Make it as idiot proof as possible.

Any chance we can get this moving? This issue is five years and eight months old. Feels like the US Government here :-D

OK, let's call it towards the low end of major :) (I'm surprised, however, that so many people were resetting their passwords while already logged in...)

Thanks for the testing. There is probably going to be a release on April 20 and I think this should be able to get in for that. I am going to leave it for a little while before committing it, though, to leave a bit more time for additional reviews or testing (since it's a new patch).

Hey... you build something idiot proof and someone goes and makes a better idiot! Thanks.

I will see about getting some more people in here to test it. Do some advertising :-D

added the updated patch #207 in our 7.43 site. If you don't hear back from me its because it works flawlessly.

We had been using patch from #186 for a while and it was working splendidly. I'll test the one from #207 next, but I'm sure it'll be fine. Thanks for everyone's work on this!

Because the discussion has moved on and this has been RTBC for two months, I've deleted the demo Platform.sh project I used for #186.

Two months at RTBC...

April 20th has come and past. What's the issue with getting this into core? Is there something wrong with the patch?

To the comment of so many people resetting their passwords while logged in. NEVER underestimate the stupidity of the general public. Not to sound callous, but it is the truth. Build something idiot proof, and someone goes and invents a better idiot.

In general I am fine with fixing that and the patch looks good.

However I do not get why we need to log the user out to forward them to the /edit page with the right token?

Could someone, more familiar with the issue explain that?

Are you asking why we are going to kill the session and log the user out when they follow a password reset link via email?

#218: Yes, I would have imagined:

drupal_goto('user/edit', 'token=' . my_secret_pw_token());

Would be enough ...

@Fabianx: This is stated in the original issue. Specifically, #6 "Following the link takes you to the user edit form, which requires the current password if you enter a new one".

If you are requesting a new password, it is safe to assume you do not know the current password. For security, you cannot change your existing password without your existing password while you are logged into your account. So this patch kills your session and logs you out before proceeding to the change password when you follow the reset link sent via email (the token).

I have stated this previously that this is one of the biggest UX improvements to D7. I already implemented this on my site over a year ago, I cut customer support requests by 90% after implementing these patches. I would average 2 or 3 support tickets a week with frustrated users because they could not remember their password and requested to change it via email but they were already logged into the site.

Ya I agree with Perignon, this patch is a huge improvement. I've had to contact drupal site owners to ask them to apply this patch so that I could reset my password. Most people would lose patience and give up or open a new account under a different name.

I haven't heard of any problems with this patch. The wetkit distro uses it, see the make file here. .

I am not talking about the UX. I am talking about the technical implementation:

Why is it not enough to do:

if ($user->uid == $uid) {
        $token = drupal_random_key();
        $_SESSION['pass_reset_' . $user->uid] = $token;

      drupal_set_message(t('You are logged in as %user. <a href="!user_edit">Change your password.</a>', array('%user' => $user->name, '!user_edit' => url("user/$user->uid/edit", array('query' => array('pass-reset-token' => $token)))));

        drupal_goto('user/' . $user->uid . '/edit', array('query' => array('pass-reset-token' => $token)));
}

That would give the user immediate possibility to change their password either via the link or via the goto.

Obviously would want to put that code in a helper - as its copied from below.

April 20th has come and past. What's the issue with getting this into core? Is there something wrong with the patch?

Sorry, I got very busy and didn't do a release in April. I think this is still on target for the next release though.

The good news is that now that we have two new Drupal 7 core committers, I don't have to (and probably shouldn't) commit the patch which I myself worked a lot on :)

However I do not get why we need to log the user out to forward them to the /edit page with the right token?

I think the answer is that we don't, but it's done that way because that's what the Drupal 8 commit did, and also for simplicity and consistency. (See #150.)

I don't think leaving them logged in would necessarily be as simple as #222, because we'd still want to ensure that:

1. The password reset link gets invalidated after it is used (you should not be able to use it twice), and
2. The user sees a confirmation form after clicking on the link, similar to what anonymous users see (my understanding is that Drupal does that currently so as to prevent any link pre-fetchers, e.g. in email programs, from invalidating the one-time login link)

So it would be possible, but probably more complicated. Whereas logging them out puts them into the same flow as the regular (much more common) anonymous user case, which already handles all the complexities.

Thanks, for the explanation, David!

The patch is fine as is. The D8 implementation is a good argument - I did miss that it had the same problem.

Marking for commit.

Woohoo!!! Time for a party!

Commited and pushed to 7.x! Thanks @all to fix this 5 years old isssue!

Added a change record, too: https://www.drupal.org/node/2759023

Thanks a lot @fabianx and and we're delighted you're now part of the Drupal 7.x team.
We're very happy to see this patch finally getting in.
Special thanks to @Perignon for pushing this, with some amazing quotes:
"you build something idiot proof and someone [...]" :D

Not to mention all the patches and coding efforts.
Thanks all.
Cheers!