WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30647 closed defect (bug) (fixed)

wp_update_user() allows using existing user_email, user_login, and user_nicename

Reported by: danielbachhuber Owned by: wonderboymusic
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.9.2
Component: Users Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

When updating a user with wp_update_user() (which, in turn, uses wp_insert_user()), it's possible to set the user_email or user_nicename to those of an existing user. It's also possible to supply user_login, which gets silently discarded.

Each of these values need to be considered unique per user. If the values were supplied and not unique, I'd expect WP_Error to be returned.

Here's the relevant faulty logic:

if ( ! $update && username_exists( $user_login ) ) {
	return new WP_Error( 'existing_user_login', __( 'Sorry, that username already exists!' ) );
}
if ( ! $update && ! defined( 'WP_IMPORTING' ) && email_exists( $user_email ) ) {
	return new WP_Error( 'existing_user_email', __( 'Sorry, that email address is already used!' ) );
}

Looks like this was introduced in r12778.

Discovered in:

Attachments (4)

30647.patch (618 bytes) - added by rittesh.patel 5 years ago.
30647.diff (881 bytes) - added by rittesh.patel 5 years ago.
Patch updated
user.diff (1.3 KB) - added by rittesh.patel 5 years ago.
test case for 30647.diff patch
30647.2.diff (2.2 KB) - added by DrewAPicture 5 years ago.

Download all attachments as: .zip

Change History (15)

#1 follow-up: @danielbachhuber
5 years ago

It would be nice to have a nicename_exists() function, similar to username_exists()

@rittesh.patel
5 years ago

#2 @rittesh.patel
5 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed
if ( ! $update && ! defined( 'WP_IMPORTING' ) && email_exists( $user_email ) )

Here $update is playing a critical role. In case of new user, $update will be false hence email_exist condition will be checked resulting in an error saying Sorry, that email address is already used!.

But, when we update an existing user, email_exist will not be checked at all since the very first check for $update will fail itself.

And anyhow email_exist condition should be checked for all the cases. Hence I've removed $update. I'm not sure about WP_IMPORTING hence kept it as it is.

Dev suggestions will be appreciated if I'm missing anything.

#3 @danielbachhuber
5 years ago

Any patch should have copious unit tests to even be considered.

@rittesh.patel
5 years ago

Patch updated

@rittesh.patel
5 years ago

test case for 30647.diff patch

#4 @rittesh.patel
5 years ago

Find the updated patch and test case for that patch in attachment.

The old version of patch was throwing error for test_update_user check when checked with phpunit, it wasn't updating any detail of user at all when ! $update removed from if condition.

Following is the current code to check for email exist and throw WP_Error accordingly

( ! $update && ! defined( 'WP_IMPORTING' ) && email_exists( $user_email ) )

I have updated it to the following

( ( ! $update || ( ! empty( $old_user_data ) && $user_email !== $old_user_data->user_email ) ) && ! defined( 'WP_IMPORTING' ) && email_exists( $user_email ) )

It will simply first check whether these is update or insert, if it's insert just check for the email exist and if it's update than first check whether email has been changed or not and then check accordingly for email exist.

#5 @rachelbaker
5 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 4.2

#6 @rachelbaker
5 years ago

  • Version set to 2.9.2

#7 in reply to: ↑ 1 @SergeyBiryukov
5 years ago

Replying to danielbachhuber:

It would be nice to have a nicename_exists() function, similar to username_exists()

Related: #31106

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


5 years ago

@DrewAPicture
5 years ago

#9 @DrewAPicture
5 years ago

  • Keywords commit added; dev-feedback removed

30647.2.diff combines the unit tests diff with the patch and cleans up the syntax. Unit tests pass.

Moving for commit consideration.

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


5 years ago

#11 @wonderboymusic
5 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 31963:

When updating the email address for an existing user, make sure the email address is not already in use.

Adds unit tests.

Props rittesh.patel, DrewAPicture.
Fixes #30647.

Note: See TracTickets for help on using tickets.