diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 46727f7923..8824b9e7db 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -124,7 +124,6 @@ def _has_access_course_desc(user, action, course): 'load' -- load the courseware, see inside the course 'load_forum' -- can load and contribute to the forums (one access level for now) 'load_mobile' -- can load from a mobile context - 'load_mobile_no_enrollment_check' -- can load from a mobile context without checking for enrollment 'enroll' -- enroll. Checks for enrollment window, ACCESS_REQUIRE_STAFF_FOR_COURSE, 'see_exists' -- can see that the course exists. @@ -157,30 +156,20 @@ def _has_access_course_desc(user, action, course): """ Can this user access this course from a mobile device? """ - return ( - # check mobile requirements - can_load_mobile_no_enroll_check() and - # check enrollment - ( - CourseEnrollment.is_enrolled(user, course.id) or - _has_staff_access_to_descriptor(user, course, course.id) - ) - ) - - def can_load_mobile_no_enroll_check(): - """ - Can this enrolled user access this course from a mobile device? - Note: does not check for enrollment since it is assumed the caller has done so. - """ return ( # check start date can_load() and # check mobile_available flag is_mobile_available_for_user(user, course) and - # check staff access, if not then check for unfulfilled milestones ( + # either is a staff user or _has_staff_access_to_descriptor(user, course, course.id) or - not any_unfulfilled_milestones(course.id, user.id) + ( + # check enrollment + CourseEnrollment.is_enrolled(user, course.id) and + # check for unfulfilled milestones + not any_unfulfilled_milestones(course.id, user.id) + ) ) ) @@ -307,7 +296,6 @@ def _has_access_course_desc(user, action, course): 'view_courseware_with_prerequisites': can_view_courseware_with_prerequisites, 'load_forum': can_load_forum, 'load_mobile': can_load_mobile, - 'load_mobile_no_enrollment_check': can_load_mobile_no_enroll_check, 'enroll': can_enroll, 'see_exists': see_exists, 'staff': lambda: _has_staff_access_to_descriptor(user, course, course.id), diff --git a/lms/djangoapps/mobile_api/course_info/tests.py b/lms/djangoapps/mobile_api/course_info/tests.py index a426a740b8..fe2a490cf7 100644 --- a/lms/djangoapps/mobile_api/course_info/tests.py +++ b/lms/djangoapps/mobile_api/course_info/tests.py @@ -11,41 +11,12 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml_importer import import_course_from_xml from ..testutils import ( - MobileAPITestCase, MobileCourseAccessTestMixin, MobileEnrolledCourseAccessTestMixin, MobileAuthTestMixin + MobileAPITestCase, MobileCourseAccessTestMixin, MobileAuthTestMixin ) -class TestAbout(MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin): - """ - Tests for /api/mobile/v0.5/course_info/{course_id}/about - """ - REVERSE_INFO = {'name': 'course-about-detail', 'params': ['course_id']} - - def verify_success(self, response): - super(TestAbout, self).verify_success(response) - self.assertTrue('overview' in response.data) - - def init_course_access(self, course_id=None): - # override this method since enrollment is not required for the About endpoint. - self.login() - - def test_about_static_rewrite(self): - self.login() - - about_usage_key = self.course.id.make_usage_key('about', 'overview') - about_module = modulestore().get_item(about_usage_key) - underlying_about_html = about_module.data - - # check that we start with relative static assets - self.assertIn('\"/static/', underlying_about_html) - - # but shouldn't finish with any - response = self.api_response() - self.assertNotIn('\"/static/', response.data['overview']) - - @ddt.ddt -class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin): """ Tests for /api/mobile/v0.5/course_info/{course_id}/updates """ @@ -111,7 +82,7 @@ class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAc self.assertIn("Update" + str(num), update_data['content']) -class TestHandouts(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestHandouts(MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin): """ Tests for /api/mobile/v0.5/course_info/{course_id}/handouts """ diff --git a/lms/djangoapps/mobile_api/course_info/urls.py b/lms/djangoapps/mobile_api/course_info/urls.py index 7b1d2f8ce4..149ae62481 100644 --- a/lms/djangoapps/mobile_api/course_info/urls.py +++ b/lms/djangoapps/mobile_api/course_info/urls.py @@ -4,15 +4,10 @@ URLs for course_info API from django.conf.urls import patterns, url from django.conf import settings -from .views import CourseAboutDetail, CourseUpdatesList, CourseHandoutsList +from .views import CourseUpdatesList, CourseHandoutsList urlpatterns = patterns( 'mobile_api.course_info.views', - url( - r'^{}/about$'.format(settings.COURSE_ID_PATTERN), - CourseAboutDetail.as_view(), - name='course-about-detail' - ), url( r'^{}/handouts$'.format(settings.COURSE_ID_PATTERN), CourseHandoutsList.as_view(), diff --git a/lms/djangoapps/mobile_api/course_info/views.py b/lms/djangoapps/mobile_api/course_info/views.py index 764a580fc6..8c8fd3e694 100644 --- a/lms/djangoapps/mobile_api/course_info/views.py +++ b/lms/djangoapps/mobile_api/course_info/views.py @@ -87,33 +87,3 @@ class CourseHandoutsList(generics.ListAPIView): else: # course_handouts_module could be None if there are no handouts raise Http404(u"No handouts for {}".format(unicode(course.id))) - - -@mobile_view() -class CourseAboutDetail(generics.RetrieveAPIView): - """ - **Use Case** - - Get the HTML for the course about page. - - **Example request**: - - GET /api/mobile/v0.5/course_info/{organization}/{course_number}/{course_run}/about - - **Response Values** - - * overview: The HTML for the course About page. - """ - - @mobile_course_access(verify_enrolled=False) - def get(self, request, course, *args, **kwargs): - # There are other fields, but they don't seem to be in use. - # see courses.py:get_course_about_section. - # - # This can also return None, so check for that before calling strip() - about_section_html = get_course_about_section(course, "overview") - about_section_html = make_static_urls_absolute(self.request, about_section_html) - - return Response( - {"overview": about_section_html.strip() if about_section_html else ""} - ) diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 57fd67a14f..dafe73f852 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -7,8 +7,7 @@ Test utilities for mobile API tests: Test Mixins to be included by concrete test classes and provide implementation of common test methods: MobileAuthTestMixin - tests for APIs with mobile_view and is_user=False. MobileAuthUserTestMixin - tests for APIs with mobile_view and is_user=True. - MobileCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=False. - MobileEnrolledCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=True. + MobileCourseAccessTestMixin - tests for APIs with mobile_course_access. """ # pylint: disable=no-member import ddt @@ -130,7 +129,6 @@ class MobileAuthUserTestMixin(MobileAuthTestMixin): class MobileCourseAccessTestMixin(MobileAPIMilestonesMixin): """ Test Mixin for testing APIs marked with mobile_course_access. - (Use MobileEnrolledCourseAccessTestMixin when verify_enrolled is set to True.) Subclasses are expected to inherit from MobileAPITestCase. Subclasses can override verify_success, verify_failure, and init_course_access methods. """ @@ -197,11 +195,6 @@ class MobileCourseAccessTestMixin(MobileAPIMilestonesMixin): else: self.verify_failure(response) - -class MobileEnrolledCourseAccessTestMixin(MobileCourseAccessTestMixin): - """ - Test Mixin for testing APIs marked with mobile_course_access with verify_enrolled=True. - """ def test_unenrolled_user(self): self.login() self.unenroll() diff --git a/lms/djangoapps/mobile_api/users/serializers.py b/lms/djangoapps/mobile_api/users/serializers.py index e745d95de3..1674313343 100644 --- a/lms/djangoapps/mobile_api/users/serializers.py +++ b/lms/djangoapps/mobile_api/users/serializers.py @@ -31,16 +31,10 @@ class CourseField(serializers.RelatedField): kwargs={'course_id': course_id}, request=request ) - course_about_url = reverse( - 'course-about-detail', - kwargs={'course_id': course_id}, - request=request - ) else: video_outline_url = None course_updates_url = None course_handouts_url = None - course_about_url = None return { "id": course_id, @@ -59,7 +53,6 @@ class CourseField(serializers.RelatedField): "video_outline": video_outline_url, "course_updates": course_updates_url, "course_handouts": course_handouts_url, - "course_about": course_about_url, "subscription_id": course.clean_id(padding_char='_'), } diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 53ceb300d9..5f5d45df4a 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -10,7 +10,7 @@ from certificates.models import CertificateStatuses from certificates.tests.factories import GeneratedCertificateFactory from .. import errors -from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileAuthUserTestMixin, MobileEnrolledCourseAccessTestMixin +from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileAuthUserTestMixin, MobileCourseAccessTestMixin from .serializers import CourseEnrollmentSerializer @@ -43,7 +43,7 @@ class TestUserInfoApi(MobileAPITestCase, MobileAuthTestMixin): self.assertTrue(self.username in response['location']) -class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin, MobileCourseAccessTestMixin): """ Tests for /api/mobile/v0.5/users//course_enrollments/ """ @@ -160,7 +160,7 @@ class CourseStatusAPITestCase(MobileAPITestCase): ) -class TestCourseStatusGET(CourseStatusAPITestCase, MobileAuthUserTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestCourseStatusGET(CourseStatusAPITestCase, MobileAuthUserTestMixin, MobileCourseAccessTestMixin): """ Tests for GET of /api/mobile/v0.5/users//course_status_info/{course_id} """ @@ -178,7 +178,7 @@ class TestCourseStatusGET(CourseStatusAPITestCase, MobileAuthUserTestMixin, Mobi ) -class TestCourseStatusPATCH(CourseStatusAPITestCase, MobileAuthUserTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestCourseStatusPATCH(CourseStatusAPITestCase, MobileAuthUserTestMixin, MobileCourseAccessTestMixin): """ Tests for PATCH of /api/mobile/v0.5/users//course_status_info/{course_id} """ diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 446d7a934d..1942889376 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -212,7 +212,6 @@ class UserCourseEnrollmentsList(generics.ListAPIView): * url: URL to the downloadable version of the certificate, if exists. * course: A collection of data about the course: - * course_about: The URI to get the data for the course About page. * course_updates: The URI to get data for course updates. * number: The course number. * org: The organization that created the course. diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py index 5291693ce7..2ef7270561 100644 --- a/lms/djangoapps/mobile_api/utils.py +++ b/lms/djangoapps/mobile_api/utils.py @@ -1,79 +1,18 @@ """ Common utility methods and decorators for Mobile APIs. """ - -import functools - -from django.http import Http404 - -from rest_framework import permissions, status, response - -from opaque_keys.edx.keys import CourseKey - -from courseware.courses import get_course_with_access -from openedx.core.lib.api.permissions import IsUserInUrl -from openedx.core.lib.api.authentication import ( - SessionAuthenticationAllowInactiveUser, - OAuth2AuthenticationAllowInactiveUser, -) -from util.milestones_helpers import any_unfulfilled_milestones -from xmodule.modulestore.django import modulestore +from openedx.core.lib.api.view_utils import view_course_access, view_auth_classes -def mobile_course_access(depth=0, verify_enrolled=True): +def mobile_course_access(depth=0): """ Method decorator for a mobile API endpoint that verifies the user has access to the course in a mobile context. """ - def _decorator(func): - """Outer method decorator.""" - @functools.wraps(func) - def _wrapper(self, request, *args, **kwargs): - """ - Expects kwargs to contain 'course_id'. - Passes the course descriptor to the given decorated function. - Raises 404 if access to course is disallowed. - """ - course_id = CourseKey.from_string(kwargs.pop('course_id')) - with modulestore().bulk_operations(course_id): - try: - course = get_course_with_access( - request.user, - 'load_mobile' if verify_enrolled else 'load_mobile_no_enrollment_check', - course_id, - depth=depth - ) - except Http404: - # any_unfulfilled_milestones called a second time since get_course_with_access returns a bool - if any_unfulfilled_milestones(course_id, request.user.id): - message = { - "developer_message": "Cannot access content with unfulfilled pre-requisites or unpassed entrance exam." # pylint: disable=line-too-long - } - return response.Response( - data=message, - status=status.HTTP_204_NO_CONTENT - ) - else: - raise - return func(self, request, course=course, *args, **kwargs) - return _wrapper - return _decorator + return view_course_access(depth=depth, access_action='load_mobile', check_for_milestones=True) def mobile_view(is_user=False): """ Function and class decorator that abstracts the authentication and permission checks for mobile api views. """ - def _decorator(func_or_class): - """ - Requires either OAuth2 or Session-based authentication. - If is_user is True, also requires username in URL matches the request user. - """ - func_or_class.authentication_classes = ( - OAuth2AuthenticationAllowInactiveUser, - SessionAuthenticationAllowInactiveUser - ) - func_or_class.permission_classes = (permissions.IsAuthenticated,) - if is_user: - func_or_class.permission_classes += (IsUserInUrl,) - return func_or_class - return _decorator + return view_auth_classes(is_user) diff --git a/lms/djangoapps/mobile_api/video_outlines/tests.py b/lms/djangoapps/mobile_api/video_outlines/tests.py index 143ab081a7..b6d1ecf2c5 100644 --- a/lms/djangoapps/mobile_api/video_outlines/tests.py +++ b/lms/djangoapps/mobile_api/video_outlines/tests.py @@ -18,7 +18,7 @@ from xmodule.partitions.partitions import Group, UserPartition from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup -from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin +from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin class TestVideoAPITestCase(MobileAPITestCase): @@ -407,7 +407,7 @@ class TestNonStandardCourseStructure(MobileAPITestCase, TestVideoAPIMixin): @ddt.ddt class TestVideoSummaryList( - TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation + TestVideoAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation ): """ Tests for /api/mobile/v0.5/video_outlines/courses/{course_id}.. @@ -863,7 +863,7 @@ class TestVideoSummaryList( class TestTranscriptsDetail( - TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation + TestVideoAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation ): """ Tests for /api/mobile/v0.5/video_outlines/transcripts/{course_id}.. diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index 83734fc064..6ca1cf2362 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -1,6 +1,5 @@ from django.conf import settings from rest_framework import permissions -from rest_framework.exceptions import PermissionDenied from django.http import Http404 diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 4286d33c48..bc657ebbfb 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -1,12 +1,25 @@ """ Utilities related to API views """ +import functools from django.core.exceptions import NON_FIELD_ERRORS, ValidationError from django.http import Http404 +from rest_framework import status, response from rest_framework.exceptions import APIException from rest_framework.response import Response +from lms.djangoapps.courseware.courses import get_course_with_access +from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import modulestore + +from openedx.core.lib.api.authentication import ( + SessionAuthenticationAllowInactiveUser, + OAuth2AuthenticationAllowInactiveUser, +) +from openedx.core.lib.api.permissions import IsUserInUrl, IsAuthenticatedOrDebug +from util.milestones_helpers import any_unfulfilled_milestones + class DeveloperErrorViewMixin(object): """ @@ -48,3 +61,60 @@ class DeveloperErrorViewMixin(object): return self.make_validation_error_response(exc) else: raise + + +def view_course_access(depth=0, access_action='load', check_for_milestones=False): + """ + Method decorator for an API endpoint that verifies the user has access to the course. + """ + def _decorator(func): + """Outer method decorator.""" + @functools.wraps(func) + def _wrapper(self, request, *args, **kwargs): + """ + Expects kwargs to contain 'course_id'. + Passes the course descriptor to the given decorated function. + Raises 404 if access to course is disallowed. + """ + course_id = CourseKey.from_string(kwargs.pop('course_id')) + with modulestore().bulk_operations(course_id): + try: + course = get_course_with_access( + request.user, + access_action, + course_id, + depth=depth + ) + except Http404: + # any_unfulfilled_milestones called a second time since has_access returns a bool + if check_for_milestones and any_unfulfilled_milestones(course_id, request.user.id): + message = { + "developer_message": "Cannot access content with unfulfilled " + "pre-requisites or unpassed entrance exam." + } + return response.Response(data=message, status=status.HTTP_204_NO_CONTENT) + else: + raise + return func(self, request, course=course, *args, **kwargs) + return _wrapper + return _decorator + + +def view_auth_classes(is_user=False): + """ + Function and class decorator that abstracts the authentication and permission checks for api views. + """ + def _decorator(func_or_class): + """ + Requires either OAuth2 or Session-based authentication. + If is_user is True, also requires username in URL matches the request user. + """ + func_or_class.authentication_classes = ( + OAuth2AuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser + ) + func_or_class.permission_classes = (IsAuthenticatedOrDebug,) + if is_user: + func_or_class.permission_classes += (IsUserInUrl,) + return func_or_class + return _decorator