Merge lp:~mhall119/django-openid-auth/strict-username-requirements into lp:~django-openid-auth/django-openid-auth/trunk

[Michael Hall]

Adds an extra OPENID_STRICT_USERNAMES setting that will cause the authentication to fail if the OpenID response does not contain a nickname (used for User.username in Django), or if a Django User with that username already exists but doesn't have the same OpenID identity_url

[Anthony Lenton (elachuni)]

Tests would be nice, but it looks ok to me :)

review: Approve

[James Henstridge (jamesh)]

I agree with Anthony here.

Here is a quick sketch for the test:

1. add a test to test_views.py. Using test_login_create_users() as a base would probably be easiest.
2. create a User and UserOpenID object with a given nickname and identity.
3. set OPENID_STRICT_USERNAMES, and try to log in using a different identity URL but the same nickname
4. verify that the login has failed.

After adding the test, this change looks fine.

review: Needs Fixing

[Anthony Lenton (elachuni)]

Hi Michael,

With the tests this now looks fine. Could you add a brief snippet to README.txt to mention this new setting and what it does?

[lp:~mhall119/django-openid-auth/strict-username-requirements]

81. By Michael Hall on 2011-03-23

Added notes about the new settings option to README.txt

[Anthony Lenton (elachuni)]

review: Approve

[James Henstridge (jamesh)]

Looks good. In test_strict_username_no_nickname, I'd leave out the nickname from the SReg response entirely though, since that is what you'd see from a provider that doesn't support nicknames.

review: Approve

=== modified file 'README.txt'
--- README.txt	2010-05-25 07:24:14 +0000
+++ README.txt	2011-03-23 19:28:45 +0000
@@ -125,3 +125,13 @@
 It is worth noting that a user needs to be be marked as a "staff user" to be able to access the admin interface.  A new openid user will not normally be a "staff user".  
 The easiest way to resolve this is to use traditional authentication (OPENID_USE_AS_ADMIN_LOGIN = False) to sign in as your first user with a password and authorise your 
 openid user to be staff.
+
+== Require a valid nickname ==
+
+If you must have a valid, unique nickname in order to create a user accont, add the following setting:
+
+        OPENID_STRICT_USERNAMES = True
+        
+This will cause an OpenID login attempt to fail if the provider does not return a 'nickname' (username) for the user, or if the nickname conflicts with an existing user with a different openid identiy url.
+Without this setting, logins without a nickname will be given the username 'openiduser', and upon conflicts with existing username, an incrementing number will be appended to the username until it is unique.
+

=== modified file 'django_openid_auth/auth.py'
--- django_openid_auth/auth.py	2010-10-15 09:08:10 +0000
+++ django_openid_auth/auth.py	2011-03-23 19:28:45 +0000
@@ -42,7 +42,9 @@
 class IdentityAlreadyClaimed(Exception):
     pass
 
-
+class StrictUsernameViolation(Exception):
+    pass
+    
 class OpenIDBackend:
     """A django.contrib.auth backend that authenticates the user based on
     an OpenID response."""
@@ -72,7 +74,10 @@
                 claimed_id__exact=openid_response.identity_url)
         except UserOpenID.DoesNotExist:
             if getattr(settings, 'OPENID_CREATE_USERS', False):
-                user = self.create_user_from_openid(openid_response)
+                try:
+                    user = self.create_user_from_openid(openid_response)
+                except StrictUsernameViolation:
+                    return None
         else:
             user = user_openid.user
 
@@ -142,6 +147,12 @@
         nickname = details['nickname'] or 'openiduser'
         email = details['email'] or ''
 
+        if getattr(settings, 'OPENID_STRICT_USERNAMES', False):
+            if details['nickname'] is None or details['nickname'] == '':
+                raise StrictUsernameViolation("No username")
+            if User.objects.filter(username__exact=nickname).count() > 0:
+                raise StrictUsernameViolation("Duplicate username: %s" % nickname)
+                
         # Pick a username for the user based on their nickname,
         # checking for conflicts.
         i = 1

=== modified file 'django_openid_auth/tests/test_views.py'
--- django_openid_auth/tests/test_views.py	2011-01-13 08:38:01 +0000
+++ django_openid_auth/tests/test_views.py	2011-03-23 19:28:45 +0000
@@ -131,12 +131,14 @@
 
         self.old_login_redirect_url = getattr(settings, 'LOGIN_REDIRECT_URL', '/accounts/profile/')
         self.old_create_users = getattr(settings, 'OPENID_CREATE_USERS', False)
+        self.old_strict_usernames = getattr(settings, 'OPENID_STRICT_USERNAMES', False)
         self.old_update_details = getattr(settings, 'OPENID_UPDATE_DETAILS_FROM_SREG', False)
         self.old_sso_server_url = getattr(settings, 'OPENID_SSO_SERVER_URL', None)
         self.old_teams_map = getattr(settings, 'OPENID_LAUNCHPAD_TEAMS_MAPPING', {})
         self.old_use_as_admin_login = getattr(settings, 'OPENID_USE_AS_ADMIN_LOGIN', False)
 
         settings.OPENID_CREATE_USERS = False
+        settings.OPENID_STRICT_USERNAMES = False
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = False
         settings.OPENID_SSO_SERVER_URL = None
         settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = {}
@@ -145,6 +147,7 @@
     def tearDown(self):
         settings.LOGIN_REDIRECT_URL = self.old_login_redirect_url
         settings.OPENID_CREATE_USERS = self.old_create_users
+        settings.OPENID_STRICT_USERNAMES = self.old_strict_usernames
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = self.old_update_details
         settings.OPENID_SSO_SERVER_URL = self.old_sso_server_url
         settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = self.old_teams_map
@@ -286,6 +289,62 @@
         self.assertEquals(user.last_name, 'User')
         self.assertEquals(user.email, '[e-mail]')
 
+    def test_strict_username_no_nickname(self):
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_STRICT_USERNAMES = True
+
+        # Posting in an identity URL begins the authentication request:
+        response = self.client.post('/openid/login/',
+            {'openid_identifier': 'http://example.com/identity',
+             'next': '/getuser/'})
+        self.assertContains(response, 'OpenID transaction in progress')
+
+        # Complete the request, passing back some simple registration
+        # data.  The user is redirected to the next URL.
+        openid_request = self.provider.parseFormPost(response.content)
+        sreg_request = sreg.SRegRequest.fromOpenIDRequest(openid_request)
+        openid_response = openid_request.answer(True)
+        sreg_response = sreg.SRegResponse.extractResponse(
+            sreg_request, {'nickname': '', # No nickname
+                           'fullname': 'Some User',
+                           'email': '[e-mail]'})
+        openid_response.addExtension(sreg_response)
+        response = self.complete(openid_response)
+        
+        # Status code should be 403: Forbidden
+        self.assertEquals(403, response.status_code)
+
+    def test_strict_username_duplicate_user(self):
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_STRICT_USERNAMES = True
+        # Create a user with the same name as we'll pass back via sreg.
+        user = User.objects.create_user('someuser', '[e-mail]')
+        useropenid = UserOpenID(
+            user=user,
+            claimed_id='http://example.com/different_identity',
+            display_id='http://example.com/different_identity')
+        useropenid.save()
+
+        # Posting in an identity URL begins the authentication request:
+        response = self.client.post('/openid/login/',
+            {'openid_identifier': 'http://example.com/identity',
+             'next': '/getuser/'})
+        self.assertContains(response, 'OpenID transaction in progress')
+
+        # Complete the request, passing back some simple registration
+        # data.  The user is redirected to the next URL.
+        openid_request = self.provider.parseFormPost(response.content)
+        sreg_request = sreg.SRegRequest.fromOpenIDRequest(openid_request)
+        openid_response = openid_request.answer(True)
+        sreg_response = sreg.SRegResponse.extractResponse(
+            sreg_request, {'nickname': 'someuser', 'fullname': 'Some User',
+                           'email': '[e-mail]'})
+        openid_response.addExtension(sreg_response)
+        response = self.complete(openid_response)
+        
+        # Status code should be 403: Forbidden
+        self.assertEquals(403, response.status_code)
+
     def test_login_update_details(self):
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
         user = User.objects.create_user('testuser', '[e-mail]')