From 5452de20d8cf6acf09ebfa346fae2dcd959cde03 Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Fri, 28 Feb 2014 09:31:36 -0500 Subject: [PATCH 1/2] Modified ssl certificate authentication to handle next redirection Makes small changes in lms and cms both so that user's go to the original page they intended to if they weren't already logged in --- cms/djangoapps/contentstore/views/public.py | 14 ++-- .../external_auth/tests/test_ssl.py | 82 ++++++++++++++++++- common/djangoapps/external_auth/views.py | 25 ++++-- common/djangoapps/student/views.py | 8 +- 4 files changed, 109 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/views/public.py b/cms/djangoapps/contentstore/views/public.py index da805a7bd7..2a80057de3 100644 --- a/cms/djangoapps/contentstore/views/public.py +++ b/cms/djangoapps/contentstore/views/public.py @@ -9,8 +9,9 @@ from django.conf import settings from edxmako.shortcuts import render_to_response -from external_auth.views import ssl_login_shortcut, ssl_get_cert_from_request -from microsite_configuration import microsite +from external_auth.views import (ssl_login_shortcut, ssl_get_cert_from_request, + redirect_with_get) +from microsite_configuration.middleware import MicrositeConfiguration __all__ = ['signup', 'login_page', 'howitworks'] @@ -26,7 +27,7 @@ def signup(request): if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): # Redirect to course to login to process their certificate if SSL is enabled # and registration is disabled. - return redirect(reverse('login')) + return redirect_with_get('login', request.GET, False) return render_to_response('register.html', {'csrf': csrf_token}) @@ -43,11 +44,14 @@ def login_page(request): # SSL login doesn't require a login view, so redirect # to course now that the user is authenticated via # the decorator. - return redirect('/course') + next_url = request.GET.get('next') + if next_url: + return redirect(next_url) + else: + return redirect('/course') if settings.FEATURES.get('AUTH_USE_CAS'): # If CAS is enabled, redirect auth handling to there return redirect(reverse('cas-login')) - return render_to_response( 'login.html', { diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index cd9940bf7e..974e81bda2 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -21,19 +21,29 @@ from mock import Mock from edxmako.middleware import MakoMiddleware from external_auth.models import ExternalAuthMap import external_auth.views +from student.roles import CourseStaffRole from student.tests.factories import UserFactory +from student.models import CourseEnrollment +from xmodule.modulestore import Location +from xmodule.modulestore.django import loc_mapper from xmodule.modulestore.exceptions import InsufficientSpecificationError +from xmodule.modulestore.tests.django_utils import (ModuleStoreTestCase, + mixed_store_config) +from xmodule.modulestore.tests.factories import CourseFactory FEATURES_WITH_SSL_AUTH = settings.FEATURES.copy() FEATURES_WITH_SSL_AUTH['AUTH_USE_CERTIFICATES'] = True FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP = FEATURES_WITH_SSL_AUTH.copy() FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP['AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'] = True +FEATURES_WITH_SSL_AUTH_AUTO_ACTIVATE = FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP.copy() +FEATURES_WITH_SSL_AUTH_AUTO_ACTIVATE['BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'] = True FEATURES_WITHOUT_SSL_AUTH = settings.FEATURES.copy() FEATURES_WITHOUT_SSL_AUTH['AUTH_USE_CERTIFICATES'] = False +TEST_DATA_MIXED_MODULESTORE = mixed_store_config(settings.COMMON_TEST_DATA_ROOT, {}) @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH) -class SSLClientTest(TestCase): +class SSLClientTest(ModuleStoreTestCase): """ Tests SSL Authentication code sections of external_auth """ @@ -170,7 +180,8 @@ class SSLClientTest(TestCase): response = self.client.get( reverse('dashboard'), follow=True, SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL)) - self.assertIn(reverse('dashboard'), response['location']) + self.assertEquals(('http://testserver/dashboard', 302), + response.redirect_chain[-1]) self.assertIn(SESSION_KEY, self.client.session) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @@ -316,3 +327,70 @@ class SSLClientTest(TestCase): self.assertEqual(1, len(ExternalAuthMap.objects.all())) self.assertTrue(self.mock.called) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH_AUTO_ACTIVATE, + MODULESTORE=TEST_DATA_MIXED_MODULESTORE) + def test_ssl_lms_redirection(self): + """ + Auto signup auth user and ensure they return to the original + url they visited after being logged in. + """ + course = CourseFactory.create( + org='MITx', + number='999', + display_name='Robot Super Course' + ) + + external_auth.views.ssl_login(self._create_ssl_request('/')) + user = User.objects.get(email=self.USER_EMAIL) + CourseEnrollment.enroll(user, course.id) + course_private_url = '/courses/MITx/999/Robot_Super_Course/courseware' + + self.assertFalse(SESSION_KEY in self.client.session) + + response = self.client.get( + course_private_url, + follow=True, + SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL), + HTTP_ACCEPT='text/html' + ) + self.assertEqual(('http://testserver{0}'.format(course_private_url), 302), + response.redirect_chain[-1]) + self.assertIn(SESSION_KEY, self.client.session) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'cms.urls', 'Test only valid in cms') + @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH_AUTO_ACTIVATE) + def test_ssl_cms_redirection(self): + """ + Auto signup auth user and ensure they return to the original + url they visited after being logged in. + """ + course = CourseFactory.create( + org='MITx', + number='999', + display_name='Robot Super Course' + ) + + external_auth.views.ssl_login(self._create_ssl_request('/')) + user = User.objects.get(email=self.USER_EMAIL) + CourseEnrollment.enroll(user, course.id) + + CourseStaffRole(course.location).add_users(user) + location = Location(['i4x', 'MITx', '999', 'course', + Location.clean('Robot Super Course'), None]) + new_location = loc_mapper().translate_location( + location.course_id, location, True, True + ) + course_private_url = new_location.url_reverse('course/', '') + self.assertFalse(SESSION_KEY in self.client.session) + + response = self.client.get( + course_private_url, + follow=True, + SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL), + HTTP_ACCEPT='text/html' + ) + self.assertEqual(('http://testserver{0}'.format(course_private_url), 302), + response.redirect_chain[-1]) + self.assertIn(SESSION_KEY, self.client.session) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 0f3baf7b92..f7258f1c89 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -440,7 +440,10 @@ def ssl_login(request): (_user, email, fullname) = _ssl_dn_extract_info(cert) - retfun = functools.partial(redirect, '/') + redirect_to = request.GET.get('next') + if not redirect_to: + redirect_to = '/' + retfun = functools.partial(redirect, redirect_to) return _external_login_or_signup( request, external_id=email, @@ -580,14 +583,14 @@ def course_specific_login(request, course_id): course = course_from_id(course_id) except ItemNotFoundError: # couldn't find the course, will just return vanilla signin page - return _redirect_with_get_querydict('signin_user', request.GET) + return redirect_with_get('signin_user', request.GET) # now the dispatching conditionals. Only shib for now if settings.FEATURES.get('AUTH_USE_SHIB') and course.enrollment_domain.startswith(SHIBBOLETH_DOMAIN_PREFIX): - return _redirect_with_get_querydict('shib-login', request.GET) + return redirect_with_get('shib-login', request.GET) # Default fallthrough to normal signin page - return _redirect_with_get_querydict('signin_user', request.GET) + return redirect_with_get('signin_user', request.GET) def course_specific_register(request, course_id): @@ -599,24 +602,28 @@ def course_specific_register(request, course_id): course = course_from_id(course_id) except ItemNotFoundError: # couldn't find the course, will just return vanilla registration page - return _redirect_with_get_querydict('register_user', request.GET) + return redirect_with_get('register_user', request.GET) # now the dispatching conditionals. Only shib for now if settings.FEATURES.get('AUTH_USE_SHIB') and course.enrollment_domain.startswith(SHIBBOLETH_DOMAIN_PREFIX): # shib-login takes care of both registration and login flows - return _redirect_with_get_querydict('shib-login', request.GET) + return redirect_with_get('shib-login', request.GET) # Default fallthrough to normal registration page - return _redirect_with_get_querydict('register_user', request.GET) + return redirect_with_get('register_user', request.GET) -def _redirect_with_get_querydict(view_name, get_querydict): +def redirect_with_get(view_name, get_querydict, do_reverse=True): """ Helper function to carry over get parameters across redirects Using urlencode(safe='/') because the @login_required decorator generates 'next' queryparams with '/' unencoded """ + if do_reverse: + url = reverse(view_name) + else: + url = view_name if get_querydict: - return redirect("%s?%s" % (reverse(view_name), get_querydict.urlencode(safe='/'))) + return redirect("%s?%s" % (url, get_querydict.urlencode(safe='/'))) return redirect(view_name) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5fab7b96fd..8f76f7b8c1 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -328,7 +328,7 @@ def signin_user(request): # SSL login doesn't require a view, so redirect # branding and allow that to process the login if it # is enabled and the header is in the request. - return redirect(reverse('root')) + return external_auth.views.redirect_with_get('root', request.GET) if settings.FEATURES.get('AUTH_USE_CAS'): # If CAS is enabled, redirect auth handling to there return redirect(reverse('cas-login')) @@ -361,7 +361,7 @@ def register_user(request, extra_context=None): if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): # Redirect to branding to process their certificate if SSL is enabled # and registration is disabled. - return redirect(reverse('root')) + return external_auth.views.redirect_with_get('root', request.GET) context = { 'course_id': request.GET.get('course_id'), @@ -686,7 +686,7 @@ def accounts_login(request): if settings.FEATURES['AUTH_USE_CERTIFICATES']: # SSL login doesn't require a view, so redirect # to branding and allow that to process the login. - return redirect(reverse('root')) + return external_auth.views.redirect_with_get('root', request.GET) # see if the "next" parameter has been set, whether it has a course context, and if so, whether # there is a course-specific place to redirect redirect_to = request.GET.get('next') @@ -776,7 +776,7 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un # This is actually the common case, logging in user without external linked login AUDIT_LOG.info("User %s w/o external auth attempting login", user) - # see if account has been locked out due to excessive login failures + # see if account has been locked out due to excessive login failres user_found_by_email_lookup = user if user_found_by_email_lookup and LoginFailures.is_feature_enabled(): if LoginFailures.is_user_locked_out(user_found_by_email_lookup): From cbf525f6cf5af44f11f48f86134a7c6d49c3a744 Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Fri, 28 Feb 2014 11:55:40 -0500 Subject: [PATCH 2/2] Fix infinite redirect loop on logout caused by django caching --- cms/djangoapps/contentstore/views/public.py | 3 +- .../external_auth/tests/test_ssl.py | 30 +++++++++++++++++-- common/djangoapps/student/views.py | 12 ++++---- lms/djangoapps/branding/views.py | 6 ++++ 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/views/public.py b/cms/djangoapps/contentstore/views/public.py index 2a80057de3..3d37818a80 100644 --- a/cms/djangoapps/contentstore/views/public.py +++ b/cms/djangoapps/contentstore/views/public.py @@ -11,7 +11,7 @@ from edxmako.shortcuts import render_to_response from external_auth.views import (ssl_login_shortcut, ssl_get_cert_from_request, redirect_with_get) -from microsite_configuration.middleware import MicrositeConfiguration +from microsite_configuration import microsite __all__ = ['signup', 'login_page', 'howitworks'] @@ -52,6 +52,7 @@ def login_page(request): if settings.FEATURES.get('AUTH_USE_CAS'): # If CAS is enabled, redirect auth handling to there return redirect(reverse('cas-login')) + return render_to_response( 'login.html', { diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index 974e81bda2..21a497d011 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -194,7 +194,8 @@ class SSLClientTest(ModuleStoreTestCase): response = self.client.get( reverse('register_user'), follow=True, SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL)) - self.assertIn(reverse('dashboard'), response['location']) + self.assertEquals(('http://testserver/dashboard', 302), + response.redirect_chain[-1]) self.assertIn(SESSION_KEY, self.client.session) @unittest.skipUnless(settings.ROOT_URLCONF == 'cms.urls', 'Test only valid in cms') @@ -236,7 +237,8 @@ class SSLClientTest(ModuleStoreTestCase): response = self.client.get( reverse('signin_user'), follow=True, SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL)) - self.assertIn(reverse('dashboard'), response['location']) + self.assertEquals(('http://testserver/dashboard', 302), + response.redirect_chain[-1]) self.assertIn(SESSION_KEY, self.client.session) @@ -394,3 +396,27 @@ class SSLClientTest(ModuleStoreTestCase): self.assertEqual(('http://testserver{0}'.format(course_private_url), 302), response.redirect_chain[-1]) self.assertIn(SESSION_KEY, self.client.session) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH_AUTO_ACTIVATE) + def test_ssl_logout(self): + """ + Because the branding view is cached for anonymous users and we + use that to login users, the browser wasn't actually making the + request to that view as the redirect was being cached. This caused + a redirect loop, and this test confirms that that won't happen. + + Test is only in LMS because we don't use / in studio to login SSL users. + """ + response = self.client.get( + reverse('dashboard'), follow=True, + SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL)) + self.assertEquals(('http://testserver/dashboard', 302), + response.redirect_chain[-1]) + self.assertIn(SESSION_KEY, self.client.session) + response = self.client.get( + reverse('logout'), follow=True, + SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL) + ) + # Make sure that even though we logged out, we have logged back in + self.assertIn(SESSION_KEY, self.client.session) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 8f76f7b8c1..5cecaff5df 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -26,6 +26,7 @@ from django.shortcuts import redirect from django_future.csrf import ensure_csrf_cookie from django.utils.http import cookie_date, base36_to_int from django.utils.translation import ugettext as _, get_language +from django.views.decorators.cache import never_cache from django.views.decorators.http import require_POST, require_GET from django.template.response import TemplateResponse @@ -328,7 +329,7 @@ def signin_user(request): # SSL login doesn't require a view, so redirect # branding and allow that to process the login if it # is enabled and the header is in the request. - return external_auth.views.redirect_with_get('root', request.GET) + return external_auth.views.redirect_with_get('root', request.GET) if settings.FEATURES.get('AUTH_USE_CAS'): # If CAS is enabled, redirect auth handling to there return redirect(reverse('cas-login')) @@ -675,6 +676,7 @@ def _get_course_enrollment_domain(course_id): return None +@never_cache @ensure_csrf_cookie def accounts_login(request): """ @@ -684,9 +686,9 @@ def accounts_login(request): if settings.FEATURES.get('AUTH_USE_CAS'): return redirect(reverse('cas-login')) if settings.FEATURES['AUTH_USE_CERTIFICATES']: - # SSL login doesn't require a view, so redirect - # to branding and allow that to process the login. - return external_auth.views.redirect_with_get('root', request.GET) + # SSL login doesn't require a view, so login + # directly here + return external_auth.views.ssl_login(request) # see if the "next" parameter has been set, whether it has a course context, and if so, whether # there is a course-specific place to redirect redirect_to = request.GET.get('next') @@ -776,7 +778,7 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un # This is actually the common case, logging in user without external linked login AUDIT_LOG.info("User %s w/o external auth attempting login", user) - # see if account has been locked out due to excessive login failres + # see if account has been locked out due to excessive login failures user_found_by_email_lookup = user if user_found_by_email_lookup and LoginFailures.is_feature_enabled(): if LoginFailures.is_user_locked_out(user_found_by_email_lookup): diff --git a/lms/djangoapps/branding/views.py b/lms/djangoapps/branding/views.py index 06939e04cc..b0549ce61b 100644 --- a/lms/djangoapps/branding/views.py +++ b/lms/djangoapps/branding/views.py @@ -25,6 +25,12 @@ def index(request): if settings.FEATURES.get('AUTH_USE_CERTIFICATES'): from external_auth.views import ssl_login + # Set next URL to dashboard if it isn't set to avoid + # caching a redirect to / that causes a redirect loop on logout + if not request.GET.get('next'): + req_new = request.GET.copy() + req_new['next'] = reverse('dashboard') + request.GET = req_new return ssl_login(request) enable_mktg_site = microsite.get_value(