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..244233a95f13 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, 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 @@ -122,6 +123,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, @@ -978,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. @@ -1000,74 +993,83 @@ 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) + + 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 rolename not in ROLES: - error = strip_tags(f"unknown rolename '{rolename}'") - log.error(error) - return HttpResponseBadRequest(error) + 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) - - -@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'] @@ -1083,33 +1085,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 @@ -1712,15 +1722,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 +1760,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..ca535d3da211 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. """ @@ -22,8 +23,8 @@ 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('modify_access', api.modify_access, name='modify_access'), + path('list_course_role_members', api.ListCourseRoleMembersView.as_view(), name='list_course_role_members'), + 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'), @@ -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'), diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py new file mode 100644 index 000000000000..463f05ad45b8 --- /dev/null +++ b/lms/djangoapps/instructor/views/serializer.py @@ -0,0 +1,61 @@ +""" 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 .tools import get_student_from_identifier + +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'] + + +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 diff --git a/lms/envs/common.py b/lms/envs/common.py index 419c966e1bb1..70112a5cf45f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3344,6 +3344,18 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ######################### Django Rest Framework ######################## 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', 'DEFAULT_RENDERER_CLASSES': ( 'rest_framework.renderers.JSONRenderer', @@ -3360,7 +3372,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..5150304339db --- /dev/null +++ b/openedx/core/djangolib/default_auth_classes.py @@ -0,0 +1,55 @@ +""" +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 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 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)