Opened 11 years ago

Last modified 7 years ago

#11348 new defect

Issues with EmailVerification

Reported by: izzy Owned by:
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: user register verify email
Cc: Trac Release: 1.0


I just updated one of my Trac installations (from Trac 0.11 to v1.0, AccountManager 0.2.1dev + EmailVerification patch from ticket:5509 to v0.4.3). While everything seems to work fine so far, I've noticed some issues concerning EmailVerification. Let me first list the processing steps, so I can address the issues better:

  1. User "Joe" visits the /register link, fills the form (including EMail), and submits it
  2. Mail is sent to the Trac Admin ("New user created"), user is informed (on page) to log in
  3. Joe logs in for the first time
  4. Joe is re-directed to the profile (to complete other data), while the verification mail is sent in background. A corresponding information is displayed on the page.

No issue with step 1. But at the point of step 2, first problem arises: if Admin checks the account of Joe on the WebUI, it says there the email address had been verified successfully – which is simply wrong and misleading, as the verification mail had not even been sent. Which means the Admin has no way to tell which accounts are really "verified". This is especially bad when your site has many bots visiting, which simply register and never come back (I encounter that frequently).

Next issue comes in at step 4: "A mail was sent to to verify your new address". You see, the mail address itself is missing there ("to to"), though it's used correctly in the mail sent (this part might be related to what's described in ticket:10215 – but as that was closed more than a year ago, it could be something else as well).

For a solution to the first issue, may I suggest to adjust the "workflow" a little towards how it had been back with the mentioned patch? I would very much welcome to have the verification mail sent right on submit of the registration form (at least optionally/configurable, but I wouldn't mind at all if that were mandatory). This would show the account as "unverified" in the admin WebUI immediately, as the fields email_verification_sent_to and email_verification_token are set right from the start. I fully understand the advantages of the current model (even accounts created before AccountManager was activated will be verified on the next login), but that check could still remain intact (affected accounts have no email_verification_sent_to set, so they could be identified based on this fact).

Even if another path is chosen: I guess to have this as "mandatory" wouldn't require much more than one or two lines of code added somewhere in I would really appreciate to know this code so I could patch it in my installation, as I require this order of processing for multiple other reasons as well (beside the WebUI, I also use cron jobs identifying unverified accounts this way, amongst other things).

Attachments (3)

model_py_emailverified.patch (627 bytes) - added by izzy 11 years ago.
fix result for Admin WebUI on users not having been sent any verification mail to ("step 2")
model_py_emailverified_lastvisit.patch (1.1 KB) - added by izzy 11 years ago.
cumulative fix: result for Admin WebUI on users not having been sent any verification mail to ("step 2") plus setting last_visit = now on registration
register_py_email_display.patch (516 bytes) - added by izzy 11 years ago.
registration message hides email address due to incorrect formatting. This fix works around the issue, so the user sees a correct message.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 11 years ago by izzy

PS: Shouldn't the last_visit field be set on registration already (finally, filling the registration form requires to visit the site – and the column is not named last_login but last_visit)? As it's done currently, how to tell what time the registration took place – e.g. to purge out "forgotten registrations" (see above: those "bots" which just register but never come back)? last_visit points to Jan 1, 1970 until the user logged in for the first time (and the verification mail was sent). So if I want to purge the "forgotten ones", and a user had just registered a minute before, his "fresh account" simply would be purged along:

SELECT s.sid sid, datetime(s.last_visit,'unixepoch') last_visit, name, a.value value
  FROM session s, session_attribute a
 WHERE s.sid = a.sid
   AND s.authenticated = 1
 ORDER BY s.sid;

sid       last_visit           name                        value
--------  -------------------  --------------------------  ---------------------
FunnyBot  1970-01-01 00:00:00  email             
FunnyBot  1970-01-01 00:00:00  name              
JohnDoe   1970-01-01 00:00:00  email             
JohnDoe   1970-01-01 00:00:00  name                        John Doe
Johnny    2013-10-10 10:07:06  chrome.warnings.0           <span>Ihre Berecht...
Johnny    2013-10-10 10:07:06  email             
Johnny    2013-10-10 10:07:06  email_verification_sent_to
Johnny    2013-10-10 10:07:06  email_verification_token    TPxy_kNM

While "Johnny" already logged in once (and thus got the verification mail sent), JohnDoe did not: he just registered 5 minutes ago (I know that because I did it – otherwise I would have been unable to tell). "FunnyBot" registered 1 month ago, but I cannot tell those dates apart by means of SQL. Or did I miss something here? Is there another way to identify and remove the "dead bones" without hurting the "just recently registered" which I overlooked?

comment:2 Changed 11 years ago by izzy

For the error in step 2 of the original report, see acct_mgr/ in def email_verified: If the verification mail was not (yet) sent, none of the for loops will be run – so it directly goes to the last line and returns TRUE (i.e. "verified successfully"). The very first query thus should be:

  FROM session_attribute
 WHERE sid=%s AND name='email_verification_sent_to'

If the result of this is 0, return FALSE (or None, to conform with existing code and not breaking anything – that would be the same as if the user just had changed his mail address, which then wouldn't be verified yet either): No verification mail sent, no validation done – no further checks required.

Changed 11 years ago by izzy

fix result for Admin WebUI on users not having been sent any verification mail to ("step 2")

comment:3 Changed 11 years ago by izzy

I've just modified acct_mgr/ as described in comment:2 and attached the patch model_py_emailverified.patch to this ticket. In "step 2", the Admin WebUI now correctly shows "This address has not been verified yet" – so the first issue can be considered "solved" :)

Remain the last_visit issue from comment:1, and the possibility to have the verification mail sent immediately on registration (original report). Still need to find the relevant place in the code for those – too long ago that I've been working at this...

Changed 11 years ago by izzy

cumulative fix: result for Admin WebUI on users not having been sent any verification mail to ("step 2") plus setting last_visit = now on registration

comment:4 Changed 11 years ago by izzy

If you agree that submitting the registration form should be counted a visit (and I can name a lot of reasons why it should – the only opposing thing being that trac.core might handle it differently; though, I cannot imagine what could break on this change), find a cumulative patch attached.

As for the error in step 4, there seems to be an issue with Markup() (see line 533: An email has been sent to <%(email)s>), which translates into HTML as An email has been sent to <> literally, i.e. the <> are not converted into the proper HMTL entities, which results in the mail address being hidden by the browser.

I'm not sure how to proceed here. The Markup() function is imported from Genshi (genshi.core.Markup). Is this a bug which should be filed against Genshi – or should we simply replace <> by the appropriate HTML entities? I feel the latter could raise other issues, and thus would propose to use [] instead as a work-around/quick-fix (which would also require to have the .po files adjusted, unfortunately). A better (still ugly but easy) fix was replacing email=email by email='I>'+email+'</I', which left the translation intact and simply makes the email address printed in italics. I'll attach the patch for that as well; feel free to adjust in any way you think is needed.

My major part it seems I must leave to you: I meanwhile found where the verification email is sent (EMailVerificationModule.post_request()), and why that's not done immediately on registration (if not req.session.authenticated return), so it's not just placing a call somewhere. Are there any chances to get that option? Or should that be moved to a separate ticket, so this one might be closed (having the appropriate patches attached)?

Changed 11 years ago by izzy

registration message hides email address due to incorrect formatting. This fix works around the issue, so the user sees a correct message.

comment:5 Changed 7 years ago by Ryan J Ollos

Owner: Steffen Hoffmann deleted

Modify Ticket

Change Properties
Set your email in Preferences
as new The ticket will remain with no owner.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.