diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 63ffd741..72da64de 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, 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 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) 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/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 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..64fd376a 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): @@ -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 = [ 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