Commit 57ea923b by Victor Shnayder

Make start dates work for all modules

* error modules now hidden via access control
* get_module() returns None if user doesn't have access
parent b5f7efd6
...@@ -49,9 +49,9 @@ class ABTestModule(XModule): ...@@ -49,9 +49,9 @@ class ABTestModule(XModule):
return json.dumps({'group': self.group}) return json.dumps({'group': self.group})
def displayable_items(self): def displayable_items(self):
return [self.system.get_module(child) return filter(None, [self.system.get_module(child)
for child for child
in self.definition['data']['group_content'][self.group]] in self.definition['data']['group_content'][self.group]])
# TODO (cpennington): Use Groups should be a first class object, rather than being # TODO (cpennington): Use Groups should be a first class object, rather than being
......
...@@ -6,7 +6,7 @@ from xmodule.util.decorators import lazyproperty ...@@ -6,7 +6,7 @@ from xmodule.util.decorators import lazyproperty
from xmodule.graders import load_grading_policy from xmodule.graders import load_grading_policy
from xmodule.modulestore import Location from xmodule.modulestore import Location
from xmodule.seq_module import SequenceDescriptor, SequenceModule 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__) log = logging.getLogger(__name__)
...@@ -18,16 +18,10 @@ class CourseDescriptor(SequenceDescriptor): ...@@ -18,16 +18,10 @@ class CourseDescriptor(SequenceDescriptor):
super(CourseDescriptor, self).__init__(system, definition, **kwargs) super(CourseDescriptor, self).__init__(system, definition, **kwargs)
msg = None msg = None
try: if self.start is None:
self.start = parse_time(self.metadata["start"]) msg = "Course loaded without a valid start date. id = %s" % self.id
except KeyError: # hack it -- start in 1970
msg = "Course loaded without a start date. id = %s" % self.id self.metadata['start'] = stringify_time(time.gmtime(0))
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
log.critical(msg) log.critical(msg)
system.error_tracker(msg) system.error_tracker(msg)
......
...@@ -24,16 +24,8 @@ class ErrorModule(XModule): ...@@ -24,16 +24,8 @@ class ErrorModule(XModule):
return self.system.render_template('module-error.html', { return self.system.render_template('module-error.html', {
'data' : self.definition['data']['contents'], 'data' : self.definition['data']['contents'],
'error' : self.definition['data']['error_msg'], '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): class ErrorDescriptor(EditingDescriptor):
""" """
......
...@@ -3,9 +3,17 @@ Helper functions for handling time in the format we like. ...@@ -3,9 +3,17 @@ Helper functions for handling time in the format we like.
""" """
import time import time
TIME_FORMAT = "%Y-%m-%dT%H:%M"
def parse_time(time_str): 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. 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)
...@@ -219,9 +219,11 @@ class XModule(HTMLSnippet): ...@@ -219,9 +219,11 @@ class XModule(HTMLSnippet):
Return module instances for all the children of this module. Return module instances for all the children of this module.
''' '''
if self._loaded_children is None: if self._loaded_children is None:
self._loaded_children = [ # get_module returns None if the current user doesn't have access
self.system.get_module(child) # to the location.
for child in self.definition.get('children', [])] self._loaded_children = filter(None,
[self.system.get_module(child)
for child in self.definition.get('children', [])])
return self._loaded_children return self._loaded_children
...@@ -385,9 +387,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet): ...@@ -385,9 +387,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
self.category = self.location.category self.category = self.location.category
self.shared_state_key = kwargs.get('shared_state_key') 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._child_instances = None
self._inherited_metadata = set() self._inherited_metadata = set()
...@@ -401,6 +400,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet): ...@@ -401,6 +400,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
self.url_name.replace('_', ' ')) self.url_name.replace('_', ' '))
@property @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): def own_metadata(self):
""" """
Return the metadata that is not inherited, but was defined on this module. Return the metadata that is not inherited, but was defined on this module.
...@@ -609,7 +617,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): ...@@ -609,7 +617,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
""" """
if key in self.metadata: if key in self.metadata:
try: try:
parse_time(self.metadata[key]) return parse_time(self.metadata[key])
except ValueError as e: except ValueError as e:
msg = "Descriptor {} loaded with a bad metadata key '{}': '{}'".format( msg = "Descriptor {} loaded with a bad metadata key '{}': '{}'".format(
self.location.url(), self.metadata[key], e) self.location.url(), self.metadata[key], e)
...@@ -709,7 +717,8 @@ class ModuleSystem(object): ...@@ -709,7 +717,8 @@ class ModuleSystem(object):
files. Update or remove. files. Update or remove.
get_module - function that takes (location) and returns a corresponding 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 render_template - a function that takes (template_file, context), and
returns rendered html. returns rendered html.
......
...@@ -8,6 +8,7 @@ import time ...@@ -8,6 +8,7 @@ import time
from django.conf import settings from django.conf import settings
from xmodule.course_module import CourseDescriptor from xmodule.course_module import CourseDescriptor
from xmodule.error_module import ErrorDescriptor
from xmodule.modulestore import Location from xmodule.modulestore import Location
from xmodule.timeparse import parse_time from xmodule.timeparse import parse_time
from xmodule.x_module import XModule, XModuleDescriptor from xmodule.x_module import XModule, XModuleDescriptor
...@@ -49,6 +50,10 @@ def has_access(user, obj, action): ...@@ -49,6 +50,10 @@ def has_access(user, obj, action):
if isinstance(obj, CourseDescriptor): if isinstance(obj, CourseDescriptor):
return _has_access_course_desc(user, obj, action) 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): if isinstance(obj, XModuleDescriptor):
return _has_access_descriptor(user, obj, action) return _has_access_descriptor(user, obj, action)
...@@ -94,6 +99,7 @@ def _has_access_course_desc(user, course, 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): if (start is None or now > start) and (end is None or now < end):
# in enrollment period, so any user is allowed to enroll. # in enrollment period, so any user is allowed to enroll.
debug("Allow: in enrollment period")
return True return True
# otherwise, need staff access # otherwise, need staff access
...@@ -115,6 +121,7 @@ def _has_access_course_desc(user, course, action): ...@@ -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 # if this feature is on, only allow courses that have ispublic set to be
# seen by non-staff # seen by non-staff
if course.metadata.get('ispublic'): if course.metadata.get('ispublic'):
debug("Allow: ACCESS_REQUIRE_STAFF_FOR_COURSE and ispublic")
return True return True
return _has_staff_access_to_descriptor(user, course) return _has_staff_access_to_descriptor(user, course)
...@@ -130,6 +137,25 @@ def _has_access_course_desc(user, course, action): ...@@ -130,6 +137,25 @@ def _has_access_course_desc(user, course, action):
return _dispatch(checkers, action, user, course) 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): def _has_access_descriptor(user, descriptor, action):
""" """
Check if user has access to this descriptor. Check if user has access to this descriptor.
...@@ -145,6 +171,7 @@ def _has_access_descriptor(user, descriptor, action): ...@@ -145,6 +171,7 @@ def _has_access_descriptor(user, descriptor, action):
def can_load(): def can_load():
# If start dates are off, can always load # If start dates are off, can always load
if settings.MITX_FEATURES['DISABLE_START_DATES']: if settings.MITX_FEATURES['DISABLE_START_DATES']:
debug("Allow: DISABLE_START_DATES")
return True return True
# Check start date # Check start date
...@@ -152,11 +179,13 @@ def _has_access_descriptor(user, descriptor, action): ...@@ -152,11 +179,13 @@ def _has_access_descriptor(user, descriptor, action):
now = time.gmtime() now = time.gmtime()
if now > descriptor.start: if now > descriptor.start:
# after start date, everyone can see it # after start date, everyone can see it
debug("Allow: now > start date")
return True return True
# otherwise, need staff access # otherwise, need staff access
return _has_staff_access_to_descriptor(user, descriptor) return _has_staff_access_to_descriptor(user, descriptor)
# No start date, so can always load. # No start date, so can always load.
debug("Allow: no start date")
return True return True
checkers = { checkers = {
...@@ -212,7 +241,9 @@ def _dispatch(table, action, user, obj): ...@@ -212,7 +241,9 @@ def _dispatch(table, action, user, obj):
result = table[action]() result = table[action]()
debug("%s user %s, object %s, action %s", debug("%s user %s, object %s, action %s",
'ALLOWED' if result else 'DENIED', 'ALLOWED' if result else 'DENIED',
user, obj, action) user,
obj.location.url() if isinstance(obj, XModuleDescriptor) else str(obj)[:60],
action)
return result return result
raise ValueError("Unknown action for object type '{}': '{}'".format( raise ValueError("Unknown action for object type '{}': '{}'".format(
...@@ -239,15 +270,19 @@ def _has_staff_access_to_location(user, location): ...@@ -239,15 +270,19 @@ def _has_staff_access_to_location(user, location):
course is a string: the course field of the location being accessed. course is a string: the course field of the location being accessed.
''' '''
if user is None or (not user.is_authenticated()): if user is None or (not user.is_authenticated()):
debug("Deny: no user or anon user")
return False return False
if user.is_staff: if user.is_staff:
debug("Allow: user.is_staff")
return True return True
# If not global staff, is the user in the Auth group for this class? # 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()] user_groups = [x[1] for x in user.groups.values_list()]
staff_group = _course_staff_group_name(location) staff_group = _course_staff_group_name(location)
if staff_group in user_groups: if staff_group in user_groups:
debug("Allow: user in group %s", staff_group)
return True return True
debug("Deny: user not in group %s", staff_group)
return False return False
def _has_staff_access_to_course_id(user, course_id): def _has_staff_access_to_course_id(user, course_id):
......
...@@ -63,7 +63,12 @@ def grade(student, request, course, student_module_cache=None): ...@@ -63,7 +63,12 @@ def grade(student, request, course, student_module_cache=None):
scores = [] scores = []
# TODO: We need the request to pass into here. If we could forgo that, our arguments # TODO: We need the request to pass into here. If we could forgo that, our arguments
# would be simpler # 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 # 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 # Then, we may not need to instatiate any problems if they are already in the database
......
...@@ -65,6 +65,9 @@ def toc_for_course(user, request, course, active_chapter, active_section): ...@@ -65,6 +65,9 @@ def toc_for_course(user, request, course, active_chapter, active_section):
Everything else comes from the xml, or defaults to "". Everything else comes from the xml, or defaults to "".
chapters with name 'hidden' are skipped. 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) 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): ...@@ -144,6 +147,11 @@ def get_module(user, request, location, student_module_cache, position=None):
location = Location(location) location = Location(location)
descriptor = modulestore().get_item(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 #TODO Only check the cache if this module can possibly have state
instance_module = None instance_module = None
shared_module = None shared_module = None
...@@ -192,6 +200,9 @@ def get_module(user, request, location, student_module_cache, position=None): ...@@ -192,6 +200,9 @@ def get_module(user, request, location, student_module_cache, position=None):
'default_queuename': xqueue_default_queuename.replace(' ', '_')} 'default_queuename': xqueue_default_queuename.replace(' ', '_')}
def _get_module(location): def _get_module(location):
"""
Delegate to get_module. It does an access check, so may return None
"""
return get_module(user, request, location, return get_module(user, request, location,
student_module_cache, position) student_module_cache, position)
...@@ -308,10 +319,14 @@ def xqueue_callback(request, course_id, userid, id, dispatch): ...@@ -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)) student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(user, modulestore().get_item(id))
instance = get_module(user, request, id, student_module_cache) 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) instance_module = get_instance_module(user, instance, student_module_cache)
if instance_module is None: 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 raise Http404
oldgrade = instance_module.grade oldgrade = instance_module.grade
...@@ -365,6 +380,11 @@ def modx_dispatch(request, dispatch=None, id=None, course_id=None): ...@@ -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)) student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(request.user, modulestore().get_item(id))
instance = get_module(request.user, request, id, student_module_cache) 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) instance_module = get_instance_module(request.user, instance, student_module_cache)
shared_module = get_shared_instance_module(request.user, instance, student_module_cache) shared_module = get_shared_instance_module(request.user, instance, student_module_cache)
......
...@@ -25,7 +25,7 @@ from student.models import Registration ...@@ -25,7 +25,7 @@ from student.models import Registration
from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import modulestore
from xmodule.modulestore import Location from xmodule.modulestore import Location
from xmodule.modulestore.xml_importer import import_from_xml from xmodule.modulestore.xml_importer import import_from_xml
from xmodule.timeparse import stringify_time
def parse_json(response): def parse_json(response):
"""Parse response, which is assumed to be json""" """Parse response, which is assumed to be json"""
...@@ -371,8 +371,8 @@ class TestViewAuth(PageLoader): ...@@ -371,8 +371,8 @@ class TestViewAuth(PageLoader):
# Make courses start in the future # Make courses start in the future
tomorrow = time.time() + 24*3600 tomorrow = time.time() + 24*3600
self.toy.start = self.toy.metadata['start'] = time.gmtime(tomorrow) self.toy.metadata['start'] = stringify_time(time.gmtime(tomorrow))
self.full.start = self.full.metadata['start'] = time.gmtime(tomorrow) self.full.metadata['start'] = stringify_time(time.gmtime(tomorrow))
self.assertFalse(self.toy.has_started()) self.assertFalse(self.toy.has_started())
self.assertFalse(self.full.has_started()) self.assertFalse(self.full.has_started())
......
...@@ -136,6 +136,10 @@ def index(request, course_id, chapter=None, section=None, ...@@ -136,6 +136,10 @@ def index(request, course_id, chapter=None, section=None,
module = get_module(request.user, request, module = get_module(request.user, request,
section_descriptor.location, section_descriptor.location,
student_module_cache) 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() context['content'] = module.get_html()
else: else:
log.warning("Couldn't find a section descriptor for course_id '{0}'," log.warning("Couldn't find a section descriptor for course_id '{0}',"
......
...@@ -2,12 +2,10 @@ ...@@ -2,12 +2,10 @@
<h1>There has been an error on the <em>MITx</em> servers</h1> <h1>There has been an error on the <em>MITx</em> servers</h1>
<p>We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible. Please email us at <a href="mailto:technical@mitx.mit.edu">technical@mitx.mit.edu</a> to report any problems or downtime.</p> <p>We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible. Please email us at <a href="mailto:technical@mitx.mit.edu">technical@mitx.mit.edu</a> to report any problems or downtime.</p>
% if is_staff: <h1>Details below:</h1>
<h1>Staff-only details below:</h1>
<p>Error: ${error | h}</p> <p>Error: ${error | h}</p>
<p>Raw data: ${data | h}</p> <p>Raw data: ${data | h}</p>
% endif
</section> </section>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment