#442 closed enhancement (fixed)
[patch] Add email verification for new/changed email addresses
Reported by: | 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)
Change History (41)
comment:1 Changed 18 years ago by
comment:3 Changed 18 years ago by
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
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 Changed 18 years ago by
Trac Release: | 0.9 → 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 18 years ago by
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:
- 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.
- 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.
- There is no problem keeping multiple nonces -- the same mechanism still works.
- 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
Oh. It would also be simple to extend this for use as an administrator approval mechanism.
comment:8 follow-up: 9 Changed 17 years ago by
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 Changed 17 years ago by
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
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
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
another vote! or some way to restrict regitered addresses to a specific domain would be nice too.
comment:13 Changed 16 years ago by
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
Attachment: | verify_account_registrations.patch added |
---|
comment:14 Changed 16 years ago by
Owner: | changed from Matt Good to Pedro Algarvio, aka, s0undt3ch |
---|---|
Status: | new → assigned |
Patch updated, more stable.
comment:15 Changed 16 years ago by
Owner: | changed from Pedro Algarvio, aka, s0undt3ch to Matt Good |
---|---|
Status: | assigned → new |
Sorry, wrongly reassigned the ticket to me.
Oh, and see the raw patch, it's a mercurial patch.
comment:16 follow-ups: 17 35 Changed 16 years ago by
comment:17 Changed 16 years ago by
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
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
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 527 527 528 528 def post_process_request(self, req, template, data, content_type): 529 529 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')))) 538 540 elif (self.env.is_component_enabled(EmailVerificationModule) and 539 541 NotificationSystem(self.env).smtp_enabled and 540 542 self.env.config.getbool('account-manager', 'enforce_emails') and … … 575 577 return base64.urlsafe_b64encode(urandom(6)) 576 578 577 579 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
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
Attachment: | enforce_emails.patch added |
---|
comment:21 Changed 16 years ago by
Ticket #3153 refers to this ticket. That ticket can actually be marked duplicate?
comment:22 Changed 16 years ago by
Cc: | tekknokrat added; anonymous removed |
---|
comment:23 Changed 16 years ago by
Changed 16 years ago by
Attachment: | fix_email_verification.patch added |
---|
don't check against anonymous users
comment:24 Changed 16 years ago by
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: 26 Changed 16 years ago by
comment:26 Changed 16 years ago by
comment:27 Changed 16 years ago by
comment:28 Changed 16 years ago by
# 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
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)
comment:29 Changed 16 years ago by
Trac Release: | 0.10 → 0.11 |
---|
comment:30 Changed 16 years ago by
Trac Release: | 0.11 → 0.10 |
---|
comment:31 Changed 16 years ago by
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
Cc: | Thijs Triemstra added |
---|
comment:33 Changed 14 years ago by
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
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 Changed 14 years ago by
Keywords: | password reset email verify user change register added; needinfo removed |
---|---|
Status: | new → assigned |
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
Trac Release: | 0.10 → 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 13 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → 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 13 years ago by
(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.
#443 has been marked as a duplicate of this ticket.