diff --git a/OpenOversight/app/main/model_view.py b/OpenOversight/app/main/model_view.py index db52c6379..9fe6008e4 100644 --- a/OpenOversight/app/main/model_view.py +++ b/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: diff --git a/OpenOversight/app/main/views.py b/OpenOversight/app/main/views.py index 9970474f7..97f49999c 100644 --- a/OpenOversight/app/main/views.py +++ b/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: diff --git a/OpenOversight/app/models/config.py b/OpenOversight/app/models/config.py index ac58a7518..ee97fb5af 100644 --- a/OpenOversight/app/models/config.py +++ b/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"]) diff --git a/OpenOversight/app/utils/cloud.py b/OpenOversight/app/utils/cloud.py index 629cdc3e5..18a59aefe 100644 --- a/OpenOversight/app/utils/cloud.py +++ b/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, diff --git a/OpenOversight/app/utils/constants.py b/OpenOversight/app/utils/constants.py index 3f16f6f51..8787c01eb 100644 --- a/OpenOversight/app/utils/constants.py +++ b/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 diff --git a/OpenOversight/app/utils/general.py b/OpenOversight/app/utils/general.py index 72b222716..84ddcf44f 100644 --- a/OpenOversight/app/utils/general.py +++ b/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] ) diff --git a/OpenOversight/tests/conftest.py b/OpenOversight/tests/conftest.py index 854864bc9..948cb5b73 100644 --- a/OpenOversight/tests/conftest.py +++ b/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__)) diff --git a/OpenOversight/tests/images/415Cat.tiff b/OpenOversight/tests/images/415Cat.tiff new file mode 100644 index 000000000..12adde62e Binary files /dev/null and b/OpenOversight/tests/images/415Cat.tiff differ diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py index 29fa24ccb..3adaa6e70 100644 --- a/OpenOversight/tests/routes/test_officer_and_department.py +++ b/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): diff --git a/OpenOversight/tests/test_utils.py b/OpenOversight/tests/test_utils.py index 594b4da00..decfeb3bf 100644 --- a/OpenOversight/tests/test_utils.py +++ b/OpenOversight/tests/test_utils.py @@ -1,6 +1,5 @@ from io import BytesIO -import PIL import pytest from flask import current_app from flask_login import current_user @@ -10,8 +9,8 @@ from OpenOversight.app.utils.cloud import ( compute_hash, crop_image, - upload_image_to_s3_and_store_in_db, - upload_obj_to_s3, + save_image_to_s3_and_db, + upload_file_to_s3, ) from OpenOversight.app.utils.db import unit_choices from OpenOversight.app.utils.forms import filter_by_form, grab_officers @@ -21,7 +20,7 @@ # Utils tests upload_s3_patch = patch( - "OpenOversight.app.utils.cloud.upload_obj_to_s3", + "OpenOversight.app.utils.cloud.upload_file_to_s3", MagicMock(return_value="https://s3-some-bucket/someaddress.jpg"), ) @@ -164,24 +163,24 @@ def test_compute_hash(mockdata): assert hash_result == expected_hash -def test_s3_upload_png(mockdata, test_png_BytesIO): +def test_s3_upload_png(mockdata, test_png_bytes_io): mocked_connection = Mock() mocked_resource = Mock() with patch("boto3.client", Mock(return_value=mocked_connection)): with patch("boto3.resource", Mock(return_value=mocked_resource)): - upload_obj_to_s3(test_png_BytesIO, "test_cop1.png") + upload_file_to_s3(test_png_bytes_io, "test_cop1.png") assert ( mocked_connection.method_calls[0][2]["ExtraArgs"]["ContentType"] == "image/png" ) -def test_s3_upload_jpeg(mockdata, test_jpg_BytesIO): +def test_s3_upload_jpeg(mockdata, test_jpg_bytes_io): mocked_connection = Mock() mocked_resource = Mock() with patch("boto3.client", Mock(return_value=mocked_connection)): with patch("boto3.resource", Mock(return_value=mocked_resource)): - upload_obj_to_s3(test_jpg_BytesIO, "test_cop5.jpg") + upload_file_to_s3(test_jpg_bytes_io, "test_cop5.jpg") assert ( mocked_connection.method_calls[0][2]["ExtraArgs"]["ContentType"] == "image/jpeg" @@ -215,89 +214,88 @@ def test_unit_choices(mockdata): @upload_s3_patch -def test_upload_image_to_s3_and_store_in_db_increases_images_in_db( - mockdata, test_png_BytesIO, client +def test_save_image_to_s3_and_db_increases_images_in_db( + mockdata, test_png_bytes_io, client ): original_image_count = Image.query.count() - upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) + save_image_to_s3_and_db(test_png_bytes_io, 1, 1) assert Image.query.count() == original_image_count + 1 @upload_s3_patch -def test_upload_existing_image_to_s3_and_store_in_db_returns_existing_image( - mockdata, test_png_BytesIO, client +def test_save_image_to_s3_and_db_returns_existing_image( + mockdata, test_png_bytes_io, client ): # Disable file closing for this test - test_png_BytesIO.close = lambda: None - firstUpload = upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) - secondUpload = upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) + test_png_bytes_io.close = lambda: None + firstUpload = save_image_to_s3_and_db(test_png_bytes_io, 1, 1) + secondUpload = save_image_to_s3_and_db(test_png_bytes_io, 1, 1) assert type(secondUpload) == Image assert firstUpload.id == secondUpload.id @upload_s3_patch -def test_upload_image_to_s3_and_store_in_db_does_not_set_tagged( - mockdata, test_png_BytesIO, client +def test_save_image_to_s3_and_db_does_not_set_tagged( + mockdata, test_png_bytes_io, client ): - upload = upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) + upload = save_image_to_s3_and_db(test_png_bytes_io, 1, 1) assert not upload.is_tagged @patch( - "OpenOversight.app.utils.cloud.upload_obj_to_s3", + "OpenOversight.app.utils.cloud.upload_file_to_s3", MagicMock(return_value="https://s3-some-bucket/someaddress.jpg"), ) -def test_upload_image_to_s3_and_store_in_db_saves_filename_in_correct_format( - mockdata, test_png_BytesIO, client +def test_save_image_to_s3_and_db_saves_filename_in_correct_format( + mockdata, test_png_bytes_io, client ): mocked_connection = Mock() mocked_resource = Mock() with patch("boto3.client", Mock(return_value=mocked_connection)): with patch("boto3.resource", Mock(return_value=mocked_resource)): - upload = upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) + upload = save_image_to_s3_and_db(test_png_bytes_io, 1, 1) filename = upload.filepath.split("/")[-1] filename_parts = filename.split(".") assert len(filename_parts) == 2 -def test_upload_image_to_s3_and_store_in_db_throws_exception_for_unrecognized_format( - mockdata, client +def test_save_image_to_s3_and_db_invalid_image(mockdata, client): + with pytest.raises(ValueError): + save_image_to_s3_and_db(BytesIO(b"invalid-image"), 1, 1) + + +def test_save_image_to_s3_and_db_unrecognized_format( + mockdata, test_tiff_bytes_io, client ): - with pytest.raises(PIL.UnidentifiedImageError): - upload_image_to_s3_and_store_in_db(BytesIO(b"invalid-image"), 1, 1) + with pytest.raises(ValueError): + save_image_to_s3_and_db(test_tiff_bytes_io, 1, 1) @patch( - "OpenOversight.app.utils.cloud.upload_obj_to_s3", + "OpenOversight.app.utils.cloud.upload_file_to_s3", MagicMock(return_value="https://s3-some-bucket/someaddress.jpg"), ) -def test_upload_image_to_s3_and_store_in_db_does_not_throw_exception_for_recognized_format( - mockdata, test_png_BytesIO, client -): +def test_save_image_to_s3_and_db_recognized_format(mockdata, test_png_bytes_io, client): try: - upload_image_to_s3_and_store_in_db(test_png_BytesIO, 1, 1) + save_image_to_s3_and_db(test_png_bytes_io, 1, 1) except ValueError: pytest.fail("Unexpected value error") -def test_crop_image_calls_upload_image_to_s3_and_store_in_db_with_user_id( - mockdata, client -): +def test_crop_image_calls_save_image_to_s3_and_db_with_user_id(mockdata, client): with current_app.test_request_context(): login_user(client) department = Department.query.first() image = Image.query.first() with patch( - "OpenOversight.app.utils.cloud.upload_image_to_s3_and_store_in_db" - ) as upload_image_to_s3_and_store_in_db: + "OpenOversight.app.utils.cloud.save_image_to_s3_and_db" + ) as save_image_to_s3_and_db: crop_image(image, None, department.id) - assert ( - current_user.get_id() in upload_image_to_s3_and_store_in_db.call_args[0] - ) + assert current_user.get_id() in save_image_to_s3_and_db.call_args[0] @pytest.mark.parametrize(