Page MenuHomePhabricator

Email notification reject
Closed, ResolvedPublic

Description

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.


Version: unspecified
Severity: enhancement

Details

Reference
bz13450

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:07 PM
bzimport set Reference to bz13450.
bzimport added a subscriber: Unknown Object (MLST).

Created attachment 4747
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.

attachment patch3.patch ignored as obsolete

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

attachment patchemail.patch ignored as obsolete

Created attachment 4758
New patch, per Brion

Change to User::confirmationToken to use setters instead of direclty updating the database. Per Brion on IRC

attachment patchemail.patch ignored as obsolete

Created attachment 4759
Minor tweaks

Another patch, per comments from Brion on IRC, minor tweaks to functions, comments, and clarification of messages.

attachment patchemail.patch ignored as obsolete

Created attachment 4760
Fix

Fix spelling error in previous patch.

attachment patchemail.patch ignored as obsolete

Created attachment 4761
Fix line numbers

Exactly the same thing as before, but with correct line numbers in MessagesEn.

attachment patchemail.patch ignored as obsolete

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

Attached:

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.