From f7693a8ff7dce18c0d65ec584be0da9e433101f8 Mon Sep 17 00:00:00 2001 From: Maxim Beder Date: Tue, 23 Nov 2021 17:48:39 +0100 Subject: [PATCH 1/2] fix: don't pass tpa_hint in next query parameter --- .../core/djangoapps/user_authn/middleware.py | 14 +++++++++---- .../user_authn/tests/test_middlewares.py | 21 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/middleware.py b/openedx/core/djangoapps/user_authn/middleware.py index 8b58026ce8e2..f89709c9d05f 100644 --- a/openedx/core/djangoapps/user_authn/middleware.py +++ b/openedx/core/djangoapps/user_authn/middleware.py @@ -17,7 +17,8 @@ class RedirectUnauthenticatedToLoginMiddleware: register pages. If redirects, passes the requested url to login as 'next' query string - parameter. + parameter. If the original url has a 'tpa_hint' query parameter, the + parameter is passed to login without being escaped. To enable the middleware, ENABLE_REDIRECT_UNAUTHENTICATED_USERS_TO_LOGIN setting has to be set to True. @@ -55,6 +56,11 @@ def _get_redirect_query_string(self, request): Generates query string for redirect. """ # calling copy to get mutable QueryDict - query = QueryDict().copy() - query['next'] = request.get_full_path() - return query.urlencode() + request_query = request.GET.copy() + redirect_query = QueryDict().copy() + + if 'tpa_hint' in request_query: + redirect_query.setlist('tpa_hint', request_query.pop('tpa_hint')) + redirect_query['next'] = request.path + '?' + request_query.urlencode() + + return redirect_query.urlencode() diff --git a/openedx/core/djangoapps/user_authn/tests/test_middlewares.py b/openedx/core/djangoapps/user_authn/tests/test_middlewares.py index 61e8fcdb89e3..510cbfe05434 100644 --- a/openedx/core/djangoapps/user_authn/tests/test_middlewares.py +++ b/openedx/core/djangoapps/user_authn/tests/test_middlewares.py @@ -73,6 +73,27 @@ def test_passes_url_in_next_query_string(self): self.assertIn('next', query) self.assertEqual(query['next'], '/dashboard?test=123') + def test_passes_tpa_hint_parameter_to_login(self): + """ + Middleware passes 'tpa_hint' query parameter to login. + + When redirecting, if the original url has 'tpa_hint' query string + parameter, the middleware does not pass the parameter throug the 'next' + query string parameter, doesn't escape it, and passes it to login as + normal query string parameter. + """ + request = RequestFactory().get('/dashboard?tpa_hint=test-tpa&test=123') + request.user = AnonymousUserFactory.create() + + response = self.middleware(request) + + self.assertNotEqual(response, self.mock_response) + query = QueryDict(urlparse(response.url).query) + self.assertIn('next', query) + self.assertEqual(query['next'], '/dashboard?test=123') + self.assertIn('tpa_hint', query) + self.assertEqual(query['tpa_hint'], 'test-tpa') + def test_does_not_redirect_if_user_is_authenticated(self): """ Middleware doesn't redirect authenticated users. From 1cdd71f8bb49b9e050b0c833397625655a643cf0 Mon Sep 17 00:00:00 2001 From: Maxim Beder Date: Tue, 23 Nov 2021 18:47:18 +0100 Subject: [PATCH 2/2] feat: add whitelist for urls --- openedx/core/djangoapps/user_authn/middleware.py | 16 ++++++++++++++++ .../user_authn/tests/test_middlewares.py | 12 ++++++++++++ 2 files changed, 28 insertions(+) diff --git a/openedx/core/djangoapps/user_authn/middleware.py b/openedx/core/djangoapps/user_authn/middleware.py index f89709c9d05f..cf86da886bcc 100644 --- a/openedx/core/djangoapps/user_authn/middleware.py +++ b/openedx/core/djangoapps/user_authn/middleware.py @@ -26,6 +26,11 @@ class RedirectUnauthenticatedToLoginMiddleware: Assumed that the requests passed to the middleware have user attribute set. """ + WHITELIST = [ + '/admin/', + '/auth/', + ] + def __init__(self, get_response): self.get_response = get_response @@ -44,10 +49,12 @@ def _should_redirect(self, request): is_get_request = request.method == 'GET' is_login_or_register_url = request.path in (settings.LOGIN_URL, '/register') + is_in_whitelist = self._check_whitelist(request.path) return ( is_get_request and (not is_login_or_register_url) and + (not is_in_whitelist) and (not request.user.is_authenticated) ) @@ -64,3 +71,12 @@ def _get_redirect_query_string(self, request): redirect_query['next'] = request.path + '?' + request_query.urlencode() return redirect_query.urlencode() + + def _check_whitelist(self, path): + """ + Check if path is whitelisted. + """ + return any(map( + lambda pattern: path.startswith(pattern), + RedirectUnauthenticatedToLoginMiddleware.WHITELIST, + )) diff --git a/openedx/core/djangoapps/user_authn/tests/test_middlewares.py b/openedx/core/djangoapps/user_authn/tests/test_middlewares.py index 510cbfe05434..7f74b5363db7 100644 --- a/openedx/core/djangoapps/user_authn/tests/test_middlewares.py +++ b/openedx/core/djangoapps/user_authn/tests/test_middlewares.py @@ -141,3 +141,15 @@ def test_does_not_redirect_unauthenticated_user_if_setting_disabled(self): response = self.middleware(request) self.assertEqual(response, self.mock_response) + + @ddt.data(*RedirectUnauthenticatedToLoginMiddleware.WHITELIST) + def test_does_not_redirect_unauthenticated_user_if_path_in_whitelist(self, whitelisted_url): + """ + Middleware doesn't redirect if requested path is whitelisted. + """ + request = RequestFactory().get(whitelisted_url + 'test') + request.user = AnonymousUserFactory.create() + + response = self.middleware(request) + + self.assertEqual(response, self.mock_response)