Merge lp:~mhall119/django-openid-auth/quick-sequence-check into lp:~django-openid-auth/django-openid-auth/trunk

[Michael Hall]

skip ahead to a number that is likely available when generating a sequence number to append to a duplicate username

Overview
========
When you have multiple duplicates of username, the process of incrementing the appended number starting from i=0 can become quite time consuming and allows a race condition

Details
=======
Where there are many duplicates of a username, especially when using 'openiduser' for logins without a nickname, making a db call from 1 to n to find an available username causes a delay which allows the user to attempt the login again. Because we create the django.contrib.auth User first and then associate it with an openid, this duplicate request will create a duplicate User record but will then fail when assigning it to an openid.

This patch should drastically reduce the time it takes to identify an available username and closes the window where this race condition can occur.

[lp:~mhall119/django-openid-auth/quick-sequence-check]

89. By Michael Hall on 2011-09-29

Add test cases for optimized unique username sequence generation

[Anthony Lenton (elachuni)]

Thanks for the fix Michael!

review: Approve

=== modified file 'django_openid_auth/auth.py'
--- django_openid_auth/auth.py	2011-09-12 13:50:50 +0000
+++ django_openid_auth/auth.py	2011-09-29 17:40:28 +0000
@@ -198,8 +198,10 @@
                     "already in use for a different account." % nickname)
 
         # Pick a username for the user based on their nickname,
-        # checking for conflicts.
-        i = 1
+        # checking for conflicts.  Start with number of existing users who's
+        # username starts with this nickname to avoid having to iterate over
+        # all of the existing ones.
+        i = User.objects.filter(username__startswith=nickname).count() + 1
         while True:
             username = nickname
             if i > 1:

=== modified file 'django_openid_auth/tests/test_views.py'
--- django_openid_auth/tests/test_views.py	2011-09-12 13:50:50 +0000
+++ django_openid_auth/tests/test_views.py	2011-09-29 17:40:28 +0000
@@ -569,6 +569,98 @@
         self.assertEquals(user.last_name, 'User')
         self.assertEquals(user.email, '[e-mail]')
 
+    def test_login_duplicate_username_numbering(self):
+        settings.OPENID_FOLLOW_RENAMES = False
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to conflict with
+        user = User.objects.create_user('testuser', '[e-mail]')
+
+        # identity url is for 'renameuser'
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        # but returned username is for 'testuser', which already exists for another identity
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Test User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # Since this username is already taken by someone else, we go through
+        # the process of adding +i to it, and get testuser2.
+        self.assertEquals(response.content, 'testuser2')
+
+    def test_login_duplicate_username_numbering_with_conflicts(self):
+        settings.OPENID_FOLLOW_RENAMES = False
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to conflict with
+        user = User.objects.create_user('testuser', '[e-mail]')
+        user = User.objects.create_user('testuser3', '[e-mail]')
+
+        # identity url is for 'renameuser'
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        # but returned username is for 'testuser', which already exists for another identity
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Test User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # Since this username is already taken by someone else, we go through
+        # the process of adding +i to it starting with the count of users with
+        # username starting with 'testuser', of which there are 2.  i should
+        # start at 3, which already exists, so it should skip to 4.
+        self.assertEquals(response.content, 'testuser4')
+
+    def test_login_duplicate_username_numbering_with_holes(self):
+        settings.OPENID_FOLLOW_RENAMES = False
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to conflict with
+        user = User.objects.create_user('testuser', '[e-mail]')
+        user = User.objects.create_user('testuser1', '[e-mail]')
+        user = User.objects.create_user('testuser6', '[e-mail]')
+        user = User.objects.create_user('testuser7', '[e-mail]')
+        user = User.objects.create_user('testuser8', '[e-mail]')
+
+        # identity url is for 'renameuser'
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        # but returned username is for 'testuser', which already exists for another identity
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Test User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # Since this username is already taken by someone else, we go through
+        # the process of adding +i to it starting with the count of users with
+        # username starting with 'testuser', of which there are 5.  i should
+        # start at 6, and increment until it reaches 9.
+        self.assertEquals(response.content, 'testuser9')
+
+    def test_login_duplicate_username_numbering_with_nonsequential_matches(self):
+        settings.OPENID_FOLLOW_RENAMES = False
+        settings.OPENID_CREATE_USERS = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to conflict with
+        user = User.objects.create_user('testuser', '[e-mail]')
+        user = User.objects.create_user('testuserfoo', '[e-mail]')
+
+        # identity url is for 'renameuser'
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        # but returned username is for 'testuser', which already exists for another identity
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Test User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # Since this username is already taken by someone else, we go through
+        # the process of adding +i to it starting with the count of users with
+        # username starting with 'testuser', of which there are 2.  i should
+        # start at 3, which will be available.
+        self.assertEquals(response.content, 'testuser3')
+
     def test_login_follow_rename(self):
         settings.OPENID_FOLLOW_RENAMES = True
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = True