From 660244201f54a881446a8f2c35d378d394899c2b Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Fri, 28 Feb 2014 18:26:39 -0500 Subject: [PATCH] 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 | 176 ------------------ lms/djangoapps/course_wiki/editors.py | 3 +- lms/djangoapps/course_wiki/middleware.py | 102 ++++++++++ .../plugins/markdownedx/mdx_image.py | 2 +- lms/djangoapps/course_wiki/tests/tests.py | 7 +- lms/djangoapps/course_wiki/utils.py | 1 + lms/djangoapps/course_wiki/views.py | 15 +- lms/envs/common.py | 5 +- lms/templates/wiki/base.html | 4 +- lms/urls.py | 2 - 10 files changed, 122 insertions(+), 195 deletions(-) delete mode 100644 lms/djangoapps/course_wiki/course_nav.py create mode 100644 lms/djangoapps/course_wiki/middleware.py diff --git a/lms/djangoapps/course_wiki/course_nav.py b/lms/djangoapps/course_wiki/course_nav.py deleted file mode 100644 index 1816708d9a..0000000000 --- a/lms/djangoapps/course_wiki/course_nav.py +++ /dev/null @@ -1,176 +0,0 @@ -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[^/]+/[^/]+/[^/]+)/wiki/(?P.*|)$' - -IN_COURSE_WIKI_COMPILED_REGEX = re.compile(IN_COURSE_WIKI_REGEX) -WIKI_ROOT_ACCESS_COMPILED_REGEX = re.compile(r'^/wiki/(?P.*|)$') - -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.*|)$', destination) - if path_match: - # We are going to the wiki. Check if we came from a course - course_match = re.match(r'/courses/(?P[^/]+/[^/]+/[^/]+)/.*', 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 {} diff --git a/lms/djangoapps/course_wiki/editors.py b/lms/djangoapps/course_wiki/editors.py index 129457cc35..3708cec1af 100644 --- a/lms/djangoapps/course_wiki/editors.py +++ b/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) diff --git a/lms/djangoapps/course_wiki/middleware.py b/lms/djangoapps/course_wiki/middleware.py new file mode 100644 index 0000000000..6fe632d3ec --- /dev/null +++ b/lms/djangoapps/course_wiki/middleware.py @@ -0,0 +1,102 @@ +"""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 diff --git a/lms/djangoapps/course_wiki/plugins/markdownedx/mdx_image.py b/lms/djangoapps/course_wiki/plugins/markdownedx/mdx_image.py index af0413f841..6abfede52f 100755 --- a/lms/djangoapps/course_wiki/plugins/markdownedx/mdx_image.py +++ b/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([^:/?#])+://)?(?P([^/?#]*)/)?(?P[^?#]*\.(?P[^?#]{3,4}))(?:\?([^#]*))?(?:#(.*))?$') + r'^(?P([^:/?#])+://)?(?P([^/?#]*)/)?(?P[^?#]*\.(?P[^?#]{3,4}))(?:\?([^#]*))?(?:#(.*))?$') class ImageLink(markdown.inlinepatterns.Pattern): diff --git a/lms/djangoapps/course_wiki/tests/tests.py b/lms/djangoapps/course_wiki/tests/tests.py index 21db8a057f..3bad49f773 100644 --- a/lms/djangoapps/course_wiki/tests/tests.py +++ b/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) diff --git a/lms/djangoapps/course_wiki/utils.py b/lms/djangoapps/course_wiki/utils.py index 9f9956a7b2..331894b81d 100644 --- a/lms/djangoapps/course_wiki/utils.py +++ b/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 /. This means in case there diff --git a/lms/djangoapps/course_wiki/views.py b/lms/djangoapps/course_wiki/views.py index b7e6473dc4..724125a716 100644 --- a/lms/djangoapps/course_wiki/views.py +++ b/lms/djangoapps/course_wiki/views.py @@ -1,3 +1,6 @@ +""" +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) diff --git a/lms/envs/common.py b/lms/envs/common.py index 1d31faf8f6..ebc339db14 100644 --- a/lms/envs/common.py +++ b/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' diff --git a/lms/templates/wiki/base.html b/lms/templates/wiki/base.html index 73e91c4a3b..5561ebe6a9 100644 --- a/lms/templates/wiki/base.html +++ b/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 %}
diff --git a/lms/urls.py b/lms/urls.py index b214d49741..bf3cc089cd 100644 --- a/lms/urls.py +++ b/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