From 62fcc6f23a307d5f7b61f058842b4642439ac490 Mon Sep 17 00:00:00 2001
From: David Baumgold <david@davidbaumgold.com>
Date: Fri, 5 Sep 2014 14:07:03 -0400
Subject: [PATCH 1/2] Use ddt for shib tests

---
 .../external_auth/tests/test_shib.py          | 284 +++++++++---------
 1 file changed, 143 insertions(+), 141 deletions(-)

diff --git a/common/djangoapps/external_auth/tests/test_shib.py b/common/djangoapps/external_auth/tests/test_shib.py
index 7880baef0c..780d4e1e19 100644
--- a/common/djangoapps/external_auth/tests/test_shib.py
+++ b/common/djangoapps/external_auth/tests/test_shib.py
@@ -5,6 +5,7 @@ Tests for Shibboleth Authentication
 """
 import unittest
 from mock import patch
+from ddt import ddt, data
 
 from django.conf import settings
 from django.http import HttpResponseRedirect
@@ -74,6 +75,7 @@ def gen_all_identities():
                     yield _build_identity_dict(mail, display_name, given_name, surname)
 
 
+@ddt
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE, SESSION_ENGINE='django.contrib.sessions.backends.cache')
 class ShibSPTest(ModuleStoreTestCase):
     """
@@ -271,37 +273,38 @@ class ShibSPTest(ModuleStoreTestCase):
         self._base_test_extauth_auto_activate_user_with_flag(log_user_string="user.id: 1")
 
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_registration_form(self):
+    @data(*gen_all_identities())
+    def test_registration_form(self, identity):
         """
         Tests the registration form showing up with the proper parameters.
 
         Uses django test client for its session support
         """
-        for identity in gen_all_identities():
-            client = DjangoTestClient()
-            # identity k/v pairs will show up in request.META
-            response = client.get(path='/shib-login/', data={}, follow=False, **identity)
+        client = DjangoTestClient()
+        # identity k/v pairs will show up in request.META
+        response = client.get(path='/shib-login/', data={}, follow=False, **identity)
 
-            self.assertEquals(response.status_code, 200)
-            mail_input_HTML = '<input class="" id="email" type="email" name="email"'
-            if not identity.get('mail'):
-                self.assertContains(response, mail_input_HTML)
-            else:
-                self.assertNotContains(response, mail_input_HTML)
-            sn_empty = not identity.get('sn')
-            given_name_empty = not identity.get('givenName')
-            displayname_empty = not identity.get('displayName')
-            fullname_input_html = '<input id="name" type="text" name="name"'
-            if sn_empty and given_name_empty and displayname_empty:
-                self.assertContains(response, fullname_input_html)
-            else:
-                self.assertNotContains(response, fullname_input_html)
+        self.assertEquals(response.status_code, 200)
+        mail_input_HTML = '<input class="" id="email" type="email" name="email"'
+        if not identity.get('mail'):
+            self.assertContains(response, mail_input_HTML)
+        else:
+            self.assertNotContains(response, mail_input_HTML)
+        sn_empty = not identity.get('sn')
+        given_name_empty = not identity.get('givenName')
+        displayname_empty = not identity.get('displayName')
+        fullname_input_html = '<input id="name" type="text" name="name"'
+        if sn_empty and given_name_empty and displayname_empty:
+            self.assertContains(response, fullname_input_html)
+        else:
+            self.assertNotContains(response, fullname_input_html)
 
-            # clean up b/c we don't want existing ExternalAuthMap for the next run
-            client.session['ExternalAuthMap'].delete()
+        # clean up b/c we don't want existing ExternalAuthMap for the next run
+        client.session['ExternalAuthMap'].delete()
 
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_registration_form_submit(self):
+    @data(*gen_all_identities())
+    def test_registration_form_submit(self, identity):
         """
         Tests user creation after the registration form that pops is submitted.  If there is no shib
         ExternalAuthMap in the session, then the created user should take the username and email from the
@@ -309,84 +312,84 @@ class ShibSPTest(ModuleStoreTestCase):
 
         Uses django test client for its session support
         """
-        for identity in gen_all_identities():
-            # First we pop the registration form
-            client = DjangoTestClient()
-            response1 = client.get(path='/shib-login/', data={}, follow=False, **identity)
-            # Then we have the user answer the registration form
-            # These are unicode because request.POST returns unicode
-            postvars = {'email': u'post_email@stanford.edu',
-                        'username': u'post_username',  # django usernames can't be unicode
-                        'password': u'post_pássword',
-                        'name': u'post_náme',
-                        'terms_of_service': u'true',
-                        'honor_code': u'true'}
-            # use RequestFactory instead of TestClient here because we want access to request.user
-            request2 = self.request_factory.post('/create_account', data=postvars)
-            request2.session = client.session
-            request2.user = AnonymousUser()
+        # First we pop the registration form
+        client = DjangoTestClient()
+        response1 = client.get(path='/shib-login/', data={}, follow=False, **identity)
+        # Then we have the user answer the registration form
+        # These are unicode because request.POST returns unicode
+        postvars = {'email': u'post_email@stanford.edu',
+                    'username': u'post_username',  # django usernames can't be unicode
+                    'password': u'post_pássword',
+                    'name': u'post_náme',
+                    'terms_of_service': u'true',
+                    'honor_code': u'true'}
+        # use RequestFactory instead of TestClient here because we want access to request.user
+        request2 = self.request_factory.post('/create_account', data=postvars)
+        request2.session = client.session
+        request2.user = AnonymousUser()
 
-            mako_middleware_process_request(request2)
-            with patch('student.views.AUDIT_LOG') as mock_audit_log:
-                _response2 = create_account(request2)
+        mako_middleware_process_request(request2)
+        with patch('student.views.AUDIT_LOG') as mock_audit_log:
+            _response2 = create_account(request2)
 
-            user = request2.user
-            mail = identity.get('mail')
+        user = request2.user
+        mail = identity.get('mail')
 
-            # verify logging of login happening during account creation:
-            audit_log_calls = mock_audit_log.method_calls
-            self.assertEquals(len(audit_log_calls), 3)
-            method_name, args, _kwargs = audit_log_calls[0]
-            self.assertEquals(method_name, 'info')
-            self.assertEquals(len(args), 1)
-            self.assertIn(u'Login success on new account creation', args[0])
-            self.assertIn(u'post_username', args[0])
-            method_name, args, _kwargs = audit_log_calls[1]
-            self.assertEquals(method_name, 'info')
-            self.assertEquals(len(args), 2)
-            self.assertIn(u'User registered with external_auth', args[0])
-            self.assertEquals(u'post_username', args[1])
-            method_name, args, _kwargs = audit_log_calls[2]
-            self.assertEquals(method_name, 'info')
-            self.assertEquals(len(args), 3)
-            self.assertIn(u'Updated ExternalAuthMap for ', args[0])
-            self.assertEquals(u'post_username', args[1])
-            self.assertEquals(u'test_user@stanford.edu', args[2].external_id)
+        # verify logging of login happening during account creation:
+        audit_log_calls = mock_audit_log.method_calls
+        self.assertEquals(len(audit_log_calls), 3)
+        method_name, args, _kwargs = audit_log_calls[0]
+        self.assertEquals(method_name, 'info')
+        self.assertEquals(len(args), 1)
+        self.assertIn(u'Login success on new account creation', args[0])
+        self.assertIn(u'post_username', args[0])
+        method_name, args, _kwargs = audit_log_calls[1]
+        self.assertEquals(method_name, 'info')
+        self.assertEquals(len(args), 2)
+        self.assertIn(u'User registered with external_auth', args[0])
+        self.assertEquals(u'post_username', args[1])
+        method_name, args, _kwargs = audit_log_calls[2]
+        self.assertEquals(method_name, 'info')
+        self.assertEquals(len(args), 3)
+        self.assertIn(u'Updated ExternalAuthMap for ', args[0])
+        self.assertEquals(u'post_username', args[1])
+        self.assertEquals(u'test_user@stanford.edu', args[2].external_id)
 
-            # check that the created user has the right email, either taken from shib or user input
-            if mail:
-                self.assertEqual(user.email, mail)
-                self.assertEqual(list(User.objects.filter(email=postvars['email'])), [])
-                self.assertIsNotNone(User.objects.get(email=mail))  # get enforces only 1 such user
-            else:
-                self.assertEqual(user.email, postvars['email'])
-                self.assertEqual(list(User.objects.filter(email=mail)), [])
-                self.assertIsNotNone(User.objects.get(email=postvars['email']))  # get enforces only 1 such user
+        # check that the created user has the right email, either taken from shib or user input
+        if mail:
+            self.assertEqual(user.email, mail)
+            self.assertEqual(list(User.objects.filter(email=postvars['email'])), [])
+            self.assertIsNotNone(User.objects.get(email=mail))  # get enforces only 1 such user
+        else:
+            self.assertEqual(user.email, postvars['email'])
+            self.assertEqual(list(User.objects.filter(email=mail)), [])
+            self.assertIsNotNone(User.objects.get(email=postvars['email']))  # get enforces only 1 such user
 
-            # check that the created user profile has the right name, either taken from shib or user input
-            profile = UserProfile.objects.get(user=user)
-            sn_empty = not identity.get('sn')
-            given_name_empty = not identity.get('givenName')
-            displayname_empty = not identity.get('displayName')
+        # check that the created user profile has the right name, either taken from shib or user input
+        profile = UserProfile.objects.get(user=user)
+        sn_empty = not identity.get('sn')
+        given_name_empty = not identity.get('givenName')
+        displayname_empty = not identity.get('displayName')
 
-            if displayname_empty:
-                if sn_empty and given_name_empty:
-                    self.assertEqual(profile.name, postvars['name'])
-                else:
-                    self.assertEqual(profile.name, request2.session['ExternalAuthMap'].external_name)
-                    self.assertNotIn(u';', profile.name)
+        if displayname_empty:
+            if sn_empty and given_name_empty:
+                self.assertEqual(profile.name, postvars['name'])
             else:
                 self.assertEqual(profile.name, request2.session['ExternalAuthMap'].external_name)
-                self.assertEqual(profile.name, identity.get('displayName').decode('utf-8'))
+                self.assertNotIn(u';', profile.name)
+        else:
+            self.assertEqual(profile.name, request2.session['ExternalAuthMap'].external_name)
+            self.assertEqual(profile.name, identity.get('displayName').decode('utf-8'))
 
-            # clean up for next loop
-            request2.session['ExternalAuthMap'].delete()
-            UserProfile.objects.filter(user=user).delete()
-            Registration.objects.filter(user=user).delete()
-            user.delete()
+        # clean up for next loop
+        request2.session['ExternalAuthMap'].delete()
+        UserProfile.objects.filter(user=user).delete()
+        Registration.objects.filter(user=user).delete()
+        user.delete()
 
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_course_specific_login_and_reg(self):
+    @data("", "shib:https://idp.stanford.edu/")
+    def test_course_specific_login_and_reg(self, domain):
         """
         Tests that the correct course specific login and registration urls work for shib
         """
@@ -398,71 +401,70 @@ class ShibSPTest(ModuleStoreTestCase):
         )
 
         # Test for cases where course is found
-        for domain in ["", "shib:https://idp.stanford.edu/"]:
-            # set domains
+        # set domains
 
-            # temporarily set the branch to draft-preferred so we can update the course
-            with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course.id):
-                course.enrollment_domain = domain
-                self.store.update_item(course, self.test_user_id)
+        # temporarily set the branch to draft-preferred so we can update the course
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course.id):
+            course.enrollment_domain = domain
+            self.store.update_item(course, self.test_user_id)
 
-            # setting location to test that GET params get passed through
-            login_request = self.request_factory.get('/course_specific_login/MITx/999/Robot_Super_Course' +
-                                                     '?course_id=MITx/999/Robot_Super_Course' +
-                                                     '&enrollment_action=enroll')
-            _reg_request = self.request_factory.get('/course_specific_register/MITx/999/Robot_Super_Course' +
-                                                    '?course_id=MITx/999/course/Robot_Super_Course' +
-                                                    '&enrollment_action=enroll')
+        # setting location to test that GET params get passed through
+        login_request = self.request_factory.get('/course_specific_login/MITx/999/Robot_Super_Course' +
+                                                 '?course_id=MITx/999/Robot_Super_Course' +
+                                                 '&enrollment_action=enroll')
+        _reg_request = self.request_factory.get('/course_specific_register/MITx/999/Robot_Super_Course' +
+                                                '?course_id=MITx/999/course/Robot_Super_Course' +
+                                                '&enrollment_action=enroll')
 
-            login_response = course_specific_login(login_request, 'MITx/999/Robot_Super_Course')
-            reg_response = course_specific_register(login_request, 'MITx/999/Robot_Super_Course')
-
-            if "shib" in domain:
-                self.assertIsInstance(login_response, HttpResponseRedirect)
-                self.assertEqual(login_response['Location'],
-                                 reverse('shib-login') +
-                                 '?course_id=MITx/999/Robot_Super_Course' +
-                                 '&enrollment_action=enroll')
-                self.assertIsInstance(login_response, HttpResponseRedirect)
-                self.assertEqual(reg_response['Location'],
-                                 reverse('shib-login') +
-                                 '?course_id=MITx/999/Robot_Super_Course' +
-                                 '&enrollment_action=enroll')
-            else:
-                self.assertIsInstance(login_response, HttpResponseRedirect)
-                self.assertEqual(login_response['Location'],
-                                 reverse('signin_user') +
-                                 '?course_id=MITx/999/Robot_Super_Course' +
-                                 '&enrollment_action=enroll')
-                self.assertIsInstance(login_response, HttpResponseRedirect)
-                self.assertEqual(reg_response['Location'],
-                                 reverse('register_user') +
-                                 '?course_id=MITx/999/Robot_Super_Course' +
-                                 '&enrollment_action=enroll')
-
-            # Now test for non-existent course
-            # setting location to test that GET params get passed through
-            login_request = self.request_factory.get('/course_specific_login/DNE/DNE/DNE' +
-                                                     '?course_id=DNE/DNE/DNE' +
-                                                     '&enrollment_action=enroll')
-            _reg_request = self.request_factory.get('/course_specific_register/DNE/DNE/DNE' +
-                                                    '?course_id=DNE/DNE/DNE/Robot_Super_Course' +
-                                                    '&enrollment_action=enroll')
-
-            login_response = course_specific_login(login_request, 'DNE/DNE/DNE')
-            reg_response = course_specific_register(login_request, 'DNE/DNE/DNE')
+        login_response = course_specific_login(login_request, 'MITx/999/Robot_Super_Course')
+        reg_response = course_specific_register(login_request, 'MITx/999/Robot_Super_Course')
 
+        if "shib" in domain:
+            self.assertIsInstance(login_response, HttpResponseRedirect)
+            self.assertEqual(login_response['Location'],
+                             reverse('shib-login') +
+                             '?course_id=MITx/999/Robot_Super_Course' +
+                             '&enrollment_action=enroll')
+            self.assertIsInstance(login_response, HttpResponseRedirect)
+            self.assertEqual(reg_response['Location'],
+                             reverse('shib-login') +
+                             '?course_id=MITx/999/Robot_Super_Course' +
+                             '&enrollment_action=enroll')
+        else:
             self.assertIsInstance(login_response, HttpResponseRedirect)
             self.assertEqual(login_response['Location'],
                              reverse('signin_user') +
-                             '?course_id=DNE/DNE/DNE' +
+                             '?course_id=MITx/999/Robot_Super_Course' +
                              '&enrollment_action=enroll')
             self.assertIsInstance(login_response, HttpResponseRedirect)
             self.assertEqual(reg_response['Location'],
                              reverse('register_user') +
-                             '?course_id=DNE/DNE/DNE' +
+                             '?course_id=MITx/999/Robot_Super_Course' +
                              '&enrollment_action=enroll')
 
+        # Now test for non-existent course
+        # setting location to test that GET params get passed through
+        login_request = self.request_factory.get('/course_specific_login/DNE/DNE/DNE' +
+                                                 '?course_id=DNE/DNE/DNE' +
+                                                 '&enrollment_action=enroll')
+        _reg_request = self.request_factory.get('/course_specific_register/DNE/DNE/DNE' +
+                                                '?course_id=DNE/DNE/DNE/Robot_Super_Course' +
+                                                '&enrollment_action=enroll')
+
+        login_response = course_specific_login(login_request, 'DNE/DNE/DNE')
+        reg_response = course_specific_register(login_request, 'DNE/DNE/DNE')
+
+        self.assertIsInstance(login_response, HttpResponseRedirect)
+        self.assertEqual(login_response['Location'],
+                         reverse('signin_user') +
+                         '?course_id=DNE/DNE/DNE' +
+                         '&enrollment_action=enroll')
+        self.assertIsInstance(login_response, HttpResponseRedirect)
+        self.assertEqual(reg_response['Location'],
+                         reverse('register_user') +
+                         '?course_id=DNE/DNE/DNE' +
+                         '&enrollment_action=enroll')
+
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
     def test_enrollment_limit_by_domain(self):
         """

From 9c81004fc7cdf546d5a488e0ae97fcbccc464df0 Mon Sep 17 00:00:00 2001
From: David Baumgold <david@davidbaumgold.com>
Date: Wed, 17 Sep 2014 10:40:30 -0400
Subject: [PATCH 2/2] Remove test cleanups

Since `ddt` should make them unnecessary
---
 common/djangoapps/external_auth/tests/test_shib.py | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/common/djangoapps/external_auth/tests/test_shib.py b/common/djangoapps/external_auth/tests/test_shib.py
index 780d4e1e19..3b9668aedc 100644
--- a/common/djangoapps/external_auth/tests/test_shib.py
+++ b/common/djangoapps/external_auth/tests/test_shib.py
@@ -299,9 +299,6 @@ class ShibSPTest(ModuleStoreTestCase):
         else:
             self.assertNotContains(response, fullname_input_html)
 
-        # clean up b/c we don't want existing ExternalAuthMap for the next run
-        client.session['ExternalAuthMap'].delete()
-
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
     @data(*gen_all_identities())
     def test_registration_form_submit(self, identity):
@@ -381,12 +378,6 @@ class ShibSPTest(ModuleStoreTestCase):
             self.assertEqual(profile.name, request2.session['ExternalAuthMap'].external_name)
             self.assertEqual(profile.name, identity.get('displayName').decode('utf-8'))
 
-        # clean up for next loop
-        request2.session['ExternalAuthMap'].delete()
-        UserProfile.objects.filter(user=user).delete()
-        Registration.objects.filter(user=user).delete()
-        user.delete()
-
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
     @data("", "shib:https://idp.stanford.edu/")
     def test_course_specific_login_and_reg(self, domain):
@@ -532,8 +523,6 @@ class ShibSPTest(ModuleStoreTestCase):
                 if course is open_enroll_course or student is shib_student:
                     self.assertEqual(response.status_code, 200)
                     self.assertTrue(CourseEnrollment.is_enrolled(student, course.id))
-                    # Clean up
-                    CourseEnrollment.unenroll(student, course.id)
                 else:
                     self.assertEqual(response.status_code, 400)
                     self.assertFalse(CourseEnrollment.is_enrolled(student, course.id))