Use email verification when changing user email addresses

LOL! Why single me out :) I'm innocent, I didn't do anything, I promise!

Can the code be extended for deal with another situation where the user's email is not confirmed: when new user accounts must be reviewed by the admin before being set active. I have had this happen on a Drupal site. If the settings is that the admin must authorized new accounts, the admin may waste time reviewing new accounts created by robots or by a human spammer with fake emails. It is not always obvious which account is bona-fide, which is fake. The admin shouldn't have to make a decision on an account with an invalid email (user error is also possible).

The workflow should be:
1) user creates account
2) user confirms email is valid (where part of your code is relevant)
3) admin authorizes (or not) the account.

The following ancient proposal is not necessarily good, but it may give some ideas for a better systematic approach: http://drupal.org/node/64861.

My point is that your new function user_change_mail() should be user_confirm_mail() so that it is general enough, and reusable in the different scenario I've exposed.

I hope you don't regret that I DID follow up with "the not so easy bit" ;)

Your 'sneakiness' worked (and I don't mind it, so don't worry) :)

I know different issues must be kept separate, so I perfectly agree with your point.

My question still stand, though: is there a way to make your new function more general so it can be used for this bug report and reused for other separate issues later?
How difficult would it be to reuse existing code and make it more general to reuse in every situation when we want a mail check?

going to test this later.

Some t calls in #7 need work:

watchdog('user', t('User %name used one-time email change link at time %timestamp.', array('%name' => "<em>$account->name</em>", '%timestamp' => $timestamp)));

Afaik, % calls theme_placeholder, additional em-tags are not necessary. In the mail t with %-variables will get you em-tags in the plaintext mail.

updated patch attached. this addresses many issues:

1. user admins should not trigger a confirmation email
2. if user changes their password, the new password must be used in the hash
3. error messages missing 'error' status
4. unnecessary elements in $variables array
5. email string substitutions should use !, not %
6. drupal_goto after confirmation needed to be more intelligent
7. cleaned up wording of the confirmation email

patch has been pretty thoroughly tested on a clean HEAD site, and seems to work perfectly.

Excellent work, Chad!

We shouldn't set 'X-Mailer' => 'Drupal'. AFAIK, drupal_mail already does that ...

I'll provide a more in-depth review shortly.

adding to queue...

@Dries: looks to me like we don't need that header array at all there.

attached patch removes it. anything else we need to do here?

This patch needs work. Punctuation is not consistent; some sentences end with a point, other don't. Also, the URL scheme is awkard.

Restting a password happens at ?q=user/reset. Restting an e-mail adress happens at ?q=user/change/mail. Maybe that should be ?q=user/reset-password and q?=user/reset-mail.

I haven't tested the patch yet but it is not clear what happens as long the new e-mail adress hasn't been confirmed. What are the implications?

okay, i believe i've fixed up the last of the problems:

• corrected bad/inconsistent punctuation, particularlry missing periods and inconsistency in the use of the word e-mail
• changed the callback name to user/change-mail. user/reset-mail is not really an accurate name
• changed the mail key to user_change_mail. mail_change_mail_send seemed awfully confusing

tested the mail change and password reset functionalities on a clean HEAD site, and both are working perfectly AFAICT. it would be great if we could get one more person to test them both...

Tested on fresh head:

1) Email changed only when the activation link is clicked. Did not wait to see if the link is disabled after the timeout though
2) Reset password works as it normally does

I noticed that in the confirmation mail that was sent when the email is changed, the From field is the user's old email address. Is this intentional? Or should the mail come from the site's email address?

per chx's consult, changed $from to the site mail.

Just before the beta. To sum up: what's the point in the initial email confirmation if I can change it to foo whenever?

patch broken by the capitalization commit. updated patch attached. no changes in code functionality whatsoever--just fixed up the conflict and updated the capitalization.

chx: note that we store the initial e-mail address, even if you chose to change it later. $user->init should always be valid, and we should have some sort of audit trail.

(We've never had this kind of functionality, and all of a sudden it is critical? The critical status is debatable, IMO.)

Note that this is the child of the contact spam issue which is indeed a very serious one -- hence the critical.

I think there is room for improvement:

1. I wonder what my mother will think when she reads: "The account identification numbers did not match.
2. "There was a problem with this one time link. Please try again.'" Try what again? And what difference could it make?

per Dries' request, i have reworked the user messages in this patch.

Is there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.

"If it is not used, your e-mail address change will not be processed." might want to say what will happen instead of what won't happen- "If not used, your email address at [site] will not change."

Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?

s there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.

per dries' comment in #15 i'm leaving this change in.

If not used, your email address at [site] will not change.

fixed.

Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?

yeah, seems like a good idea. added.

i question the need for this. it is trivially easy to forge the from address of any email, not just ones sent from drupal. if proper authentication is our goal, i suggest we add a header like X-Drupal-UID and send the uid there. Or send the $user->init email address. Those are both authoritative/verified.

I don't really object to this going in, but it doesn't buy a whole lot and the critical status is shaky at best.

actually, contact.module already states the UID of the sender so we are covered there. thanks for illustrating that, boggieman.

patch updated. functionality is the same.

Patch no longer applies; re-rolled with updates to hook_menu; untested.

-K

no time to put into this anymore. any takers?

I'd say this is critical. We need to be sure that a user owns the email address registered for their account, otherwise what's the point confirming it in the first place?

Also, without this confirmation, a user could change their registered email address to someone else's, causing all kinds of problems (content associated with a person, unable to create a new account, etc).

Hello everyone, has this problem been solved in drupal 5.x versions?

Changing back to 6.x-dev. I doubt this will be changed for 5.x.

This can happily live in a contrib. Viva la form_alter!

No surprise, but this fails against HEAD:

$ patch -p0 < user-mail-change.patch
(Stripping trailing CRs from patch.)
patching file modules/user/user.module
Hunk #1 FAILED at 801.
Hunk #2 FAILED at 1256.
Hunk #3 succeeded at 1381 (offset 106 lines).
Hunk #4 succeeded at 1536 with fuzz 1 (offset -14 lines).
Hunk #5 FAILED at 1763.
3 out of 5 hunks FAILED -- saving rejects to file modules/user/user.module.rej

Re: #40: A user is not able to change her email address to one that is already used by another user.

As chx said: "This can happily live in a contrib" and here is the proof:

http://drupal.org/project/email_confirm

feedback is welcome

I withdraw my objection earlier. This reallty does make perfect sense. Considering where we are in the release cycle, critical is warranted here.

I should elaborate on my use case. My client is is borrowing an idea from Facebook about authorization. Specifically, if you have a verified @harvard.edu email address then you belong to harvard group and are allowed to see certain premium nodes and so on. Naturally, we need to verify email address changes so that people don't just switch into harvard group.

So, I hope the need has been proven and someone can move on to polishing and committing the patch.

Should this patch get re-rolled for D7, please have it call hook_user('update') - for Moshe's use case, other modules may need to know when the email address has been changed and take action based on the new address.

FYI - Decscription / Feature request for the email_confirm module mentioned in #46:

"Account hijack prevention management (prerequisites)"
http://drupal.org/node/475264

+1 subscribing

This seems like a good core feature to have for D7. I hope it doesn't die here.

+1 subscribe

"feature request" and "critical" are mutually exclusive.
maybe priority-major is warranted.

7.x is feature frozen anyway.

Changing to major as per tag.

Tag update

subscribing

Subscribing. Some of this functionality should, IMHO, live in core. I've form altered the registration and account forms too many times to get this/similar functionality :-/

Unless I'm totally mistaken, this feature is already in D7, so this is fixed.

Wait... I thought the issue was about actually confirming the email address itself, not the user's *intention* of changing the address. E.g. If a user changes his email, then the user would get a confirmation email sent to the new email address, and the new email address wouldn't be used for $user->mail until its confirmed.

Right? That's kind of what I read in most of the posts here... I think.

Oops! You are right, sorry. :)

Totally the wrong place to post this. That's what you get when you post while asleep. Sorry folks

I use LT along with http://drupal.org/project/email_confirm which deals with this issue.

My problem is when a user changes their email address BEFORE verifying the old one. This function is needed as a second chance for users to use a different email address and activate their account. But even though email_confirm is one form of email confirmation, LT is nor aware of it or that it already verified the address. I think I will write something for email_confirm to integrate with LT.

How do force account verification from PHP?

I support this request, as not having a valid user email address at all times is also a potential legal issue.
Users could post illegal content and there would be no ways of persecuting them.

It's not hugely problematic, as Drupal retains the original, validated address in the "init" field. But that address might be outdated and no longer functional.

This functionality should stay in contrib, email could be changed by custom auth provider and it's not really useful for now. Let's close this issue

IMO, this is important to have in core. Absolutely needed.

Agreed. Some even thought to make it critical -- while that's debatable, it's definitely a hole that needs to be fixed and should be part of core's default system.

There's a potential security issue here, user password may somehow happened exposed -- simple password, same password with other low secure sites etc.

Then people get the password can have full control of the account -- change password, email etc. And you'll never can get back your account!

So, we should at least let the hackers not able to change email easily. User needs to have email change confirm in the old email address and this features is very common in websites.

This feature should be able to port to D7, as it's a potential security issue!

Is there a contrib module that provides this functionality for Drupal 7? Thanks

http://drupal.org/project/email_confirm - have a beta release for testing on d7.

@rogical - since Drupal doesn't store the password in plaintext I don't see how a password change could expose the user's current password. Further, I don't see how someone can be tricked into setting their email to that of an attacker.

Tagging to increase awareness.

I rather see a legal problem here and not a security problem. As a web site owner, you usually wish to maintain a valid email address for each of your users, in case they do anything illegal. So users shouldn't be allowed to change their address to an unconfirmed one.

You could also defend it from a usability point, since the verification protects the users from misspelled addresses which would prevent them from receiving updates.

This is the re-roll against 8.x from #37 and a working patch.

This bug has been in existence since D5.x (2006) it's managed to exist before, during and after code freeze on D6, D7 and D8. Whilst the need for debate is understood this is a pretty basic and low level feature. The use case for it has been iterated and re-iterated many times on this issue queue and whilst a contrib module does exist the case is STILL there for this to appear in core.

There are, from what I can see, no blockers to this becoming a core feature so the time has come to ask why, after 7 years, this is not already a feature?

Is this ever likely to appear in core?

#69 Posted by arnoldbird on February 23, 2012 at 6:55pm

Is there a contrib module that provides this functionality for Drupal 7? Thanks

Hi, may out of topic but may more around questioned by you one of my bookmarks

knowledge-base/contributed-modules-securing-your-drupal-site
(Submitted by Ben Jeavons on Thu, 09/08/2011 - 15:53)
[Among the thousands of modules on drupal.org there are over 100 in the security category. Unfortunately some of those are abandoned or inaccurately tagged. We've looked at every module and compiled this resource to help you understand the security-related community modules available. Not all modules provide security exactly, some are about hardening your site against weaknesses and others are about monitoring and reporting abuses (....)]

.. or link drupal-7-user-login-and-registration

There is, at least in part. The module is: https://drupal.org/project/email_confirm

Related to #2014185: Check usernames that are email addresses more rigidly, only allow if matches email

This sounds like a feature request contrib should provide and probably can.

I think this is an omission in core, for the reasons outlined in the issue summary. I don't think it should be left to contrib.

Don't see why this is a feature request. Smells like a bug to me. Today, at least. Maybe not 9 years ago when Drupal was a hobbyist project.

In addition to be a slow-spam method, it has UX-implications and/or potential security risk: If you by mistake enter a non-functional email, you won't receive reset password info if you forget it. Your account is effectively lost and you need to contact support. And who is support to verify that whoever calls is the rightful owner? It's just a random email nobody owns.

The email address is more and more used as the primary ID of user accounts. The concept of user name is a fading thing. That Drupal 8 will come out with a ID-mechanism this flawed is, uhm, not good. Yes, contrib can do this, but having this functionality in core will assert its quality and assure that the user is who it claims to be.

+1 this should be fixed. It is clearly a security concern, especially for sites that integrate with 3rd party services that rely on a verified email address.
No idea why Drupal would verify a new user email address, but not reverify when they change it.

Just a reminder to other users that the patch in #90 is for D8 and not D7.

@pjonckiere: Regarding the need for a follow-up, it would be great to get this fixed in D7/D6 as well. I have actually had a couple users change their emails to harass people on my site, so it would be awesome to fix this.

Patch works great for me.
+1 and absolutely important to also fix in D7 :)

1. +++ b/core/modules/user/config/install/user.mail.yml
   @@ -4,6 +4,12 @@ cancel_confirm:
   +  body: "[user:name],\n\nA request to change your e-mail address has been made at [site:name]. In order to complete the change you will need to follow the instructions sent to your new e-mail address within one day."
   ...
   +  body: "[user:name],\n\nA request to change your e-mail address has been made at [site:name]. You need to verif the change by clicking on the link below or copying and pasting it in your browser:\n\n[user:mail-change-login-url]\n\nThis is a one-time URL, so it can be used only once. It expires after one day. If not used, your e-mail address at [site:name] will not change."
   

   shouldn't "one day" be "24 hours" to be more precise?

   Also shouldn't the mail_change_notification contain the new e-mail address? If it was a legitimate change, the user already knows their own e-mail address. Else they have some clue as to who tried to change their email address.

2. +++ b/core/modules/user/config/schema/user.schema.yml
   @@ -20,6 +20,12 @@ user.settings:
   +          label: 'Verify user of an email address change'
   

   How about: Require email verification when a user changes their email address

3. +++ b/core/modules/user/src/AccountForm.php
   @@ -388,13 +388,31 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    $user = \Drupal::currentUser();
   ...
   +    if(!$account->isNew() && $account->getEmail() !== $new_email && !$user->hasPermission('administer users')) {
   

   Let's just do \Drupal::currentUser()->hasPermission();

4. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -312,6 +312,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => t('Edit the e-mail messages sent to users old mail address who change mail address.') . ' ' . $email_token_help,
   

   Edit the e-mail message sent to a user's old e-mail address when the e-mail address is changed.

5. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -312,6 +312,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => t('Edit the e-mail messages sent to users new mail address who change mail address.') . ' ' . $email_token_help,
   

   Edit the e-mail message sent to a user's new e-mail address when the e-mail address is changed.

6. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -138,6 +138,55 @@ public function resetPass($uid, $timestamp, $hash) {
   +   * Returens the user change email page.
   

   Returns

7. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -138,6 +138,55 @@ public function resetPass($uid, $timestamp, $hash) {
   +        drupal_set_message(t('You have tried to use a one-time e-mail change link for %account that has expired--your change of e-mail request was not completed. Please visit your account edit page if you wish to attempt the change again.', array('%account' => $account->name)), 'error');
   

   expired -- your

8. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -138,6 +138,55 @@ public function resetPass($uid, $timestamp, $hash) {
   +        drupal_set_message(t('There was a problem verifying your change of e-mail request. Please visit your account edit page and attempt the change again'), 'error');
   

   Let's inform the user a little more about the actual problem.

9. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -138,6 +138,55 @@ public function resetPass($uid, $timestamp, $hash) {
   +        drupal_set_message(t('Your e-mail address is now %mail.', array('%mail' => $new_email)));
   

   Your e-mail address has been changed to %email.

10. +++ b/core/modules/user/user.module
    @@ -931,12 +957,14 @@ function user_mail($key, &$message, $params) {
    + *   - email: The email address of the user.
    

    The user's email address.

Updated patch from #90 to current head.

Remarks from #96:
1. To stay consistent with the other user emails, (being cancel_confirm and password_reset), I thinks its better to keep “one day” instead of 24 hours.
2. Ok
3. Ok
4. Changed it to "Edit the email messages sent to users' old email address when the email address is changed.” to stay consistent with the other descriptions.
5. Changed to "Edit the email messages sent to users' new email address when the email address is changed.” for consistency.
6. Ok
7. Ok
8. Ok
9. Ok
10. Ok

I added an extra warning message when an email is changed with validation, to ensure the user knows an action is required.

Bugs which still needs to be addressed:
- Provide a token to show the new email address in the mail_change_notification email. Since we are not storing the old emailadres at this moment, there's no quick fix
- When a user is not logged in while clicking the email validation change link, an error occurs.

Correct patch now, remarks see #97

Setting to needs review so the testbot can have a look at #98.

Bugs which still needs to be addressed:
- Provide a token to show the new email address in the mail_change_notification email. Since we are not storing the old emailadres at this moment, there's no quick fix
- When a user is not logged in while clicking the email validation change link, an error occurs.

One thing to be careful of here is that if Drupal actually verified the email addresses of all accounts, the privacy problems discussed in the issues I'm linking to would become significantly more severe.

I rerolled the patch. It is my first rerroll. So, if I did something wrong, please let me know. I will try to fix asap. Thank you!

@LOBsTerr Well done! Achievement unlocked! I noticed only one thing, looking at this side-by-side with the prior patch:

+++ b/core/modules/user/config/install/user.mail.yml
@@ -2,8 +2,14 @@ cancel_confirm:
-  body: "[user:display-name],\n\nA request to reset the password for your account has been made at [site:name].\n\nYou may now log in by clicking this link or copying and pasting it into your browser:\n\n[user:one-time-login-url]\n\nThis link can only be used once to log in and will lead you to a page where you can set your password. It expires after one day and nothing will happen if it's not used.\n\n--  [site:name] team"
-  subject: 'Replacement login information for [user:display-name] at [site:name]'
+  body: "[user:name],\n\nA request to reset the password for your account has been made at [site:name].\n\nYou may now log in by clicking this link or copying and pasting it to your browser:\n\n[user:one-time-login-url]\n\nThis link can only be used once to log in and will lead you to a page where you can set your password. It expires after one day and nothing will happen if it's not used.\n\n--  [site:name] team"
+  subject: 'Replacement login information for [user:name] at [site:name]'

This should not be in the patch (should have taken HEAD's version). Generally, when resolving conflicts, HEAD is "correct" and you have to carefully consider what the prior patch was doing in that place. Dreditor helps. You can easily sort out the thing above with a commit on your reroll branch then submit a new patch.

@cilefen - Thank you for information. I will fix it.

@cilefen - I have created an interdiff. I hope now it will be ok! I'm sorry the next time I will be more careful!

Renamed interdiff to be ignored by the Testbot

Rename interdiff one more. I missed "do-not-test" part :(

I dare mark it RTBC

+++ b/core/modules/user/config/install/user.mail.yml
@@ -2,8 +2,15 @@ cancel_confirm:
 password_reset:
+
   body: "[user:display-name],\n\nA request to reset the password for your account has been made at [site:name].\n\nYou may now log in by clicking this link or copying and pasting it into your browser:\n\n[user:one-time-login-url]\n\nThis link can only be used once to log in and will lead you to a page where you can set your password. It expires after one day and nothing will happen if it's not used.\n\n--  [site:name] team"

There is a random, unrelated newline here.

@cilefen - You are right. Nice catch. Missed. Fixed it in a new patch

+++ b/core/modules/user/config/install/user.mail.yml
@@ -4,6 +4,12 @@ cancel_confirm:
+mail_change_notification:
+  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. In order to complete the change you will need to follow the instructions sent to your new e-mail address within one day."
+  subject: 'E-mail change information for [user:display-name] at [site:name]'
+mail_change_verification:
+  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. You need to verify the change by clicking on the link below or copying and pasting it in your browser:\n\n[user:mail-change-login-url]\n\nThis is a one-time URL, so it can be used only once. It expires after one day. If not used, your e-mail address at [site:name] will not change."
+  subject: 'E-mail change information for [user:display-name] at [site:name]'

A hook_update_N() is needed to set the body and subject for these message for existing sites.

+++ b/core/modules/user/config/schema/user.schema.yml
@@ -20,6 +20,12 @@ user.settings:
+        mail_change_notification:
+          type: boolean
+          label: 'Notify user when email changes'
+        mail_change_verification:
+          type: boolean
+          label: 'Require email verification when a user changes their email address'

There does not seem to be a form element for a site admin to set these booleans, as there is with the "Account blocked" notification, for example.

Working on this @Drupalcamp Vienna

Fixed a few minor things, works for me now. I also added the update hook.

For me everything looks good: Updated logic to get user, to set an e-mail and update it only when it is updated, return result of redirect function.
Checked the update hook everything was set properly.

Since this contains form and string changes, moving to 8.1.x

I didn't do a full review, so leaving at RTBC isn't an indication either way of whether I think it's actually RTBC.

1. +++ b/core/modules/user/config/install/user.mail.yml
   @@ -4,6 +4,12 @@ cancel_confirm:
   +  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. In order to complete the change you will need to follow the instructions sent to your new e-mail address within one day."
   ...
   +  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. You need to verify the change by clicking on the link below or copying and pasting it in your browser:\n\n[user:mail-change-login-url]\n\nThis is a one-time URL, so it can be used only once. It expires after one day. If not used, your e-mail address at [site:name] will not change."
   
   +++ b/core/modules/user/config/schema/user.schema.yml
   @@ -20,6 +20,12 @@ user.settings:
   +          label: 'Notify user when email changes'
   ...
   +          label: 'Require email verification when a user changes their email address'
   
   @@ -61,6 +67,12 @@ user.mail:
   +    label: 'Change mail notification'
   ...
   +    label: 'Change mail verification'
   
   +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +      drupal_set_message($this->t('Your email address needs to be validated. Further instructions have been sent to your new email address.'), 'warning');
   
   +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -303,6 +303,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#title' => t('Email change notification'),
   ...
   +      '#description' => t("Edit the email messages sent to users' old email address when the email address is changed.") . ' ' . $email_token_help,
   ...
   +      '#title' => t('Email change verification'),
   ...
   +      '#description' => t("Edit the email messages sent to users' new email address when the email address is changed.") . ' ' . $email_token_help,
   
   +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +        drupal_set_message(t('You have tried to use a one-time email address change link for %account that has expired -- your change of email address was not completed. Please visit your account edit page if you wish to attempt the change again.', array('%account' => $account->name)), 'error');
   ...
   +        drupal_set_message(t('You are currently logged in as %user, and are attempting to confirm an email address change for %account, which is not allowed. Please log in as %account and initiate a new change of email request.', array('%user' => $user->name, '%account' => $account->name)), 'error');
   ...
   +        drupal_set_message(t('There was a problem validating the used link. Please visit your account edit page and retry changing your email address.'), 'error');
   ...
   +        drupal_set_message(t('Your email address has been changed to %mail.', array('%mail' => $new_email)));
   
   +++ b/core/modules/user/src/Tests/UserEditedOwnAccountTest.php
   @@ -43,5 +47,20 @@ function testUserEditedOwnAccount() {
   +    // Set a new email address for the user account.
   ...
   +    $this->assertTrue(count($user_mail), 'A verification email was sent to the user.');
   
   +++ b/core/modules/user/user.module
   @@ -593,6 +593,32 @@ function user_pass_reset_url($account, $options = array()) {
   + * Generates a unique URL for a one time email change confirmation.
   ...
   + *   A unique URL that provides a one-time email change confirmation for the
   
   @@ -940,6 +966,7 @@ function user_mail($key, &$message, $params) {
   + *   - email: The user's email address.
   
   @@ -1176,6 +1204,8 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
   + *   - 'mail_change_notification': Email change notification.
   + *   - 'mail_change_verification': Email change verification.
   
   +++ b/core/modules/user/user.routing.yml
   @@ -150,3 +150,14 @@ user.reset:
   +    _title: 'Change email address'
   

   We currently use "E-mail", not "e-mail", "email", or "mail", when referring the E-mail in human-readable strings.

2. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +    if(!$account->isNew() && $account->getEmail() !== $new_email && !\Drupal::currentUser()->hasPermission('administer users')) {
   +      $old_email = $account->getEmail();
   

   Move $old_email before if() and use it in if() condition. Also consider to name it $old_mail, $new_mail (we typically use "mail" as machine-readable name).

   Also you can replace \Drupal::currentUser() with $this->currentUser()

3. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -303,6 +303,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#title' => t('Subject'),
   ...
   +      '#title' => t('Body'),
   ...
   +      '#title' => t('Subject'),
   ...
   +      '#title' => t('Body'),
   

   For all t(...) inside this function consider replacing them with $this->t(...).

4. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +   * Returns the user change email page.
   

   Maybe "Provides a callback for user E-mail change page"?

5. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +   * @param string new_email
   

   Missed the '$' prefix.

6. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +  public function changeEmail($uid, $timestamp, $new_email, $hash) {
   

   Use $this->t(...) instead of t() for all translated strings inside this function.

7. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +    $user = \Drupal::currentUser();
   ...
   +      else if ($user->id() && $user->id() != $account->id()) {
   ...
   +      else if ($hash != user_pass_rehash($account, $timestamp)) {
   

   You don't need $user variable. Just use $this->currentUser or, better, $this->currentUser().

8. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +    // We need to set the new email here to validate the hash correctly, which is
   +    // created using the new mail adress. We only save the account if the hash matches.
   

   Lines should wrap at 80 characters.

9. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
   +    $current = $_SERVER['REQUEST_TIME'];
   

   We are not using directly this variable. Use the REQUEST_TIME constant instead.

10. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
    +      if($current - $timestamp > $timeout) {
    

    Space after "if".

11. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -139,6 +139,58 @@ public function resetPass($uid, $timestamp, $hash) {
    +      else if ($hash != user_pass_rehash($account, $timestamp)) {
    

    We are using the compact form "elseif" w/o space between.

12. +++ b/core/modules/user/user.install
    @@ -85,3 +85,16 @@ function user_install() {
    + * Update config for change mail notifications.
    

    "Updates", maybe?

13. +++ b/core/modules/user/user.install
    @@ -85,3 +85,16 @@ function user_install() {
    +  $mail_config->set('mail_change_notification.body',"[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. In order to complete the change you will need to follow the instructions sent to your new e-mail address within one day.");
    +  $mail_config->set('mail_change_notification.subject','E-mail change information for [user:display-name] at [site:name]');
    +  $mail_config->set('mail_change_verification.body',"[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. You need to verify the change by clicking on the link below or copying and pasting it in your browser:\n\n[user:mail-change-login-url]\n\nThis is a one-time URL, so it can be used only once. It expires after one day. If not used, your e-mail address at [site:name] will not change.");
    +  $mail_config->set('mail_change_verification.subject','E-mail change information for [user:display-name] at [site:name]');
    

    This I don't like. Is it possible to read to parse directly the YAML file and get the values instead hardcoding them?

14. +++ b/core/modules/user/user.module
    @@ -593,6 +593,32 @@ function user_pass_reset_url($account, $options = array()) {
    +function user_change_mail_url($account, $options = array()) {
    

    Use modern [] instead of array().

    I wonder if we cannot place this as static method in an external class. It's used rarely, why loading user.module?

We currently use "E-mail", not "e-mail", "email", or "mail", when referring the E-mail in human-readable strings.

Nope, "email" is correct - see https://www.drupal.org/drupalorg/style-guide/content#relatedwords

Ouch, sorry. I missed that.

The test could actually call the URL from the notification mail and make shure the email address is the new one afterwards.

1. +++ b/core/modules/user/config/install/user.mail.yml
   @@ -4,6 +4,12 @@ cancel_confirm:
   +  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. In order to complete the change you will need to follow the instructions sent to your new e-mail address within one day."
   +  subject: 'E-mail change information for [user:display-name] at [site:name]'
   ...
   +  body: "[user:display-name],\n\nA request to change your e-mail address has been made at [site:name]. You need to verify the change by clicking on the link below or copying and pasting it in your browser:\n\n[user:mail-change-login-url]\n\nThis is a one-time URL, so it can be used only once. It expires after one day. If not used, your e-mail address at [site:name] will not change."
   +  subject: 'E-mail change information for [user:display-name] at [site:name]'
   

   Per #137, let's revert to "email", "Email" form. Sorry again for confusing.

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,57 @@ public function resetPass($uid, $timestamp, $hash) {
   +   * Returns the user change email page.
   

   Maybe swap "change" with "email"? For me "Returns the user email change page" sounds better, but I'm not a native English speaker.

3. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,57 @@ public function resetPass($uid, $timestamp, $hash) {
   +    $current = REQUEST_TIME;
   ...
   +    if ($timestamp < $current) {
   +      if ($current - $timestamp > $timeout) {
   ...
   +      elseif ($timestamp > $account->getLastLoginTime() && $timestamp < $current) {
   

   Now, let's drop $current and use directly the constant.

4. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -139,6 +139,57 @@ public function resetPass($uid, $timestamp, $hash) {
   +        drupal_set_message(t('You have tried to use a one-time email address change link for %account that has expired -- your change of email address was not completed. Please visit your account edit page if you wish to attempt the change again.', array('%account' => $account->name)), 'error');
   ...
   +        drupal_set_message(t('You are currently logged in as %user, and are attempting to confirm an email address change for %account, which is not allowed. Please log in as %account and initiate a new change of email request.', array('%user' => $user->name, '%account' => $account->name)), 'error');
   ...
   +        drupal_set_message(t('There was a problem validating the used link. Please visit your account edit page and retry changing your email address.'), 'error');
   ...
   +        drupal_set_message(t('Your email address has been changed to %mail.', array('%mail' => $new_mail)));
   

   s/t()/$this->t().

   Also, use [] instead of array().

5. +++ b/core/modules/user/user.module
   @@ -593,6 +593,32 @@ function user_pass_reset_url($account, $options = array()) {
   +  $timestamp = $_SERVER['REQUEST_TIME'];
   

   Drop $timestamp, use REQUEST_TIME constant.

+++ b/core/modules/user/src/Controller/UserController.php
@@ -139,6 +139,57 @@ public function resetPass($uid, $timestamp, $hash) {
+  public function changeEmail($uid, $timestamp, $new_mail, $hash) {
...
+  }

+++ b/core/modules/user/user.module
@@ -593,6 +593,32 @@ function user_pass_reset_url($account, $options = array()) {
+function user_change_mail_url($account, $options = []) {
...
+}

@@ -947,6 +974,7 @@ function user_mail($key, &$message, $params) {
+    $replacements['[user:mail-change-login-url]'] = user_change_mail_url($data['user'], $options);

+++ b/core/modules/user/user.routing.yml
@@ -150,3 +150,14 @@ user.reset:
+user.change_email:
+  path: 'user/change-mail/{uid}/{timestamp}/{new_email}/{hash}'
+  defaults:
+    _controller: '\Drupal\user\Controller\UserController::changeEmail'

I wonder if we cannot create a new Drupal\user\Controller\ChangeEmailController. Then we put there the changeEmail() (maybe as changeEmailPage()?) page and also we move there the user_change_mail_url() as a static method called changeEmailUrl(). I don't like adding new procedural functions to *.module files that are parsed on every request.

EDIT: Also we can open a follow-up to do the same with reset password, in the future.

Because the interdiff is against #140, I cannot see the cause of test failures in #146, #148. And I'm so curious :)

@claudiu.cristea.

1) in comment 144 - As, you asked I change the new_email to new_mail

public static function changeEmailUrl($account, $options = []) {
    $langcode = isset($options['langcode']) ? $options['langcode'] : $account->getPreferredLangcode();
    $url_options = array('absolute' => TRUE, 'language' => \Drupal::languageManager()->getLanguage($langcode));
    return \Drupal::url('user.change_email', [
      'uid' => $account->id(),
      'timestamp' => REQUEST_TIME,
      'new_email' => $account->getEmail(), // THIS LINE!!!
      'hash' => user_pass_rehash($account, REQUEST_TIME),
    ], $url_options);
  }

2) in comment 146. I used the wrong namespace

use use Drupal\user\ChangeEmailController;

instead of

use Drupal\user\Controller\ChangeEmailController;

If you need I can create interdiffs. Was a little bit in hurry didn't check it properly :(

Ah, OK. Thank you for explanations. Minor changes. However, It's a recommended practice to create interdiffs on each patch agains the last patch, so that the reviewer understands exactly what was changed since the last patch.

Some more reviews:

1. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -303,6 +303,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t("Edit the email messages sent to users' old email address when the email address is changed.") . ' ' . $email_token_help,
   

   s/users'/user's

2. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   + * Contains \Drupal\user\Controller\UserController.
   

   ChangeEmailController?

3. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +use Drupal\Core\Controller\ControllerBase;
   

   Missed:

   use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
   

4. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +    $account = \Drupal::entityManager()->getStorage('user')->load($uid);
   

   Entity manager is deprecated but the parent ControllerBase already provides $this->entityTypeManager(). Let's use that. Also consider to a add a type hint comment above, telling to IDE that $account is UserInterface.

   /** @var \Drupal\user\UserInterface $account */
   

5. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +    // We need to set the new email here to validate the hash correctly,
   +    // which is created using the new mail adress. We only save the account
   +    // if the hash matches.
   

   The wrapping is still incorrect. "which" can go up 1 line. Also words from the 3rd line can go up.

6. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +        drupal_set_message($this->t('You have tried to use a one-time email address change link for %account that has expired -- your change of email address was not completed. Please visit your account edit page if you wish to attempt the change again.', array('%account' => $account->name)), 'error');
   ...
   +        drupal_set_message($this->t('You are currently logged in as %user, and are attempting to confirm an email address change for %account, which is not allowed. Please log in as %account and initiate a new change of email request.', array('%user' => $user->name, '%account' => $account->name)), 'error');
   ...
   +        drupal_set_message($this->t('Your email address has been changed to %mail.', array('%mail' => $new_mail)));
   

   Forgot to convert array() to [] :)

7. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +      // Deny access if the timestamp passed is in the future.
   +      throw new AccessDeniedHttpException();
   

   We need to test this.

8. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +   * @param object $account
   

   This is wrong. It should be type-hinted to \Drupal\user\UserInterface, not object.

9. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +   *    URLs. If langcode is NULL the users preferred language is used.
   

   This line indent is wrong "URLs" should left-align with "langcode".

10. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,92 @@
    +   * @return
    

    Missed the type-hint.

11. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,92 @@
    +  public static function changeEmailUrl($account, $options = []) {
    

    $account & $options must be type-hinted.

12. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,92 @@
    +    $url_options = array('absolute' => TRUE, 'language' => \Drupal::languageManager()->getLanguage($langcode));
    

    Let's use the modern [] instead of array(). Also we have the service already in, let's use it: replace \Drupal::languageManager() with $this->languageManager().

13. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,92 @@
    +    return \Drupal::url('user.change_email', [
    

    \Drupal::url() is deprecated. Use Url::fromRoute() instead.

14. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -71,7 +71,7 @@ public static function create(ContainerInterface $container) {
    -   * Returns the user password reset page.
    +   * Provides a callback for user email change page.
    
    @@ -112,7 +112,6 @@ public function resetPass($uid, $timestamp, $hash) {
    -    $current = REQUEST_TIME;
    
    @@ -120,11 +119,11 @@ public function resetPass($uid, $timestamp, $hash) {
    -      if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {
    +      if ($user->getLastLoginTime() && REQUEST_TIME - $timestamp > $timeout) {
    ...
    -      elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
    +      elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= REQUEST_TIME) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
    
    @@ -210,7 +209,7 @@ public function confirmCancel(UserInterface $user, $timestamp = 0, $hashed_pass
    -        drupal_set_message(t('You have tried to use an account cancellation link that has expired. Please request a new one using the form below.'), 'error');
    +        drupal_set_message($this->t('You have tried to use an account cancellation link that has expired. Please request a new one using the form below.'), 'error');
    

    All changes to UserController are unrelated. Remove them from this patch.

I'm sorry, I didn't about the patches. I thought, the last successful patch must have been used.

No problem :)

And more...

1. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +        drupal_set_message($this->t('You have tried to use a one-time email address change link for %account that has expired -- your change of email address was not completed. Please visit your account edit page if you wish to attempt the change again.', array('%account' => $account->name)), 'error');
   ...
   +        drupal_set_message($this->t('You are currently logged in as %user, and are attempting to confirm an email address change for %account, which is not allowed. Please log in as %account and initiate a new change of email request.', array('%user' => $user->name, '%account' => $account->name)), 'error');
   

   There's no $account->name but only $account->getAccountName(). Also, there's no $user object.

2. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +        drupal_set_message($this->t('Your email address has been changed to %mail.', array('%mail' => $new_mail)));
   

   array() -> [].

3. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,92 @@
   +      if (REQUEST_TIME - $timestamp > $timeout) {
   ...
   +      elseif ($this->currentUser()->id() && $this->currentUser()->id() != $account->id()) {
   ...
   +      elseif ($hash != user_pass_rehash($account, $timestamp)) {
   ...
   +      elseif ($timestamp > $account->getLastLoginTime() && $timestamp < REQUEST_TIME) {
   

   We need to test each of these cases.

@claudiu.cristea

ChangeEmailController? - what should be done here ?

That is about the line before

/**
 * @file
 * Contains \Drupal\user\Controller\UserController.
 */

You forgot the class name when copied the file :)

@claudiu.cristea Thanks for review, back to work :)

I have added simple tests to test the change email functionality.

Thank you for the great work.

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +    $account = $this->getEntity($form_state);
   

   This method takes no argument. Just remove $form_state. Also it would be nice to type-hint the $account var with /** @var ... */ above, like you did in other place.

2. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +    if (!$account->isNew() && $old_mail !== $new_mail && !$this->currentUser()->hasPermission('administer users')) {
   

   I would wrap $old_mail !== $new_mail in parenthesis.

3. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -303,6 +303,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t("Edit the email message sent to the user old email address when the email address is changed.") . ' ' . $email_token_help,
   

   Isn't it "...sent to user's old email..."? I'm not a native English speaker, I might be wrong.

4. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -303,6 +303,50 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t("Edit the email messages sent to users' new email address when the email address is changed.") . ' ' . $email_token_help,
   

   s/users'/user's

5. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,95 @@
   +   *   UID of user requesting reset.
   

   Let's change it to "The ID of the user requesting reset."

6. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +      $account->setEmail($new_mail);
   ...
   +        // Send notification email to the old email address.
   +        $account->setEmail($old_mail);
   +        _user_mail_notify('mail_change_notification', $account);
   +      }
   

   I don't like that we are playing here with the $account object by changing its email 2 times. Also if the 1st notification fail, the $account will have the $new_mail as email and that's a lie. I know that this seems to not affect the current flow, but we cannot predict how contrib or custom modules will add their submit callbacks and they will not find there the correct email. I would prefer to use a fake cloned account just to send that email.

   $cloned_account = clone $account;
   $cloned_account->setEmail($new_mail);
   if (_user_mail_notify('mail_change_verification', $cloned_account)) {
     // Send notification email to the old email address.
     _user_mail_notify('mail_change_notification', $account);
   }
   

   Maybe $cloned_account is not a very beautiful name?

7. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +      drupal_set_message($this->t('Your email address needs to be validated. Further instructions have been sent to your new email address.'), 'warning');
   

   Maybe "Your new email address..."?

8. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,95 @@
   +      elseif ($this->currentUser()->id() && $this->currentUser()->id() != $account->id()) {
   

   Let's use !$this->currentUser()->isAnonymous() instead of the 1st condition. Also let's wrap the 2nd condition in parenthesis.

9. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,95 @@
   +      elseif ($hash != user_pass_rehash($account, $timestamp)) {
   

   Well, this is very problematic. I don't think we can rely on user_pass_rehash() to check the integrity of the $hash. I see that hash generated by that function vary on: $timestamp, $account->getLastLoginTime(), $account->id(), $account->getEmail() and $account->getPassword(). Let's take this scenario:

   1. The user changes its email
   2. A link with the hash is sent to the new email. Remember, the hash is dependent on the last login time
   3. The user ignores the mail.
   4. The user logs in again.
   5. After that, the user remembers that he needs to confirm the new email. He clicks on the link from notification.
   6. When he arrives here, Buuum! The user_pass_rehash() will generate a different hash because in the meantime the last login time has changed.

   I see this solved in other way. Let's add a new public method in this controller called changeEmailAccess. Also add the access check to the route. Add a new protected static method in the controller called getHash(). We will use that method to generate the hash in both places —­ url and validation. gatHash() might be similar with user_pass_rehash() but depend only on non-alterable variables: $timestamp, $account->id(), $account->getEmail(), $account->getCreatedTime(), $account->getInitialEmail(). In this way we can remove this check because the URL will not be accessible for wrong hashes.

10. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,95 @@
    +      elseif ($timestamp > $account->getLastLoginTime() && $timestamp < REQUEST_TIME) {
    

    The 2nd condition is redundant, we tested that in the wrapping if (). I don't see the reason of the first condition. I think we can replace the elseif with else.

11. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,95 @@
    +      // Deny access if the timestamp passed is in the future.
    

    Well, you added the ->isActive() condition. I think is correct, but we should reflect this also in this comment.

12. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
    @@ -0,0 +1,95 @@
    +  public static function changeEmailUrl(\Drupal\user\Entity\User $account, array $options = []) {
    

    Move the namespaced name to user Drupal\user\UserInterface;. use UserInterface instead of User for type-hinting.

13. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * The profile to install as a basis for testing.
    +   *
    +   * @var string
    +   */
    +  protected $profile = 'standard';
    

    Why we need standard? If 'user' module is not installed just add it to $modules. But I think is installed by default.

14. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  public static $modules = [];
    

    Remove it.

15. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    $token_service = \Drupal::token();
    

    In tests, the container is available $this->container->get('token'); You can also add a type-hint declaration for $token_service.

16. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    $account = $this->drupalCreateUser(['change own username']);
    +    $this->drupalLogin($account);
    

    Why that permission?

    Also I see that we are doing this in each test. Then we need to reuse that by implementing setUp() method. We declare a protected $account; in class and in setUp() we assign the account and login. Then in each test, we only use $this->account if case.

17. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    $edit = array();
    +    $edit['mail'] = $new_mail;
    +    $edit['current_pass'] = $account->pass_raw;
    

    $edit = [
      'mail' => ...,
      'current_pass' => ...,
    ];
    

18. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    // Take email settings from user.mail.yml.
    +    $mail_settings = Yaml::parse(file_get_contents(__DIR__ . '/../../config/install/user.mail.yml'));
    

    No. We use the Drupal config system to retrieve configurations. YAML files are only a way to import configs into Drupal when a module gets enabled.

    $mail_settings = $this->config('user.mail');

19. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    // Verify that the user was sent a notification email.
    ...
    +    // Verify that the user was sent a verification email.
    ...
    +    // Ensure the change email URL is not cached.
    

    "... the user has sent..." ?

    and generally we use a comment for assertions like:

    "Check that the user...." —­ let's standardise all assertion comments like that.

20. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    $this->assertMail('to', $account->getEmail(), 'Notification email with link to change the email has sent to user.');
    

    We can omit assertion messages.

21. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * Retrieves the change email and extracts the link.
    +   */
    

    Missing @param and @return.

22. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  public function getChangeEmailURLFromEmail($mail_id) {
    

    protected? Are we using it outside?

23. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +   * Generates random email.
    

    "Generates a random..."?

24. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  public function getRandomEmail() {
    

    protected. Are we using the function outside the test?

25. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    return $this->randomMachineName() . '@example.com';
    

    Maybe it would be nicer to have only lowercase email user. Then Unicode::strtolower($this->randomMachineName()).

26. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +   *   Unique hash.
    

    If a parameter is optional you should tell that and also provide the default. See https://www.drupal.org/coding-standards/docs#param

27. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +  public function getChangeEmailURL(\Drupal\user\UserInterface $account, $timestamp, $hash = NULL) {
    

    Based on our coding standards we need to use lowerCamel naming for methods. See https://www.drupal.org/node/608152#naming. So, getChangeEmailUrl(). Also use use Drupal\user\UserInterface; to avoid the full qualified name in method signature.

28. +++ b/core/modules/user/src/Tests/UserChangeEmailTest.php
    @@ -0,0 +1,182 @@
    +    return '/user/change-mail/' . $account->id() . "/$timestamp/" . $this->getRandomEmail() . '/' . $hash;
    

    Hmmm. Why not using the already existing controller url generator?

29. +++ b/core/modules/user/user.install
    @@ -85,3 +87,18 @@ function user_install() {
    +function user_update_8002() {
    

    We need to test that. See how other modules are testing upgrades.

I need to review more on tests but it's difficult right now because we need to implement first the changes.

@claudiu.cristea - as usual, thank you very much for you great review!

@claudiu.cristea - It is not necessary, but if you don't mind I have fixed the codding standards for AccountForm.php, AccountSettingsForm.php, because there are only minors changes.

I forgot to add the test case for update hook :( I will do it tomorrow.

@claudiu.cristea Unfortunately, I cannot find examples in the core modules. I'm not sure how to properly check the update hook, I just need to check the current configuration with values in YML files?

This is only a brief, bird-eye, formal review. More to come. I didn't had time to review the test scenarios and to think more on hashing and security.

An update test: \Drupal\field\Tests\Update\FieldUpdateTest. Create a similar one in core/modules/user/src/Tests/Update

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -98,7 +98,8 @@ public function form(array $form, FormStateInterface $form_state) {
   -    // Only show name field on registration form or user can change own username.
   +    // Only show name field on registration form or user can change own
   +    // username.
   
   @@ -313,10 +314,9 @@ public function syncUserLangcode($entity_type_id, UserInterface $user, array &$f
   -    //   set on the field, which throws an exception as the list requires
   -    //   numeric keys. Allow to override this per field. As this function is
   -    //   called twice, we have to prevent it from getting the array keys twice.
   -
   +    // set on the field, which throws an exception as the list requires
   +    // numeric keys. Allow to override this per field. As this function is
   +    // called twice, we have to prevent it from getting the array keys twice.
   
   @@ -355,7 +355,7 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
   -      'preferred_admin_langcode'
   +      'preferred_admin_langcode',
   
   @@ -373,7 +373,7 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   -      'preferred_admin_langcode'
   +      'preferred_admin_langcode',
   
   +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -109,8 +109,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    // Do not allow users to set the anonymous or authenticated user roles as the
   -    // administrator role.
   +    // Do not allow users to set the anonymous or authenticated user roles as
   +    // the administrator role.
   
   @@ -157,13 +157,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      '#description' => $this->t('New users will be required to validate their email address prior to logging into the site, and will be assigned a system-generated password. With this setting disabled, users will be logged in immediately upon registering, and may select their own passwords during registration.')
   +      '#description' => $this->t('New users will be required to validate their email address prior to logging into the site, and will be assigned a system-generated password. With this setting disabled, users will be logged in immediately upon registering, and may select their own passwords during registration.'),
   
   @@ -174,7 +174,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      '#description' => $this->t('Users with the %select-cancel-method or %administer-users <a href=":permissions-url">permissions</a> can override this default method.', array('%select-cancel-method' => $this->t('Select method for cancelling account'), '%administer-users' => $this->t('Administer users'), ':permissions-url' => $this->url('user.admin_permissions'))),
   +      '#description' => $this->t('Users with the %select-cancel-method or %administer-users <a href=":permissions-url">permissions</a> can override this default method.',
   +        array(
   +          '%select-cancel-method' => $this->t('Select method for cancelling account'),
   +          '%administer-users' => $this->t('Administer users'),
   +          ':permissions-url' => $this->url('user.admin_permissions'),
   +        )
   +      ),
   
   @@ -219,7 +225,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      '#default_value' =>  $mail_config->get('register_admin_created.body'),
   +      '#default_value' => $mail_config->get('register_admin_created.body'),
   
   +++ b/core/modules/user/user.module
   @@ -577,13 +578,12 @@ function user_user_logout($account) {
   -  $timestamp = REQUEST_TIME;
   ...
   -      'timestamp' => $timestamp,
   -      'hash' => user_pass_rehash($account, $timestamp),
   +      'timestamp' => REQUEST_TIME,
   +      'hash' => user_pass_rehash($account, REQUEST_TIME),
   

   These changes are unrelated. Please remove them.

2. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,35 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +      $account_cloned = clone $account;
   +      $account_cloned->setEmail($new_mail);
   

   I think $account_copy sounds better. What you think?

3. +++ b/core/modules/user/src/AccountForm.php
   @@ -386,13 +386,35 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +      if (_user_mail_notify('mail_change_verification', $account_cloned, NULL)) {
   

   Don't send NULL, that argument always defaults to NULL.

4. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,139 @@
   +  public function changeEmailPage($uid, $timestamp, $new_mail) {
   ...
   +  public static function changeEmailUrl(UserInterface $account, array $options = [], $timestamp = REQUEST_TIME, $hash = NULL) {
   ...
   +  public function changeEmailAccess($uid, $timestamp, $hash, $new_mail) {
   

   Now that we have a dedicated controller only for E-mail change, we can simplify the name changing. Let's:

   • changeEmailPage() > page()
   • changeEmailUrl() > url()
   • changeEmailAccess() > access()

5. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,139 @@
   +  public static function getHash(UserInterface $account, $timestamp) {
   

   We should add other per-user variables? I mentioned earlier the getCreatedTime() and getInitEmail().

6. +++ b/core/modules/user/src/Controller/ChangeEmailController.php
   @@ -0,0 +1,139 @@
   +    return Crypt::hmacBase64($data, Settings::getHashSalt() . $account->getPassword());
   

   I'm not sure. What if the user changes its pass after issuing the E-mail change but before verifying? Anyway this is open for discussion. I don't see very clear right now how to deal with this. I agree that the passwords hash is a good value of randomness. Leave it as it is for now.

7. +++ b/core/modules/user/user.install
   @@ -85,3 +87,18 @@ function user_install() {
   +
   

   Remove extra-line.

@claudiu.cristea - I took an example from other core modules. They use this naming $account_cloned and not $account_copy. About the first section, I know it is not related to this task, but I just wanted to fix them, because these changes are minor and it is easy to figure out what has been changed. If you insist I will remove them and will not touch other parts of the code anymore.

I took an example from other core modules. They use this naming $account_cloned and not $account_copy

OK, let's go in this way.

I know it is not related to this task, but I just wanted to fix them, because these changes are minor and it is easy to figure out what has been changed. If you insist I will remove them and will not touch other parts of the code anymore.

Reviewing a patch is not easy. Unrelated changes make patches hard to review. also, each issue should fix only one thing. Yes, please remove them from patch.

EDIT: There are exceptions. For example you edit a line in the scope of the issue but the above comment, referring to the line you've just edited, doesn't respect the coding standards. Of course you can correct that comment. But it has to be in the area you already touched. But here is not the case.