From 0016b2844bdf98043d0a4320c715ba267998e899 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 15 Aug 2023 10:13:26 -0400 Subject: [PATCH 1/6] feat: add observability for default auth classes (#33003) Added new authentication classes to be used in DEFAULT_AUTHENTICATION_CLASSES that include observability. This will enable us to see more about the endpoints using the defaults, to help us make choices about changes in the defaults. We make the DRF default of Session and Basic Authentication explicit by setting DEFAULT_AUTHENTICATION_CLASSES explicitly. (cherry picked from commit b9134c64ff35b2b14825c5e58a69735ceb0a4fd3) --- lms/envs/common.py | 9 +- .../core/djangolib/default_auth_classes.py | 87 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 openedx/core/djangolib/default_auth_classes.py diff --git a/lms/envs/common.py b/lms/envs/common.py index 419c966e1bb1..9e1d55618b5a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3344,6 +3344,13 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ######################### Django Rest Framework ######################## REST_FRAMEWORK = { + # This matches the original DRF default of Session and Basic Authentication, but + # adds observability to help us potentially adjust the defaults. We would like to + # add JwtAuthentication and drop BasicAuthentication, based on our findings. + 'DEFAULT_AUTHENTICATION_CLASSES': [ + 'openedx.core.djangolib.default_auth_classes.DefaultSessionAuthentication', + 'openedx.core.djangolib.default_auth_classes.DefaultBasicAuthentication' + ], 'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination', 'DEFAULT_RENDERER_CLASSES': ( 'rest_framework.renderers.JSONRenderer', @@ -3360,7 +3367,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # .. setting_name: REGISTRATION_VALIDATION_RATELIMIT # .. setting_default: 30/7d -# .. setting_description: Whenver a user tries to register on edx, the data entered during registration +# .. setting_description: Whenever a user tries to register on edx, the data entered during registration # is validated via RegistrationValidationView. # It's POST endpoint is rate-limited up to 30 requests per IP Address in a week by default. # It was introduced because an attacker can guess or brute force a series of names to enumerate valid users. diff --git a/openedx/core/djangolib/default_auth_classes.py b/openedx/core/djangolib/default_auth_classes.py new file mode 100644 index 000000000000..238993c7aa8a --- /dev/null +++ b/openedx/core/djangolib/default_auth_classes.py @@ -0,0 +1,87 @@ +""" +Default Authentication classes that are ONLY meant to be used by +DEFAULT_AUTHENTICATION_CLASSES for observability purposes. +""" +from edx_django_utils.monitoring import set_custom_attribute +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from rest_framework.authentication import BasicAuthentication, SessionAuthentication + + +class DefaultSessionAuthentication(SessionAuthentication): + """ Default SessionAuthentication with observability """ + + def authenticate(self, request): + # .. custom_attribute_name: using_default_auth_classes + # .. custom_attribute_description: This custom attribute will always be + # True (if not NULL), and signifies that a default authentication + # class was used. This can be used to find endpoints using the + # default authentication classes. + set_custom_attribute('using_default_auth_classes', True) + + try: + user_and_auth = super().authenticate(request) + if user_and_auth: + # .. custom_attribute_name: session_auth_result + # .. custom_attribute_description: The result of session auth, represented + # by: 'success', 'failure', or 'skipped'. + set_custom_attribute('session_auth_result', 'success') + else: + set_custom_attribute('session_auth_result', 'skipped') + return user_and_auth + except Exception as exception: + set_custom_attribute('session_auth_result', 'failure') + raise + + +class DefaultBasicAuthentication(BasicAuthentication): + """ + Default BasicAuthentication with observability + + Note that BasicAuthentication was a default because it was a DRF default. + Observability will be used to determine if BasicAuthentication could + instead be dropped as a default. + """ + + def authenticate(self, request): + # .. custom_attribute_name: using_default_auth_classes + # .. custom_attribute_description: This custom attribute will always be + # True (if not NULL), and signifies that a default authentication + # class was used. This can be used to find endpoints using the + # default authentication classes. + set_custom_attribute('using_default_auth_classes', True) + + try: + user_and_auth = super().authenticate(request) + if user_and_auth: + # .. custom_attribute_name: basic_auth_result + # .. custom_attribute_description: The result of basic auth, represented + # by: 'success', 'failure', or 'skipped'. + set_custom_attribute('basic_auth_result', 'success') + else: + set_custom_attribute('basic_auth_result', 'skipped') + return user_and_auth + except Exception as exception: + set_custom_attribute('basic_auth_result', 'failure') + raise + + +class DefaultJwtAuthentication(JwtAuthentication): + """ + Default JwtAuthentication with observability + + Note that the plan is to add JwtAuthentication as a default, but it + is not yet used. This class will be used during the transition. + """ + + def authenticate(self, request): + # .. custom_attribute_name: using_default_auth_classes + # .. custom_attribute_description: This custom attribute will always be + # True (if not NULL), and signifies that a default authentication + # class was used. This can be used to find endpoints using the + # default authentication classes. + set_custom_attribute('using_default_auth_classes', True) + + # Unlike the other DRF authentication classes, JwtAuthentication already + # includes a jwt_auth_result custom attribute, so we do not need to + # reimplement that observability in this class. + return super().authenticate(request) From b011fe00094d5d37736deab40748290717a6c717 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Mon, 21 Aug 2023 14:45:34 -0400 Subject: [PATCH 2/6] feat!: remove BasicAuthentication default Removed BasicAuthentication as a default from DRF endpoints that have not overridden the authentication classes. It appears this is not in use, and was just implicitly a default because it came from DRF's defaults. See DEPR: https://github.com/openedx/edx-platform/issues/33028 (cherry picked from commit 71136240ad5b32d0768a31142f631696542f9b55) --- lms/envs/common.py | 6 ++-- .../core/djangolib/default_auth_classes.py | 34 +------------------ 2 files changed, 3 insertions(+), 37 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 9e1d55618b5a..bd100a211dc4 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3344,12 +3344,10 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ######################### Django Rest Framework ######################## REST_FRAMEWORK = { - # This matches the original DRF default of Session and Basic Authentication, but - # adds observability to help us potentially adjust the defaults. We would like to - # add JwtAuthentication and drop BasicAuthentication, based on our findings. + # These default classes add observability around endpoints using defaults, and should + # not be used anywhere else. 'DEFAULT_AUTHENTICATION_CLASSES': [ 'openedx.core.djangolib.default_auth_classes.DefaultSessionAuthentication', - 'openedx.core.djangolib.default_auth_classes.DefaultBasicAuthentication' ], 'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination', 'DEFAULT_RENDERER_CLASSES': ( diff --git a/openedx/core/djangolib/default_auth_classes.py b/openedx/core/djangolib/default_auth_classes.py index 238993c7aa8a..5150304339db 100644 --- a/openedx/core/djangolib/default_auth_classes.py +++ b/openedx/core/djangolib/default_auth_classes.py @@ -4,7 +4,7 @@ """ from edx_django_utils.monitoring import set_custom_attribute from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication -from rest_framework.authentication import BasicAuthentication, SessionAuthentication +from rest_framework.authentication import SessionAuthentication class DefaultSessionAuthentication(SessionAuthentication): @@ -33,38 +33,6 @@ def authenticate(self, request): raise -class DefaultBasicAuthentication(BasicAuthentication): - """ - Default BasicAuthentication with observability - - Note that BasicAuthentication was a default because it was a DRF default. - Observability will be used to determine if BasicAuthentication could - instead be dropped as a default. - """ - - def authenticate(self, request): - # .. custom_attribute_name: using_default_auth_classes - # .. custom_attribute_description: This custom attribute will always be - # True (if not NULL), and signifies that a default authentication - # class was used. This can be used to find endpoints using the - # default authentication classes. - set_custom_attribute('using_default_auth_classes', True) - - try: - user_and_auth = super().authenticate(request) - if user_and_auth: - # .. custom_attribute_name: basic_auth_result - # .. custom_attribute_description: The result of basic auth, represented - # by: 'success', 'failure', or 'skipped'. - set_custom_attribute('basic_auth_result', 'success') - else: - set_custom_attribute('basic_auth_result', 'skipped') - return user_and_auth - except Exception as exception: - set_custom_attribute('basic_auth_result', 'failure') - raise - - class DefaultJwtAuthentication(JwtAuthentication): """ Default JwtAuthentication with observability From a4b7e7220419048e495290078c47bd3d63bca4ab Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 20 Jul 2023 15:02:34 -0400 Subject: [PATCH 3/6] feat!: Add JwtAuthentication as a default DRF auth class. By default DRF sets 'DEFAULT_AUTHENTICATION_CLASSES' to: ``` [ 'rest_framework.authentication.SessionAuthentication', 'rest_framework.authentication.BasicAuthentication' ] ``` We also want to allow for JWT Authentication as a valid default auth choice. This will allow users to send JWT tokens in the authorization header to any existing API endpoints and access them. If any APIs have set custom authentication classes, this will not override that. I believe this is a fairly safe change to make since it only adds one authentication class and does not impact authorization of any of the endpoints that might be affected. Note: This change changes the default for both the LMS and CMS because `cms/envs/common.py` imports this value from the LMS. BREAKING CHANGE: For any affected endpoint that also required the user to be authenticated, the endpoint will now return a 401 in place of a 403 when the user is not authenticated. - See [these DRF docs](https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/authentication.md#unauthorized-and-forbidden-responses) for a deeper explanation about why this changes. - Here is [an example endpoint](https://github.com/openedx/edx-platform/blob/b8ecfed67dc0520b8c4d95de3096b35acc083611/openedx/core/djangoapps/embargo/views.py#L20-L21) that does not override defaults and checks for IsAuthenticated. Generally speaking, this is should not be a problem. An issue would appear only if the caller of the endpoint is specifically handling 403s in a way that would be missed for 401s. (cherry picked from commit 7af2b1db2417ed06ddace6dd8c6bf7c28de85f38) --- lms/envs/common.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lms/envs/common.py b/lms/envs/common.py index bd100a211dc4..70112a5cf45f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3346,7 +3346,14 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring REST_FRAMEWORK = { # These default classes add observability around endpoints using defaults, and should # not be used anywhere else. + # Notes on Order: + # 1. `JwtAuthentication` does not check `is_active`, so email validation does not affect it. However, + # `SessionAuthentication` does. These work differently, and order changes in what way, which really stinks. See + # https://github.com/openedx/public-engineering/issues/165 for details. + # 2. `JwtAuthentication` may also update the database based on contents. Since the LMS creates these JWTs, this + # shouldn't have any affect at this time. But it could, when and if another service started creating the JWTs. 'DEFAULT_AUTHENTICATION_CLASSES': [ + 'openedx.core.djangolib.default_auth_classes.DefaultJwtAuthentication', 'openedx.core.djangolib.default_auth_classes.DefaultSessionAuthentication', ], 'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination', From e1fb05427e7d197950fd58ee7e17041b56b1a085 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Tue, 30 Jul 2024 13:18:49 +0500 Subject: [PATCH 4/6] feat: converting existing api to drf base api. (#35039) Adding generic permission class. Added standard authentication classes. (cherry picked from commit 39dd3c002b296a205e867f147a90a4eea6aee8c3) --- lms/djangoapps/instructor/permissions.py | 12 +++- lms/djangoapps/instructor/tests/test_api.py | 51 ++++++++++++++- lms/djangoapps/instructor/views/api.py | 71 ++++++++++++++------- lms/djangoapps/instructor/views/api_urls.py | 3 +- 4 files changed, 111 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 80dc649e5a95..bc5556945c0c 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -1,11 +1,13 @@ """ Permissions for the instructor dashboard and associated actions """ - from bridgekeeper import perms from bridgekeeper.rules import is_staff +from opaque_keys.edx.keys import CourseKey +from rest_framework.permissions import BasePermission from lms.djangoapps.courseware.rules import HasAccessRule, HasRolesRule +from openedx.core.lib.courses import get_course_by_id ALLOW_STUDENT_TO_BYPASS_ENTRANCE_EXAM = 'instructor.allow_student_to_bypass_entrance_exam' ASSIGN_TO_COHORTS = 'instructor.assign_to_cohorts' @@ -79,3 +81,11 @@ ) | HasAccessRule('staff') | HasAccessRule('instructor') perms[VIEW_ENROLLMENTS] = HasAccessRule('staff') perms[VIEW_FORUM_MEMBERS] = HasAccessRule('staff') + + +class InstructorPermission(BasePermission): + """Generic permissions""" + def has_permission(self, request, view): + course = get_course_by_id(CourseKey.from_string(view.kwargs.get('course_id'))) + permission = getattr(view, 'permission_name', None) + return request.user.has_perm(permission, course) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index bb6b69eb9f03..aee8d13e330c 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -2927,7 +2927,37 @@ def test_get_student_progress_url(self): response = self.client.post(url, data) assert response.status_code == 200 res_json = json.loads(response.content.decode('utf-8')) - assert 'progress_url' in res_json + expected_data = { + 'course_id': str(self.course.id), + 'progress_url': f'/courses/{self.course.id}/progress/{self.students[0].id}/' + } + + for key, value in expected_data.items(): + self.assertIn(key, res_json) + self.assertEqual(res_json[key], value) + + def test_get_student_progress_url_response_headers(self): + """ + Test that the progress_url endpoint returns the correct headers. + """ + url = reverse('get_student_progress_url', kwargs={'course_id': str(self.course.id)}) + data = {'unique_student_identifier': self.students[0].email} + response = self.client.post(url, data) + assert response.status_code == 200 + + expected_headers = { + 'Allow': 'POST, OPTIONS', # drf view brings this key. + 'Cache-Control': 'no-cache, no-store, must-revalidate', + 'Content-Language': 'en', + 'Content-Length': str(len(response.content.decode('utf-8'))), + 'Content-Type': 'application/json', + 'Vary': 'Cookie, Accept-Language, origin', + 'X-Frame-Options': 'DENY' + } + + for key, value in expected_headers.items(): + self.assertIn(key, response.headers) + self.assertEqual(response.headers[key], value) def test_get_student_progress_url_from_uname(self): """ Test that progress_url is in the successful response. """ @@ -2937,6 +2967,14 @@ def test_get_student_progress_url_from_uname(self): assert response.status_code == 200 res_json = json.loads(response.content.decode('utf-8')) assert 'progress_url' in res_json + expected_data = { + 'course_id': str(self.course.id), + 'progress_url': f'/courses/{self.course.id}/progress/{self.students[0].id}/' + } + + for key, value in expected_data.items(): + self.assertIn(key, res_json) + self.assertEqual(res_json[key], value) def test_get_student_progress_url_noparams(self): """ Test that the endpoint 404's without the required query params. """ @@ -2950,6 +2988,17 @@ def test_get_student_progress_url_nostudent(self): response = self.client.post(url) assert response.status_code == 400 + def test_get_student_progress_url_without_permissions(self): + """ Test that progress_url returns 403 without credentials. """ + + # removed both roles from courses for instructor + CourseDataResearcherRole(self.course.id).remove_users(self.instructor) + CourseInstructorRole(self.course.id).remove_users(self.instructor) + url = reverse('get_student_progress_url', kwargs={'course_id': str(self.course.id)}) + data = {'unique_student_identifier': self.students[0].email} + response = self.client.post(url, data) + assert response.status_code == 403 + class TestInstructorAPIRegradeTask(SharedModuleStoreTestCase, LoginEnrollmentTestCase): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index ad147f14472d..831775ed156d 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -122,6 +122,7 @@ from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from openedx.core.lib.courses import get_course_by_id +from openedx.core.lib.api.serializers import CourseKeyField from openedx.features.course_experience.url_helpers import get_learning_mfe_home_url from .tools import ( dump_block_extensions, @@ -1712,15 +1713,35 @@ def get_student_enrollment_status(request, course_id): return JsonResponse(response_payload) -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.ENROLLMENT_REPORT) -@require_post_params( - unique_student_identifier="email or username of student for whom to get progress url" -) -@common_exceptions_400 -def get_student_progress_url(request, course_id): +class StudentProgressUrlSerializer(serializers.Serializer): + """Serializer for course renders""" + unique_student_identifier = serializers.CharField(write_only=True) + course_id = CourseKeyField(required=False) + progress_url = serializers.SerializerMethodField() + + def get_progress_url(self, obj): # pylint: disable=unused-argument + """ + Return the progress URL for the student. + Args: + obj (dict): The dictionary containing data for the serializer. + Returns: + str: The URL for the progress of the student in the course. + """ + user = get_student_from_identifier(obj.get('unique_student_identifier')) + course_id = obj.get('course_id') # Adjust based on your data structure + + if course_home_mfe_progress_tab_is_active(course_id): + progress_url = get_learning_mfe_home_url(course_id, url_fragment='progress') + if user is not None: + progress_url += '/{}/'.format(user.id) + else: + progress_url = reverse('student_progress', kwargs={'course_id': str(course_id), 'student_id': user.id}) + + return progress_url + + +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class StudentProgressUrl(APIView): """ Get the progress url of a student. Limited to staff access. @@ -1730,21 +1751,25 @@ def get_student_progress_url(request, course_id): 'progress_url': '/../...' } """ - course_id = CourseKey.from_string(course_id) - user = get_student_from_identifier(request.POST.get('unique_student_identifier')) - - if course_home_mfe_progress_tab_is_active(course_id): - progress_url = get_learning_mfe_home_url(course_id, url_fragment='progress') - if user is not None: - progress_url += '/{}/'.format(user.id) - else: - progress_url = reverse('student_progress', kwargs={'course_id': str(course_id), 'student_id': user.id}) + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + serializer_class = StudentProgressUrlSerializer + permission_name = permissions.ENROLLMENT_REPORT - response_payload = { - 'course_id': str(course_id), - 'progress_url': progress_url, - } - return JsonResponse(response_payload) + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """Post method for validating incoming data and generating progress URL""" + data = { + 'course_id': course_id, + 'unique_student_identifier': request.data.get('unique_student_identifier') + } + serializer = self.serializer_class(data=data) + serializer.is_valid(raise_exception=True) + return Response(serializer.data) @transaction.non_atomic_requests diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 0b4a88d1b7c6..6b072ef0c12e 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -1,3 +1,4 @@ + """ Instructor API endpoint urls. """ @@ -32,7 +33,7 @@ path('get_students_who_may_enroll', api.get_students_who_may_enroll, name='get_students_who_may_enroll'), path('get_anon_ids', api.get_anon_ids, name='get_anon_ids'), path('get_student_enrollment_status', api.get_student_enrollment_status, name="get_student_enrollment_status"), - path('get_student_progress_url', api.get_student_progress_url, name='get_student_progress_url'), + path('get_student_progress_url', api.StudentProgressUrl.as_view(), name='get_student_progress_url'), path('reset_student_attempts', api.reset_student_attempts, name='reset_student_attempts'), path('rescore_problem', api.rescore_problem, name='rescore_problem'), path('override_problem_score', api.override_problem_score, name='override_problem_score'), From f07e63a6bf9d623ea3f662db98bed1119102910a Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Wed, 7 Aug 2024 16:40:27 +0500 Subject: [PATCH 5/6] feat: Upgrading api list_course_role_members ( 2nd api ) (#35105) * feat: upgrading simple api to drf compatible. (cherry picked from commit 99760f80f0f7d8c16f77fd1741ebb2346d740b4c) --- lms/djangoapps/instructor/views/api.py | 63 ++++++++++--------- lms/djangoapps/instructor/views/api_urls.py | 2 +- lms/djangoapps/instructor/views/serializer.py | 30 +++++++++ 3 files changed, 65 insertions(+), 30 deletions(-) create mode 100644 lms/djangoapps/instructor/views/serializer.py diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 831775ed156d..cde877b9a1bf 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -105,6 +105,7 @@ from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import ReportStore +from lms.djangoapps.instructor.views.serializer import RoleNameSerializer, UserSerializer from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted from openedx.core.djangoapps.course_groups.models import CourseUserGroup @@ -1060,15 +1061,11 @@ def modify_access(request, course_id): return JsonResponse(response_payload) -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.EDIT_COURSE_ACCESS) -@require_post_params(rolename="'instructor', 'staff', or 'beta'") -def list_course_role_members(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ListCourseRoleMembersView(APIView): """ - List instructors and staff. - Requires instructor access. + View to list instructors and staff for a specific course. + Requires the user to have instructor access. rolename is one of ['instructor', 'staff', 'beta', 'ccx_coach'] @@ -1084,33 +1081,41 @@ def list_course_role_members(request, course_id): ] } """ - course_id = CourseKey.from_string(course_id) - course = get_course_with_access( - request.user, 'instructor', course_id, depth=None - ) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.EDIT_COURSE_ACCESS - rolename = request.POST.get('rolename') + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + Handles POST request to list instructors and staff. - if rolename not in ROLES: - return HttpResponseBadRequest() + Args: + request (HttpRequest): The request object containing user data. + course_id (str): The ID of the course to list instructors and staff for. - def extract_user_info(user): - """ convert user into dicts for json view """ + Returns: + Response: A Response object containing the list of instructors and staff or an error message. - return { - 'username': user.username, - 'email': user.email, - 'first_name': user.first_name, - 'last_name': user.last_name, + Raises: + Http404: If the course does not exist. + """ + course_id = CourseKey.from_string(course_id) + course = get_course_with_access( + request.user, 'instructor', course_id, depth=None + ) + role_serializer = RoleNameSerializer(data=request.data) + role_serializer.is_valid(raise_exception=True) + rolename = role_serializer.data['rolename'] + + users = list_with_level(course.id, rolename) + serializer = UserSerializer(users, many=True) + + response_payload = { + 'course_id': str(course_id), + rolename: serializer.data, } - response_payload = { - 'course_id': str(course_id), - rolename: list(map(extract_user_info, list_with_level( - course.id, rolename - ))), - } - return JsonResponse(response_payload) + return Response(response_payload, status=status.HTTP_200_OK) class ProblemResponseReportPostParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 6b072ef0c12e..f35265f0817b 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -23,7 +23,7 @@ urlpatterns = [ path('students_update_enrollment', api.students_update_enrollment, name='students_update_enrollment'), path('register_and_enroll_students', api.register_and_enroll_students, name='register_and_enroll_students'), - path('list_course_role_members', api.list_course_role_members, name='list_course_role_members'), + path('list_course_role_members', api.ListCourseRoleMembersView.as_view(), name='list_course_role_members'), path('modify_access', api.modify_access, name='modify_access'), path('bulk_beta_modify_access', api.bulk_beta_modify_access, name='bulk_beta_modify_access'), path('get_problem_responses', api.get_problem_responses, name='get_problem_responses'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py new file mode 100644 index 000000000000..b4f6f7626013 --- /dev/null +++ b/lms/djangoapps/instructor/views/serializer.py @@ -0,0 +1,30 @@ +""" Instructor apis serializers. """ + +from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.core.exceptions import ValidationError +from django.utils.translation import gettext as _ +from rest_framework import serializers + +from lms.djangoapps.instructor.access import ROLES + + +class RoleNameSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer that describes the response of the problem response report generation API. + """ + + rolename = serializers.CharField(help_text=_("Role name")) + + def validate_rolename(self, value): + """ + Check that the rolename is valid. + """ + if value not in ROLES: + raise ValidationError(_("Invalid role name.")) + return value + + +class UserSerializer(serializers.ModelSerializer): + class Meta: + model = User + fields = ['username', 'email', 'first_name', 'last_name'] From 232586c9994156583f588740f7bb0ad478d0eeec Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Tue, 20 Aug 2024 14:25:17 +0200 Subject: [PATCH 6/6] feat: upgrading simple api to drf compatible. (#35260) (cherry picked from commit af9ae77bbc35c2ac5b20a487c538d3f9a7fd6fb8) --- lms/djangoapps/instructor/views/api.py | 122 +++++++++--------- lms/djangoapps/instructor/views/api_urls.py | 2 +- lms/djangoapps/instructor/views/serializer.py | 31 +++++ 3 files changed, 95 insertions(+), 60 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index cde877b9a1bf..244233a95f13 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -105,7 +105,7 @@ from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import ReportStore -from lms.djangoapps.instructor.views.serializer import RoleNameSerializer, UserSerializer +from lms.djangoapps.instructor.views.serializer import RoleNameSerializer, UserSerializer, AccessSerializer from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted from openedx.core.djangoapps.course_groups.models import CourseUserGroup @@ -980,17 +980,8 @@ def bulk_beta_modify_access(request, course_id): return JsonResponse(response_payload) -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.EDIT_COURSE_ACCESS) -@require_post_params( - unique_student_identifier="email or username of user to change access", - rolename="'instructor', 'staff', 'beta', or 'ccx_coach'", - action="'allow' or 'revoke'" -) -@common_exceptions_400 -def modify_access(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ModifyAccess(APIView): """ Modify staff/instructor access of other user. Requires instructor access. @@ -1002,64 +993,77 @@ def modify_access(request, course_id): rolename is one of ['instructor', 'staff', 'beta', 'ccx_coach'] action is one of ['allow', 'revoke'] """ - course_id = CourseKey.from_string(course_id) - course = get_course_with_access( - request.user, 'instructor', course_id, depth=None - ) - try: - user = get_student_from_identifier(request.POST.get('unique_student_identifier')) - except User.DoesNotExist: - response_payload = { - 'unique_student_identifier': request.POST.get('unique_student_identifier'), - 'userDoesNotExist': True, - } - return JsonResponse(response_payload) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.EDIT_COURSE_ACCESS + serializer_class = AccessSerializer - # Check that user is active, because add_users - # in common/djangoapps/student/roles.py fails - # silently when we try to add an inactive user. - if not user.is_active: - response_payload = { - 'unique_student_identifier': user.username, - 'inactiveUser': True, - } - return JsonResponse(response_payload) + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + Modify staff/instructor access of other user. + Requires instructor access. + """ + course_id = CourseKey.from_string(course_id) + course = get_course_with_access( + request.user, 'instructor', course_id, depth=None + ) - rolename = request.POST.get('rolename') - action = request.POST.get('action') + serializer_data = AccessSerializer(data=request.data) + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - if rolename not in ROLES: - error = strip_tags(f"unknown rolename '{rolename}'") - log.error(error) - return HttpResponseBadRequest(error) + user = serializer_data.validated_data.get('unique_student_identifier') + if not user: + response_payload = { + 'unique_student_identifier': request.data.get('unique_student_identifier'), + 'userDoesNotExist': True, + } + return JsonResponse(response_payload) + + if not user.is_active: + response_payload = { + 'unique_student_identifier': user.username, + 'inactiveUser': True, + } + return JsonResponse(response_payload) + + rolename = serializer_data.data['rolename'] + action = serializer_data.data['action'] + + if rolename not in ROLES: + error = strip_tags(f"unknown rolename '{rolename}'") + log.error(error) + return HttpResponseBadRequest(error) + + # disallow instructors from removing their own instructor access. + if rolename == 'instructor' and user == request.user and action != 'allow': + response_payload = { + 'unique_student_identifier': user.username, + 'rolename': rolename, + 'action': action, + 'removingSelfAsInstructor': True, + } + return JsonResponse(response_payload) + + if action == 'allow': + allow_access(course, user, rolename) + if not is_user_enrolled_in_course(user, course_id): + CourseEnrollment.enroll(user, course_id) + elif action == 'revoke': + revoke_access(course, user, rolename) + else: + return HttpResponseBadRequest(strip_tags( + f"unrecognized action u'{action}'" + )) - # disallow instructors from removing their own instructor access. - if rolename == 'instructor' and user == request.user and action != 'allow': response_payload = { 'unique_student_identifier': user.username, 'rolename': rolename, 'action': action, - 'removingSelfAsInstructor': True, + 'success': 'yes', } return JsonResponse(response_payload) - if action == 'allow': - allow_access(course, user, rolename) - elif action == 'revoke': - revoke_access(course, user, rolename) - else: - return HttpResponseBadRequest(strip_tags( - f"unrecognized action u'{action}'" - )) - - response_payload = { - 'unique_student_identifier': user.username, - 'rolename': rolename, - 'action': action, - 'success': 'yes', - } - return JsonResponse(response_payload) - @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') class ListCourseRoleMembersView(APIView): diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index f35265f0817b..ca535d3da211 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -24,7 +24,7 @@ path('students_update_enrollment', api.students_update_enrollment, name='students_update_enrollment'), path('register_and_enroll_students', api.register_and_enroll_students, name='register_and_enroll_students'), path('list_course_role_members', api.ListCourseRoleMembersView.as_view(), name='list_course_role_members'), - path('modify_access', api.modify_access, name='modify_access'), + path('modify_access', api.ModifyAccess.as_view(), name='modify_access'), path('bulk_beta_modify_access', api.bulk_beta_modify_access, name='bulk_beta_modify_access'), path('get_problem_responses', api.get_problem_responses, name='get_problem_responses'), path('get_grading_config', api.get_grading_config, name='get_grading_config'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index b4f6f7626013..463f05ad45b8 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -4,6 +4,7 @@ from django.core.exceptions import ValidationError from django.utils.translation import gettext as _ from rest_framework import serializers +from .tools import get_student_from_identifier from lms.djangoapps.instructor.access import ROLES @@ -28,3 +29,33 @@ class UserSerializer(serializers.ModelSerializer): class Meta: model = User fields = ['username', 'email', 'first_name', 'last_name'] + + +class AccessSerializer(serializers.Serializer): + """ + Serializer for managing user access changes. + This serializer validates and processes the data required to modify + user access within a system. + """ + unique_student_identifier = serializers.CharField( + max_length=255, + help_text="Email or username of user to change access" + ) + rolename = serializers.CharField( + help_text="Role name to assign to the user" + ) + action = serializers.ChoiceField( + choices=['allow', 'revoke'], + help_text="Action to perform on the user's access" + ) + + def validate_unique_student_identifier(self, value): + """ + Validate that the unique_student_identifier corresponds to an existing user. + """ + try: + user = get_student_from_identifier(value) + except User.DoesNotExist: + return None + + return user