Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MIT CCX: Use CCX Keys - responses to code review

Browse Source 
remove references to middleware that were missed previously

use key apis rather than local implementation of key conversion.  remove local implementationa

remove spurious test for attribute

fix test setUp to avoid unneeded flattening

code quality fixes

add security check ensuring that the coach is coach for *this* CCX.

prevent ccx/deprecated course id problems

1.  do not allow ccx objects to be created if the course id is deprecated
2.  filter out any ccx memberships that involve deprecated course ids (in case there are bad ccxs in the database)

Fix test failures and errors arising from incorrect code path execution

Create context manager to handle unwrapping and restoring ccx values for the modulestore wrapper, employ it throughout modulestore wrapper implementation
This commit is contained in:
cewing
2015-06-08 11:29:46 -07:00
parent 6a0c9aee9d
commit ef4836503f
11 changed files with 207 additions and 188 deletions
Download Patch File Download Diff File Expand all files Collapse all files

280
lms/djangoapps/ccx/modulestore.py
View File

@@ -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)

28
lms/djangoapps/ccx/tests/test_ccx_modulestore.py
View File

@@ -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,

12
lms/djangoapps/ccx/tests/test_utils.py
View File

@@ -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)

11
lms/djangoapps/ccx/utils.py
View File

@@ -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

23
lms/djangoapps/ccx/views.py
View File

@@ -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,

9
lms/djangoapps/courseware/access.py
View File

@@ -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):
"""

9
lms/djangoapps/dashboard/sysadmin.py
View File

@@ -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 = []

9
lms/djangoapps/dashboard/tests/test_sysadmin.py
View File

@@ -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)

1
lms/envs/aws.py
View File

@@ -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',
)

1
lms/envs/yaml_config.py
View File

@@ -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',
)

12
lms/templates/ccx/coach_dashboard.html
View File

@@ -22,6 +22,18 @@ from django.core.urlresolvers import reverse
<section class="instructor-dashboard-content-2" id="ccx-coach-dashboard-content">
<h1>${_("CCX Coach Dashboard")}</h1>
% if messages:
<ul class="messages">
% for message in messages:
% if message.tags:
<li class="${message.tags}">${message}</li>
% else:
<li>${message}</li>
% endif
% endfor
</ul>
% endif
%if not ccx:
<section>
<form action="${create_ccx_url}" method="POST">