diff --git a/lms/djangoapps/ccx/modulestore.py b/lms/djangoapps/ccx/modulestore.py index ed5172abff..d8ac7d7169 100644 --- a/lms/djangoapps/ccx/modulestore.py +++ b/lms/djangoapps/ccx/modulestore.py @@ -10,35 +10,21 @@ returned from the modulestore will have their keys updated to be the CCX version that was passed in. """ from contextlib import contextmanager +from functools import partial from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator -def ccx_locator_to_course_locator(ccx_locator): - return CourseLocator( - org=ccx_locator.org, - course=ccx_locator.course, - run=ccx_locator.run, - branch=ccx_locator.branch, - version_guid=ccx_locator.version_guid, - ) - - def strip_ccx(val): retval = val ccx_id = None - if hasattr(retval, 'ccx'): - if isinstance(retval, CCXLocator): - ccx_id = retval.ccx - retval = ccx_locator_to_course_locator(retval) - elif isinstance(object, CCXBlockUsageLocator): - ccx_locator = retval.course_key - ccx_id = ccx_locator.ccx - course_locator = ccx_locator_to_course_locator(ccx_locator) - retval = BlockUsageLocator( - course_locator, retval.block_type, retval.block_id - ) - if hasattr(retval, 'location'): + if isinstance(retval, CCXLocator): + ccx_id = retval.ccx + retval = retval.to_course_locator() + elif isinstance(object, CCXBlockUsageLocator): + ccx_id = retval.course_key.ccx + retval = retval.to_block_locator() + elif hasattr(retval, 'location'): retval.location, ccx_id = strip_ccx(retval.location) return retval, ccx_id @@ -56,7 +42,9 @@ def restore_ccx(val, ccx_id): return val -def restore_ccx_collection(field_value, ccx_id): +def restore_ccx_collection(field_value, ccx_id=None): + if ccx_id is None: + return field_value if isinstance(field_value, list): field_value = [restore_ccx(fv, ccx_id) for fv in field_value] elif isinstance(field_value, dict): @@ -67,6 +55,12 @@ def restore_ccx_collection(field_value, ccx_id): return field_value +@contextmanager +def remove_ccx(to_strip): + stripped, ccx = strip_ccx(to_strip) + yield stripped, partial(restore_ccx_collection, ccx_id=ccx) + + class CCXModulestoreWrapper(object): def __init__(self, modulestore): @@ -78,11 +72,10 @@ class CCXModulestoreWrapper(object): return getattr(self._modulestore, name) def _clean_locator_for_mapping(self, locator): - locator, ccx = strip_ccx(locator) - retval = self._modulestore._clean_locator_for_mapping(locator) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(locator) as stripped: + locator, restore = stripped + retval = self._modulestore._clean_locator_for_mapping(locator) + return restore(retval) def _get_modulestore_for_courselike(self, locator=None): if locator is not None: @@ -94,11 +87,10 @@ class CCXModulestoreWrapper(object): Some course_keys are used without runs. This function calls the corresponding fill_in_run function on the appropriate modulestore. """ - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.fill_in_run(course_key) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.fill_in_run(course_key) + return restore(retval) def has_item(self, usage_key, **kwargs): """ @@ -111,35 +103,32 @@ class CCXModulestoreWrapper(object): """ see parent doc """ - usage_key, ccx = strip_ccx(usage_key) - retval = self._modulestore.get_item(usage_key, depth, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(usage_key) as stripped: + usage_key, restore = stripped + retval = self._modulestore.get_item(usage_key, depth, **kwargs) + return restore(retval) def get_items(self, course_key, **kwargs): - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.get_items(course_key, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.get_items(course_key, **kwargs) + return restore(retval) def get_course(self, course_key, depth=0, **kwargs): - # from nose.tools import set_trace; set_trace() - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.get_course(course_key, depth=depth, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.get_course( + course_key, depth=depth, **kwargs + ) + return restore(retval) def has_course(self, course_id, ignore_case=False, **kwargs): - course_id, ccx = strip_ccx(course_id) - retval = self._modulestore.has_course( - course_id, ignore_case=ignore_case, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_id) as stripped: + course_id, restore = stripped + retval = self._modulestore.has_course( + course_id, ignore_case=ignore_case, **kwargs + ) + return restore(retval) def delete_course(self, course_key, user_id): """ @@ -149,32 +138,28 @@ class CCXModulestoreWrapper(object): return self._modulestore.delete_course(course_key, user_id) def get_parent_location(self, location, **kwargs): - location, ccx = strip_ccx(location) - retval = self._modulestore.get_parent_location(location, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.get_parent_location(location, **kwargs) + return restore(retval) def get_block_original_usage(self, usage_key): - usage_key, ccx = strip_ccx(usage_key) - orig_key, version = self._modulestore.get_block_original_usage(usage_key) - if orig_key and ccx: - orig_key = restore_ccx_collection(orig_key, ccx) - return orig_key, version + with remove_ccx(usage_key) as stripped: + usage_key, restore = stripped + orig_key, version = self._modulestore.get_block_original_usage(usage_key) + return restore(orig_key), version def get_modulestore_type(self, course_id): - course_id, ccx = strip_ccx(course_id) - retval = self._modulestore.get_modulestore_type(course_id) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_id) as stripped: + course_id, restore = stripped + retval = self._modulestore.get_modulestore_type(course_id) + return restore(retval) def get_orphans(self, course_key, **kwargs): - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.get_orphans(course_key, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.get_orphans(course_key, **kwargs) + return restore(retval) def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): source_course_id, source_ccx = strip_ccx(source_course_id) @@ -187,109 +172,94 @@ class CCXModulestoreWrapper(object): return retval def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.create_item( - user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.create_item( + user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + return restore(retval) def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): - parent_usage_key, ccx = strip_ccx(parent_usage_key) - retval = self._modulestore.create_child( - user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(parent_usage_key) as stripped: + parent_usage_key, restore = stripped + retval = self._modulestore.create_child( + user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + return restore(retval) def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.import_xblock( - user_id, course_key, block_type, block_id, fields=fields, runtime=runtime, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.import_xblock( + user_id, course_key, block_type, block_id, fields=fields, runtime=runtime, **kwargs + ) + return restore(retval) def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): - dest_key, ccx = strip_ccx(dest_key) - retval = self._modulestore.copy_from_template( - source_keys, dest_key, user_id, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(dest_key) as stripped: + dest_key, restore = stripped + retval = self._modulestore.copy_from_template( + source_keys, dest_key, user_id, **kwargs + ) + return restore(retval) def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): - xblock, ccx = strip_ccx(xblock) - retval = self._modulestore.update_item( - xblock, user_id, allow_not_found=allow_not_found, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(xblock) as stripped: + xblock, restore = stripped + retval = self._modulestore.update_item( + xblock, user_id, allow_not_found=allow_not_found, **kwargs + ) + return restore(retval) def delete_item(self, location, user_id, **kwargs): - location, ccx = strip_ccx(location) - retval = self._modulestore.delete_item(location, user_id, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.delete_item(location, user_id, **kwargs) + return restore(retval) def revert_to_published(self, location, user_id): - location, ccx = strip_ccx(location) - retval = self._modulestore.revert_to_published(location, user_id) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.revert_to_published(location, user_id) + return restore(retval) def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): - course_key, ccx = strip_ccx(course_key) - retval = self._modulestore.create_xblock( - runtime, course_key, block_type, block_id=block_id, fields=fields, **kwargs - ) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(course_key) as stripped: + course_key, restore = stripped + retval = self._modulestore.create_xblock( + runtime, course_key, block_type, block_id=block_id, fields=fields, **kwargs + ) + return restore(retval) def has_published_version(self, xblock): - xblock.scope_ids.usage_id.course_key, ccx = strip_ccx( - xblock.scope_ids.usage_id.course_key - ) - retval = self._modulestore.has_published_version(xblock) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(xblock) as stripped: + xblock, restore = stripped + retval = self._modulestore.has_published_version(xblock) + return restore(retval) def publish(self, location, user_id, **kwargs): - location, ccx = strip_ccx(location) - retval = self._modulestore.publish(location, user_id, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.publish(location, user_id, **kwargs) + return restore(retval) def unpublish(self, location, user_id, **kwargs): - location, ccx = strip_ccx(location) - retval = self._modulestore.unpublish(location, user_id, **kwargs) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.unpublish(location, user_id, **kwargs) + return restore(retval) def convert_to_draft(self, location, user_id): - location, ccx = strip_ccx(location) - retval = self._modulestore.convert_to_draft(location, user_id) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(location) as stripped: + location, restore = stripped + retval = self._modulestore.convert_to_draft(location, user_id) + return restore(retval) def has_changes(self, xblock): - xblock.location, ccx = strip_ccx(xblock.location) - retval = self._modulestore.has_changes(xblock) - if ccx: - retval = restore_ccx_collection(retval, ccx) - return retval + with remove_ccx(xblock) as stripped: + xblock, restore = stripped + retval = self._modulestore.has_changes(xblock) + return restore(retval) def check_supports(self, course_key, method): course_key, _ = strip_ccx(course_key) diff --git a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py index c8fc0858b6..79129a38d7 100644 --- a/lms/djangoapps/ccx/tests/test_ccx_modulestore.py +++ b/lms/djangoapps/ccx/tests/test_ccx_modulestore.py @@ -48,23 +48,17 @@ class TestCCXModulestoreWrapper(ModuleStoreTestCase): 2010, 7, 7, 0, 0, tzinfo=pytz.UTC) chapters = [ItemFactory.create(start=start, parent=course) for _ in xrange(2)] - sequentials = flatten([ - [ - ItemFactory.create(parent=chapter) for _ in xrange(2) - ] for chapter in chapters - ]) - verticals = flatten([ - [ - ItemFactory.create( - due=due, parent=sequential, graded=True, format='Homework' - ) for _ in xrange(2) - ] for sequential in sequentials - ]) - blocks = flatten([ # pylint: disable=unused-variable - [ - ItemFactory.create(parent=vertical) for _ in xrange(2) - ] for vertical in verticals - ]) + sequentials = [ + ItemFactory.create(parent=c) for _ in xrange(2) for c in chapters + ] + verticals = [ + ItemFactory.create( + due=due, parent=s, graded=True, format='Homework' + ) for _ in xrange(2) for s in sequentials + ] + blocks = [ + ItemFactory.create(parent=v) for _ in xrange(2) for v in verticals + ] self.ccx = ccx = CustomCourseForEdX( course_id=course.id, diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index d390ca4af0..25325059de 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -69,9 +69,9 @@ class TestEmailEnrollmentState(ModuleStoreTestCase): """verify behavior for non-user email address """ ee_state = self.create_one(email='nobody@nowhere.com') - for attr in ['user', 'member', 'full_name', 'in_ccx']: - value = getattr(ee_state, attr, 'missing attribute') - self.assertFalse(value, "{}: {}".format(value, attr)) + for attribute in ['user', 'member', 'full_name', 'in_ccx']: + value = getattr(ee_state, attribute, 'missing attribute') + self.assertFalse(value, "{}: {}".format(value, attribute)) def test_enrollment_state_for_non_member_user(self): """verify behavior for email address of user who is not a ccx memeber @@ -89,10 +89,10 @@ class TestEmailEnrollmentState(ModuleStoreTestCase): self.create_user() self.register_user_in_ccx() ee_state = self.create_one() - for attr in ['user', 'in_ccx']: + for attribute in ['user', 'in_ccx']: self.assertTrue( - getattr(ee_state, attr, False), - "attribute {} is missing or False".format(attr) + getattr(ee_state, attribute, False), + "attribute {} is missing or False".format(attribute) ) self.assertEqual(ee_state.member, self.user) self.assertEqual(ee_state.full_name, self.user.profile.name) diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 72a8391ffb..88f5ea187e 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -262,6 +262,17 @@ def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): elif course.location.org in org_filter_out_set: continue + # If, somehow, we've got a ccx that has been created for a + # course with a deprecated ID, we must filter it out. Emit a + # warning to the log so we can clean up. + if course.location.deprecated: + log.warning( + "CCX {} exists for course {} with deprecated id".format( + ccx, ccx.course_id + ) + ) + continue + yield (ccx, membership, course) else: log.error("User {0} enrolled in {2} course {1}".format( # pylint: disable=logging-format-interpolation diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 67f6e9ba5c..e07049f784 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -18,6 +18,7 @@ from django.http import ( HttpResponseForbidden, HttpResponseRedirect, ) +from django.contrib import messages from django.core.exceptions import ValidationError from django.core.validators import validate_email from django.http import Http404 @@ -74,8 +75,6 @@ def coach_dashboard(view): course_key = CourseKey.from_string(course_id) ccx = None if isinstance(course_key, CCXLocator): - # is there a security leak here in not checking that this user is - # the coach for this ccx? ccx_id = course_key.ccx ccx = CustomCourseForEdX.objects.get(pk=ccx_id) course_key = ccx.course_id @@ -86,6 +85,15 @@ def coach_dashboard(view): _('You must be a CCX Coach to access this view.')) course = get_course_by_id(course_key, depth=None) + + # if there is a ccx, we must validate that it is the ccx for this coach + if ccx is not None: + coach_ccx = get_ccx_for_coach(course, request.user) + if coach_ccx is None or coach_ccx.id != ccx.id: + return HttpResponseForbidden( + _('You must be the coach for this ccx to access this view') + ) + return view(request, course, ccx) return wrapper @@ -140,6 +148,17 @@ def create_ccx(request, course, ccx=None): Create a new CCX """ name = request.POST.get('name') + + # prevent CCX objects from being created for deprecated course ids. + if course.id.deprecated: + messages.error(_( + "You cannot create a CCX from a course using a deprecated id. " + "Please create a rerun of this course in the studio to allow " + "this action.") + ) + url = reverse('ccx_coach_dashboard', kwargs={'course_id', course.id}) + return redirect(url) + ccx = CustomCourseForEdX( course_id=course.id, coach=request.user, diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 4f1963ee72..0d66aa17e3 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -110,9 +110,10 @@ def has_access(user, action, obj, course_key=None): if isinstance(obj, XBlock): return _has_access_descriptor(user, action, obj, course_key) + if isinstance(obj, CCXLocator): + return _has_access_ccx_key(user, action, obj) + if isinstance(obj, CourseKey): - if isinstance(obj, CCXLocator): - obj = obj.to_course_locator() return _has_access_course_key(user, action, obj) if isinstance(obj, UsageKey): @@ -493,6 +494,10 @@ def _has_access_course_key(user, action, course_key): return _dispatch(checkers, action, user, course_key) +def _has_access_ccx_key(user, action, ccx_key): + course_key = ccx_key.to_course_locator() + return _has_access_course_key(user, action, course_key) + def _has_access_string(user, action, perm): """ diff --git a/lms/djangoapps/dashboard/sysadmin.py b/lms/djangoapps/dashboard/sysadmin.py index 2b656cecbd..da10935ca0 100644 --- a/lms/djangoapps/dashboard/sysadmin.py +++ b/lms/djangoapps/dashboard/sysadmin.py @@ -43,6 +43,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml import XMLModuleStore from opaque_keys.edx.locations import SlashSeparatedCourseKey +from ccx.modulestore import CCXModulestoreWrapper log = logging.getLogger(__name__) @@ -59,9 +60,13 @@ class SysadminDashboardView(TemplateView): modulestore_type and return msg """ - self.def_ms = modulestore() + self.def_ms = check_ms = modulestore() + # if the modulestore is wrapped by CCX, unwrap it for checking purposes + if isinstance(check_ms, CCXModulestoreWrapper): + check_ms = check_ms._modulestore + self.is_using_mongo = True - if isinstance(self.def_ms, XMLModuleStore): + if isinstance(check_ms, XMLModuleStore): self.is_using_mongo = False self.msg = u'' self.datatable = [] diff --git a/lms/djangoapps/dashboard/tests/test_sysadmin.py b/lms/djangoapps/dashboard/tests/test_sysadmin.py index 735ffb29f6..8ed5c07d56 100644 --- a/lms/djangoapps/dashboard/tests/test_sysadmin.py +++ b/lms/djangoapps/dashboard/tests/test_sysadmin.py @@ -33,6 +33,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST from xmodule.modulestore.xml import XMLModuleStore +from ccx.modulestore import CCXModulestoreWrapper TEST_MONGODB_LOG = { @@ -315,8 +316,12 @@ class TestSysadmin(SysadminBaseTestCase): # Create git loaded course response = self._add_edx4edx() - def_ms = modulestore() - self.assertIn('xml', str(def_ms.__class__)) + def_ms = check_ms = modulestore() + # if the modulestore is wrapped by CCX, unwrap it for testing purposes + if isinstance(check_ms, CCXModulestoreWrapper): + check_ms = check_ms._modulestore + + self.assertIn('xml', str(check_ms.__class__)) course = def_ms.courses.get('{0}/edx4edx_lite'.format( os.path.abspath(settings.DATA_DIR)), None) self.assertIsNotNone(course) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 06e9febead..fc1a9770b1 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -612,7 +612,6 @@ ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', ECOMMERCE_API_TI ##### Custom Courses for EdX ##### if FEATURES.get('CUSTOM_COURSES_EDX'): INSTALLED_APPS += ('ccx',) - MIDDLEWARE_CLASSES += ('ccx.overrides.CcxMiddleware',) FIELD_OVERRIDE_PROVIDERS += ( 'ccx.overrides.CustomCoursesForEdxOverrideProvider', ) diff --git a/lms/envs/yaml_config.py b/lms/envs/yaml_config.py index 6e17d439cf..db25bab6dd 100644 --- a/lms/envs/yaml_config.py +++ b/lms/envs/yaml_config.py @@ -307,7 +307,6 @@ GRADES_DOWNLOAD_ROUTING_KEY = HIGH_MEM_QUEUE ##### Custom Courses for EdX ##### if FEATURES.get('CUSTOM_COURSES_EDX'): INSTALLED_APPS += ('ccx',) - MIDDLEWARE_CLASSES += ('ccx.overrides.CcxMiddleware',) FIELD_OVERRIDE_PROVIDERS += ( 'ccx.overrides.CustomCoursesForEdxOverrideProvider', ) diff --git a/lms/templates/ccx/coach_dashboard.html b/lms/templates/ccx/coach_dashboard.html index 86e778ab84..41c34f3e3d 100644 --- a/lms/templates/ccx/coach_dashboard.html +++ b/lms/templates/ccx/coach_dashboard.html @@ -22,6 +22,18 @@ from django.core.urlresolvers import reverse

${_("CCX Coach Dashboard")}

+ % if messages: + + % endif + %if not ccx: