From f78e2be3c548e22cc5310eb39386f0f2b07074c7 Mon Sep 17 00:00:00 2001 From: Jan Brauer Date: Wed, 19 Jul 2017 10:52:10 +0200 Subject: [PATCH] Fix infinite login loop if "prompt=login" (#198) * Add test to expose issue #197 * Strip 'login' from prompt before redirecting This fixes #197. Otherwise the user would have to login once, then is immediately logged out and prompted to login again. * Only remove 'login' if present * Don't append an empty prompt parameter * Inline variable --- .../tests/test_authorize_endpoint.py | 14 ++++++++++-- oidc_provider/views.py | 22 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/oidc_provider/tests/test_authorize_endpoint.py b/oidc_provider/tests/test_authorize_endpoint.py index 41fbb19..361f27f 100644 --- a/oidc_provider/tests/test_authorize_endpoint.py +++ b/oidc_provider/tests/test_authorize_endpoint.py @@ -1,9 +1,9 @@ from oidc_provider.lib.errors import RedirectUriError try: - from urllib.parse import urlencode + from urllib.parse import urlencode, quote except ImportError: - from urllib import urlencode + from urllib import urlencode, quote try: from urllib.parse import parse_qs, urlsplit except ImportError: @@ -369,10 +369,20 @@ class AuthorizationCodeFlowTestCase(TestCase, AuthorizeEndpointMixin): response = self._auth_request('get', data) self.assertIn(settings.get('OIDC_LOGIN_URL'), response['Location']) + self.assertNotIn( + quote('prompt=login'), + response['Location'], + "Found prompt=login, this leads to infinite login loop. See https://github.com/juanifioren/django-oidc-provider/issues/197." + ) response = self._auth_request('get', data, is_user_authenticated=True) self.assertIn(settings.get('OIDC_LOGIN_URL'), response['Location']) self.assertTrue(logout_function.called_once()) + self.assertNotIn( + quote('prompt=login'), + response['Location'], + "Found prompt=login, this leads to infinite login loop. See https://github.com/juanifioren/django-oidc-provider/issues/197." + ) def test_prompt_login_none_parameter(self): """ diff --git a/oidc_provider/views.py b/oidc_provider/views.py index a5f8cc6..27d1499 100644 --- a/oidc_provider/views.py +++ b/oidc_provider/views.py @@ -66,13 +66,14 @@ class AuthorizeView(View): client=authorize.client) if hook_resp: return hook_resp - + if 'login' in authorize.params['prompt']: if 'none' in authorize.params['prompt']: raise AuthorizeError(authorize.params['redirect_uri'], 'login_required', authorize.grant_type) else: django_user_logout(request) - return redirect_to_login(request.get_full_path(), settings.get('OIDC_LOGIN_URL')) + next_page = self.strip_prompt_login(request.get_full_path()) + return redirect_to_login(next_page, settings.get('OIDC_LOGIN_URL')) if 'select_account' in authorize.params['prompt']: # TODO: see how we can support multiple accounts for the end-user. @@ -127,6 +128,9 @@ class AuthorizeView(View): else: if 'none' in authorize.params['prompt']: raise AuthorizeError(authorize.params['redirect_uri'], 'login_required', authorize.grant_type) + if 'login' in authorize.params['prompt']: + next_page = self.strip_prompt_login(request.get_full_path()) + return redirect_to_login(next_page, settings.get('OIDC_LOGIN_URL')) return redirect_to_login(request.get_full_path(), settings.get('OIDC_LOGIN_URL')) @@ -174,6 +178,20 @@ class AuthorizeView(View): return redirect(uri) + @staticmethod + def strip_prompt_login(path): + """ + Strips 'login' from the 'prompt' query parameter. + """ + uri = urlsplit(path) + query_params = parse_qs(uri.query) + if 'login' in query_params['prompt']: + query_params['prompt'].remove('login') + if not query_params['prompt']: + del query_params['prompt'] + uri = uri._replace(query=urlencode(query_params, doseq=True)) + return urlunsplit(uri) + class TokenView(View): def post(self, request, *args, **kwargs):