Added full test coverage for ssl login decorator

I also removed conflicting implementation of SSL_AUTH_IMMEDIATE_SIGNUP
and rebased on the current master.

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)

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)