EmailVerificationModule not documented - and not enforcing email to be entered on registration

[hanswurst]

The EmailVerificationModule is not documented at ​http://trac-hacks.org/wiki/AccountManagerPlugin nor in the source code.

I need to use ​http://trac-hacks.org/wiki/AccountManagerPlugin to require a valid email address when a user creates an account. The password should then be send to the given email address to verify that it is a valid address. I was told that EmailVerificationModule can do this, but without documentation I cannot figure out if this is the case and how this is set.

[Sebastian Krysmanski]

Simply enabling the component does the trick for me.

[anonymous]

After doing so, it is still possible to create an account w/o specifying an email address (at least on my test server) - which to me is not the same as "requiring a valid email address" (it's not even requiring an email address at all). Though existing users with an email address specified in their records get notified they have to verify it. Guess I'm doing something wrong, since this makes no sense to me - but can anybody tell me what it is?

Besides: Enabling EmailVerificationModule without having a valid smtp server configured lets Trac crash with an Error 500 - so without shell access to disable it in the config, one is messed up. Shouldn't that be caught? (Just in case: I'm using the latest SVN code, r6741, on Trac 0.11)

[izzy]

Patch to force entering an email address on registration when EmailVerificationModule is enabled

[izzy]

I just attached a patch which forces the new user to enter something into the email field. This still leaves two things open:

• the email field is still in the "optional" section, which is a bit confusing
• there's no check whether the entered value is a valid email address

Is there something like a "timeout" removing unfinished accounts?

[izzy]

The latest patch (registration_emailcheck.diff) also moves the email input to the mandatory part of the form if the verification module is enabled, but leaves it in the optional part otherwise. Now it's only missing some (at least raw) check on the syntax for the email address. Maybe some regexp like /^.+@.+\..+/is (i.e. make sure it has some chars followed by the '@', followed by more chars plus a '.' plus some more chars), which would be at least a very basic check? Though this would also allow invalid addresses like "a@…", so someone may provide something better here...

[izzy]

Patch to force entering an email address on registration when EmailVerificationModule? is enabled (includes previous plus moves email input to mandatory part, and does a raw check on email syntax)

[izzy]

Just replaced the latest diff with an updated version doing a basic regexp check on the entered email. The regexp I used here is ^[A-Z0-9._%+-]+@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$ (used case insensitive) - which matches e.g. foo@…, Foo.Bar@…, and foo.bar@…' - but not foo, foo@bar, or foo@…. Note that this also matches the latest one and two letter second-level-domains (I just read they really did that for .de) - so it should be safe for 99.9% of all email addresses.

Of course, nothing speaks against adding a config option for whether or not to run this check, if somebody will do that...

Concerning the last issue of comment:4 - I searched the code, but found no "cleanup procedure" for attempted but not approved registrations (i.e. the verification code was sent, but never entered). Maybe I'll write a little script for that to be run via cron (or some similar scheduler). IMHO, if one didn't enter it within two weeks, it's unlikely it will happen later - so those accounts will most probably stay unused and thus could be removed to keep things clean.

[izzy]

Replying to hanswurst:

The EmailVerificationModule is not documented at ​http://trac-hacks.org/wiki/AccountManagerPlugin nor in the source code.

I was so free to add a short description to the wiki page (I thought: It's a wiki, so why shouldn't I do so ;)). Anybody having experience with this module, please crossread and correct, if necessary.

[Pedro Ferreira <pedro.ferreira@…>]

Could this please be integrated into the trunk?

[Daniel Keyhani]

New patch incorporating changes from previous patches, with following additional changes: diff against trunk (v0.12), check whether email address already exists, and removed unnecessary blurb (email isn't required to send verification token, but by policy - token is sent only to verify compliance to policy)

[Steffen Hoffmann]

Replying to Pedro Ferreira <pedro.ferreira@strigo.eu>:

Could this please be integrated into the trunk?

Sorry for such a long time of virtually no support for this important plugin. I've been awarded maintainership only last night, so give me some time to review. After all there are 80+ other tickets right now.

But by accepting this ticket I declare, this is on my (short term) ToDo list. Thank you for your patience.

[Steffen Hoffmann]

Replying to izzy:

Just replaced the latest diff with an updated version doing a basic regexp check on the entered email. The regexp I used here is ^[A-Z0-9._%+-]+@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$ (used case insensitive) [...]

Did you have a look at Trac itself? At least it has the input form for email in user preferences on it's own, and I'm curious, how input validation is done there, if done at all. And we could compare to other publicly available code for non-interactive email address validation and lift some good code from there.

Of course, nothing speaks against adding a config option for whether or not to run this check, if somebody will do that...

Sure, an option comes to mind. But how about coupling activation of verification module and position of email input in mandatory or optional section plus corresponding logic behind? IMHO enabling verification and making email required for new registrations in the same moment is exactly the desired, expected effect. At least this is the bare minimum.

We might want to help user administration in how to deal with existing accounts without email as well. Optionally treating all accounts without email same way as accounts with unverified email would fit in nicely, and provide an additional, alternative warning with a clear hint to add an email now. It sounds reasonable, right?

Concerning the last issue of comment:4 - I searched the code, but found no "cleanup procedure" for attempted but not approved registrations (i.e. the verification code was sent, but never entered). Maybe I'll write a little script for that to be run via cron (or some similar scheduler). IMHO, if one didn't enter it within two weeks, it's unlikely it will happen later - so those accounts will most probably stay unused and thus could be removed to keep things clean.

+1 for this idea as well. Patch welcome.

[Steffen Hoffmann]

Replying to hasienda:

[...]

We might want to help user administration in how to deal with existing accounts without email as well. Optionally treating all accounts without email same way as accounts with unverified email would fit in nicely, and provide an additional, alternative warning with a clear hint to add an email now. It sounds reasonable, right?

This is even the default behavior right now, if you happen to add an email and remove it later: You're trapped in the verification procedure and need to

• disable verification or
• re-enter a valid email and finish the verification process

to re-establish your permissions as an authenticated user. Certainly today this is unintentionally, but I deduce from this findings that there is little to add to get to a saner state.

[anonymous]

I committed the latest version of this patch to ​http://bitbucket.org/larsivi/trac-accountmanager-plugin

[anonymous]

Replying to hasienda:

Replying to izzy:

Of course, nothing speaks against adding a config option for whether or not to run this check, if somebody will do that...

Sure, an option comes to mind. But how about coupling activation of verification module and position of email input in mandatory or optional section plus corresponding logic behind? IMHO enabling verification and making email required for new registrations in the same moment is exactly the desired, expected effect. At least this is the bare minimum.

I would suggest adding an option to DISABLE this default behaviour. While most people will use the email verification module because they require their users to enter a valid email address, some may use it to ensure that IF an email address is entered at all, it belongs to the user. This may be used f.ex. to ensure that their Trac installation is not abused to spam people.

I am not sure how the email verification interacts with email notifications at this point - does having an unverified address prevent them? Might be a good idea to check, just in case. Unfortunately I'm busy with other stuff at the moment, but I'll definitely come back to working on Trac at some point. If this is still open then, I'll happily chip in some more patches.

[Daniel Keyhani]

Oops, that would have been me posting.

[Steffen Hoffmann]

(In [9277]) AccountManagerPlugin: Enforce email verification, closes #5509.

These are the changes provided by izzy and updated by dake, just slightly modified to better fit to surrounding code. We still need to take care for a possible dead-lock situation, when notification is disabled, refs #3989.

[Steffen Hoffmann]

Replying to anonymous:

Replying to hasienda: ![...] I would suggest adding an option to DISABLE this default behaviour. ![...]

Done in [9304].

I am not sure how the email verification interacts with email notifications at this point - does having an unverified address prevent them? Might be a good idea to check, just in case.

Looks like this is the issue raised with #3989, right?

[Steffen Hoffmann]

(In [10519]) AccountManagerPlugin: Make option verify_email effective for RegistrationModule too, refs #3153, #3989, #5509 and #9051.

Only module state (enabled/disabled) has been checked before, when deciding on the email address field being optional vs. required since changeset [9304].

[Steffen Hoffmann]

(for the record: user izzy returned positive test feedback as well after fixing a typo in local configuration)