Problem/Motivation

Currently, when a new user regsiters for a Drupal account the details are sent to the users supplied email address. This provides a basic mechanism that confirms the user is at that email address. However, once registered, users are permitted to change their email address without further confirming that the user is in fact at that email address.

Possible implications

  • A user can change their email address to be that of an unsusspecting third party as no confirmation of change is required. Using a second Drupal account (with it's email address also faked using the same method) the first user is then able to send anonymous malicious messages to the unsusspecting third party
  • A slow method for sending spam but exploitable none the less

Proposed resolution

Add a mechanism (similar to reset password) that:

  1. Sends an E-mail to the new address requiring the verification of the new address (similar to register confirmation).
  2. Sends a notification E-mal to the old address.
  3. Allow the site builder to customise both messages at admin/config/people/accounts
  4. Provides an update path to set the default behaviour and messages content.
  5. Write tests.

Remaining tasks

None.

User interface changes

New UI additions to admin/config/people/accounts:

API changes

New controller used for mail changing: \Drupal\user\Controller\MailChangeController

Data model changes

New schema for configs user.settings and user.mail.

Files: 

Comments

beginner’s picture

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" ;)

AjK’s picture

Not at all, your proposal sounds good. However, as drumm has pointed out, let's keep core patches nice, simple and byte sized ;)

This issue addresses one simple potential exploit which I would class as a bug. What you want is, imho, is a feature. A good one, but none the less, a feature. Introducing that on this issue will hamper it's chances of a commit.

The current issue is plain, simple and hopefully committable (review will tell).

I'd be happy to look at what you want to do, just lets get it on another issue and thrash it out there so this cuurent issue doesn't get bogged down.

Sound reasonable?

Thanks for the feedback.

regards,
-AjK

(p.s. I didn't single you in particular, just a gentle prod to get a review! How sneaky of me ;)

beginner’s picture

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?

AjK’s picture

The answer to your question is "probably fairly easy". The only reason I'm holding back on this is that we are in code freeze right now.

I believe this issue is regards a "critical bug" and labelled as such stands a chance of being committed.

I believe what you want is a "feature" and if this issue thread goes down that road I'm 100% certain this issue won't be addressed in the code freeze before Drupal5 is released.

I would suggest you start a new issue for what you want and in link back to this issue and say "could have gone in that issue but missed the boat".

I'm all up for doing what you want, it's a good solution, just not on this thread (we can develope what you want on a new thread right now, it doesn't have to be postponed, just I'd like it on it's own issue, pref as a patch after this patch gets commited, to modify the function in core, not modify a patch with a patch).

seem reasonable? (if/when you start a new issue, contact me via my contact form so I can get subscribed to it)

regards,
--AjK

drumm’s picture

Title: Prevent "contact form" email abuse » Verify changing user email addresses
AjK’s picture

Status: Needs review » Needs work

Found a bug in this patch.

When user updates their "my account" but leaves the password fields empty (i.e. they don't want to change their password but are changing something else) the email still gets sent informing them of their "intention to change their password".

Obviously, if the password fields are empty the "change email" workflow should not kick in and the email should not get sent.

I'll sort this out shortly but for now changing status to "code needs work"

regards,
--AjK

AjK’s picture

Status: Needs work » Needs review
FileSize
6.21 KB

Corrected bug in #6

webchick’s picture

going to test this later.

Heine’s picture

Status: Needs review » Needs work

Some t calls in #7 need work:

watchdog('user', t('User %name used one-time email change link at time %timestamp.', array('%name' => "$account->name", '%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.

hunmonk’s picture

FileSize
6.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_mail_change_confirm.patch. Unable to apply patch. See the log in the details link for more information. View

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.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Excellent work, Chad!

Dries’s picture

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

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

webchick’s picture

adding to queue...

hunmonk’s picture

Assigned: Unassigned » hunmonk
FileSize
5.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm.patch. Unable to apply patch. See the log in the details link for more information. View

@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?

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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?

AjK’s picture

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?

The "reset email" that is sent out is time limited (24hours). If the reset link is not activated within this time, the email address is not altered and remains the same. In fact, it doesn't change to the new address until activation by the one time, time limited email link.

As for the implications, it means that if the Drupal generated link in the email sent out isn't activated within it's given timeout value, the email address is not altered.

Did that make sense? It's late!

AjK’s picture

Status: Needs work » Needs review
FileSize
7.49 KB

Maybe that should be ?q=user/reset-password and q?=user/reset-mail.

Patch attached addresses / corrects this.

Punctuation is not consistent; some sentences end with a point

Hmm, I may be English but languages is "low" on my "best of" list. So, I failed to spot the missing full stop!

hunmonk’s picture

FileSize
6.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_0.patch. Unable to apply patch. See the log in the details link for more information. View

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

edmund.kwok’s picture

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?

hunmonk’s picture

FileSize
6.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_1.patch. Unable to apply patch. See the log in the details link for more information. View

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

hunmonk’s picture

FileSize
6.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_2.patch. Unable to apply patch. See the log in the details link for more information. View

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

Dries’s picture

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

chx’s picture

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

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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?

hunmonk’s picture

FileSize
7.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_3.patch. Unable to apply patch. See the log in the details link for more information. View

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

hunmonk’s picture

Status: Needs work » Needs review
drumm’s picture

Status: Needs review » Needs work

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?

AjK’s picture

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.

see #15

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?

Good point, I would say so (if we're copying the functionality often found on other sites, might as well make it a deep copy ;)

hunmonk’s picture

Status: Needs work » Needs review
FileSize
8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_4.patch. Unable to apply patch. See the log in the details link for more information. View

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.

moshe weitzman’s picture

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.

moshe weitzman’s picture

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

moshe weitzman’s picture

Priority: Critical » Normal
hunmonk’s picture

FileSize
8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_5.patch. Unable to apply patch. See the log in the details link for more information. View

patch updated. functionality is the same.

AjK’s picture

Version: x.y.z » 5.x-dev
drumm’s picture

Version: 5.x-dev » 6.x-dev
Zen’s picture

FileSize
7.83 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user-mail-change.patch. View

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

-K

hunmonk’s picture

Assigned: hunmonk » Unassigned

no time to put into this anymore. any takers?

hickory’s picture

Priority: Normal » Critical

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?

hickory’s picture

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

igorzh’s picture

Version: 6.x-dev » 5.x-dev

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

hickory’s picture

Version: 5.x-dev » 6.x-dev

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

chx’s picture

Version: 6.x-dev » 7.x-dev
Category: bug » feature
Priority: Critical » Normal

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

birdmanx35’s picture

Status: Needs review » Needs work

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

sun’s picture

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

jaydub’s picture

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

http://drupal.org/project/email_confirm

feedback is welcome

moshe weitzman’s picture

Priority: Normal » Critical

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

moshe weitzman’s picture

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.

earnie’s picture

subscribing. Currently I'm drawn toward leaving this feature as a module add-on. Workflow is different for different sites and what one might do another will do something else. If the feature is added to core it needs to be an option that the admin can use or not use. So, this might as well as just be a contrib package.

mikeryan’s picture

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.

Taxoman’s picture

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

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

robby.smith’s picture

+1 subscribing

momendo’s picture

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

+1 subscribe

aaronbauman’s picture

Priority: Critical » Normal

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

Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev

7.x is feature frozen anyway.

marcingy’s picture

Priority: Normal » Major

Changing to major as per tag.

MustangGB’s picture

Tag update

amc’s picture

subscribing

geerlingguy’s picture

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 :-/

webchick’s picture

Status: Needs work » Fixed

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

Current password field on user profile

geerlingguy’s picture

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.

webchick’s picture

Status: Fixed » Needs work

Oops! You are right, sorry. :)

shadysamir’s picture

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?

ralf.strobel’s picture

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.

andypost’s picture

Status: Needs work » Postponed

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

Taxoman’s picture

Status: Postponed » Needs work

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

amc’s picture

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.

rogical’s picture

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!

arnoldbird’s picture

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

jaydub’s picture

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

greggles’s picture

Issue tags: +Security improvements

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

ralf.strobel’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
14.27 KB
FAILED: [[SimpleTest]]: [MySQL] 48,694 pass(es), 37 fail(s), and 42 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 85494-73.patch, failed testing.

Homotechsual’s picture

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?

kifuzzy’s picture

#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

Homotechsual’s picture

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

dawehner’s picture

Issue summary: View changes

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.

joachim’s picture

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.

kaare’s picture

Category: Feature request » Bug report

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.

andypost’s picture

Issue summary: View changes
Issue tags: +security, +Needs beta evaluation, +Needs issue summary update, +Needs reroll
kaare’s picture

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.

pjonckiere’s picture

Assigned: Unassigned » pjonckiere
FileSize
6.25 KB

I'm working on this. I wasn't able to rebase the patch in #73, so I started rewriting it. I finished the first part for now. This patch contains the WIP.

pjonckiere’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Needs tests
FileSize
11.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 26,985 pass(es), 812 fail(s), and 825 exception(s). View

I rewrote the rest of the patch, but didn't test this manually yet. I'm just curious if we have failing tests now, so triggering testbot.

Also this will need tests, so tagging accordingly.

Status: Needs review » Needs work

The last submitted patch, 84: verify_changing_user-85494-84.patch, failed testing.

pjonckiere’s picture

FileSize
604 bytes
12.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,148 pass(es), 23 fail(s), and 2 exception(s). View
pjonckiere’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 86: verify_changing_user-85494-86.patch, failed testing.

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
13.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,799 pass(es). View
pjonckiere’s picture

FileSize
7.67 KB
17.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,809 pass(es). View

Added some minor changes, and added some coverage.

Some remarks/questions:
- I think we might need a follow up for migration from D6/D7.
- The current permissions don't specify changing an email address, only 'cancel own user account', 'change own username' and 'view user information'. Should we add one specifically for this use case, e.g. 'change own email address'?
- I can't find any unit test coverage for UserController. Shouldn't we do that here? Specifically for UserController::changeEmail().

donutdan4114’s picture

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

deanflory’s picture

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

ptmkenny’s picture

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

pjonckiere’s picture

Assigned: pjonckiere » Unassigned
Issue tags: +needs backport to D7

Re #92: Indeed. The policy is to first fix this in D8 and then backport it to previous versions.

Re #93: With a follow-up I meant a new issue to tackle migration issue that might occur due to this fix.

Anybody’s picture

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

legolasbo’s picture

Status: Needs review » Needs work
  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.

StryKaizer’s picture

FileSize
9.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch verify_changing_user-85494-97.patch. Unable to apply patch. See the log in the details link for more information. View
9.41 KB

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.

StryKaizer’s picture

FileSize
18.13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,420 pass(es). View

Correct patch now, remarks see #97

borisson_’s picture

Status: Needs work » Needs review

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

The last submitted patch, 97: verify_changing_user-85494-97.patch, failed testing.

StryKaizer’s picture

Status: Needs review » Needs work

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.

David_Rothstein’s picture

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.

LOBsTerr’s picture

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’s picture

Status: Needs work » Needs review
cilefen’s picture

Status: Needs review » Needs work

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

LOBsTerr’s picture

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

LOBsTerr’s picture

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

LOBsTerr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 107: interdiff-85494-103-107.patch, failed testing.

LOBsTerr’s picture

Renamed interdiff to be ignored by the Testbot

LOBsTerr’s picture

LOBsTerr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

LOBsTerr’s picture

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

LOBsTerr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

LOBsTerr’s picture

LOBsTerr’s picture

FileSize
4.89 KB
LOBsTerr’s picture

Status: Needs work » Needs review

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

I dare mark it RTBC

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

cilefen’s picture

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

LOBsTerr’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
18.31 KB

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

LOBsTerr’s picture

Status: Needs work » Needs review

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

cilefen’s picture

Status: Needs review » Needs work
+++ 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.

k4v’s picture

Assigned: Unassigned » k4v

Working on this @Drupalcamp Vienna

k4v’s picture

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

k4v’s picture

Assigned: k4v » Unassigned
Status: Needs work » Needs review

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

LOBsTerr’s picture

Status: Needs review » Reviewed & tested by the community

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.

The last submitted patch, 110: interdiff-85494-103-110.diff, failed testing.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

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.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Security improvements, -Needs beta evaluation
  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?

LOBsTerr’s picture

Assigned: Unassigned » LOBsTerr
David_Rothstein’s picture

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

claudiu.cristea’s picture

Ouch, sorry. I missed that.

k4v’s picture

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

LOBsTerr’s picture

Assigned: LOBsTerr » Unassigned
Status: Needs work » Needs review
FileSize
19.83 KB
10.58 KB
claudiu.cristea’s picture

Status: Needs review » Needs work
  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.

LOBsTerr’s picture

Assigned: Unassigned » LOBsTerr
claudiu.cristea’s picture

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

LOBsTerr’s picture

Assigned: LOBsTerr » Unassigned
Status: Needs work » Needs review
FileSize
23.21 KB
17.16 KB

Status: Needs review » Needs work

The last submitted patch, 144: verify_changing_user-85494-144.patch, failed testing.

LOBsTerr’s picture

Status: Needs work » Needs review
FileSize
23.21 KB
17.15 KB

Status: Needs review » Needs work

The last submitted patch, 146: verify_changing_user-85494-145.patch, failed testing.

LOBsTerr’s picture

Status: Needs work » Needs review
FileSize
17.17 KB
23.22 KB
claudiu.cristea’s picture

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

LOBsTerr’s picture

@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 :(

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs review » Needs work
LOBsTerr’s picture

Assigned: Unassigned » LOBsTerr

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

claudiu.cristea’s picture

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.

LOBsTerr’s picture

@claudiu.cristea

ChangeEmailController? - what should be done here ?

claudiu.cristea’s picture

That is about the line before

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

You forgot the class name when copied the file :)

LOBsTerr’s picture

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

LOBsTerr’s picture

Assigned: LOBsTerr » Unassigned
Status: Needs work » Needs review
FileSize
25.77 KB
19.23 KB

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

claudiu.cristea’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path tests

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.

LOBsTerr’s picture

Assigned: Unassigned » LOBsTerr

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

LOBsTerr’s picture

Assigned: LOBsTerr » Unassigned
Status: Needs work » Needs review
FileSize
26.45 KB
31.91 KB

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

LOBsTerr’s picture

@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?

claudiu.cristea’s picture

Status: Needs review » Needs work

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.

LOBsTerr’s picture

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

claudiu.cristea’s picture

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.

LOBsTerr’s picture

Status: Needs work » Needs review
FileSize
11.91 KB
25.82 KB

I have created this test, but I cannot run it. There are some issues with update tests. I tested other simple tests which use UpdatePathTestBase. All their breaks with the same errors. So, I will try to find an issue for that and upload this test later.

Status: Needs review » Needs work

The last submitted patch, 166: verify_changing_user-85494-166.patch, failed testing.

LOBsTerr’s picture

Status: Needs work » Needs review
FileSize
25.79 KB
5.87 KB

Status: Needs review » Needs work

The last submitted patch, 168: verify_changing_user-85494-168.patch, failed testing.

LOBsTerr’s picture

@claudiu.cristea - It is strange, I tried to run UserChangeEmailTest before I uploaded the patch.

51 passes, 0 fails, 0 exceptions, 14 debug messages

Could it be some glitches with Drupal testing system?

pjonckiere’s picture

Status: Needs work » Needs review
FileSize
1 KB
25.82 KB

Afaik, it should work. I ran the test several times locally and it always passes.

I did fix a typo while I was waiting for the test to run.

Status: Needs review » Needs work

The last submitted patch, 171: verify_changing_user-85494-171.patch, failed testing.

claudiu.cristea’s picture

@LOBsTerr, probably the HEAD is ahead and you need to update your codebase from repo.

LOBsTerr’s picture

Status: Needs work » Needs review
FileSize
881 bytes
25.83 KB

I always try to pull the latest changes. Anyway tried to pull changes again and added some minor fixes.

claudiu.cristea’s picture

Status: Needs review » Needs work

Green, thank you. Then let's add the Update test and I will review the whole patch. Also we need to figure out what to make with the hash.

LOBsTerr’s picture

@claudiu.cristea - I think it is ok to remove password as you suggested above, because we enough unique information to generate a secure hash.

LOBsTerr’s picture

@claudiu.cristea - I have been struggled with UpdateTest and I cannot make it work. Actually, I cannot make work any update test in the core. Usually, I have about 150 fails, but it happens UpdatePathTestBase. It is strange, because the same tests obviously run without any issue on the Drupal.org. Can you try to run any UpdateTest locally? Maybe I need to install something to make it work. I can send the patch with my UserUpdateTest class, but I cannot test it locally:(
As you see it is quite simple, but it fails, because something breaks in $this->runUpdates();

Thank you for helping me.

<?php

/**
 * @file
 * Contains \Drupal\user\Tests\Update\UserUpdateTest.
 */

namespace Drupal\user\Tests\Update;

use Drupal\system\Tests\Update\UpdatePathTestBase;
use Symfony\Component\Yaml\Yaml;

/**
 * Tests that user settings are properly updated during database updates.
 *
 * @group user
 */
class UserUpdateTest extends UpdatePathTestBase {

  /**
   * {@inheritdoc}
   */
  public function setDatabaseDumpFiles() {
    $this->databaseDumpFiles = [__DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz'];
  }

  /**
   * Tests user_update_8002().
   *
   * @see user_update_8002()
   */
  public function testUserUpdate8002() {
    $mail_config = Yaml::parse(file_get_contents(__DIR__ . '/../../../config/install/user.mail.yml'));
    $this->runUpdates();
    $mail_settings = $this->config('user.mail');

    $this->assertEqual($mail_config['mail_change_notification']['subject'], $mail_settings->get('mail_change_notification.subject'));
    $this->assertEqual($mail_config['mail_change_notification']['body'], $mail_settings->get('mail_change_notification.body'));
    $this->assertEqual($mail_config['mail_change_verification']['subject'], $mail_settings->get('mail_change_verification.subject'));
    $this->assertEqual($mail_config['mail_change_verification']['body'], $mail_settings->get('mail_change_verification.body'));
  }

}
claudiu.cristea’s picture

I don't see the test in the patch. Please add the patch with the test to see the failures.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs tests, -Needs upgrade path tests +needs security review, +Needs usability review
FileSize
30.95 KB
44.16 KB
348.92 KB

Here we go.

attiks’s picture

Code is looking good, same phrasing sounds a bit weird to me, so best that somebody else can review it as well

Didn't had a chance to test the patch, will try later today

  1. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,140 @@
    +      // Reflect the changes in the session if the user is user is logged in.
    

    Strange sentence

  2. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,140 @@
    +   *   (optional) The timestamp when hash is created. If missed, the current
    +   *   request time is used.
    ...
    +   *   (optional) Unique hash. If missed, the hash is computed based on the
    +   *   account data and timestamp.
    

    'If missed' => 'When not provided'

claudiu.cristea’s picture

FileSize
30.93 KB
1.35 KB

Thank you @attiks.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

swentel’s picture

  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -382,13 +382,32 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
    +    if (!$account->isNew() && ($old_mail !== $new_mail) && !$this->currentUser()->hasPermission('administer users')) {
    +      // Send a verification to the new email address.
    +      $account_cloned = clone $account;
    +      $account_cloned->setEmail($new_mail);
    +      if (_user_mail_notify('mail_change_verification', $account_cloned)) {
    +        // Send notification email to the old email address.
    +        _user_mail_notify('mail_change_notification', $account);
    +      }
    +
    +      // The user's mail address will be updated only after verification.
    +      $form_state->setValue('mail', $old_mail);
    +
    +      drupal_set_message($this->t('Your new email address needs to be validated. Further instructions have been sent to your new email address.'), 'warning');
    +    }
    

    The logic seems off here. If you don't enable mail change verification, you will still see the message and your old mail will be stored, no ?

  2. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,140 @@
    +      drupal_set_message($this->t('You are currently logged in as %user, and are attempting to confirm an email address change for other account. Please <a href=":logout">log out</a> and try using the link again.', ['%user' => $current_user->getAccountName(), ':logout' => Url::fromRoute('user.logout')->toString()]), 'error');
    

    'for another account' instead of 'for other account'

claudiu.cristea’s picture

FileSize
4.61 KB
30.91 KB

@swentel, thank you for looking at this issue.

#184.1: (EDIT) mail_change_verification is not a flag but a pair of E-mail subject + body texts. So _user_mail_notify('mail_change_verification', $account_cloned) is sending out an E-mail. The fact that the mail was sent cannot be guaranteed. The logic there would be

Even the notification E-mails were not sent (because some emailing system error) you will still see the status message and the old email will be stored.

But note that we store the old E-mail because we want the account to be saved with the old E-mail. We don't allow the new E-mail to be stored till the user will confirm the new E-mail. We effectively change the E-mail when the user clicks the verification link, in \Drupal\user\Controller\MailChangeController::page().

#184.2: Fixed Thanks.

I also dropped MailChangeController::getHash() because a more simple way was to change user_pass_rehash() to accept a new optional parameter.

claudiu.cristea’s picture

FileSize
31.71 KB
2.32 KB

@swentel, you are right in #184. Forgot that we have also checkboxes for that notification.

swentel’s picture

Looks good to me now imo. Wondering how to trigger usability and security reviews on this one .. maybe just put to RTBC, that will at least set more eyes on this one.

The last submitted patch, 185: 85494-185.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 186: 85494-187.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
33.28 KB
8.1 KB

@swentel, yes probably it's good to have it RTBC. This issue is 10 years old and has 72 followers :)

Fixed the test and converted the test to BrowserTestBase test. Also I added a new test testMailChangeNoVerification() that tests the case when mail change validation is switched off. Fixed also other nits.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Nit in the test.

+++ b/core/modules/user/tests/src/Functional/UserMailChangeTest.php
@@ -64,15 +70,41 @@ public function testMailChange() {
+    // Save a new email and verify that the user has sent an email.
+    $new_mail = $this->getRandomEmailAddress();

Hmm, should be something like '.. and verify that the no email was sent'.

Moving to RTBC, let's see what the core committers think of this.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +8.3.0 release notes
FileSize
33.29 KB
1.38 KB

Fixed the comments.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Changed the status by mistake

xjm’s picture

Title: Verify changing user email addresses » Use email verification when changing user email addresses
swentel’s picture

+++ b/core/modules/user/user.post_update.php
@@ -0,0 +1,37 @@
+/**
+ * Update config for change mail notifications.
+ */
+function user_post_update_mail_change() {
+  $config_factory = \Drupal::service('config.factory');

So I've been thinking more about this update which enables the email change verification. While this update was handy in my case (because we require it for a project), I'm not sure if we should really run this by default when users are going to upgrade.

It's an extremely useful feature of course, which on new installations warrant to be enabled by default, but I'm not sure about upgrades, because maybe users don't necessarily see this as a bug ?

Not setting to needs work for this, because I'm fine if it gets committed as is as well, just wanted to express my thoughts.

dawehner’s picture

+++ b/core/modules/user/user.post_update.php
@@ -0,0 +1,37 @@
+  $config = Yaml::parse(file_get_contents(__DIR__ . '/config/install/user.settings.yml'));
+  $config_factory->getEditable('user.settings')
+    ->set('notify.mail_change_notification', $config['notify']['mail_change_notification'])
+    ->set('notify.mail_change_verification', $config['notify']['mail_change_verification'])
+    ->set('mail_change_timeout', 86400)
+    ->save();

Personally I'm a huge fan of changing default values, but not update to those default values, but rather keep existing behaviours. You know, it just avoids unnecessary problems.

claudiu.cristea’s picture

I agree with #195 and #196. More clarifications:

  1. +++ b/core/modules/user/config/install/user.settings.yml
    @@ -9,8 +9,11 @@ notify:
    +  mail_change_notification: true
    +  mail_change_verification: true
    

    I think this should remain as it is, so new site installations will get advantage of this fix.

  2. +++ b/core/modules/user/user.post_update.php
    @@ -0,0 +1,37 @@
    +  $config_factory->getEditable('user.settings')
    +    ->set('notify.mail_change_notification', $config['notify']['mail_change_notification'])
    +    ->set('notify.mail_change_verification', $config['notify']['mail_change_verification'])
    

    Here we need these values to FALSE, to disable this feature for existing sites.

  3. +++ b/core/modules/user/user.post_update.php
    @@ -0,0 +1,37 @@
    +  $config_factory->getEditable('user.mail')
    +    ->set('mail_change_notification', $config['mail_change_notification'])
    +    ->set('mail_change_verification', $config['mail_change_verification'])
    

    This should remain even the feature will be disabled for the existing sites because here we set the default mail subject & body. Will not have effect but will be set in the case the admin manually enable them.

Are we OK with this plan?

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
33.12 KB
1.75 KB

@swentel,

Implemented #195, #196, #197. I'm setting the issue to "Needs review" to get your feedback.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sounds perfect to me.

+++ b/core/modules/user/src/Tests/Update/UpdateMailChangeTest.php
@@ -0,0 +1,59 @@
+    // Check that mail change notifications were set to TRUE.

Extreme nit, should be FALSE :)

Moving back to RTBC, if anyone wants to fix that nit, leave the status on RTBC :)

claudiu.cristea’s picture

Oh, yeah. Missed that :)

alexpott’s picture

+++ b/core/modules/user/user.module
@@ -638,15 +639,19 @@ function user_cancel_url(UserInterface $account, $options = array()) {
+function user_pass_rehash(UserInterface $account, $timestamp, $mail = NULL) {
+  $mail = $mail ?: $account->getEmail();
   $data = $timestamp;
   $data .= $account->getLastLoginTime();
   $data .= $account->id();
-  $data .= $account->getEmail();
+  $data .= $mail;
   return Crypt::hmacBase64($data, Settings::getHashSalt() . $account->getPassword());
 }

Initially looking at this change I wondered if this was a security issue - ie it is possible to generate a hash without an email address - but looking at the code it appears not.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs product manager review
  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -382,13 +382,30 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
    +        drupal_set_message($this->t('Your new email address needs to be validated. Further instructions have been sent to your new email address.'), 'warning');
    

    This should probably be 'updated' rather than new. The e-mail address is new to the site, but probably not to the user.

  2. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,111 @@
    +    $timeout = $this->config('user.settings')->get('mail_change_timeout');
    

    Is there a UI for this? If not why not put it in $settings?

  3. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,111 @@
    +      // Save the new email but refresh also the last login time so that this
    +      // mail change link get expired.
    ...
    +    }
    +    // Timestamp from the link is abnormal (in the future) or user registered a
    +    // new login in the meantime or the hash is not valid.
    +    else {
    

    Nit, should be 'gets expired' or 'is expired'.

    Should 'registered a new login' bit 'logged in'?

  4. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,111 @@
    +      // Reflect the changes in the session if the user is user is logged in.
    

    if the user is user is

  5. +++ b/core/modules/user/user.post_update.php
    @@ -0,0 +1,36 @@
    +  $config = Yaml::parse(file_get_contents(__DIR__ . '/config/install/user.mail.yml'));
    

    Shouldn't this duplicate the value from the configuration instead of reading it in? Otherwise the config might change again, and this update will have different results depending on which version is updated to.

Also this is still tagged usability review. I didn't review the whole issue to see whether that had been done, would be good to update the issue summary and remove the tag if it has, or ask someone to review it if not. Tagging for product review as well.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs usability review
FileSize
33.8 KB
16.41 KB

Fixed #202.

Also removed t() from test according to https://www.drupal.org/node/2783189#t and replaced the deprecated REQUEST_TIME with the new service.

claudiu.cristea’s picture

Removed unused use statement

swentel’s picture

+++ b/core/modules/user/config/schema/user.schema.yml
@@ -53,9 +53,6 @@ user.settings:
     password_reset_timeout:
       type: integer
       label: 'Password reset timeout'
-    mail_change_timeout:
-      type: integer
-      label: 'Mail change timeout'
     password_strength:

This isn't really consistent. Password reset timeout doesn't have a UI either and it's in config. So I'd leave mail_change_timeout in config as well and create a follow up to expose both password and mail change timeout in the UI (if wanted).

Looks good to me other than that.

claudiu.cristea’s picture

Reverted mail_change_timeout to a config in order to keep the symmetry with password_reset_timeout. Opened #2823877: Expose user.settings:password_reset_timeout and mail_change_timeout in UI to provide UI for both, as a follow-up.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ok, looks good to me. Let's give committers an eye on this again.

catch’s picture

re #205 I think really the password reset timeout should move to settings as well - this looks like something that got converted from variables to config all in one go.

claudiu.cristea’s picture

@catch, If somebody has overwritten that value in a custom module he will lose the setting as we cannot provide an update path. Or I'm missing something.

EDIT: Or a custom/contrib module just uses the config for some reasons. After conversion, this will crash: \Drupal::config('user.settings')->get('password_reset_timeout')

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

.

  1. Re mail_change_timeout

    Is there a UI for this? If not why not put it in $settings?

    Lack of UI does not mean something should be in settings. I think this belongs in config because it is precisely the sort of thing you want the same in all environments and to be able to be able to deploy via a config change.

    Also this still needs a product manager review. It definitely needs a change record too.

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -382,13 +382,30 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
    +    if (!$account->isNew() && ($old_mail !== $new_mail) && !$this->currentUser()->hasPermission('administer users')) {
    +      // Send a verification to the new email address.
    +      $account_cloned = clone $account;
    +      $account_cloned->setEmail($new_mail);
    +      if (_user_mail_notify('mail_change_verification', $account_cloned) !== NULL) {
    +        // Send notification email to the old email address.
    +        _user_mail_notify('mail_change_notification', $account);
    +        // The user's mail address will be updated only after verification.
    +        $form_state->setValue('mail', $old_mail);
    +        drupal_set_message($this->t('Your updated email address needs to be validated. Further instructions have been sent to your new email address.'), 'warning');
    +      }
    +    }
    

    Doing this in the submitForm is wrong - it needs to be done in User::postSave() so the changes via the API have the same logic.

  3. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,114 @@
    +use Drupal\Core\Site\Settings;
    

    Not used.

  4. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,114 @@
    +      drupal_set_message($this->t('You have tried to use an email address change link that has expired. Please visit your account and change your email again.'), 'error');
    

    Will this actually work? Ah yes the mail is only updated after the verification is successful.

  5. +++ b/core/modules/user/src/Controller/MailChangeController.php
    @@ -0,0 +1,114 @@
    +    return $this->redirect('<front>');
    

    Given that everything redirects I don't think we have the issue with hash exposure that we had with one time links. Nice! However I think there should be a similar comment in the function docblock...

       * In order to never disclose a reset link via a referrer header this
       * controller must always return a redirect response.
    
  6. +++ b/core/modules/user/user.routing.yml
    @@ -200,3 +200,16 @@ user.reset.form:
    +    _maintenance_access: TRUE
    

    Are we sure about this?

alexpott’s picture

Re #210.2 It actually might need to be done in preSave actually because you're resetting the value back to the old value.

claudiu.cristea’s picture

@alexpott, sending Email notifications from preSave()? Brrrr

claudiu.cristea’s picture

As an effect of #210.2, #211, we need to test the REST part too. Test in \Drupal\user\Tests\RestRegisterUserTest

k4v’s picture

I just took a quick look at User::preSave, where should we get the old value from? The new address is already set at this point.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

@k4v, I already started to work on this. Assigned.