Make start dates work for all modules

* error modules now hidden via access control
* get_module() returns None if user doesn't have access

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

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)
 

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

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)

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()
 
@@ -401,6 +400,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
                                  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):
         """
         Return the metadata that is not inherited, but was defined on this module.
@@ -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.

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

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

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)

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

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}',"

lms/templates/module-error.html

@@ -2,12 +2,10 @@
   <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>
 
-% if is_staff:
-<h1>Staff-only details below:</h1>
+<h1>Details below:</h1>
 
 <p>Error: ${error | h}</p>
 
 <p>Raw data: ${data | h}</p>
-% endif
 
 </section>