Address review comments

Author: codingjoe

SECURITY.md

@@ -12,7 +12,8 @@ We use [pre-signed POST URLs](s3-pre-signed-url) to upload files to AWS S3.
 it before fetching files from S3.
 
 Please note, that Django's signer uses the `SECRET_KEY`, rotating the key will void all
-signatures.
+signatures. Should you rotate the secret key, between a form GET and POST request, the
+form will fail. Similarly, Django will expire all sessions if you rotate the key.
 
 [s3-pre-signed-url]: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/s3-presigned-urls.html
 [django-signing]: https://docs.djangoproject.com/en/stable/topics/signing/

s3file/forms.py

@@ -8,7 +8,7 @@
 from storages.utils import safe_join
 
 from s3file.middleware import S3FileMiddleware
-from s3file.storages import storage
+from s3file.storages import get_aws_location, storage
 
 logger = logging.getLogger("s3file")
 
@@ -18,7 +18,7 @@ class S3FileInputMixin:
 
     needs_multipart_form = False
     upload_path = safe_join(
-        str(storage.aws_location),
+        str(get_aws_location()),
         str(
             getattr(
                 settings, "S3FILE_UPLOAD_PATH", pathlib.PurePosixPath("tmp", "s3file")

s3file/middleware.py

@@ -6,7 +6,7 @@
 from django.utils.crypto import constant_time_compare
 
 from . import views
-from .storages import local_dev, storage
+from .storages import get_aws_location, local_dev, storage
 
 logger = logging.getLogger("s3file")
 
@@ -40,10 +40,7 @@ def __call__(self, request):
     @classmethod
     def get_files_from_storage(cls, paths, signature):
         """Return S3 file where the name does not include the path."""
-        try:
-            location = storage.aws_location
-        except AttributeError:
-            location = storage.location
+        location = get_aws_location()
         for path in paths:
             path = pathlib.PurePosixPath(path)
             if not constant_time_compare(
@@ -54,7 +51,7 @@ def get_files_from_storage(cls, paths, signature):
                 relative_path = str(path.relative_to(location))
             except ValueError as e:
                 raise SuspiciousFileOperation(
-                    f"Path is not inside the designated upload location: {path}"
+                    f"Path is outside the storage location: {path}"
                 ) from e
 
             try:

s3file/storages.py

@@ -10,13 +10,9 @@
 
 
 class S3MockStorage(FileSystemStorage):
-    @property
-    def aws_location(self):
-        return getattr(settings, "AWS_LOCATION", "")
-
     @property
     def location(self):
-        return safe_join(os.path.abspath(self.base_location), self.aws_location)
+        return safe_join(os.path.abspath(self.base_location), get_aws_location())
 
     class connection:
         class meta:
@@ -56,3 +52,7 @@ class bucket:
 local_dev = isinstance(default_storage, FileSystemStorage)
 
 storage = default_storage if not local_dev else S3MockStorage()
+
+
+def get_aws_location():
+    return getattr(settings, "AWS_LOCATION", "")

tests/conftest.py

@@ -7,7 +7,7 @@
 from selenium import webdriver
 from selenium.common.exceptions import WebDriverException
 
-from s3file.storages import storage
+from s3file.storages import get_aws_location
 
 
 @pytest.fixture(scope="session")
@@ -26,7 +26,7 @@ def driver():
 @pytest.fixture
 def freeze_upload_folder(monkeypatch):
     """Freeze the upload folder which by default contains a random UUID v4."""
-    upload_folder = Path(storage.aws_location) / "tmp" / "s3file"
+    upload_folder = Path(get_aws_location()) / "tmp" / "s3file"
     monkeypatch.setattr(
         "s3file.forms.S3FileInputMixin.upload_folder",
         str(upload_folder),

tests/test_middleware.py

@@ -6,7 +6,7 @@
 from django.core.files.uploadedfile import SimpleUploadedFile
 
 from s3file.middleware import S3FileMiddleware
-from s3file.storages import storage
+from s3file.storages import get_aws_location, storage
 
 
 class TestS3FileMiddleware:
@@ -16,7 +16,7 @@ def test_get_files_from_storage(self, freeze_upload_folder):
             "tmp/s3file/test_get_files_from_storage", ContentFile(content)
         )
         files = S3FileMiddleware.get_files_from_storage(
-            [os.path.join(storage.aws_location, name)],
+            [os.path.join(get_aws_location(), name)],
             "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
         )
         file = next(files)