Modify

Opened 15 years ago

Closed 13 years ago

Last modified 10 years ago

#5509 closed enhancement (fixed)

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

Reported by: hanswurst Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: user verify email option
Cc: izzy, Carsten, Daniel Keyhani Trac Release: 0.12

Description

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.

Attachments (3)

web_ui.emailcheck.diff (593 bytes) - added by izzy 14 years ago.
Patch to force entering an email address on registration when EmailVerificationModule is enabled
registration_emailcheck.diff (2.5 KB) - added by izzy 14 years ago.
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)
require_email.diff (3.6 KB) - added by Daniel Keyhani 14 years ago.
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)

Download all attachments as: .zip

Change History (23)

comment:1 Changed 15 years ago by Sebastian Krysmanski

Simply enabling the component does the trick for me.

comment:2 Changed 14 years ago by 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)

comment:3 Changed 14 years ago by izzy

Cc: izzy added; anonymous removed

Changed 14 years ago by izzy

Attachment: web_ui.emailcheck.diff added

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

comment:4 Changed 14 years ago by 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?

comment:5 Changed 14 years ago by izzy

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

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

Changed 14 years ago by 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)

comment:6 Changed 14 years ago by 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.

comment:7 in reply to:  description Changed 14 years ago by 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.

comment:8 Changed 14 years ago by Carsten

Cc: Carsten added

comment:9 Changed 14 years ago by Pedro Ferreira <pedro.ferreira@…>

Could this please be integrated into the trunk?

Changed 14 years ago by Daniel Keyhani

Attachment: require_email.diff added

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)

comment:10 Changed 14 years ago by Daniel Keyhani

Cc: Daniel Keyhani added
Trac Release: 0.110.12
Type: defectenhancement

comment:11 in reply to:  9 Changed 14 years ago by Steffen Hoffmann

Keywords: user verify email option added
Owner: changed from John Hampton to Steffen Hoffmann
Status: newassigned

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.

comment:12 in reply to:  6 ; Changed 14 years ago by 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.

comment:13 in reply to:  12 Changed 14 years ago by 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.

comment:14 Changed 14 years ago by anonymous

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

comment:15 in reply to:  12 ; Changed 14 years ago by 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.

comment:16 in reply to:  15 Changed 14 years ago by Daniel Keyhani

Oops, that would have been me posting.

comment:17 Changed 13 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

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

comment:18 in reply to:  15 Changed 13 years ago by 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?

comment:19 Changed 13 years ago by 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].

comment:20 Changed 10 years ago by Steffen Hoffmann

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.