623783 - Ensure email addresses passed in unicode or punycode hash to the same userid

Status: RESOLVED FIXED

(Cloud Services :: Server: Identity, defect)

Description

[JR Conlin [:jrconlin,:jconlin]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=623777 for description

Comment 1

[JR Conlin [:jrconlin,:jconlin]]

Attachment: updates to two files

These have been pending for a bit. I'd like to submit them so I don't build up a huge changelist. Obviously, they depend on the IDN hasher being present in the services libraries.

Comment 2

[Tarek Ziadé (:tarek)]

Comment on attachment 503865 [details] [diff] [review]
updates to two files

>Index: /home/jconlin/src/mozilla/identity/server-identity/oidserver/controllers/auth.py
>--- a/oidserver/controllers/auth.pyWed Jan 05 14:18:03 2011 +0100
>+++ b/oidserver/controllers/auth.pyFri Jan 14 08:26:00 2011 -0800
>@@ -1,10 +1,12 @@
> from webob.exc import HTTPTemporaryRedirect
> from webob import Response
> 
>-from services.util import text_response
>+from services.util import text_response,extract_username
> from oidserver.util import get_template
> from oidserver.controllers import BaseController

Use your custom logger instead

>+import logging
>+
> 
> class AuthController(BaseController):
> 
>@@ -12,13 +14,14 @@
>         if self.logged(request) is not None:
>             request.environ['beaker.session'].delete()
>         return text_response('You have been logged out.')
>-
>+  
>     def login(self, request, **kw):
>         redirect = request.environ['beaker.session'].get('redirect')
> 
>         if 'email' in request.POST:
>-            # XXX need to base32 it
>             email = request.POST['email']

you should catch the unicode error here

>+            username = extract_username(email)
>+            logging.info("Converted '%s' to '%s'" % (email, username))
>             password = request.POST['password']
>             uid = self.app.authtool.authenticate_user(email, password)
> 
>#===
>Index: /home/jconlin/src/mozilla/identity/server-identity/deps/server-core/services/wsgiauth.py
>--- a/services/wsgiauth.pyThu Jan 06 22:52:43 2011 +0100
>+++ b/services/wsgiauth.pyFri Jan 14 08:36:35 2011 -0800
>@@ -43,6 +43,7 @@
> 
> from services.auth import get_auth
> from services.cef import log_failure
>+from services.util import extract_username
> 
> 
> class Authentication(object):
>@@ -105,10 +106,18 @@
>                             config)
>                 raise HTTPUnauthorized()

Same remark than the other patch: the original user_name is used just here 
so no need to add it in the environ

>+            # if this is an email, hash it. Save the original for logging and debugging.
>+            if "@" in user_name:
>+                environ['REMOTE_USER_ORIGINAL'] = user_name
>+            user_name = extract_username(user_name)
>+            
>             # let's try an authentication
>             user_id = self.backend.authenticate_user(user_name, password)
>             if user_id is None:
>-                log_failure('Authentication Failed', 5, environ, config)
>+                err = 'Authentication Failed for Backend service ' + user_name
>+                if (user_name != environ.get('REMOTE_USER_ORIGINAL')):
>+                    err += ' (%s)' % (environ.get('REMOTE_USER_ORIGINAL','Undefined'))
>+                log_failure(err, 5, environ, config)
>                 raise HTTPUnauthorized()
> 
>             # we're all clear ! setting up REMOTE_USER
>
>

Comment 3

[JR Conlin [:jrconlin,:jconlin]]

Attachment: Additional changes per final resoultion to 623777

- converted logger to internal one. (Added a comment for personal clarity, but it may help others in the future.)
- flake8 checked.
- Other changes were already made, including a deeper diff to illustrate.

Comment 4

[Tarek Ziadé (:tarek)]

Comment on attachment 506469 [details] [diff] [review]
Additional changes per final resoultion to 623777

>--- a/oidserver/controllers/auth.py     Mon Nov 29 11:48:59 2010 +0100
>+++ b/oidserver/controllers/auth.py     Mon Jan 24 11:38:11 2011 -0800
>@@ -1,10 +1,13 @@
>-from webob.exc import HTTPTemporaryRedirect
>+from webob.exc import HTTPTemporaryRedirect, HTTPBadRequest
> from webob import Response
> 
>-from services.util import text_response
>+from services.util import text_response, extract_username
> from oidserver.util import get_template
> from oidserver.controllers import BaseController
> 
>+# Using the private logger defined in oidserver.__init__
>+from oidserver import logger
>+
> 
> class AuthController(BaseController):
> 
>@@ -17,10 +20,14 @@
>         redirect = request.environ['beaker.session'].get('redirect')
> 
>         if 'email' in request.POST:
>-            # XXX need to base32 it
>             email = request.POST['email']
>             password = request.POST['password']
>-            uid = self.app.authtool.authenticate_user(email, password)


>+            try:
>+                username = extract_username(email)
>+            except UnicodeError:

Maybe we should add a logger.error() here ?


>+                raise HTTPBadRequest('Invalid username')

From previous discussions, this log seems superfluous.
 

The rest of the patch looks good

Comment 5

[Tarek Ziadé (:tarek)]

=> From previous discussions, this log seems superfluous.

My comment was truncated: I was referring to the logger.debug here

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

Submmitted: 55:edbd37b1fe20