fix!: Disable changing discussions providers once a course run has started
For authenticated users that are not global staff, changing discussion providers after a course has started fails with 403 Forbidden. Related issues: * [BB-4253](https://tasks.opencraft.com/browse/BB-4253) * [TNL-8142](https://openedx.atlassian.net/browse/TNL-8142) BREAKING CHANGE: Course staff, who were previously allowed to do this operation, will instead receive a 403 Forbidden response.
This commit is contained in:
committed by
Awais Jibran
parent
87a620612f
commit
530e4932bb
@@ -338,6 +338,7 @@ class ModuleStoreTestUsersMixin():
|
||||
Create a test user for a course.
|
||||
"""
|
||||
if user_type is CourseUserType.ANONYMOUS:
|
||||
self.client.logout()
|
||||
return AnonymousUser()
|
||||
|
||||
is_enrolled = user_type is CourseUserType.ENROLLED
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
Test app view logic
|
||||
"""
|
||||
# pylint: disable=test-inherits-tests
|
||||
from datetime import datetime, timedelta, timezone
|
||||
import unittest
|
||||
|
||||
import ddt
|
||||
@@ -422,3 +423,54 @@ class DataTest(AuthorizedApiTest):
|
||||
assert data['provider_type'] == 'legacy'
|
||||
assert not data['plugin_configuration']['allow_anonymous']
|
||||
assert not data['lti_configuration']
|
||||
|
||||
@ddt.data(*[
|
||||
user_type.name for user_type in CourseUserType
|
||||
if user_type not in { # pylint: disable=undefined-variable
|
||||
CourseUserType.ANONYMOUS,
|
||||
CourseUserType.GLOBAL_STAFF
|
||||
}
|
||||
])
|
||||
def test_unable_to_change_provider_for_running_course(self, user_type):
|
||||
"""
|
||||
Ensure that certain users cannot change provider for a running course.
|
||||
"""
|
||||
self.course.start = datetime.now(timezone.utc) - timedelta(days=5)
|
||||
self.course = self.update_course(self.course, self.user.id)
|
||||
|
||||
# use the global staff user to do the initial config
|
||||
# so we're sure to not get permissions errors
|
||||
response = self._post({
|
||||
'enabled': True,
|
||||
'provider_type': 'legacy',
|
||||
})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
|
||||
self.create_user_for_course(self.course, CourseUserType[user_type])
|
||||
|
||||
response = self._post({
|
||||
'enabled': True,
|
||||
'provider_type': 'piazza',
|
||||
})
|
||||
assert response.status_code == status.HTTP_403_FORBIDDEN
|
||||
|
||||
def test_global_staff_can_change_provider_for_running_course(self):
|
||||
"""
|
||||
Ensure that global staff can change provider for a running course.
|
||||
"""
|
||||
self.course.start = datetime.now(timezone.utc) - timedelta(days=5)
|
||||
self.course = self.update_course(self.course, self.user.id)
|
||||
|
||||
# use the global staff user to do the initial config
|
||||
# so we're sure to not get permissions errors
|
||||
response = self._post({
|
||||
'enabled': True,
|
||||
'provider_type': 'legacy',
|
||||
})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
|
||||
response = self._post({
|
||||
'enabled': True,
|
||||
'provider_type': 'piazza',
|
||||
})
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
|
||||
@@ -4,10 +4,12 @@ Handle view-logic for the djangoapp
|
||||
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
|
||||
from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser
|
||||
from rest_framework.permissions import BasePermission
|
||||
from rest_framework.exceptions import PermissionDenied
|
||||
from rest_framework.response import Response
|
||||
from rest_framework.views import APIView
|
||||
|
||||
from common.djangoapps.student.roles import CourseStaffRole
|
||||
from common.djangoapps.student.roles import CourseStaffRole, GlobalStaff
|
||||
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
|
||||
from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser
|
||||
from openedx.core.lib.api.view_utils import validate_course_key
|
||||
from .models import DiscussionsConfiguration
|
||||
@@ -36,6 +38,33 @@ class IsStaff(BasePermission):
|
||||
).has_user(request.user)
|
||||
|
||||
|
||||
def user_permissions_for_course(course, user):
|
||||
"""
|
||||
Return the user's permissions over the discussion configuration of the course.
|
||||
"""
|
||||
return {
|
||||
"change_provider": not course.has_started() or GlobalStaff().has_user(user),
|
||||
}
|
||||
|
||||
PERMISSION_MESSAGES = {
|
||||
"change_provider": "Must be global staff to change discussion provider after the course has started.",
|
||||
}
|
||||
|
||||
DEFAULT_MESSAGE = "You're not authorized to perform this operation."
|
||||
|
||||
|
||||
def check_course_permissions(course, user, permission):
|
||||
"""
|
||||
Check the user has permissions for the operation over the course configuration.
|
||||
|
||||
Raises PermissionDenied if the user does not have permission
|
||||
"""
|
||||
permissions = user_permissions_for_course(course, user)
|
||||
granted = permissions.get(permission)
|
||||
if not granted:
|
||||
raise PermissionDenied(PERMISSION_MESSAGES.get(permission, DEFAULT_MESSAGE))
|
||||
|
||||
|
||||
class DiscussionsConfigurationView(APIView):
|
||||
"""
|
||||
Handle configuration-related view-logic
|
||||
@@ -54,7 +83,12 @@ class DiscussionsConfigurationView(APIView):
|
||||
"""
|
||||
course_key = validate_course_key(course_key_string)
|
||||
configuration = DiscussionsConfiguration.get(course_key)
|
||||
serializer = DiscussionsConfigurationSerializer(configuration)
|
||||
serializer = DiscussionsConfigurationSerializer(
|
||||
configuration,
|
||||
context={
|
||||
'user_id': request.user.id,
|
||||
}
|
||||
)
|
||||
return Response(serializer.data)
|
||||
|
||||
def post(self, request, course_key_string: str, **_kwargs) -> Response:
|
||||
@@ -63,6 +97,7 @@ class DiscussionsConfigurationView(APIView):
|
||||
"""
|
||||
course_key = validate_course_key(course_key_string)
|
||||
configuration = DiscussionsConfiguration.get(course_key)
|
||||
course = CourseOverview.get_from_id(course_key)
|
||||
serializer = DiscussionsConfigurationSerializer(
|
||||
configuration,
|
||||
context={
|
||||
@@ -72,5 +107,8 @@ class DiscussionsConfigurationView(APIView):
|
||||
partial=True,
|
||||
)
|
||||
if serializer.is_valid(raise_exception=True):
|
||||
if serializer.validated_data['provider_type'] != configuration.provider_type:
|
||||
check_course_permissions(course, request.user, 'change_provider')
|
||||
|
||||
serializer.save()
|
||||
return Response(serializer.data)
|
||||
|
||||
Reference in New Issue
Block a user