Use email verification when changing user email addresses

Patch doesn't apply for 9.5.2

Patch doesn't apply for 9.5.2

Patch doesn't apply for 9.5.2

Why bother? It has been 16 years...

Clearly this tells me that this just a "nice to have" feature that most people are living happily without.

Also, it seems there are a lot of hoops, walls and gates to jump through to get this into core.

This should've been moved into contrib or scrapped all together a long time ago.

Re-rolls should be done for the latest Core branch, so 10.1.x, not 9.5.x.

Just because an issue has been around for a long time, and still has some work to do, doesn't make it any less valuable or worthy of inclusion in Core. If this functionality is needed for a site there are already contrib modules that will do this.

Re-rolled patch at #395 against 10.1.x. Please review

The last submitted patch, 409: 85494-409.patch, failed testing. View results →

Updated the version information in the related CR and also triggered a new build on #409 because it might have failed for transient errors before.

Unfortunately, it also seems to me that the latest patch still solves this issue on the form level only and does not cover other presentation layers, like APIs. I searched for "JSONAPI" in this thread and I have only found that @larowlan was asked about that in #334 only... even though it is a fundamental building block of Drupal core now and there was an API first initiative.

Added #344 to remaining tasks to have a clear answer for that.

Why bother? It has been 16 years...

Clearly this tells me that this just a "nice to have" feature that most people are living happily without.

Let me gently disagree with this statement in #407. This is very much a requirement by enterprise companies and the fact that this is not OOTB in Drupal makes Drupal less appealing to them. Selling this as custom development does not sound good to them.

Being that said, we have also developed our custom solution once or twice, it is not perfect either, therefore it does not worth sharing either. Plus the customer owns the code.

• fix #334 reference
• convert comment references to links with dreditors help

There is a new level of indent in `user.schema.yml` so I had to adjust the spacing for the `user.mail` changes to make this apply to 9.5.4.

Updated patch for 9.5 support

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled patch #417 for 11.x branch, please review it.

Thanks!

Fixed custom command failure #421. Please review

Fixed CCF issues, Please have a look.

The last submitted patch, 423: 85494-423.patch, failed testing. View results →

Fixed fail test cases, Please have a look.

larowlan → changed the visibility of the branch 85494-use-email-verification-9.3.x to hidden.

larowlan → changed the visibility of the branch 85494-use-email-verification-9.4.x to hidden.

larowlan → changed the visibility of the branch force-rebase to hidden.

Updating remaining tasks, hiding patches

Next steps are JSON:API and a reroll.

All other UX followups can be improved incrementally per #360

Locally, I apply the diff to 11.x and fixed the patch rejections from 5 files,

./core/modules/user/user.post_update.php.rej
./core/modules/user/src/AccountForm.php.rej
./core/modules/user/src/AccountSettingsForm.php.rej
./core/modules/user/config/schema/user.schema.yml.rej
./core/modules/user/user.module.rej

They were all straightforward except for AccountSettings form. That needed extra work due to 📌 Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed . The it failed spell check and I changed usages of E-mail and Please. I also learned that the AccountSettings form does not have a test and I did not look for one.

To do because of the changes I made to rebase.

• Is there an issue for adding a test of AccountSettings Form? if not, make one.
• Review all comments
• Review all user facing text to make sure it aligns with the Usability review.

There is a test of the Account settings form and it is failing.

Account settings form has a test, and that was failing due to the changes here, so updated that test with the new settings. Change to Account settings form for two fields that needed to use #config_target

Fixed the autowire test and added some type hints and constructor property promotion while I was at it

larowlan → changed the visibility of the branch 85494-use-email-verification to hidden.

Pushed some minor cleanups.

Left a review from a security POV

Added new tasks to the issue summary as follows:

1. I don't think we should be cloning the user and setting the email on it, it takes one contrib hook to call $user->save() and the email change is permanent. This means not reusing user_mail_notify for sending the verification email, and a custom case in user_mail for this case. We should keep the account as is and pass an additional param for the new email.
2. The controller needs flood control/protection to prevent brute force. This is the same as \Drupal\user\Controller\UserController::resetPassLogin

Made what I hope are improvements to the comments and some cleanup. Then did the low handing fruit from @larowlan review.

I've made an initial attempt to add flood control. It passed a local test of one test class but then my rig broke and I need to stop for now so pushed as-is.

Rebased to 11.x, removed a few stray "revert" commits that had ended up in this branch.

Pushed a commit that fixes a new bug in MailChangeController. UserMailChangeTest was failing (in GitLab CI and locally) because it was triggering this fatal error:

Error: Call to a member function get() on null in Drupal\user\Controller\MailChangeController->page() (line 49 of core/modules/user/src/Controller/MailChangeController.php).

That gets the test passing locally. Let's see what GitLab CI thinks.

Leaving NW since there are still a lot of un-addressed MR threads I haven't gotten to, yet. Only trying to get back to green tests for now.

The MR branch had conflicts when rebasing to 11.x due to 🐛 Harden user_pass_rehash() against attack Fixed . Since this is a security hardening issue, and these details are so important, I'm attaching a file showing the rebase conflict, and how I decided to merge the two versions. Would be wise for someone else to review this and confirm I've done it right, such that both this issue and 3277003 are happy.

#440: I've checked the change, and searched for any similar areas in the full diff, looks good to me.

Double checked #440 as well and logic looks the same to me too.

Copying comms from slack about the fail in ##44

I think this might be a manifestation of the remaining point in my review regarding overloading of user_mail_notify
because the hash is being generated with the new email (because user_pass_rehash considers email now)
see line 145 of MailChangeController, we don't pass an email there, so it now uses $account->getEmail and because account is a cloned version of the user with the new email (which I said was dangerous) this is happening
pretty sure that validates my reservations about using user_mail_notify with a cloned version of the account
the risk of the temp email getting into places it shouldn't was a genuine fear it seems
but in the controller it uses the old email so the hash doesn't match

I checked the old MR and ported the three unresolved comments/question to the current MR. My apologies if that is repeating answered questions. I figured it was better to do that instead of me reading all the comments to get up to speed.

I did a bit of work on this. This is new stuff for me so I just did what made sense to me today. There are still unresolved comments to address but it would be good to review the changes I made.

InaW → changed the visibility of the branch 85494-use-email-verification to active.

I rerolled the patch from #409 🐛 Use email verification when changing user email addresses Needs work for 10.2.3

larowlan → closed merge request !437

Left a review, there are also some aspects of #279 that are yet to be addressed

We discussed this again in the monthly bugsmash meeting and resolved to move this to a feature request because:

a) it is adding new features
b) there is a contrib module for this

Involved in the discussion were catch, griffynh, Amber Hines Matz, quietone, longwave and myself.

Reroll #417 for drupal 9.5.11

I attempted to apply patch 85494-451-drupal9511_0.patch → to drupal 11.1.8 but it failed to apply.

From the IS:

Don't re-use user_mail_notify or mutate the email on a cloned version of the user in case someone calls ::save() on it

I think this would be straight forward to implement after 📌 Extract _user_mail_notify() into a user MailHandler Active .

Rebased for OOP hook conversion.

https://git.drupalcode.org/project/drupal/-/merge_requests/5773.patch can't be applied to d11.2 any more after the update of #454 🐛 Use email verification when changing user email addresses Needs work

Reroll Merge request !5773 for 11.2.3 without tests code.

What's the status of items from #85494-279: Use email verification when changing user email addresses → ? If those are in, we can set this back to Needs Review. Otherwise, we need to get them in. Also, please update the Remaining Tasks in the issue description. Thanks!