Last modified: 2008-03-25 22:04:32 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T15450, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 13450 - Email notification reject
Email notification reject
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Email (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: easy, patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-20 12:59 UTC by Peter Gervai (grin)
Modified: 2008-03-25 22:04 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch to add an invalidateEmail() function to User.php (2.41 KB, patch)
2008-03-22 04:56 UTC, Alex Z.
Details
New patch (5.59 KB, patch)
2008-03-24 20:29 UTC, Alex Z.
Details
New patch, per Brion (6.21 KB, patch)
2008-03-24 21:36 UTC, Alex Z.
Details
Minor tweaks (7.04 KB, patch)
2008-03-25 00:39 UTC, Alex Z.
Details
Fix (6.90 KB, patch)
2008-03-25 00:45 UTC, Alex Z.
Details
Fix line numbers (6.83 KB, patch)
2008-03-25 01:01 UTC, Alex Z.
Details
Per Brion (7.52 KB, patch)
2008-03-25 20:45 UTC, Alex Z.
Details

Description Peter Gervai (grin) 2008-03-20 12:59:30 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.
Comment 1 Alex Z. 2008-03-22 04:56:45 UTC
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.
Comment 2 Brion Vibber 2008-03-24 18:57:21 UTC
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
Comment 3 Peter Gervai (grin) 2008-03-24 20:11:41 UTC
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...)
Comment 4 Brion Vibber 2008-03-24 20:21:31 UTC
Yes. Resetting password by mail is another way to confirm an email address.
Comment 5 Alex Z. 2008-03-24 20:29:09 UTC
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.
Comment 6 Alex Z. 2008-03-24 21:36:34 UTC
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
Comment 7 Alex Z. 2008-03-25 00:39:52 UTC
Created attachment 4759 [details]
Minor tweaks

Another patch, per comments from Brion on IRC, minor tweaks to functions, comments, and clarification of messages.
Comment 8 Alex Z. 2008-03-25 00:45:16 UTC
Created attachment 4760 [details]
Fix

Fix spelling error in previous patch.
Comment 9 Alex Z. 2008-03-25 01:01:03 UTC
Created attachment 4761 [details]
Fix line numbers

Exactly the same thing as before, but with correct line numbers in MessagesEn.
Comment 10 Peter Gervai (grin) 2008-03-25 07:41:57 UTC
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.)
Comment 11 Alex Z. 2008-03-25 15:53:38 UTC
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).
Comment 12 Brion Vibber 2008-03-25 18:49:12 UTC
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.
Comment 13 Peter Gervai (grin) 2008-03-25 20:24:10 UTC
It should wipe the confirmation timestamp IMHO, unless the token expired of course.
Comment 14 Alex Z. 2008-03-25 20:45:55 UTC
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)
Comment 15 Brion Vibber 2008-03-25 22:04:32 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links