Merge pull request #11444 from mitocw/gdm_permission_fix_#193

Modified permission classes for CCX REST APIs

lms/djangoapps/ccx/api/v0/views.py

@@ -23,7 +23,7 @@ from instructor.enrollment import (
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
-from openedx.core.lib.api.permissions import IsCourseInstructor
+from openedx.core.lib.api import permissions
 from student.models import CourseEnrollment
 from student.roles import CourseCcxCoachRole
 
@@ -301,7 +301,7 @@ class CCXListView(GenericAPIView):
             }
     """
     authentication_classes = (OAuth2Authentication, SessionAuthentication,)
-    permission_classes = (IsAuthenticated, IsCourseInstructor)
+    permission_classes = (IsAuthenticated, permissions.IsMasterCourseStaffInstructor)
     serializer_class = CCXCourseSerializer
     pagination_class = CCXAPIPagination
 
@@ -510,9 +510,17 @@ class CCXDetailView(GenericAPIView):
     """
 
     authentication_classes = (OAuth2Authentication, SessionAuthentication,)
-    permission_classes = (IsAuthenticated, IsCourseInstructor)
+    permission_classes = (IsAuthenticated, permissions.IsCourseStaffInstructor)
     serializer_class = CCXCourseSerializer
 
+    def get_object(self, course_id, is_ccx=False):  # pylint: disable=arguments-differ
+        """
+        Override the default get_object to allow a custom getter for the CCX
+        """
+        course_object, course_key, error_code, http_status = get_valid_course(course_id, is_ccx)
+        self.check_object_permissions(self.request, course_object)
+        return course_object, course_key, error_code, http_status
+
     def get(self, request, ccx_course_id=None):
         """
         Gets a CCX Course information.
@@ -524,7 +532,7 @@ class CCXDetailView(GenericAPIView):
         Return:
             A JSON serialized representation of the CCX course.
         """
-        ccx_course_object, _, error_code, http_status = get_valid_course(ccx_course_id, is_ccx=True)
+        ccx_course_object, _, error_code, http_status = self.get_object(ccx_course_id, is_ccx=True)
         if ccx_course_object is None:
             return Response(
                 status=http_status,
@@ -543,7 +551,7 @@ class CCXDetailView(GenericAPIView):
             request (Request): Django request object.
             ccx_course_id (string): URI element specifying the CCX course location.
         """
-        ccx_course_object, ccx_course_key, error_code, http_status = get_valid_course(ccx_course_id, is_ccx=True)
+        ccx_course_object, ccx_course_key, error_code, http_status = self.get_object(ccx_course_id, is_ccx=True)
         if ccx_course_object is None:
             return Response(
                 status=http_status,
@@ -571,7 +579,7 @@ class CCXDetailView(GenericAPIView):
             request (Request): Django request object.
             ccx_course_id (string): URI element specifying the CCX course location.
         """
-        ccx_course_object, ccx_course_key, error_code, http_status = get_valid_course(ccx_course_id, is_ccx=True)
+        ccx_course_object, ccx_course_key, error_code, http_status = self.get_object(ccx_course_id, is_ccx=True)
         if ccx_course_object is None:
             return Response(
                 status=http_status,

openedx/core/lib/api/permissions.py

@@ -6,6 +6,8 @@ from django.conf import settings
 from django.http import Http404
 from rest_framework import permissions
 
+from opaque_keys import InvalidKeyError
+from opaque_keys.edx.keys import CourseKey
 from student.roles import CourseStaffRole, CourseInstructorRole
 
 
@@ -64,13 +66,49 @@ class IsUserInUrl(permissions.BasePermission):
         return True
 
 
-class IsCourseInstructor(permissions.BasePermission):
+class IsCourseStaffInstructor(permissions.BasePermission):
     """
-    Permission to check that user is a course instructor.
+    Permission to check that user is a course instructor or staff of
+    a master course given a course object or the user is a coach of
+    the course itself.
     """
 
     def has_object_permission(self, request, view, obj):
-        return hasattr(request, 'user') and CourseInstructorRole(obj.course_id).has_user(request.user)
+        return (hasattr(request, 'user') and
+                # either the user is a staff or instructor of the master course
+                (hasattr(obj, 'course_id') and
+                 (CourseInstructorRole(obj.course_id).has_user(request.user) or
+                  CourseStaffRole(obj.course_id).has_user(request.user))) or
+                # or it is a safe method and the user is a coach on the course object
+                (request.method in permissions.SAFE_METHODS
+                 and hasattr(obj, 'coach') and obj.coach == request.user))
+
+
+class IsMasterCourseStaffInstructor(permissions.BasePermission):
+    """
+    Permission to check that user is instructor or staff of the master course.
+    """
+    def has_permission(self, request, view):
+        """
+        This method is assuming that a `master_course_id` parameter
+        is available in the request as a GET parameter, a POST parameter
+        or it is in the JSON payload included in the request.
+        The reason is because this permission class is going
+        to check if the user making the request is an instructor
+        for the specified course.
+        """
+        master_course_id = (request.GET.get('master_course_id')
+                            or request.POST.get('master_course_id')
+                            or request.data.get('master_course_id'))
+        if master_course_id is not None:
+            try:
+                course_key = CourseKey.from_string(master_course_id)
+            except InvalidKeyError:
+                raise Http404()
+            return (hasattr(request, 'user') and
+                    (CourseInstructorRole(course_key).has_user(request.user) or
+                     CourseStaffRole(course_key).has_user(request.user)))
+        return False
 
 
 class IsUserInUrlOrStaff(IsUserInUrl):

openedx/core/lib/api/tests/test_permissions.py

 """ Tests for API permissions classes. """
 
 import ddt
+from django.contrib.auth.models import AnonymousUser
+from django.http import Http404
 from django.test import TestCase, RequestFactory
 
 from student.roles import CourseStaffRole, CourseInstructorRole
-from openedx.core.lib.api.permissions import IsStaffOrOwner, IsCourseInstructor
+from openedx.core.lib.api.permissions import (
+    IsStaffOrOwner,
+    IsCourseStaffInstructor,
+    IsMasterCourseStaffInstructor,
+)
 from student.tests.factories import UserFactory
 from opaque_keys.edx.keys import CourseKey
 
@@ -16,35 +22,88 @@ class TestObject(object):
         self.course_id = course_id
 
 
-class IsCourseInstructorTests(TestCase):
-    """ Test for IsCourseInstructor permission class. """
+class TestCcxObject(TestObject):
+    """ Fake class for object permission for CCX Courses """
+    def __init__(self, user=None, course_id=None):
+        super(TestCcxObject, self).__init__(user, course_id)
+        self.coach = user
+
+
+class IsCourseStaffInstructorTests(TestCase):
+    """ Test for IsCourseStaffInstructor permission class. """
 
     def setUp(self):
-        super(IsCourseInstructorTests, self).setUp()
-        self.permission = IsCourseInstructor()
+        super(IsCourseStaffInstructorTests, self).setUp()
+        self.permission = IsCourseStaffInstructor()
+        self.coach = UserFactory.create()
+        self.user = UserFactory.create()
         self.request = RequestFactory().get('/')
+        self.request.user = self.user
         self.course_key = CourseKey.from_string('edx/test123/run')
-        self.obj = TestObject(course_id=self.course_key)
+        self.obj = TestCcxObject(user=self.coach, course_id=self.course_key)
 
-    def test_course_staff_has_no_access(self):
-        user = UserFactory.create()
-        self.request.user = user
-        CourseStaffRole(course_key=self.course_key).add_users(user)
+    def test_course_staff_has_access(self):
+        CourseStaffRole(course_key=self.course_key).add_users(self.user)
+        self.assertTrue(self.permission.has_object_permission(self.request, None, self.obj))
+
+    def test_course_instructor_has_access(self):
+        CourseInstructorRole(course_key=self.course_key).add_users(self.user)
+        self.assertTrue(self.permission.has_object_permission(self.request, None, self.obj))
+
+    def test_course_coach_has_access(self):
+        self.request.user = self.coach
+        self.assertTrue(self.permission.has_object_permission(self.request, None, self.obj))
 
-        self.assertFalse(
-            self.permission.has_object_permission(self.request, None, self.obj))
+    def test_any_user_has_no_access(self):
+        self.assertFalse(self.permission.has_object_permission(self.request, None, self.obj))
+
+    def test_anonymous_has_no_access(self):
+        self.request.user = AnonymousUser()
+        self.assertFalse(self.permission.has_object_permission(self.request, None, self.obj))
+
+
+class IsMasterCourseStaffInstructorTests(TestCase):
+    """ Test for IsMasterCourseStaffInstructorTests permission class. """
+
+    def setUp(self):
+        super(IsMasterCourseStaffInstructorTests, self).setUp()
+        self.permission = IsMasterCourseStaffInstructor()
+        master_course_id = 'edx/test123/run'
+        self.user = UserFactory.create()
+        self.get_request = RequestFactory().get('/?master_course_id={}'.format(master_course_id))
+        self.get_request.user = self.user
+        self.post_request = RequestFactory().post('/', data={'master_course_id': master_course_id})
+        self.post_request.user = self.user
+        self.course_key = CourseKey.from_string(master_course_id)
+
+    def test_course_staff_has_access(self):
+        CourseStaffRole(course_key=self.course_key).add_users(self.user)
+        self.assertTrue(self.permission.has_permission(self.get_request, None))
+        self.assertTrue(self.permission.has_permission(self.post_request, None))
 
     def test_course_instructor_has_access(self):
-        user = UserFactory.create()
-        self.request.user = user
-        CourseInstructorRole(course_key=self.course_key).add_users(user)
+        CourseInstructorRole(course_key=self.course_key).add_users(self.user)
+        self.assertTrue(self.permission.has_permission(self.get_request, None))
+        self.assertTrue(self.permission.has_permission(self.post_request, None))
 
-        self.assertTrue(
-            self.permission.has_object_permission(self.request, None, self.obj))
+    def test_any_user_has_partial_access(self):
+        self.assertFalse(self.permission.has_permission(self.get_request, None))
+        self.assertFalse(self.permission.has_permission(self.post_request, None))
 
     def test_anonymous_has_no_access(self):
-        self.assertFalse(
-            self.permission.has_object_permission(self.request, None, self.obj))
+        user = AnonymousUser()
+        self.get_request.user = user
+        self.post_request.user = user
+        self.assertFalse(self.permission.has_permission(self.get_request, None))
+        self.assertFalse(self.permission.has_permission(self.post_request, None))
+
+    def test_wrong_course_id_raises(self):
+        get_request = RequestFactory().get('/?master_course_id=this_is_invalid')
+        with self.assertRaises(Http404):
+            self.permission.has_permission(get_request, None)
+        post_request = RequestFactory().post('/', data={'master_course_id': 'this_is_invalid'})
+        with self.assertRaises(Http404):
+            self.permission.has_permission(post_request, None)
 
 
 @ddt.ddt