Issues with EmailVerification

[izzy]

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 register.py. 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).

[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,
       a.name 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                       asdf.rzus@gmail.com
FunnyBot  1970-01-01 00:00:00  name                        asdf.rzus@gmail.com
JohnDoe   1970-01-01 00:00:00  email                       john@xxxx.net
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                       joe@zzzz.org
Johnny    2013-10-10 10:07:06  email_verification_sent_to  joe@zzzz.org
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?

[izzy]

For the error in step 2 of the original report, see acct_mgr/model.py 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:

SELECT COUNT(*)
  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.

[izzy]

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

[izzy]

I've just modified acct_mgr/model.py 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...

[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

[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 register.py line 533: An email has been sent to <%(email)s>), which translates into HTML as An email has been sent to <user@domain.com> 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)?

[izzy]

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