Problem/Motivation

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

Detail

Steps to reproduce:

  1. Log off if necessary
  2. Request a new password
  3. Log back in with the old password [a]
  4. On receiving the password reset email, click on that link to log on [b]
  5. You are taken to the home page of the site, with a message "You are logged in as USERNAME. [LINK]Change your password[/LINK].
  6. Following the link takes you to the user edit form, which requires the current password if you enter a new one

Note that this requires 2 steps which don't seem terribly logical on the part of the user:

[a] Why would you request a new password, and then log in?
[b] Having logged in again, why would you click the link in the email?

Possible explanations include:

* the user was on the wrong machine or the wrong browser, and then remembered to switch to something that has their password stored.
* the user simply remembered it after requesting the reset
* the user is currently logged in, but is aware that they have forgotten their password, and wishes to reset it, and therefore goes to the site in a 2nd browser to get a password reset email sent to them, because that's the only way they know to reset their password without knowing the existing one.

Proposed resolution

add this token when user follows the password reset link AND they are logged in.

Remaining tasks

  • Fix error (See #58 and #67
  • manually test (once there is a new patch)
  • add automated tests? (is this possible?)
  • review 8.0.x
  • commit 8.0.x
  • backport to d7

User interface changes

A message when click the link.

API changes

No.

Files: 
CommentFileSizeAuthor
#124 drupal7-backport-889772-114.patch8.1 KBtuutti
PASSED: [[SimpleTest]]: [MySQL] 41,500 pass(es).
[ View ]
#114 889772-114.patch5.66 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,513 pass(es).
[ View ]
#111 889772-110.patch5.87 KBtuutti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,849 pass(es).
[ View ]
#108 889772-108.patch5.87 KBtuutti
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-108.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#98 889772-98.patch5.7 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-98.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#83 interdiff-889772-76-81.txt2.77 KBstefan.r
#81 889772-81.patch5.67 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#76 889772-76.patch2.77 KBSutharsan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,039 pass(es).
[ View ]
#76 interdiff-889772-74-76.txt1.15 KBSutharsan
#74 889772-74.patch2.86 KBopdavies
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es).
[ View ]
#67 following_a_password-889772-67.patch2.44 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,902 pass(es).
[ View ]
#59 drupal-reset-password-link-logged-in-broken-889772-59-D7-more-radical.patch1009 bytesdas-peter
PASSED: [[SimpleTest]]: [MySQL] 40,911 pass(es).
[ View ]
#59 drupal-reset-password-link-logged-in-broken-889772-59-D7.patch1.02 KBdas-peter
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-reset-password-link-logged-in-broken-889772-59-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#58 interdiff-889772-54-58.txt1.81 KBopdavies
#54 following_a_password-889772-54.patch1.56 KBZerdiox
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,854 pass(es).
[ View ]
#44 889772.drupal.d8.user-password-reset-logged-in.patch1.42 KBjoachim
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,233 pass(es).
[ View ]
#42 889772.drupal.d7.user-password-reset-logged-in.patch1.26 KBjoachim
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772.drupal.d7.user-password-reset-logged-in.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

asimmonds’s picture

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?

AnalogFile’s picture

Status:Active» Closed (fixed)

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

mandclu’s picture

Version:7.0-alpha6» 7.0

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.

mandclu’s picture

Status:Closed (fixed)» Active

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

droplet’s picture

Status:Active» Postponed (maintainer needs more info)

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

mandclu’s picture

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.

droplet’s picture

Version:7.0» 7.x-dev
Status:Postponed (maintainer needs more info)» Active

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.

dwalker51’s picture

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

bastiansalmela’s picture

I have the same problem, with drupal 7.8.

.b

paulrooney’s picture

Title:A requested password reset for user/1 requires the current password» "Request new password" generates a one-time login link, but the current password is required to actually change the password

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

ronald_istos’s picture

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.

earthday47’s picture

Component:base system» user.module

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

SolomonGifford’s picture

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

malc_b’s picture

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.

Steven Jones’s picture

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

Steven Jones’s picture

Status:Active» Postponed (maintainer needs more info)
hongquan’s picture

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

hongquan’s picture

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

mrfelton’s picture

Status:Postponed (maintainer needs more info)» Active

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.

timjh’s picture

Version:7.x-dev» 7.14

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.

SolomonGifford’s picture

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

timjh’s picture

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?

SolomonGifford’s picture

Status:Active» Closed (works as designed)

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.

timjh’s picture

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.

Routh’s picture

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.

IWasBornToWin’s picture

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.

jainparidhi’s picture

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

IWasBornToWin’s picture

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>
jainparidhi’s picture

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

IWasBornToWin’s picture

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.

jainparidhi’s picture

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!

IWasBornToWin’s picture

Yes, "reset"

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

bshaddad’s picture

#13 worked for me !

Thank you

jaykaycgn’s picture

Version:7.14» 7.19
Status:Closed (works as designed)» Active

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

Road Kill’s picture

Yes same problem here.

Road Kill’s picture

#13 worked for me !

Thank you

jcmiller09’s picture

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]

malc0mn’s picture

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:

<?php
[...]
        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...

ChrisValentine’s picture

Issue summary:View changes

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

joachim’s picture

Title:"Request new password" generates a one-time login link, but the current password is required to actually change the password» following a password reset link while logged in leaves users unable to change their password
Version:7.19» 8.x-dev
Issue summary:View changes

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.

joachim’s picture

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.

joachim’s picture

StatusFileSize
new1.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772.drupal.d7.user-password-reset-logged-in.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Proof of concept patch on D7 for method B.

joachim’s picture

Issue summary:View changes
joachim’s picture

Status:Active» Needs review
StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,233 pass(es).
[ View ]

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.

joachim’s picture

Issue tags:+affects Drupal.org

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

YesCT’s picture

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.

joachim’s picture

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

mgifford’s picture

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.

joachim’s picture

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

mgifford’s picture

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.

Harry Slaughter’s picture

Still seeing this problem.

Reproduced it multiple times (without ever logging in).

Deleting all site cookies fixes it.

Seems like a pretty legit bug.

lizzjoy’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

The patch no longer applies.

Zerdiox’s picture

Issue tags:+Amsterdam2014

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

Zerdiox’s picture

StatusFileSize
new1.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,854 pass(es).
[ View ]

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

Zerdiox’s picture

Status:Needs work» Needs review

The last submitted patch, 42: 889772.drupal.d7.user-password-reset-logged-in.patch, failed testing.

opdavies’s picture

I'll give this a review.

opdavies’s picture

Status:Needs review» Needs work
StatusFileSize
new1.81 KB

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

das-peter’s picture

StatusFileSize
new1.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-reset-password-link-logged-in-broken-889772-59-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1009 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,911 pass(es).
[ View ]

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

YesCT’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll

The last submitted patch, 59: drupal-reset-password-link-logged-in-broken-889772-59-D7.patch, failed testing.

Status:Needs review» Needs work
YesCT’s picture

Version:8.0.x-dev» 7.x-dev
Issue summary:View changes
Status:Needs work» Needs review

sorry, versions tripped me up.

YesCT’s picture

retesting with those on 7.x.

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

YesCT’s picture

Version:7.x-dev» 8.0.x-dev
Issue summary:View changes
Issue tags:+Needs tests
StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,902 pass(es).
[ View ]
new2.26 KB

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.

opdavies’s picture

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?

The last submitted patch, 59: drupal-reset-password-link-logged-in-broken-889772-59-D7.patch, failed testing.

The last submitted patch, 59: drupal-reset-password-link-logged-in-broken-889772-59-D7.patch, failed testing.

opdavies’s picture

StatusFileSize
new519 bytes
new2.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,918 pass(es).
[ View ]

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

Sutharsan’s picture

StatusFileSize
new941 bytes
new2.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,034 pass(es).
[ View ]

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

opdavies’s picture

StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es).
[ View ]
new1.3 KB

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

opdavies’s picture

Sutharsan’s picture

StatusFileSize
new1.15 KB
new2.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,039 pass(es).
[ View ]

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

opdavies’s picture

That's cleaner. :)

I didn't try it that way.

YesCT’s picture

Still needs tests?

stefan.r’s picture

Issue tags:-Needs tests
StatusFileSize
new5.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,502 pass(es), 9 fail(s), and 1 exception(s).
[ View ]

Test added

Status:Needs review» Needs work

The last submitted patch, 79: 889772-79.patch, failed testing.

stefan.r’s picture

Status:Needs work» Needs review
StatusFileSize
new5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
YesCT’s picture

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.

stefan.r’s picture

StatusFileSize
new2.77 KB
new2.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to create checkout database.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 83: 889772-81-testonly.patch, failed testing.

stefan.r’s picture

Status:Needs work» Needs review
prab.hu’s picture

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.

System Lord’s picture

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

System Lord’s picture

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" : [ "" ]
        }
      }
    ]
  }
}
opdavies’s picture

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?

stefan.r’s picture

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

jhedstrom’s picture

joachim queued 83: 889772-81-testonly.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 83: 889772-81-testonly.patch, failed testing.

stefan.r’s picture

Status:Needs work» Needs review

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

hussainweb queued 81: 889772-81.patch for re-testing.

The last submitted patch, 81: 889772-81.patch, failed testing.

idebr’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll
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
hussainweb’s picture

Status:Needs work» Needs review
Issue tags:-Amsterdam2014, -Needs reroll
StatusFileSize
new5.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-98.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just rerolling the patch in #81 for now.

Status:Needs review» Needs work

The last submitted patch, 98: 889772-98.patch, failed testing.

Status:Needs work» Needs review

t0xicCode queued 98: 889772-98.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 98: 889772-98.patch, failed testing.

MJCO’s picture

Status:Needs work» Needs review

opdavies queued 98: 889772-98.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 98: 889772-98.patch, failed testing.

opdavies’s picture

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.

joelpittet’s picture

Issue tags:+Needs reroll
opdavies’s picture

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

tuutti’s picture

StatusFileSize
new5.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 889772-108.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled #98 and fixed failing tests.

tuutti’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 108: 889772-108.patch, failed testing.

tuutti’s picture

Status:Needs work» Needs review
StatusFileSize
new5.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,849 pass(es).
[ View ]
opdavies’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

#111 works for me.

xjm’s picture

Priority:Normal» Major
Status:Reviewed & tested by the community» Needs work

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.

tuutti’s picture

StatusFileSize
new5.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,513 pass(es).
[ View ]

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.

tuutti’s picture

Status:Needs work» Needs review
opdavies’s picture

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

I'll move on and perform a functional review.

opdavies’s picture

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 114: 889772-114.patch, failed testing.

Status:Needs work» Needs review

tuutti queued 114: 889772-114.patch for re-testing.

lauriii’s picture

Status:Needs review» Reviewed & tested by the community

Looks like a test bot failure

xjm’s picture

Title:following a password reset link while logged in leaves users unable to change their password» Following a password reset link while logged in leaves users unable to change their password
Status:Reviewed & tested by the community» Fixed

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.

  • xjm committed 7e70214 on 8.0.x
    Issue #889772 by stefan.r, tuutti, opdavies, Sutharsan, joachim, das-...
joachim’s picture

Version:8.0.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

Yay!

Can this be backported?

tuutti’s picture

StatusFileSize
new8.1 KB
PASSED: [[SimpleTest]]: [MySQL] 41,500 pass(es).
[ View ]

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.

tuutti’s picture

Status:Patch (to be ported)» Needs review
opdavies’s picture

Status:Needs review» Reviewed & tested by the community

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.