diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py
index 035413a402..ca00db4c9a 100644
--- a/common/lib/xmodule/xmodule/abtest_module.py
+++ b/common/lib/xmodule/xmodule/abtest_module.py
@@ -49,9 +49,9 @@ class ABTestModule(XModule):
return json.dumps({'group': self.group})
def displayable_items(self):
- return [self.system.get_module(child)
- for child
- in self.definition['data']['group_content'][self.group]]
+ return filter(None, [self.system.get_module(child)
+ for child
+ in self.definition['data']['group_content'][self.group]])
# TODO (cpennington): Use Groups should be a first class object, rather than being
diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py
index 2c440cba2f..11d4e090f9 100644
--- a/common/lib/xmodule/xmodule/course_module.py
+++ b/common/lib/xmodule/xmodule/course_module.py
@@ -6,7 +6,7 @@ from xmodule.util.decorators import lazyproperty
from xmodule.graders import load_grading_policy
from xmodule.modulestore import Location
from xmodule.seq_module import SequenceDescriptor, SequenceModule
-from xmodule.timeparse import parse_time
+from xmodule.timeparse import parse_time, stringify_time
log = logging.getLogger(__name__)
@@ -18,16 +18,10 @@ class CourseDescriptor(SequenceDescriptor):
super(CourseDescriptor, self).__init__(system, definition, **kwargs)
msg = None
- try:
- self.start = parse_time(self.metadata["start"])
- except KeyError:
- msg = "Course loaded without a start date. id = %s" % self.id
- except ValueError as e:
- msg = "Course loaded with a bad start date. %s '%s'" % (self.id, e)
-
- # Don't call the tracker from the exception handler.
- if msg is not None:
- self.start = time.gmtime(0) # The epoch
+ if self.start is None:
+ msg = "Course loaded without a valid start date. id = %s" % self.id
+ # hack it -- start in 1970
+ self.metadata['start'] = stringify_time(time.gmtime(0))
log.critical(msg)
system.error_tracker(msg)
diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py
index 20301ee460..bdd7179a0a 100644
--- a/common/lib/xmodule/xmodule/error_module.py
+++ b/common/lib/xmodule/xmodule/error_module.py
@@ -24,16 +24,8 @@ class ErrorModule(XModule):
return self.system.render_template('module-error.html', {
'data' : self.definition['data']['contents'],
'error' : self.definition['data']['error_msg'],
- 'is_staff' : self.system.is_staff,
})
- def displayable_items(self):
- """Hide errors in the profile and table of contents for non-staff
- users.
- """
- if self.system.is_staff:
- return [self]
- return []
class ErrorDescriptor(EditingDescriptor):
"""
diff --git a/common/lib/xmodule/xmodule/timeparse.py b/common/lib/xmodule/xmodule/timeparse.py
index 28a437bd5f..117105d085 100644
--- a/common/lib/xmodule/xmodule/timeparse.py
+++ b/common/lib/xmodule/xmodule/timeparse.py
@@ -3,9 +3,17 @@ Helper functions for handling time in the format we like.
"""
import time
+TIME_FORMAT = "%Y-%m-%dT%H:%M"
+
def parse_time(time_str):
"""
- Takes a time string in our format ("%Y-%m-%dT%H:%M"), and returns
+ Takes a time string in TIME_FORMAT, returns
it as a time_struct. Raises ValueError if the string is not in the right format.
"""
- return time.strptime(time_str, "%Y-%m-%dT%H:%M")
+ return time.strptime(time_str, TIME_FORMAT)
+
+def stringify_time(time_struct):
+ """
+ Convert a time struct to a string
+ """
+ return time.strftime(TIME_FORMAT, time_struct)
diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py
index 94f8010e10..dd2f879113 100644
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -219,9 +219,11 @@ class XModule(HTMLSnippet):
Return module instances for all the children of this module.
'''
if self._loaded_children is None:
- self._loaded_children = [
- self.system.get_module(child)
- for child in self.definition.get('children', [])]
+ # get_module returns None if the current user doesn't have access
+ # to the location.
+ self._loaded_children = filter(None,
+ [self.system.get_module(child)
+ for child in self.definition.get('children', [])])
return self._loaded_children
@@ -385,9 +387,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
self.category = self.location.category
self.shared_state_key = kwargs.get('shared_state_key')
- # look for a start time, setting to None if not present
- self.start = self._try_parse_time('start')
-
self._child_instances = None
self._inherited_metadata = set()
@@ -400,6 +399,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
return self.metadata.get('display_name',
self.url_name.replace('_', ' '))
+ @property
+ def start(self):
+ """
+ If self.metadata contains start, return it. Else return None.
+ """
+ if 'start' not in self.metadata:
+ return None
+ return self._try_parse_time('start')
+
@property
def own_metadata(self):
"""
@@ -609,7 +617,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
"""
if key in self.metadata:
try:
- parse_time(self.metadata[key])
+ return parse_time(self.metadata[key])
except ValueError as e:
msg = "Descriptor {} loaded with a bad metadata key '{}': '{}'".format(
self.location.url(), self.metadata[key], e)
@@ -709,7 +717,8 @@ class ModuleSystem(object):
files. Update or remove.
get_module - function that takes (location) and returns a corresponding
- module instance object.
+ module instance object. If the current user does not have
+ access to that location, returns None.
render_template - a function that takes (template_file, context), and
returns rendered html.
diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py
index a8145aa1ce..8039d16896 100644
--- a/lms/djangoapps/courseware/access.py
+++ b/lms/djangoapps/courseware/access.py
@@ -8,6 +8,7 @@ import time
from django.conf import settings
from xmodule.course_module import CourseDescriptor
+from xmodule.error_module import ErrorDescriptor
from xmodule.modulestore import Location
from xmodule.timeparse import parse_time
from xmodule.x_module import XModule, XModuleDescriptor
@@ -49,6 +50,10 @@ def has_access(user, obj, action):
if isinstance(obj, CourseDescriptor):
return _has_access_course_desc(user, obj, action)
+ if isinstance(obj, ErrorDescriptor):
+ return _has_access_error_desc(user, obj, action)
+
+ # NOTE: any descriptor access checkers need to go above this
if isinstance(obj, XModuleDescriptor):
return _has_access_descriptor(user, obj, action)
@@ -94,6 +99,7 @@ def _has_access_course_desc(user, course, action):
if (start is None or now > start) and (end is None or now < end):
# in enrollment period, so any user is allowed to enroll.
+ debug("Allow: in enrollment period")
return True
# otherwise, need staff access
@@ -115,6 +121,7 @@ def _has_access_course_desc(user, course, action):
# if this feature is on, only allow courses that have ispublic set to be
# seen by non-staff
if course.metadata.get('ispublic'):
+ debug("Allow: ACCESS_REQUIRE_STAFF_FOR_COURSE and ispublic")
return True
return _has_staff_access_to_descriptor(user, course)
@@ -130,6 +137,25 @@ def _has_access_course_desc(user, course, action):
return _dispatch(checkers, action, user, course)
+def _has_access_error_desc(user, descriptor, action):
+ """
+ Only staff should see error descriptors.
+
+ Valid actions:
+ 'load' -- load this descriptor, showing it to the user.
+ 'staff' -- staff access to descriptor.
+ """
+ def check_for_staff():
+ return _has_staff_access_to_descriptor(user, descriptor)
+
+ checkers = {
+ 'load': check_for_staff,
+ 'staff': check_for_staff
+ }
+
+ return _dispatch(checkers, action, user, descriptor)
+
+
def _has_access_descriptor(user, descriptor, action):
"""
Check if user has access to this descriptor.
@@ -145,6 +171,7 @@ def _has_access_descriptor(user, descriptor, action):
def can_load():
# If start dates are off, can always load
if settings.MITX_FEATURES['DISABLE_START_DATES']:
+ debug("Allow: DISABLE_START_DATES")
return True
# Check start date
@@ -152,11 +179,13 @@ def _has_access_descriptor(user, descriptor, action):
now = time.gmtime()
if now > descriptor.start:
# after start date, everyone can see it
+ debug("Allow: now > start date")
return True
# otherwise, need staff access
return _has_staff_access_to_descriptor(user, descriptor)
# No start date, so can always load.
+ debug("Allow: no start date")
return True
checkers = {
@@ -212,7 +241,9 @@ def _dispatch(table, action, user, obj):
result = table[action]()
debug("%s user %s, object %s, action %s",
'ALLOWED' if result else 'DENIED',
- user, obj, action)
+ user,
+ obj.location.url() if isinstance(obj, XModuleDescriptor) else str(obj)[:60],
+ action)
return result
raise ValueError("Unknown action for object type '{}': '{}'".format(
@@ -239,15 +270,19 @@ def _has_staff_access_to_location(user, location):
course is a string: the course field of the location being accessed.
'''
if user is None or (not user.is_authenticated()):
+ debug("Deny: no user or anon user")
return False
if user.is_staff:
+ debug("Allow: user.is_staff")
return True
# If not global staff, is the user in the Auth group for this class?
user_groups = [x[1] for x in user.groups.values_list()]
staff_group = _course_staff_group_name(location)
if staff_group in user_groups:
+ debug("Allow: user in group %s", staff_group)
return True
+ debug("Deny: user not in group %s", staff_group)
return False
def _has_staff_access_to_course_id(user, course_id):
diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py
index aa160ee22a..8417bf9a48 100644
--- a/lms/djangoapps/courseware/grades.py
+++ b/lms/djangoapps/courseware/grades.py
@@ -63,7 +63,12 @@ def grade(student, request, course, student_module_cache=None):
scores = []
# TODO: We need the request to pass into here. If we could forgo that, our arguments
# would be simpler
- section_module = get_module(student, request, section_descriptor.location, student_module_cache)
+ section_module = get_module(student, request,
+ section_descriptor.location, student_module_cache)
+ if section_module is None:
+ # student doesn't have access to this module, or something else
+ # went wrong.
+ continue
# TODO: We may be able to speed this up by only getting a list of children IDs from section_module
# Then, we may not need to instatiate any problems if they are already in the database
diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py
index 768139c2f0..86ccba7345 100644
--- a/lms/djangoapps/courseware/module_render.py
+++ b/lms/djangoapps/courseware/module_render.py
@@ -65,6 +65,9 @@ def toc_for_course(user, request, course, active_chapter, active_section):
Everything else comes from the xml, or defaults to "".
chapters with name 'hidden' are skipped.
+
+ NOTE: assumes that if we got this far, user has access to course. Returns
+ None if this is not the case.
'''
student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(user, course, depth=2)
@@ -144,6 +147,11 @@ def get_module(user, request, location, student_module_cache, position=None):
location = Location(location)
descriptor = modulestore().get_item(location)
+ # Short circuit--if the user shouldn't have access, bail without doing any work
+ if not has_access(user, descriptor, 'load'):
+ return None
+
+
#TODO Only check the cache if this module can possibly have state
instance_module = None
shared_module = None
@@ -192,6 +200,9 @@ def get_module(user, request, location, student_module_cache, position=None):
'default_queuename': xqueue_default_queuename.replace(' ', '_')}
def _get_module(location):
+ """
+ Delegate to get_module. It does an access check, so may return None
+ """
return get_module(user, request, location,
student_module_cache, position)
@@ -308,10 +319,14 @@ def xqueue_callback(request, course_id, userid, id, dispatch):
student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(user, modulestore().get_item(id))
instance = get_module(user, request, id, student_module_cache)
+ if instance is None:
+ log.debug("No module {} for user {}--access denied?".format(id, user))
+ raise Http404
+
instance_module = get_instance_module(user, instance, student_module_cache)
if instance_module is None:
- log.debug("Couldn't find module '%s' for user '%s'", id, user)
+ log.debug("Couldn't find instance of module '%s' for user '%s'", id, user)
raise Http404
oldgrade = instance_module.grade
@@ -365,6 +380,11 @@ def modx_dispatch(request, dispatch=None, id=None, course_id=None):
student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(request.user, modulestore().get_item(id))
instance = get_module(request.user, request, id, student_module_cache)
+ if instance is None:
+ # Either permissions just changed, or someone is trying to be clever
+ # and load something they shouldn't have access to.
+ log.debug("No module {} for user {}--access denied?".format(id, user))
+ raise Http404
instance_module = get_instance_module(request.user, instance, student_module_cache)
shared_module = get_shared_instance_module(request.user, instance, student_module_cache)
diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py
index 8eb9c22584..92ddb2767e 100644
--- a/lms/djangoapps/courseware/tests/tests.py
+++ b/lms/djangoapps/courseware/tests/tests.py
@@ -25,7 +25,7 @@ from student.models import Registration
from xmodule.modulestore.django import modulestore
from xmodule.modulestore import Location
from xmodule.modulestore.xml_importer import import_from_xml
-
+from xmodule.timeparse import stringify_time
def parse_json(response):
"""Parse response, which is assumed to be json"""
@@ -371,8 +371,8 @@ class TestViewAuth(PageLoader):
# Make courses start in the future
tomorrow = time.time() + 24*3600
- self.toy.start = self.toy.metadata['start'] = time.gmtime(tomorrow)
- self.full.start = self.full.metadata['start'] = time.gmtime(tomorrow)
+ self.toy.metadata['start'] = stringify_time(time.gmtime(tomorrow))
+ self.full.metadata['start'] = stringify_time(time.gmtime(tomorrow))
self.assertFalse(self.toy.has_started())
self.assertFalse(self.full.has_started())
diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py
index f54a76da1f..97bafb3e0f 100644
--- a/lms/djangoapps/courseware/views.py
+++ b/lms/djangoapps/courseware/views.py
@@ -136,6 +136,10 @@ def index(request, course_id, chapter=None, section=None,
module = get_module(request.user, request,
section_descriptor.location,
student_module_cache)
+ if module is None:
+ # User is probably being clever and trying to access something
+ # they don't have access to.
+ raise Http404
context['content'] = module.get_html()
else:
log.warning("Couldn't find a section descriptor for course_id '{0}',"
diff --git a/lms/templates/module-error.html b/lms/templates/module-error.html
index 7c731db17a..2a51f5b11a 100644
--- a/lms/templates/module-error.html
+++ b/lms/templates/module-error.html
@@ -2,12 +2,10 @@
There has been an error on the MITx servers
We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible. Please email us at technical@mitx.mit.edu to report any problems or downtime.
-% if is_staff:
-Staff-only details below:
+Details below:
Error: ${error | h}
Raw data: ${data | h}
-% endif