Modify

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#442 closed enhancement (fixed)

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

Reported by: Gunnar Wagenknecht <gunnar@…> Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: password reset email verify user change register
Cc: tekknokrat, thijs Trac Release: 0.11

Description

It would be nice if new registered users could be verified by mail.

For example, generate password and send it via mail. But in this case every email address change must be verified thereafter.

Attachments (3)

verify_account_registrations.patch (30.3 KB) - added by s0undt3ch 6 years ago.
enforce_emails.patch (11.6 KB) - added by s0undt3ch 6 years ago.
fix_email_verification.patch (2.8 KB) - added by s0undt3ch 6 years ago.
don't check against anonymous users

Download all attachments as: .zip

Change History (41)

comment:1 Changed 8 years ago by mgood

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

comment:2 Changed 8 years ago by mgood

#590 should be implemented in order to support this.

comment:3 Changed 8 years ago by s0undt3ch

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.

comment:4 Changed 8 years ago by moo

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.

comment:5 in reply to: ↑ description Changed 8 years ago by prasand

  • Trac Release changed from 0.9 to 0.10

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

comment:6 Changed 8 years ago by shap

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.

comment:7 Changed 8 years ago by shap

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

comment:8 follow-up: Changed 7 years ago by izzysoft@…

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?

comment:9 in reply to: ↑ 8 Changed 7 years ago by mgood

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.

comment:10 Changed 7 years ago by anonymous

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.

comment:11 Changed 7 years ago by mgood

  • Keywords helpwanted added

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

comment:12 Changed 7 years ago by stijn.verstraeten@…

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

comment:13 Changed 6 years ago by s0undt3ch

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.

Changed 6 years ago by s0undt3ch

comment:14 Changed 6 years ago by s0undt3ch

  • Owner changed from mgood to s0undt3ch
  • Status changed from new to assigned

Patch updated, more stable.

comment:15 Changed 6 years ago by s0undt3ch

  • Owner changed from s0undt3ch to mgood
  • Status changed from assigned to new

Sorry, wrongly reassigned the ticket to me.

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

comment:16 follow-ups: Changed 6 years ago by mgood

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

comment:17 in reply to: ↑ 16 Changed 6 years ago by s0undt3ch

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.

comment:18 Changed 6 years ago by s0undt3ch

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.

comment:19 Changed 6 years ago by s0undt3ch

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:

  • acct_mgr/web_ui.py

    diff --git a/acct_mgr/web_ui.py b/acct_mgr/web_ui.py
    a b  
    527527 
    528528    def post_process_request(self, req, template, data, content_type): 
    529529        if req.session.get('email') != req.session.get('email_verification_sent_to'): 
    530             req.session['email_verification_token'] = self._gen_token() 
    531             req.session['email_verification_sent_to'] = req.session.get('email') 
    532             self._send_email(req) 
    533             chrome.add_notice(req, MessageWrapper(tag.span( 
    534                     'An email has been sent to ', req.session['email'], 
    535                     ' with a token to ', 
    536                     tag.a(href=req.href.verify_email())( 
    537                         'verify your new email address')))) 
     530            if req.session.get('email'): 
     531                # only notify if user provided an email address 
     532                req.session['email_verification_token'] = self._gen_token() 
     533                req.session['email_verification_sent_to'] = req.session['email'] 
     534                self._send_email(req) 
     535                chrome.add_notice(req, MessageWrapper(tag.span( 
     536                        'An email has been sent to ', req.session['email'], 
     537                        ' with a token to ', 
     538                        tag.a(href=req.href.verify_email())( 
     539                            'verify your new email address')))) 
    538540        elif (self.env.is_component_enabled(EmailVerificationModule) and 
    539541          NotificationSystem(self.env).smtp_enabled and 
    540542          self.env.config.getbool('account-manager', 'enforce_emails') and 
     
    575577        return base64.urlsafe_b64encode(urandom(6)) 
    576578 
    577579    def _send_email(self, req): 
    578         notifier = EmailVerificationNotification(self.env) 
    579         notifier.notify(req.authname, req.session['email_verification_token']) 
     580        username = req.authname 
     581        if username == 'anonymous': 
     582            username = req.session.get('email') 
     583        if username: 
     584            notifier = EmailVerificationNotification(self.env) 
     585            notifier.notify(username, req.session['email_verification_token']) 

comment:20 Changed 6 years ago by s0undt3ch

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)

Changed 6 years ago by s0undt3ch

comment:21 Changed 6 years ago by s0undt3ch

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

comment:22 Changed 6 years ago by tekknokrat

  • Cc gunnar_thielebein@… added

comment:23 Changed 6 years ago by tekknokrat

  • Cc tekknokrat added; gunnar_thielebein@… removed

Changed 6 years ago by s0undt3ch

don't check against anonymous users

comment:24 Changed 6 years ago by s0undt3ch

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.

comment:25 follow-up: Changed 6 years ago by s0undt3ch

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

comment:26 in reply to: ↑ 25 Changed 6 years ago by s0undt3ch

Replying to s0undt3ch:

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

P.S: Only because of the tests.

comment:27 Changed 6 years ago by pacopablo

(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

comment:28 Changed 6 years ago by anonymous

#   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

     
    533533            # that anonymous users can't edit wiki pages and change or create 
    534534            # tickets. As such, this email verifying code won't be used on them 
    535535            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'): 
    537537            req.session['email_verification_token'] = self._gen_token() 
    538538            req.session['email_verification_sent_to'] = req.session.get('email') 
    539539            self._send_email(req) 

comment:29 Changed 6 years ago by anonymous

  • Trac Release changed from 0.10 to 0.11

comment:30 Changed 6 years ago by anonymous

  • Trac Release changed from 0.11 to 0.10

comment:31 Changed 6 years ago by olly@…

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.

comment:32 Changed 5 years ago by thijs

  • Cc thijs added

comment:33 Changed 4 years ago by hasienda

  • Owner changed from mgood to pacopablo

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?

comment:34 Changed 4 years ago by hasienda

  • Keywords needinfo added; helpwanted removed
  • Owner changed from pacopablo to hasienda

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

comment:35 in reply to: ↑ 16 Changed 4 years ago by hasienda

  • Keywords password reset email verify user change register added; needinfo removed
  • Status changed from new to assigned
  • Summary changed from Email verification to [patch] Add email verification for new/changed email addresses

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?

comment:36 Changed 3 years ago by hasienda

  • Trac Release changed from 0.10 to 0.11

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?

comment:37 Changed 3 years ago by hasienda

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

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

comment:38 Changed 3 years ago by hasienda

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

Add Comment

Modify Ticket

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