From 40f270c94578ab30e6a1cd8736b484957cd0930d Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Wed, 24 Sep 2014 14:46:30 +0500 Subject: [PATCH] add couse key verification decorator in common for use in both studio and lms PLAT-88 --- cms/djangoapps/contentstore/tests/tests.py | 28 ++++++++++++++ .../contentstore/views/import_export.py | 4 ++ common/djangoapps/util/views.py | 23 +++++++++++ lms/djangoapps/courseware/tests/test_tabs.py | 2 +- lms/djangoapps/courseware/tests/test_views.py | 9 +++-- lms/djangoapps/courseware/views.py | 38 +++++-------------- 6 files changed, 71 insertions(+), 33 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/tests.py b/cms/djangoapps/contentstore/tests/tests.py index 0de3e01bd5..c86ab407a3 100644 --- a/cms/djangoapps/contentstore/tests/tests.py +++ b/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) diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index fa982cfb0a..3cf2b558e6 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/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\d{1,11})-(?P\d{1,11})/(?P\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. diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index e874002b78..05b6789ee5 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -1,6 +1,7 @@ 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'): """ diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 8dad0342c5..0cb978fb79 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/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): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index e00eb5dbd0..ff89b1a290 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/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) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 15d0edaf99..c216b0ceea 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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.