Modify

Opened 18 years ago

Closed 13 years ago

Last modified 19 months ago

#442 closed enhancement (fixed)

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

Reported by: Gunnar Wagenknecht <gunnar@…> Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: password reset email verify user change register
Cc: tekknokrat, Thijs Triemstra 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 Pedro Algarvio, aka, s0undt3ch 16 years ago.
enforce_emails.patch (11.6 KB) - added by Pedro Algarvio, aka, s0undt3ch 16 years ago.
fix_email_verification.patch (2.8 KB) - added by Pedro Algarvio, aka, s0undt3ch 16 years ago.
don't check against anonymous users

Download all attachments as: .zip

Change History (41)

comment:1 Changed 18 years ago by Matt Good

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

comment:2 Changed 18 years ago by Matt Good

#590 should be implemented in order to support this.

comment:3 Changed 18 years ago by Pedro Algarvio, aka, 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 18 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 18 years ago by Prasand J.

Trac Release: 0.90.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 18 years ago by Jonathan S. Shapiro

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 18 years ago by Jonathan S. Shapiro

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

comment:8 Changed 17 years ago by izzy

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 17 years ago by Matt Good

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 17 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 17 years ago by Matt Good

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 17 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 16 years ago by Pedro Algarvio, aka, 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 16 years ago by Pedro Algarvio, aka, s0undt3ch

comment:14 Changed 16 years ago by Pedro Algarvio, aka, s0undt3ch

Owner: changed from Matt Good to Pedro Algarvio, aka, s0undt3ch
Status: newassigned

Patch updated, more stable.

comment:15 Changed 16 years ago by Pedro Algarvio, aka, s0undt3ch

Owner: changed from Pedro Algarvio, aka, s0undt3ch to Matt Good
Status: assignednew

Sorry, wrongly reassigned the ticket to me.

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

comment:16 Changed 16 years ago by Matt Good

(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 16 years ago by Pedro Algarvio, aka, 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 16 years ago by Pedro Algarvio, aka, 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 16 years ago by Pedro Algarvio, aka, 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 16 years ago by Pedro Algarvio, aka, 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 16 years ago by Pedro Algarvio, aka, s0undt3ch

Attachment: enforce_emails.patch added

comment:21 Changed 16 years ago by Pedro Algarvio, aka, s0undt3ch

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

comment:22 Changed 16 years ago by tekknokrat

Cc: tekknokrat added; anonymous removed

comment:23 Changed 16 years ago by tekknokrat

Changed 16 years ago by Pedro Algarvio, aka, s0undt3ch

don't check against anonymous users

comment:24 Changed 16 years ago by Pedro Algarvio, aka, 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 Changed 16 years ago by Pedro Algarvio, aka, s0undt3ch

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

comment:26 in reply to:  25 Changed 16 years ago by Pedro Algarvio, aka, 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 16 years ago by John Hampton

(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 16 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 16 years ago by anonymous

Trac Release: 0.100.11

comment:30 Changed 16 years ago by anonymous

Trac Release: 0.110.10

comment:31 Changed 15 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 15 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:33 Changed 14 years ago by Steffen Hoffmann

Owner: changed from Matt Good to John Hampton

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 14 years ago by Steffen Hoffmann

Keywords: needinfo added; helpwanted removed
Owner: changed from John Hampton to Steffen Hoffmann

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

comment:35 in reply to:  16 Changed 14 years ago by Steffen Hoffmann

Keywords: password reset email verify user change register added; needinfo removed
Status: newassigned
Summary: Email verification[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 13 years ago by Steffen Hoffmann

Trac Release: 0.100.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 13 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

(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 13 years ago by Steffen Hoffmann

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

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.