Merge pull request #565 from MITx/feature/victor/fix-queue-access-control

Fix latent bug in access checks in get_module

lms/djangoapps/courseware/module_render.py

@@ -144,8 +144,8 @@ def get_module(user, request, location, student_module_cache, course_id, positio
 
     Arguments:
       - user                  : User for whom we're getting the module
-      - request               : current django HTTPrequest -- used in particular for auth
+      - request               : current django HTTPrequest.  Note: request.user isn't used for anything--all auth
-                                (This is important e.g. for prof impersonation of students in progress view)
+                                and such works based on user.
       - location              : A Location-like object identifying the module to load
       - student_module_cache  : a StudentModuleCache
       - course_id             : the course_id in the context of which to load module
@@ -171,12 +171,10 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
     descriptor = modulestore().get_instance(course_id, location)
 
     # Short circuit--if the user shouldn't have access, bail without doing any work
-    # NOTE: Do access check on request.user -- that's who actually needs access (e.g. could be prof
+    if not has_access(user, descriptor, 'load'):
-    # impersonating a user)
-    if not has_access(request.user, descriptor, 'load'):
         return None
 
-    #TODO Only check the cache if this module can possibly have state
+    # Only check the cache if this module can possibly have state
     instance_module = None
     shared_module = None
     if user.is_authenticated():

lms/djangoapps/courseware/tests/tests.py

@@ -411,8 +411,6 @@ class TestViewAuth(PageLoader):
             """list of urls that only instructors/staff should be able to see"""
             urls = reverse_urls(['instructor_dashboard','gradebook','grade_summary'],
                                 course)
-            urls.append(reverse('student_progress', kwargs={'course_id': course.id,
-                                                     'student_id': user(self.student).id}))
             return urls
 
         def check_non_staff(course):
@@ -435,6 +433,17 @@ class TestViewAuth(PageLoader):
                 print 'checking for 200 on {0}'.format(url)
                 self.check_for_get_code(200, url)
 
+            # The student progress tab is not accessible to a student
+            # before launch, so the instructor view-as-student feature should return a 404 as well.
+            # TODO (vshnayder): If this is not the behavior we want, will need
+            # to make access checking smarter and understand both the effective
+            # user (the student), and the requesting user (the prof)
+            url = reverse('student_progress', kwargs={'course_id': course.id,
+                                                     'student_id': user(self.student).id})
+            print 'checking for 404 on view-as-student: {0}'.format(url)
+            self.check_for_get_code(404, url)
+
+
         # First, try with an enrolled student
         print '=== Testing student access....'
         self.login(self.student, self.password)

lms/djangoapps/courseware/views.py

@@ -333,6 +333,10 @@ def progress(request, course_id, student_id=None):
     course_module = get_module(student, request, course.location,
                                student_module_cache, course_id)
 
+    # The course_module should be accessible, but check anyway just in case something went wrong:
+    if course_module is None:
+        raise Http404("Course does not exist")
+
     courseware_summary = grades.progress_summary(student, course_module,
                                                  course.grader, student_module_cache)
     grade_summary = grades.grade(student, request, course, student_module_cache)