diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 8a96d2533f..6c3db9bc18 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/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 - (This is important e.g. for prof impersonation of students in progress view) + - request : current django HTTPrequest. Note: request.user isn't used for anything--all auth + 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 - # impersonating a user) - if not has_access(request.user, descriptor, 'load'): + if not has_access(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(): diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index bb48dce609..88e0f53f70 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/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) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 50b7a2d645..92f6716320 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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)