Regression: Username check introduced in WP 6.2 should allow updates to the same user

[polevaultweb]

If you create a user with the username set as the email address, when you use wp_update_user to update the user's data, in WP 6.2 RC3 it will throw a 'existing_user_email_as_login' error due to the code introduced to fix #57394 / [55358] and [55360].

The code does not account for update calls to wp_insert_user, and if the function is called as part of an update it should check that the user being updated is different to the user found by the email search.

[audrasjb]

Hello and thanks for the ticket and patch!

The general logic of your patch sounds good to me, but it still needs testing.

I'm moving it to 6.2 for visibility, but it's probably going to end up in milestone 6.2.1 as we're very close to WP 6.2.

[polevaultweb]

Thanks @audrasjb. I'm concerned that this may cause breakages for a good deal of plugins that use wp_update_user and the email address is used for the user_login. It might be a better idea to revert https://core.trac.wordpress.org/ticket/57394?

[audrasjb]

Yes, looking back at this issue, I think reverting both [55358] and [55360] may be our safer option.

Pinging @adamsilverstein and @SergeyBiryukov who committed both changesets for another thought.

[polevaultweb]

Patch with unit tests

[polevaultweb]

Thanks @audrasjb. I've just added a new patch with unit tests in case others want to fix rather than revert.

[costdev]

Thanks for adding the patch with unit tests @polevaultweb! Are you comfortable opening a GitHub PR against ​wordpress-develop:trunk to run the tests against CI and for review, or do you mind if I do that?

[polevaultweb]

You're welcome @costdev. Just raised the PR here ​https://github.com/WordPress/wordpress-develop/pull/4257

[costdev]

Thanks @polevaultweb!

[peterwilsoncc]

Replying to audrasjb:

Yes, looking back at this issue, I think reverting both [55358] and [55360] may be our safer option.

I agree this is the best approach this late in the release candidate period.

[hellofromTonya]

Hello @polevaultweb,

Thank you for reporting this regression.

In doing a review with @costdev and @audrasjb, we've confirmed that yes this is indeed a regression and it must be fixed for 6.2.0.

How many plugins might be affected?
Doing searches in the wp.org plugins repository, there are:

• ​2,346 plugins using wp_update_user().
• ​2,235 plugins using wp_insert_user().

Revert or fix?

This regression could be fixed if there's a solid, well-tested, low risk solution that committers have high confidence in that it will solve the problem without introducing additional issues.

Else, the safest approach is to revert [55358] and [55360].

[SergeyBiryukov]

Replying to peterwilsoncc:

Replying to audrasjb:

Yes, looking back at this issue, I think reverting both [55358] and [55360] may be our safer option.

I agree this is the best approach this late in the release candidate period.

I agree too, let's revert for now and try again in 6.3 with the suggested patch.

[hellofromTonya]

In the light of a brand new day, I also agree with reverting both [55358] and [55360].

Why?

tl;dr

I suspect there are more go/no-go combinations to be considered, especially against BC.

Longer reasoning

@azaozz has ​raised valid questions:

$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?

In observing the ​discussion in the patch and looking through the current testing scenarios, I too have more questions.

I think this area of the code needs a full list of happy and unhappy testing scenarios to cover the what ifs and what should and shouldn't happen. I suspect there are more combinations to be considered, especially against BC.

Other than [53501] (which made 2 strings translatable) and [52954] (which relocated an existing user_nicename length check), the wp_insert_user() function has not been touched in years, suggesting caution is needed when adding additional no-go checks for inserting or updating a user.

Inserting new users and updating existing users are fundamental tasks. The risk is too high for breakages by trying to fix this regression at this late stage in the 6.2 release, as a fix may introduce more issues.

What are the risks of reverting?

IMO None.

Reverting removes the risk to the 6.2 release. Why? It removes the new scenario added during the release, restoring the wp_insert_user() back to pre-6.2.

Are there other changes made that could possibly be impacted by these reverts?

Hmm, I don't think so. Why?

• wp_insert_user(): no other changes were made in the 6.2 cycle.
• wp_update_user(): [55161] introduced switch_to_user_locale(), but I don't see a correlation to go/no-go for update/insert user logic.

[polevaultweb]

Thanks for the triage and thoughtful approach here @hellofromTonya. Completely agree too, especially with this -

the wp_insert_user() function has not been touched in years, suggesting caution is needed when adding additional no-go checks for inserting or updating a user.

[spacedmonkey]

Shouldn't this be punted to 6.2.1 or 6.3?

[costdev]

The regression is quite a significant one. I think the revert needs to be done pre-6.2 release, otherwise we'll likely have quite a lot of support/trac tickets about this.

[azaozz]

Replying to spacedmonkey:

Thinking no. This is a ticket to fix/revert the regression for 6.2. After that is done the original #57394 can be milestoned for 6.3, or a new ticket can be opened to implement the checks needed when email is used as username.

There seem to be few edge cases. For example when a user changes their email address and the new email matches another user's username. Then there are additional complications as there weren't any similar checks in WP until now, so this has to be backwards compatible and not block editing of existing users even when their name/email is "incorrect".

[adamsilverstein]

Yes, looking back at this issue, I think reverting both [55358] and [55360] may be our safer option.

I also agree, lets revert this for now and then rework it to account for the issue raised here in 6.3.

[jorbin]

Agree with the reverting. Some initial checks sows that svn merge -c -55360 . && svn merge -c -55358 . will be a clean revert.

[hellofromTonya]

Once the changesets for #57394 are reverted, thinking this ticket can be closed and then conversation moved over to #57394 for consideration of how to solve its issue with added knowledge. Do you agree?

[audrasjb]

In 55584:

Login and Registration: Revert [55358] and [55360].

This reverts the changes implemented in [55358] and [55360].

Changeset [55358] was committed to prevent login name collision when one user registers with the email address user@example.com and a second user tries to register with the username user@example.com. However, it also introduced a potential backward compatibility issues for plugins that use wp_update_user(). When updating an existing user, it throws an existing_user_email_as_login error if the email address is also used for the user login, due to the code introduced in [55358].

This changeset removes the new scenario added in [55358] and [55360], restoring the wp_insert_user() function back to its previous state.

Props polevaultweb, audrasjb, costdev, peterwilsoncc, hellofromTonya, SergeyBiryukov, azaozz.
See #57967, #57394.

[audrasjb]

In 55585:

Login and Registration: Revert [55358] and [55360].

This reverts the changes implemented in [55358] and [55360].

Changeset [55358] was committed to prevent login name collision when one user registers with the email address user@example.com and a second user tries to register with the username user@example.com. However, it also introduced a potential backward compatibility issues for plugins that use wp_update_user(). When updating an existing user, it throws an existing_user_email_as_login error if the email address is also used for the user login, due to the code introduced in [55358].

This changeset removes the new scenario added in [55358] and [55360], restoring the wp_insert_user() function back to its previous state.

Props polevaultweb, audrasjb, costdev, peterwilsoncc, hellofromTonya, SergeyBiryukov, azaozz.
Reviewed by hellofromTonya.
Merges [55584] to the 6.2 branch.
See #57967, #57394.

[hellofromTonya]

The regression is now fixed with the reverts. Closing this ticket as fixed.

What about the patch work done in this ticket?

The knowledge gained in this ticket should be considered in scope for #57394. Noted in that Trac ticket. So the patch here can continue in that ticket.