fix: return a 403 instead of a redirect from course_home_api api methods (#32210)

* fix: return 403 on course access error rather than redirect
This commit is contained in:
Jansen Kantor
2023-05-24 12:09:34 -04:00
committed by GitHub
parent bbb1fdbdf1
commit 7b91f8579f
6 changed files with 116 additions and 10 deletions

View File

@@ -13,9 +13,10 @@ from rest_framework.response import Response
from common.djangoapps.student.models import CourseEnrollment
from lms.djangoapps.course_goals.models import UserActivity
from lms.djangoapps.course_home_api.dates.serializers import DatesTabSerializer
from lms.djangoapps.course_home_api.utils import get_course_or_403
from lms.djangoapps.courseware.access import has_access
from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs
from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_with_access
from lms.djangoapps.courseware.courses import get_course_date_blocks
from lms.djangoapps.courseware.date_summary import TodaysDate
from lms.djangoapps.courseware.masquerade import setup_masquerade
from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser
@@ -59,6 +60,7 @@ class DatesTabView(RetrieveAPIView):
* 200 on success with above fields.
* 401 if the user is not authenticated.
* 403 if the user does not have access to the course.
* 404 if the course is not available or cannot be seen.
"""
@@ -79,7 +81,7 @@ class DatesTabView(RetrieveAPIView):
monitoring_utils.set_custom_attribute('user_id', request.user.id)
monitoring_utils.set_custom_attribute('is_staff', request.user.is_staff)
course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False)
course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=False)
is_staff = bool(has_access(request.user, 'staff', course_key))
_, request.user = setup_masquerade(

View File

@@ -109,6 +109,16 @@ class ResumeCourseSerializer(serializers.Serializer):
url = serializers.URLField()
class OutlineTabCourseAccessRedirectSerializer(serializers.Serializer):
"""
Serializer for a Course Access Redirect response from the outline tab
"""
url = serializers.URLField()
error_code = serializers.CharField(source='access_error.error_code')
developer_message = serializers.CharField(source='access_error.developer_message')
user_message = serializers.CharField(source='access_error.user_message')
class OutlineTabSerializer(DatesBannerSerializer, VerifiedModeSerializer):
"""
Serializer for the Outline Tab

View File

@@ -27,10 +27,13 @@ from lms.djangoapps.course_goals.api import (
get_course_goal,
)
from lms.djangoapps.course_goals.models import CourseGoal
from lms.djangoapps.course_home_api.outline.serializers import OutlineTabSerializer
from lms.djangoapps.course_home_api.outline.serializers import (
OutlineTabSerializer,
)
from lms.djangoapps.course_home_api.utils import get_course_or_403
from lms.djangoapps.courseware.access import has_access
from lms.djangoapps.courseware.context_processor import user_timezone_locale_prefs
from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_info_section, get_course_with_access
from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_info_section
from lms.djangoapps.courseware.date_summary import TodaysDate
from lms.djangoapps.courseware.masquerade import is_masquerading, setup_masquerade
from lms.djangoapps.courseware.views.views import get_cert_data
@@ -76,7 +79,9 @@ class OutlineTabView(RetrieveAPIView):
**Response Values**
Body consists of the following fields:
Body consists of two possible shapes.
For a good 200 response, the response will include:
access_expiration: An object detailing when access to this course will expire
expiration_date: (str) When the access expires, in ISO 8601 notation
@@ -143,9 +148,19 @@ class OutlineTabView(RetrieveAPIView):
welcome_message_html: (str) Raw HTML for the course updates banner
user_has_passing_grade: (bool) Whether the user currently is passing the course
If the learner does not have access to the course for a specific reason and should be redirected this
view will return a 403 and the following data:
url: (str) The URL to which the user should be redirected
error_code: (str) A system identifier for the reason the user is being redirected
developer_message: (str) A message explaining why the user is being redirected,
intended for developer debugging.
user_message: (str) A message explaining why the user is being redirected, intended to be shown to the user.
**Returns**
* 200 on success with above fields.
* 200 on success.
* 403 if the user does not currently have access to the course and should be redirected.
* 404 if the course is not available or cannot be seen.
"""
@@ -167,7 +182,7 @@ class OutlineTabView(RetrieveAPIView):
monitoring_utils.set_custom_attribute('user_id', request.user.id)
monitoring_utils.set_custom_attribute('is_staff', request.user.is_staff)
course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False)
course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=False)
masquerade_object, request.user = setup_masquerade(
request,
@@ -365,7 +380,7 @@ def dismiss_welcome_message(request): # pylint: disable=missing-function-docstr
try:
course_key = CourseKey.from_string(course_id)
course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)
course = get_course_or_403(request.user, 'load', course_key, check_if_enrolled=True)
dismiss_current_update_for_user(request, course)
return Response({'message': _('Welcome message successfully dismissed.')})
except Exception:

View File

@@ -21,8 +21,9 @@ from lms.djangoapps.course_blocks.api import get_course_blocks
from lms.djangoapps.course_blocks.transformers import start_date
from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException
from lms.djangoapps.course_home_api.utils import get_course_or_403
from lms.djangoapps.courseware.courses import (
get_course_blocks_completion_summary, get_course_with_access, get_studio_url,
get_course_blocks_completion_summary, get_studio_url,
)
from lms.djangoapps.courseware.masquerade import setup_masquerade
from lms.djangoapps.courseware.views.views import credit_course_requirements, get_cert_data
@@ -128,6 +129,7 @@ class ProgressTabView(RetrieveAPIView):
* 200 on success with above fields.
* 401 if the user is not authenticated or not enrolled.
* 403 if the user does not have access to the course.
* 404 if the course is not available or cannot be seen.
"""
@@ -190,7 +192,7 @@ class ProgressTabView(RetrieveAPIView):
student = self._get_student_user(request, course_key, student_id, is_staff)
username = get_enterprise_learner_generic_name(request) or student.username
course = get_course_with_access(student, 'load', course_key, check_if_enrolled=False)
course = get_course_or_403(student, 'load', course_key, check_if_enrolled=False)
course_overview = CourseOverview.get_from_id(course_key)
enrollment = CourseEnrollment.get_enrollment(student, course_key)

View File

@@ -0,0 +1,53 @@
""" Tests for course home api utils """
from contextlib import contextmanager
from rest_framework.exceptions import PermissionDenied
from unittest import mock
from lms.djangoapps.course_home_api.utils import get_course_or_403
from lms.djangoapps.courseware.access_response import AccessError
from lms.djangoapps.courseware.exceptions import CourseAccessRedirect
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
class GetCourseOr403Test(ModuleStoreTestCase):
""" Tests for get_course_or_403 """
@contextmanager
def mock_get_course(self, *args, **kwargs):
""" Mock base_get_course_with_access helper """
with mock.patch(
'lms.djangoapps.course_home_api.utils.base_get_course_with_access',
*args,
**kwargs
) as mock_get:
yield mock_get
def test_no_exception(self):
""" If no exception is raised we should return the return of get_course_with_access """
expected_return = mock.Mock()
with self.mock_get_course(return_value=expected_return):
assert get_course_or_403() == expected_return
def test_redirect(self):
""" Test for behavior when get_course_with_access raises a redirect error """
expected_url = "www.testError.access/redirect.php?work=yes"
mock_access_error = AccessError('code', 'dev_msg', 'usr_msg')
mock_course_access_redirect = CourseAccessRedirect(expected_url, mock_access_error)
with self.mock_get_course(side_effect=mock_course_access_redirect):
try:
get_course_or_403()
self.fail('Call to get_course_or_403 should raise exception')
except PermissionDenied as e:
assert str(e.detail) == mock_access_error.user_message
assert e.detail.code == mock_access_error.error_code
def test_other_exception(self):
""" Any other exception should not be caught """
class MyException(Exception):
pass
with self.mock_get_course(side_effect=MyException()):
with self.assertRaises(MyException):
get_course_or_403()

View File

@@ -0,0 +1,24 @@
""" Utilities for views in the course home api"""
from rest_framework.exceptions import PermissionDenied
from lms.djangoapps.courseware.courses import get_course_with_access as base_get_course_with_access
from lms.djangoapps.courseware.exceptions import CourseAccessRedirect
def get_course_or_403(*args, **kwargs):
"""
When we make requests to the various Learner Home API endpoints, we do not want to return the actual redirects,
Instead we should return an error code. The redirect info is returned from the course metadata endpoint and the
URL can be followed by whatever client is calling.
Raises:
- 404 if course is not found
- 403 if the requesting user does not have access to the course
"""
try:
return base_get_course_with_access(*args, **kwargs)
except CourseAccessRedirect as e:
raise PermissionDenied(
detail=e.access_error.user_message,
code=e.access_error.error_code
) from e