Opened 20 months ago
Closed 20 months ago
#57967 closed defect (bug) (fixed)
Regression: Username check introduced in WP 6.2 should allow updates to the same user
Reported by: | polevaultweb | Owned by: | 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 )
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)
Change History (30)
This ticket was mentioned in Slack in #core by polevaultweb. View the logs.
20 months ago
#2
@
20 months ago
- Component changed from General to Users
- Keywords needs-testing added
- Milestone changed from Awaiting Review to 6.2
#4
@
20 months ago
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?
#6
@
20 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
@
20 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?
#8
@
20 months ago
You're welcome @costdev. Just raised the PR here https://github.com/WordPress/wordpress-develop/pull/4257
#11
@
20 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:
- 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.
#13
in reply to:
↑ 10
@
20 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
@
20 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] introducedswitch_to_user_locale()
, but I don't see a correlation to go/no-go for update/insert user logic.
#15
@
20 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.
#17
@
20 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
@
20 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 after they used an email as a username or their 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".
This ticket was mentioned in Slack in #core by jorbin. View the logs.
20 months ago
#21
@
20 months ago
Agree with the reverting. Some initial checks sows that svn merge -c -55360 . && svn merge -c -55358 .
will be a clean revert.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
20 months ago
#27
@
20 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.
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.