diff --git a/charts/prod-ohstaff-values.yaml b/charts/prod-ohstaff-values.yaml
index a6e55ca..0540bb7 100644
--- a/charts/prod-ohstaff-values.yaml
+++ b/charts/prod-ohstaff-values.yaml
@@ -6,7 +6,7 @@ replicaCount: 1
image:
repository: uclalibrary/oral-history-staff-ui
- tag: v1.1.6
+ tag: v1.1.7
pullPolicy: Always
nameOverride: ""
diff --git a/oh_staff_ui/templates/oh_staff_ui/release_notes.html b/oh_staff_ui/templates/oh_staff_ui/release_notes.html
index 335cacd..7a153a0 100644
--- a/oh_staff_ui/templates/oh_staff_ui/release_notes.html
+++ b/oh_staff_ui/templates/oh_staff_ui/release_notes.html
@@ -4,6 +4,15 @@
Release Notes
+1.1.7
+May 28, 2024
+
+ - Fixed bug allowing staff with lower permissions to delete files if internal URLs were known.
+ - Fixed bug allowing download of master files by anonymous users if URLs were known.
+ - Updated gunicorn, pillow, and requests packages to patch security issues.
+ - Fixed issue where PDF/XML submaster files displayed before master files in "File Information" table.
+
+
1.1.6
May 22, 2024
diff --git a/oh_staff_ui/tests.py b/oh_staff_ui/tests.py
index 36eacd8..fb8a732 100644
--- a/oh_staff_ui/tests.py
+++ b/oh_staff_ui/tests.py
@@ -1,3 +1,4 @@
+from http import HTTPStatus
from shutil import rmtree
from lxml import etree
from pathlib import Path
@@ -7,8 +8,8 @@
from django.core.management.base import CommandError
from django.db import IntegrityError
from django.http import HttpRequest
-from django.test import SimpleTestCase, TestCase, override_settings, Client
-from django.contrib.auth.models import User
+from django.test import SimpleTestCase, TestCase, override_settings
+from django.contrib.auth.models import User, Group
from eulxml.xmlmap import load_xmlobject_from_string, mods
from oh_staff_ui.classes.GeneralFileHandler import GeneralFileHandler
from oh_staff_ui.classes.ImageFileHandler import ImageFileHandler
@@ -87,9 +88,6 @@ def setUpTestData(cls):
# Get mock request with generic user info for command-line processing.
cls.mock_request = HttpRequest()
cls.mock_request.user = User.objects.get(username=cls.user.username)
- # Add a Django client for testing web requests for media files.
- cls.client = Client()
- cls.client.force_login(user=cls.user)
def get_full_path(self, relative_path: str) -> Path:
# MediaFile.file.name contains a path relative to MEDIA_ROOT;
@@ -704,15 +702,28 @@ def test_file_url_static_thumbnail(self):
)
@override_settings(RUN_ENV="prod", DEBUG=False)
- def test_master_file_can_be_served_prod(self):
- # Confirm master files are web-accessible in production.
+ def test_file_is_served_prod(self):
+ # Confirm media files are web-accessible in production mode,
+ # where DEBUG=False.
master = self.create_master_general_file()
handler = GeneralFileHandler(master)
handler.process_files()
url = master.media_file.file_url
+ self.client.force_login(self.user)
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
+ def test_media_file_is_not_served_without_login(self):
+ # Confirm media files are not web-accessible to anonymous users.
+ master = self.create_master_general_file()
+ handler = GeneralFileHandler(master)
+ handler.process_files()
+ url = master.media_file.file_url
+ response = self.client.get(url)
+ # Login is required, so request is redirected
+ expected_url = f"/accounts/login/?next=/media/{master.media_file.file.name}"
+ self.assertRedirects(response, expected_url=expected_url)
+
def test_file_size_file_exists(self):
# Uses real file samples/sample.xml
file = self.create_master_general_file()
@@ -1725,7 +1736,7 @@ def setUpTestData(cls):
cls.mock_request = HttpRequest()
cls.mock_request.user = User.objects.get(username=cls.user.username)
- def create_master_and_derivatives(self, user, mock_request):
+ def create_master_and_derivatives(self, user, mock_request) -> ProjectItem:
item = ProjectItem.objects.create(
ark="fake/abcdef",
created_by=user,
@@ -1745,20 +1756,10 @@ def create_master_and_derivatives(self, user, mock_request):
handler = ImageFileHandler(master_image_file)
handler.process_files()
- # get created date from derivative images, so we can check if they are updated
- thumbnail_create_date = MediaFile.objects.get(
- item=item, file_type__file_code="image_thumbnail"
- ).create_date
- submaster_create_date = MediaFile.objects.get(
- item=item, file_type__file_code="image_submaster"
- ).create_date
-
- return item, thumbnail_create_date, submaster_create_date
+ return item
def test_delete_file_and_children(self):
- self.item, self.thumbnail_created_date, self.submaster_created_date = (
- self.create_master_and_derivatives(self.user, self.mock_request)
- )
+ self.item = self.create_master_and_derivatives(self.user, self.mock_request)
# check that the thumbnail and submaster images are created
self.assertTrue(MediaFile.objects.filter(item=self.item).count() == 3)
@@ -1787,24 +1788,7 @@ def test_delete_file_and_children(self):
self.assertFalse(submaster_path.is_file())
def test_delete_file_and_children_no_children(self):
- self.item = ProjectItem.objects.create(
- ark="fake/abcdef",
- created_by=self.user,
- last_modified_by=self.user,
- title="Fake title",
- type=ItemType.objects.get(type="Audio"),
- )
- file_type = MediaFileType.objects.get(file_code="image_master")
- master_image_file = OralHistoryFile(
- self.item.id,
- "samples/sample_marbles.tif",
- file_type,
- "master",
- self.mock_request,
- )
-
- handler = ImageFileHandler(master_image_file)
- handler.process_files()
+ self.item = self.create_master_and_derivatives(self.user, self.mock_request)
# check that the thumbnail and submaster images are created
self.assertTrue(MediaFile.objects.filter(item=self.item).count() == 3)
@@ -1812,7 +1796,7 @@ def test_delete_file_and_children_no_children(self):
submaster_mediafile = MediaFile.objects.get(
item=self.item, file_type__file_code="image_submaster"
)
- # delete files
+ # delete submaster only
delete_file_and_children(submaster_mediafile, self.user)
# check that master and thumbnail files are still there (in DB and on disk)
@@ -1828,6 +1812,33 @@ def test_delete_file_and_children_no_children(self):
)
self.assertFalse(submaster_path.is_file())
+ def test_unauthorized_user_cannot_delete_files_via_view(self):
+ self.item = self.create_master_and_derivatives(self.user, self.mock_request)
+ file_id = MediaFile.objects.last().id
+ url = f"/delete_file/{file_id}"
+ # Default user created in setUpTestData() does not have the necessary permission.
+ self.client.force_login(self.user)
+ response = self.client.get(url)
+ self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN)
+
+ def test_authorized_user_can_delete_files_via_view(self):
+ self.item = self.create_master_and_derivatives(self.user, self.mock_request)
+ file_id = MediaFile.objects.last().id
+ url = f"/delete_file/{file_id}"
+ # Default user created in setUpTestData() does not have the necessary permission,
+ # so create new one and the necessary group.
+ auth_user = User.objects.create_user("auth_tester")
+ authorized_group, created = Group.objects.get_or_create(
+ name="Oral History Staff"
+ )
+ auth_user.groups.add(authorized_group)
+ self.client.force_login(auth_user)
+ # Successful deletion redirects to the upload_file view of the associated
+ # project item.
+ expected_url = f"/upload_file/{self.item.id}"
+ response = self.client.get(url)
+ self.assertRedirects(response, expected_url=expected_url)
+
def tearDown(self) -> None:
media_files = MediaFile.objects.filter(item=self.item)
# delete files on disk first
diff --git a/oh_staff_ui/urls.py b/oh_staff_ui/urls.py
index f56afa0..4ccfab0 100644
--- a/oh_staff_ui/urls.py
+++ b/oh_staff_ui/urls.py
@@ -1,5 +1,3 @@
-from django.conf import settings
-from django.views.static import serve
from django.urls import path, re_path
from . import views
@@ -20,8 +18,8 @@
path("delete_file/", views.delete_file, name="delete_file"),
path("order_files/", views.order_files, name="order_files"),
path("browse/", views.browse, name="browse"),
- # Allow access to download media files via built-in view django.views.static.serve()
- re_path(r"^media/(?P.*)$", serve, {"document_root": settings.MEDIA_ROOT}),
+ # Allow staff access to download media files
+ re_path(r"^media/(?P.*)$", views.serve_media_file, name="serve_media_file"),
# To follow OAI practice, a query parameter combination is requred of either:
# verb=GetRecord and identifier={ark_value}
# verb=ListRecords
diff --git a/oh_staff_ui/views.py b/oh_staff_ui/views.py
index 61b5193..64eaeff 100644
--- a/oh_staff_ui/views.py
+++ b/oh_staff_ui/views.py
@@ -1,12 +1,15 @@
import logging
from threading import Thread
from requests.exceptions import HTTPError
+from django.conf import settings
from django.contrib import messages
+from django.core.exceptions import PermissionDenied
from django.core.management.base import CommandError
from django.shortcuts import redirect, render
from django.contrib.auth.decorators import login_required
from django.http.request import HttpRequest # for code completion
from django.http.response import HttpResponse # for code completion
+from django.views.static import serve
from oh_staff_ui.forms import (
FileUploadForm,
ProjectItemForm,
@@ -134,8 +137,10 @@ def show_log(request, line_count: int = 200) -> HttpResponse:
@login_required
def upload_file(request: HttpRequest, item_id: int) -> HttpResponse:
item = ProjectItem.objects.get(pk=item_id)
+ # Sort for display: file_code works for images & audio, file(name)
+ # breaks the tie for pdf & text, since oh_masters -> oh_submasters.
files = MediaFile.objects.filter(item=item).order_by(
- "sequence", "file_type__file_code"
+ "sequence", "file_type__file_code", "file"
)
# add "children" attribute to each file to hold its derivatives
# this is used in the template to display child files before deletion
@@ -195,8 +200,25 @@ def upload_file(request: HttpRequest, item_id: int) -> HttpResponse:
def delete_file(request: HttpRequest, file_id: int) -> HttpResponse:
media_file = MediaFile.objects.get(pk=file_id)
item_id = media_file.item.pk
- delete_file_and_children(media_file, request.user)
- return redirect("upload_file", item_id=item_id)
+ # Ensure only authorized users can execute this.
+ if user_in_oh_staff_group(request.user):
+ delete_file_and_children(media_file, request.user)
+ return redirect("upload_file", item_id=item_id)
+ else:
+ logger.warning(
+ f"Unauthorized attempt to delete {media_file.file} ({file_id}) by {request.user}"
+ )
+ raise PermissionDenied
+
+
+@login_required
+def serve_media_file(request: HttpRequest, path: str) -> HttpResponse:
+ # Wrap django.views.static.serve view, which normally is in urls.py,
+ # so that login requirement is enforced.
+ # The path parameter comes from re_path in urls.py.
+ # django.views.static.serve is a view, so returns HttpResponse already;
+ # we just pass it on.
+ return serve(request, path=path, document_root=settings.MEDIA_ROOT)
@login_required
diff --git a/requirements.txt b/requirements.txt
index 74f556c..b260c1f 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,10 +1,10 @@
Django == 4.2.11
psycopg2 == 2.9.5
-gunicorn == 20.1.0
+gunicorn == 22.0.0
whitenoise == 6.0.0
-requests == 2.31.0
-pillow == 10.2.0
+requests == 2.32.2
+pillow == 10.3.0
ffmpeg-python == 0.2.0
eulxml == 1.1.3
ply == 3.11
-django-bootstrap5 == 23.1
\ No newline at end of file
+django-bootstrap5 == 23.1