Merge lp:~mhall119/django-openid-auth/fixes-642132 into lp:~django-openid-auth/django-openid-auth/trunk

[Michael Hall]

Adds a new settings paramater: OPENID_FOLLOW_RENAMES

When set to True, and when OPENID_UPDATE_DETAILS_FROM_SREG is also True, the django User.username for the User with a matching identity_url will be updated with the nickname from the OpenID response.

[Michael Hall (mhall119)]

Can somebody please review this, it is needed for loco.ubuntu.com and summit.ubuntu.com

[Anthony Lenton (elachuni)]

Hi Michael,

Thanks for your work on this. The code around lines 8-10 of the diff would need to be a bit more robust when updating the User's username, as there might be some other user in the system with the new username already.

I think the underlying problem is what we really want to do in this case (ie if 'joesmith' changes his openid username to 'joebobs', but you already have a 'joebobs' in your system). Skipping the username update in that case would work but it'll leave you with out-of-sync data. Refusing to sign the user in would be quite violent, and merging the two accounts would be definitely a bad idea. What would work in your use case?

Sites with a single OpenID provider would be less affected by this issue, but it could still bite you.

Otoh, I'm not sure why django-openid-auth doesn't currently update username together with fullname and email when OPENID_UPDATE_DETAILS_FROM_SREG is True, but using a second setting for this as your code does seems like a good way to make it work for everybody.

[Michael Hall (mhall119)]

I think in the case of an existing username conflict, then we could go back to $username+1 increments until we find a unique one. Unfortunately it would do this incrementing process every time the user logged in, until their new LP username is available in Django, but I don't think that would be such a performance hit.

[James Henstridge (jamesh)]

This should do the check to see if anyone else has the same user name, and should ideally use the same name checking code as create_user_from_openid() does (that way we only need to update the algorithm once if we want to change it).

I would suggest only changing the user name if the existing user name does not have the sreg nickname as a prefix. This should prevent us from trying to change the nickname on every login if there is a duplicate nickname.

With respect to the tests, how about factoring out the body of test_login_update_details() into a helper method, from the first client.post() call and returning the response object returned by client.get('/getuser/'). That helper could then be used to run both tests. I'd also like to see a test of when there is a conflict for the new nickname.

Lastly, a more basic question: do we want to have a setting for this behaviour? If it has minimal overhead and is something most people would want, is there any reason not to always do this?

review: Needs Fixing

[Michael Hall (mhall119)]

There may be cases where renames aren't desirable, for example if the username is used to link tables, rather than a constant user id.

[lp:~mhall119/django-openid-auth/fixes-642132]

75. By Anthony Lenton on 2011-01-05

Merged in lp:~stuartmetcalfe/django-openid-auth/staff-assignment

76. By Anthony Lenton on 2011-01-07

Merged in lp:~elachuni/django-openid-auth/django1.1-support

77. By Anthony Lenton on 2011-01-12

Merged in lp:~michael.nelson/django-openid-auth/701484-optional-sreg-fields

78. By Anthony Lenton on 2011-01-12

Merged lp:~michael.nelson/django-openid-auth/701489-fire-event-with-sreg-response

79. By Anthony Lenton on 2011-01-13

Merged in lp:~michael.nelson/django-openid-auth/701489-rename-signal-provide-openid-response

[Ronnie (ronnie.vd.c)]

I would suggest checking the OpenId-Provider (url like: https://login.launchpad.net/) from the claimed id.

If the new username exists, but claimed_id does not match (but is from the same provider), The old username should be called OpenIdUserX,

OPENIDPROVIDER1 (Google)
    - {'nickname': 'Ben', 'claimed_id': 'http://login.google.com/ABCD', 'display_id': '...'}

OPENIDPROVIDER2 (Launchpad)
    - {'nickname': 'Ben', 'claimed_id': 'https://login.launchpad.net/+id/AAAA', 'display_id': '...'}
    - {'nickname': 'Cees', 'claimed_id': 'https://login.launchpad.net/+id/ABCD', 'display_id': '...'}
======
    - Ben deletes his account on launchpad
    - Cees changed name to Ben

OPENIDCONSUMER
    - Ben (http://login.google.com/ABCD) register => username 'Ben'
    - Ben (https://login.launchpad.net/+id/AAAA) registers => username 'Ben1'
    - Cees (https://login.launchpad.net/+id/ABCD) registers => username 'Cees'
=======
    - Ben (previous Cees | https://login.launchpad.net/+id/ABCD) logs in =>
        -- user 'Ben1' should be renamed to 'OpenIdUserX'
        -- user Cees should be renamed to 'Ben1'

small catch: What to do if Ben (https://login.launchpad.net/+id/AAAA) changes his name to 'Ben1' on the openid-provider?

[Ronnie (ronnie.vd.c)]

A more general approach, which also solves the small catch:

The username should always be renamed if it has changed, one exception for this is when the same username exists for another openid-provider. If there is already an user with that username (same provider) then the old_user should be renamed to the name without number (if available) else to the name + first_available_number.

If the same username is kept by another openid-provider, the user should keep its old username (if exists), if a new user is created, it should be username1 (username2 etc which first is available)

Therefore in the example above:
  OPENIDPROVIDER
    User Ben(https://login.launchpad.net/+id/AAAA) change name to Ben1
    User Cees(https://login.launchpad.net/+id/ABCD) change name to Ben
  OPENIDCONSUMER
    Ben (previous Cees | https://login.launchpad.net/+id/ABCD) logs in =>
       user 'Ben1' (https://login.launchpad.net/+id/AAAA) should be renamed to 'Ben2'
       user 'Cees' should be renamed to 'Ben1'
    Ben2 (https://login.launchpad.net/+id/AAAA) logs in with username 'Ben1'
       user 'Ben1' (previous Cees | https://login.launchpad.net/+id/ABCD) should be renamed to 'Ben3'
       user 'Ben2' should be renamed to 'Ben1'

All next logins for Ben3 (Cees/Ben (https://login.launchpad.net/+id/ABCD)) should check if 'Ben' is available. If not, keep the old username

[lp:~mhall119/django-openid-auth/fixes-642132]

80. By Michael Hall on 2011-03-20

Merge from trunk

81. By Michael Hall on 2011-03-21

Extra renaming considerations and tests for conflicts and false positives

82. By Michael Hall on 2011-03-21

Add another test to make sure renaming doesn't get confused by not-quite nickna me+1 usernames

83. By Michael Hall on 2011-03-21

Finish new test and altered comments

84. By Michael Hall on 2011-03-23

Add new option to README.txt and removed an unecessary check in _get_available_username()

85. By Michael Hall on 2011-04-19

Updates from trunk

86. By Michael Hall on 2011-05-12

Check for STRICT_USERNAMES before defaulting the nickname to openiduser

87. By Michael Hall on 2011-05-12

Make sure auto-mapping is turned off when testing teams->group

Unmerged revisions

=== 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-21 11:58:29 +0000
@@ -81,7 +81,7 @@
 
         if getattr(settings, 'OPENID_UPDATE_DETAILS_FROM_SREG', False):
             details = self._extract_user_details(openid_response)
-            self.update_user_details(user, details)
+            self.update_user_details(user, details, openid_response)
 
         teams_response = teams.TeamsResponse.fromSuccessResponse(
             openid_response)
@@ -98,7 +98,6 @@
             email = sreg_response.get('email')
             fullname = sreg_response.get('fullname')
             nickname = sreg_response.get('nickname')
-
         # If any attributes are provided via Attribute Exchange, use
         # them in preference.
         fetch_response = ax.FetchResponse.fromSuccessResponse(openid_response)
@@ -137,10 +136,36 @@
         return dict(email=email, nickname=nickname,
                     first_name=first_name, last_name=last_name)
 
-    def create_user_from_openid(self, openid_response):
-        details = self._extract_user_details(openid_response)
-        nickname = details['nickname'] or 'openiduser'
-        email = details['email'] or ''
+    def _get_available_username(self, nickname, identity_url):
+        nickname = nickname or 'openiduser'
+        # See if we already have this nickname assigned to a username
+        try:
+            user = User.objects.get(username__exact=nickname)
+        except User.DoesNotExist:
+            # No conflict, we can use this nickname
+            return nickname
+            
+        # Check if we already have nickname+i for this identity_url
+        try:
+            user_openid = UserOpenID.objects.get(
+                claimed_id__exact=identity_url,
+                user__username__startswith=nickname)
+            # No exception means we have an existing user for this identity
+            # that starts with this nickname, so it's possible we've had to
+            # assign them to nickname+i already.
+            oid_username = user_openid.user.username
+            if len(oid_username) > len(nickname):
+                try:
+                    # check that it ends with a number
+                    int(oid_username[len(nickname):])
+                    return oid_username
+                except ValueError:
+                    # username starts with nickname, but isn't nickname+#
+                    pass
+        except UserOpenID.DoesNotExist:
+            # No user associated with this identity_url
+            pass
+            
 
         # Pick a username for the user based on their nickname,
         # checking for conflicts.
@@ -150,15 +175,27 @@
             if i > 1:
                 username += str(i)
             try:
-                User.objects.get(username__exact=username)
+                user = User.objects.get(username__exact=username)
+                if user.useropenid_set.filter(claimed_id__exact=identity_url).count() > 0:
+                    # username already belongs to this openid user, so it's okay
+                    return username
+                    
             except User.DoesNotExist:
                 break
             i += 1
+        return username
+    
+    def create_user_from_openid(self, openid_response):
+        details = self._extract_user_details(openid_response)
+        nickname = details['nickname'] or 'openiduser'
+        email = details['email'] or ''
+
+        username = self._get_available_username(details['nickname'], openid_response.identity_url)
 
         user = User.objects.create_user(username, email, password=None)
-        self.update_user_details(user, details)
-
         self.associate_openid(user, openid_response)
+        self.update_user_details(user, details, openid_response)
+
         return user
 
     def associate_openid(self, user, openid_response):
@@ -181,7 +218,7 @@
 
         return user_openid
 
-    def update_user_details(self, user, details):
+    def update_user_details(self, user, details, openid_response):
         updated = False
         if details['first_name']:
             user.first_name = details['first_name']
@@ -192,6 +229,9 @@
         if details['email']:
             user.email = details['email']
             updated = True
+        if getattr(settings, 'OPENID_FOLLOW_RENAMES', False):
+            user.username = self._get_available_username(details['nickname'], openid_response.identity_url)
+            updated = True
 
         if updated:
             user.save()

=== 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-21 11:58:29 +0000
@@ -135,12 +135,14 @@
         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)
+        self.old_follow_renames = getattr(settings, 'OPENID_FOLLOW_RENAMES', False)
 
         settings.OPENID_CREATE_USERS = False
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = False
         settings.OPENID_SSO_SERVER_URL = None
         settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = {}
         settings.OPENID_USE_AS_ADMIN_LOGIN = False
+        settings.OPENID_FOLLOW_RENAMES = False
 
     def tearDown(self):
         settings.LOGIN_REDIRECT_URL = self.old_login_redirect_url
@@ -149,6 +151,7 @@
         settings.OPENID_SSO_SERVER_URL = self.old_sso_server_url
         settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = self.old_teams_map
         settings.OPENID_USE_AS_ADMIN_LOGIN = self.old_use_as_admin_login
+        settings.OPENID_FOLLOW_RENAMES = self.old_follow_renames
 
         setDefaultFetcher(None)
         super(RelyingPartyTests, self).tearDown()
@@ -286,6 +289,213 @@
         self.assertEquals(user.last_name, 'User')
         self.assertEquals(user.email, '[e-mail]')
 
+    def _do_user_login(self, openid_req, openid_resp):
+        # Posting in an identity URL begins the authentication request:
+        response = self.client.post('/openid/login/', openid_req)
+        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, openid_resp)
+        openid_response.addExtension(sreg_response)
+        response = self.complete(openid_response)
+        self.assertRedirects(response, 'http://testserver/getuser/')
+
+    def test_login_without_nickname(self):
+        settings.OPENID_CREATE_USERS = True
+
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        openid_resp =  {'nickname': '', 'fullname': 'Openid User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # username defaults to 'openiduser'
+        self.assertEquals(response.content, 'openiduser')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Openid')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
+    def test_login_follow_rename(self):
+        settings.OPENID_FOLLOW_RENAMES = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        user = User.objects.create_user('testuser', '[e-mail]')
+        useropenid = UserOpenID(
+            user=user,
+            claimed_id='http://example.com/identity',
+            display_id='http://example.com/identity')
+        useropenid.save()
+
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        openid_resp =  {'nickname': 'someuser', 'fullname': 'Some User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # If OPENID_FOLLOW_RENAMES, they are logged in as 
+        # someuser (the passed in nickname has changed the username)
+        self.assertEquals(response.content, 'someuser')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Some')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
+    def test_login_follow_rename_conflict(self):
+        settings.OPENID_FOLLOW_RENAMES = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to switch to
+        user = User.objects.create_user('testuser', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=user,
+            claimed_id='http://example.com/existing_identity',
+            display_id='http://example.com/existing_identity')
+
+        # Setup user who is going to try to change username to 'testuser'
+        renamed_user = User.objects.create_user('renameuser', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=renamed_user,
+            claimed_id='http://example.com/identity',
+            display_id='http://example.com/identity')
+
+        # 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': 'Rename User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
+        # but since that 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')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Rename')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
+    def test_login_follow_rename_false_onlyonce(self):
+        settings.OPENID_FOLLOW_RENAMES = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to switch to
+        user = User.objects.create_user('testuser', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=user,
+            claimed_id='http://example.com/existing_identity',
+            display_id='http://example.com/existing_identity')
+
+        # Setup user who is going to try to change username to 'testuser'
+        renamed_user = User.objects.create_user('testuser2000eight', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=renamed_user,
+            claimed_id='http://example.com/identity',
+            display_id='http://example.com/identity')
+
+        # identity url is for 'testuser2000eight'
+        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': 'testuser2', 'fullname': 'Rename User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
+        # but since that username is already taken by someone else, we go through
+        # the process of adding +i to it.  Even though it looks like the username
+        # follows the nickname+i scheme, it has non-numbers in the suffix, so 
+        # it's not an auto-generated one.  The regular process of renaming to 
+        # 'testuser' has a conflict, so we get +2 at the end.
+        self.assertEquals(response.content, 'testuser2')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Rename')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
+    def test_login_follow_rename_conflict_onlyonce(self):
+        settings.OPENID_FOLLOW_RENAMES = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's name we're going to switch to
+        user = User.objects.create_user('testuser', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=user,
+            claimed_id='http://example.com/existing_identity',
+            display_id='http://example.com/existing_identity')
+
+        # Setup user who is going to try to change username to 'testuser'
+        renamed_user = User.objects.create_user('testuser2000', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=renamed_user,
+            claimed_id='http://example.com/identity',
+            display_id='http://example.com/identity')
+
+        # identity url is for 'testuser2000'
+        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': 'Rename User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
+        # but since that username is already taken by someone else, we go through
+        # the process of adding +i to it.  Since the user for this identity url
+        # already has a name matching that pattern, check if first.
+        self.assertEquals(response.content, 'testuser2000')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Rename')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
+    def test_login_follow_rename_false_conflict(self):
+        settings.OPENID_FOLLOW_RENAMES = True
+        settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
+        # Setup existing user who's username matches the name+i pattern
+        user = User.objects.create_user('testuser2', '[e-mail]')
+        UserOpenID.objects.get_or_create(
+            user=user,
+            claimed_id='http://example.com/identity',
+            display_id='http://example.com/identity')
+
+        # identity url is for 'testuser2'
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        # but returned username is for 'testuser', which looks like we've done
+        # a username+1 for them already, but 'testuser' isn't actually taken
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Same User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
+        response = self.client.get('/getuser/')
+
+        # If OPENID_FOLLOW_RENAMES, username should be changed to 'testuser'
+        # because it wasn't currently taken
+        self.assertEquals(response.content, 'testuser')
+
+        # The user's full name and email have been updated.
+        user = User.objects.get(username=response.content)
+        self.assertEquals(user.first_name, 'Same')
+        self.assertEquals(user.last_name, 'User')
+        self.assertEquals(user.email, '[e-mail]')
+        
     def test_login_update_details(self):
         settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
         user = User.objects.create_user('testuser', '[e-mail]')
@@ -295,31 +505,17 @@
             display_id='http://example.com/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)
-        self.assertRedirects(response, 'http://testserver/getuser/')
-
-        # And they are now logged in as testuser (the passed in
-        # nickname has not caused the username to change).
+        openid_req = {'openid_identifier': 'http://example.com/identity',
+               'next': '/getuser/'}
+        openid_resp =  {'nickname': 'testuser', 'fullname': 'Some User',
+                 'email': '[e-mail]'}
+        self._do_user_login(openid_req, openid_resp)
         response = self.client.get('/getuser/')
+
         self.assertEquals(response.content, 'testuser')
 
         # The user's full name and email have been updated.
-        user = User.objects.get(username='testuser')
+        user = User.objects.get(username=response.content)
         self.assertEquals(user.first_name, 'Some')
         self.assertEquals(user.last_name, 'User')
         self.assertEquals(user.email, '[e-mail]')