Make WordPress Core

#57967 closed defect (bug) (fixed)

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

Reported by: polevaultweb's profile polevaultweb Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: Users Keywords: has-patch has-unit-tests needs-testing changes-requested fixed-major
Focuses: Cc:

Description (last modified by hellofromTonya)

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.

Attachments (3)

57967.patch (795 bytes) - added by polevaultweb 11 months ago.
57967_with_unit_tests.patch (2.8 KB) - added by polevaultweb 11 months ago.
Patch with unit tests
57967_with_improved_unit_tests.patch (3.5 KB) - added by polevaultweb 11 months ago.

Download all attachments as: .zip

Change History (30)

This ticket was mentioned in Slack in #core by polevaultweb. View the logs.


11 months ago

#2 @audrasjb
11 months ago

  • Component changed from General to Users
  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.2

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.

#3 @costdev
11 months ago

  • Keywords needs-unit-tests added

#4 @polevaultweb
11 months ago

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

Version 1, edited 11 months ago by polevaultweb (previous) (next) (diff)

#5 follow-up: @audrasjb
11 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

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
11 months ago

Patch with unit tests

#6 @polevaultweb
11 months ago

  • Keywords needs-unit-tests removed

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

#7 @costdev
11 months ago

  • Keywords reporter-feedback added

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?

Last edited 11 months ago by costdev (previous) (diff)

#8 @polevaultweb
11 months ago

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

#9 @costdev
11 months ago

  • Keywords reporter-feedback removed

Thanks @polevaultweb!

#10 in reply to: ↑ 5 ; follow-up: @peterwilsoncc
11 months ago

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.

#11 @hellofromTonya
11 months ago

  • Summary changed from Username check introduced in WP 6.2 should allow updates to the same user to Regression: Username check introduced in WP 6.2 should allow updates to the same user

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:

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

Last edited 11 months ago by hellofromTonya (previous) (diff)

#12 @hellofromTonya
11 months ago

  • Description modified (diff)
  • Keywords has-unit-tests added

#13 in reply to: ↑ 10 @SergeyBiryukov
11 months ago

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.

#14 @hellofromTonya
11 months ago

  • Keywords changes-requested added

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.

#15 @polevaultweb
11 months ago

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.

Last edited 11 months ago by polevaultweb (previous) (diff)

#16 follow-up: @spacedmonkey
11 months ago

Shouldn't this be punted to 6.2.1 or 6.3?

#17 @costdev
11 months ago

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.

#18 in reply to: ↑ 16 @azaozz
11 months ago

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

Last edited 11 months ago by azaozz (previous) (diff)

#19 @adamsilverstein
11 months ago

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.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


11 months ago

#21 @jorbin
11 months ago

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

#22 @hellofromTonya
11 months ago

  • Keywords close added

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?

#23 @audrasjb
11 months ago

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.

#24 @audrasjb
11 months ago

  • Keywords fixed-major added

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


11 months ago

#26 @audrasjb
11 months ago

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.

#27 @hellofromTonya
11 months ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.