Merge pull request #1862 from carsongee/bugfix/mitx/external_auth-retfun

Fix external_auth to properly use retfun for @ssl_login_shortcut()

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,93 @@ 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()
+        # 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()))
+
+    @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)

common/djangoapps/external_auth/views.py

@@ -177,10 +177,10 @@ 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)
+            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,7 @@ def _flatten_to_ascii(txt):
 
 
 @ensure_csrf_cookie
-def _signup(request, eamap):
+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 +246,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
@@ -260,7 +263,11 @@ def _signup(request, eamap):
                          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
@@ -349,13 +356,27 @@ 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
+            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():
+            """Wrap function again for call by _external_login_or_signup"""
+            return fn(*args, **kwargs)
+
         (_user, email, fullname) = _ssl_dn_extract_info(cert)
         return _external_login_or_signup(
             request,
@@ -363,7 +384,8 @@ def ssl_login_shortcut(fn):
             external_domain="ssl:MIT",
             credentials=cert,
             email=email,
-            fullname=fullname
+            fullname=fullname,
+            retfun=retfun
         )
     return wrapped
 
@@ -397,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,