Fix/username check

[polevaultweb]

This patch improves the logic around the username checks added in WP 6.2, to make sure the check doesn't cause issues when updating a user.

Trac ticket: https://core.trac.wordpress.org/ticket/57967

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Hi @polevaultweb! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

• PHP Coding Standards
• CSS Coding Standards
• HTML Coding Standards
• JavaScript Coding Standards
• Accessibility Coding Standards
• Inline Documentation Standards

Thank you,
The WordPress Project

[costdev]

Thanks for the PR @polevaultweb! I've left some thoughts below 🙂

[polevaultweb]

Thanks @costdev, applied and updated

[azaozz]

Is the isset( $user_id ) && $existing_user_id !== $user_id needed here? The isset( $user_id ) doesn't seem necessary as if $update is true, it is always set (see above).

Then the $existing_user_id !== $user_id means that only logged-in users would be able to make changes for themselves (an admin may not be able to make changes to another user?). As this bug seems to exist only when the username and email address match, perhaps can check that instead?

On the other hand looking at the username check above it seems if ( ! $update && email_exists( $user_login ) ) would be sufficient?

[costdev]

If the user is being updated to share the same email address as another user, this should return a WP_Error.

$existing_user_id = The user with that email address
$user_id = The user being updated.

• If they don't match, then it should return an error
  • This will prevent updating a user to share the same email address as another.
• If they do match, the user should be able to be updated
  • This updates a user whose user_login and user_email are the same. This is one of the two key issues with the regression.

[costdev]

I believe you're right about isset( $user_id ) though, that doesn't seem to be necessary here.

[azaozz]

$existing_user_id = The user with that email address
$user_id = The user being updated.

May be missing something but this means that if user A has a username that matches user B's email address, user A cannot be updated? Shouldn't that be the other way round: prevent an user from changing their email address to match another user's username?

[costdev]

this means that if user A has a username that matches user B's email address, user A cannot be updated?

Just ran a test with the PR - that's correct.

Shouldn't that be the other way round: prevent an user from changing their email address to match another user's username?

I see your point. I wonder, is there ever a situation where one user should have a username that matches another's email address, or vice versa?

What if:

• We only allow creating if:
  • The username doesn't match an existing username or email address.
  • The email doesn't match an existing username or email address.
• We only allow updating if:
  • Context: Updating with a unique username
    • The username doesn't exist
  • Context: Updating with a unique email address
    • The email address doesn't exist
  • Context: Updating a user without changing the username or email address
    • The username exists, but for the same user.
    • The email address exists, but for the same user.
  • Context: Updating a user to make the username and email address match
    • The username exists as an email address, but it's for the same user.
    • The email address exists as a username, but it's for the same user.

At any rate, it may be better to revert the related changesets while we determine what the "right" path forward is.

[costdev]

FWIW, the above does seem to be achievable with some additional checks. Here's the results of some tests I have locally:

And the checks:

// Already existed in part, updated to cover updates too: Username must not match another user's username.
$username_exists = username_exists( $user_login );
if ( $username_exists && ( ! $update || $username_exists !== $user_id ) ) {
	return new WP_Error( 'existing_user_login', __( 'Sorry, that username already exists!' ) );
}

// New but at least partially handled elsewhere in Core I believe: Email must not match another user's email address.
$email_exists = email_exists( $userdata['user_email'] );
if ( $email_exists && ( ! $update || $email_exists !== $user_id ) ) {
	return new WP_Error( 'existing_user_email', __( 'Sorry, that email address already exists!' ) );
}

// From this PR (isset() removed + variable rename) Username must not match another user's email address.
$existing_user_email_as_login = email_exists( $user_login );
if ( $existing_user_email_as_login && ( ! $update || $existing_user_email_as_login !== $user_id ) ) {
	return new WP_Error( 'existing_user_email_as_login', __( 'Sorry, that username is not available.' ) );
}

// New: Email address must not match another user's username.
$existing_user_login_as_email = username_exists( $userdata['user_email'] );
if ( $existing_user_login_as_email && ( ! $update || $existing_user_login_as_email !== $user_id ) ) {
	return new WP_Error( 'existing_user_login_as_email', __( 'Sorry, that email address is not available.' ) );
}

(All lines/branches are covered by the local tests)

[azaozz]

We only allow creating if:

• The username doesn't match an existing username or email address.
• The email doesn't match an existing username or email address.

Right. These are the correct requirements imho.

I wonder, is there ever a situation where one user should have a username that matches another's email address, or vice versa?

There shouldn't be any of these. However WP has no checks to catch these cases. Thinking there should be back-compat to keep updates of existing users with "wrong" username or email still working. This makes it somewhat complicated but seems $old_user_data can be useful for that.

[azaozz]

We only allow updating if:
...

Using an email address as a username is somewhat complex in WP as usernames are generally public. Then changing the email would mean that user now has two emails to test against.

The logic here is good imho, just couple of additions:

• Context: Updating the username (generally usernames cannot be updated, needs a plugin):
  • The username doesn't exist.
  • If the username looks like an email address, the email is not used by another user.
• Context: Updating the email address
  • The email address doesn't exist.
  • The email address is not another user's username.
• Context: Updating a user without changing the username or email address
  • The username exists, but for the same user.
  • The email address exists, but for the same user.
  • Allow the user's email to be another user's username and/or the user's username to be another user's email (back-compat).
• Context: Updating a user to make the username and email address match
  • The username exists as an email address, but it's for the same user.
  • The email address exists as a username, but it's for the same user.
  • Do not allow (obviously) if someone else has the requested username or email. See the back-compat above.

[azaozz]

Looking a bit more, perhaps this can be implemented as a "long list of ifs" :)
Something like:

if ( $update ) {
    // Check updated email
    if ( $old_user_data->data->user_email !== $userdata['user_email'] ) {
        if ( [another user's email] ) {
            // Throw error
        }

        if ( $userdata['user_email'] === [another user's username] ) {
            // Throw error
        }
    }

    // Check updated username
    if ( $old_user_data->data->user_login !== $userdata['user_login'] ) {
    ...
}

May need to do something for user_nicename too. Currently it gets a suffix until it is unique but seems there is no check if it matches another user's user_login, or vice-versa.

[costdev]

Pinging @hellofromtonya for a confidence check.

This PR only touches wp_insert_user(). It may therefore be necessary to either add @covers ::wp_insert_user to the covered functions (otherwise nothing in this PR is shown in a coverage report), or change the tests to only target the behaviour of wp_insert_user() (i.e. no calls to wp_update_user()), and update the @covers annotation accordingly.

Not 100% sure on the right call here.

[hellofromtonya]

Thanks for the ping @costdev.

The Trac ticket specifically mentions using wp_update_user(). While the PR touches only the wp_insert_user(), using wp_update_user() helps to document the issue.

So I think adding an @covers for both functions is needed.

[hellofromtonya]

This test passes before applying the fix in this PR. This seems correct as it should reject for this dataset.

[costdev]

Suggested change

	* @covers ::wp_update_user
	* @covers ::wp_insert_user
	* @covers ::wp_update_user

[costdev]

Suggested change

	* @covers ::wp_update_user
	* @covers ::wp_insert_user
	* @covers ::wp_update_user

[hellofromtonya]

Before applying the fix in this PR, this test fails as an instance of WP_Errror is returned with the error message of Sorry, that username is not available. This is expected ✅

[azaozz]

Wondering if this PR should be repurposed or a new one opened to implement the logic discussed in #4257 (comment) and the following comments.