Users: Require confirmation for email changes made outside the profile screen - #13488
Open
techjewel wants to merge 9 commits into
Open
Users: Require confirmation for email changes made outside the profile screen#13488techjewel wants to merge 9 commits into
techjewel wants to merge 9 commits into
Conversation
…e screen. Splits `send_confirmation_on_profile_email()` into a function that sends the confirmation request and one that processes a confirmed change, then calls the former from `WP_REST_Users_Controller::update_item()`, so that a user changing their own email address over the REST API is asked to confirm it in the same way they are on the profile screen. Adds a `wp-login.php?action=confirmemail` route so a change started outside of the admin can be confirmed. This is the patch from WordPress#3813 rebased onto trunk, with no changes other than conflict resolution against the `$user_id` parameter added to `send_confirmation_on_profile_email()` in 7.0.3. See #57413.
`send_user_email_change_confirmation_process()` uses `$wpdb` in the Multisite branch, but the code was moved out of `wp-admin/user-edit.php`, where `$wpdb` was already in scope at the top level, so the function had no `global $wpdb;` declaration. Confirming an email change on Multisite called a method on `null`. Also guards the lookups the function performs: an unknown user ID, a missing `_new_email` value, and a `wp_update_user()` failure are all now reported as a failed confirmation rather than assumed to have worked. Reading `['hash']` off a non-array `_new_email` was only silent because `hash_equals()` was reached through `! $new_email`. See #57413.
The split dropped three things that `send_confirmation_on_profile_email()` did before it: * The `0 === $current_user->ID || $current_user->ID !== (int) $user_id` guard. Without it, `get_user_by( 'id', 0 )` returns `false` on a logged-out request and `$user->user_email` fatals on PHP 8. * Restoring `$_POST['email']` to `addslashes( $current_user->user_email )` when the address is rejected, added in 7.0.3, so a rejected change does not fall through to `wp_update_user()`. * Comparing the current and submitted addresses strictly. `send_user_email_change_confirmation_email()` now also returns early when it is not handed a user that exists, since it is a public function that callers other than the two in core may reach. See #57413.
…InCE tags. `send_user_email_change_confirmation_process()` does not send anything, so it is renamed to `confirm_user_email_change()`, which says what it does and matches the `confirmemail` action it backs. The `user_email_confirmed` action moves from `wp-login.php` into the function, so it fires for both confirmation routes rather than only the one. Previously a change confirmed from the profile screen fired nothing. Replaces the `@since x.x` placeholders with 7.2.0 and expands the function documentation. See #57413.
…er REST. `WP_REST_Users_Controller::update_item()` discarded a `WP_Error` from `send_user_email_change_confirmation_email()` and carried on, which would apply an address the confirmation step had just rejected. It now returns a 400 using the `rest_user_invalid_email` code the endpoint already uses for a rejected address. The empty check also becomes `is_string()`, matching the `email_exists()` check earlier in the same method, so that a non-string `email` is not silently skipped. In `wp-login.php`, a confirmation link followed while logged out sent the user to a login form that then dropped them on the dashboard, losing the confirmation. The redirect now carries the confirmation URL through `redirect_to`. The user ID and hash are also cast and sanitized before use, and the route exits rather than breaking, matching the `confirmaction` route above it. See #57413.
Adds the unit tests asked for in the PR, for the two functions the split introduced rather than only for the callers: * `send_user_email_change_confirmation_email()`: an unchanged address, an invalid address, an address already in use (and that it discards an earlier pending change), storing the pending change without touching the account, skipping the confirmation for an administrator editing another user, and the `should_send_email_for_email_change` filter, including the arguments it is passed. * `confirm_user_email_change()`: applying a pending change and firing `user_email_confirmed`, rejecting a wrong hash while leaving the pending change in place, no pending change, and an unknown user. Also covers the REST endpoint holding the change while applying other fields in the same request, an unchanged address, and the filter turning the confirmation off. See #57413.
…ail. wp_update_user() mails a "Notice of Email Change" to the old address on every change, so asserting that no mail at all was sent failed whenever the change was applied immediately. The assertion is now that nothing was sent to the new address asking for confirmation. See #57413.
`email_exists()` is not case sensitive, so routing the profile screen and the REST API through the same check made correcting the case of your own address fail: on REST as a 400 from `rest_user_invalid_email`, breaking the behaviour added in #44672, and on the profile screen as "The email address is already used." A change of case is the same mailbox, so it is now not a change that needs confirming and is applied directly. This matches `wp_update_user()`, which guards its own `email_exists()` check with the same `strcasecmp()` comparison. See #57413, #44672.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
`send_user_email_change_confirmation_email()` is documented as returning `true|WP_Error|null`, but used bare `return;` statements, which PHPStan reports as `return.empty` and, separately, as the null in the return type never being used. Returning `null` explicitly says what the function means and keeps both out of the baselines, which exist to be emptied rather than added to. Moving the Multisite branch out of `wp-admin/user-edit.php` also halves the baselined `variable.undefined` count for `$wpdb` in that file, from 12 to 6. That entry is what the missing `global $wpdb;` in the moved code would have been reported as, had the new function not been exempt by being new. See #57413.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trac ticket: https://core.trac.wordpress.org/ticket/57413
This is a refresh of #3813 by @dd32, open since January 2023. That branch is on his fork so I couldn't push to it. The first commit here is his patch rebased onto trunk with his authorship intact, and everything after it is a fix, so the diff against that commit is exactly what changed. Props dd32.
What the original patch does
WP_REST_Users_Controller::update_item()callswp_update_user()directly, so a user changing their own email address over REST is never asked to confirm it. The same change on the profile screen is held in the_new_emailuser meta until they follow a link sent to the new address. The patch splitssend_confirmation_on_profile_email()into a function that sends the confirmation and one that processes a confirmed change, calls the first from the REST controller, and adds awp-login.php?action=confirmemailroute so a change started outside the admin can be confirmed.What needed fixing
Fatal error on multisite.
send_user_email_change_confirmation_process()uses$wpdbin the multisite branch but has noglobal $wpdb;. The body was moved out ofwp-admin/user-edit.php, where$wpdbwas already in scope at the top level, so confirming an email change on multisite called a method on null. Putting theglobalback is a one line fix, but nothing caught it, so the new tests cover it: taking it out again turnsTests_User::test_confirm_user_email_change_applies_the_pending_changeintoUndefined variable $wpdbon the multisite run.Regression against #44672.
email_exists()is not case sensitive, so routing both paths through one check meant a user correcting the case of their own address was told it was already in use. That's a 400 on REST and an error on the profile screen.test_update_item_existing_email_casefails on the unmodified patch. A change of case is the same mailbox, so it's no longer treated as a change that needs confirming, matching thestrcasecmp()guardwp_update_user()already applies to its ownemail_exists()check.Profile screen behaviour lost in the split. The
0 === $current_user->ID || $current_user->ID !== (int) $user_idguard was dropped, soget_user_by( 'id', 0 )returns false on a logged out request and$user->user_emailfatals on PHP 8. Theaddslashes()restore of$_POST['email']on a rejected address, added in 7.0.3, was dropped too.Rejected addresses ignored over REST.
update_item()discarded aWP_Errorfrom the confirmation function and carried on, applying an address the confirmation step had just rejected. It now returns a 400 with therest_user_invalid_emailcode the endpoint already uses.Confirmation lost when logged out. Following a confirmation link while logged out redirected to a bare login form, dropping the user on the dashboard. The redirect now carries the confirmation URL through
redirect_to.Other changes
send_user_email_change_confirmation_process()does not send anything, so it's renamed toconfirm_user_email_change(). @dd32 asked for a second opinion on the naming in the PR.user_email_confirmedaction moves fromwp-login.phpinto that function, so it fires for both confirmation routes rather than only one. A change confirmed from the profile screen previously fired nothing.@since x.xplaceholders filled in, and the function and filter documentation expanded.send_user_email_change_confirmation_email()returns null explicitly rather than using barereturn;, which PHPStan reports asreturn.empty. The baselines are for emptying, so this seemed better than adding a new file to them.should_send_email_for_email_changefilter and the arguments it receives, applying a confirmed change and the action it fires, a wrong hash, no pending change, and an unknown user. Plus REST coverage for other fields applied while the email change is pending, an unchanged address, and the filter turning the confirmation off.Not changed
The confirmation hash is still
md5( $email . time() . wp_rand() ), carried over from the existing code, and_new_emailstill never expires. Both are worth a look, and the first overlaps #12285, but neither is this ticket.wp_xmlrpc_server::wp_editProfile()has the same gap and is left alone pending a scope decision on the ticket.Testing
Single site and multisite, against trunk:
--group 57413, 18 tests, 62 assertions--group 16470,--group 44672,--group 40015passWP_Test_REST_Users_Controller, 136 tests, 960 assertionsTests_User, 133 tests, 452 assertionsphpcsreports no new errors or warnings on the changed filesphpstanreports no errorsThis 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.