Fix #278: Allow ACS_DEFAULT_REDIRECT_URL to override LOGIN_REDIRECT_URL in more places (#285)

Author: Jaap Roes

djangosaml2/tests/__init__.py

@@ -27,14 +27,15 @@
 from django.core.exceptions import ImproperlyConfigured
 from django.test import Client, TestCase, override_settings
 from django.test.client import RequestFactory
-from django.urls import reverse
+from django.urls import reverse, reverse_lazy
 from django.utils.encoding import force_text
 from djangosaml2 import views
 from djangosaml2.cache import OutstandingQueriesCache
 from djangosaml2.conf import get_config
 from djangosaml2.middleware import SamlSessionMiddleware
 from djangosaml2.tests import conf
-from djangosaml2.utils import (get_idp_sso_supported_bindings,
+from djangosaml2.utils import (get_fallback_login_redirect_url,
+                               get_idp_sso_supported_bindings,
                                get_session_id_from_saml2,
                                get_subject_id_from_saml2,
                                saml2_from_httpredirect_request)
@@ -76,6 +77,23 @@ def test_get_config_missing_function(self):
         with self.assertRaisesMessage(ImproperlyConfigured, 'Module "djangosaml2.tests" does not define a "nonexisting_function" attribute/class'):
             get_config('djangosaml2.tests.nonexisting_function')
 
+    @override_settings(LOGIN_REDIRECT_URL='/accounts/profile/')
+    def test_get_fallback_login_redirect_url(self):
+        self.assertEqual(get_fallback_login_redirect_url(), '/accounts/profile/')
+
+        with override_settings():
+            del settings.LOGIN_REDIRECT_URL
+            # Neither LOGIN_REDIRECT_URL nor ACS_DEFAULT_REDIRECT_URL is configured
+            self.assertEqual(get_fallback_login_redirect_url(), '/')
+
+        with override_settings(ACS_DEFAULT_REDIRECT_URL='testprofiles:dashboard'):
+            # ACS_DEFAULT_REDIRECT_URL is configured, so it is used (and resolved)
+            self.assertEqual(get_fallback_login_redirect_url(), '/dashboard/')
+
+        with override_settings(ACS_DEFAULT_REDIRECT_URL=reverse_lazy('testprofiles:dashboard')):
+            # Lazy urls are resolved
+            self.assertEqual(get_fallback_login_redirect_url(), '/dashboard/')
+
 
 class SAML2Tests(TestCase):
 
@@ -158,7 +176,7 @@ def test_unsigned_post_authn_request(self):
     def test_login_evil_redirect(self):
         """
         Make sure that if we give an URL other than our own host as the next
-        parameter, it is replaced with the default LOGIN_REDIRECT_URL.
+        parameter, it is replaced with the fallback login redirect url.
         """
 
         # monkey patch SAML configuration
@@ -178,10 +196,19 @@ def test_login_evil_redirect(self):
 
                     self.assertEqual(params['RelayState'], ['/dashboard/'])
 
+            with self.subTest(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                with override_settings(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                    response = self.client.get(
+                        reverse('saml2_login') + '?next=http://evil.com')
+                    url = urlparse(response['Location'])
+                    params = parse_qs(url.query)
+
+                    self.assertEqual(params['RelayState'], ['/dashboard/'])
+
     def test_no_redirect(self):
         """
         Make sure that if we give an empty path as the next parameter,
-        it is replaced with the default LOGIN_REDIRECT_URL.
+        it is replaced with the fallback login redirect url.
         """
 
         # monkey patch SAML configuration
@@ -200,6 +227,14 @@ def test_no_redirect(self):
 
                     self.assertEqual(params['RelayState'], ['/dashboard/'])
 
+            with self.subTest(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                with override_settings(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                    response = self.client.get(reverse('saml2_login') + '?next=')
+                    url = urlparse(response['Location'])
+                    params = parse_qs(url.query)
+
+                    self.assertEqual(params['RelayState'], ['/dashboard/'])
+
     @override_settings(SAML_IGNORE_AUTHENTICATED_USERS_ON_LOGIN=True)
     def test_login_already_logged(self):
         self.client.force_login(User.objects.create(username='user', password='pass'))
@@ -215,6 +250,16 @@ def test_login_already_logged(self):
                         response = self.client.get(reverse('saml2_login') + '?next=http://evil.com')
                         self.assertRedirects(response, '/dashboard/')
 
+            with self.subTest(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                with override_settings(ACS_DEFAULT_REDIRECT_URL=redirect_url):
+                    with self.subTest('no next url'):
+                        response = self.client.get(reverse('saml2_login'))
+                        self.assertRedirects(response, '/dashboard/')
+
+                    with self.subTest('evil next url'):
+                        response = self.client.get(reverse('saml2_login') + '?next=http://evil.com')
+                        self.assertRedirects(response, '/dashboard/')
+
     def test_unknown_idp(self):
         # monkey patch SAML configuration
         settings.SAML_CONFIG = conf.create_conf(
@@ -300,7 +345,7 @@ def test_login_several_idps(self):
         self.assertIn('AuthnRequest xmlns', decode_base64_and_inflate(
             saml_request).decode('utf-8'))
 
-    @override_settings(LOGIN_REDIRECT_URL='testprofiles:dashboard')
+    @override_settings(ACS_DEFAULT_REDIRECT_URL='testprofiles:dashboard')
     def test_assertion_consumer_service(self):
         # Get initial number of users
         initial_user_count = User.objects.count()
@@ -350,11 +395,11 @@ def test_assertion_consumer_service(self):
             'RelayState': came_from,
         })
 
-        # as the RelayState is empty we have redirect to LOGIN_REDIRECT_URL
+        # as the RelayState is empty we have redirect to ACS_DEFAULT_REDIRECT_URL
         self.assertRedirects(response, '/dashboard/')
         self.assertEqual(force_text(new_user.id), client.session[SESSION_KEY])
 
-    @override_settings(LOGIN_REDIRECT_URL='testprofiles:dashboard')
+    @override_settings(ACS_DEFAULT_REDIRECT_URL='testprofiles:dashboard')
     def test_assertion_consumer_service_default_relay_state(self):
         settings.SAML_CONFIG = conf.create_conf(
             sp_host='sp.example.com',
@@ -375,7 +420,7 @@ def test_assertion_consumer_service_default_relay_state(self):
         })
         self.assertEqual(response.status_code, 302)
 
-        # The RelayState is missing, redirect to LOGIN_REDIRECT_URL
+        # The RelayState is missing, redirect to ACS_DEFAULT_REDIRECT_URL
         self.assertRedirects(response, '/dashboard/')
         self.assertEqual(force_text(new_user.id), self.client.session[SESSION_KEY])
 

djangosaml2/utils.py

@@ -82,6 +82,11 @@ def get_location(http_info):
         return http_info['url']
 
 
+def get_fallback_login_redirect_url():
+    login_redirect_url = get_custom_setting('LOGIN_REDIRECT_URL', '/')
+    return resolve_url(get_custom_setting('ACS_DEFAULT_REDIRECT_URL', login_redirect_url))
+
+
 def validate_referral_url(request, url):
     # Ensure the user-originating redirection url is safe.
     # By setting SAML_ALLOWED_HOSTS in settings.py the user may provide a list of "allowed"
@@ -92,7 +97,7 @@ def validate_referral_url(request, url):
         getattr(settings, 'SAML_ALLOWED_HOSTS', [request.get_host()]))
 
     if not is_safe_url(url=url, allowed_hosts=saml_allowed_hosts):
-        return resolve_url(settings.LOGIN_REDIRECT_URL)
+        return get_fallback_login_redirect_url()
     return url
 
 

djangosaml2/views.py

@@ -24,7 +24,7 @@
 from django.core.exceptions import PermissionDenied, SuspiciousOperation
 from django.http import (HttpRequest, HttpResponse, HttpResponseBadRequest,
                          HttpResponseRedirect, HttpResponseServerError)
-from django.shortcuts import render, resolve_url
+from django.shortcuts import render
 from django.template import TemplateDoesNotExist
 from django.urls import reverse
 from django.utils.decorators import method_decorator
@@ -52,6 +52,7 @@
 from .exceptions import IdPConfigurationMissing
 from .overrides import Saml2Client
 from .utils import (add_idp_hinting, available_idps, get_custom_setting,
+                    get_fallback_login_redirect_url,
                     get_idp_sso_supported_bindings, get_location,
                     validate_referral_url)
 
@@ -116,7 +117,7 @@ def get_next_path(self, request: HttpRequest) -> str:
             If the user is already logged in (and if allowed), he will redirect to there immediately.
         '''
 
-        next_path = resolve_url(settings.LOGIN_REDIRECT_URL)
+        next_path = get_fallback_login_redirect_url()
         if 'next' in request.GET:
             next_path = request.GET['next']
         elif 'RelayState' in request.GET:
@@ -500,9 +501,7 @@ def post_login_hook(self, request: HttpRequest, user: settings.AUTH_USER_MODEL,
     def build_relay_state(self) -> str:
         """ The relay state is a URL used to redirect the user to the view where they came from.
         """
-        login_redirect_url = get_custom_setting('LOGIN_REDIRECT_URL', '/')
-        default_relay_state = resolve_url(
-            get_custom_setting('ACS_DEFAULT_REDIRECT_URL', login_redirect_url))
+        default_relay_state = get_fallback_login_redirect_url()
         relay_state = self.request.POST.get('RelayState', default_relay_state)
         relay_state = self.customize_relay_state(relay_state)
         if not relay_state:

docs/source/contents/setup.rst

@@ -138,8 +138,9 @@ hostnames to be used for the post-login redirect.  In such cases, the setting::
 May be set to a list of allowed post-login redirect hostnames (note, the URL components beyond the hostname
 may be specified by the client - typically with the ?next= parameter.)
 
-In the absence of a `?next=parameter`, the `LOGIN_REDIRECT_URL` setting will be used (assuming the destination hostname
-either matches the output of get_host() or is included in the SAML_ALLOWED_HOSTS setting)
+In the absence of a ``?next=parameter``, the ``ACS_DEFAULT_REDIRECT_URL`` or ``LOGIN_REDIRECT_URL`` setting will
+be used (assuming the destination hostname either matches the output of get_host() or is included in the
+``SAML_ALLOWED_HOSTS`` setting)
 
 Preferred sso binding
 =====================
@@ -312,7 +313,7 @@ authentication::
 
 Particularly useful when you only plan to use
 IdP initiated login and the IdP does not have a configured RelayState
-parameter. The default is ``/``.
+parameter. If not set Django's ``LOGIN_REDIRECT_URL`` or ``/`` will be used.
 
 The other thing you will probably want to configure is the mapping of
 SAML2 user attributes to Django user attributes. By default only the