Make WordPress Core

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#34281 closed task (blessed) (fixed)

Allow admins to send users a 'Reset Password' link

Reported by: ipstenu's profile Ipstenu Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: has-screenshots has-ux-feedback has-patch has-dev-note
Focuses: javascript, privacy 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 (16)

34281.diff (14.0 KB) - added by adamsilverstein 8 years ago.
34281.2.diff (17.7 KB) - added by adamsilverstein 8 years ago.
34281.3.diff (17.8 KB) - added by eventualo 8 years ago.
I refred the patch
34281.4.diff (18.0 KB) - added by adamsilverstein 8 years ago.
34281.5.diff (18.0 KB) - added by eventualo 8 years ago.
34281.6.diff (18.1 KB) - added by adamsilverstein 8 years ago.
34281.7.diff (13.2 KB) - added by adamsilverstein 5 years ago.
34281.8.diff (13.1 KB) - added by adamsilverstein 5 years ago.
34281.9.diff (18.4 KB) - added by bookdude13 4 years ago.
Refreshed patch
34281.10.diff (26.0 KB) - added by adamsilverstein 3 years ago.
34281.11.diff (27.8 KB) - added by adamsilverstein 3 years ago.
34281.12.diff (21.6 KB) - added by johnbillion 3 years ago.
34281.13.diff (21.6 KB) - added by adamsilverstein 3 years ago.
34281.14.diff (21.8 KB) - added by adamsilverstein 3 years ago.
34281.15.diff (2.4 KB) - added by audrasjb 3 years ago.
Remove the IP Address from Admin generated password reset
34281.16.diff (1.1 KB) - added by audrasjb 3 years ago.
Remove the IP Address from Admin generated password reset

Download all attachments as: .zip

Change History (140)

#1 @mordauk
8 years ago

I support this times 100000x.

#2 @knutsp
8 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
8 years ago

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

#4 @adamsilverstein
8 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
8 years ago

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

#6 follow-up: @DrewAPicture
8 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
8 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.


8 years ago

#9 follow-up: @Ipstenu
8 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 8 years ago by Ipstenu (previous) (diff)

#10 in reply to: ↑ 9 @adamsilverstein
8 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
8 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
8 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
8 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
8 years ago

  • Milestone changed from Awaiting Review to 4.4

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


8 years ago

#16 @Thomas Vitale
8 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 8 years ago by Thomas Vitale (previous) (diff)

@eventualo
8 years ago

I refred the patch

#17 follow-up: @eventualo
8 years ago

I refreshed the patch for latest version.

#18 follow-up: @wonderboymusic
8 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
8 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
8 years ago

Replying to eventualo:

I refreshed the patch for latest version.

Thanks for refreshing!

#21 @adamsilverstein
8 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
8 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 8 years ago by eventualo (previous) (diff)

@eventualo
8 years ago

#23 in reply to: ↑ 22 ; follow-up: @adamsilverstein
8 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
8 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 8 years ago by eventualo (previous) (diff)

#25 in reply to: ↑ 24 ; follow-up: @adamsilverstein
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 years ago

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

#33 in reply to: ↑ 32 @adamsilverstein
8 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
8 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
8 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.


8 years ago

#37 follow-up: @afercia
8 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
8 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
8 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
8 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.


8 years ago

#42 @jorbin
8 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
8 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
7 years ago

#38915 was marked as a duplicate.

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


7 years ago

#46 @afercia
7 years 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.


6 years ago

#48 @JoshuaWold
6 years 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
6 years ago

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

#50 @adamsilverstein
6 years ago

  • Milestone changed from Future Release to 5.0

#51 @dainismichel
6 years 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

#52 @pento
5 years ago

  • Milestone changed from 5.0 to 5.1

#53 @adamsilverstein
5 years ago

  • Milestone changed from 5.1 to 5.2

34281.7.diff is a refresh against trunk, resolving several merge conflicts. Given the close proximity of 5.1 beta, milestoning this as 5.2 so we can test further and add some audible feedback for screen readers as suggested in https://core.trac.wordpress.org/timeline?from=2017-01-23T18%3A02%3A57Z&precision=second

#54 @desrosj
5 years ago

  • Milestone changed from 5.2 to 5.3

This still needs feedback and testing. Punting to 5.3 since 5.2 beta 1 is in less than 2 days.

#55 @adamsilverstein
5 years ago

Milestoning this as 'future release' until there is some more progress.

#56 @adamsilverstein
5 years ago

  • Milestone changed from 5.3 to Future Release

#57 @adamsilverstein
4 years ago

  • Milestone changed from Future Release to 5.4

Going to try to tackle this issue in 5.4, milestoning as such.

#58 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

Most recent patch fails against trunk. Marking for a refresh.

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


4 years ago

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


4 years ago

@bookdude13
4 years ago

Refreshed patch

#61 @bookdude13
4 years ago

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

Patch refreshed. Had to merge the retrieve_password function in wp-login.php into the one added in functions.php by earlier patches. It seems to be needed in functions.php to have the scope needed for both the login screen and the edit-users screen. A quick round of testing and the patch seems to work well. Very clean code from earlier contribs!

Note that the @since tags should be double checked; some of those were added in the original patches here back in about 4.4.

Side note, is the dev-feedback keyword still needed?

#62 @davidbaumwald
4 years ago

@adamsilverstein With the refreshed patch, do you think this can land for 5.4 Beta 1 today?

#63 @adamsilverstein
4 years ago

I will review today and give this a test to see if it ready to land.

#64 @karmatosed
4 years ago

  • Milestone changed from 5.4 to Future Release

With beta 1 coming today and expected in an hour or so, let's move this to future release just for now. This doesn't mean it won't get worked on, just right now it's going into next one.

#65 @bookdude13
4 years ago

  • Keywords needs-refresh added

Sounds good. Note that the @since tags will need updating then

#66 @adamsilverstein
4 years ago

Apologies, I wasn't able to spend time on this for 5.4, I'll work on it for 5.5.

This ticket was mentioned in PR #302 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#67

  • Keywords needs-refresh removed

Trac ticket:

adamsilverstein commented on PR #302:


3 years ago
#68

How this looks currently:

https://i0.wp.com/user-images.githubusercontent.com/2676022/106201743-203f8100-6176-11eb-9699-eebdc1e6ba9b.png
"Send Reset Link" on profile page (user is "Theme Reviewer")

https://i0.wp.com/user-images.githubusercontent.com/2676022/106202012-8cba8000-6176-11eb-9a7c-934099fef5b8.png

  • Send password reset in the bulk action menu

This ticket was mentioned in PR #954 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#69

Trac ticket: https://core.trac.wordpress.org/ticket/34281

https://i0.wp.com/user-images.githubusercontent.com/2676022/106201743-203f8100-6176-11eb-9699-eebdc1e6ba9b.png
"Send Reset Link" on profile page.

https://i0.wp.com/user-images.githubusercontent.com/2676022/106210912-fb9ed580-6184-11eb-9308-a8c2ba184e03.png

  • Confirmation.

https://i0.wp.com/user-images.githubusercontent.com/2676022/106202012-8cba8000-6176-11eb-9a7c-934099fef5b8.png

  • Send password reset in the bulk action menu

https://i0.wp.com/user-images.githubusercontent.com/2676022/106210833-d3af7200-6184-11eb-959d-71a4a1e10269.png
User list confirmation.

https://i0.wp.com/user-images.githubusercontent.com/2676022/106210863-e3c75180-6184-11eb-82f6-0dd00a7d68c2.png
User list reset password action.

#70 @adamsilverstein
3 years ago

  • Keywords commit added; dev-feedback needs-testing removed

34281.10.diff is a refresh against trunk. Everything looks good and tested well for me and this feels like a genuinely useful addition.

I have added some screenshots above.

Going to commit this as is before 5.7 beta1 so it can get some wider testing.

#71 @johnbillion
3 years ago

  • Keywords commit removed

This looks good. The retrieve_password() function needs some work though. If you diff it with the current one in wp-login.php some of the recent changes have been lost, for example the docs updates, the IP address message, the lowercase ERROR strings, and the lostpassword_errors filter. Might be some more, I didn't check too thoroughly.

Also it will need a @since entry for the new parameter, and probably also one to mention that prior to 5.7.0 it only existed in wp-login.php.

My only concern about the functionality itself is whether the "Send password reset" link is needed on the Users list table. It adds a lot of visual noise. Is this action common enough to warrant that link?

#72 @adamsilverstein
3 years ago

@johnbillion Thanks for the review! In 34281.11.diff

  • I copied the retrieve_password function over fresh, reintroduced the change, and added your suggested @since and not annotations to the doc block.

My only concern about the functionality itself is whether the "Send password reset" link is needed on the Users list table. It adds a lot of visual noise. Is this action common enough to warrant that link?

Yea, I tend to agree: I was wondering about this myself. It feels like having it as an option in the bulk menu is sufficient? I have left it in for now, I would like to get some more design review before making a final decision.

With these changes in place, do you feel like this is ready? If so, I would like to commit it before 5.7 beta 1 tomorrow.

@johnbillion
3 years ago

#73 @johnbillion
3 years ago

34281.12.diff makes a couple of tweaks:

  • Removed the formatting-only changes (mostly == to ===) that aren't related to the functional change, these should happen in a separate ticket so they can be checked properly.
  • Adjusted the formatting of retrieve_password() to minimise the non-functional changes and make it easier to see the diff from its old version. Below is the actual diff.
  • There are two noticeable changes to retrieve_password() as a result. These appear to create a functional change that might not be expected. The two sanitize_*() functions both strip octets and entities, for example. @adamsilverstein what's the reason to switching to using these functions?
    • sanitize_email(...) is used instead of trim( wp_unslash(...) )
    • sanitize_user(...) is used instead of trim( wp_unslash(...) )
  • .php

    old new  
    351351 * Handles sending a password retrieval email to a user.
    352352 *
    353353 * @since 2.5.0
     354 * @since 5.7.0 Added `$user_login` parameter.
    354355 *
     356 * Note: prior to 5.7.0 this function was in wp_login.php.
     357 *
     358 * @global wpdb         $wpdb       WordPress database abstraction object.
     359 * @global PasswordHash $wp_hasher  Portable PHP password hashing framework.
     360 *
     361 * @param  string       $user_login Optional user_login, default null. Uses
     362 *                                  `$_POST['user_login']` if `$user_login` not set.
    355363 * @return true|WP_Error True when finished, WP_Error object on error.
    356364 */
    357 function retrieve_password() {
     365function retrieve_password( $user_login = null ) {
    358366        $errors    = new WP_Error();
    359367        $user_data = false;
    360368
    361         if ( empty( $_POST['user_login'] ) || ! is_string( $_POST['user_login'] ) ) {
     369        // Use the passed $user_login if available, otherwise use $_POST['user_login'].
     370        if ( ! $user_login && ! empty( $_POST['user_login'] ) ) {
     371                $user_login = $_POST['user_login'];
     372        }
     373
     374        if ( empty( $user_login ) ) {
    362375                $errors->add( 'empty_username', __( '<strong>Error</strong>: Please enter a username or email address.' ) );
    363         } elseif ( strpos( $_POST['user_login'], '@' ) ) {
    364                 $user_data = get_user_by( 'email', trim( wp_unslash( $_POST['user_login'] ) ) );
     376        } elseif ( strpos( $user_login, '@' ) ) {
     377                $user_data = get_user_by( 'email', sanitize_email( $user_login ) );
    365378                if ( empty( $user_data ) ) {
    366379                        $errors->add( 'invalid_email', __( '<strong>Error</strong>: There is no account with that username or email address.' ) );
    367380                }
    368381        } else {
    369                 $login     = trim( wp_unslash( $_POST['user_login'] ) );
    370                 $user_data = get_user_by( 'login', $login );
     382                $user_data = get_user_by( 'login', sanitize_user( $user_login ) );
    371383        }
    372384
    373385        /**

#74 @adamsilverstein
3 years ago

@johnbillion Excellent, thanks for reviewing.

Removed the formatting-only changes (mostly == to ===) that aren't related to the functional change, these should happen in a separate ticket so they can be checked properly.

Thanks. I had changed these to assuage the linter, better to leave them out!

Adjusted the formatting of retrieve_password() to minimise the non-functional changes and make it easier to see the diff from its old version. Below is the actual diff.

Super, thanks for providing that and reformatting.

There are two noticeable changes to retrieve_password() as a result. These appear to create a functional change that might not be expected. The two sanitize_*() functions both strip octets and entities, for example. @adamsilverstein what's the reason to switching to using these functions?

This looks like a mistake, likely from, the original patch work which we have studiously brought forward. I will revert these changes.

#75 @johnbillion
3 years ago

Great, so I think with a revert of those sanitize functions and a decision on whether the links are needed in the Users list table, this is good to go.

#76 @adamsilverstein
3 years ago

In 34281.13.diff:

  • Revert sanitization changes.

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


3 years ago

#78 @paaljoachim
3 years ago

Hi

I am testing the feature. Thank you for adding it!
Thank you for the general ping in the design channel @adamsilverstein

Disadvantage.
Hiding a feature in the Bulk actions is not very user friendly. People will likely not notice it.

Advantage.
It is easier to add multiple actions into the Bulk actions drop down than it is to list them all directly in the open when hovering the username. As @johnbillion mentioned it does feel crowded adding it to the users list table. I do agree on that.

I pinged both @kellychoffman and @hedgefield in the design channel. I do not know if they will be able to give feedback in time.

My conclusion would be to merge this feature now and iterate on it later.

#79 @adamsilverstein
3 years ago

My conclusion would be to merge this feature now and iterate on it later.

Good idea. We can always remove the hover link later if we decide it is better without it.

#80 @hedgefield
3 years ago

This looks good to me from a design perspective. Makes sense. Useful feature. I too say merge and iterate if needed.

#81 @adamsilverstein
3 years ago

  • Keywords commit added

This looks good to me from a design perspective. Makes sense. Useful feature. I too say merge and iterate if needed.

Thanks for the review @hedgefield, I will work on the commit.

This ticket was mentioned in PR #970 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#82

Trac ticket:

#83 @adamsilverstein
3 years ago

34281.14.diff fixes a jslint error. (userProfileL10n was not defined, added it to globals comment).

#84 follow-up: @adamsilverstein
3 years ago

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

@afercia I realized we don't have this in place yet. We have a single place we use an ajax action to sent the password reset:

var resetAction = wp.ajax.post( 'send-password-reset', data );

Would wp.a11y.speak be added immediately before calling the ajax post? eg

wp.a11y.speak( 'Sending password reset' );

or would we fire immediately after the action with
wp.a11y.speak( 'Password reset sent' );

Or both?

#85 @paaljoachim
3 years ago

Hey Adam.

@afercia is away.
I believe @joedolson will be able to reply to your questions.

#86 @adamsilverstein
3 years ago

Thanks @paaljoachim . Going to commit this as we have it and will re-open to add the accessibility improvements.

#87 @adamsilverstein
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 50129:

Users: enable admins to send users a reset password link.

Add a feature so Admins can send users a 'password reset' email. This doesn't change the password or force a password change. It only emails the user the password reset link.

The feature appears in several places:

  • A "Send Reset Link" button on user profile screen.
  • A "Send password reset" option in the user list bulk action dropdown.
  • A "Send password reset" quick action when hovering over a username in the user list.

Props Ipstenu, DrewAPicture, eventualo, wonderboymusic, knutsp, ericlewis, afercia, JoshuaWold, johnbillion, paaljoachim, hedgefield.
Fixes #34281.

#88 @adamsilverstein
3 years ago

  • Milestone changed from Future Release to 5.7
  • Resolution fixed deleted
  • Status changed from closed to reopened

#89 @adamsilverstein
3 years ago

Re-opening to add accessibility improvements as discussed in this comment.

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

#90 @SergeyBiryukov
3 years ago

In 50139:

Users: Use consistent strings for error messages in wp-admin/users.php.

Use _n() for a string with plural forms.

Follow-up to [50129].

See #34281.

#91 @SergeyBiryukov
3 years ago

In 50140:

Users: Move retrieve_password() to wp-includes/user.php, for consistency with other user functions.

Follow-up to [25231], [50129].

Props jfarthing84, dimadin.
See #34281, #31039.

#92 @SergeyBiryukov
3 years ago

In 50141:

Docs: Update documentation for retrieve_password() per the documentation standards.

Follow-up to [50129], [50140].

See #34281.

#93 @adamsilverstein
3 years ago

Thanks for the follow/clean up commits @SergeyBiryukov!

#95 @desrosj
3 years ago

In 50152:

Coding Standards: Fix several minor coding standards issues.

These are made by running composer format.

Follow up to [50124], [50129], [50143].

See #49961, #52192, #34281.

#96 @SergeyBiryukov
3 years ago

  • Keywords commit removed
  • Type changed from enhancement to task (blessed)

#97 @adamsilverstein
3 years ago

Thanks @desrosj!

#98 @kellychoffman
3 years ago

Bringing a design review here from Slack: +1 to only having this setting live in the Bulk Action dropdown and reserving the hover links for frequent actions.

#99 in reply to: ↑ 84 @joedolson
3 years ago

Replying to adamsilverstein:

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

@afercia I realized we don't have this in place yet. We have a single place we use an ajax action to sent the password reset:

var resetAction = wp.ajax.post( 'send-password-reset', data );

Would wp.a11y.speak be added immediately before calling the ajax post? eg

wp.a11y.speak( 'Sending password reset' );

or would we fire immediately after the action with
wp.a11y.speak( 'Password reset sent' );

Or both?

Ideally, you'd send the wp.a11y.speak notification after the ajax post; it should probably be inside the addInlineNotice helper function, so that it reflects the same message that's shown to users.

I don't think this behavior is something that will take long enough that an 'in process' message is required.

#100 @joedolson
3 years ago

If this ends up only in the bulk actions dropdown, the accessibility comments might be irrelevant. However, I question whether this should be primarily a bulk action; in my own experience, I almost *always* am dealing with a password reset for just one user. The bulk action could be useful in some cases, but for convenience, the main action is more useful.

#101 @johnbillion
3 years ago

  • Keywords needs-dev-note added

#102 @audrasjb
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#103 @carike
3 years ago

I had a quick look at the P2 post. I agree that it would be preferable to remove the IP address entirely.
The reasoning for this is that when a user requests a change of their own password, they would have one of two legitimate interests in knowing the information:
1.) It is their own IP address;
2.) Someone is possibly trying to phish / hack them and they have a security-based interest.
Both of these are legitimate interests.

However, it is not desirable to give users access to the admin's IP address. I would not even want users to have access to my username (I would feel more comfortable if it included the display name, if any details had to be included at all).

I'm also concerned that with many WordPress users not being very tech savvy (and that is okay, that is the magic of crowd-sourcing code), this feature may make it easier for bad actors to phish.
Banks have spent billions trying to teach people not to click on any links in e-mails that you did not initiate yourself. :sunflower:

#104 @audrasjb
3 years ago

  • Focuses privacy added

In that case, I think our best option is to add a parameter to the retrieve_password function so it’s possible to display the IP when it’s needed (lost password link) and to remove it in all other cases 🙂

@audrasjb
3 years ago

Remove the IP Address from Admin generated password reset

#105 @audrasjb
3 years ago

In 34281.15.diff:

  • Add a $context parameter to retrieve_password to avoid to send IP addresses when it's not needed.

send_password_reset_from_admin is probably not the best name… but you know… naming things is hard.

#106 follow-up: @SergeyBiryukov
3 years ago

Looking at 34281.15.diff:

Used to avoid sending the Admin IP address when the password reset was not asked by the user.

It seems like this could be done without adding a new parameter to the function, just by comparing the $user_login parameter to the current user's login instead.

#107 in reply to: ↑ 106 ; follow-up: @audrasjb
3 years ago

Replying to SergeyBiryukov:

It seems like this could be done without adding a new parameter to the function, just by comparing the $user_login parameter to the current user's login instead.

Maybe I'm wrong @SergeyBiryukov, but I'm afraid it won't work for the Lost password link, as the user is not logged in in this context, by definition :) (and that's the main context where we'll want to provide the IP Address).

#108 in reply to: ↑ 107 ; follow-up: @SergeyBiryukov
3 years ago

Replying to audrasjb:

I'm afraid it won't work for the Lost password link, as the user is not logged in in this context, by definition :) (and that's the main context where we'll want to provide the IP Address).

That was my point, only add the IP address if the login is not the same as the current user's login (which would be the case when requesting the password reset link from the admin). But it's possible that I'm missing something too :)

#109 in reply to: ↑ 108 ; follow-up: @audrasjb
3 years ago

Replying to SergeyBiryukov:

That was my point, only add the IP address if the login is not the same as the current user's login (which would be the case when requesting the password reset link from the admin). But it's possible that I'm missing something too :)

Ah! I believe we want the opposite :)

  • Lost password link on wp-login: send the IP address so the user can be prevented from request from other IPs
  • New reset password methods on WP-Admin: don't send the IP address as the password reset is asked by a known user on the website (and it fixes some potential privacy issues)

#110 in reply to: ↑ 109 @SergeyBiryukov
3 years ago

Replying to audrasjb:

Ah! I believe we want the opposite :)

Right, my comment doesn't make much sense on second thought. Replying to too many things at a time, sorry :)

  • Lost password link on wp-login: send the IP address so the user can be prevented from request from other IPs
  • New reset password methods on WP-Admin: don't send the IP address as the password reset is asked by a known user on the website (and it fixes some potential privacy issues)

I think this can be done with one of two options:

  • Only include the IP address if the user is not logged in (which would be the case when requesting the password reset link from wp-login.php).
  • Only include the IP address when requesting the password reset link from wp-login.php specifically, by checking the $pagenow global, like we do in several other places in core.

Per your second point, it looks like the first option would preferable.

My concern with adding a new parameter this late in the release cycle is that it's hard to adjust later without breaking backward compatibility, which might lead to minor inconsistencies in the API in the future. This would require some careful thinking, so I'd like to avoid adding a new parameter for now if there's another way.

#111 @audrasjb
3 years ago

My concern with adding a new parameter this late in the release cycle is that it's hard to adjust later without breaking backward compatibility, which might lead to minor inconsistencies in the API in the future. This would require some careful thinking, so I'd like to avoid adding a new parameter for now if there's another way.

I understand. I'll update a new patch with the second option ;)

@audrasjb
3 years ago

Remove the IP Address from Admin generated password reset

#112 @audrasjb
3 years ago

I tested this solution and it works fine on my side.

#113 @gmariani405
3 years ago

@audrasjb "Remove the IP Address from Admin generated password reset"

Not sure this is great idea either. The IP address (while fraught with privacy concerns) is the only thing validating that this email came from the website and is not a phishing email. Unless there is a better way to validate the authenticity of the email I'd say it would be worthwhile to keep it.

Scenario, user asks host or web designer for help. The host/web designer goes in and resets their password. User receives both the legitimate mail and by chance a phishing email. Both would look indistinguishable. The IP helps distinguish it as legitimate.

I realize it's not fool-proof, a savvy enough bad-actor could DNS lookup the IP before sending the phishing email but that is more labor intensive when you're mass-mailing. This also relies on the end-user being skilled enough to validate the website domain against what was displayed in the email, but I say to that, at-least it's better than nothing.

#114 @Ipstenu
3 years ago

The IP address (while fraught with privacy concerns) is the only thing validating that this email came from the website and is not a phishing email.

It is though? I could use my phone to send a reset, and I would have no idea what my IP was. And that can easily be faked. Omitting the IP actually reduces the data being sent out that could be used by bad-actors.

I think it's more likely we'd have a savvy bad actor than end users who would need to ask for a password reset but also know what a valid IP is and how to ask about it.

Not that we shouldn't look into something like logging or 'proving' ... off the top of my head, if when we use the password reset, it sets a key in the user's account, and the user has to enter that key to reset the password? That could work.

Edited to add: I feel this is a "release and iterate" case! We can improve this down the road, and I do not think my off the cuff idea is a blocker!

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

#115 @juliobox
3 years ago

This IP address is just useless, can we just remove it?
Who knows how to compare an IP with its own to be sure it's not a fake? Power user I guess.
Problems resolved ;)

#116 @desrosj
3 years ago

In 50415:

Users: Ensure reset password emails are in the receiving user’s locale.

This ensures that reset password emails initiated by an administrator are sent to the user in their preferred locale.

Follow up to [50129,50139-50141,50152].

Props chouby, davidbaumwald, audrasjb, johnbillion.
Fixes #52605. See #34281.

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


3 years ago

#118 @gmariani405
3 years ago

I agree that the IP would only be valuable in a power-user scenario and it's by no means fool-proof. But SOMETHING should be available to help prove the authenticity of the email.

"This IP address is just useless," - A bit of an overstatement there. If ANY user (even if just a power-user) is able to avoid a ransomware attack by this information proves it has some value.

#119 @SergeyBiryukov
3 years ago

In 50422:

Users: Only include the IP address in password reset email if the user is not logged in.

This avoids unnecessarily disclosing the IP address when sending a password reset link to another user from the admin.

Follow-up to [49255], [50129].

Props carike, audrasjb, gmariani405, Ipstenu.
See #34281.

#120 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Let's call this one fixed for 5.7. Please create new tickets for any follow-up issues.

Thanks everyone!

#121 @Ipstenu
3 years ago

FYI made a follow up at #52630

#122 @peterwilsoncc
3 years ago

In 50439:

Users: Use localized site title for password reset emails.

When sending password reset emails, switch to the user's locale prior to obtaining the site title to allow for plugins filtering based on locale.

Follow up to [50129,50139-50141,50152,50415].

Props chouby.
Fixes #52605. See #34281.

#123 @SergeyBiryukov
3 years ago

In 50440:

Users: Use localized site title for password reset emails.

When sending password reset emails, switch to the user's locale prior to obtaining the site title to allow for plugins filtering based on locale.

Follow up to [50129,50139-50141,50152,50415].

Props chouby.
Merges [50439] to the 5.7 branch.
Fixes #52605. See #34281.

#124 @ocean90
3 years ago

In 51077:

Users: Add user’s locale to password reset link to ensure login screen matches the language of the email.

Props walbo.
See #34281, #52605.
Fixes #53321.

Note: See TracTickets for help on using tickets.