From 980ae2ab29cb25182404eb1ccc919573edcc44f5 Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Sun, 19 May 2013 12:57:06 +0200 Subject: [PATCH] Fix #20447: URL names given to contrib.auth.views are now resolved. This commit also adds tests for the redirect feature of most auth views. It also cleans up the tests, most notably using @override_settings instead of ad-hoc setUp/tearDown methods. Thanks to caumons for the report. Conflicts: docs/releases/1.6.txt --- django/contrib/auth/tests/test_views.py | 214 +++++++++++++++++------- django/contrib/auth/tests/urls.py | 11 ++ django/contrib/auth/views.py | 9 + docs/releases/1.6.txt | 6 + 4 files changed, 180 insertions(+), 60 deletions(-) diff --git a/django/contrib/auth/tests/test_views.py b/django/contrib/auth/tests/test_views.py index 7cbf72327e..cd045fb793 100644 --- a/django/contrib/auth/tests/test_views.py +++ b/django/contrib/auth/tests/test_views.py @@ -1,6 +1,10 @@ import itertools import os import re +try: + from urllib.parse import urlparse, ParseResult +except ImportError: # Python 2 + from urlparse import urlparse, ParseResult from django.conf import global_settings, settings from django.contrib.sites.models import Site, RequestSite @@ -46,15 +50,26 @@ class AuthViewsTestCase(TestCase): 'username': 'testclient', 'password': password, }) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith(settings.LOGIN_REDIRECT_URL)) self.assertTrue(SESSION_KEY in self.client.session) + return response def assertFormError(self, response, error): """Assert that error is found in response.context['form'] errors""" form_errors = list(itertools.chain(*response.context['form'].errors.values())) self.assertIn(force_text(error), form_errors) + def assertURLEqual(self, url, expected): + """ + Given two URLs, make sure all their components (the ones given by + urlparse) are equal, only comparing components that are present in both + URLs. + """ + fields = ParseResult._fields + + for attr, x, y in zip(fields, urlparse(url), urlparse(expected)): + if x and y and x != y: + self.fail("%r != %r (%s doesn't match)" % (url, expected, attr)) + @skipIfCustomUser class AuthViewNamedURLTests(AuthViewsTestCase): @@ -153,7 +168,6 @@ class PasswordResetTest(AuthViewsTestCase): def _test_confirm_start(self): # Start by creating the email response = self.client.post('/password_reset/', {'email': 'staffmember@example.com'}) - self.assertEqual(response.status_code, 302) self.assertEqual(len(mail.outbox), 1) return self._read_signup_email(mail.outbox[0]) @@ -205,8 +219,6 @@ class PasswordResetTest(AuthViewsTestCase): url, path = self._test_confirm_start() response = self.client.post(path, {'new_password1': 'anewpassword', 'new_password2': 'anewpassword'}) - # It redirects us to a 'complete' page: - self.assertEqual(response.status_code, 302) # Check the password has been changed u = User.objects.get(email='staffmember@example.com') self.assertTrue(u.check_password("anewpassword")) @@ -221,6 +233,47 @@ class PasswordResetTest(AuthViewsTestCase): 'new_password2': 'x'}) self.assertFormError(response, SetPasswordForm.error_messages['password_mismatch']) + def test_reset_redirect_default(self): + response = self.client.post('/password_reset/', + {'email': 'staffmember@example.com'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_reset/done/') + + def test_reset_custom_redirect(self): + response = self.client.post('/password_reset/custom_redirect/', + {'email': 'staffmember@example.com'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/custom/') + + def test_reset_custom_redirect_named(self): + response = self.client.post('/password_reset/custom_redirect/named/', + {'email': 'staffmember@example.com'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_reset/') + + def test_confirm_redirect_default(self): + url, path = self._test_confirm_start() + response = self.client.post(path, {'new_password1': 'anewpassword', + 'new_password2': 'anewpassword'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/reset/done/') + + def test_confirm_redirect_custom(self): + url, path = self._test_confirm_start() + path = path.replace('/reset/', '/reset/custom/') + response = self.client.post(path, {'new_password1': 'anewpassword', + 'new_password2': 'anewpassword'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/custom/') + + def test_confirm_redirect_custom_named(self): + url, path = self._test_confirm_start() + path = path.replace('/reset/', '/reset/custom/named/') + response = self.client.post(path, {'new_password1': 'anewpassword', + 'new_password2': 'anewpassword'}) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_reset/') + @override_settings(AUTH_USER_MODEL='auth.CustomUser') class CustomUserPasswordResetTest(AuthViewsTestCase): @@ -285,8 +338,6 @@ class ChangePasswordTest(AuthViewsTestCase): 'new_password1': 'password1', 'new_password2': 'password1', }) - self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/password_change/done/')) self.fail_login() self.login(password='password1') @@ -298,13 +349,43 @@ class ChangePasswordTest(AuthViewsTestCase): 'new_password2': 'password1', }) self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/password_change/done/')) + self.assertURLEqual(response.url, '/password_change/done/') + @override_settings(LOGIN_URL='/login/') def test_password_change_done_fails(self): - with self.settings(LOGIN_URL='/login/'): - response = self.client.get('/password_change/done/') - self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/login/?next=/password_change/done/')) + response = self.client.get('/password_change/done/') + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/login/?next=/password_change/done/') + + def test_password_change_redirect_default(self): + self.login() + response = self.client.post('/password_change/', { + 'old_password': 'password', + 'new_password1': 'password1', + 'new_password2': 'password1', + }) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_change/done/') + + def test_password_change_redirect_custom(self): + self.login() + response = self.client.post('/password_change/custom/', { + 'old_password': 'password', + 'new_password1': 'password1', + 'new_password2': 'password1', + }) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/custom/') + + def test_password_change_redirect_custom_named(self): + self.login() + response = self.client.post('/password_change/custom/named/', { + 'old_password': 'password', + 'new_password1': 'password1', + 'new_password2': 'password1', + }) + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_reset/') @skipIfCustomUser @@ -374,63 +455,68 @@ class LoginTest(AuthViewsTestCase): # the custom authentication form used by this login asserts # that a request is passed to the form successfully. + @skipIfCustomUser class LoginURLSettings(AuthViewsTestCase): - - def setUp(self): - super(LoginURLSettings, self).setUp() - self.old_LOGIN_URL = settings.LOGIN_URL - - def tearDown(self): - super(LoginURLSettings, self).tearDown() - settings.LOGIN_URL = self.old_LOGIN_URL - - def get_login_required_url(self, login_url): - settings.LOGIN_URL = login_url + """Tests for settings.LOGIN_URL.""" + def assertLoginURLEquals(self, url): response = self.client.get('/login_required/') self.assertEqual(response.status_code, 302) - return response.url + self.assertURLEqual(response.url, url) + @override_settings(LOGIN_URL='/login/') def test_standard_login_url(self): - login_url = '/login/' - login_required_url = self.get_login_required_url(login_url) - querystring = QueryDict('', mutable=True) - querystring['next'] = '/login_required/' - self.assertEqual(login_required_url, 'http://testserver%s?%s' % - (login_url, querystring.urlencode('/'))) + self.assertLoginURLEquals('/login/?next=/login_required/') + @override_settings(LOGIN_URL='login') + def test_named_login_url(self): + self.assertLoginURLEquals('/login/?next=/login_required/') + + @override_settings(LOGIN_URL='http://remote.example.com/login') def test_remote_login_url(self): - login_url = 'http://remote.example.com/login' - login_required_url = self.get_login_required_url(login_url) - querystring = QueryDict('', mutable=True) - querystring['next'] = 'http://testserver/login_required/' - self.assertEqual(login_required_url, - '%s?%s' % (login_url, querystring.urlencode('/'))) + quoted_next = urlquote('http://testserver/login_required/') + expected = 'http://remote.example.com/login?next=%s' % quoted_next + self.assertLoginURLEquals(expected) + @override_settings(LOGIN_URL='https:///login/') def test_https_login_url(self): - login_url = 'https:///login/' - login_required_url = self.get_login_required_url(login_url) - querystring = QueryDict('', mutable=True) - querystring['next'] = 'http://testserver/login_required/' - self.assertEqual(login_required_url, - '%s?%s' % (login_url, querystring.urlencode('/'))) + quoted_next = urlquote('http://testserver/login_required/') + expected = 'https:///login/?next=%s' % quoted_next + self.assertLoginURLEquals(expected) + @override_settings(LOGIN_URL='/login/?pretty=1') def test_login_url_with_querystring(self): - login_url = '/login/?pretty=1' - login_required_url = self.get_login_required_url(login_url) - querystring = QueryDict('pretty=1', mutable=True) - querystring['next'] = '/login_required/' - self.assertEqual(login_required_url, 'http://testserver/login/?%s' % - querystring.urlencode('/')) + self.assertLoginURLEquals('/login/?pretty=1&next=/login_required/') + @override_settings(LOGIN_URL='http://remote.example.com/login/?next=/default/') def test_remote_login_url_with_next_querystring(self): - login_url = 'http://remote.example.com/login/' - login_required_url = self.get_login_required_url('%s?next=/default/' % - login_url) - querystring = QueryDict('', mutable=True) - querystring['next'] = 'http://testserver/login_required/' - self.assertEqual(login_required_url, '%s?%s' % (login_url, - querystring.urlencode('/'))) + quoted_next = urlquote('http://testserver/login_required/') + expected = 'http://remote.example.com/login/?next=%s' % quoted_next + self.assertLoginURLEquals(expected) + + +@skipIfCustomUser +class LoginRedirectUrlTest(AuthViewsTestCase): + """Tests for settings.LOGIN_REDIRECT_URL.""" + def assertLoginRedirectURLEqual(self, url): + response = self.login() + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, url) + + def test_default(self): + self.assertLoginRedirectURLEqual('/accounts/profile/') + + @override_settings(LOGIN_REDIRECT_URL='/custom/') + def test_custom(self): + self.assertLoginRedirectURLEqual('/custom/') + + @override_settings(LOGIN_REDIRECT_URL='password_reset') + def test_named(self): + self.assertLoginRedirectURLEqual('/password_reset/') + + @override_settings(LOGIN_REDIRECT_URL='http://remote.example.com/welcome/') + def test_remote(self): + self.assertLoginRedirectURLEqual('http://remote.example.com/welcome/') @skipIfCustomUser @@ -457,11 +543,11 @@ class LogoutTest(AuthViewsTestCase): self.login() response = self.client.get('/logout/next_page/') self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/somewhere/')) + self.assertURLEqual(response.url, '/somewhere/') response = self.client.get('/logout/next_page/?next=/login/') self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/login/')) + self.assertURLEqual(response.url, '/login/') self.confirm_logged_out() @@ -470,7 +556,7 @@ class LogoutTest(AuthViewsTestCase): self.login() response = self.client.get('/logout/next_page/') self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/somewhere/')) + self.assertURLEqual(response.url, '/somewhere/') self.confirm_logged_out() def test_logout_with_redirect_argument(self): @@ -478,7 +564,7 @@ class LogoutTest(AuthViewsTestCase): self.login() response = self.client.get('/logout/?next=/login/') self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/login/')) + self.assertURLEqual(response.url, '/login/') self.confirm_logged_out() def test_logout_with_custom_redirect_argument(self): @@ -486,7 +572,15 @@ class LogoutTest(AuthViewsTestCase): self.login() response = self.client.get('/logout/custom_query/?follow=/somewhere/') self.assertEqual(response.status_code, 302) - self.assertTrue(response.url.endswith('/somewhere/')) + self.assertURLEqual(response.url, '/somewhere/') + self.confirm_logged_out() + + def test_logout_with_named_redirect(self): + "Logout resolves names or URLs passed as next_page." + self.login() + response = self.client.get('/logout/next_page/named/') + self.assertEqual(response.status_code, 302) + self.assertURLEqual(response.url, '/password_reset/') self.confirm_logged_out() def test_security_check(self, password='password'): diff --git a/django/contrib/auth/tests/urls.py b/django/contrib/auth/tests/urls.py index 51b05be648..835ff41de7 100644 --- a/django/contrib/auth/tests/urls.py +++ b/django/contrib/auth/tests/urls.py @@ -62,8 +62,19 @@ def custom_request_auth_login(request): urlpatterns = urlpatterns + patterns('', (r'^logout/custom_query/$', 'django.contrib.auth.views.logout', dict(redirect_field_name='follow')), (r'^logout/next_page/$', 'django.contrib.auth.views.logout', dict(next_page='/somewhere/')), + (r'^logout/next_page/named/$', 'django.contrib.auth.views.logout', dict(next_page='password_reset')), (r'^remote_user/$', remote_user_auth_view), (r'^password_reset_from_email/$', 'django.contrib.auth.views.password_reset', dict(from_email='staffmember@example.com')), + (r'^password_reset/custom_redirect/$', 'django.contrib.auth.views.password_reset', dict(post_reset_redirect='/custom/')), + (r'^password_reset/custom_redirect/named/$', 'django.contrib.auth.views.password_reset', dict(post_reset_redirect='password_reset')), + (r'^reset/custom/(?P[0-9A-Za-z]{1,13})-(?P[0-9A-Za-z]{1,13}-[0-9A-Za-z]{1,20})/$', + 'django.contrib.auth.views.password_reset_confirm', + dict(post_reset_redirect='/custom/')), + (r'^reset/custom/named/(?P[0-9A-Za-z]{1,13})-(?P[0-9A-Za-z]{1,13}-[0-9A-Za-z]{1,20})/$', + 'django.contrib.auth.views.password_reset_confirm', + dict(post_reset_redirect='password_reset')), + (r'^password_change/custom/$', 'django.contrib.auth.views.password_change', dict(post_change_redirect='/custom/')), + (r'^password_change/custom/named/$', 'django.contrib.auth.views.password_change', dict(post_change_redirect='password_reset')), (r'^admin_password_reset/$', 'django.contrib.auth.views.password_reset', dict(is_admin_site=True)), (r'^login_required/$', login_required(password_reset)), (r'^login_required_login_url/$', login_required(password_reset, login_url='/somewhere/')), diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index 8a554b0ad8..9691b31544 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -72,6 +72,9 @@ def logout(request, next_page=None, """ auth_logout(request) + if next_page is not None: + next_page = resolve_url(next_page) + if redirect_field_name in request.REQUEST: next_page = request.REQUEST[redirect_field_name] # Security check -- don't allow redirection to a different host. @@ -140,6 +143,8 @@ def password_reset(request, is_admin_site=False, extra_context=None): if post_reset_redirect is None: post_reset_redirect = reverse('django.contrib.auth.views.password_reset_done') + else: + post_reset_redirect = resolve_url(post_reset_redirect) if request.method == "POST": form = password_reset_form(request.POST) if form.is_valid(): @@ -193,6 +198,8 @@ def password_reset_confirm(request, uidb36=None, token=None, assert uidb36 is not None and token is not None # checked by URLconf if post_reset_redirect is None: post_reset_redirect = reverse('django.contrib.auth.views.password_reset_complete') + else: + post_reset_redirect = resolve_url(post_reset_redirect) try: uid_int = base36_to_int(uidb36) user = UserModel._default_manager.get(pk=uid_int) @@ -243,6 +250,8 @@ def password_change(request, current_app=None, extra_context=None): if post_change_redirect is None: post_change_redirect = reverse('django.contrib.auth.views.password_change_done') + else: + post_change_redirect = resolve_url(post_change_redirect) if request.method == "POST": form = password_change_form(user=request.user, data=request.POST) if form.is_valid(): diff --git a/docs/releases/1.6.txt b/docs/releases/1.6.txt index 6643fb7d32..a6244498f2 100644 --- a/docs/releases/1.6.txt +++ b/docs/releases/1.6.txt @@ -243,6 +243,12 @@ Minor features * The reason phrase can be customized in HTTP responses. +* When giving the URL of the next page for :func:`~django.contrib.auth.views.logout`, + :func:`~django.contrib.auth.views.password_reset`, + :func:`~django.contrib.auth.views.password_reset_confirm`, + and :func:`~django.contrib.auth.views.password_change`, you can now pass + URL names and they will be resolved. + Backwards incompatible changes in 1.6 =====================================