Verify changing user email addresses
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | feature request |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
This issue follows on from http://drupal.org/node/69202
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
This patch (originally by chx) averts this by having a "confirm stage" email sent before Drupal actually accepts the new email address.
drumm has originally mentioned a lack of comments. Well, most of the patch adds code into existing functions. The main change in the introduction of user_change_mail() which is a new callback. Added appropiate comment to this function.
I'm hoping beginner will follow up this with "the not so easy bit" ;)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| user.module_r1.682_patch | 6.2 KB | Ignored | None | None |
#1
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" ;)
#2
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 ;)
#3
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?
#4
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
#5
#6
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
#7
Corrected bug in #6
#8
going to test this later.
#9
Some t calls in #7 need work:
<?phpwatchdog('user', t('User %name used one-time email change link at time %timestamp.', array('%name' => "<em>$account->name</em>", '%timestamp' => $timestamp)));
?>
Afaik, % calls theme_placeholder, additional em-tags are not necessary. In the mail t with %-variables will get you em-tags in the plaintext mail.
#10
updated patch attached. this addresses many issues:
patch has been pretty thoroughly tested on a clean HEAD site, and seems to work perfectly.
#11
Excellent work, Chad!
#12
We shouldn't set 'X-Mailer' => 'Drupal'. AFAIK, drupal_mail already does that ...
I'll provide a more in-depth review shortly.
#13
adding to queue...
#14
@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?
#15
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?
#16
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!
#17
Patch attached addresses / corrects this.
Hmm, I may be English but languages is "low" on my "best of" list. So, I failed to spot the missing full stop!
#18
okay, i believe i've fixed up the last of the problems:
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...
#19
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?
#20
per chx's consult, changed $from to the site mail.
#21
Just before the beta. To sum up: what's the point in the initial email confirmation if I can change it to foo whenever?
#22
patch broken by the capitalization commit. updated patch attached. no changes in code functionality whatsoever--just fixed up the conflict and updated the capitalization.
#23
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.)
#24
Note that this is the child of the contact spam issue which is indeed a very serious one -- hence the critical.
#25
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?
#26
per Dries' request, i have reworked the user messages in this patch.
#27
#28
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?
#29
see #15
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 ;)
#30
per dries' comment in #15 i'm leaving this change in.
fixed.
yeah, seems like a good idea. added.
#31
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.
#32
actually, contact.module already states the UID of the sender so we are covered there. thanks for illustrating that, boggieman.
#33
#34
patch updated. functionality is the same.
#35
#36
#37
Patch no longer applies; re-rolled with updates to hook_menu; untested.
-K
#38
no time to put into this anymore. any takers?
#39
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?
#40
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).
#41
Hello everyone, has this problem been solved in drupal 5.x versions?
#42
Changing back to 6.x-dev. I doubt this will be changed for 5.x.
#43
This can happily live in a contrib. Viva la form_alter!
#44
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
#45
Re: #40: A user is not able to change her email address to one that is already used by another user.
#46
As chx said: "This can happily live in a contrib" and here is the proof:
http://drupal.org/project/email_confirm
feedback is welcome
#47
I withdraw my objection earlier. This reallty does make perfect sense. Considering where we are in the release cycle, critical is warranted here.
#48
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.
#49
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.
#50
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.
#51
FYI - Decscription / Feature request for the email_confirm module mentioned in #46:
"Account hijack prevention management (prerequisites)"
http://drupal.org/node/475264