REST create user: existing_user_login is returned before existing_user_email

[bbrian]

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.

[johnbillion]

@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.

[sconzof]

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?

[sconzof]

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.

[guzzilar]

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?

[shooper]

Adds HTTP 409 response code, and unit tests

[shooper]

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.

[shooper]

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

[sconzof]

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

[DrewAPicture]

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

[bartj]

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.

[desrosj]

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!

[TimothyBlynJacobs]

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

[sconzof]

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!