From e868759ceb82d3f1cc51fbb596d51c0b3f3acc25 Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 12 Aug 2013 20:41:58 +0000 Subject: [PATCH 1/6] fix external_auth @ssl_login_shortcut decorator to properly use retfun --- common/djangoapps/external_auth/views.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 42d0f2bf89..94ab224f24 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -180,7 +180,7 @@ def _external_login_or_signup(request, return _signup(request, eamap) else: log.info('No user for %s yet. doing signup', eamap.external_email) - return _signup(request, eamap) + return _signup(request, eamap, retfun) # We trust shib's authentication, so no need to authenticate using the password again uname = internal_user.username @@ -198,7 +198,7 @@ def _external_login_or_signup(request, if user is None: # we want to log the failure, but don't want to log the password attempted: AUDIT_LOG.warning('External Auth Login failed for "%s"', uname) - return _signup(request, eamap) + return _signup(request, eamap, retfun) if not user.is_active: AUDIT_LOG.warning('User "%s" is not active after external login', uname) @@ -237,7 +237,8 @@ def _flatten_to_ascii(txt): @ensure_csrf_cookie -def _signup(request, eamap): +@cache_if_anonymous +def _signup(request, eamap, retfun=None): """ Present form to complete for signup via external authentication. Even though the user has external credentials, he/she still needs @@ -246,6 +247,9 @@ def _signup(request, eamap): eamap is an ExternalAuthMap object, specifying the external user for which to complete the signup. + + retfun is a function to execute for the return value, if immediate + signup is used. That allows @ssl_login_shortcut() to work. """ # save this for use by student.views.create_account request.session['ExternalAuthMap'] = eamap @@ -352,10 +356,17 @@ def ssl_login_shortcut(fn): if not settings.FEATURES['AUTH_USE_MIT_CERTIFICATES']: return fn(*args, **kwargs) request = args[0] + + if request.user and request.user.is_authenticated(): # don't re-authenticate + return fn(*args, **kwargs) + cert = _ssl_get_cert_from_request(request) if not cert: # no certificate information - show normal login window return fn(*args, **kwargs) + def retfun(): + return fn(*args, **kwargs) + (_user, email, fullname) = _ssl_dn_extract_info(cert) return _external_login_or_signup( request, @@ -363,7 +374,8 @@ def ssl_login_shortcut(fn): external_domain="ssl:MIT", credentials=cert, email=email, - fullname=fullname + fullname=fullname, + retfun=retfun ) return wrapped From 246fc030bd84c47ac4b25cb361b3d94994187150 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 4 Sep 2013 23:01:36 -0400 Subject: [PATCH 2/6] add missing retfun --- common/djangoapps/external_auth/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 94ab224f24..7272b2eead 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -177,7 +177,7 @@ def _external_login_or_signup(request, return default_render_failure(request, failure_msg) except User.DoesNotExist: log.info('SHIB: No user for %s yet, doing signup', eamap.external_email) - return _signup(request, eamap) + return _signup(request, eamap, retfun) else: log.info('No user for %s yet. doing signup', eamap.external_email) return _signup(request, eamap, retfun) From 07e235a0c46300af36095215067c02173e0cf926 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 8 Oct 2013 10:01:12 -0400 Subject: [PATCH 3/6] remove @cache_if_anonymous --- common/djangoapps/external_auth/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 7272b2eead..6ce5d9655a 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -237,7 +237,6 @@ def _flatten_to_ascii(txt): @ensure_csrf_cookie -@cache_if_anonymous def _signup(request, eamap, retfun=None): """ Present form to complete for signup via external authentication. From caf44c36853459ede5ad32c07a71111273c58eea Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Thu, 5 Dec 2013 13:27:07 -0500 Subject: [PATCH 4/6] Added full test coverage for ssl login decorator I also removed conflicting implementation of SSL_AUTH_IMMEDIATE_SIGNUP and rebased on the current master. --- .../external_auth/tests/test_ssl.py | 107 ++++++++++++++++++ common/djangoapps/external_auth/views.py | 17 ++- 2 files changed, 121 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index d72fb00b28..43c42edfa3 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -3,6 +3,8 @@ Provides unit tests for SSL based authentication portions of the external_auth app. """ +import logging +import StringIO import unittest from django.conf import settings @@ -13,14 +15,19 @@ from django.test import TestCase from django.test.client import Client from django.test.client import RequestFactory from django.test.utils import override_settings +from mock import Mock +from edxmako.middleware import MakoMiddleware from external_auth.models import ExternalAuthMap import external_auth.views +from student.tests.factories import UserFactory FEATURES_WITH_SSL_AUTH = settings.FEATURES.copy() FEATURES_WITH_SSL_AUTH['AUTH_USE_MIT_CERTIFICATES'] = True FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP = FEATURES_WITH_SSL_AUTH.copy() FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP['AUTH_USE_MIT_CERTIFICATES_IMMEDIATE_SIGNUP'] = True +FEATURES_WITHOUT_SSL_AUTH = settings.FEATURES.copy() +FEATURES_WITHOUT_SSL_AUTH['AUTH_USE_MIT_CERTIFICATES'] = False @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH) @@ -32,6 +39,7 @@ class SSLClientTest(TestCase): AUTH_DN = '/C=US/ST=Massachusetts/O=Massachusetts Institute of Technology/OU=Client CA v1/CN={0}/emailAddress={1}' USER_NAME = 'test_user_ssl' USER_EMAIL = 'test_user_ssl@EDX.ORG' + MOCK_URL = '/' def _create_ssl_request(self, url): """Creates a basic request for SSL use.""" @@ -41,6 +49,17 @@ class SSLClientTest(TestCase): middleware = SessionMiddleware() middleware.process_request(request) request.session.save() + MakoMiddleware().process_request(request) + return request + + def _create_normal_request(self, url): + """Creates sessioned request without SSL headers""" + request = self.factory.get(url) + request.user = AnonymousUser() + middleware = SessionMiddleware() + middleware.process_request(request) + request.session.save() + MakoMiddleware().process_request(request) return request def setUp(self): @@ -48,6 +67,7 @@ class SSLClientTest(TestCase): super(SSLClientTest, self).setUp() self.client = Client() self.factory = RequestFactory() + self.mock = Mock() @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') def test_ssl_login_with_signup_lms(self): @@ -184,3 +204,90 @@ class SSLClientTest(TestCase): SSL_CLIENT_S_DN=self.AUTH_DN.format(self.USER_NAME, self.USER_EMAIL)) self.assertIn(reverse('dashboard'), response['location']) self.assertIn('_auth_user_id', self.client.session) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP) + def test_ssl_bad_eamap(self): + """ + This tests the response when a user exists but their eamap + password doesn't match their internal password. + + This should start failing and can be removed when the + eamap.internal_password dependency is removed. + """ + external_auth.views.ssl_login(self._create_ssl_request('/')) + user = User.objects.get(email=self.USER_EMAIL) + user.set_password('not autogenerated') + user.save() + + # Validate user failed by checking log + output = StringIO.StringIO() + audit_log_handler = logging.StreamHandler(output) + audit_log = logging.getLogger("audit") + audit_log.addHandler(audit_log_handler) + + request = self._create_ssl_request('/') + external_auth.views.ssl_login(request) + self.assertIn('External Auth Login failed for', output.getvalue()) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @override_settings(FEATURES=FEATURES_WITHOUT_SSL_AUTH) + def test_ssl_decorator_no_certs(self): + """Make sure no external auth happens without SSL enabled""" + + dec_mock = external_auth.views.ssl_login_shortcut(self.mock) + request = self._create_normal_request(self.MOCK_URL) + request.user = AnonymousUser() + dec_mock(request) + self.assertTrue(self.mock.called) + self.assertEqual(0, len(ExternalAuthMap.objects.all())) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + def test_ssl_login_decorator(self): + """Create mock function to test ssl login decorator""" + + dec_mock = external_auth.views.ssl_login_shortcut(self.mock) + + # Test that anonymous without cert doesn't create authmap + request = self._create_normal_request(self.MOCK_URL) + dec_mock(request) + self.assertTrue(self.mock.called) + self.assertEqual(0, len(ExternalAuthMap.objects.all())) + + # Test valid user + self.mock.reset_mock() + request = self._create_ssl_request(self.MOCK_URL) + dec_mock(request) + self.assertFalse(self.mock.called) + self.assertEqual(1, len(ExternalAuthMap.objects.all())) + + # Test logged in user gets called + self.mock.reset_mock() + request = self._create_ssl_request(self.MOCK_URL) + request.user = UserFactory() + dec_mock(request) + self.assertTrue(self.mock.called) + + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @override_settings(FEATURES=FEATURES_WITH_SSL_AUTH_IMMEDIATE_SIGNUP) + def test_ssl_decorator_auto_signup(self): + """ + Test that with auto signup the decorator + will bypass registration and call retfun. + """ + + dec_mock = external_auth.views.ssl_login_shortcut(self.mock) + request = self._create_ssl_request(self.MOCK_URL) + dec_mock(request) + # Assert our user exists in both eamap and Users + try: + ExternalAuthMap.objects.get(external_id=self.USER_EMAIL) + except ExternalAuthMap.DoesNotExist, ex: + self.fail('User did not get properly added to external auth map, exception was {0}'.format(str(ex))) + try: + User.objects.get(email=self.USER_EMAIL) + except ExternalAuthMap.DoesNotExist, ex: + self.fail('User did not get properly added to internal users, exception was {0}'.format(str(ex))) + self.assertEqual(1, len(ExternalAuthMap.objects.all())) + + self.assertTrue(self.mock.called) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 6ce5d9655a..c38b01650d 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -246,7 +246,7 @@ def _signup(request, eamap, retfun=None): eamap is an ExternalAuthMap object, specifying the external user for which to complete the signup. - + retfun is a function to execute for the return value, if immediate signup is used. That allows @ssl_login_shortcut() to work. """ @@ -263,7 +263,11 @@ def _signup(request, eamap, retfun=None): terms_of_service=u'true') log.info('doing immediate signup for %s, params=%s', username, post_vars) student.views.create_account(request, post_vars) - return redirect('/') + # should check return content for successful completion before + if retfun is not None: + return retfun() + else: + return redirect('/') # default conjoin name, no spaces, flattened to ascii b/c django can't handle unicode usernames, sadly # but this only affects username, not fullname @@ -352,11 +356,17 @@ def ssl_login_shortcut(fn): based on existing ExternalAuth record and MIT ssl certificate. """ def wrapped(*args, **kwargs): + """ + This manages the function wrapping, by determining whether to inject + the _external signup or just continuing to the internal function + call. + """ + if not settings.FEATURES['AUTH_USE_MIT_CERTIFICATES']: return fn(*args, **kwargs) request = args[0] - if request.user and request.user.is_authenticated(): # don't re-authenticate + if request.user and request.user.is_authenticated(): # don't re-authenticate return fn(*args, **kwargs) cert = _ssl_get_cert_from_request(request) @@ -364,6 +374,7 @@ def ssl_login_shortcut(fn): return fn(*args, **kwargs) def retfun(): + """Wrap function again for call by _external_login_or_signup""" return fn(*args, **kwargs) (_user, email, fullname) = _ssl_dn_extract_info(cert) From 50e2e833a2415e396b0bcaea3eea52a18f4b686c Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Tue, 10 Dec 2013 18:23:54 -0500 Subject: [PATCH 5/6] Added comment as requested --- common/djangoapps/external_auth/tests/test_ssl.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index 43c42edfa3..266938fdf0 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -238,6 +238,9 @@ class SSLClientTest(TestCase): dec_mock = external_auth.views.ssl_login_shortcut(self.mock) request = self._create_normal_request(self.MOCK_URL) request.user = AnonymousUser() + # Call decorated mock function to make sure it passes + # the call through without hitting the external_auth functions and + # thereby creating an external auth map object. dec_mock(request) self.assertTrue(self.mock.called) self.assertEqual(0, len(ExternalAuthMap.objects.all())) From 93b0357978bee2806362cad0d56ea93549a99c3d Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Thu, 12 Dec 2013 10:49:20 -0500 Subject: [PATCH 6/6] Replaced retfun in ssl_login so that it properly redirect to dashboard --- common/djangoapps/external_auth/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index c38b01650d..efbf4ccb63 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -419,7 +419,7 @@ def ssl_login(request): (_user, email, fullname) = _ssl_dn_extract_info(cert) - retfun = functools.partial(student.views.index, request) + retfun = functools.partial(redirect, '/') return _external_login_or_signup( request, external_id=email,