Make WordPress Core

Opened 3 years ago

Closed 20 months ago

Last modified 20 months ago

#53634 closed defect (bug) (fixed)

Editing user in Dashboard and using "Send Reset Link " broken by retrieve_password()

Reported by: boblindner's profile boblindner Owned by: pbearne's profile pbearne
Milestone: 6.1 Priority: normal
Severity: major Version:
Component: Users Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

If you change the email of a user created with the username of an email address you are unable to send a reset link because retrieve_password() in wp-includes/user.php mistakenly thinks the username is an email address for a user (because it contains an @) so you get:

Error: There is no account with that username or email address.

Steps to reproduce:

  • make a user with the same username AND email address. e.g. foo(@)example.com
  • edit that user and change the email address (username not editable) to the email address. e.g. bar(@)example.com and save user
  • Try to use the “Send Reset Link” button while editing that user again (/wp-admin/user-edit.php)

I think this is happening because the call to retrieve_password() passes in the username and retrieve_password() mistakenly believes everything with an "@" in must be an email address.

Attachments (4)

53634.diff (1.7 MB) - added by azouamauriac 2 years ago.
53634.1.diff (1.2 KB) - added by azouamauriac 2 years ago.
Hello please ignore the previous attachement, it was mistake. This is the fix i proposed.
53634.2.diff (1.0 KB) - added by SergeyBiryukov 20 months ago.
53634.3.diff (2.4 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (27)

This ticket was mentioned in PR #1536 on WordPress/wordpress-develop by donmhico.


3 years ago
#1

  • Keywords has-patch added

This PR takes into account that username can have @ char.

For backward-compatibility, we are still sending the WP_Error with error message invalid_email if the passed user login has @ character and no user data was found.

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

This ticket was mentioned in PR #1536 on WordPress/wordpress-develop by donmhico.


3 years ago
#2

This PR takes into account that username can have @ char.

For backward-compatibility, we are still sending the WP_Error with error message invalid_email if the passed user login has @ character and no user data was found.

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

#3 @daxelrod
3 years ago

Hello, I ran across this same issue. The referenced PR seems to have the right idea (check email, but fallback to username).

This is related to the wider issue #22367, however I do not believe the patch there addresses this issue.

@azouamauriac
2 years ago

@azouamauriac
2 years ago

Hello please ignore the previous attachement, it was mistake. This is the fix i proposed.

#4 @azouamauriac
2 years ago

  • Keywords needs-testing added
  • Severity changed from normal to major
  • Version 5.7.2 deleted

#5 @azouamauriac
2 years ago

@SergeyBiryukov @flixos90 hello can you take a look over here? I'm pinging you because i don't know who is component maintainer. thank you.

#6 @pbearne
2 years ago

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

#8 @pbearne
2 years ago

  • Keywords has-unit-tests added; needs-testing removed

I was able to fix this by just changing what (to the email from the username) we passed to the retrieve_password

But looking at this there an optimization to be done as we can pass the whole WP_User object to retrieve_password as this first action to get the object so I have done that in this ticket (which would fix this ticket as well) #56028

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

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


22 months ago

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


20 months ago

#11 @pbearne
20 months ago

  • Milestone changed from Awaiting Review to 6.1

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


20 months ago

#13 @SergeyBiryukov
20 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Hi there, welcome to WordPress Trac! Thanks for the ticket, patches and the PRs.

I think I would prefer PR 1536 here, as it attempts to fetch the user both by email and username, without adding a new parameter to the function.

PR 2837 may look simpler, but I have a few concerns:

  • It doesn't feel right to pass an email to the function if the parameter is specifically documented as a username. Should the documentation be updated in that case?
     * @param string $user_login Optional. Username to send a password retrieval email for.
     *                           Defaults to `$_POST['user_login']` if not set.
    
  • This is not the only instance where core passes user_login to retrieve_password(), there is at least one other line in wp-admin/users.php that would need to be updated. In the future, we would also need to remember to pass an email to the function instead of a username.
  • While passing an email instead of username fixes this particular ticket, it would be a good idea to confirm that it does not cause unintentional breaking changes in other cases. Some unit tests would be helpful here.

#14 @SergeyBiryukov
20 months ago

53634.2.diff is a slightly simplified version of PR 1536.

#15 @audrasjb
20 months ago

Thanks @pbearne, I tested your PR with the provided testing steps and it also fixes the issue on my side.

Steps to reproduce:

  • make a user with the same username AND email address. e.g. foo(@)example.com
  • edit that user and change the email address (username not editable) to the email address. e.g. bar(@)example.com and save user
  • Try to use the “Send Reset Link” button while editing that user again (/wp-admin/user-edit.php)

After applying the patch, the email is sent.

#16 @audrasjb
20 months ago

Alright, just saw @SergeyBiryukov's comments.
Sounds legit to me 👍

#17 @SergeyBiryukov
20 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

53634.3.diff adds a unit test.

#19 @pbearne
20 months ago

I would still include the patch I did, as this passes the name, which is a safer value to pass

#20 @SergeyBiryukov
20 months ago

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

In 54477:

Users: Fetch user by login in retrieve_password() if not found by email.

This ensures that sending a password reset link works as expected if the user's login and email were initially the same, but the email address was subsequently updated and no longer matches the login, which is still set to the old address.

Follow-up to [6643], [18513], [19056], [37474], [50129], [50140].

Props donmhico, pbearne, azouamauriac, boblindner, daxelrod, audrasjb, SergeyBiryukov.
Fixes #53634.

SergeyBiryukov commented on PR #3414:


20 months ago
#21

Merged in r54477.

SergeyBiryukov commented on PR #1536:


20 months ago
#22

Thanks for the PR! Merged a slightly simplified version in r54477.

SergeyBiryukov commented on PR #2837:


20 months ago
#23

Thanks for the PR! This should be resolved in r54477.

Note: See TracTickets for help on using tickets.