Merge lp:~jamesh/django-openid-auth/sanitise-url into lp:~django-openid-auth/django-openid-auth/trunk

Proposed by James Henstridge on 2010-01-25
Status: Merged
Approved by: James Henstridge on 2010-01-28
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jamesh/django-openid-auth/sanitise-url
Merge into: lp:~django-openid-auth/django-openid-auth/trunk
Diff against target: 189 lines (+64/-30)
5 files modified
django_openid_auth/__init__.py (+0/-23)
django_openid_auth/admin.py (+24/-0)
django_openid_auth/forms.py (+0/-1)
django_openid_auth/tests/test_views.py (+30/-0)
django_openid_auth/views.py (+10/-6)
To merge this branch: bzr merge lp:~jamesh/django-openid-auth/sanitise-url
Reviewer Review Type Date Requested Status
Martin Albisetti (community) Approve on 2010-01-25
John O'Brien (community) 2010-01-25 Approve on 2010-01-25
Review via email: mp+17997@code.launchpad.net

Commit message

Make sanitise_redirect_url map the empty string to LOGIN_REDIRECT_URL. Fixes bug #510866.

To post a comment you must log in.
James Henstridge (jamesh) wrote :

Fix the sanitise_redirect_url() function to convert the empty string into settings.LOGIN_REDIRECT_URL again. Without this, the login_complete() view redirects back to itself if no "next" parameter was given.

John O'Brien (jdobrien) wrote :

I would like to approve this, however I can't test it as there are a lot of unrelated errors when testing it with our code.

John O'Brien (jdobrien) wrote :

This got it working and users experiencing the error no longer get it with this branch. Thank you.

Thanks for fixing the bad import of forms.py too.

review: Approve
Dave Walker (davewalker) wrote :

I can confirm at revision 67, it resolves the issue experienced on bug #10866 on an unrelated project to UbuntuOne.

Martin Albisetti (beuno) wrote :

This fixes the problem in my local instance.

review: Approve
Elliot Murphy (statik) wrote :
Download full text (24.6 KiB)

The attempt to merge lp:~jamesh/django-openid-auth/sanitise-url into lp:django-openid-auth failed.Below is the output from the failed tests.

PYTHONPATH=/tmp/django-openid-auth python example_consumer/manage.py test \
    --verbosity=2 django_openid_auth
Creating test database...
Processing auth.Permission model
Creating table auth_permission
Processing auth.Group model
Creating table auth_group
Processing auth.User model
Creating table auth_user
Processing auth.Message model
Creating table auth_message
Processing contenttypes.ContentType model
Creating table django_content_type
Processing sessions.Session model
Creating table django_session
Processing admin.LogEntry model
Creating table django_admin_log
Processing django_openid_auth.Nonce model
Creating table django_openid_auth_nonce
Processing django_openid_auth.Association model
Creating table django_openid_auth_association
Processing django_openid_auth.UserOpenID model
Creating table django_openid_auth_useropenid
Creating many-to-many tables for auth.Group model
Creating many-to-many tables for auth.User model
Running post-sync handlers for application auth
Adding permission 'auth | permission | Can add permission'
Adding permission 'auth | permission | Can change permission'
Adding permission 'auth | permission | Can delete permission'
Adding permission 'auth | group | Can add group'
Adding permission 'auth | group | Can change group'
Adding permission 'auth | group | Can delete group'
Adding permission 'auth | user | Can add user'
Adding permission 'auth | user | Can change user'
Adding permission 'auth | user | Can delete user'
Adding permission 'auth | message | Can add message'
Adding permission 'auth | message | Can change message'
Adding permission 'auth | message | Can delete message'
Running post-sync handlers for application contenttypes
Adding permission 'contenttypes | content type | Can add content type'
Adding permission 'contenttypes | content type | Can change content type'
Adding permission 'contenttypes | content type | Can delete content type'
Running post-sync handlers for application sessions
Adding permission 'sessions | session | Can add session'
Adding permission 'sessions | session | Can change session'
Adding permission 'sessions | session | Can delete session'
Running post-sync handlers for application admin
Adding permission 'admin | log entry | Can add log entry'
Adding permission 'admin | log entry | Can change log entry'
Adding permission 'admin | log entry | Can delete log entry'
Running post-sync handlers for application django_openid_auth
Adding permission 'django_openid_auth | nonce | Can add nonce'
Adding permission 'django_openid_auth | nonce | Can change nonce'
Adding permission 'django_openid_auth | nonce | Can delete nonce'
Adding permission 'django_openid_auth | association | Can add association'
Adding permission 'django_openid_auth | association | Can change association'
Adding permission 'django_openid_auth | association | Can delete association'
Adding permission 'django_openid_auth | user open id | Can add user open id'
Adding permission 'django_openid_auth | user open id | Can change user open id'
Adding permission 'django_openid_auth | user ...

Nicola Larosa (teknico) wrote :

Using this, I'm getting the same behavior as with rev.36 of django-openid-auth, that is, the "hola" user gets logged in, but has no subscription, and only sees the "Account" tab.

Elliot Murphy (statik) wrote :

make check passed on my jaunty machine. it fails on my lucid machine. I've tried downgrading python-django to 1.0.2-1ubuntu0.1, and I still get the failures on my lucid machine.

Martin Albisetti (beuno) wrote :

FWIW, all test seem to pass in karmic

Dave Walker (davewalker) wrote :

It seems to pass on Lucid for me:
http://pastebin.com/f55390dc2

dave@micmini:~/tmp/sanitise-url$ django-admin --version
1.1.1

Elliot Murphy (statik) wrote :

thanks for trying that out dave! i am at a loss as to why it's continuing to fail on my box. i'm firing up a testdrive-select-iso instance of lucid right now to test it there and see if i can figure anything out.

Dave Walker (davewalker) wrote :

My lucid box was around a week out of date, i've just upgraded and run the tests again.. and it did indeed fail. This indicates that something has recently changed, causing these issues outside of django-openid-auth.

make check log:
http://pastebin.com/f3179579a

Elliot Murphy (statik) wrote :

I tried this again tonight with a brand new pristine install of lucid and got the same 5 tests failing. I wonder what has changed in lucid to cause the currently logged in user to be ''.

Dave Walker (davewalker) wrote :

It's interesting that python-openid in Lucid was last sync'd 5 days ago. Whilst it seems to be the identical version as Karmic, and the sole difference being promotion to main, it does interest me why the unittests seemed to work on Lucid(-week of update), and fails now.

Dave Walker (davewalker) wrote :

Running the tests on Lucid under python2.6 fail. Running under python2.5 passes.

I ran this testing doing:

=== modified file 'Makefile'
 check:
- PYTHONPATH=$(shell pwd) python example_consumer/manage.py test \
+ PYTHONPATH=$(shell pwd): python2.5 example_consumer/manage.py test \

& to bring deps into path:

~/tmp/sanitise-url$ ln -s /usr/lib/pymodules/python2.6/django .
~/tmp/sanitise-url$ ln -s /usr/lib/pymodules/python2.6/openid .

Python2.6 was also lasted updated 5 days ago, with a hefty changelog.

James Henstridge (jamesh) wrote :

I'm upgrading my laptop to Lucid to try and reproduce the bug.

As far as the bug goes, the view that is being rendered by the test prints response.user.username. If we aren't authenticated, then request.user will be an AnonymousUser instance, whose username attribute is the empty string. If this is a real bug rather than a quirk of the test environment, then it is a serious bug.

That said, it is a bug that is orthogonal to this merge proposal. Given that it does pass on Karmic, I think merging should be fine.

James Henstridge (jamesh) wrote :

This appears to be the change in the python2.6 package causing the failures:

+--- Lib/Cookie.py (.../tags/r264) (Revision 77683)
++++ Lib/Cookie.py (.../branches/release26-maint) (Revision 77683)
+@@ -624,7 +624,9 @@
+ if type(rawdata) == type(""):
+ self.__ParseString(rawdata)
+ else:
+- self.update(rawdata)
++ # self.update() wouldn't call our custom __setitem__
++ for k, v in rawdata.items():
++ self[k] = v
+ return
+ # end load()
+

Putting a copy of Cookie.py with that change backed out on the python path makes the tests pass. The code appears to function correctly with the standard Cookie.py, so I wonder if the bug is inside the Django test infrastructure?

James Henstridge (jamesh) wrote :

I've summarised the problem here:

http://code.djangoproject.com/ticket/12720

As it is definitely a Django bug (or a Python bug if the Django developers can convince the Python developers ...), I'm going to merge this.

Elliot Murphy (statik) wrote :

Thanks for getting this fixed upstream so fast! Also, thanks for filing https://bugs.edge.launchpad.net/django/+bug/513719 to track the fix going into Lucid.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'django_openid_auth/__init__.py'
2--- django_openid_auth/__init__.py 2010-01-21 21:50:13 +0000
3+++ django_openid_auth/__init__.py 2010-01-25 14:43:11 +0000
4@@ -2,7 +2,6 @@
5 #
6 # Copyright (C) 2007 Simon Willison
7 # Copyright (C) 2008-2009 Canonical Ltd.
8-# Copyright (C) 2010 Dave Walker
9 #
10 # Redistribution and use in source and binary forms, with or without
11 # modification, are permitted provided that the following conditions
12@@ -28,25 +27,3 @@
13 # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
14 # POSSIBILITY OF SUCH DAMAGE.
15
16-""" Support for allowing openid authentication for /admin (django.contrib.admin) """
17-
18-from django.conf import settings
19-
20-if getattr(settings, 'OPENID_USE_AS_ADMIN_LOGIN', False):
21- from django.http import HttpResponseRedirect
22- from django.contrib.admin import sites
23- from django_openid_auth import views
24-
25- def _openid_login(self, request, error_message='', extra_context=None):
26- if request.user.is_authenticated():
27- if not request.user.is_staff:
28- return views.render_failure(request, "User %s does not have admin access."
29- % request.user.username)
30- return views.render_failure(request, "Unknown Error: %s" % error_message)
31- else:
32- # Redirect to openid login path,
33- return HttpResponseRedirect(settings.LOGIN_URL+"?next="+request.get_full_path())
34-
35- # Overide the standard admin login form.
36- sites.AdminSite.display_login_form = _openid_login
37-
38
39=== modified file 'django_openid_auth/admin.py'
40--- django_openid_auth/admin.py 2009-04-07 10:26:04 +0000
41+++ django_openid_auth/admin.py 2010-01-25 14:43:11 +0000
42@@ -1,6 +1,7 @@
43 # django-openid-auth - OpenID integration for django.contrib.auth
44 #
45 # Copyright (C) 2008-2009 Canonical Ltd.
46+# Copyright (C) 2010 Dave Walker
47 #
48 # Redistribution and use in source and binary forms, with or without
49 # modification, are permitted provided that the following conditions
50@@ -26,6 +27,7 @@
51 # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
52 # POSSIBILITY OF SUCH DAMAGE.
53
54+from django.conf import settings
55 from django.contrib import admin
56 from django_openid_auth.models import Nonce, Association, UserOpenID
57 from django_openid_auth.store import DjangoOpenIDStore
58@@ -64,3 +66,25 @@
59 search_fields = ('claimed_id',)
60
61 admin.site.register(UserOpenID, UserOpenIDAdmin)
62+
63+
64+# Support for allowing openid authentication for /admin (django.contrib.admin)
65+if getattr(settings, 'OPENID_USE_AS_ADMIN_LOGIN', False):
66+ from django.http import HttpResponseRedirect
67+ from django_openid_auth import views
68+
69+ def _openid_login(self, request, error_message='', extra_context=None):
70+ if request.user.is_authenticated():
71+ if not request.user.is_staff:
72+ return views.render_failure(
73+ request, "User %s does not have admin access."
74+ % request.user.username)
75+ return views.render_failure(
76+ request, "Unknown Error: %s" % error_message)
77+ else:
78+ # Redirect to openid login path,
79+ return HttpResponseRedirect(
80+ settings.LOGIN_URL + "?next=" + request.get_full_path())
81+
82+ # Overide the standard admin login form.
83+ admin.sites.AdminSite.display_login_form = _openid_login
84
85=== modified file 'django_openid_auth/forms.py'
86--- django_openid_auth/forms.py 2009-06-21 15:44:29 +0000
87+++ django_openid_auth/forms.py 2010-01-25 14:43:11 +0000
88@@ -27,7 +27,6 @@
89 # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
90 # POSSIBILITY OF SUCH DAMAGE.
91
92-import settings
93 from django import forms
94 from django.contrib.auth.admin import UserAdmin
95 from django.contrib.auth.forms import UserChangeForm
96
97=== modified file 'django_openid_auth/tests/test_views.py'
98--- django_openid_auth/tests/test_views.py 2010-01-22 05:14:38 +0000
99+++ django_openid_auth/tests/test_views.py 2010-01-25 14:43:11 +0000
100@@ -126,6 +126,7 @@
101 self.provider = StubOpenIDProvider('http://example.com/')
102 setDefaultFetcher(self.provider, wrap_exceptions=False)
103
104+ self.old_login_redirect_url = getattr(settings, 'LOGIN_REDIRECT_URL', '/accounts/profile/')
105 self.old_create_users = getattr(settings, 'OPENID_CREATE_USERS', False)
106 self.old_update_details = getattr(settings, 'OPENID_UPDATE_DETAILS_FROM_SREG', False)
107 self.old_sso_server_url = getattr(settings, 'OPENID_SSO_SERVER_URL')
108@@ -139,6 +140,7 @@
109 settings.OPENID_USE_AS_ADMIN_LOGIN = False
110
111 def tearDown(self):
112+ settings.LOGIN_REDIRECT_URL = self.old_login_redirect_url
113 settings.OPENID_CREATE_USERS = self.old_create_users
114 settings.OPENID_UPDATE_DETAILS_FROM_SREG = self.old_update_details
115 settings.OPENID_SSO_SERVER_URL = self.old_sso_server_url
116@@ -190,6 +192,31 @@
117 response = self.client.get('/getuser/')
118 self.assertEquals(response.content, 'someuser')
119
120+ def test_login_no_next(self):
121+ """Logins with no next parameter redirect to LOGIN_REDIRECT_URL."""
122+ user = User.objects.create_user('someuser', 'someone@example.com')
123+ useropenid = UserOpenID(
124+ user=user,
125+ claimed_id='http://example.com/identity',
126+ display_id='http://example.com/identity')
127+ useropenid.save()
128+
129+ settings.LOGIN_REDIRECT_URL = '/getuser/'
130+ response = self.client.post('/openid/login/',
131+ {'openid_identifier': 'http://example.com/identity'})
132+ self.assertContains(response, 'OpenID transaction in progress')
133+
134+ openid_request = self.provider.parseFormPost(response.content)
135+ self.assertEquals(openid_request.mode, 'checkid_setup')
136+ self.assertTrue(openid_request.return_to.startswith(
137+ 'http://testserver/openid/complete/'))
138+
139+ # Complete the request. The user is redirected to the next URL.
140+ openid_response = openid_request.answer(True)
141+ response = self.complete(openid_response)
142+ self.assertRedirects(
143+ response, 'http://testserver' + settings.LOGIN_REDIRECT_URL)
144+
145 def test_login_sso(self):
146 settings.OPENID_SSO_SERVER_URL = 'http://example.com/identity'
147 user = User.objects.create_user('someuser', 'someone@example.com')
148@@ -381,6 +408,9 @@
149 ("http://example.net/foo/bar?baz=quux", False),
150 ("/somewhere/local", True),
151 ("/somewhere/local?url=http://fail.com/bar", True),
152+ # An empty path, as seen when no "next" parameter is passed.
153+ ("", False),
154+ ("/path with spaces", False),
155 ]
156 for url, returns_self in urls:
157 sanitised = sanitise_redirect_url(url)
158
159=== modified file 'django_openid_auth/views.py'
160--- django_openid_auth/views.py 2010-01-14 21:39:55 +0000
161+++ django_openid_auth/views.py 2010-01-25 14:43:11 +0000
162@@ -64,7 +64,10 @@
163 def sanitise_redirect_url(redirect_to):
164 """Sanitise the redirection URL."""
165 # Light security check -- make sure redirect_to isn't garbage.
166- if not redirect_to or '//' in redirect_to or ' ' in redirect_to:
167+ is_valid = True
168+ if not redirect_to or ' ' in redirect_to:
169+ is_valid = False
170+ elif '//' in redirect_to:
171 # Allow the redirect URL to be external if it's a permitted domain
172 allowed_domains = getattr(settings,
173 "ALLOWED_EXTERNAL_OPENID_REDIRECT_DOMAINS", [])
174@@ -75,11 +78,12 @@
175 if netloc.find(":") != -1:
176 netloc, _ = netloc.split(":", 1)
177 if netloc not in allowed_domains:
178- redirect_to = settings.LOGIN_REDIRECT_URL
179- else:
180- # netloc is blank, so it's a local URL (possibly with another URL
181- # passed in the querystring. Allow it.)
182- pass
183+ is_valid = False
184+
185+ # If the return_to URL is not valid, use the default.
186+ if not is_valid:
187+ redirect_to = settings.LOGIN_REDIRECT_URL
188+
189 return redirect_to
190
191

Subscribers

People subscribed via source and target branches