Address bug in S3 uploading logic (#1022)

OpenOversight/app/main/model_view.py

@@ -39,10 +39,7 @@ class ModelView(MethodView):
 
     def get(self, obj_id):
         if obj_id is None:
-            if request.args.get("page"):
-                page = int(request.args.get("page"))
-            else:
-                page = 1
+            page = int(request.args.get("page", 1))
 
             if self.order_by:
                 if not self.descending:

OpenOversight/app/main/views.py

@@ -78,7 +78,7 @@
 )
 from OpenOversight.app.utils.auth import ac_or_admin_required, admin_required
 from OpenOversight.app.utils.choices import AGE_CHOICES, GENDER_CHOICES, RACE_CHOICES
-from OpenOversight.app.utils.cloud import crop_image, upload_image_to_s3_and_store_in_db
+from OpenOversight.app.utils.cloud import crop_image, save_image_to_s3_and_db
 from OpenOversight.app.utils.constants import (
     ENCODING_UTF_8,
     FLASH_MSG_PERMANENT_REDIRECT,
@@ -1882,7 +1882,7 @@ def upload(department_id: int = 0, officer_id: int = 0):
         )
 
     try:
-        image = upload_image_to_s3_and_store_in_db(
+        image = save_image_to_s3_and_db(
             file_to_upload, current_user.get_id(), department_id=department_id
         )
     except ValueError:

OpenOversight/app/models/config.py

@@ -8,6 +8,7 @@
     KEY_ENV_TESTING,
     KEY_OFFICERS_PER_PAGE,
     KEY_OO_MAIL_SUBJECT_PREFIX,
+    KEY_S3_BUCKET_NAME,
     KEY_TIMEZONE,
     MEGABYTE,
 )
@@ -56,7 +57,7 @@ def __init__(self):
         self.AWS_ACCESS_KEY_ID = os.environ.get("AWS_ACCESS_KEY_ID")
         self.AWS_DEFAULT_REGION = os.environ.get("AWS_DEFAULT_REGION")
         self.AWS_SECRET_ACCESS_KEY = os.environ.get("AWS_SECRET_ACCESS_KEY")
-        self.S3_BUCKET_NAME = os.environ.get("S3_BUCKET_NAME")
+        self.S3_BUCKET_NAME = os.environ.get(KEY_S3_BUCKET_NAME)
 
         # Upload Settings
         self.ALLOWED_EXTENSIONS = set(["jpeg", "jpg", "jpe", "png", "gif", "webp"])

OpenOversight/app/utils/cloud.py

@@ -12,9 +12,11 @@
 from flask import current_app
 from flask_login import current_user
 from PIL import Image as Pimage
+from PIL import UnidentifiedImageError
 from PIL.PngImagePlugin import PngImageFile
 
 from OpenOversight.app.models.database import Image, db
+from OpenOversight.app.utils.constants import KEY_ALLOWED_EXTENSIONS, KEY_S3_BUCKET_NAME
 
 
 def compute_hash(data_to_hash):
@@ -57,26 +59,25 @@ def crop_image(image, crop_data=None, department_id=None):
     cropped_image_buf = BytesIO()
     pimage.save(cropped_image_buf, "jpeg", quality=95, optimize=True, progressive=True)
 
-    return upload_image_to_s3_and_store_in_db(
+    return save_image_to_s3_and_db(
         cropped_image_buf, current_user.get_id(), department_id
     )
 
 
-def find_date_taken(pimage):
+# 36867 in the exif tags holds the date and the original image was taken
+# https://www.awaresystems.be/imaging/tiff/tifftags/privateifd/exif.html
+EXIF_KEY_DATE_TIME_ORIGINAL = 36867
+
+
+def get_date_taken(pimage):
     if isinstance(pimage, PngImageFile):
         return None
 
     exif = hasattr(pimage, "_getexif") and pimage._getexif()
-    if exif:
-        # 36867 in the exif tags holds the date and the original image was taken
-        # https://www.awaresystems.be/imaging/tiff/tifftags/privateifd/exif.html
-        if 36867 in exif:
-            return exif[36867]
-    else:
-        return None
+    return exif.get(EXIF_KEY_DATE_TIME_ORIGINAL, None) if exif else None
 
 
-def upload_obj_to_s3(file_obj, dest_filename):
+def upload_file_to_s3(file_obj, dest_filename: str):
     s3_client = boto3.client("s3")
 
     # Folder to store files in on S3 is first two chars of dest_filename
@@ -88,7 +89,7 @@ def upload_obj_to_s3(file_obj, dest_filename):
     s3_path = f"{s3_folder}/{s3_filename}"
     s3_client.upload_fileobj(
         file_obj,
-        current_app.config["S3_BUCKET_NAME"],
+        current_app.config[KEY_S3_BUCKET_NAME],
         s3_path,
         ExtraArgs={"ContentType": s3_content_type, "ACL": "public-read"},
     )
@@ -97,26 +98,29 @@ def upload_obj_to_s3(file_obj, dest_filename):
     config.signature_version = botocore.UNSIGNED
     url = boto3.resource("s3", config=config).meta.client.generate_presigned_url(
         "get_object",
-        Params={"Bucket": current_app.config["S3_BUCKET_NAME"], "Key": s3_path},
+        Params={"Bucket": current_app.config[KEY_S3_BUCKET_NAME], "Key": s3_path},
     )
 
     return url
 
 
-def upload_image_to_s3_and_store_in_db(image_buf, user_id, department_id=None):
+def save_image_to_s3_and_db(image_buf, user_id, department_id=None):
     """
     Just a quick explanation of the order of operations here...
     we have to scrub the image before we do anything else like hash it,
     but we also have to get the date for the image before we scrub it.
     """
     image_buf.seek(0)
-    pimage = Pimage.open(image_buf)
+    try:
+        pimage = Pimage.open(image_buf)
+    except UnidentifiedImageError:
+        raise ValueError("Attempted to pass an invalid image.")
     image_format = pimage.format.lower()
-    if image_format not in current_app.config["ALLOWED_EXTENSIONS"]:
+    if image_format not in current_app.config[KEY_ALLOWED_EXTENSIONS]:
         raise ValueError(f"Attempted to pass invalid data type: {image_format}")
     image_buf.seek(0)
 
-    date_taken = find_date_taken(pimage)
+    date_taken = get_date_taken(pimage)
     if date_taken:
         date_taken = datetime.datetime.strptime(date_taken, "%Y:%m:%d %H:%M:%S")
     pimage.getexif().clear()
@@ -132,7 +136,7 @@ def upload_image_to_s3_and_store_in_db(image_buf, user_id, department_id=None):
     try:
         new_filename = f"{hash_img}.{image_format}"
         scrubbed_image_buf.seek(0)
-        url = upload_obj_to_s3(scrubbed_image_buf, new_filename)
+        url = upload_file_to_s3(scrubbed_image_buf, new_filename)
         new_image = Image(
             filepath=url,
             hash_img=hash_img,

OpenOversight/app/utils/constants.py

@@ -13,6 +13,7 @@
 KEY_DEPT_TOTAL_OFFICERS = "total_department_officers"
 
 # Config Key Constants
+KEY_ALLOWED_EXTENSIONS = "ALLOWED_EXTENSIONS"
 KEY_DATABASE_URI = "SQLALCHEMY_DATABASE_URI"
 KEY_ENV = "ENV"
 KEY_ENV_DEV = "development"
@@ -21,6 +22,7 @@
 KEY_NUM_OFFICERS = "NUM_OFFICERS"
 KEY_OFFICERS_PER_PAGE = "OFFICERS_PER_PAGE"
 KEY_OO_MAIL_SUBJECT_PREFIX = "OO_MAIL_SUBJECT_PREFIX"
+KEY_S3_BUCKET_NAME = "S3_BUCKET_NAME"
 KEY_TIMEZONE = "TIMEZONE"
 
 # Flash Message Constants

OpenOversight/app/utils/general.py

@@ -6,6 +6,8 @@
 
 from flask import current_app, url_for
 
+from OpenOversight.app.utils.constants import KEY_ALLOWED_EXTENSIONS
+
 
 def ac_can_edit_officer(officer, ac):
     if officer.department_id == ac.ac_department_id:
@@ -17,7 +19,7 @@ def allowed_file(filename):
     return (
         "." in filename
         and filename.rsplit(".", 1)[1].lower()
-        in current_app.config["ALLOWED_EXTENSIONS"]
+        in current_app.config[KEY_ALLOWED_EXTENSIONS]
     )
 
 

OpenOversight/tests/conftest.py

@@ -330,7 +330,7 @@ def session(db):
 
 
 @pytest.fixture
-def test_png_BytesIO():
+def test_png_bytes_io():
     test_dir = os.path.dirname(os.path.realpath(__file__))
     local_path = os.path.join(test_dir, "images/204Cat.png")
     img = Pimage.open(local_path)
@@ -342,7 +342,7 @@ def test_png_BytesIO():
 
 
 @pytest.fixture
-def test_jpg_BytesIO():
+def test_jpg_bytes_io():
     test_dir = os.path.dirname(os.path.realpath(__file__))
     local_path = os.path.join(test_dir, "images/200Cat.jpeg")
     img = Pimage.open(local_path)
@@ -353,6 +353,18 @@ def test_jpg_BytesIO():
     return byte_io
 
 
+@pytest.fixture
+def test_tiff_bytes_io():
+    test_dir = os.path.dirname(os.path.realpath(__file__))
+    local_path = os.path.join(test_dir, "images/415Cat.tiff")
+    img = Pimage.open(local_path)
+
+    byte_io = BytesIO()
+    img.save(byte_io, img.format)
+    byte_io.seek(0)
+    return byte_io
+
+
 @pytest.fixture
 def test_csv_dir():
     test_dir = os.path.dirname(os.path.realpath(__file__))

OpenOversight/tests/routes/test_officer_and_department.py

@@ -2028,13 +2028,13 @@ def test_find_officer_redirect(client, mockdata, session):
 
 
 def test_admin_can_upload_photos_of_dept_officers(
-    mockdata, client, session, test_jpg_BytesIO
+    mockdata, client, session, test_jpg_bytes_io
 ):
     with current_app.test_request_context():
         login_admin(client)
 
         data = dict(
-            file=(test_jpg_BytesIO, "204Cat.png"),
+            file=(test_jpg_bytes_io, "204Cat.png"),
         )
 
         department = Department.query.filter_by(id=AC_DEPT).first()
@@ -2050,7 +2050,7 @@ def test_admin_can_upload_photos_of_dept_officers(
         crop_mock = MagicMock(return_value=image)
         upload_mock = MagicMock(return_value=image)
         with patch(
-            "OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db",
+            "OpenOversight.app.main.views.save_image_to_s3_and_db",
             upload_mock,
         ):
             with patch("OpenOversight.app.main.views.crop_image", crop_mock):
@@ -2070,22 +2070,20 @@ def test_admin_can_upload_photos_of_dept_officers(
 
 
 def test_upload_photo_sends_500_on_s3_error(
-    mockdata, client, session, test_png_BytesIO
+    mockdata, client, session, test_png_bytes_io
 ):
     with current_app.test_request_context():
         login_admin(client)
 
         data = dict(
-            file=(test_png_BytesIO, "204Cat.png"),
+            file=(test_png_bytes_io, "204Cat.png"),
         )
 
         department = Department.query.filter_by(id=AC_DEPT).first()
         mock = MagicMock(return_value=None)
         officer = department.officers[0]
         officer_face_count = len(officer.face)
-        with patch(
-            "OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db", mock
-        ):
+        with patch("OpenOversight.app.main.views.save_image_to_s3_and_db", mock):
             rv = client.post(
                 url_for(
                     "main.upload", department_id=department.id, officer_id=officer.id
@@ -2138,12 +2136,12 @@ def test_user_cannot_upload_officer_photo(mockdata, client, session):
 
 
 def test_ac_can_upload_photos_of_dept_officers(
-    mockdata, client, session, test_png_BytesIO
+    mockdata, client, session, test_png_bytes_io
 ):
     with current_app.test_request_context():
         login_ac(client)
         data = dict(
-            file=(test_png_BytesIO, "204Cat.png"),
+            file=(test_png_bytes_io, "204Cat.png"),
         )
         department = Department.query.filter_by(id=AC_DEPT).first()
         officer = department.officers[4]
@@ -2158,7 +2156,7 @@ def test_ac_can_upload_photos_of_dept_officers(
         crop_mock = MagicMock(return_value=image)
         upload_mock = MagicMock(return_value=image)
         with patch(
-            "OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db",
+            "OpenOversight.app.main.views.save_image_to_s3_and_db",
             upload_mock,
         ):
             with patch("OpenOversight.app.main.views.crop_image", crop_mock):