Last modified: 2008-03-25 22:04:32 UTC
Right now the email confirmation contains a link to accept the email for the given user. We do need a rejection link as well, which would immediately invalidate the email making it impossible to accept it by the first link later. Reason: mailing lists. EvilUser registers, enters mediawiki-tech-l@wikimedia.org (or whatever) as email, sends confirmation. He gets the email through the list and confirms. Then sets all email communication to yes. Then it is possible, naturally, to resend the password and kill the account by logging in, but maybe it's not going to be YOU who logs in fastest, and if you're not the list admin [and the admin is clueless] it could take forever to try to catch the account; and it's a needless hassle anyway. I see no cons to implement (apart from the time required to code the code). If the recipient is valid, s/he won't reject. If s/he is not valid, s/he definitely not wishing to get more confirmation emails, or anything associated with the imposter.
Created attachment 4747 [details] Patch to add an invalidateEmail() function to User.php Adds invalidateEmail() function to User.php and adds an extra link to the confirmation message. The URL is the same, but with ?action=invalidate added to the end. If this link is used instead of the normal confirmation link, it will reset the user_email, user_email_token, and user_email_token_expires fields.
Gave some feedback on the patch in irc... summary: * Needs to use user object setter methods rather than raw DB access (otherwise will break caching etc) * Clearing the email field is potentially unsafe, as you can lose access to your account if you click this by accident and forgot your password * URL query-string append will break on configs with "ugly URLs" * Extra param on URL may be undesireable as we wish to keep the email links as short as possible * A second special entry point may be cleaner for that
Re Brion comment #2: this is the phase when user try to _confirm_ the email, clearing that should have no effect, since: - if the email is valid and the user clicks on bad link s/he should see a page telling YOU INVALIDATED AND EMPTIED THE EMAIL OF THIS ACCOUNT, IF YOU MISCLICKED SET YOUR EMAIL AGAIN DUMMY ;-) - if the email was not valid, what happens is just fine. I didn't check the code - does password reminder send new password to unconfirmed email? If yes, the comment have merits but still I think the warning above should lower the possible collateral damage. Or maybe the best would be to offer a "clear [block?] this email from this account" button... (Or a "report email abuse on this account" button... eh. No resources to handle...)
Yes. Resetting password by mail is another way to confirm an email address.
Created attachment 4757 [details] New patch Patch based on Brion's comments. * Needs to use user object setter methods rather than raw DB access (otherwise will break caching etc) **Done - with a change to saveSettings() so it will actually save the email token and email expiry time in the database, this was the issue I brought up on IRC * Clearing the email field is potentially unsafe, as you can lose access to your account if you click this by accident and forgot your password **Doesn't do this anymore, but if it is desired it would be a trivial change (one additional line to invalidateEmail() ) * URL query-string append will break on configs with "ugly URLs" * Extra param on URL may be undesireable as we wish to keep the email links as short as possible * A second special entry point may be cleaner for that **Added an additional special page (Special:InvalidateEmail) **Had an issue with the way the confirmation/invalidation URLs were generated, it was giving 2 different tokens, 1 for each URL, so I had to change a bit of the flow in sendConfirmationMail() and the input for confirmationTokenUrl so the token would only be generated once.
Created attachment 4758 [details] New patch, per Brion Change to User::confirmationToken to use setters instead of direclty updating the database. Per Brion on IRC
Created attachment 4759 [details] Minor tweaks Another patch, per comments from Brion on IRC, minor tweaks to functions, comments, and clarification of messages.
Created attachment 4760 [details] Fix Fix spelling error in previous patch.
Created attachment 4761 [details] Fix line numbers Exactly the same thing as before, but with correct line numbers in MessagesEn.
Mr.Z: what happens now if person#1 validates, *then* person2 invalidates the email? (It ought to invalidate it anyway I guess, based on the original problem about the mailing lists.)
The function that actually confirms the email leaves the confirmation code in the database, so as long as the confirmation code hasn't expired, invalidation will still work. Since invalidation clears the expiration and confirmation code from the database, it doesn't work the other way around (if person1 invalidates, person2 cannot confirm it).
Latest patch is looking good -- almost ready to go! One problem with the issue noted just above about clicking the invalidation link after the confirmation link. The invalidation wipes out the *confirmation request token*, but doesn't actually invalidate an already-set confirmation. This means you cannot de-authenticate an already-authenticated email by clicking the old invalidation link -- although it will appear as though it did, since the UI indicates that the confirmation was canceled. Either the invalidation needs to also wipe the actual confirmation timestamp field, or the validation should also wipe the token to avoid these false positives (in which case clicking the invalidation link will tell you you've got an invalid or expired token). Additionally, note that the "If you did not register" line in the English message file has grown to 98 characters; messages meant for mail should be wrapped to about 72 characters per line.
It should wipe the confirmation timestamp IMHO, unless the token expired of course.
Created attachment 4764 [details] Per Brion More fixes per Brion: *Invalidation now wipes the user_email_authenticated timestamp *MessagesEn fixed for shorter line lengths (All the other lines were longer as well, so I wrapped those too)
Applied in r32449, looks good! The invalidation messages got dropped in the last version of the patch; I copied them in from the previous one.
