OpenID integration for django.contrib.auth

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

Proposed by Michael Hall on 2010-10-13
Status: Superseded
Proposed branch: lp:~mhall119/django-openid-auth/fixes-642132
Merge into: lp:django-openid-auth
Diff against target: 391 lines (+267/-31) 2 files modified
To merge this branch: bzr merge lp:~mhall119/django-openid-auth/fixes-642132
Reviewer Review Type Date Requested Status
James Henstridge 2010-10-13 Needs Fixing on 2010-12-22
Review via email: mp+38335@code.launchpad.net

This proposal has been superseded by a proposal from 2011-03-21.

Description of the Change

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.

To post a comment you must log in.
Michael Hall (mhall119) wrote :

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

Anthony Lenton (elachuni) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Ronnie (ronnie.vd.c) wrote :

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) wrote :

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

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

Preview Diff

1=== modified file 'django_openid_auth/auth.py'
2--- django_openid_auth/auth.py 2010-10-15 09:08:10 +0000
3+++ django_openid_auth/auth.py 2011-03-21 11:58:29 +0000
4@@ -81,7 +81,7 @@
5
6 if getattr(settings, 'OPENID_UPDATE_DETAILS_FROM_SREG', False):
7 details = self._extract_user_details(openid_response)
8- self.update_user_details(user, details)
9+ self.update_user_details(user, details, openid_response)
10
11 teams_response = teams.TeamsResponse.fromSuccessResponse(
12 openid_response)
13@@ -98,7 +98,6 @@
14 email = sreg_response.get('email')
15 fullname = sreg_response.get('fullname')
16 nickname = sreg_response.get('nickname')
17-
18 # If any attributes are provided via Attribute Exchange, use
19 # them in preference.
20 fetch_response = ax.FetchResponse.fromSuccessResponse(openid_response)
21@@ -137,10 +136,36 @@
22 return dict(email=email, nickname=nickname,
23 first_name=first_name, last_name=last_name)
24
25- def create_user_from_openid(self, openid_response):
26- details = self._extract_user_details(openid_response)
27- nickname = details['nickname'] or 'openiduser'
28- email = details['email'] or ''
29+ def _get_available_username(self, nickname, identity_url):
30+ nickname = nickname or 'openiduser'
31+ # See if we already have this nickname assigned to a username
32+ try:
33+ user = User.objects.get(username__exact=nickname)
34+ except User.DoesNotExist:
35+ # No conflict, we can use this nickname
36+ return nickname
37+
38+ # Check if we already have nickname+i for this identity_url
39+ try:
40+ user_openid = UserOpenID.objects.get(
41+ claimed_id__exact=identity_url,
42+ user__username__startswith=nickname)
43+ # No exception means we have an existing user for this identity
44+ # that starts with this nickname, so it's possible we've had to
45+ # assign them to nickname+i already.
46+ oid_username = user_openid.user.username
47+ if len(oid_username) > len(nickname):
48+ try:
49+ # check that it ends with a number
50+ int(oid_username[len(nickname):])
51+ return oid_username
52+ except ValueError:
53+ # username starts with nickname, but isn't nickname+#
54+ pass
55+ except UserOpenID.DoesNotExist:
56+ # No user associated with this identity_url
57+ pass
58+
59
60 # Pick a username for the user based on their nickname,
61 # checking for conflicts.
62@@ -150,15 +175,27 @@
63 if i > 1:
64 username += str(i)
65 try:
66- User.objects.get(username__exact=username)
67+ user = User.objects.get(username__exact=username)
68+ if user.useropenid_set.filter(claimed_id__exact=identity_url).count() > 0:
69+ # username already belongs to this openid user, so it's okay
70+ return username
71+
72 except User.DoesNotExist:
73 break
74 i += 1
75+ return username
76+
77+ def create_user_from_openid(self, openid_response):
78+ details = self._extract_user_details(openid_response)
79+ nickname = details['nickname'] or 'openiduser'
80+ email = details['email'] or ''
81+
82+ username = self._get_available_username(details['nickname'], openid_response.identity_url)
83
84 user = User.objects.create_user(username, email, password=None)
85- self.update_user_details(user, details)
86-
87 self.associate_openid(user, openid_response)
88+ self.update_user_details(user, details, openid_response)
89+
90 return user
91
92 def associate_openid(self, user, openid_response):
93@@ -181,7 +218,7 @@
94
95 return user_openid
96
97- def update_user_details(self, user, details):
98+ def update_user_details(self, user, details, openid_response):
99 updated = False
100 if details['first_name']:
101 user.first_name = details['first_name']
102@@ -192,6 +229,9 @@
103 if details['email']:
104 user.email = details['email']
105 updated = True
106+ if getattr(settings, 'OPENID_FOLLOW_RENAMES', False):
107+ user.username = self._get_available_username(details['nickname'], openid_response.identity_url)
108+ updated = True
109
110 if updated:
111 user.save()
112
113=== modified file 'django_openid_auth/tests/test_views.py'
114--- django_openid_auth/tests/test_views.py 2011-01-13 08:38:01 +0000
115+++ django_openid_auth/tests/test_views.py 2011-03-21 11:58:29 +0000
116@@ -135,12 +135,14 @@
117 self.old_sso_server_url = getattr(settings, 'OPENID_SSO_SERVER_URL', None)
118 self.old_teams_map = getattr(settings, 'OPENID_LAUNCHPAD_TEAMS_MAPPING', {})
119 self.old_use_as_admin_login = getattr(settings, 'OPENID_USE_AS_ADMIN_LOGIN', False)
120+ self.old_follow_renames = getattr(settings, 'OPENID_FOLLOW_RENAMES', False)
121
122 settings.OPENID_CREATE_USERS = False
123 settings.OPENID_UPDATE_DETAILS_FROM_SREG = False
124 settings.OPENID_SSO_SERVER_URL = None
125 settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = {}
126 settings.OPENID_USE_AS_ADMIN_LOGIN = False
127+ settings.OPENID_FOLLOW_RENAMES = False
128
129 def tearDown(self):
130 settings.LOGIN_REDIRECT_URL = self.old_login_redirect_url
131@@ -149,6 +151,7 @@
132 settings.OPENID_SSO_SERVER_URL = self.old_sso_server_url
133 settings.OPENID_LAUNCHPAD_TEAMS_MAPPING = self.old_teams_map
134 settings.OPENID_USE_AS_ADMIN_LOGIN = self.old_use_as_admin_login
135+ settings.OPENID_FOLLOW_RENAMES = self.old_follow_renames
136
137 setDefaultFetcher(None)
138 super(RelyingPartyTests, self).tearDown()
139@@ -286,6 +289,213 @@
140 self.assertEquals(user.last_name, 'User')
141 self.assertEquals(user.email, 'foo@example.com')
142
143+ def _do_user_login(self, openid_req, openid_resp):
144+ # Posting in an identity URL begins the authentication request:
145+ response = self.client.post('/openid/login/', openid_req)
146+ self.assertContains(response, 'OpenID transaction in progress')
147+
148+ # Complete the request, passing back some simple registration
149+ # data. The user is redirected to the next URL.
150+ openid_request = self.provider.parseFormPost(response.content)
151+ sreg_request = sreg.SRegRequest.fromOpenIDRequest(openid_request)
152+ openid_response = openid_request.answer(True)
153+ sreg_response = sreg.SRegResponse.extractResponse(
154+ sreg_request, openid_resp)
155+ openid_response.addExtension(sreg_response)
156+ response = self.complete(openid_response)
157+ self.assertRedirects(response, 'http://testserver/getuser/')
158+
159+ def test_login_without_nickname(self):
160+ settings.OPENID_CREATE_USERS = True
161+
162+ openid_req = {'openid_identifier': 'http://example.com/identity',
163+ 'next': '/getuser/'}
164+ openid_resp = {'nickname': '', 'fullname': 'Openid User',
165+ 'email': 'foo@example.com'}
166+ self._do_user_login(openid_req, openid_resp)
167+ response = self.client.get('/getuser/')
168+
169+ # username defaults to 'openiduser'
170+ self.assertEquals(response.content, 'openiduser')
171+
172+ # The user's full name and email have been updated.
173+ user = User.objects.get(username=response.content)
174+ self.assertEquals(user.first_name, 'Openid')
175+ self.assertEquals(user.last_name, 'User')
176+ self.assertEquals(user.email, 'foo@example.com')
177+
178+ def test_login_follow_rename(self):
179+ settings.OPENID_FOLLOW_RENAMES = True
180+ settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
181+ user = User.objects.create_user('testuser', 'someone@example.com')
182+ useropenid = UserOpenID(
183+ user=user,
184+ claimed_id='http://example.com/identity',
185+ display_id='http://example.com/identity')
186+ useropenid.save()
187+
188+ openid_req = {'openid_identifier': 'http://example.com/identity',
189+ 'next': '/getuser/'}
190+ openid_resp = {'nickname': 'someuser', 'fullname': 'Some User',
191+ 'email': 'foo@example.com'}
192+ self._do_user_login(openid_req, openid_resp)
193+ response = self.client.get('/getuser/')
194+
195+ # If OPENID_FOLLOW_RENAMES, they are logged in as
196+ # someuser (the passed in nickname has changed the username)
197+ self.assertEquals(response.content, 'someuser')
198+
199+ # The user's full name and email have been updated.
200+ user = User.objects.get(username=response.content)
201+ self.assertEquals(user.first_name, 'Some')
202+ self.assertEquals(user.last_name, 'User')
203+ self.assertEquals(user.email, 'foo@example.com')
204+
205+ def test_login_follow_rename_conflict(self):
206+ settings.OPENID_FOLLOW_RENAMES = True
207+ settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
208+ # Setup existing user who's name we're going to switch to
209+ user = User.objects.create_user('testuser', 'someone@example.com')
210+ UserOpenID.objects.get_or_create(
211+ user=user,
212+ claimed_id='http://example.com/existing_identity',
213+ display_id='http://example.com/existing_identity')
214+
215+ # Setup user who is going to try to change username to 'testuser'
216+ renamed_user = User.objects.create_user('renameuser', 'someone@example.com')
217+ UserOpenID.objects.get_or_create(
218+ user=renamed_user,
219+ claimed_id='http://example.com/identity',
220+ display_id='http://example.com/identity')
221+
222+ # identity url is for 'renameuser'
223+ openid_req = {'openid_identifier': 'http://example.com/identity',
224+ 'next': '/getuser/'}
225+ # but returned username is for 'testuser', which already exists for another identity
226+ openid_resp = {'nickname': 'testuser', 'fullname': 'Rename User',
227+ 'email': 'rename@example.com'}
228+ self._do_user_login(openid_req, openid_resp)
229+ response = self.client.get('/getuser/')
230+
231+ # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
232+ # but since that username is already taken by someone else, we go through
233+ # the process of adding +i to it, and get testuser2.
234+ self.assertEquals(response.content, 'testuser2')
235+
236+ # The user's full name and email have been updated.
237+ user = User.objects.get(username=response.content)
238+ self.assertEquals(user.first_name, 'Rename')
239+ self.assertEquals(user.last_name, 'User')
240+ self.assertEquals(user.email, 'rename@example.com')
241+
242+ def test_login_follow_rename_false_onlyonce(self):
243+ settings.OPENID_FOLLOW_RENAMES = True
244+ settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
245+ # Setup existing user who's name we're going to switch to
246+ user = User.objects.create_user('testuser', 'someone@example.com')
247+ UserOpenID.objects.get_or_create(
248+ user=user,
249+ claimed_id='http://example.com/existing_identity',
250+ display_id='http://example.com/existing_identity')
251+
252+ # Setup user who is going to try to change username to 'testuser'
253+ renamed_user = User.objects.create_user('testuser2000eight', 'someone@example.com')
254+ UserOpenID.objects.get_or_create(
255+ user=renamed_user,
256+ claimed_id='http://example.com/identity',
257+ display_id='http://example.com/identity')
258+
259+ # identity url is for 'testuser2000eight'
260+ openid_req = {'openid_identifier': 'http://example.com/identity',
261+ 'next': '/getuser/'}
262+ # but returned username is for 'testuser', which already exists for another identity
263+ openid_resp = {'nickname': 'testuser2', 'fullname': 'Rename User',
264+ 'email': 'rename@example.com'}
265+ self._do_user_login(openid_req, openid_resp)
266+ response = self.client.get('/getuser/')
267+
268+ # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
269+ # but since that username is already taken by someone else, we go through
270+ # the process of adding +i to it. Even though it looks like the username
271+ # follows the nickname+i scheme, it has non-numbers in the suffix, so
272+ # it's not an auto-generated one. The regular process of renaming to
273+ # 'testuser' has a conflict, so we get +2 at the end.
274+ self.assertEquals(response.content, 'testuser2')
275+
276+ # The user's full name and email have been updated.
277+ user = User.objects.get(username=response.content)
278+ self.assertEquals(user.first_name, 'Rename')
279+ self.assertEquals(user.last_name, 'User')
280+ self.assertEquals(user.email, 'rename@example.com')
281+
282+ def test_login_follow_rename_conflict_onlyonce(self):
283+ settings.OPENID_FOLLOW_RENAMES = True
284+ settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
285+ # Setup existing user who's name we're going to switch to
286+ user = User.objects.create_user('testuser', 'someone@example.com')
287+ UserOpenID.objects.get_or_create(
288+ user=user,
289+ claimed_id='http://example.com/existing_identity',
290+ display_id='http://example.com/existing_identity')
291+
292+ # Setup user who is going to try to change username to 'testuser'
293+ renamed_user = User.objects.create_user('testuser2000', 'someone@example.com')
294+ UserOpenID.objects.get_or_create(
295+ user=renamed_user,
296+ claimed_id='http://example.com/identity',
297+ display_id='http://example.com/identity')
298+
299+ # identity url is for 'testuser2000'
300+ openid_req = {'openid_identifier': 'http://example.com/identity',
301+ 'next': '/getuser/'}
302+ # but returned username is for 'testuser', which already exists for another identity
303+ openid_resp = {'nickname': 'testuser', 'fullname': 'Rename User',
304+ 'email': 'rename@example.com'}
305+ self._do_user_login(openid_req, openid_resp)
306+ response = self.client.get('/getuser/')
307+
308+ # If OPENID_FOLLOW_RENAMES, attempt to change username to 'testuser'
309+ # but since that username is already taken by someone else, we go through
310+ # the process of adding +i to it. Since the user for this identity url
311+ # already has a name matching that pattern, check if first.
312+ self.assertEquals(response.content, 'testuser2000')
313+
314+ # The user's full name and email have been updated.
315+ user = User.objects.get(username=response.content)
316+ self.assertEquals(user.first_name, 'Rename')
317+ self.assertEquals(user.last_name, 'User')
318+ self.assertEquals(user.email, 'rename@example.com')
319+
320+ def test_login_follow_rename_false_conflict(self):
321+ settings.OPENID_FOLLOW_RENAMES = True
322+ settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
323+ # Setup existing user who's username matches the name+i pattern
324+ user = User.objects.create_user('testuser2', 'someone@example.com')
325+ UserOpenID.objects.get_or_create(
326+ user=user,
327+ claimed_id='http://example.com/identity',
328+ display_id='http://example.com/identity')
329+
330+ # identity url is for 'testuser2'
331+ openid_req = {'openid_identifier': 'http://example.com/identity',
332+ 'next': '/getuser/'}
333+ # but returned username is for 'testuser', which looks like we've done
334+ # a username+1 for them already, but 'testuser' isn't actually taken
335+ openid_resp = {'nickname': 'testuser', 'fullname': 'Same User',
336+ 'email': 'same@example.com'}
337+ self._do_user_login(openid_req, openid_resp)
338+ response = self.client.get('/getuser/')
339+
340+ # If OPENID_FOLLOW_RENAMES, username should be changed to 'testuser'
341+ # because it wasn't currently taken
342+ self.assertEquals(response.content, 'testuser')
343+
344+ # The user's full name and email have been updated.
345+ user = User.objects.get(username=response.content)
346+ self.assertEquals(user.first_name, 'Same')
347+ self.assertEquals(user.last_name, 'User')
348+ self.assertEquals(user.email, 'same@example.com')
349+
350 def test_login_update_details(self):
351 settings.OPENID_UPDATE_DETAILS_FROM_SREG = True
352 user = User.objects.create_user('testuser', 'someone@example.com')
353@@ -295,31 +505,17 @@
354 display_id='http://example.com/identity')
355 useropenid.save()
356
357- # Posting in an identity URL begins the authentication request:
358- response = self.client.post('/openid/login/',
359- {'openid_identifier': 'http://example.com/identity',
360- 'next': '/getuser/'})
361- self.assertContains(response, 'OpenID transaction in progress')
362-
363- # Complete the request, passing back some simple registration
364- # data. The user is redirected to the next URL.
365- openid_request = self.provider.parseFormPost(response.content)
366- sreg_request = sreg.SRegRequest.fromOpenIDRequest(openid_request)
367- openid_response = openid_request.answer(True)
368- sreg_response = sreg.SRegResponse.extractResponse(
369- sreg_request, {'nickname': 'someuser', 'fullname': 'Some User',
370- 'email': 'foo@example.com'})
371- openid_response.addExtension(sreg_response)
372- response = self.complete(openid_response)
373- self.assertRedirects(response, 'http://testserver/getuser/')
374-
375- # And they are now logged in as testuser (the passed in
376- # nickname has not caused the username to change).
377+ openid_req = {'openid_identifier': 'http://example.com/identity',
378+ 'next': '/getuser/'}
379+ openid_resp = {'nickname': 'testuser', 'fullname': 'Some User',
380+ 'email': 'foo@example.com'}
381+ self._do_user_login(openid_req, openid_resp)
382 response = self.client.get('/getuser/')
383+
384 self.assertEquals(response.content, 'testuser')
385
386 # The user's full name and email have been updated.
387- user = User.objects.get(username='testuser')
388+ user = User.objects.get(username=response.content)
389 self.assertEquals(user.first_name, 'Some')
390 self.assertEquals(user.last_name, 'User')
391 self.assertEquals(user.email, 'foo@example.com')

Subscribers

People subscribed via source and target branches