diff --git a/edx_exams/apps/api/permissions.py b/edx_exams/apps/api/permissions.py index b88699aa..22c2c350 100644 --- a/edx_exams/apps/api/permissions.py +++ b/edx_exams/apps/api/permissions.py @@ -2,20 +2,25 @@ from rest_framework.permissions import BasePermission -class StaffUserPermissions(BasePermission): - """ Permission class to check if user is staff """ +class CourseStaffUserPermissions(BasePermission): + """ Permission class to check if user has course staff permissions """ def has_permission(self, request, view): + if not request.user.is_authenticated: + return False + + if view.kwargs.get('course_id'): + return request.user.is_staff or request.user.has_course_staff_permission(view.kwargs['course_id']) return request.user.is_staff -class StaffUserOrReadOnlyPermissions(BasePermission): +class CourseStaffOrReadOnlyPermissions(CourseStaffUserPermissions): """ - Permission class granting write access to staff users and - read-only access to authenticated users + Permission class granting write access to course staff users + and read-only access to other authenticated users """ def has_permission(self, request, view): - return request.user.is_staff or ( - request.user.is_authenticated and - request.method == 'GET' - ) + if request.user.is_authenticated and request.method == 'GET': + return True + else: + return super().has_permission(request, view) diff --git a/edx_exams/apps/api/v1/urls.py b/edx_exams/apps/api/v1/urls.py index 9323bc82..c783dc89 100644 --- a/edx_exams/apps/api/v1/urls.py +++ b/edx_exams/apps/api/v1/urls.py @@ -53,8 +53,8 @@ LatestExamAttemptView.as_view(), name='exams-attempt-latest', ), - path( - 'instructor_view/attempts', + re_path( + fr'instructor_view/course_id/{COURSE_ID_PATTERN}/attempts', InstructorAttemptsListView.as_view(), name='instructor-attempts-list' ), diff --git a/edx_exams/apps/api/v1/views.py b/edx_exams/apps/api/v1/views.py index 48d857ee..bf53f4e5 100644 --- a/edx_exams/apps/api/v1/views.py +++ b/edx_exams/apps/api/v1/views.py @@ -15,7 +15,7 @@ from rest_framework.response import Response from token_utils.api import sign_token_for -from edx_exams.apps.api.permissions import StaffUserOrReadOnlyPermissions, StaffUserPermissions +from edx_exams.apps.api.permissions import CourseStaffOrReadOnlyPermissions, CourseStaffUserPermissions from edx_exams.apps.api.serializers import ( ExamSerializer, InstructorViewAttemptSerializer, @@ -82,7 +82,7 @@ class CourseExamsView(ExamsAPIView): """ authentication_classes = (JwtAuthentication,) - permission_classes = (StaffUserPermissions,) + permission_classes = (CourseStaffUserPermissions,) @classmethod def update_exam(cls, exam_object, fields): @@ -239,7 +239,7 @@ class CourseExamConfigurationsView(ExamsAPIView): """ authentication_classes = (JwtAuthentication,) - permission_classes = (StaffUserOrReadOnlyPermissions,) + permission_classes = (CourseStaffOrReadOnlyPermissions,) def get(self, request, course_id): """ @@ -321,7 +321,7 @@ class ExamAccessTokensView(ExamsAPIView): """ authentication_classes = (JwtAuthentication,) - permission_classes = (StaffUserOrReadOnlyPermissions,) + permission_classes = (CourseStaffOrReadOnlyPermissions,) @classmethod def get_expiration_window(cls, exam_attempt, default_exp_seconds): @@ -552,7 +552,8 @@ def put(self, request, attempt_id): ) action_mapping = {} - if request.user.is_staff: + course_id = attempt.exam.course_id + if request.user.is_staff or request.user.has_course_staff_permission(course_id): action_mapping = { 'verify': ExamAttemptStatus.verified, 'reject': ExamAttemptStatus.rejected, @@ -569,7 +570,7 @@ def put(self, request, attempt_id): else: error_msg = ( f'user_id={attempt.user.id} attempted to update attempt_id={attempt.id} in ' - f'course_id={attempt.exam.course_id} but does not have access to it. (action={action})' + f'course_id={course_id} but does not have access to it. (action={action})' ) error = {'detail': error_msg} return Response(status=status.HTTP_403_FORBIDDEN, data=error) @@ -612,8 +613,11 @@ def delete(self, request, attempt_id): data={'detail': f'Attempt with attempt_id={attempt_id} does not exist.'} ) - # TODO: this staff check will be updated once an instructor role is added - if not request.user.is_staff and exam_attempt.user.id != request.user.id: + if ( + exam_attempt.user.id != request.user.id and + not request.user.is_staff and + not request.user.has_course_staff_permission(exam_attempt.exam.course_id) + ): error_msg = ( f'user_id={exam_attempt.user.id} attempted to delete attempt_id={exam_attempt.id} in ' f'course_id={exam_attempt.exam.course_id} but does not have access to it.' @@ -637,9 +641,9 @@ class InstructorAttemptsListView(ExamsAPIView): """ authentication_classes = (JwtAuthentication,) - permission_classes = (StaffUserPermissions,) + permission_classes = (CourseStaffUserPermissions,) - def get(self, request): + def get(self, request, course_id): # pylint: disable=unused-argument """ HTTP GET handler to fetch all exam attempt data for a given exam. diff --git a/edx_exams/apps/core/admin.py b/edx_exams/apps/core/admin.py index 5f487345..dbac2d25 100644 --- a/edx_exams/apps/core/admin.py +++ b/edx_exams/apps/core/admin.py @@ -6,7 +6,15 @@ from edx_exams.apps.lti.utils import get_lti_root -from .models import CourseExamConfiguration, Exam, ExamAttempt, ProctoringProvider, User +from .models import ( + AssessmentControlResult, + CourseExamConfiguration, + CourseStaffRole, + Exam, + ExamAttempt, + ProctoringProvider, + User +) class CustomUserAdmin(UserAdmin): @@ -63,8 +71,25 @@ class CourseExamConfigurationAdmin(admin.ModelAdmin): ordering = ('course_id',) +class AssessmentControlResultAdmin(admin.ModelAdmin): + """ Admin configuration for the AssessmentControlResult model """ + list_display = ('user', 'course_id', 'exam_name') + search_fields = ('user__username', 'course_id', 'exam_name') + ordering = ('-modified',) + + +class CourseStaffRoleAdmin(admin.ModelAdmin): + """ Admin configuration for the Course Staff Role model """ + list_display = ('user', 'course_id') + list_filter = ('course_id',) + search_fields = ('user__username', 'course_id') + ordering = ('course_id',) + + admin.site.register(User, CustomUserAdmin) admin.site.register(ProctoringProvider, ProctoringProviderAdmin) admin.site.register(Exam, ExamAdmin) admin.site.register(ExamAttempt, ExamAttemptAdmin) admin.site.register(CourseExamConfiguration, CourseExamConfigurationAdmin) +admin.site.register(AssessmentControlResult, AssessmentControlResultAdmin) +admin.site.register(CourseStaffRole, CourseStaffRoleAdmin) diff --git a/edx_exams/apps/core/migrations/0018_staff_roles.py b/edx_exams/apps/core/migrations/0018_staff_roles.py new file mode 100644 index 00000000..18bfda83 --- /dev/null +++ b/edx_exams/apps/core/migrations/0018_staff_roles.py @@ -0,0 +1,41 @@ +# Generated by Django 3.2.20 on 2023-08-14 23:00 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0017_assessmentcontrolresult'), + ] + + operations = [ + migrations.AlterField( + model_name='examattempt', + name='status', + field=models.CharField(choices=[('created', 'created'), ('download_software_clicked', 'download_software_clicked'), ('ready_to_start', 'ready_to_start'), ('started', 'started'), ('ready_to_submit', 'ready_to_submit'), ('timed_out', 'timed_out'), ('submitted', 'submitted'), ('verified', 'verified'), ('rejected', 'rejected'), ('expired', 'expired'), ('second_review_required', 'second_review_required'), ('error', 'error')], max_length=64), + ), + migrations.AlterField( + model_name='historicalexamattempt', + name='status', + field=models.CharField(choices=[('created', 'created'), ('download_software_clicked', 'download_software_clicked'), ('ready_to_start', 'ready_to_start'), ('started', 'started'), ('ready_to_submit', 'ready_to_submit'), ('timed_out', 'timed_out'), ('submitted', 'submitted'), ('verified', 'verified'), ('rejected', 'rejected'), ('expired', 'expired'), ('second_review_required', 'second_review_required'), ('error', 'error')], max_length=64), + ), + migrations.CreateModel( + name='CourseStaffRole', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('course_id', models.CharField(db_index=True, max_length=255)), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'course staff role', + 'db_table': 'exams_coursestaffrole', + }, + ), + ] diff --git a/edx_exams/apps/core/models.py b/edx_exams/apps/core/models.py index 260f2226..35b3a37c 100644 --- a/edx_exams/apps/core/models.py +++ b/edx_exams/apps/core/models.py @@ -47,6 +47,28 @@ class Meta: def get_full_name(self): return self.full_name or super().get_full_name() + def has_course_staff_permission(self, course_id): + """ + Return True if the user is a staff member for the given course. + """ + return self.is_staff or CourseStaffRole.objects.filter(user_id=self.id, course_id=course_id).exists() + + +class CourseStaffRole(TimeStampedModel): + """ + Users with staff access to a course. + + .. no_pii: + """ + user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) + + course_id = models.CharField(max_length=255, db_index=True) + + class Meta: + """ Meta class for this Django model """ + db_table = 'exams_coursestaffrole' + verbose_name = 'course staff role' + class ProctoringProvider(TimeStampedModel): """ @@ -288,6 +310,18 @@ class Meta: db_table = 'exams_assessmentcontrolresult' verbose_name = 'assessment control result' + @property + def user(self): + return self.attempt.user + + @property + def course_id(self): + return self.attempt.exam.course_id + + @property + def exam_name(self): + return self.attempt.exam.exam_name + class CourseExamConfiguration(TimeStampedModel): """ diff --git a/edx_exams/apps/lti/views.py b/edx_exams/apps/lti/views.py index 3c3122fb..6948faf3 100644 --- a/edx_exams/apps/lti/views.py +++ b/edx_exams/apps/lti/views.py @@ -340,12 +340,6 @@ def launch_instructor_tool(request, exam_id): View to initiate an LTI launch of the Instructor Tool for an exam. """ user = request.user - - # TODO: this should eventually be replaced with a permission check - # for course staff - if not user.is_staff: - return Response(status=status.HTTP_403_FORBIDDEN) - exam = get_exam_by_id(exam_id) if not exam: return Response( @@ -353,6 +347,9 @@ def launch_instructor_tool(request, exam_id): data={'detail': f'Exam with exam_id={exam_id} does not exist.'} ) + if not user.is_staff or not user.has_course_staff_permission(exam.course_id): + return Response(status=status.HTTP_403_FORBIDDEN) + lti_config_id = exam.provider.lti_configuration_id lti_config = LtiConfiguration.objects.get(id=lti_config_id) launch_data = Lti1p3LaunchData( diff --git a/edx_exams/apps/router/views.py b/edx_exams/apps/router/views.py index fabbd8fe..a6a111f7 100644 --- a/edx_exams/apps/router/views.py +++ b/edx_exams/apps/router/views.py @@ -11,7 +11,7 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.views import APIView -from edx_exams.apps.api.permissions import StaffUserPermissions +from edx_exams.apps.api.permissions import CourseStaffUserPermissions from edx_exams.apps.core.exam_types import get_exam_type from edx_exams.apps.router.interop import get_provider_settings, get_student_exam_attempt_data, register_exams @@ -25,7 +25,7 @@ class CourseExamsLegacyView(APIView): Forwards all requests to edx-proctoring """ authentication_classes = (JwtAuthentication,) - permission_classes = (StaffUserPermissions,) + permission_classes = (CourseStaffUserPermissions,) def patch(self, request, course_id): """