Merge pull request #5920 from edx/aleffert/additional-beta-checks

Add a check for beta users in the video_info API

lms/djangoapps/mobile_api/tests.py

@@ -0,0 +1,55 @@
+"""
+Tests for mobile API utilities
+"""
+
+import ddt
+from rest_framework.test import APITestCase
+
+from courseware.tests.factories import UserFactory
+from student import auth
+
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+from .utils import mobile_available_when_enrolled
+
+ROLE_CASES = (
+    (auth.CourseBetaTesterRole, True),
+    (auth.CourseStaffRole, True),
+    (auth.CourseInstructorRole, True),
+    (None, False)
+)
+
+
+@ddt.ddt
+class TestMobileApiUtils(ModuleStoreTestCase, APITestCase):
+    """
+    Tests for mobile API utilities
+    """
+
+    def setUp(self):
+        self.user = UserFactory.create()
+
+    @ddt.data(*ROLE_CASES)
+    @ddt.unpack
+    def test_mobile_role_access(self, role, should_have_access):
+        """
+        Verifies that our mobile access function properly handles using roles to grant access
+        """
+        course = CourseFactory.create(mobile_available=False)
+        if role:
+            role(course.id).add_users(self.user)
+        self.assertEqual(should_have_access, mobile_available_when_enrolled(course, self.user))
+
+    def test_mobile_explicit_access(self):
+        """
+        Verifies that our mobile access function listens to the mobile_available flag as it should
+        """
+        course = CourseFactory.create(mobile_available=True)
+        self.assertTrue(mobile_available_when_enrolled(course, self.user))
+
+    def test_missing_course(self):
+        """
+        Verifies that we handle the case where a course doesn't exist
+        """
+        self.assertFalse(mobile_available_when_enrolled(None, self.user))

lms/djangoapps/mobile_api/users/tests.py

@@ -13,6 +13,7 @@ from django.core.urlresolvers import reverse
 from mobile_api.users.serializers import CourseEnrollmentSerializer
 from student.models import CourseEnrollment
 from student import auth
+from mobile_api.tests import ROLE_CASES
 
 
 @ddt.ddt
@@ -48,7 +49,7 @@ class TestUserApi(ModuleStoreTestCase, APITestCase):
         })
         self.assertEqual(resp.status_code, 200)
 
-    def _verify_single_course_enrollment(self, course):
+    def _verify_single_course_enrollment(self, course, should_succeed):
         """
         check that enrolling in course adds us to it
         """
@@ -58,36 +59,32 @@ class TestUserApi(ModuleStoreTestCase, APITestCase):
         self._enroll(course)
         response = self.client.get(url)
 
-        self.assertEqual(response.status_code, 200)
-
         courses = response.data   # pylint: disable=maybe-no-member
-        self.assertEqual(len(courses), 1)
 
-        found_course = courses[0]['course']
-        self.assertTrue('video_outline' in found_course)
-        self.assertTrue('course_handouts' in found_course)
-        self.assertEqual(found_course['id'], unicode(course.id))
-        self.assertEqual(courses[0]['mode'], 'honor')
-
-    def test_non_mobile_enrollments(self):
-        url = self._enrollment_url()
-        non_mobile_course = CourseFactory.create(mobile_available=False)
-        self.client.login(username=self.username, password=self.password)
-
-        self._enroll(non_mobile_course)
-        response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.data, [])    # pylint: disable=maybe-no-member
 
-    @ddt.data(auth.CourseBetaTesterRole, auth.CourseStaffRole, auth.CourseInstructorRole)
-    def test_privileged_enrollments(self, role):
+        if should_succeed:
+            self.assertEqual(len(courses), 1)
+            found_course = courses[0]['course']
+            self.assertTrue('video_outline' in found_course)
+            self.assertTrue('course_handouts' in found_course)
+            self.assertEqual(found_course['id'], unicode(course.id))
+            self.assertEqual(courses[0]['mode'], 'honor')
+        else:
+            self.assertEqual(len(courses), 0)
+
+    @ddt.data(*ROLE_CASES)
+    @ddt.unpack
+    def test_non_mobile_enrollments(self, role, should_succeed):
         non_mobile_course = CourseFactory.create(mobile_available=False)
-        role(non_mobile_course.id).add_users(self.user)
 
-        self._verify_single_course_enrollment(non_mobile_course)
+        if role:
+            role(non_mobile_course.id).add_users(self.user)
+
+        self._verify_single_course_enrollment(non_mobile_course, should_succeed)
 
     def test_mobile_enrollments(self):
-        self._verify_single_course_enrollment(self.course)
+        self._verify_single_course_enrollment(self.course, True)
 
     def test_user_overview(self):
         self.client.login(username=self.username, password=self.password)

lms/djangoapps/mobile_api/users/views.py

@@ -8,10 +8,9 @@ from rest_framework.authentication import OAuth2Authentication, SessionAuthentic
 from rest_framework.decorators import api_view, authentication_classes, permission_classes
 from rest_framework.permissions import IsAuthenticated
 
-from courseware import access
 from student.models import CourseEnrollment, User
-from student.roles import CourseBetaTesterRole
-from student import auth
+
+from mobile_api.utils import mobile_available_when_enrolled
 
 from .serializers import CourseEnrollmentSerializer, UserSerializer
 
@@ -129,10 +128,5 @@ def mobile_course_enrollments(enrollments, user):
     for enr in enrollments:
         course = enr.course
 
-        # Implicitly includes instructor role via the following has_access check
-        role = CourseBetaTesterRole(course.id)
-
-        # The course doesn't always really exist -- we can have bad data in the enrollments
-        # pointing to non-existent (or removed) courses, in which case `course` is None.
-        if course and (course.mobile_available or auth.has_access(user, role) or access.has_access(user, 'staff', course)):
+        if mobile_available_when_enrolled(course, user):
             yield enr

lms/djangoapps/mobile_api/utils.py

@@ -0,0 +1,31 @@
+"""
+Tests for video outline API
+"""
+
+
+from courseware import access
+from student.roles import CourseBetaTesterRole
+from student import auth
+
+
+def mobile_available_when_enrolled(course, user):
+    """
+    Determines whether a user has access to a course in a mobile context.
+    Checks if the course is marked as mobile_available or the user has extra permissions
+    that gives them access anyway.
+    Does not check if the user is actually enrolled in the course
+    """
+
+    # The course doesn't always really exist -- we can have bad data in the enrollments
+    # pointing to non-existent (or removed) courses, in which case `course` is None.
+    if not course:
+        return None
+
+    # Implicitly includes instructor role via the following has_access check
+    beta_tester_role = CourseBetaTesterRole(course.id)
+
+    return (
+        course.mobile_available
+        or auth.has_access(user, beta_tester_role)
+        or access.has_access(user, 'staff', course)
+    )

lms/djangoapps/mobile_api/video_outlines/tests.py

@@ -1,6 +1,9 @@
 """
 Tests for video outline API
 """
+
+import ddt
+
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.video_module import transcripts_utils
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -11,14 +14,17 @@ from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 from django.conf import settings
 from rest_framework.test import APITestCase
+from mobile_api.tests import ROLE_CASES
 from edxval import api
 from uuid import uuid4
+
 import copy
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
 TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex
 
 
+@ddt.ddt
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE, CONTENTSTORE=TEST_DATA_CONTENTSTORE)
 class TestVideoOutline(ModuleStoreTestCase, APITestCase):
     """
@@ -99,11 +105,20 @@ class TestVideoOutline(ModuleStoreTestCase, APITestCase):
 
         self.client.login(username=self.user.username, password='test')
 
-    def test_course_not_available(self):
-        nonmobile = CourseFactory.create()
+    @ddt.data(*ROLE_CASES)
+    @ddt.unpack
+    def test_non_mobile_access(self, role, should_succeed):
+        nonmobile = CourseFactory.create(mobile_available=False)
+
+        if role:
+            role(nonmobile.id).add_users(self.user)
+
         url = reverse('video-summary-list', kwargs={'course_id': unicode(nonmobile.id)})
         response = self.client.get(url)
-        self.assertEqual(response.status_code, 403)
+        if should_succeed:
+            self.assertEqual(response.status_code, 200)
+        else:
+            self.assertEqual(response.status_code, 403)
 
     def _get_video_summary_list(self):
         """

lms/djangoapps/mobile_api/video_outlines/views.py

@@ -17,10 +17,11 @@ from rest_framework.exceptions import PermissionDenied
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locator import BlockUsageLocator
 
-from courseware.access import has_access
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore.django import modulestore
 
+from mobile_api.utils import mobile_available_when_enrolled
+
 from .serializers import BlockOutline, video_summary
 
 
@@ -139,7 +140,7 @@ def get_mobile_course(course_id, user):
     requesting user is a staff member.
     """
     course = modulestore().get_course(course_id, depth=None)
-    if course.mobile_available or has_access(user, 'staff', course):
+    if mobile_available_when_enrolled(course, user):
         return course
 
     raise PermissionDenied(detail="Course not available on mobile.")