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 | Owned by: | 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)
Change History (19)
#1
@
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
#3
@
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?
#4
@
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
@
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?
#7
@
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
@
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?
#10
@
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
#13
@
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.
#14
@
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
@
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
@
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!
@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.