Merge pull request #3657 from edx/opaque-keys-access

Don't fetch the course just to find out if the user has access to it.

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'

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