Tighten security around assessment sign offs and resource uploads

loads/forms.py

@@ -265,6 +265,12 @@ class Meta:
 
 
 class AssessmentStateSignOffForm(ModelForm):
+    """Form for adding assessment sign offs"""
+
+    def __init__(self, *args, **kwargs):
+        self.user = kwargs.pop('user', None)
+        super(AssessmentStateSignOffForm, self).__init__(*args, **kwargs)
+
     class Meta:
         model = AssessmentStateSignOff
         fields = ['assessment_state', 'notes', 'module', 'signed_by']
@@ -274,7 +280,8 @@ def clean(self):
         """Check the user can invoke this state"""
         assessment_state = self.cleaned_data['assessment_state']
         module = self.cleaned_data['module']
-        signed_by = self.cleaned_data['signed_by']
+        # Override with the passed in user (which should be request.user) for security reasons
+        signed_by = self.user
         staff = Staff.objects.get(user=signed_by)
 
         if not assessment_state.can_be_set_by(staff, module):

loads/tests_clients.py

@@ -160,6 +160,14 @@ def setUp(self):
         module.programmes.add(other_programme)
         module.save()
 
+        # Module ABC101 is in programme 123 as a lead programme
+        # Module ABC101 in in programme 456 as a programme (but not lead)
+
+        # Programme 123 has an examiner "externalA" (a lead examiner)
+        # Programme 456 has an examiner "externalB" (an examiner but not lead)
+
+        # So "external" and "externalC" are externals with nothing to examine
+
         # and staffB on teaching team
         ModuleStaff.objects.create(
             module=module,
@@ -304,7 +312,7 @@ def test_superuser_index_pages(self):
         self.assertEqual(response.status_code, 200)
 
 
-    def test_external_index_pages(self):
+    def test_external_no_role_index_pages(self):
         """This checks that an External Examiner can access the various index pages, and not others"""
 
         # Log the User in
@@ -427,9 +435,6 @@ def test_staff_no_role_module_pages(self):
         response = self.client.get("/modules/details/%u" % module.id)
         self.assertEqual(response.status_code, 200)
 
-        response = self.client.get("/modules/add_assessment_resource/%u" % module.id)
-        self.assertEqual(response.status_code, 200)
-
         response = self.client.get("/modules/add_assessment_sign_off/%u" % module.id)
         self.assertEqual(response.status_code, 200)
 
@@ -447,12 +452,15 @@ def test_staff_no_role_module_pages(self):
         response = self.client.get("/modules/delete/%u" % module.id)
         self.assertEqual(response.status_code, 403)
 
+        response = self.client.get("/modules/add_assessment_resource/%u" % module.id)
+        self.assertEqual(response.status_code, 403)
+
 
-    def test_external_module_pages(self):
+    def test_external_no_role_module_pages(self):
         """This checks that a Staff member with no specific has appropriate module views"""
 
         # Log the User in
-        user = User.objects.get(username='external')
+        user = User.objects.get(username='externalC')
         staff = Staff.objects.get(user=user)
         # force_login bypasses potential custom authentication back ends
         self.client.force_login(user)
@@ -463,6 +471,42 @@ def test_external_module_pages(self):
         response = self.client.get("/modules/details/%u" % module.id)
         self.assertNotEqual(response.status_code, 200)
 
+        response = self.client.get("/modules/add_assessment_sign_off/%u" % module.id)
+        self.assertEqual(response.status_code, 200)
+
+        # These views should be response code 404 (Not Found)
+        response = self.client.get("/modules/details/9999")
+        self.assertEqual(response.status_code, 404)
+
+        # These views should be response code 403 (Forbidden)
+        response = self.client.get("/modules/create/")
+        self.assertEqual(response.status_code, 403)
+
+        response = self.client.get("/modules/update/%u" % module.id)
+        self.assertEqual(response.status_code, 403)
+
+        response = self.client.get("/modules/delete/%u" % module.id)
+        self.assertEqual(response.status_code, 403)
+
+        response = self.client.get("/modules/add_assessment_resource/%u" % module.id)
+        self.assertEqual(response.status_code, 403)
+
+
+    def test_external_with_role_module_pages(self):
+        """This checks that a Staff member with no specific has appropriate module views"""
+
+        # Log the User in
+        user = User.objects.get(username='externalA')
+        staff = Staff.objects.get(user=user)
+        # force_login bypasses potential custom authentication back ends
+        self.client.force_login(user)
+
+        module = Module.objects.get(module_code="ABC101")
+
+        # Should be OK
+        response = self.client.get("/modules/details/%u" % module.id)
+        self.assertEqual(response.status_code, 200)
+
         response = self.client.get("/modules/add_assessment_resource/%u" % module.id)
         self.assertEqual(response.status_code, 200)
 

loads/views.py

@@ -1097,10 +1097,30 @@ def add_assessment_resource(request, module_id):
     # Get the module itself
     module = get_object_or_404(Module, pk=module_id)
 
-    # Check for a valid permission at this stage
-    # can_override = request.user.has_perm('loads.add_assessment_resource')
-    # if not can_override:
-    #    return HttpResponseRedirect(reverse('forbidden'))
+    # Assume a lack of permission, unless a coordinator, teaching team member, moderator, examiner, or superuser
+    permission = False
+    logger.debug("[%s] checking status for module %s, pre upload" % (request.user, module))
+    if staff is module.coordinator:
+        logger.debug("[%s] is a module coordinator" % request.user)
+        permission = True
+    elif staff in module.moderators.all():
+        logger.debug("[%s] is a module moderator" % request.user)
+        permission = True
+    elif staff in ModuleStaff.objects.filter(module=module):
+        logger.debug("[%s] is on the module team" % request.user)
+        permission = True
+    elif staff.can_examine_module(module):
+        logger.debug("[%s] is a module examiner" % request.user)
+        permission = True
+    elif staff in AssessmentStaff.objects.filter(package=module.package):
+        logger.debug("[%s] is on the assessment team for the package" % request.user)
+        permission = True
+    elif staff.user.is_superuser:
+        logger.debug("[%s] is a superuser" % request.user)
+        permission = True
+
+    if not permission:
+        raise PermissionDenied("Sorry, you do not have permission to upload this resource")
 
     # if this is a POST request we need to process the form data
     if request.method == 'POST':
@@ -1571,7 +1591,7 @@ def add_assessment_sign_off(request, module_id):
     # if this is a POST request we need to process the form data
     if request.method == 'POST':
         # create a form instance and populate it with data from the request:
-        form = AssessmentStateSignOffForm(request.POST)
+        form = AssessmentStateSignOffForm(request.POST, user=request.user)
 
         # check whether it's valid:
         if form.is_valid():
@@ -1588,7 +1608,7 @@ def add_assessment_sign_off(request, module_id):
 
     # if a GET (or any other method) we'll create a blank form
     else:
-        form = AssessmentStateSignOffForm()
+        form = AssessmentStateSignOffForm(user=request.user)
         form.fields['assessment_state'].queryset = next_states
         form.fields['signed_by'].initial = request.user
         form.fields['module'].initial = module