WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 22 months ago

#41672 assigned enhancement

REST create user: existing_user_login is returned before existing_user_email

Reported by: bbrian Owned by: shooper
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: good-first-bug 2nd-opinion has-patch has-unit-tests
Focuses: rest-api Cc:
PR Number:

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 (2)

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

Download all attachments as: .zip

Change History (12)

#1 @johnbillion
2 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
2 years ago

  • Keywords good-first-bug added

#3 @sconzof
2 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 2 years ago by sconzof (previous) (diff)

@sconzof
2 years ago

#4 @sconzof
2 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.


2 years ago

#6 @guzzilar
2 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 2 years ago by guzzilar (previous) (diff)

@shooper
2 years ago

Adds HTTP 409 response code, and unit tests

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

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

#10 @DrewAPicture
22 months ago

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

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

Note: See TracTickets for help on using tickets.