Merge pull request #5362 from edx/zub/bugfix/plat-88-importstatusinvalidkeyerror

add couse key verification decorator in common for use in both studio an...

cms/djangoapps/contentstore/tests/tests.py

@@ -4,6 +4,7 @@ This test file will test registration, login, activation, and session activity t
 import time
 import mock
 import unittest
+from ddt import ddt, data, unpack
 
 from django.test.utils import override_settings
 from django.core.cache import cache
@@ -315,3 +316,30 @@ class ForumTestCase(CourseTestCase):
         ]
         self.course.discussion_blackouts = [(t.isoformat(), t2.isoformat()) for t, t2 in times2]
         self.assertFalse(self.course.forum_posts_allowed)
+
+
+@ddt
+class CourseKeyVerificationTestCase(CourseTestCase):
+    def setUp(self):
+        """
+        Create test course.
+        """
+        super(CourseKeyVerificationTestCase, self).setUp()
+        self.course = CourseFactory.create(org='edX', number='test_course_key', display_name='Test Course')
+
+    @data(('edX/test_course_key/Test_Course', 200), ('slashes:edX+test_course_key+Test_Course', 404))
+    @unpack
+    def test_course_key_decorator(self, course_key, status_code):
+        """
+        Tests for the ensure_valid_course_key decorator.
+        """
+        url = '/import/{course_key}'.format(course_key=course_key)
+        resp = self.client.get_html(url)
+        self.assertEqual(resp.status_code, status_code)
+
+        url = '/import_status/{course_key}/{filename}'.format(
+            course_key=course_key,
+            filename='xyz.tar.gz'
+        )
+        resp = self.client.get_html(url)
+        self.assertEqual(resp.status_code, status_code)

cms/djangoapps/contentstore/views/import_export.py

@@ -34,6 +34,7 @@ from extract_tar import safetar_extractall
 from student import auth
 from student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff
 from util.json_request import JsonResponse
+from util.views import ensure_valid_course_key
 
 from contentstore.utils import reverse_course_url, reverse_usage_url
 
@@ -52,6 +53,7 @@ CONTENT_RE = re.compile(r"(?P<start>\d{1,11})-(?P<stop>\d{1,11})/(?P<end>\d{1,11
 @login_required
 @ensure_csrf_cookie
 @require_http_methods(("GET", "POST", "PUT"))
+@ensure_valid_course_key
 def import_handler(request, course_key_string):
     """
     The restful handler for importing a course.
@@ -299,6 +301,7 @@ def _save_request_status(request, key, status):
 @require_GET
 @ensure_csrf_cookie
 @login_required
+@ensure_valid_course_key
 def import_status_handler(request, course_key_string, filename=None):
     """
     Returns an integer corresponding to the status of a file import. These are:
@@ -328,6 +331,7 @@ def import_status_handler(request, course_key_string, filename=None):
 @ensure_csrf_cookie
 @login_required
 @require_http_methods(("GET",))
+@ensure_valid_course_key
 def export_handler(request, course_key_string):
     """
     The restful handler for exporting a course.

common/djangoapps/util/views.py

 import json
 import logging
 import sys
+from functools import wraps
 
 from django.conf import settings
 from django.core.validators import ValidationError, validate_email
@@ -16,10 +17,32 @@ from microsite_configuration import microsite
 import calc
 import track.views
 
+from opaque_keys import InvalidKeyError
+from opaque_keys.edx.keys import CourseKey
 
 log = logging.getLogger(__name__)
 
 
+def ensure_valid_course_key(view_func):
+    """
+    This decorator should only be used with views which have argument course_key_string (studio) or course_id (lms).
+    If course_key_string (studio) or course_id (lms) is not valid raise 404.
+    """
+    @wraps(view_func)
+    def inner(request, *args, **kwargs):
+        course_key = kwargs.get('course_key_string') or kwargs.get('course_id')
+        if course_key is not None:
+            try:
+                CourseKey.from_string(course_key)
+            except InvalidKeyError:
+                raise Http404
+
+        response = view_func(request, *args, **kwargs)
+        return response
+
+    return inner
+
+
 @requires_csrf_token
 def jsonable_server_error(request, template_name='500.html'):
     """

lms/djangoapps/courseware/tests/test_tabs.py

@@ -47,7 +47,7 @@ class StaticTabDateTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase):
     def test_invalid_course_key(self):
         request = get_request_for_user(UserFactory.create())
         with self.assertRaises(Http404):
-            static_tab(request, 'edX/toy', 'new_tab')
+            static_tab(request, course_id='edX/toy', tab_slug='new_tab')
 
     @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
     def test_get_static_tab_contents(self):

lms/djangoapps/courseware/tests/test_views.py

@@ -35,6 +35,7 @@ from course_modes.models import CourseMode
 import shoppingcart
 
 from util.tests.test_date_utils import fake_ugettext, fake_pgettext
+from util.views import ensure_valid_course_key
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -568,9 +569,9 @@ class ProgressPageTests(ModuleStoreTestCase):
         self.assertEqual(resp.status_code, 200)
 
 
-class TestVerifyCourseIdDecorator(TestCase):
+class VerifyCourseKeyDecoratorTests(TestCase):
     """
-    Tests for the verify_course_id decorator.
+    Tests for the ensure_valid_course_key decorator.
     """
 
     def setUp(self):
@@ -580,12 +581,12 @@ class TestVerifyCourseIdDecorator(TestCase):
 
     def test_decorator_with_valid_course_id(self):
         mocked_view = create_autospec(views.course_about)
-        view_function = views.verify_course_id(mocked_view)
+        view_function = ensure_valid_course_key(mocked_view)
         view_function(self.request, course_id=self.valid_course_id)
         self.assertTrue(mocked_view.called)
 
     def test_decorator_with_invalid_course_id(self):
         mocked_view = create_autospec(views.course_about)
-        view_function = views.verify_course_id(mocked_view)
+        view_function = ensure_valid_course_key(mocked_view)
         self.assertRaises(Http404, view_function, self.request, course_id=self.invalid_course_id)
         self.assertFalse(mocked_view.called)

lms/djangoapps/courseware/views.py

@@ -55,6 +55,7 @@ from microsite_configuration import microsite
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from instructor.enrollment import uses_shib
 
+from util.views import ensure_valid_course_key
 log = logging.getLogger("edx.courseware")
 
 template_imports = {'urllib': urllib}
@@ -85,25 +86,6 @@ def user_groups(user):
     return group_names
 
 
-def verify_course_id(view_func):
-    """
-    This decorator should only be used with views whose kwargs must contain course_id.
-    If course_id is not valid raise 404.
-    """
-
-    @wraps(view_func)
-    def _decorated(request, *args, **kwargs):
-        course_id = kwargs.get("course_id")
-        try:
-            SlashSeparatedCourseKey.from_deprecated_string(course_id)
-        except InvalidKeyError:
-            raise Http404
-        response = view_func(request, *args, **kwargs)
-        return response
-
-    return _decorated
-
-
 @ensure_csrf_cookie
 @cache_if_anonymous
 def courses(request):
@@ -265,7 +247,7 @@ def chat_settings(course, user):
 @login_required
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
-@verify_course_id
+@ensure_valid_course_key
 def index(request, course_id, chapter=None, section=None,
           position=None):
     """
@@ -503,7 +485,7 @@ def index(request, course_id, chapter=None, section=None,
 
 
 @ensure_csrf_cookie
-@verify_course_id
+@ensure_valid_course_key
 def jump_to_id(request, course_id, module_id):
     """
     This entry point allows for a shorter version of a jump to where just the id of the element is
@@ -562,7 +544,7 @@ def jump_to(request, course_id, location):
 
 
 @ensure_csrf_cookie
-@verify_course_id
+@ensure_valid_course_key
 def course_info(request, course_id):
     """
     Display the course's info.html, or 404 if there is no such course.
@@ -610,7 +592,7 @@ def course_info(request, course_id):
 
 
 @ensure_csrf_cookie
-@verify_course_id
+@ensure_valid_course_key
 def static_tab(request, course_id, tab_slug):
     """
     Display the courses tab with the given name.
@@ -644,7 +626,7 @@ def static_tab(request, course_id, tab_slug):
 
 
 @ensure_csrf_cookie
-@verify_course_id
+@ensure_valid_course_key
 def syllabus(request, course_id):
     """
     Display the course's syllabus.html, or 404 if there is no such course.
@@ -756,7 +738,7 @@ def course_about(request, course_id):
 
 @ensure_csrf_cookie
 @cache_if_anonymous
-@verify_course_id
+@ensure_valid_course_key
 def mktg_course_about(request, course_id):
     """
     This is the button that gets put into an iframe on the Drupal site
@@ -814,7 +796,7 @@ def mktg_course_about(request, course_id):
 @login_required
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 @transaction.commit_manually
-@verify_course_id
+@ensure_valid_course_key
 def progress(request, course_id, student_id=None):
     """
     Wraps "_progress" with the manual_transaction context manager just in case
@@ -895,7 +877,7 @@ def fetch_reverify_banner_info(request, course_key):
 
 
 @login_required
-@verify_course_id
+@ensure_valid_course_key
 def submission_history(request, course_id, student_username, location):
     """Render an HTML fragment (meant for inclusion elsewhere) that renders a
     history of all state changes made by this user for this problem location.
@@ -1003,7 +985,7 @@ def get_static_tab_contents(request, course, tab):
 
 
 @require_GET
-@verify_course_id
+@ensure_valid_course_key
 def get_course_lti_endpoints(request, course_id):
     """
     View that, given a course_id, returns the a JSON object that enumerates all of the LTI endpoints for that course.