Problem/Motivation

When useres register for an account and follow their one-time login link, or requests a one-time-link because they forgot their password, the resulting user/edit form does not require them to choose a new password. If by chance they miss that part of the form (which happens quite often as it's not highlighted), they're left with the inital random password.

To make matters worse, when they return to the user account page to change their password later on, they are prompted to enter their OLD password - the one they have forgotten. This creates a terrible user experience for users, and a headache for site admins.

Proposed resolution

It would make for a better user-experience all around if the prompt to reset your password were on the one-time login page.

Remaining tasks

simpletests.

User interface changes

None.

API changes

user_pass_reset():

  • moved the second phase (the form response) to user_pass_reset_submit().
  • Clarified the status message for already used links. Expired links are caught by the first if, so let's be specific here: In the end, a user needs to know if there are signs that a 3rd person could have used the link and taken over the user's account.
  • The status message for logged-in users: "You have already used this one-time login link. It is not necessary to use this link to login anymore. You are already logged in." is misleading. While it is true that it is not necessary to use this link, first of all it is also true that it is not possible anymore. Changed this to "You have already used this one-time login link, therefore you need to login with your password. In this case this is not necessary because you are already logged in."

user_pass_reset_submit():

  • To save the new password I'm straightly using user_save() unlike in patch #4 where a custom query was used. True, that user_save is a bit behemoth. But we need to do all checks, call all hooks, allow for alternate password hashing schemes etc. Certainly we don't want to reimplement or copy/paste that code. In the end using user_save is no problem because resetting the password is no performance-critical task.

user_pass():
The button title "E-mail new password" has been incorrect for quite a while. We're not sending passwords, but one-time login links. I changed the button title to "Request password reset" which is generic enough not to be incorrect, yet easy to understand.
Also, a description was added to the textfield.

user_pass_submit():

  • The message "Further instructions have been sent to your e-mail address." misses out the one-time login link and therefore leaves the user unclear about whether this means his action was successful or not. Changed to: "A one-time login link and further instructions have been sent to your e-mail address."
  • The redirect to the login page is confusing because logging in is exactly what the user can't do at the moment. Rather we should leave the user where s/he is. S/he anyway needs to check mail, first.

_user_mail_text():

  • Changed the e-mail text so it reflects our changes.
  • Maybe we can improve on some of the messages.

Also I wasn't sure if hook_user_login (invoked by user_authenticate_finalize) should be provided both password and username, so modules hooking in there can make use of the information (at that point there is not enough documentation about the hooks available). As hook_user_login currently doesn't get any useful information, going with what we have should be okay. If someone knows more about this aspect, I'd appreciate some info though.

Original report by joshk

When users register for an account and follow their one-time login link, or requests a one-time-link because they forgot their password, the resulting user/edit form does not require them to choose a new password. If by chance they miss that part of the form (which happens quite often as it's not highlighted), they're left with the inital random password.

It would make for a better user-experience all around if it were required that newly registered users were required to choose a password which they can remember. This would mean:

1) Making the password fields required for the one-time-login form
2) Give the password section of the form a very light weight to put it at the top

I'll create a patch for this unless there's a compelling reason not to.

CommentFileSizeAuthor
#72 core-reset_password_form-75924-72-do-not-test.patch6.94 KBjenlampton
#69 core-reset_password_form-75924-69-do-not-test.patch6.72 KBjenlampton
#66 drupal-75924-reset-password-form-66-do-not-test.patch6.71 KBjenlampton
#49 Screen shot 2012-11-14 at 9.42.01 AM.png54.21 KBhefox
#49 Screen shot 2012-11-14 at 9.42.19 AM.png49.32 KBhefox
#49 Screen shot 2012-11-14 at 9.43.28 AM.png64.2 KBhefox
#46 drupal_password_reset-75924-46.patch7.49 KBhefox
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_password_reset-75924-46.patch. Unable to apply patch. See the log in the details link for more information. View
#44 drupal_password_reset-75924-44.patch5.64 KBhefox
PASSED: [[SimpleTest]]: [MySQL] 47,888 pass(es). View
#40 drupal-75924-reset-password-form-40.patch6.76 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 35,699 pass(es), 16 fail(s), and 2 exception(s). View
#36 create.png30.78 KBJon Pugh
#36 reset.png31.42 KBJon Pugh
#36 password_fields_on_reset_form-75924-36.patch5.25 KBJon Pugh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch password_fields_on_reset_form-75924-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#32 password_fields_on_reset_form-75924-32.patch4.67 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 34,085 pass(es), 16 fail(s), and 2 exception(es). View
#32 password_fields_on_reset_form-75924-32-D7.patch4.65 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch password_fields_on_reset_form-75924-32-D7.patch. Unable to apply patch. See the log in the details link for more information. View
#27 require_new_pass_d6.16.patch5.49 KBtmetzger
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_d6.16.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#20 require_new_pass_d6.15.patch6.54 KBAlan.Guggenheim
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in require_new_pass_d6.15.patch. View
#13 require_new_pass_2.patch8.46 KBPancho
Failed: 7644 passes, 13 fails, 1 exception View
#15 require_new_pass_3.patch8.34 KBPancho
Failed: 7644 passes, 13 fails, 1 exception View
#14 require_new_pass_d6.patch8.06 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_d6.patch. Unable to apply patch. See the log in the details link for more information. View
#4 require_new_pass_1.patch4.61 KBjoshk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#3 require_new_pass_0.patch2.93 KBjoshk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#1 require_new_pass.patch1.49 KBjoshk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joshk’s picture

Status: Active » Needs review
FileSize
1.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Simple patch to implement this functionality. I add 'passreset' as a query in the drupal_goto() that fires after the one-time "Log In" button is pressed, which adds the #required attribute on the user//edit form.

I think this is a good user-experience improvement, as some users will otherwise use one-time login links, login, and then fail to set their password to something they can (hopefully) remember.

drumm’s picture

It would be nice if the password field were actually on the password reset page.

joshk’s picture

FileSize
2.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Patch attached does that. It adds password fields to the one-time-login form (formerly just a login button). If they match, the password is stored. If not, the same form refreshes with form_set_error. Also made some changes to the message on the successful login screen to reflect that the user already changed his/her password, and to remind him/her to make a note of the new pass.

joshk’s picture

FileSize
4.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

This version creates a user_pass_reset_submit() function to handle successful submissions. Note, may be duplicate functionality of a patch from Moshe.

killes@www.drop.org’s picture

Version: 4.7.2 » x.y.z

Features go into cvs.

dmitrig01’s picture

Version: x.y.z » 6.x-dev
Status: Needs review » Needs work

love the idea, but it's do for a re-roll

Pancho’s picture

Good idea. Moving to D7 queue, though.

cburschka’s picture

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

You may have meant to, but you didn't actually do so. So here.

Pancho’s picture

Right :) Thanks...

goatvirus’s picture

ok, i'm confused. is there a corresponding version 6.x patch to accomplish this? if so, where?

thanks!
Fish

StevenPatz’s picture

No. Things are fixed in the 7.x branch first and then (possibly) backported.

Pancho’s picture

Assigned: Unassigned » Pancho
Category: feature » bug

I'm working on a fix against HEAD. Close to done.
Actually this is a usability bug.

Pancho’s picture

FileSize
8.46 KB
Failed: 7644 passes, 13 fails, 1 exception View

Here's the patch against HEAD.

The central change: the user is now required to set a new password right after using the one-time login link. This effectively prevents hopping away from the user/edit page without actually changing the password. Also, the one-time login link is only invalidated after the new password has been set (except it expires).

Rather minor changes:

user_pass_reset():
Like joshk did before, I moved the second phase (the form response) to user_pass_reset_submit().
Clarified the status message for already used links. Expired links are caught by the first if, so let's be specific here: In the end, a user needs to know if there are signs that a 3rd person could have used the link and taken over the user's account.
The status message for logged-in users: "You have already used this one-time login link. It is not necessary to use this link to login anymore. You are already logged in." is misleading. While it is true that it is not necessary to use this link, first of all it is also true that it is not possible anymore. Changed this to "You have already used this one-time login link, therefore you need to login with your password. In this case this is not necessary because you are already logged in."
user_pass_reset_submit():
To save the new password I'm straightly using user_save() unlike in patch #4 where a custom query was used. True, that user_save is a bit behemoth. But we need to do all checks, call all hooks, allow for alternate password hashing schemes etc. Certainly we don't want to reimplement or copy/paste that code. In the end using user_save is no problem because resetting the password is no performance-critical task.
user_pass():
The button title "E-mail new password" has been incorrect for quite a while. We're not sending passwords, but one-time login links. I changed the button title to "Request password reset" which is generic enough not to be incorrect, yet easy to understand.
Also, a description was added to the textfield.
user_pass_submit():
The message "Further instructions have been sent to your e-mail address." misses out the one-time login link and therefore leaves the user unclear about whether this means his action was successful or not. Changed to: "A one-time login link and further instructions have been sent to your e-mail address."
The redirect to the login page is confusing because logging in is exactly what the user can't do at the moment. Rather we should leave the user where s/he is. S/he anyway needs to check mail, first.
_user_mail_text():
Changed the e-mail text so it reflects our changes.

Maybe we can improve on some of the messages.

Also I wasn't sure if hook_user_login (invoked by user_authenticate_finalize) should be provided both password and username, so modules hooking in there can make use of the information (at that point there is not enough documentation about the hooks available). As hook_user_login currently doesn't get any useful information, going with what we have should be okay. If someone knows more about this aspect, I'd appreciate some info though.

Otherwise, all that is left to be done, is taking care of the simpletests. I'll do so asap.

Pancho’s picture

Status: Needs work » Needs review
FileSize
8.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_d6.patch. Unable to apply patch. See the log in the details link for more information. View

@goatvirus:
Here is also an untested D6 version of the patch. Please test, and give me some feedback. Possibly we can get the backport committed to D6.

Pancho’s picture

FileSize
8.34 KB
Failed: 7644 passes, 13 fails, 1 exception View

Some codestyle.

goatvirus’s picture

Pancho - I have tested the patch in #14, against Drupal 6.5... I haven't had time to verify every little text change, but the basic functionality (ie: making sure that the user MUST change the password, when using a one-time only login link) seems fine. Nice work!

cheers
Fish
http://goatvirus.com/resume

Status: Needs review » Needs work

The last submitted patch failed testing.

Bodo Maass’s picture

Isn't it better to pass form values as 'value' instead of 'hidden' ?
In the patch from #15, the uid is passed as
$form['uid'] = array('#type' => 'hidden', '#value' => $account->uid);

According to the Forms API docs, this would better be the following, because then the uid would not be sent to the browser:
$form['uid'] = array('#type' => 'value', '#value' => $account->uid);

mr.baileys’s picture

Alan.Guggenheim’s picture

Version: 7.x-dev » 6.15
FileSize
6.54 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in require_new_pass_d6.15.patch. View

Thanks for all the contributions.
I am migrating a 5000+ users site from WordPress to Drupal and CiviCRM, and the login/password issues have been a real BIG problem. Most people get their one time login, maybe even type anew password in the fields, but, because the form is so long, they do not go to the bottom, and do not save the new password. They are logged-in, so they can post and do whatever they need, then next time when they try to login it does not work!
I applied the user_passs_reset_force_0 patch and tried the password_reset_patch545230_06, but that does not address all the issues.
So I used #14 as a base and patched my files. here is the diff for you guys to check and try to get a real patch out for 6.15.

At least my site is working now.
Thanks
Alan

Alan.Guggenheim’s picture

Usability issues:
I really think that some of the wording of messages (dispalyed on the screen) should be easily changeable by the admin (parameters, not hard coded).
For some sites, User login, password reset,... is so critical, that it needs to be fine tuned to the audience...
One thing I would like to change is the message on the screen people get when they click on the one time link. I am currently using Logintobogan, so it says:
"You have successfully validated your e-mail address"
and the below Reset password with two fields and the help: Please enter the new password in both fields.
Apparently for my users, this is not clear enough. They get confused and think they are back to the same screen requesting a new password!
Anyway, can we change it to something like:
"You have successfully validated your e-mail address. Please now enter your new password (twice) in the fields below and then click on the Log In button"
or something like that?
Thanks

Alan.Guggenheim’s picture

Version: 6.15 » 6.16
Priority: Normal » Critical

I upgraded to 6.16 and the issue is still there. It seems incredible that after several years, the new user registration/login and password reset features are still so arcane and too hard to use for 90% of the people!
On 6.16, when a new user registers, he gets an email that says:
Thank you for registering at . You may now log in by clicking on this link or copying and pasting it in your browser:
http:///user/validate/5873/1268094884/99a2e7...
This is a one-time login, so it can be used only once. After logging in, you will be redirected to http:///user/5873/edit so you can change your password.
This is actually wrong, as he does not get re-directed.
He gets a message saying:
•Login successful.
•You have just used your one-time login link. It is no longer possible to use this link to login. Please change your password.

And there is no indication on how to change the password!!!

StevenPatz’s picture

Version: 6.16 » 8.x-dev

Most likely too late for the 6.x series. And 7.x is about to be released, so moving to 8.x development branch,

Alan.Guggenheim’s picture

Wow! for an issue that is impacting a lot of people, and has been active for 4 years, having to wait an other 2 years?

StevenPatz’s picture

I feel your pain. I'm still waiting for issues in the 4.6 series to be fixed.

goatvirus’s picture

re: #17... Any details on why that patch (in #15) failed, or what has been done since? Maybe my testing wasn't as rigorous as yours, but I found that the patch worked and helped A LOT with readability/usability of the Password Reset feature.

If any of you are really needing a fix for this NOW, and not willing to wait for Drupal 8, I would suggest trying to implement that patch. Though you may have to do it manually if you are on any version higher than Drupal 6.5...

cheers
Fish
http://earthangelconsulting.com

tmetzger’s picture

FileSize
5.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch require_new_pass_d6.16.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Here's one for d16.

Damien Tournoud’s picture

Status: Needs work » Closed (won't fix)

I don't see anything that cannot be implemented as a contrib module here.

http://drupal.org/project/password_policy seems to implement a big part of it.

catch’s picture

Priority: Critical » Major
Pancho’s picture

Status: Closed (won't fix) » Needs work

Damien: That might be true, however bugs are nothing we leave to be fixed by contrib.
Preparing an updated patch against HEAD.

quicksketch’s picture

Title: Password Reset Doesn't Require Password to be Reset » Include fields for resetting password on the one-time password reset page
Category: bug » feature

This is a great usability improvement and I'd love to see it implemented. However it's not actually a bug as core had never intended to work this way. If the discussion in #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 is implemented (and it looks like it will), major and critical bugs will actually prevent any new development of Drupal core, so I'm re-categorizing issues to their proper levels/categories. See http://drupal.org/node/45111 for an extensive description of priorities.

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs backport to 6.x
FileSize
4.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch password_fields_on_reset_form-75924-32-D7.patch. Unable to apply patch. See the log in the details link for more information. View
4.67 KB
FAILED: [[SimpleTest]]: [MySQL] 34,085 pass(es), 16 fail(s), and 2 exception(es). View

This is a re-work of a patch from #138805: Security/usabillity: require password change after one-time login

I'm going to need some help with the tests. Chx had some good ones at http://drupal.org/node/138805#comment-1903692

dww’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs backport to 6.x

bjaspan, myself and chx spent a bunch of effort on something like this over at #138805: Security/usabillity: require password change after one-time login which was then casually postponed and eventually forgotten. That issue was recently marked duplicate with this. Just letting folks know there's some good discussion and patches over there if anyone's interested...

Cheers,
-Derek

dww’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs backport to 6.x

argh, sorry...

Status: Needs review » Needs work

The last submitted patch, password_fields_on_reset_form-75924-32.patch, failed testing.

Jon Pugh’s picture

FileSize
5.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch password_fields_on_reset_form-75924-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
31.42 KB
30.78 KB

I've been dying to take a stab at cleaning up these pages. So here's another patch that addresses a couple of things:

  • When you submit the form, I see three messages:

    You have just used your one-time login link. It is no longer possible to use this link to login. Removed in patch
    Password changed. Made more friendly in patch
    You are logged in as admin. Change your password. Couldn't figure out how to determine showing this or not. $form_state['submitted'] didn't seem to change here.

  • The wording of "one time login link" is now a bit confusing. Really, this is now a "ResetPassword" form, or in the case of a new user, it is a "Create Password" form. The patch changes all mentions of "one-time login link" to "reset password link" checks $account->login and changes the wording to "Create Password" if they are brand new.
  • In that vein, is is really necessary to have all the wording: "This is a one-time login for admin and will expire on Sat, 12/31/2011 - 10:28. This form can be used only once."? Usability wise, this is confusing and unnecessary. The expiration date of the form is irrelevant to the viewing user, and in fact, the form can be used more than once if you "request a new password" again. We still explain that a link has expired if it has expired. Why burden the user with these details for such a targeted task?
  • This page is called "Reset Password", while the links and tab to the form for forgotten password say "Request new Password". It should be consistent, I prefer "Reset Password" since you are given a chance to reset it, you are not actually "requesting" a new password with this thing, however that should be another issue.
  • I think the username should be more prominent. It can be very hard to see and the user should know perfectly clear the account they are resetting the password for. I made the username an #item, and, just in case, added a "Not You?" link.

Attached are the patch, and screenshots for any people interested in quickly reviewing the usability of this.

Thoughts?

Jon Pugh’s picture

Status: Needs work » Needs review

Needs review

Status: Needs review » Needs work

The last submitted patch, password_fields_on_reset_form-75924-36.patch, failed testing.

timhilliard’s picture

While I agree that this would be awesome change to make, I think that any change we do make needs to be compatible with drush uli or at least provide an alternative method for drush uli to work. As a system administrator it is sometimes important to be able to log in as other users but I don't want to have to know the other users password or change it. I also use this method more often than not to log in as user 1 rather than using a username and password.

Steven Jones’s picture

Assigned: Pancho » Unassigned
Status: Needs work » Needs review
FileSize
6.76 KB
FAILED: [[SimpleTest]]: [MySQL] 35,699 pass(es), 16 fail(s), and 2 exception(s). View

Seems that the patch in #36 is actually a patch on top of the patch in #32 so here's a re-roll of 36, that applies directly to 8.x.

Given the UI changes alone, I'm not sure this change could even be considered for backport to D7, let alone D6?

Status: Needs review » Needs work

The last submitted patch, drupal-75924-reset-password-form-40.patch, failed testing.

David_Rothstein’s picture

Issue tags: -Needs backport to 6.x

Is the title of this issue correct? It sounds like it's more about making fields that are already there required (and/or moving them to a new form), rather than adding new fields...

Also, like @timhilliard said, there are use cases for one-time logins where the user does not want to actually change their password: drush uli, as he mentioned, and another would be where you (or perhaps your five-year-old sitting at the keyboard) mistypes your password a bunch of times and triggers the flood control; at that point, you can still use the one-time login link to log in without waiting for the flood control time limit to expire, but that doesn't mean you actually want to set a new password on your account. So whatever we do here should probably find a way to preserve those use cases. (Maybe "request new password" and "request a one-time login link" should actually be two separate actions?)

And yeah, like Steven Jones said, I don't see how a feature like this could ever be backported :) Removing those tags...

hefox’s picture

On a d6 install how I have it working is that the password fields show on on password reset form, but they are not required, so user may set their password there if desired.

hefox’s picture

FileSize
5.64 KB
PASSED: [[SimpleTest]]: [MySQL] 47,888 pass(es). View

Here's a middle ground patch, that allows the one time login link to work exactly as it worked before but with password field to set if desired.

Removed user_password_reset being a form callback and put the form logic in a simple form callback and that user_password_reset calls.

hefox’s picture

Status: Needs work » Needs review
hefox’s picture

FileSize
7.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_password_reset-75924-46.patch. Unable to apply patch. See the log in the details link for more information. View

Adding some tests.

User password reset tests don't actually test logging in currently, so added that + change password.

I tried reusing the same user via $account = user_load($account->uid) but even with TRUE, to reset, the url generated was invalid (due to outdated $account->login likely), so not sure what was up with that and didn't feel like digging into it, so changed it to create a new user each test :-/

Rob C’s picture

Interesting idea. I've tested #46 and it seems to work. (also together with #115801 and that works just fine)

I'm working on #115801: Allow password on registration without disabling e-mail verification that's very much related to these developments. It offers a checkbox on the 'admin/config/people/accounts' page to enable / disable the functionality provided by the patch i'm working on. Seems the feature in this issue does not offer a config option? (yet?)

And will setting a password on the reset form be required later on? That would really finish it for the scenario where i want to use this. (i can now just click on login and i'm in, like it normally does.)

YesCT’s picture

updating issue summary.

@hefox and @Rob C, please double check the remaining tasks.

some tests were added. are more needed?

screenshots and steps to manually test might help answer #47 about if there is a config option.

needs an interdiff and also a test only patch.

hefox’s picture

Patch currently doesn't have a config option -- I'm not of the opinion that core needs to provide a config option for this.

As far as tests go, not sure -- I believe I found the the current password reset tests are a bit lacking as they don't test using the password reset page (which the patch adds, both variants).

I am confused about having a failing test -- this is a feature request, not a bug, so the purpose of a patch only is to show the feature doesn't exist?

Screenshots attached of the areas the current patch visually changes (and one it doesn't).

To test #47
1) Create an account
2) log out
3) request password email
4) visit page
Pass: password reset fields appear, text makes sense
5) Change password to two different passwords
Pass: validation error
6) Change password to two same passwords and login in
Pass: Logged in
7) Logout, login with new password
Pass: logged in
8) Logout and request new password again
9) login w/o changing password
Pass: logged in
10) logout and use exiting password to login
Pass: logged in

Rob C’s picture

@hefox

And what if we want to have #115801: Allow password on registration without disabling e-mail verification and the current feature request in core? (this issue)

Then you are able to set a password Before and During registration/reset. (set with 1 or 2 config options)
(the idea is that #115801 wouldn't work if we apply #75924 without an option, because then people need to set a password twice (if both patches are applied).)

(willing to work on providing a proposal-patch to create the options.)

hefox’s picture

My patch (47) doesn't require setting password reset -- if password fields are left blank, it retains the current functionality of logging in the user and not changing their password.

Rob C’s picture

Ok, got it, thanks for the quick feedback.
But you will still see them and no way to customize anything, not a huge deal, but something to remember.
(just scouting the issue queue a bit, looking into what's possible / what's not.)

Nephele’s picture

FYI, there is a separate issue, #1628120: New user, with email validation, can finish registration with no password, that is being used for developing a minimal fix for d7/d8 in response to the original bug, as opposed to refactoring the whole password-reset system.

Nephele’s picture

Shai’s picture

Adding to Nephele's list, please note the Password Reset Landing Page project created by Jitesh Doshi

It's new and needs more work, but it creates a password reset only form which people get to when they click the one-time log-in link in their emails. It's for Drupal 7 and it works right now.

Shai Gluskin

YesCT’s picture

@hefox Ah yes, if this is adding new functionality, then dont need to show failing tests, as you say, failing tests would only show the feature did not already exist.

I missed that screenshots were added in #49. Personally, I'm a fan of embeded screenshots (dreditor is helpful to do that) as it makes it easier to scan an issue and get quick reviews.

Steps to test added in #49 are very helpful. Those could be added to the issue summary.

the issue summary likely needs updating to make reviews easier to get. The screenshots of before and after could be added there too.

I'll resend the latest patch to the testbot, and review it.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update, +Needs screenshots

The last submitted patch, drupal_password_reset-75924-46.patch, failed testing.

YesCT’s picture

Issue tags: -Needs tests
+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -35,6 +35,28 @@ function testUserPasswordReset() {
+    $account = $this->drupalCreateUser();
+    $new_password = user_password();
+    $edit = array(
+      'pass[pass1]' => $new_password,
+      'pass[pass2]' => $new_password,
+    );
+    $this->drupalPost($user_pass_reset_url($account), $edit, t('Log in'));
+    $this->assertText(t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Your password has been updated.'), 'One time login with password reset completed.');
+    $account = user_load($account->uid, TRUE);
+    $this->assertTrue(user_check_password($new_password, $account), 'Password reset successful.');

this section could use a comment to show how it's different from the test right before it.

+++ b/core/modules/user/user.pages.incundefined
@@ -95,7 +95,7 @@ function user_pass_submit($form, &$form_state) {
  * Menu callback; process one time login link and redirects to the user page on success.
  */
-function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $action = NULL) {
+function user_pass_reset($uid, $timestamp, $hashed_pass, $action = NULL) {

I think the doc block for this might need to be updated to agree with the standards in http://drupal.org/node/1354#functions

specifically:

callback
After the summary, the next paragraph should indicate what function the callback is passed to, and which Drupal function makes use of it (which could be a list of functions). Example:

/**
 * [standard first line]
 *
 * Callback for uasort() within system_themes_page().

And I think @params

+++ b/core/modules/user/user.pages.incundefined
@@ -168,6 +147,55 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+ * One time login form to log in the user.
+ */
+function user_pass_reset_form($form, &$form_state, $account, $timestamp, $timeout) {

see the section on Form-generating functions

in http://drupal.org/node/1354#functions

Specifically, use the sentence structure that begins like: Form constructor for the user login form.

@params
@ingroup

And the "notes" to:

Notes:
Omit @param and @return documentation for the standard parameters and return value (such as $form and $form_state.
Document parameters specific to this form constructor.

+++ b/core/modules/user/user.pages.incundefined
@@ -168,6 +147,55 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+    '#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.</p>', array('%user_name' => $account->name, '%expiration_date' => format_date($timestamp + $timeout))),
...
+    '#markup' => '<p>' . t('This login can be used only once.') . '</p>'

I dont know if html like the paragraph tags should be in the t() or not, but these two lines take opposite approaches.

The api on t() does not say anything about markup. http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... I looked at a few examples, linked from the api and didn't see anything instructive.

+++ b/core/modules/user/user.pages.incundefined
@@ -168,6 +147,55 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+ * Logs in the user and changes the password if provided.
+ */
+function user_pass_reset_form_submit($form, &$form_state) {

similar to the form function above, the sumbit handler should be like:

/**
 * Form submission handler for user_login_form().
 *
 * @see user_login_form_validate()
 */
function user_login_form_submit($form, &$form_state) {
+++ b/core/modules/user/user.pages.incundefined
@@ -168,6 +147,55 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+  // user_login_finalize() also updates the login timestamp of the
+  // user, which invalidates further use of the one-time login link.
+  user_login_finalize();

finalize is being repurposed here? to invalidate, but also does other stuff? Sounds ok...

these comment lines could be wrapped to 80 chars (not critical)

+++ b/core/modules/user/user.pages.incundefined
@@ -168,6 +147,55 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+    drupal_goto();

drupal_goto was used in the code that this is replacing so seems reasonable.

Other coding standards look pretty good.
@params are a core gate,
http://drupal.org/core-gates#documentation
so needs work for those.

tests were added, removing needs tests tag

yannickoo’s picture

Issue tags: -Needs screenshots

Tagging

yannickoo’s picture

Issue summary: View changes

Updated issue summary with remaining tasks to make it easier for new people

PavanL’s picture

Issue summary: View changes
jenlampton’s picture

Issue summary: View changes
jenlampton’s picture

Issue summary: View changes
jenlampton’s picture

Patch from #40 is still a huge improvement for D7 sites.

@timhilliard I don't think we should leave the UX broken because we have existing CLI tools that depend on it being that way. It should be the other way around... first we fix the UX. Then we build CLI tools that let us do the things we need.

Les Lim’s picture

Related issue/patch converting the user_pass_reset() procedural logic into a Controller and Form object: #1998198: Convert user_pass_reset to a new-style Form object

jenlampton’s picture

Here's a re-roll of the approach in #40 against 7.26

jenlampton’s picture

most recent patch still applies cleanly to 7.32

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

jenlampton’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jenlampton’s picture

Patch rerolled for 7.50

Manjit.Singh’s picture

Status: Needs work » Needs review

The last submitted patch, 66: drupal-75924-reset-password-form-66-do-not-test.patch, failed testing.

The last submitted patch, 69: core-reset_password_form-75924-69-do-not-test.patch, failed testing.

jenlampton’s picture

Patch applies to 7.52

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jenlampton’s picture

Patch still applies cleanly to Drupal 7.56.

cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs usability review, +Usability

Cool - let's get a Drupal 8 patch done. I don't see that this has had a usability review.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Issue tags: -Needs usability review

Looking at this issue on the Drupal Usability meeting today. This was at the top of the Needs Usability Review queue (oldest highest priority issue :) We remembered we looked at this issue about #1259892: Users could not find the Change password fields recently as well, which would improve the underlying user form but not enough to solve this problem obviously. We also looked at how others do password reset, including Facebook, Twitter, Google, Apple.

I believe our official policy still is to add new features to Drupal 8 first, but we wanted to look at the usability aspect first, since the tag was added to avoid coding if the usability team finds it problematic anyway.

1. We found it problematic to change the behavior of password reset. At least we know experts use the feature to not need to remember passwords, so they don't know their old password but not interested in changing it either. There is no harm done to present this password change form for a user already logged in, that is how Drupal already works.

2. The dedicated password reset form looks like a definite improvement for this process, we would cut down considerably on the words on the form and mark the fields required. The button would be "Save password" or something along those lines since you would already be logged in. We would either provide a way to cancel out this action with a "Cancel" link along the submit button or some other way.

benjifisher’s picture

I see a lot of screenshots attached to this issue, but none of them are shown in the issue summary. I was about to add the "Needs issue summary update" tag, but I see it is already there. It would help reviewers a lot to show the current and proposed screens, in D7 and D8.

Another way to phrase (1) from Comment 81: -1 for requiring the user to set a new password after using the one-time link.

I say +1 for skipping the current page where all you can do is click the login button. The login link provided by drush already skips that page.

After using the one-time link, I want to be logged in to the site. Please do not take away my admin menu! If I can navigate normally, then I do not need a dedicated Cancel button. I think these requests are consistent with having a simplified form to set a new password.

If I remember correctly, the way it currently works is that the one-time link contains a token. When I click the Login button, a new token is generated for use on the user-edit form. It seems simpler to skip that login page and just use one token. Am I missing anything? That is, is there some extra security in having two different tokens? If so, then we should still use two tokens even if we skip the login page.

I wonder if this issue is easier to implement in Drupal 8 than Drupal 7. Can a big part of the work be done by adding an additional form mode for user entities?

Gábor Hojtsy’s picture

@benjifisher:

After using the one-time link, I want to be logged in to the site. Please do not take away my admin menu! If I can navigate normally, then I do not need a dedicated Cancel button.

Let's keep in mind that one time login link are not for admins only :) While you know you can navigate away from the form, someone who is less tech savvy would not know if that is going to break something *unless* there is an explicit action on the form to cancel it, in which case they would have an indication that it is safe to not change the password.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.