diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index f6543323ed..686e9968ed 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -23,6 +23,7 @@ from student.roles import ( GlobalStaff, CourseStaffRole, CourseInstructorRole, OrgStaffRole, OrgInstructorRole, CourseBetaTesterRole ) +from xmodule.modulestore.keys import CourseKey DEBUG_ACCESS = False log = logging.getLogger(__name__) @@ -81,6 +82,9 @@ def has_access(user, action, obj, course_key=None): if isinstance(obj, XBlock): return _has_access_descriptor(user, action, obj, course_key) + if isinstance(obj, CourseKey): + return _has_access_course_key(user, action, obj) + if isinstance(obj, Location): return _has_access_location(user, action, obj, course_key) @@ -297,8 +301,6 @@ def _has_access_location(user, action, location, course_key): NOTE: if you add other actions, make sure that has_access(user, location, action) == has_access(user, get_item(location), action) - - And in general, prefer checking access on loaded items, rather than locations. """ checkers = { 'staff': lambda: _has_staff_access_to_location(user, location, course_key) @@ -307,6 +309,22 @@ def _has_access_location(user, action, location, course_key): return _dispatch(checkers, action, user, location) +def _has_access_course_key(user, action, course_key): + """ + Check if user has access to the course with this course_key + + Valid actions: + 'staff' : True if the user has staff access to this location + 'instructor' : True if the user has staff access to this location + """ + checkers = { + 'staff': lambda: _has_staff_access_to_location(user, None, course_key), + 'instructor': lambda: _has_instructor_access_to_location(user, None, course_key), + } + + return _dispatch(checkers, action, user, course_key) + + def _has_access_string(user, action, perm, course_key): """ Check if user has certain special access, specified as string. Valid strings: @@ -388,27 +406,25 @@ def _adjust_start_date_for_beta_testers(user, descriptor, course_key=None): # p return descriptor.start -def _has_instructor_access_to_location(user, location, course_key=None): # pylint: disable=invalid-name - return _has_access_to_location(user, 'instructor', location, course_key) +def _has_instructor_access_to_location(user, location, course_key=None): + if course_key is None: + course_key = location.course_key + return _has_access_to_course(user, 'instructor', course_key) def _has_staff_access_to_location(user, location, course_key=None): - return _has_access_to_location(user, 'staff', location, course_key) + if course_key is None: + course_key = location.course_key + return _has_access_to_course(user, 'staff', course_key) -def _has_access_to_location(user, access_level, location, course_key): +def _has_access_to_course(user, access_level, course_key): ''' Returns True if the given user has access_level (= staff or - instructor) access to a location. For now this is equivalent to - having staff / instructor access to the course location.course. + instructor) access to the course with the given course_key. + This ensures the user is authenticated and checks if global staff or has + staff / instructor access. - This means that user is in the staff_* group or instructor_* group, or is an overall admin. - - TODO (vshnayder): this needs to be changed to allow per-course_key permissions, not per-course - (e.g. staff in 2012 is different from 2013, but maybe some people always have access) - - course is a string: the course field of the location being accessed. - location = location access_level = string, either "staff" or "instructor" ''' if user is None or (not user.is_authenticated()): @@ -423,7 +439,7 @@ def _has_access_to_location(user, access_level, location, course_key): return True if access_level not in ('staff', 'instructor'): - log.debug("Error in access._has_access_to_location access_level=%s unknown", access_level) + log.debug("Error in access._has_access_to_course access_level=%s unknown", access_level) debug("Deny: unknown access level") return False @@ -449,13 +465,6 @@ def _has_access_to_location(user, access_level, location, course_key): return False -# TODO Please change this function signature to _has_staff_access_to_course_key at next opportunity! -def _has_staff_access_to_course_id(user, course_key): - """Helper method that takes a course_key instead of a course name""" - loc = CourseDescriptor.id_to_location(course_key) - return _has_staff_access_to_location(user, loc, course_key) - - def _has_instructor_access_to_descriptor(user, descriptor, course_key): # pylint: disable=invalid-name """Helper method that checks whether the user has staff access to the course of the location. @@ -479,13 +488,11 @@ def get_user_role(user, course_key): Return corresponding string if user has staff, instructor or student course role in LMS. """ - from courseware.courses import get_course - course = get_course(course_key) if is_masquerading_as_student(user): return 'student' - elif has_access(user, 'instructor', course): + elif has_access(user, 'instructor', course_key): return 'instructor' - elif has_access(user, 'staff', course): + elif has_access(user, 'staff', course_key): return 'staff' else: return 'student' diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 51225f389d..ad1bb3b796 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -8,7 +8,6 @@ from django.test.utils import override_settings from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory from student.tests.factories import AnonymousUserFactory, CourseEnrollmentAllowedFactory -from xmodule.modulestore import Location from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE import pytz from xmodule.modulestore.locations import SlashSeparatedCourseKey @@ -31,48 +30,48 @@ class AccessTestCase(TestCase): self.course_staff = StaffFactory(course=self.course.course_key) self.course_instructor = InstructorFactory(course=self.course.course_key) - def test__has_access_to_location(self): - self.assertFalse(access._has_access_to_location( - None, 'staff', self.course, self.course.course_key + def test_has_access_to_course(self): + self.assertFalse(access._has_access_to_course( + None, 'staff', self.course.course_key )) - self.assertFalse(access._has_access_to_location( - self.anonymous_user, 'staff', self.course, self.course.course_key + self.assertFalse(access._has_access_to_course( + self.anonymous_user, 'staff', self.course.course_key )) - self.assertFalse(access._has_access_to_location( - self.anonymous_user, 'instructor', self.course, self.course.course_key + self.assertFalse(access._has_access_to_course( + self.anonymous_user, 'instructor', self.course.course_key )) - self.assertTrue(access._has_access_to_location( - self.global_staff, 'staff', self.course, self.course.course_key + self.assertTrue(access._has_access_to_course( + self.global_staff, 'staff', self.course.course_key )) - self.assertTrue(access._has_access_to_location( - self.global_staff, 'instructor', self.course, self.course.course_key + self.assertTrue(access._has_access_to_course( + self.global_staff, 'instructor', self.course.course_key )) # A user has staff access if they are in the staff group - self.assertTrue(access._has_access_to_location( - self.course_staff, 'staff', self.course, self.course.course_key + self.assertTrue(access._has_access_to_course( + self.course_staff, 'staff', self.course.course_key )) - self.assertFalse(access._has_access_to_location( - self.course_staff, 'instructor', self.course, self.course.course_key + self.assertFalse(access._has_access_to_course( + self.course_staff, 'instructor', self.course.course_key )) # A user has staff and instructor access if they are in the instructor group - self.assertTrue(access._has_access_to_location( - self.course_instructor, 'staff', self.course, self.course.course_key + self.assertTrue(access._has_access_to_course( + self.course_instructor, 'staff', self.course.course_key )) - self.assertTrue(access._has_access_to_location( - self.course_instructor, 'instructor', self.course, self.course.course_key + self.assertTrue(access._has_access_to_course( + self.course_instructor, 'instructor', self.course.course_key )) # A user does not have staff or instructor access if they are # not in either the staff or the the instructor group - self.assertFalse(access._has_access_to_location( - self.student, 'staff', self.course, self.course.course_key + self.assertFalse(access._has_access_to_course( + self.student, 'staff', self.course.course_key )) - self.assertFalse(access._has_access_to_location( - self.student, 'instructor', self.course, self.course.course_key + self.assertFalse(access._has_access_to_course( + self.student, 'instructor', self.course.course_key )) def test__has_access_string(self):