WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 weeks ago

#34281 assigned enhancement

Allow admins to send users a 'Reset Password' link

Reported by: Ipstenu Owned by: adamsilverstein
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: has-screenshots dev-feedback has-ux-feedback
Focuses: javascript Cc:

Description

Following #24633 and #34180

As Pippin said:

it'd be really nice for admins to be able to manually generate a new password for user accounts and then automatically send that new password via email.

As an extension of that, should we be able to easily send users a link to reset their passwords? It would greatly improve supporting communities. Instead of explaining "Go to domain.com/wp-admin/ and click on lost password and put in your info..." you can just press a button and send a link. Done.

I was able to make it a plugin but I'm epically failing at getting the ajax/js stuff to work right as a core patch :(

Screenshot: https://cloudup.com/cWNmv6T0SXI Code: https://cloudup.com/c2SpsmqXb14

This doesn't actually change the password or even force it. If you, as an admin, change the password of a user, they get an email anyway. This uses just emails them the reset link.

Attachments (6)

34281.diff (14.0 KB) - added by adamsilverstein 3 years ago.
34281.2.diff (17.7 KB) - added by adamsilverstein 3 years ago.
34281.3.diff (17.8 KB) - added by eventualo 3 years ago.
I refred the patch
34281.4.diff (18.0 KB) - added by adamsilverstein 3 years ago.
34281.5.diff (18.0 KB) - added by eventualo 3 years ago.
34281.6.diff (18.1 KB) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (57)

#1 @mordauk
3 years ago

I support this times 100000x.

#2 @knutsp
3 years ago

  • Focuses javascript added
  • Keywords needs-patch added

+1

This is really needed and was missed in the fix for the previous ticket.

As an admin we usually don't want to brutally change the password for a user. We want to have a simple way to enable our users to easily change their password themselves.

#3 @adamsilverstein
3 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#4 @adamsilverstein
3 years ago

34281.diff is a first pass. Notes:

  • Built off of code provided by @Ipstenu - thanks!
  • I'm leveraging the existing retrieve_password function to avoid duplicating code and allow existing password reset hooks to work as expected. To accomplish this I moved the function from wp-login.php to wp-includes/functions.php - otherwise the function is not available in this context. Feedback appreciated.

Things that would be nice to add:

  • The ability to trigger password resets as a bulk action from the user list screen.
  • Extending retrieve_password to accept a user_login argument. Currently, I'm setting up $_POST['user_login'] because thats what the function expects. That feels wrong, it would be cleaner if I could pass the user_login to the function directly.
  • Unit tests.

Would appreciate feedback on the UI as well, especially the success (or error) message inserted.

Screenshots:

Reset button: http://cl.ly/image/2V3X3E1J3b3V/Image%202015-10-13%20at%2011.16.28%20AM.png

After reset: http://cl.ly/image/1N430K3N1K2Q/Edit_User__sadasd__WordPress_2015-10-13_11-15-39.jpg

#5 @adamsilverstein
3 years ago

  • Keywords has-patch dev-feedback needs-testing has-screenshots added; needs-patch removed

#6 follow-up: @DrewAPicture
3 years ago

First impressions on 34281.diff:

  • Seems weird to display the notice inline like that, I'd expect some other kind of feedback, or a page refresh with the notice at the top
  • Something doesn't feel great about the button text + the setting title + the description text. Maybe "Reset Password" for the button, with description text "The user will be emailed a password reset link.", but then the setting title is redundant. Hmm.

#7 in reply to: ↑ 6 @adamsilverstein
3 years ago

Replying to DrewAPicture:

Thanks for the feedback. Not sure about the notice either. I like inline because its an ajax action not a full submission of the page/form. Maybe more along the lines of shiny updates?

Thinking I should disable the button immediately on click to avoid double clicks.

Agree on the language, open to suggestions and will try to make it more concise.

This ticket was mentioned in Slack in #core by majemedia. View the logs.


3 years ago

#9 follow-up: @Ipstenu
3 years ago

Drew the inline matches what we do for sessions. Since it's a click and done, it looks the same.

Sessions spits out "You are now logged out everywhere else." in the same success box.

The text needs better Englishing, though, yes.

Adam, the plugin did disable the button on click to prevent doubles :)

Last edited 3 years ago by Ipstenu (previous) (diff)

#10 in reply to: ↑ 9 @adamsilverstein
3 years ago

Replying to Ipstenu:

Drew the inline matches what we do for sessions. Since it's a click and done, it looks the same.

Good point, the inline message and behavior exactly matches the 'Log out everywhere else' button. Lets leave it that way and if we decide to change things, we can change both.

I'll work on the Englishing.

Heres what that 'log out' like after you click the button:

http://cl.ly/image/1W0j0a3l033C/Profile__sadasd__WordPress_2015-10-13_20-05-02.jpg

#11 @Ipstenu
3 years ago

  • Title: Reset
  • Button: Send Reset Link
  • Sub-text: Send USER a link to reset their password. This will not change their password, nor will it force a change.

Pursuant to that, we should probably have a note under 'generate new password' that says 'Generate a new password for USER. This will email them an alert that their password has been changed.' I mean if we're doing this everywhere else, it would be good there as well, no?

(nb: No you can't send yourself a password reset link from the admin panel, that's just silly.)

#12 @adamsilverstein
3 years ago

In 34281.2.diff:

  • Adjust language for buttons, confirmation
  • Adds a bulk action on the users screen (didn't test yet in multisite user screen)
  • Screenshots incoming

#13 @adamsilverstein
3 years ago

Send reset button on profile screen: http://cl.ly/image/1q0z3g2z1W2V/Edit_User__sadasd__WordPress_2015-10-18_21-50-39.jpg

After sending reset: http://cl.ly/image/371U3O3O3G3G/Edit_User__sadasd__WordPress_2015-10-18_21-52-12.jpg

Users screen, send reset link for single user: http://cl.ly/image/411O1S0i1B34/Users__sadasd__WordPress_2015-10-18_21-52-54.jpg

Users screen, confirmation after sending single user a reset: http://cl.ly/image/2D2H1H2x2t1q/Users__sadasd__WordPress_2015-10-18_21-53-54.jpg

Bulk action selection: http://cl.ly/image/3A0c34192Q0G/Monosnap_2015-10-18_21-54-49.jpg

Bulk reset result confirmation: http://cl.ly/image/2z3L241S1X3R/Users__sadasd__WordPress_2015-10-18_21-55-40.jpg

#14 @adamsilverstein
3 years ago

  • Milestone changed from Awaiting Review to 4.4

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

#16 @Thomas Vitale
3 years ago

The patch 34281.2.diff can't be applied anymore. This is what I got when I tried to apply it for testing it (using the latest available development version).

patch -p0 < 34281.2.diff
patching file src/wp-admin/admin-ajax.php
Hunk #1 FAILED at 62.
1 out of 1 hunk FAILED -- saving rejects to file src/wp-admin/admin-ajax.php.rej
patching file src/wp-admin/includes/ajax-actions.php
Hunk #1 succeeded at 3316 with fuzz 2 (offset 73 lines).
patching file src/wp-admin/includes/class-wp-users-list-table.php
Hunk #1 succeeded at 242 (offset 7 lines).
Hunk #2 succeeded at 416 (offset 7 lines).
patching file src/wp-admin/js/user-profile.js
patching file src/wp-admin/user-edit.php
patching file src/wp-admin/users.php
Hunk #1 succeeded at 192 (offset 1 line).
Hunk #2 succeeded at 473 (offset 1 line).
Hunk #3 succeeded at 513 (offset 1 line).
patching file src/wp-includes/functions.php
Hunk #1 succeeded at 5161 (offset 69 lines).
patching file src/wp-includes/script-loader.php
patching file src/wp-login.php
Reversed (or previously applied) patch detected!  Assume -R? [n]
Last edited 3 years ago by Thomas Vitale (previous) (diff)

@eventualo
3 years ago

I refred the patch

#17 follow-up: @eventualo
3 years ago

I refreshed the patch for latest version.

#18 follow-up: @wonderboymusic
3 years ago

  • Keywords needs-refresh added; dev-feedback removed

you should never set values on superglobals like $_POST - retrieve_password() should be altered to accept data passed to it, and not rely exclusively on $_POST

#19 in reply to: ↑ 18 @adamsilverstein
3 years ago

Replying to wonderboymusic:

you should never set values on superglobals like $_POST - retrieve_password() should be altered to accept data passed to it, and not rely exclusively on $_POST

Yea, that did feel wrong, thanks for the feedback! I will refresh the patch and update the function to accept data instead of relying on setting $_POST.

#20 in reply to: ↑ 17 @adamsilverstein
3 years ago

Replying to eventualo:

I refreshed the patch for latest version.

Thanks for refreshing!

#21 @adamsilverstein
3 years ago

In 34281.4.diff:

  • Extend retrieve_password to accept the user_login as an argument
  • pass user_login to retrieve_password instead of using $_POST

#22 follow-up: @eventualo
3 years ago

I tried the 34281.4.diff, but when I try to send the reset email using the Send reset link button in user profile I got a wrong message ([object Object]).

So I uploaded a new 34281.5.diff with these changes:

  • inside retrieve_password function: if $user_login is empty and $_POST['user_login'] is set, $user_login gets $_POST['user_login'] value; then, I updated the following IF statement to check only the value of $user_login.
  • inside user-profile.js file, I updated the addInlineNotice function: now the resultDiv prints html, because the output message can be html, and uses a fadeIn, useful if the user clicks the button again.
Last edited 3 years ago by eventualo (previous) (diff)

@eventualo
3 years ago

#23 in reply to: ↑ 22 ; follow-up: @adamsilverstein
3 years ago

Replying to eventualo:

Thanks for the updated patch... I will re-test.

  • inside user-profile.js file, I updated the addInlineNotice function: now the resultDiv prints html, because the output message can be html, and uses a fadeIn, useful if the user clicks the button again.

How would the response contain HTML? It should only be text unless I am missing something?

I'm concerned that using .html() opens a security hole because the response would be evaluated. If an attacker could alter the response, they could inject JavaScript into the admin page. I'd prefer to stick to text() which is safer.

Also I'm not sure fadein is appropriate, do we use that anywhere else in core?

In the previous version of the patch, we disable the button upon response, preventing duplicate clicks. That part is missing in your latest patch. I think this better matches what we are doing with the 'Log me out everywhere else' button right below this button. It might make more sense to fire right after clicking the button instead of on response.

#24 in reply to: ↑ 23 ; follow-up: @eventualo
3 years ago

Replying to adamsilverstein:

Thanks a lot @adamsilverstein for your feedback.

How would the response contain HTML? It should only be text unless I am missing something?

I'm concerned that using .html() opens a security hole because the response would be evaluated. If an attacker could alter the response, they could inject JavaScript into the admin page. I'd prefer to stick to text() which is safer.

During my tests, I got also some error messages during sending emails. An error message is in html: The email could not be sent.<br />Possible reason: your host may have disabled the mail() function. for this reason I propose the .html(), but I undestand the security issue, and I agree.

Also I'm not sure fadein is appropriate, do we use that anywhere else in core?

In the previous version of the patch, we disable the button upon response, preventing duplicate clicks. That part is missing in your latest patch. I think this better matches what we are doing with the 'Log me out everywhere else' button right below this button. It might make more sense to fire right after clicking the button instead of on response.

I didn't disable the button upon response. With my patch, the button is properly disabled only on successfully sending, as in previous patch. But, if there is an error, the button is not disabled: this behaviour happened also in previous version, so I proposed the fadeIn. In my opinion it could be a quick way to solve the issue of a always active button, after having an error and trying another chance.

Last edited 3 years ago by eventualo (previous) (diff)

#25 in reply to: ↑ 24 ; follow-up: @adamsilverstein
3 years ago

Replying to eventualo:

During my tests, I got also some error messages during sending emails. An error message is in html: The email could not be sent.<br />Possible reason: your host may have disabled the mail() function. for this reason I propose the .html(), but I undestand the security issue, and I agree.

Aha! Ok, I'll do some more testing and clarification on this and disabling the button. I think we may want to remove that html from the string. Thanks for keeping this fresh, I have a feeling its going to get punted until 4.5, I leave that up the the lead devs to decide.

#26 in reply to: ↑ 25 @eventualo
3 years ago

Replying to adamsilverstein:

Aha! Ok, I'll do some more testing and clarification on this and disabling the button. I think we may want to remove that html from the string. Thanks for keeping this fresh, I have a feeling its going to get punted until 4.5, I leave that up the the lead devs to decide.

Perfect, again thanks a lot for your code and tests.

#27 @adamsilverstein
3 years ago

In 34281.6.diff :

  • hide any previous error before submitting new request (and remove fade)
  • use a jQuery text transformation to strip HTML from returned messages, switch to .text() vs. .html()

#28 follow-up: @ocean90
3 years ago

I'm curious why this ticket is labeled as a bug, looks like an enhancement and plugin material to me.

#29 in reply to: ↑ 28 @adamsilverstein
3 years ago

  • Type changed from defect (bug) to enhancement

Replying to ocean90:

I'm curious why this ticket is labeled as a bug, looks like an enhancement and plugin material to me.

Definitely an enhancement, changing that. Could be a plugin, I agree. Lets discuss further.

#30 @knutsp
3 years ago

Pre 4.3, when a new password was requested, it was sent by email. When administrators wanted to change the password of a user, they usually emailed it.

As of 4.3 we discourage sending passwords by email by introducing a password reset link. This is about hardening WordPress security.

Still, if an admin wants to enable the user to log in after lost password, the old practice of sending a new password by email, manually, may persist.

Even if this could become a nice plugin, making the life of an administrator easier, it could also bee seen as part of a security hardening, following up the password changes in 4.3.

I think it goes along with the ability to log out of other sessions. That could also have been a plugin.

Apart from being nice and handy, I think it's an important enhancement.

#31 @ocean90
3 years ago

  • Milestone changed from 4.4 to Future Release

The slot for 4.4 enhancements was closed on Oct 22th.

#32 follow-up: @ericlewis
3 years ago

@adamsilverstein anything else we need here, or ready for review?

#33 in reply to: ↑ 32 @adamsilverstein
3 years ago

Replying to ericlewis:

@adamsilverstein anything else we need here, or ready for review?

Generally ready for review, I am also open to reworking as a plugin.

#34 follow-up: @ericlewis
3 years ago

  • Milestone changed from Future Release to 4.5

Patch applies to trunk for review.

I don't think it needs to be in the user list table row and bulk actions dropdown. Leaving this in the user profile next to password feels like the right context.

@helen @michaelarestad @melchoyce any feedback on the flow?

#35 @DrewAPicture
3 years ago

  • Keywords ux-feedback added; has-patch needs-testing needs-refresh removed

Seems like some user testing could provide valuable data on the best approach here.

This ticket was mentioned in Slack in #design by melchoyce. View the logs.


2 years ago

#37 follow-up: @afercia
2 years ago

  • Focuses accessibility added

Thinking when it's AJAX it would be nice to send success/failure messages to wp.a11y.speak() :)

#38 in reply to: ↑ 37 @adamsilverstein
2 years ago

  • Keywords dev-feedback added

Replying to afercia:

Thinking when it's AJAX it would be nice to send success/failure messages to wp.a11y.speak() :)

Good point, thank you: I can add that. I'm still awaiting feedback on whether this belongs in core or should be offered as a plugin.

#39 in reply to: ↑ 34 @melchoyce
2 years ago

Replying to ericlewis:

Patch applies to trunk for review.

I don't think it needs to be in the user list table row and bulk actions dropdown. Leaving this in the user profile next to password feels like the right context.

@helen @michaelarestad @melchoyce any feedback on the flow?

The flow seems pretty straight-forward. What does the password reset email look like?

#40 @Ipstenu
2 years ago

It sends the default password reset email. The one that starts "Someone requested that the password be reset for the following account:..."

This ticket was mentioned in Slack in #core by jorbin. View the logs.


2 years ago

#42 @jorbin
2 years ago

  • Milestone changed from 4.5 to Future Release

There hasn't been any recent activity on this ticket and the beta 1 deadline for 4.5 is tomorrow, so punting.

#43 @jazbek
2 years ago

Not sure how useful this is, but I wanted to share that there is already a plugin that has pretty similar functionality to what's being requested here. I tried it last night and it needs to be updated to work with WP 4.3+ so I might do that, since I really need this functionality:

https://github.com/atwellpub/resend-welcome-email

Alternatively, if any of the devs here want to turn their patches attached to this ticket into a plugin, I'd be happy to help.

#44 @knutsp
19 months ago

#38915 was marked as a duplicate.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


17 months ago

#46 @afercia
17 months ago

  • Focuses accessibility removed

Removing this from the accessibility focus to clean up the Trac reports, as the only potential a11y improvement here would be implementing audible feedbacks via wp.a11y.speak().

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


5 months ago

#48 @JoshuaWold
5 months ago

Just took a look over the UX solution provided for the profile screen. I think this works well and also like the idea of the bulk edit option! Could personally benefit from from this being added :)

#49 @JoshuaWold
5 months ago

  • Keywords has-ux-feedback added; ux-feedback removed

#50 @adamsilverstein
5 months ago

  • Milestone changed from Future Release to 5.0

#51 @dainismichel
3 weeks ago

apparently some ppl were using this plugin https://wordpress.org/plugins/re-send-welcome-email/ to accomplish "admin can send specific users a password reset link," but that plugin is now dated.

how is interest and development looking for this feature?

best, dainis

Note: See TracTickets for help on using tickets.