Make WordPress Core

Opened 7 years ago

Last modified 3 months ago

#41672 assigned enhancement

REST create user: existing_user_login is returned before existing_user_email

Reported by: bbrian's profile bbrian Owned by: shooper's profile shooper
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: good-first-bug 2nd-opinion has-patch has-unit-tests
Focuses: rest-api Cc:

Description

When I post to /wp-json/wp/v2/users to create a user:

{
	"email": "brianhenryie@gmail.com",
	"username": "brianhenryie",
	"password": "password"
}

and a user with that username and email address exists, the response is:

{
    "code": "existing_user_login",
    "message": "Sorry, that username already exists!",
    "data": null
}

whereas a more useful response would be the existing_user_email response:

{
    "code": "existing_user_email",
    "message": "Sorry, that email address is already used!",
    "data": null
}

which is learned once the original POST is updated with a new username.

i.e. existing_user_email tells a user if they already have an account. This information could be used to attempt to log in.

Attachments (3)

41672.diff (1.6 KB) - added by sconzof 7 years ago.
41672-with-409-and-unit-tests.diff (4.0 KB) - added by shooper 7 years ago.
Adds HTTP 409 response code, and unit tests
41672.2.diff (3.6 KB) - added by bartj 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @johnbillion
7 years ago

  • Component changed from General to Users
  • Focuses rest-api added
  • Keywords needs-patch needs-unit-tests added
  • Version changed from 4.8.1 to 4.7

@bbrian Thanks for the ticket, and welcome to WordPress Trac :-)

I think it makes sense to prioritise the existing email error over the existing username error so, like you said, users learn immediately that they already have a user account.

#2 @johnbillion
7 years ago

  • Keywords good-first-bug added

#3 @sconzof
7 years ago

  • Keywords 2nd-opinion added

The rest api create_item function for users (in class-wp-rest-users-controller.php) relies upon the core wp_insert_user function (in wp-includes/user.php). Logic for determining whether usernames and email addresses exist is implemented in the wp_insert_user function, and should probably stay there, and not be moved into class-wp-rest-users-controller.php.

wp_insert_user() first checks to see if the username already exists with the username_exists() function. It immediately short-circuits with the error existing_user_login, and doesn't also check if the email also exists.

Suggestion: Instead of immediately returning WP_Error( 'existing_user_login', __( 'Sorry, that username already exists!' ) ); in the case of an existing username, the code could first call email_exists(), then provide a different error message existing_user_email if BOTH the username and the email address already exist.

However, the implication here is this will affect more than just the rest-api. This will have effects upon any code that uses the wp_insert_user() function.

So, given the wider implications of such a change, should this change be made or not?

Last edited 7 years ago by sconzof (previous) (diff)

@sconzof
7 years ago

#4 @sconzof
7 years ago

  • Keywords has-patch added; needs-patch removed

I have uploaded a patch 41672.diff that would return the existing_user_email error, instead of the existing_user_login error, if both the username and email already exist. This patch can be used if that is deemed an appropriate direction to take for this enhancement request.

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


7 years ago

#6 @guzzilar
7 years ago

Hi @sconzof,

From your patch, I just wonder If it is about priority of validation, wouldn't it be better if we just move the whole line of below code:

<?php

$raw_user_email = empty( $userdata['user_email'] ) ? '' : $userdata['user_email'];

/**
 * Filters a user's email before the user is created or updated.
 *
 * @since 2.0.3
 *
 * @param string $raw_user_email The user's email.
 */
$user_email = apply_filters( 'pre_user_email', $raw_user_email );

/*
 * If there is no update, just check for `email_exists`. If there is an update,
 * check if current email and new email are the same, or not, and check `email_exists`
 * accordingly.
 */
if ( ( ! $update || ( ! empty( $old_user_data ) && 0 !== strcasecmp( $user_email, $old_user_data->user_email ) ) )
        && ! defined( 'WP_IMPORTING' )
        && email_exists( $user_email )
) {
        return new WP_Error( 'existing_user_email', __( 'Sorry, that email address is already used!' ) );
}

to above $sanitized_user_login = sanitize_user( $userdata['user_login'], true ); line?

Also, shaw we call sanitize_email() at $raw_user_email = empty( $userdata['user_email'] ) ? '' : $userdata['user_email'];?

So from

<?php

$raw_user_email = empty( $userdata['user_email'] ) ? '' : $userdata['user_email'];

/**
 * Filters a user's email before the user is created or updated.
 *
 * @since 2.0.3
 *
 * @param string $raw_user_email The user's email.
 */
$user_email = apply_filters( 'pre_user_email', $raw_user_email );

Could be

<?php

$sanitized_user_email = empty( $userdata['user_email'] ) ? '' : sanitize_email( $userdata['user_email'] );

/**
 * Filters a user's email before the user is created or updated.
 *
 * @since 2.0.3
 *
 * @param string $raw_user_email The user's email.
 */
$user_email = apply_filters( 'pre_user_email', $sanitized_user_email );

What do you think?

Last edited 7 years ago by guzzilar (previous) (diff)

@shooper
7 years ago

Adds HTTP 409 response code, and unit tests

#7 @shooper
7 years ago

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

The patch I just attached takes @sconzof's patch and adds to it an HTTP 409 (Conflict) response code. This is the same approach used when adding a term with an existing name via the RET API.

This patch also adds unit tests.

#8 @shooper
7 years ago

Discussed the 409 response code with @rachelbaker at WCUS Contributor Day today. For consistency, might change the response code here to a 400. Agreed?

#9 @sconzof
7 years ago

@shooper - yes, that seems like a good approach. I agree, FWIW.

#10 @DrewAPicture
7 years ago

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

Assigning to mark the good-first-bug as "claimed".

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


4 years ago

#12 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.8

#13 @bartj
4 years ago

I've looked into this and it appears, that nowadays REST API returns the following messages in correct order: firstly it informs if email is duplicated, then login.
Despite that, I agree, it would be better to return 409 in HTTP response than Server Error.

I've refreshed patch, adding only more meaningful response and tests for that.

@bartj
4 years ago

#14 @desrosj
4 years ago

  • Milestone changed from 5.8 to 5.9

Thanks for the discussion and contributions here, everyone!

Today is feature freeze for the 5.8, and it seems this one could benefit from a bit more testing and review before getting committed. I'm going to punt to the 5.9 release, and let's see about getting it polished in the weeks following 5.8!

#15 @TimothyBlynJacobs
3 years ago

  • Milestone changed from 5.9 to Future Release

With feature freeze tomorrow and no movement over the past six months, I'm going to move this back to Future Release.

#16 @sconzof
3 months ago

It is still useful to know when creating a user account whether an error is caused by a duplicate email address or duplicate username.

And the HTTP 409 (Conflict) response code is more useful than the current HTTP 500 response code.

What are the next steps to getting this change into a release? Is there something I can do to help? Looking for some guidance... Thanks!

Note: See TracTickets for help on using tickets.