From 863f86c41128f93ef4032e843fbecbcf082ca8c3 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Fri, 12 Oct 2018 13:33:09 -0400 Subject: [PATCH] Move and rename redirect helper. - Rename is_safe_redirect to is_safe_login_or_logout_redirect. - Moved is_safe_login_or_logout_redirect to user_authn. ARCH-256 --- common/djangoapps/student/helpers.py | 17 ++-------- .../djangoapps/student/tests/test_helpers.py | 18 +--------- .../djangoapps/user_authn/tests/test_utils.py | 33 +++++++++++++++++++ openedx/core/djangoapps/user_authn/utils.py | 18 ++++++++++ .../djangoapps/user_authn/views/logout.py | 4 +-- 5 files changed, 56 insertions(+), 34 deletions(-) create mode 100644 openedx/core/djangoapps/user_authn/tests/test_utils.py create mode 100644 openedx/core/djangoapps/user_authn/utils.py diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 654cfef21b..7b47e73cb2 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -15,7 +15,6 @@ from django.core.validators import ValidationError from django.contrib.auth import load_backend from django.contrib.auth.models import User from django.db import IntegrityError, transaction -from django.utils import http from django.utils.translation import ugettext as _ from pytz import UTC from six import iteritems, text_type @@ -37,6 +36,7 @@ from openedx.core.djangoapps.certificates.api import certificates_viewable_for_c from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming import helpers as theming_helpers from openedx.core.djangoapps.theming.helpers import get_themes +from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect from student.models import ( LinkedInAddToProfileConfiguration, PasswordHistory, @@ -326,7 +326,7 @@ def _get_redirect_to(request): # get information about a user on edx.org. In any such case drop the parameter. if redirect_to: mime_type, _ = mimetypes.guess_type(redirect_to, strict=False) - if not is_safe_redirect(request, redirect_to): + if not is_safe_login_or_logout_redirect(request, redirect_to): log.warning( u'Unsafe redirect parameter detected after login page: %(redirect_to)r', {"redirect_to": redirect_to} @@ -369,19 +369,6 @@ def _get_redirect_to(request): return redirect_to -def is_safe_redirect(request, redirect_to): - """ - Determine if the given redirect URL/path is safe for redirection. - """ - request_host = request.get_host() # e.g. 'courses.edx.org' - - login_redirect_whitelist = set(getattr(settings, 'LOGIN_REDIRECT_WHITELIST', [])) - login_redirect_whitelist.add(request_host) - - is_safe_url = http.is_safe_url(redirect_to, allowed_hosts=login_redirect_whitelist, require_https=True) - return is_safe_url - - def generate_activation_email_context(user, registration): """ Constructs a dictionary for use in activation email contexts diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index 5e8ea74228..f9a00cbdf1 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -12,7 +12,7 @@ from django.test.utils import override_settings from mock import patch from testfixtures import LogCapture -from student.helpers import get_next_url_for_login_page, is_safe_redirect +from student.helpers import get_next_url_for_login_page from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration_context LOGGER_NAME = "student.helpers" @@ -78,22 +78,6 @@ class TestLoginHelper(TestCase): next_page = get_next_url_for_login_page(req) self.assertEqual(next_page, next_url) - @ddt.data( - ('/dashboard', 'testserver', True), - ('https://edx.org/courses', 'edx.org', True), - ('https://test.edx.org/courses', 'edx.org', True), - ('https://www.amazon.org', 'edx.org', False), - ('http://edx.org/courses', 'edx.org', False), - ('http:///edx.org/courses', 'edx.org', False), # Django's is_safe_url protects against "///" - ) - @ddt.unpack - @override_settings(LOGIN_REDIRECT_WHITELIST=['test.edx.org']) - def test_safe_redirect(self, url, host, expected_is_safe): - """ Test safe next parameter """ - req = self.request.get(reverse("login"), HTTP_HOST=host) - actual_is_safe = is_safe_redirect(req, url) - self.assertEqual(actual_is_safe, expected_is_safe) - @patch('student.helpers.third_party_auth.pipeline.get') @ddt.data( # Test requests outside the TPA pipeline - tpa_hint should be added. diff --git a/openedx/core/djangoapps/user_authn/tests/test_utils.py b/openedx/core/djangoapps/user_authn/tests/test_utils.py new file mode 100644 index 0000000000..1cdf37ec52 --- /dev/null +++ b/openedx/core/djangoapps/user_authn/tests/test_utils.py @@ -0,0 +1,33 @@ +""" Test User Authentication utilities """ + +import ddt +from django.test import TestCase +from django.test.client import RequestFactory +from django.test.utils import override_settings + +from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect + + +@ddt.ddt +class TestRedirectUtils(TestCase): + """Test redirect utility methods.""" + + def setUp(self): + super(TestRedirectUtils, self).setUp() + self.request = RequestFactory() + + @ddt.data( + ('/dashboard', 'testserver', True), + ('https://edx.org/courses', 'edx.org', True), + ('https://test.edx.org/courses', 'edx.org', True), + ('https://www.amazon.org', 'edx.org', False), + ('http://edx.org/courses', 'edx.org', False), + ('http:///edx.org/courses', 'edx.org', False), # Django's is_safe_url protects against "///" + ) + @ddt.unpack + @override_settings(LOGIN_REDIRECT_WHITELIST=['test.edx.org']) + def test_safe_redirect(self, url, host, expected_is_safe): + """ Test safe next parameter """ + req = self.request.get('/login', HTTP_HOST=host) + actual_is_safe = is_safe_login_or_logout_redirect(req, url) + self.assertEqual(actual_is_safe, expected_is_safe) diff --git a/openedx/core/djangoapps/user_authn/utils.py b/openedx/core/djangoapps/user_authn/utils.py new file mode 100644 index 0000000000..14d0a806b2 --- /dev/null +++ b/openedx/core/djangoapps/user_authn/utils.py @@ -0,0 +1,18 @@ +""" +Utility functions used during user authentication. +""" +from django.conf import settings +from django.utils import http + + +def is_safe_login_or_logout_redirect(request, redirect_to): + """ + Determine if the given redirect URL/path is safe for redirection. + """ + request_host = request.get_host() # e.g. 'courses.edx.org' + + login_redirect_whitelist = set(getattr(settings, 'LOGIN_REDIRECT_WHITELIST', [])) + login_redirect_whitelist.add(request_host) + + is_safe_url = http.is_safe_url(redirect_to, allowed_hosts=login_redirect_whitelist, require_https=True) + return is_safe_url diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index 8e8c3fe3bf..29e6b4f801 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -10,7 +10,7 @@ from django.utils.http import urlencode from django.views.generic import TemplateView from provider.oauth2.models import Client from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies -from student.helpers import is_safe_redirect +from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect class LogoutView(TemplateView): @@ -35,7 +35,7 @@ class LogoutView(TemplateView): """ target_url = self.request.GET.get('redirect_url') - if target_url and is_safe_redirect(self.request, target_url): + if target_url and is_safe_login_or_logout_redirect(self.request, target_url): return target_url else: return self.default_target