Fix/file attachment report auth

compair/api/__init__.py

@@ -12,8 +12,8 @@
 from bouncer.constants import READ
 from compair.authorization import require
 from compair.kaltura import KalturaAPI
-from compair.models import File
-from compair.core import event
+from compair.models import File, SystemRole, CourseRole, Course
+from compair.core import event, abort
 on_get_file = event.signal('GET_FILE')
 
 attachment_download_parser = RequestParser()
@@ -251,15 +251,9 @@ def file_retrieve(file_type, file_name):
         if file_type == 'attachment':
             attachment = File.get_by_file_name_or_404(file_name)
 
-            for answer in attachment.answers:
-                require(READ, answer,
-                    title="Attachment Unavailable",
-                    message="Sorry, your role does not allow you to view the attachment.")
-
-            for assignment in attachment.assignments:
-                require(READ, assignment,
-                    title="Attachment Unavailable",
-                    message="Sorry, your role does not allow you to view the attachment.")
+            require(READ, attachment,
+                title="Attachment Unavailable",
+                message="Sorry, your role does not allow you to view the attachment.")
 
             # If attachment is in Kaltura, redirect the user
             if attachment.kaltura_media and KalturaAPI.enabled():
@@ -270,10 +264,30 @@ def file_retrieve(file_type, file_name):
                     kaltura_url = KalturaAPI.get_direct_access_url(entry_id, download_url, 60)
                     return redirect(kaltura_url)
 
+        elif file_type == 'report':
+            user = current_user._get_current_object()
+            is_admin = user.system_role == SystemRole.sys_admin
+
+            if not is_admin:
+                uuid_match = re.search(r'--([A-Za-z0-9_-]{22})\.[^.]+$', file_name)
+                if not uuid_match:
+                    abort(403, title="Report Unavailable",
+                        message="Sorry, your role does not allow you to view reports.")
+
+                course_uuid = uuid_match.group(1)
+                course = Course.query.filter_by(uuid=course_uuid).one_or_none()
+                has_access = course is not None and any(
+                    uc.course_id == course.id and
+                    uc.course_role in (CourseRole.instructor, CourseRole.teaching_assistant)
+                    for uc in user.user_courses
+                )
+                if not has_access:
+                    abort(403, title="Report Unavailable",
+                        message="Sorry, your role does not allow you to view reports.")
+
         if not os.path.exists(file_path):
             return make_response('invalid file name', 404)
 
-        # TODO: add bouncer for reports
         mimetype, encoding = mimetypes.guess_type(file_name)
         attachment_filename = None
         as_attachment = False

compair/api/report.py

@@ -41,7 +41,7 @@ def name_generator(course, report_name, group, file_type="csv"):
     # from https://gist.github.com/seanh/93666
     # return a file system safe filename
     valid_chars = "-_.() %s%s" % (string.ascii_letters, string.digits)
-    filename = course.name + "-" + group_name_output + report_name + "--" + date + "." + file_type
+    filename = course.name + "-" + group_name_output + report_name + "--" + date + "--" + course.uuid + "." + file_type
     return ''.join(char for char in filename if char in valid_chars)
 
 

compair/authorization.py

@@ -42,6 +42,17 @@ def if_my_student(student):
             .count()
         return bool(exists)
 
+    def if_can_read_attachment(file):
+        for assignment in file.assignments.all():
+            if can(READ, assignment):
+                return True
+
+        for answer in file.answers.all():
+            if can(READ, answer):
+                return True
+
+        return file.user_id == user.id
+
     def if_can_delete_attachment_reference(file):
         for assignment in file.assignments.all():
             if can(DELETE, assignment):
@@ -75,6 +86,8 @@ def if_can_delete_attachment_reference(file):
     # they can read and edit their own criteria
     they.can((READ, EDIT), Criterion, user_id=user.id)
 
+    # they can read files linked to content they can access, or that they uploaded
+    they.can(READ, File, if_can_read_attachment)
     # they can delete their own attachments
     they.can(DELETE, File, user_id=user.id)
 
@@ -217,11 +230,7 @@ def require(operation, target, title=None, message=None):
     try:
         ensure(operation, target)
     except Forbidden as e:
-        if not title:
-            title = "Forbidden"
-        if not message:
-            message = e.description
-        abort(403, title=title, message=message)
+        abort(403, title=title or "Forbidden", message=message or e.description)
 
 
 def is_user_access_restricted(user):

compair/models/comparison.py

@@ -299,7 +299,7 @@ def create_new_comparison(cls, assignment_id, user_id, skip_comparison_examples)
                 winner=None,
                 content=None,
             )
-            db.session.add(comparison)
+            db.session.add(comparison_criterion)
         db.session.commit()
 
         return comparison

compair/models/file.py

@@ -51,18 +51,16 @@ def get_by_uuid_or_404(cls, model_uuid, joinedloads=[], title=None, message=None
 
     @classmethod
     def get_by_file_name_or_404(cls, filename, joinedloads=[], title=None, message=None):
-        if not title:
-            title = "Attachment Unavailable"
-        if not message:
-            message = "Sorry, this attachment was deleted or is no longer accessible."
-
         query = cls.query
         for load_option in joinedloads:
             query = query.options(load_option)
 
         model = query.filter_by(name=filename).one_or_none()
         if model is None:
-            abort(404, title=title, message=message)
+            abort(404,
+                title=title or "Attachment Unavailable",
+                message=message or "Sorry, this attachment was deleted or is no longer accessible."
+            )
         return model
 
     @classmethod

compair/tests/api/test_file.py

@@ -44,11 +44,13 @@ def test_view_file(self):
         rv = self.client.get(url)
         self.assert401(rv)
 
-        # TODO: no authorization control right now and needs to be added in the future
         # test unauthorized user
-        # with self.login(self.fixtures.unauthorized_instructor.username):
-        #     rv = self.client.get(url)
-        #     self.assert403(rv)
+        with self.login(self.fixtures.unauthorized_instructor.username):
+            file_exists = mock.patch('compair.api.os.path.exists', return_value=True)
+            mock_send_file = mock.patch('compair.api.send_file', return_value=make_response("OK"))
+            with file_exists, mock_send_file:
+                rv = self.client.get(url)
+                self.assert403(rv)
 
         # valid instructor
         with self.login(self.fixtures.instructor.username):
@@ -111,6 +113,97 @@ def test_view_file(self):
                         mock_send_file.reset_mock()
 
 
+    def test_view_attachment_linked_to_assignment(self):
+        db_file = self.fixtures.add_file(self.fixtures.instructor)
+        self.fixtures.assignment.file_id = db_file.id
+        db.session.commit()
+
+        url = self.base_url + '/attachment/' + db_file.name
+
+        # unauthorized instructor (not enrolled in the course) cannot read the assignment
+        with self.login(self.fixtures.unauthorized_instructor.username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert403(rv)
+
+        # enrolled student can read the assignment, so can access the attachment
+        with self.login(self.fixtures.students[0].username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert200(rv)
+
+    def test_view_attachment_linked_to_answer(self):
+        # answers[0] belongs to students[0] (num_answers=1 in setUp)
+        student = self.fixtures.students[0]
+        answer = self.fixtures.answers[0]
+        db_file = self.fixtures.add_file(student)
+        answer.file_id = db_file.id
+        db.session.commit()
+
+        url = self.base_url + '/attachment/' + db_file.name
+
+        # unauthorized instructor cannot read the answer
+        with self.login(self.fixtures.unauthorized_instructor.username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert403(rv)
+
+        # the student who submitted the answer can access its attachment
+        with self.login(student.username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert200(rv)
+
+    def test_view_report(self):
+        course_uuid = self.fixtures.course.uuid
+        report_name = 'CourseName-participation--2026-05-01--{}.csv'.format(course_uuid)
+        url = self.base_url + '/report/' + report_name
+
+        # unauthenticated
+        rv = self.client.get(url)
+        self.assert401(rv)
+
+        # student cannot access reports
+        with self.login(self.fixtures.students[0].username):
+            rv = self.client.get(url)
+            self.assert403(rv)
+
+        # instructor from a different course cannot access this course's report
+        with self.login(self.fixtures.unauthorized_instructor.username):
+            rv = self.client.get(url)
+            self.assert403(rv)
+
+        # filename with no course UUID is rejected for non-admins
+        with self.login(self.fixtures.instructor.username):
+            rv = self.client.get(self.base_url + '/report/test_report.csv')
+            self.assert403(rv)
+
+        # TA enrolled in the course can access its report
+        with self.login(self.fixtures.ta.username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert200(rv)
+
+        # instructor enrolled in the course can access its report
+        with self.login(self.fixtures.instructor.username), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert200(rv)
+
+        # sys admin can access any report regardless of course UUID
+        with self.login('root', 'password'), \
+                mock.patch('compair.api.os.path.exists', return_value=True), \
+                mock.patch('compair.api.send_file', return_value=make_response("OK")):
+            rv = self.client.get(url)
+            self.assert200(rv)
+
+
     def test_create_attachment(self):
         url = '/api/attachment'
         test_formats = [

requirements.txt

@@ -38,6 +38,7 @@ Werkzeug==3.1.6
 #lti==0.9.2
 # required for the lti library
 oauthlib==3.2.2
+requests==2.33.0
 requests-oauthlib==1.3.1
 markupsafe==2.1.5
 itsdangerous==2.2.0