Skip to content

Commit

Permalink
Merge pull request openedx#18989 from edx/robrap/ARCH-241-logout-redi…
Browse files Browse the repository at this point in the history
…rect

ARCH-241: Add ability to redirect to subdomain for logout.
  • Loading branch information
robrap authored Oct 3, 2018
2 parents d447c6c + 027c53e commit eca340d
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 106 deletions.
4 changes: 2 additions & 2 deletions common/djangoapps/student/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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_redirect(request, redirect_to):
log.warning(
u'Unsafe redirect parameter detected after login page: %(redirect_to)r',
{"redirect_to": redirect_to}
Expand Down Expand Up @@ -369,7 +369,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.
"""
Expand Down
35 changes: 25 additions & 10 deletions common/djangoapps/student/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@
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"
Expand Down Expand Up @@ -53,7 +52,7 @@ def _add_session(request):
"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))
Expand All @@ -65,19 +64,35 @@ def test_unsafe_next(self, log_level, log_name, unsafe_url, http_accept, user_ag
)

@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(
Expand Down
88 changes: 0 additions & 88 deletions common/djangoapps/student/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@

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
Expand Down Expand Up @@ -157,91 +154,6 @@ def test_course_run_refund_status_invalid_course_key(self):
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):
Expand Down
9 changes: 3 additions & 6 deletions openedx/core/djangoapps/user_authn/views/logout.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
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):
Expand All @@ -34,11 +35,7 @@ def target(self):
"""
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
Expand Down
116 changes: 116 additions & 0 deletions openedx/core/djangoapps/user_authn/views/tests/test_logout.py
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit eca340d

Please sign in to comment.