Merge pull request #2775 from edx/dcs/wiki-no-middleware

Refactored course_nav middleware to isolate the django-wiki hack, remove unnecessary code, and clarify what it's doing.

lms/djangoapps/course_wiki/course_nav.py

-import re
-from urlparse import urlparse
-
-from django.http import Http404
-from django.shortcuts import redirect
-from django.conf import settings
-from django.core.urlresolvers import reverse
-from django.core.exceptions import PermissionDenied
-
-from wiki.models import reverse as wiki_reverse
-from courseware.access import has_access
-from courseware.courses import get_course_with_access
-from student.models import CourseEnrollment
-
-
-IN_COURSE_WIKI_REGEX = r'/courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/wiki/(?P<wiki_path>.*|)$'
-
-IN_COURSE_WIKI_COMPILED_REGEX = re.compile(IN_COURSE_WIKI_REGEX)
-WIKI_ROOT_ACCESS_COMPILED_REGEX = re.compile(r'^/wiki/(?P<wiki_path>.*|)$')
-
-class Middleware(object):
-    """
-    This middleware is to keep the course nav bar above the wiki while
-    the student clicks around to other wiki pages.
-    If it intercepts a request for /wiki/.. that has a referrer in the
-    form /courses/course_id/... it will redirect the user to the page
-    /courses/course_id/wiki/...
-
-    It is also possible that someone followed a link leading to a course
-    that they don't have access to. In this case, we redirect them to the
-    same page on the regular wiki.
-
-    If we return a redirect, this middleware makes sure that the redirect
-    keeps the student in the course.
-
-    Finally, if the student is in the course viewing a wiki, we change the
-    reverse() function to resolve wiki urls as a course wiki url by setting
-    the _transform_url attribute on wiki.models.reverse.
-
-    Forgive me Father, for I have hacked.
-    """
-
-    def __init__(self):
-        self.redirected = False
-
-    def process_request(self, request):
-        self.redirected = False
-        wiki_reverse._transform_url = lambda url: url
-
-        referer = request.META.get('HTTP_REFERER')
-        destination = request.path
-
-        # Check to see if we don't allow top-level access to the wiki via the /wiki/xxxx/yyy/zzz URLs
-        # this will help prevent people from writing pell-mell to the Wiki in an unstructured way
-        path_match = WIKI_ROOT_ACCESS_COMPILED_REGEX.match(destination)
-        if path_match and not settings.FEATURES.get('ALLOW_WIKI_ROOT_ACCESS', False):
-            raise PermissionDenied()
-
-        if request.method == 'GET':
-            new_destination = self.get_redirected_url(request.user, referer, destination)
-
-            if new_destination != destination:
-                # We mark that we generated this redirection, so we don't modify it again
-                self.redirected = True
-                return redirect(new_destination)
-
-        course_match = IN_COURSE_WIKI_COMPILED_REGEX.match(destination)
-        if course_match:
-            course_id = course_match.group('course_id')
-
-            # Authorization Check
-            # Let's see if user is enrolled or the course allows for public access
-            course = get_course_with_access(request.user, course_id, 'load')
-            if not course.allow_public_wiki_access:
-                # if a user is not authenticated, redirect them to login
-                if not request.user.is_authenticated():
-                    return redirect(reverse('accounts_login'))
-
-                is_enrolled = CourseEnrollment.is_enrolled(request.user, course.id)
-                is_staff = has_access(request.user, course, 'staff')
-                if not (is_enrolled or is_staff):
-                    # if a user is logged in, but not authorized to see a page,
-                    # we'll redirect them to the course about page
-                    return redirect(reverse('about_course', args=[course_id]))
-
-            prepend_string = '/courses/' + course_id
-            wiki_reverse._transform_url = lambda url: prepend_string + url
-
-        return None
-
-
-    def process_response(self, request, response):
-        """
-        If this is a redirect response going to /wiki/*, then we might need
-        to change it to be a redirect going to /courses/*/wiki*.
-        """
-        if not self.redirected and response.status_code == 302:   # This is a redirect
-            referer = request.META.get('HTTP_REFERER')
-            destination_url = response['LOCATION']
-            destination = urlparse(destination_url).path
-
-            new_destination = self.get_redirected_url(request.user, referer, destination)
-
-            if new_destination != destination:
-                new_url = destination_url.replace(destination, new_destination)
-                response['LOCATION'] = new_url
-
-        return response
-
-
-    def get_redirected_url(self, user, referer, destination):
-        """
-        Returns None if the destination shouldn't be changed.
-        """
-        if not referer:
-            return destination
-        referer_path = urlparse(referer).path
-
-        path_match = re.match(r'^/wiki/(?P<wiki_path>.*|)$', destination)
-        if path_match:
-            # We are going to the wiki. Check if we came from a course
-            course_match = re.match(r'/courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/.*', referer_path)
-            if course_match:
-                course_id = course_match.group('course_id')
-
-                # See if we are able to view the course. If we are, redirect to it
-                try:
-                    course = get_course_with_access(user, course_id, 'load')
-                    return "/courses/" + course.id + "/wiki/" + path_match.group('wiki_path')
-                except Http404:
-                    # Even though we came from the course, we can't see it. So don't worry about it.
-                    pass
-
-        else:
-            # It is also possible we are going to a course wiki view, but we
-            # don't have permission to see the course!
-            course_match = re.match(IN_COURSE_WIKI_REGEX, destination)
-            if course_match:
-                course_id = course_match.group('course_id')
-                # See if we are able to view the course. If we aren't, redirect to regular wiki
-                try:
-                    course = get_course_with_access(user, course_id, 'load')
-                    # Good, we can see the course. Carry on
-                    return destination
-                except Http404:
-                    # We can't see the course, so redirect to the regular wiki
-                    return "/wiki/" + course_match.group('wiki_path')
-
-        return destination
-
-
-def context_processor(request):
-    """
-    This is a context processor which looks at the URL while we are
-    in the wiki. If the url is in the form
-    /courses/(course_id)/wiki/...
-    then we add 'course' to the context. This allows the course nav
-    bar to be shown.
-    """
-
-    match = re.match(IN_COURSE_WIKI_REGEX, request.path)
-    if match:
-        course_id = match.group('course_id')
-
-        try:
-            course = get_course_with_access(request.user, course_id, 'load')
-            staff_access = has_access(request.user, course, 'staff')
-            return {'course': course,
-                    'staff_access': staff_access}
-        except Http404:
-            # We couldn't access the course for whatever reason. It is too late to change
-            # the URL here, so we just leave the course context. The middleware shouldn't
-            # let this happen
-            pass
-
-    return {}

lms/djangoapps/course_wiki/editors.py

@@ -20,7 +20,8 @@ class CodeMirrorWidget(forms.Widget):
         super(CodeMirrorWidget, self).__init__(default_attrs)
 
     def render(self, name, value, attrs=None):
-        if value is None: value = ''
+        if value is None:
+            value = ''
 
         final_attrs = self.build_attrs(attrs, name=name)
 

lms/djangoapps/course_wiki/middleware.py

+"""Middleware for course_wiki"""
+from urlparse import urlparse
+from django.conf import settings
+from django.http import Http404
+from django.shortcuts import redirect
+from django.core.exceptions import PermissionDenied
+from wiki.models import reverse
+
+from courseware.courses import get_course_with_access
+from courseware.access import has_access
+from student.models import CourseEnrollment
+from util.request import course_id_from_url
+
+
+class WikiAccessMiddleware(object):
+    """
+    This middleware wraps calls to django-wiki in order to handle authentication and redirection
+    between the root wiki and the course wikis.
+
+    TODO: removing the "root wiki" would obviate the need for this middleware; it could be replaced
+          with a wrapper function around the wiki views. This is currently difficult or impossible to do
+          because there are two sets of wiki urls loaded in urls.py
+    """
+    def _redirect_from_referrer(self, request, wiki_path):
+        """
+        redirect to course wiki url if the referrer is from a course page
+        """
+        course_id = course_id_from_url(request.META.get('HTTP_REFERER'))
+        if course_id:
+            # See if we are able to view the course. If we are, redirect to it
+            try:
+                course = get_course_with_access(request.user, course_id, 'load')
+                return redirect("/courses/{course_id}/wiki/{path}".format(course_id=course.id, path=wiki_path))
+            except Http404:
+                # Even though we came from the course, we can't see it. So don't worry about it.
+                pass
+
+    def process_view(self, request, view_func, view_args, view_kwargs):  # pylint: disable=W0613
+        """
+        This function handles authentication logic for wiki urls and redirects from
+        the "root wiki" to the "course wiki" if the user accesses the wiki from a course url
+        """
+        # we care only about requests to wiki urls
+        if not view_func.__module__.startswith('wiki.'):
+            return
+
+        course_id = course_id_from_url(request.path)
+        wiki_path = request.path.split('/wiki/', 1)[1]
+
+        # wiki pages are login required
+        if not request.user.is_authenticated():
+            return redirect(reverse('accounts_login'), next=request.path)
+
+        if course_id:
+            # this is a /course/123/wiki request
+            my_url = request.path.replace(wiki_path, '').replace('/wiki/', '')
+            # HACK: django-wiki monkeypatches the reverse function to enable
+            # urls to be rewritten
+            reverse._transform_url = lambda url: my_url + url  # pylint: disable=W0212
+            # Authorization Check
+            # Let's see if user is enrolled or the course allows for public access
+            try:
+                course = get_course_with_access(request.user, course_id, 'load')
+            except Http404:
+                # course does not exist. redirect to root wiki.
+                # clearing the referrer will cause process_response not to redirect
+                # back to a non-existent course
+                request.META['HTTP_REFERER'] = ''
+                return redirect('/wiki/{}'.format(wiki_path))
+
+            if not course.allow_public_wiki_access:
+                is_enrolled = CourseEnrollment.is_enrolled(request.user, course.id)
+                is_staff = has_access(request.user, course, 'staff')
+                if not (is_enrolled or is_staff):
+                    # if a user is logged in, but not authorized to see a page,
+                    # we'll redirect them to the course about page
+                    return redirect('about_course', course_id)
+            # set the course onto here so that the wiki template can show the course navigation
+            request.course = course
+        else:
+            # this is a request for /wiki/...
+
+            # Check to see if we don't allow top-level access to the wiki via the /wiki/xxxx/yyy/zzz URLs
+            # this will help prevent people from writing pell-mell to the Wiki in an unstructured way
+            if not settings.FEATURES.get('ALLOW_WIKI_ROOT_ACCESS', False):
+                raise PermissionDenied()
+
+            return self._redirect_from_referrer(request, wiki_path)
+
+    def process_response(self, request, response):
+        """
+        Modify the redirect from /wiki/123 to /course/foo/bar/wiki/123/
+        if the referrer comes from a course page
+        """
+        if response.status_code == 302 and response['Location'].startswith('/wiki/'):
+            wiki_path = urlparse(response['Location']).path.split('/wiki/', 1)[1]
+
+            response = self._redirect_from_referrer(request, wiki_path) or response
+
+        # END HACK: _transform_url must be set to a no-op function after it's done its work
+        reverse._transform_url = lambda url: url  # pylint: disable=W0212
+        return response

lms/djangoapps/course_wiki/plugins/markdownedx/mdx_image.py

@@ -39,7 +39,7 @@ class ImageExtension(markdown.Extension):
 
     def extendMarkdown(self, md, md_globals):
         self.add_inline(md, 'image', ImageLink,
-        r'^(?P<proto>([^:/?#])+://)?(?P<domain>([^/?#]*)/)?(?P<path>[^?#]*\.(?P<ext>[^?#]{3,4}))(?:\?([^#]*))?(?:#(.*))?$')
+                        r'^(?P<proto>([^:/?#])+://)?(?P<domain>([^/?#]*)/)?(?P<path>[^?#]*\.(?P<ext>[^?#]{3,4}))(?:\?([^#]*))?(?:#(.*))?$')
 
 
 class ImageLink(markdown.inlinepatterns.Pattern):

lms/djangoapps/course_wiki/tests/tests.py

@@ -53,10 +53,8 @@ class WikiRedirectTestCase(LoginEnrollmentTestCase):
 
         self.assertEqual(resp['Location'], 'http://testserver' + redirected_to)
 
-
         # Now we test that the student will be redirected away from that page if the course doesn't exist
         # We do this in the same test because we want to make sure the redirected_to is constructed correctly
-
         # This is a location like /courses/*/wiki/* , but with an invalid course ID
         bad_course_wiki_page = redirected_to.replace(self.toy.location.course, "bad_course")
 
@@ -119,6 +117,7 @@ class WikiRedirectTestCase(LoginEnrollmentTestCase):
         self.create_course_page(self.toy)
 
         course_wiki_page = reverse('wiki:get', kwargs={'path': self.toy.wiki_slug + '/'})
+
         referer = reverse("courseware", kwargs={'course_id': self.toy.id})
 
         resp = self.client.get(course_wiki_page, follow=True, HTTP_REFERER=referer)
@@ -167,6 +166,4 @@ class WikiRedirectTestCase(LoginEnrollmentTestCase):
         # and end up at the login page
         resp = self.client.get(course_wiki_page, follow=True)
         target_url, __ = resp.redirect_chain[-1]
-        self.assertTrue(
-            target_url.endswith(reverse('accounts_login'))
-        )
+        self.assertTrue(reverse('accounts_login') in target_url)

lms/djangoapps/course_wiki/utils.py

@@ -6,6 +6,7 @@ from django.core.exceptions import ObjectDoesNotExist
 from xmodule import modulestore
 import courseware
 
+
 def user_is_article_course_staff(user, article):
     """
     The root of a course wiki is /<course_number>. This means in case there

lms/djangoapps/course_wiki/views.py

+"""
+This file contains view functions for wrapping the django-wiki.
+"""
 import logging
 import re
 import cgi
@@ -17,7 +20,7 @@ from course_wiki.utils import course_wiki_slug
 log = logging.getLogger(__name__)
 
 
-def root_create(request):
+def root_create(request):  # pylint: disable=W0613
     """
     In the edX wiki, we don't show the root_create view. Instead, we
     just create the root automatically if it doesn't exist.
@@ -26,7 +29,7 @@ def root_create(request):
     return redirect('wiki:get', path=root.path)
 
 
-def course_wiki_redirect(request, course_id):
+def course_wiki_redirect(request, course_id):  # pylint: disable=W0613
     """
     This redirects to whatever page on the wiki that the course designates
     as it's home page. A course's wiki must be an article on the root (for
@@ -46,17 +49,17 @@ def course_wiki_redirect(request, course_id):
     if not valid_slug:
         return redirect("wiki:get", path="")
 
-
     # The wiki needs a Site object created. We make sure it exists here
     try:
-        site = Site.objects.get_current()
+        Site.objects.get_current()
     except Site.DoesNotExist:
         new_site = Site()
         new_site.domain = settings.SITE_NAME
         new_site.name = "edX"
         new_site.save()
-        if str(new_site.id) != str(settings.SITE_ID):
-            raise ImproperlyConfigured("No site object was created and the SITE_ID doesn't match the newly created one. " + str(new_site.id) + "!=" + str(settings.SITE_ID))
+        site_id = str(new_site.id)  # pylint: disable=E1101
+        if site_id != str(settings.SITE_ID):
+            raise ImproperlyConfigured("No site object was created and the SITE_ID doesn't match the newly created one. {} != {}".format(site_id, settings.SITE_ID))
 
     try:
         urlpath = URLPath.get_by_path(course_slug, select_related=True)

lms/envs/common.py

@@ -324,7 +324,6 @@ TEMPLATE_CONTEXT_PROCESSORS = (
     'django.core.context_processors.tz',
     'django.contrib.messages.context_processors.messages',
     'sekizai.context_processors.sekizai',
-    'course_wiki.course_nav.context_processor',
 
     # Hack to get required link URLs to password reset templates
     'edxmako.shortcuts.marketing_link_context_processor',
@@ -736,8 +735,6 @@ MIDDLEWARE_CLASSES = (
     'django.middleware.csrf.CsrfViewMiddleware',
     'splash.middleware.SplashMiddleware',
 
-    'course_wiki.course_nav.Middleware',
-
     # Allows us to dark-launch particular languages
     'dark_lang.middleware.DarkLangMiddleware',
     'embargo.middleware.EmbargoMiddleware',
@@ -768,6 +765,8 @@ MIDDLEWARE_CLASSES = (
 
     # use Django built in clickjacking protection
     'django.middleware.clickjacking.XFrameOptionsMiddleware',
+
+    'course_wiki.middleware.WikiAccessMiddleware',
 )
 
 # Clickjacking protection can be enabled by setting this to 'DENY'

lms/templates/wiki/base.html

@@ -49,8 +49,10 @@
 {% block nav_skip %}#wiki-content{% endblock %}
 
 {% block body %}
-  {% if course %}
+  {% if request.course %}
+  {% with course=request.course %}
     {% include "courseware/course_navigation.html" with active_page_context="wiki" %}
+  {% endwith %}
   {% endif %}
 
   <section class="container wiki {{ selected_tab }}" id="wiki-content">

lms/urls.py

@@ -159,8 +159,6 @@ if settings.WIKI_ENABLED:
     from wiki.urls import get_pattern as wiki_pattern
     from django_notify.urls import get_pattern as notify_pattern
 
-    # Note that some of these urls are repeated in course_wiki.course_nav. Make sure to update
-    # them together.
     urlpatterns += (
         # First we include views from course_wiki that we use to override the default views.
         # They come first in the urlpatterns so they get resolved first