From 546256d700eef45c93303efcb3821b5ed773625f Mon Sep 17 00:00:00 2001 From: Akiva Leffert Date: Mon, 10 Nov 2014 17:49:58 -0500 Subject: [PATCH] Add a check for beta users in the video_info API Discovered a case I missed on the original version of this. See https://github.com/edx/edx-platform/pull/5798 JIRA: MA-76 Also fixes checking for mobile_access when course is None JIRA: MOB-978 --- lms/djangoapps/mobile_api/tests.py | 55 +++++++++++++++++++ lms/djangoapps/mobile_api/users/tests.py | 43 +++++++-------- lms/djangoapps/mobile_api/users/views.py | 12 +--- lms/djangoapps/mobile_api/utils.py | 31 +++++++++++ .../mobile_api/video_outlines/tests.py | 21 ++++++- .../mobile_api/video_outlines/views.py | 5 +- 6 files changed, 130 insertions(+), 37 deletions(-) create mode 100644 lms/djangoapps/mobile_api/tests.py create mode 100644 lms/djangoapps/mobile_api/utils.py diff --git a/lms/djangoapps/mobile_api/tests.py b/lms/djangoapps/mobile_api/tests.py new file mode 100644 index 0000000000..ff02dc4dbb --- /dev/null +++ b/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)) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 8a851cd182..714422840a 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/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) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 11d47f2cb8..23223dc9fb 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/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 diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py new file mode 100644 index 0000000000..afb6155f52 --- /dev/null +++ b/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) + ) diff --git a/lms/djangoapps/mobile_api/video_outlines/tests.py b/lms/djangoapps/mobile_api/video_outlines/tests.py index 134826552c..e76473a8f9 100644 --- a/lms/djangoapps/mobile_api/video_outlines/tests.py +++ b/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): """ diff --git a/lms/djangoapps/mobile_api/video_outlines/views.py b/lms/djangoapps/mobile_api/video_outlines/views.py index 172a2c4fdc..bc8483a3d3 100644 --- a/lms/djangoapps/mobile_api/video_outlines/views.py +++ b/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.")