Address @brianhw review comments

* Fix open redirect vulnerability
* Add Logging To AUDIT_LOG : Note I had to change existing tests
  that mocked AUDIT_LOG with this
* Use external_auth.views.SHIBBOLETH_DOMAIN_PREFIX in student.views
* Add a bunch of documentation
* PEP8 / Pylint

common/djangoapps/external_auth/tests/test_helper.py

+"""
+Tests for utility functions in external_auth module
+"""
+from django.test import TestCase
+from external_auth.views import _safe_postlogin_redirect
+
+
+class ExternalAuthHelperFnTest(TestCase):
+    """
+    Unit tests for the external_auth.views helper function
+    """
+    def test__safe_postlogin_redirect(self):
+        """
+        Tests the _safe_postlogin_redirect function with different values of next
+        """
+        HOST = 'testserver'                               # pylint: disable=C0103
+        ONSITE1 = '/dashboard'                            # pylint: disable=C0103
+        ONSITE2 = '/courses/org/num/name/courseware'      # pylint: disable=C0103
+        ONSITE3 = 'http://{}/my/custom/url'.format(HOST)  # pylint: disable=C0103
+        OFFSITE1 = 'http://www.attacker.com'              # pylint: disable=C0103
+
+        for redirect_to in [ONSITE1, ONSITE2, ONSITE3]:
+            redir = _safe_postlogin_redirect(redirect_to, HOST)
+            self.assertEqual(redir.status_code, 302)
+            self.assertEqual(redir['location'], redirect_to)
+
+        redir2 = _safe_postlogin_redirect(OFFSITE1, HOST)
+        self.assertEqual(redir2.status_code, 302)
+        self.assertEqual("/", redir2['location'])

common/djangoapps/external_auth/tests/test_shib.py

-
 """
 Tests for Shibboleth Authentication
 @jbau
@@ -227,17 +226,17 @@ class ShibSPTest(ModuleStoreTestCase):
             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"'
+            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)
+                self.assertContains(response, fullname_input_html)
             else:
-                self.assertNotContains(response, fullname_input_HTML)
+                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.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_registration_formSubmit(self):
+    def test_registration_form_submit(self):
         """
         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
@@ -319,7 +318,7 @@ class ShibSPTest(ModuleStoreTestCase):
             user.delete()
 
     @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_course_specificLoginAndReg(self):
+    def test_course_specific_login_and_reg(self):
         """
         Tests that the correct course specific login and registration urls work for shib
         """

common/djangoapps/external_auth/views.py

@@ -23,7 +23,7 @@ if settings.MITX_FEATURES.get('AUTH_USE_CAS'):
 from student.models import UserProfile, TestCenterUser, TestCenterRegistration
 
 from django.http import HttpResponse, HttpResponseRedirect, HttpRequest, HttpResponseForbidden
-from django.utils.http import urlquote
+from django.utils.http import urlquote, is_safe_url
 from django.shortcuts import redirect
 from django.utils.translation import ugettext as _
 
@@ -159,7 +159,10 @@ def _external_login_or_signup(request,
     internal_user = eamap.user
     if internal_user is None:
         if uses_shibboleth:
-            # if we are using shib, try to link accounts using email
+            # If we are using shib, try to link accounts
+            # For Stanford shib, the email the idp returns is actually under the control of the user.
+            # Since the id the idps return is not user-editable, and is of the from "username@stanford.edu",
+            # use the id to link accounts instead.
             try:
                 link_user = User.objects.get(email=eamap.external_id)
                 if not ExternalAuthMap.objects.filter(user=link_user).exists():
@@ -451,10 +454,10 @@ def shib_login(request):
 
     fullname = shib['displayName'] if shib['displayName'] else u'%s %s' % (shib['givenName'], shib['sn'])
 
-    next = request.REQUEST.get('next')
+    redirect_to = request.REQUEST.get('next')
     retfun = None
-    if next:
-        retfun = functools.partial(redirect, next)
+    if redirect_to:
+        retfun = functools.partial(_safe_postlogin_redirect, redirect_to, request.get_host())
 
     return _external_login_or_signup(
         request,
@@ -467,6 +470,20 @@ def shib_login(request):
     )
 
 
+def _safe_postlogin_redirect(redirect_to, safehost, default_redirect='/'):
+    """
+    If redirect_to param is safe (not off this host), then perform the redirect.
+    Otherwise just redirect to '/'.
+    Basically copied from django.contrib.auth.views.login
+    @param redirect_to: user-supplied redirect url
+    @param safehost: which host is safe to redirect to
+    @return: an HttpResponseRedirect
+    """
+    if is_safe_url(url=redirect_to, host=safehost):
+        return redirect(redirect_to)
+    return redirect(default_redirect)
+
+
 def _make_shib_enrollment_request(request):
     """
         Need this hack function because shibboleth logins don't happen over POST
@@ -501,14 +518,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_querystring('signin_user', request.GET)
+        return _redirect_with_get_querydict('signin_user', request.GET)
 
     # now the dispatching conditionals.  Only shib for now
     if settings.MITX_FEATURES.get('AUTH_USE_SHIB') and course.enrollment_domain.startswith(SHIBBOLETH_DOMAIN_PREFIX):
-        return redirect_with_querystring('shib-login', request.GET)
+        return _redirect_with_get_querydict('shib-login', request.GET)
 
     # Default fallthrough to normal signin page
-    return redirect_with_querystring('signin_user', request.GET)
+    return _redirect_with_get_querydict('signin_user', request.GET)
 
 
 def course_specific_register(request, course_id):
@@ -520,23 +537,24 @@ 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_querystring('register_user', request.GET)
+        return _redirect_with_get_querydict('register_user', request.GET)
 
     # now the dispatching conditionals.  Only shib for now
     if settings.MITX_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_querystring('shib-login', request.GET)
+        return _redirect_with_get_querydict('shib-login', request.GET)
 
     # Default fallthrough to normal registration page
-    return redirect_with_querystring('register_user', request.GET)
+    return _redirect_with_get_querydict('register_user', request.GET)
 
 
-def redirect_with_querystring(view_name, querydict_get):
+def _redirect_with_get_querydict(view_name, get_querydict):
     """
         Helper function to carry over get parameters across redirects
+        Using urlencode(safe='/') because the @login_required decorator generates 'next' queryparams with '/' unencoded
     """
-    if querydict_get:
-        return redirect("%s?%s" % (reverse(view_name), querydict_get.urlencode(safe='/')))
+    if get_querydict:
+        return redirect("%s?%s" % (reverse(view_name), get_querydict.urlencode(safe='/')))
     return redirect(view_name)
 
 

common/djangoapps/student/tests/test_login.py

@@ -12,7 +12,7 @@ from django.conf import settings
 from django.core.cache import cache
 from django.core.urlresolvers import reverse, NoReverseMatch
 from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory
-from student.views import parse_course_id_from_string, get_course_enrollment_domain
+from student.views import _parse_course_id_from_string, _get_course_enrollment_domain
 
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config
@@ -166,27 +166,34 @@ class LoginTest(TestCase):
 
     def _assert_audit_log(self, mock_audit_log, level, log_strings):
         """
-        Check that the audit log has received the expected call.
+        Check that the audit log has received the expected call as its last call.
         """
         method_calls = mock_audit_log.method_calls
-        self.assertEquals(len(method_calls), 1)
-        name, args, _kwargs = method_calls[0]
+        name, args, _kwargs = method_calls[-1]
         self.assertEquals(name, level)
         self.assertEquals(len(args), 1)
         format_string = args[0]
         for log_string in log_strings:
             self.assertIn(log_string, format_string)
 
+
 class UtilFnTest(TestCase):
-    def test_parse_course_id_from_string(self):
-        COURSE_ID = u'org/num/run'
-        COURSE_URL = u'/courses/{}/otherstuff'.format(COURSE_ID)
-        NON_COURSE_URL = u'/blahblah'
-        self.assertEqual(parse_course_id_from_string(COURSE_URL), COURSE_ID)
-        self.assertIsNone(parse_course_id_from_string(NON_COURSE_URL))
+    """
+    Tests for utility functions in student.views
+    """
+    def test__parse_course_id_from_string(self):
+        """
+        Tests the _parse_course_id_from_string util function
+        """
+        COURSE_ID = u'org/num/run'                                # pylint: disable=C0103
+        COURSE_URL = u'/courses/{}/otherstuff'.format(COURSE_ID)  # pylint: disable=C0103
+        NON_COURSE_URL = u'/blahblah'                             # pylint: disable=C0103
+        self.assertEqual(_parse_course_id_from_string(COURSE_URL), COURSE_ID)
+        self.assertIsNone(_parse_course_id_from_string(NON_COURSE_URL))
+
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
-class ExternalAuthTest(ModuleStoreTestCase):
+class ExternalAuthShibTest(ModuleStoreTestCase):
     """
     Tests how login_user() interacts with ExternalAuth, in particular Shib
     """
@@ -215,18 +222,18 @@ class ExternalAuthTest(ModuleStoreTestCase):
         Tests that when a shib user types their email address into the login page, they get redirected
         to the shib login.
         """
-        response = self.client.post(reverse('login'), {'email':self.user_w_map.email, 'password':''})
+        response = self.client.post(reverse('login'), {'email': self.user_w_map.email, 'password': ''})
         self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.content, json.dumps({'success': False, 'redirect':reverse('shib-login')}))
+        self.assertEqual(response.content, json.dumps({'success': False, 'redirect': reverse('shib-login')}))
 
     @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
-    def test_get_course_enrollment_domain(self):
+    def test__get_course_enrollment_domain(self):
         """
-        Tests the get_course_enrollment_domain utility function
+        Tests the _get_course_enrollment_domain utility function
         """
-        self.assertIsNone(get_course_enrollment_domain("I/DONT/EXIST"))
-        self.assertIsNone(get_course_enrollment_domain(self.course.id))
-        self.assertEqual(self.shib_course.enrollment_domain, get_course_enrollment_domain(self.shib_course.id))
+        self.assertIsNone(_get_course_enrollment_domain("I/DONT/EXIST"))
+        self.assertIsNone(_get_course_enrollment_domain(self.course.id))
+        self.assertEqual(self.shib_course.enrollment_domain, _get_course_enrollment_domain(self.shib_course.id))
 
     @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
     def test_login_required_dashboard(self):
@@ -244,7 +251,7 @@ class ExternalAuthTest(ModuleStoreTestCase):
         Tests the redirects when visiting course-specific URL with @login_required.
         Should vary by course depending on its enrollment_domain
         """
-        TARGET_URL = reverse('courseware', args=[self.course.id])
+        TARGET_URL = reverse('courseware', args=[self.course.id])            # pylint: disable=C0103
         noshib_response = self.client.get(TARGET_URL, follow=True)
         self.assertEqual(noshib_response.redirect_chain[-1],
                          ('http://testserver/accounts/login?next={url}'.format(url=TARGET_URL), 302))
@@ -252,11 +259,11 @@ class ExternalAuthTest(ModuleStoreTestCase):
                                               .format(platform_name=settings.PLATFORM_NAME)))
         self.assertEqual(noshib_response.status_code, 200)
 
-        TARGET_URL_SHIB = reverse('courseware', args=[self.shib_course.id])
+        TARGET_URL_SHIB = reverse('courseware', args=[self.shib_course.id])  # pylint: disable=C0103
         shib_response = self.client.get(**{'path': TARGET_URL_SHIB,
                                            'follow': True,
-                                           'REMOTE_USER':self.extauth.external_id,
-                                           'Shib-Identity-Provider':'https://idp.stanford.edu/'})
+                                           'REMOTE_USER': self.extauth.external_id,
+                                           'Shib-Identity-Provider': 'https://idp.stanford.edu/'})
         # Test that the shib-login redirect page with ?next= and the desired page are part of the redirect chain
         # The 'courseware' page actually causes a redirect itself, so it's not the end of the chain and we
         # won't test its contents

common/djangoapps/student/views.py

@@ -25,7 +25,7 @@ from django.core.validators import validate_email, validate_slug, ValidationErro
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import IntegrityError, transaction
 from django.http import (HttpResponse, HttpResponseBadRequest, HttpResponseForbidden,
-                         HttpResponseNotAllowed, Http404, HttpResponseRedirect)
+                         HttpResponseNotAllowed, Http404)
 from django.shortcuts import redirect
 from django_future.csrf import ensure_csrf_cookie
 from django.utils.http import cookie_date
@@ -72,8 +72,6 @@ AUDIT_LOG = logging.getLogger("audit")
 
 Article = namedtuple('Article', 'title url author image deck publication publish_date')
 
-SHIB_DOMAIN_PREFIX = 'shib'
-
 
 def csrf_token(context):
     """A csrf token that can be included in a form."""
@@ -259,7 +257,7 @@ def register_user(request, extra_context=None):
     if extra_context is not None:
         context.update(extra_context)
 
-    if context.get("extauth_domain", '').startswith(SHIB_DOMAIN_PREFIX):
+    if context.get("extauth_domain", '').startswith(external_auth.views.SHIBBOLETH_DOMAIN_PREFIX):
         return render_to_response('register-shib.html', context)
     return render_to_response('register.html', context)
 
@@ -414,14 +412,24 @@ def change_enrollment(request):
         return HttpResponseBadRequest(_("Enrollment action is invalid"))
 
 
-def parse_course_id_from_string(input_str):
+def _parse_course_id_from_string(input_str):
+    """
+    Helper function to determine if input_str (typically the queryparam 'next') contains a course_id.
+    @param input_str:
+    @return: the course_id if found, None if not
+    """
     m_obj = re.match(r'^/courses/(?P<course_id>[^/]+/[^/]+/[^/]+)', input_str)
     if m_obj:
         return m_obj.group('course_id')
     return None
 
 
-def get_course_enrollment_domain(course_id):
+def _get_course_enrollment_domain(course_id):
+    """
+    Helper function to get the enrollment domain set for a course with id course_id
+    @param course_id:
+    @return:
+    """
     try:
         course = course_from_id(course_id)
         return course.enrollment_domain
@@ -435,10 +443,10 @@ def accounts_login(request):
         return redirect(reverse('cas-login'))
     # 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
-    next = request.GET.get('next')
-    if next:
-        course_id = parse_course_id_from_string(next)
-        if course_id and get_course_enrollment_domain(course_id):
+    redirect_to = request.GET.get('next')
+    if redirect_to:
+        course_id = _parse_course_id_from_string(redirect_to)
+        if course_id and _get_course_enrollment_domain(course_id):
             return external_auth.views.course_specific_login(request, course_id)
     return render_to_response('login.html')
 
@@ -465,11 +473,11 @@ def login_user(request, error=""):
     if settings.MITX_FEATURES.get('AUTH_USE_SHIB') and user:
         try:
             eamap = ExternalAuthMap.objects.get(user=user)
-            if eamap.external_domain.startswith(SHIB_DOMAIN_PREFIX):
+            if eamap.external_domain.startswith(external_auth.views.SHIBBOLETH_DOMAIN_PREFIX):
                 return HttpResponse(json.dumps({'success': False, 'redirect': reverse('shib-login')}))
         except ExternalAuthMap.DoesNotExist:
             # This is actually the common case, logging in user without external linked login
-            log.info("User %s w/o external auth attempting login", user)
+            AUDIT_LOG.info("User %s w/o external auth attempting login", user)
 
     # if the user doesn't exist, we want to set the username to an invalid
     # username so that authentication is guaranteed to fail and we can take
@@ -674,7 +682,8 @@ def create_account(request, post_override=None):
     # Can't have terms of service for certain SHIB users, like at Stanford
     tos_not_required = (settings.MITX_FEATURES.get("AUTH_USE_SHIB") and
                         settings.MITX_FEATURES.get('SHIB_DISABLE_TOS') and
-                        DoExternalAuth and eamap.external_domain.startswith(SHIB_DOMAIN_PREFIX))
+                        DoExternalAuth and
+                        eamap.external_domain.startswith(external_auth.views.SHIBBOLETH_DOMAIN_PREFIX))
 
     if not tos_not_required:
         if post_vars.get('terms_of_service', 'false') != u'true':

lms/envs/common.py

@@ -94,6 +94,8 @@ MITX_FEATURES = {
     'AUTH_USE_OPENID': False,
     'AUTH_USE_MIT_CERTIFICATES': False,
     'AUTH_USE_OPENID_PROVIDER': False,
+    # Even though external_auth is in common, shib assumes the LMS views / urls, so it should only be enabled
+    # in LMS
     'AUTH_USE_SHIB': False,
     'AUTH_USE_CAS': False,
 

lms/templates/login.html

@@ -53,9 +53,12 @@
           }  else {
             location.href="${reverse('dashboard')}";
           }
-        } else if(json.hasOwnProperty('redirect')){
-            var u=decodeURI(window.location.search);
-            location.href=json.redirect+u;
+        } else if(json.hasOwnProperty('redirect')) {
+          var u=decodeURI(window.location.search);
+          if (!isExternal(json.redirect)) { // a paranoid check.  Our server is the one providing json.redirect
+              location.href=json.redirect+u;
+          } // else we just remain on this page, which is fine since this particular path implies a login failure
+            // that has been generated via packet tampering (json.redirect has been messed with).
         } else {
           toggleSubmitButton(true);
           $('.message.submission-error').addClass('is-shown').focus();