From 027c53e61e1291ce723edfe7a0182a949b1010e2 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Fri, 21 Sep 2018 17:00:46 -0400 Subject: [PATCH] Add ability to redirect to subdomain for logout. ARCH-241 --- common/djangoapps/student/helpers.py | 4 +- .../djangoapps/student/tests/test_helpers.py | 35 ++++-- common/djangoapps/student/tests/test_views.py | 88 ------------- .../djangoapps/user_authn/views/logout.py | 9 +- .../user_authn/views/tests/test_logout.py | 116 ++++++++++++++++++ 5 files changed, 146 insertions(+), 106 deletions(-) create mode 100644 openedx/core/djangoapps/user_authn/views/tests/test_logout.py diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index d9cbc86f11..e99b60d65a 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -325,7 +325,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_redirect(request, redirect_to): log.warning( u'Unsafe redirect parameter detected after login page: %(redirect_to)r', {"redirect_to": redirect_to} @@ -368,7 +368,7 @@ def _get_redirect_to(request): return redirect_to -def _is_safe_redirect(request, redirect_to): +def is_safe_redirect(request, redirect_to): """ Determine if the given redirect URL/path is safe for redirection. """ diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index ff6b8c216f..5e8ea74228 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -9,11 +9,10 @@ from django.urls import reverse from django.test import TestCase from django.test.client import RequestFactory from django.test.utils import override_settings -from django.utils import http from mock import patch from testfixtures import LogCapture -from student.helpers import get_next_url_for_login_page +from student.helpers import get_next_url_for_login_page, is_safe_redirect from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration_context LOGGER_NAME = "student.helpers" @@ -53,7 +52,7 @@ class TestLoginHelper(TestCase): "Redirect to url path with specified filed type 'image/png' not allowed: u'" + static_url + "dummy.png" + "'"), ) @ddt.unpack - def test_unsafe_next(self, log_level, log_name, unsafe_url, http_accept, user_agent, expected_log): + def test_next_failures(self, log_level, log_name, unsafe_url, http_accept, user_agent, expected_log): """ Test unsafe next parameter """ with LogCapture(LOGGER_NAME, level=log_level) as logger: req = self.request.get(reverse("login") + "?next={url}".format(url=unsafe_url)) @@ -65,19 +64,35 @@ class TestLoginHelper(TestCase): ) @ddt.data( - ('/dashboard', 'testserver', '/dashboard'), - ('https://edx.org/courses', 'edx.org', 'https://edx.org/courses'), - ('https://test.edx.org/courses', 'edx.org', 'https://test.edx.org/courses'), - ('https://test2.edx.org/courses', 'edx.org', 'https://test2.edx.org/courses'), + ('/dashboard', 'testserver'), + ('https://edx.org/courses', 'edx.org'), + ('https://test.edx.org/courses', 'edx.org'), + ('https://test2.edx.org/courses', 'edx.org'), ) @ddt.unpack @override_settings(LOGIN_REDIRECT_WHITELIST=['test.edx.org', 'test2.edx.org']) - def test_safe_next(self, url, host, expected_url): + def test_safe_next(self, next_url, host): """ Test safe next parameter """ - req = self.request.get(reverse("login") + "?next={url}".format(url=url), HTTP_HOST=host) + req = self.request.get(reverse("login") + "?next={url}".format(url=next_url), HTTP_HOST=host) req.META["HTTP_ACCEPT"] = "text/html" # pylint: disable=no-member next_page = get_next_url_for_login_page(req) - self.assertEqual(next_page, expected_url) + 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( diff --git a/common/djangoapps/student/tests/test_views.py b/common/djangoapps/student/tests/test_views.py index c22230654a..3cb9a1e6a2 100644 --- a/common/djangoapps/student/tests/test_views.py +++ b/common/djangoapps/student/tests/test_views.py @@ -19,9 +19,6 @@ from opaque_keys import InvalidKeyError from bulk_email.models import BulkEmailFlag from course_modes.models import CourseMode -from edx_oauth2_provider.constants import AUTHORIZED_CLIENTS_SESSION_KEY -from edx_oauth2_provider.tests.factories import (ClientFactory, - TrustedClientFactory) from entitlements.tests.factories import CourseEntitlementFactory from milestones.tests.utils import MilestonesTestCaseMixin from openedx.core.djangoapps.catalog.tests.factories import ProgramFactory @@ -157,91 +154,6 @@ class TestStudentDashboardUnenrollments(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 406) -@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') -class LogoutTests(TestCase): - """ Tests for the logout functionality. """ - - def setUp(self): - """ Create a course and user, then log in. """ - super(LogoutTests, self).setUp() - self.user = UserFactory() - self.client.login(username=self.user.username, password=PASSWORD) - - def create_oauth_client(self): - """ Creates a trusted OAuth client. """ - client = ClientFactory(logout_uri='https://www.example.com/logout/') - TrustedClientFactory(client=client) - return client - - def assert_session_logged_out(self, oauth_client, **logout_headers): - """ Authenticates a user via OAuth 2.0, logs out, and verifies the session is logged out. """ - self.authenticate_with_oauth(oauth_client) - - # Logging out should remove the session variables, and send a list of logout URLs to the template. - # The template will handle loading those URLs and redirecting the user. That functionality is not tested here. - response = self.client.get(reverse('logout'), **logout_headers) - self.assertEqual(response.status_code, 200) - self.assertNotIn(AUTHORIZED_CLIENTS_SESSION_KEY, self.client.session) - - return response - - def authenticate_with_oauth(self, oauth_client): - """ Perform an OAuth authentication using the current web client. - - This should add an AUTHORIZED_CLIENTS_SESSION_KEY entry to the current session. - """ - data = { - 'client_id': oauth_client.client_id, - 'client_secret': oauth_client.client_secret, - 'response_type': 'code' - } - # Authenticate with OAuth to set the appropriate session values - self.client.post(reverse('oauth2:capture'), data, follow=True) - self.assertListEqual(self.client.session[AUTHORIZED_CLIENTS_SESSION_KEY], [oauth_client.client_id]) - - def assert_logout_redirects_to_root(self): - """ Verify logging out redirects the user to the homepage. """ - response = self.client.get(reverse('logout')) - self.assertRedirects(response, '/', fetch_redirect_response=False) - - def assert_logout_redirects_with_target(self): - """ Verify logging out with a redirect_url query param redirects the user to the target. """ - url = '{}?{}'.format(reverse('logout'), 'redirect_url=/courses') - response = self.client.get(url) - self.assertRedirects(response, '/courses', fetch_redirect_response=False) - - def test_without_session_value(self): - """ Verify logout works even if the session does not contain an entry with - the authenticated OpenID Connect clients.""" - self.assert_logout_redirects_to_root() - self.assert_logout_redirects_with_target() - - def test_client_logout(self): - """ Verify the context includes a list of the logout URIs of the authenticated OpenID Connect clients. - - The list should only include URIs of the clients for which the user has been authenticated. - """ - client = self.create_oauth_client() - response = self.assert_session_logged_out(client) - expected = { - 'logout_uris': [client.logout_uri + '?no_redirect=1'], # pylint: disable=no-member - 'target': '/', - } - self.assertDictContainsSubset(expected, response.context_data) # pylint: disable=no-member - - def test_filter_referring_service(self): - """ Verify that, if the user is directed to the logout page from a service, that service's logout URL - is not included in the context sent to the template. - """ - client = self.create_oauth_client() - response = self.assert_session_logged_out(client, HTTP_REFERER=client.logout_uri) # pylint: disable=no-member - expected = { - 'logout_uris': [], - 'target': '/', - } - self.assertDictContainsSubset(expected, response.context_data) # pylint: disable=no-member - - @ddt.ddt @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin, CompletionWaffleTestMixin): diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index dc7d311a59..8e8c3fe3bf 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -6,10 +6,11 @@ from django.conf import settings from django.contrib.auth import logout from django.urls import reverse_lazy from django.shortcuts import redirect -from django.utils.http import is_safe_url, urlencode +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 class LogoutView(TemplateView): @@ -34,11 +35,7 @@ class LogoutView(TemplateView): """ target_url = self.request.GET.get('redirect_url') - if target_url and is_safe_url( - target_url, - allowed_hosts={self.request.META.get('HTTP_HOST')}, - require_https=True, - ): + if target_url and is_safe_redirect(self.request, target_url): return target_url else: return self.default_target diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py new file mode 100644 index 0000000000..16eff1eb4f --- /dev/null +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py @@ -0,0 +1,116 @@ +""" +Tests for logout +""" +import unittest + +import ddt +from django.conf import settings +from django.test import TestCase +from django.test.utils import override_settings +from django.urls import reverse +from edx_oauth2_provider.constants import AUTHORIZED_CLIENTS_SESSION_KEY +from edx_oauth2_provider.tests.factories import ( + ClientFactory, + TrustedClientFactory +) +from student.tests.factories import UserFactory + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +@ddt.ddt +class LogoutTests(TestCase): + """ Tests for the logout functionality. """ + + def setUp(self): + """ Create a course and user, then log in. """ + super(LogoutTests, self).setUp() + self.user = UserFactory() + self.client.login(username=self.user.username, password='test') + + def _create_oauth_client(self): + """ Creates a trusted OAuth client. """ + client = ClientFactory(logout_uri='https://www.example.com/logout/') + TrustedClientFactory(client=client) + return client + + def _assert_session_logged_out(self, oauth_client, **logout_headers): + """ Authenticates a user via OAuth 2.0, logs out, and verifies the session is logged out. """ + self._authenticate_with_oauth(oauth_client) + + # Logging out should remove the session variables, and send a list of logout URLs to the template. + # The template will handle loading those URLs and redirecting the user. That functionality is not tested here. + response = self.client.get(reverse('logout'), **logout_headers) + self.assertEqual(response.status_code, 200) + self.assertNotIn(AUTHORIZED_CLIENTS_SESSION_KEY, self.client.session) + + return response + + def _authenticate_with_oauth(self, oauth_client): + """ Perform an OAuth authentication using the current web client. + + This should add an AUTHORIZED_CLIENTS_SESSION_KEY entry to the current session. + """ + data = { + 'client_id': oauth_client.client_id, + 'client_secret': oauth_client.client_secret, + 'response_type': 'code' + } + # Authenticate with OAuth to set the appropriate session values + self.client.post(reverse('oauth2:capture'), data, follow=True) + self.assertListEqual(self.client.session[AUTHORIZED_CLIENTS_SESSION_KEY], [oauth_client.client_id]) + + @ddt.data( + ('/courses', 'testserver'), + ('https://edx.org/courses', 'edx.org'), + ('https://test.edx.org/courses', 'edx.org'), + ) + @ddt.unpack + @override_settings(LOGIN_REDIRECT_WHITELIST=['test.edx.org']) + def test_logout_redirect_success(self, redirect_url, host): + url = '{logout_path}?redirect_url={redirect_url}'.format( + logout_path=reverse('logout'), + redirect_url=redirect_url + ) + response = self.client.get(url, HTTP_HOST=host) + self.assertRedirects(response, redirect_url, fetch_redirect_response=False) + + def test_no_redirect_supplied(self): + response = self.client.get(reverse('logout'), HTTP_HOST='testserver') + self.assertRedirects(response, '/', fetch_redirect_response=False) + + @ddt.data( + ('https://www.amazon.org', 'edx.org'), + ) + @ddt.unpack + def test_logout_redirect_failure(self, redirect_url, host): + url = '{logout_path}?redirect_url={redirect_url}'.format( + logout_path=reverse('logout'), + redirect_url=redirect_url + ) + response = self.client.get(url, HTTP_HOST=host) + self.assertRedirects(response, '/', fetch_redirect_response=False) + + def test_client_logout(self): + """ Verify the context includes a list of the logout URIs of the authenticated OpenID Connect clients. + + The list should only include URIs of the clients for which the user has been authenticated. + """ + client = self._create_oauth_client() + response = self._assert_session_logged_out(client) + expected = { + 'logout_uris': [client.logout_uri + '?no_redirect=1'], + 'target': '/', + } + self.assertDictContainsSubset(expected, response.context_data) + + def test_filter_referring_service(self): + """ Verify that, if the user is directed to the logout page from a service, that service's logout URL + is not included in the context sent to the template. + """ + client = self._create_oauth_client() + response = self._assert_session_logged_out(client, HTTP_REFERER=client.logout_uri) + expected = { + 'logout_uris': [], + 'target': '/', + } + self.assertDictContainsSubset(expected, response.context_data)