[patch] Add email verification for new/changed email addresses

#443 has been marked as a duplicate of this ticket.

#590 should be implemented in order to support this.

An alternate way would be to send to the user's email a link with a hash, ie, for example an md5sum of username+salt+pass+salt+email_address. When the user clicks the link he'll be directed to the verify page(handler) wich will ask them to submit username/pass/email/sent_hash if the generated hash for the users input matches the one sent to him, then register the user.

if u're going with salt way, make sure to enable 2 salt. allow using the new one while keeping the old one for a period, and dropped by hand.

Asking them to submit the hash on the verification page isn't needed. The email link can contain the hash, passing it to the verification page. Thus eliminating that extra step. If no hash was passed. Then return the user to the default wiki page, or whatever page is default (since there would be nothing on the verification page to see). If the hash is passed, return "registration successful" with a timed redirect to the default page.

It may present issues if the previous new user registration (but not-validated) is stored. As such, I propose that the verification page only processes the hash, and the salt is set in the trac.ini:

[account-manager]
verification_salt = ownersalt

If that entry (or something similar) is not set, then a random hash should be generated and that value is set. When the email verification hash is passed. The salt is recalled, the hash is decrypted, and the account is added or updated (therefore nothing needs to be stored about their "not yet verified" registration, as it's all in the hash). Any hash that is tampered with will result in it unable to be decrypted (as the decrypted string will not contain the salt).

-

This feature should also be able to be enabled or disabled. If enabled, then the text on the registration page (which indicates the email is optional) should be removed. Unless others think this should be a mandatory feature (and thus not able to be disabled).

Sticking a verification salt into the config file seems like the wrong thing. You want the salt to change on occasion. I suggest generating a new nonce on every account creation instead.

While md5sum is a fine implementation, there is no need for an HMAC here. What is needed is a nonce. If the various password backends will permit us to do it, the right way to go about this is to go ahead and create the account in a disabled state, recording all of the information about the account at that time, and then send the user an email with a nonce embedded in a URL. When the user clicks on the URL, we enable the account.

There are a couple of reasons to do it this way:

1. It closes a race condition. If we defer the actual account creation, somebody else could come in and grab the username before the user can confirm.
2. It immunizes us from local specializations of per-user information. If I decide locally that I want to collect a per-user credit card number or a reminder question or some such, the necessary changes do not propagate into the confirmation system.
3. There is no problem keeping multiple nonces -- the same mechanism still works.
4. The mechanism can be trivially extended to password updates if the per-user record contains a "pending new password" field.

To handle the nonces, we create a table of the form:

(nonce, username, expiration-time)

Periodically we clean the table to remove stale nonces. Also, we periodically clean out never-enabled accounts for which no outstanding nonce exists.

As to generating the nonce, I would use a simple random number, and I wouldn't get excited about making it too long -- an expiration of 24 hours should be ample.

Oh. It would also be simple to extend this for use as an administrator approval mechanism.

Is this issue still active? Concerning the spam I see in some places, I wouldn't like to let users create accounts without verification. So what is described here, is exactly what I'm looking for. And I hope it will be added for the 0.10 branch of trac as well. Any news on this?

Replying to izzysoft@qumran.org:

Is this issue still active?

If someone submits a patch I'll look at it, but I'm not actively working on this plugin at the moment. You could try the t:MailingList to see if any other devs are interested in working on this.

Consider this another vote for someone to work on this; I'd rather solve my trac-spam problems this way than any of the other solutions I've seen.

#2193 has been marked as a duplicate.

This is a good idea, but I don't have the time to focus on it right now. If someone provides a good patch I'll integrate it.

another vote! or some way to restrict regitered addresses to a specific domain would be nice too.

The above is a mercurial patch, to apply it use -p1 not -p0. It's meant for trac 0.11 ie, the trunk version of this plugin.

Still need to handle when a verified user changes the email address to his account.

Patch updated, more stable.

Sorry, wrongly reassigned the ticket to me.

Oh, and see the raw patch, it's a mercurial patch.

(In [3799]) add an email verification module re #442. Keeping the ticket open until this has been integrated with the admin modules too.

Replying to mgood:

(In [3799]) add an email verification module re #442. Keeping the ticket open until this has been integrated with the admin modules too.

I'm working on some changes to optionally enforce email submission on registration time.

Also, if user did submit his email, shouldn't the notification be sent right away, and not just when he has logged in for the first time? Sure this will complicate the code a bit more.

Oh and I think latest changes checked-in by mgood are for 0.11 only.

This last patch provides optional email enforcement, ie, require users to provide their email address when registering.

Config variable name might get a better name though.

Replying to mgood:

(In [3799]) add an email verification module re #442. Keeping the ticket open until this has been integrated with the admin modules too.

This code is also triggered for anonymous users when they change their email address, which brings another problem. Current notification code search emails for authenticated users only.

We either need to implement this for authenticated users only, or, provide the email address instead of the username:

diff --git a/acct_mgr/web_ui.py b/acct_mgr/web_ui.py
--- a/acct_mgr/web_ui.py
+++ b/acct_mgr/web_ui.py
@@ -527,14 +527,16 @@

     def post_process_request(self, req, template, data, content_type):
         if req.session.get('email') != req.session.get('email_verification_sent_to'):
-            req.session['email_verification_token'] = self._gen_token()
-            req.session['email_verification_sent_to'] = req.session.get('email')
-            self._send_email(req)
-            chrome.add_notice(req, MessageWrapper(tag.span(
-                    'An email has been sent to ', req.session['email'],
-                    ' with a token to ',
-                    tag.a(href=req.href.verify_email())(
-                        'verify your new email address'))))
+            if req.session.get('email'):
+                # only notify if user provided an email address
+                req.session['email_verification_token'] = self._gen_token()
+                req.session['email_verification_sent_to'] = req.session['email']
+                self._send_email(req)
+                chrome.add_notice(req, MessageWrapper(tag.span(
+                        'An email has been sent to ', req.session['email'],
+                        ' with a token to ',
+                        tag.a(href=req.href.verify_email())(
+                            'verify your new email address'))))
         elif (self.env.is_component_enabled(EmailVerificationModule) and
           NotificationSystem(self.env).smtp_enabled and
           self.env.config.getbool('account-manager', 'enforce_emails') and
@@ -575,5 +577,9 @@
         return base64.urlsafe_b64encode(urandom(6))

     def _send_email(self, req):
-        notifier = EmailVerificationNotification(self.env)
-        notifier.notify(req.authname, req.session['email_verification_token'])
+        username = req.authname
+        if username == 'anonymous':
+            username = req.session.get('email')
+        if username:
+            notifier = EmailVerificationNotification(self.env)
+            notifier.notify(username, req.session['email_verification_token'])

The more I work on this, the more I think that this should be into trac itself.

I think that this feature should be extended to anonymous users, ie, we should allow them to verify their address, and with that we should provide some more trust to that anonymous, yet verified user. Ie, another permission group besides anonymous and authenticated, verified.

Ie, verified addresses would for example be allowed to submit tickets, while anonymous could only view tickets.

But, I think that this would require internal trac changes? or am I wrong?

P.S.: with [3799] there's support for anonymous users, they'll get the warnings if they change their address, yet, they'll never get notified since they're not authenticated and current notification code does not handle this. The last patch I added, tries to handle it and also provides email enforcement for registred users(enforcement is not meant for anonymous users)

Ticket #3153 refers to this ticket. That ticket can actually be marked duplicate?

don't check against anonymous users

With the last patch, fix_email_verification.patch, email verification is not run against anonymous users.

Of course, this is the easy way out, because supporting email verification of anonymous users would probably need, to actually be useful, another virtual permission group, like authenticated, for which one could tweak permissions for.

Also, on an irc talk with coderanger, if you want verification to happen, limit what anonymous users can do, and allow them to register to be able to do more stuff like, edit wiki pages or create tickets.

I'm also going to create another ticket to include my enforce verification patch since its scope is another feature which is linked to this one but can be separate; although coderanger won't like this new changes, as he said, this library is getting too big, parts should be included in trac's core, while the features should be provided as plugins.

Oh, and the fix_email_verification.patch depends on the patch provided on #3137 :)

Replying to s0undt3ch:

Oh, and the fix_email_verification.patch depends on the patch provided on #3137 :)

P.S: Only because of the tests.

(In [3832])

• Apply patch from s0undt3ch removing email verification for anonymous users. Refs #442
• Apply patch from s0undt3ch implementing functional tests and fixing existing tests. Closes #3137.

Thanks for the patches s0undt3ch

#   File "/usr/lib64/python2.5/site-packages/Trac-0.11-py2.5.egg/trac/web/main.py", line 423, in  _dispatch_request
Code fragment:

 418. try:
 419. if not env and env_error:
 420. raise HTTPInternalError(env_error)
 421. try:
 422. dispatcher = RequestDispatcher(env)
 423. dispatcher.dispatch(req)
 424. except RequestDone:
 425. pass
 426. resp = req._response or []
 427.  
 428. except HTTPException, e:

Local variables:
Name	Value
after 	[u' except RequestDone:', u' pass', u' resp = ...
before 	[u' try:', u' if not env and env_error:', u' raise ...
dispatcher 	<trac.web.main.RequestDispatcher object at 0x168bf50>
e 	KeyError('email',)
env 	<trac.env.Environment object at 0xea6990>
env_error 	None
exc_info 	(<type 'exceptions.KeyError'>, KeyError('email',), <traceback object at ...
filename 	'/usr/lib64/python2.5/site-packages/Trac-0.11-py2.5.egg/trac/web/main.py'
frames 	[{'function': '_dispatch_request', 'lines_before': [u' try:', u' ...
has_admin 	True
line 	u' dispatcher.dispatch(req)'
lineno 	422
message 	u"KeyError: 'email'"
req 	<Request "GET u'/prefs'">
resp 	[]
tb 	<traceback object at 0x1b6ffc8>
tb_hide 	None
traceback 	'Traceback (most recent call last):\n File ...
# File "/usr/lib64/python2.5/site-packages/Trac-0.11-py2.5.egg/trac/web/main.py", line 209, in dispatch
Code fragment:

 204. if req.session:
 205. req.session.save()
 206. req.display(template, content_type or 'text/html')
 207. else: # Genshi
 208. template, data, content_type = \
 209. self._post_process_request(req, *resp)
 210. if 'hdfdump' in req.args:
 211. req.perm.require('TRAC_ADMIN')
 212. # debugging helper - no need to render first
 213. from pprint import pprint
 214. out = StringIO()

Local variables:
Name	Value
chosen_handler 	<trac.prefs.web_ui.PreferencesModule object at 0x16e0350>
chrome 	<trac.web.chrome.Chrome object at 0x168b210>
err 	(<type 'exceptions.KeyError'>, KeyError('email',), <traceback object at ...
handler 	<trac.prefs.web_ui.PreferencesModule object at 0x16e0350>
req 	<Request "GET u'/prefs'">
resp 	('prefs_general.html', {'localtz': <trac.util.datefmt.LocalTimezone object ...
self 	<trac.web.main.RequestDispatcher object at 0x168bf50>
# File "/usr/lib64/python2.5/site-packages/Trac-0.11-py2.5.egg/trac/web/main.py", line 299, in _post_process_request
Code fragment:

 294. # Trac 0.10, only filters with same arity gets passed real values.
 295. # Errors will call all filters with None arguments,
 296. # and results will not be not saved.
 297. extra_arg_count = arity(f.post_process_request) - 2
 298. if extra_arg_count == nbargs:
 299. resp = f.post_process_request(req, *resp)
 300. elif nbargs == 0:
 301. f.post_process_request(req, *(None,)*extra_arg_count)
 302. return resp
 303.  
 304.  

Local variables:
Name	Value
args 	('prefs_general.html', {'localtz': <trac.util.datefmt.LocalTimezone object ...
extra_arg_count 	3
f 	<acct_mgr.web_ui.EmailVerificationModule object at 0x16e0610>
nbargs 	3
req 	<Request "GET u'/prefs'">
resp 	('prefs_general.html', {'localtz': <trac.util.datefmt.LocalTimezone object ...
self 	<trac.web.main.RequestDispatcher object at 0x168bf50>
# File "build/bdist.linux-x86_64/egg/acct_mgr/web_ui.py", line 541, in post_process_request 

• acct_mgr/web_ui.py

  old	new
  533	533	# that anonymous users can't edit wiki pages and change or create
  534	534	# tickets. As such, this email verifying code won't be used on them
  535	535	return template, data, content_type
  536		if req.session.get('email') != req.session.get('email_verification_sent_to'):
  	536	if req.session.get('email') and req.session.get('email') != req.session.get('email_verification_sent_to'):
  537	537	req.session['email_verification_token'] = self._gen_token()
  538	538	req.session['email_verification_sent_to'] = req.session.get('email')
  539	539	self._send_email(req)

What is the status of enforce_emails.patch?

In comment:24, s0undt3ch said:

I'm also going to create another ticket to include my enforce verification patch since its scope is another feature which is linked to this one but can be separate

But I've failed to locate that other ticket.

Not having the functionality from this patch makes requiring email verification much less useful. If there are problems with the patch, I'm happy to look at trying to address them, but I need to know what they are in order to do so.

I'm a little bit confused about the state of this ticket too.

I've seen related changes in current code and fail to understand, if summary or keywords should be improved, what comment 28 is all about, or if we'd need a new ticket with better description to start it again. Comments?

BTW, watch ticket:4286 for a similar issue raised against Trac itself.

Answering my own question from comment 33 here.

Replying to mgood:

(In [3799]) add an email verification module re #442. Keeping the ticket open until this has been integrated with the admin modules too.

So this one seems to be left and waiting for a solution. I think, we could fix different aspects of this with #843 and #7111. Comments?

I withdraw my assumption on connections with other tickets. At least #843 has a quite different subject.

By now we're able to switch this feature off/on in configuration admin page and see details per account on the account details admin page. I don't feel like there is more integration needed right now. If you can proof me wrong, better open a dedicated new ticket describing exactly the missing point(s). Thank you for taking care.

Ah, I seriously doubt that there is great demand for a backport to 0.10 as initially requestet, isn’t' it?

(In [10393]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.

(In [10395]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.