Modify

Opened 5 years ago

Closed 4 years ago

Last modified 10 months ago

#5509 closed enhancement (fixed)

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

Reported by: hanswurst Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: user verify email option
Cc: izzy, Carsten, dake 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 5 years ago.
Patch to force entering an email address on registration when EmailVerificationModule is enabled
registration_emailcheck.diff (2.5 KB) - added by izzy 5 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 dake 4 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 5 years ago by manski

Simply enabling the component does the trick for me.

comment:2 Changed 5 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 5 years ago by izzy

  • Cc izzy added

Changed 5 years ago by izzy

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

comment:4 Changed 5 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 5 years ago by izzy

  • Summary changed from EmailVerificationModule not documented to EmailVerificationModule 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 5 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 follow-up: Changed 5 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 5 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 5 years ago by Carsten <CarstenFuchs@…>

  • Cc Carsten added

comment:9 follow-up: Changed 4 years ago by Pedro Ferreira <pedro.ferreira@…>

Could this please be integrated into the trunk?

Changed 4 years ago by dake

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 4 years ago by dake

  • Cc dake added
  • Trac Release changed from 0.11 to 0.12
  • Type changed from defect to enhancement

comment:11 in reply to: ↑ 9 Changed 4 years ago by hasienda

  • Keywords user verify email option added
  • Owner changed from pacopablo to hasienda
  • Status changed from new to assigned

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 ; follow-ups: Changed 4 years ago by hasienda

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 4 years ago by hasienda

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 4 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 ; follow-ups: Changed 4 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 4 years ago by dake

Oops, that would have been me posting.

comment:17 Changed 4 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 4 years ago by hasienda

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 3 years ago by hasienda

(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 months ago by hasienda

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

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.