From 28e1add0915915ad088676025b418a77b88b9f0b Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 17:10:18 -0700 Subject: [PATCH 1/7] Fix file attachment and report authorization - Replace broken per-answer/assignment require loop with a proper bouncer rule (if_can_read_attachment) that grants access if the user can read any linked assignment or answer, or uploaded the file - Add authorization gate for report file type (was a TODO) - Activate previously-skipped unauthorized access test --- compair/api/__init__.py | 29 +++++++++++++++++------------ compair/authorization.py | 19 ++++++++++++++----- compair/models/file.py | 10 ++++------ compair/tests/api/test_file.py | 10 ++++++---- 4 files changed, 41 insertions(+), 27 deletions(-) diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 63ffd741..7ec68dae 100644 --- a/compair/api/__init__.py +++ b/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 +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,21 @@ 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 + is_instructor_or_ta = any( + uc.course_role in (CourseRole.instructor, CourseRole.teaching_assistant) + for uc in user.user_courses + if uc.course_role != CourseRole.dropped + ) + if not is_admin and not is_instructor_or_ta: + 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 diff --git a/compair/authorization.py b/compair/authorization.py index bdedb7f0..1bd4c1d3 100644 --- a/compair/authorization.py +++ b/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): diff --git a/compair/models/file.py b/compair/models/file.py index 66eef79d..cc18c29a 100644 --- a/compair/models/file.py +++ b/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 diff --git a/compair/tests/api/test_file.py b/compair/tests/api/test_file.py index a8bae373..77aa8054 100644 --- a/compair/tests/api/test_file.py +++ b/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): From 3fbf2e75b7a8580b3b4513c29e6acfd9db6af2e4 Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 17:10:34 -0700 Subject: [PATCH 2/7] pin requests to 2.33.0 --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index 00df3a02..f8c385db 100644 --- a/requirements.txt +++ b/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 From e1f362b98874a1c1256e9a8c73741a54edce6796 Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 17:36:12 -0700 Subject: [PATCH 3/7] add missing authorization tests for file attachment and report access --- compair/tests/api/test_file.py | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/compair/tests/api/test_file.py b/compair/tests/api/test_file.py index 77aa8054..cc4a62d4 100644 --- a/compair/tests/api/test_file.py +++ b/compair/tests/api/test_file.py @@ -113,6 +113,77 @@ 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): + url = self.base_url + '/report/test_report.csv' + + # 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) + + # TA can access reports + 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 can access reports + 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) + def test_create_attachment(self): url = '/api/attachment' test_formats = [ From dd8d69d88681972ad2064994d0c6cef4823dee8d Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 17:47:39 -0700 Subject: [PATCH 4/7] harden report filenames with course UUID --- compair/api/__init__.py | 2 ++ compair/api/report.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 7ec68dae..7393da7e 100644 --- a/compair/api/__init__.py +++ b/compair/api/__init__.py @@ -265,6 +265,8 @@ def file_retrieve(file_type, file_name): return redirect(kaltura_url) elif file_type == 'report': + # TODO: does not verify the user has access to the specific course that generated + # the report. Report filenames include the course UUID to limit guessability. user = current_user._get_current_object() is_admin = user.system_role == SystemRole.sys_admin is_instructor_or_ta = any( diff --git a/compair/api/report.py b/compair/api/report.py index bd5249e9..9f078919 100644 --- a/compair/api/report.py +++ b/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) From 97daab3c24b15043a5edf6e0ac20fda1d0b2b545 Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 17:54:09 -0700 Subject: [PATCH 5/7] add sys admin test for report access --- compair/api/__init__.py | 1 - compair/tests/api/test_file.py | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 7393da7e..16dae230 100644 --- a/compair/api/__init__.py +++ b/compair/api/__init__.py @@ -272,7 +272,6 @@ def file_retrieve(file_type, file_name): is_instructor_or_ta = any( uc.course_role in (CourseRole.instructor, CourseRole.teaching_assistant) for uc in user.user_courses - if uc.course_role != CourseRole.dropped ) if not is_admin and not is_instructor_or_ta: abort(403, title="Report Unavailable", diff --git a/compair/tests/api/test_file.py b/compair/tests/api/test_file.py index cc4a62d4..7d542efb 100644 --- a/compair/tests/api/test_file.py +++ b/compair/tests/api/test_file.py @@ -184,6 +184,14 @@ def test_view_report(self): rv = self.client.get(url) self.assert200(rv) + # sys admin can access reports + 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 = [ From be7060bf4a92304a82ad33b64c4e05a599ae4c5b Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 19:41:31 -0700 Subject: [PATCH 6/7] fix comparison_criterion not persisted in SQLAlchemy 2.x The cascade_backrefs behavior was removed in SQLAlchemy 2.0, so setting comparison=comparison in the ComparisonCriterion constructor no longer implicitly adds the child to the session. Explicitly add comparison_criterion instead of the already-tracked comparison. --- compair/models/comparison.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compair/models/comparison.py b/compair/models/comparison.py index 9fe4c1c5..fa549433 100644 --- a/compair/models/comparison.py +++ b/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 From 1f26c1bac71ce6eb59a8514a69d56d97795e0ece Mon Sep 17 00:00:00 2001 From: catherine Date: Wed, 13 May 2026 20:40:56 -0700 Subject: [PATCH 7/7] fix report access to verify enrollment in the specific course --- compair/api/__init__.py | 28 ++++++++++++++++++---------- compair/tests/api/test_file.py | 20 ++++++++++++++++---- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 16dae230..72da64de 100644 --- a/compair/api/__init__.py +++ b/compair/api/__init__.py @@ -12,7 +12,7 @@ from bouncer.constants import READ from compair.authorization import require from compair.kaltura import KalturaAPI -from compair.models import File, SystemRole, CourseRole +from compair.models import File, SystemRole, CourseRole, Course from compair.core import event, abort on_get_file = event.signal('GET_FILE') @@ -265,17 +265,25 @@ def file_retrieve(file_type, file_name): return redirect(kaltura_url) elif file_type == 'report': - # TODO: does not verify the user has access to the specific course that generated - # the report. Report filenames include the course UUID to limit guessability. user = current_user._get_current_object() is_admin = user.system_role == SystemRole.sys_admin - is_instructor_or_ta = any( - uc.course_role in (CourseRole.instructor, CourseRole.teaching_assistant) - for uc in user.user_courses - ) - if not is_admin and not is_instructor_or_ta: - abort(403, title="Report Unavailable", - message="Sorry, your role does not allow you to view reports.") + + 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) diff --git a/compair/tests/api/test_file.py b/compair/tests/api/test_file.py index 7d542efb..64fd376a 100644 --- a/compair/tests/api/test_file.py +++ b/compair/tests/api/test_file.py @@ -159,7 +159,9 @@ def test_view_attachment_linked_to_answer(self): self.assert200(rv) def test_view_report(self): - url = self.base_url + '/report/test_report.csv' + 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) @@ -170,21 +172,31 @@ def test_view_report(self): rv = self.client.get(url) self.assert403(rv) - # TA can access reports + # 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 can access reports + # 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 reports + # 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")):